Fix the NamedFile modified property

[davisagli]

The modified property was added in #149. But it's off by a factor of 1000. This leads to making the wrong decision about when to remove existing image scales.

The return value from the property is used to construct a DateTime, which interprets a numeric value as seconds.

self._p_time is in seconds but self._modified is in milliseconds, so needs to be divided by 1000.

[mister-roboto]

@davisagli thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[davisagli]

@jenkins-plone-org please run jobs

[mauritsvanrees]

self._p_time is in seconds but self._modified is in milliseconds, so needs to be divided by 1000.

_p_mtime is in micro seconds.

For a while I thought your PR fixed it the wrong way around, but I checked and it seems good. Checking in Plone coredev 6.1 with current plone.namedfile master branch and bin/instance debug:

>>> from DateTime import DateTime
>>> image = app.Plone["picture.jpeg"]
>>> image._p_mtime
1697616690.5691967
>>> image.modified()
DateTime('2023/10/18 10:11:30.555951 GMT+2')
>>> image.image
<plone.namedfile.file.NamedBlobImage object at 0x114f1d9b0 oid 0x26a9 in <ZODB.Connection.Connection object at 0x11174f310>>
>>> image.image._p_mtime
1697616677.6966476
>>> image.image.modified
1697616677644

So this is indeed off by a factor of 1000.

With your branch and without changing anything about the image:

>>> image.image.modified
1697616677.644
>>> image.image._p_mtime
1697616677.6966476

Oh. That actually seems good.
Replacing the image with a new one has the same rough outcome.

With the original code from plone.namedfile 6.1.2, there is no modified attribute on the image, only on the scales:

>>> scale = image.restrictedTraverse("images").scale(fieldname="image", width=100, height=100)
>>> scale.modified
1697617866409

So with the new code, the modified timestamp is still off by a factor of thousand, although at least modified and _p_mtime are in the same range.

Ah, but that has always been a difference between the modified timestamp of the scale and the _p_mtime of the named image file: the last one is time.time() and the scale passes this to DateTime and gets the millis.

Checking one last time with your branch:

>>> image.image.modified
1697617866.381
>>> image.image._p_mtime
1697617866.409041
>>> scale = image.restrictedTraverse("images").scale(fieldname="image", width=100, height=100)
>>> scale.modified
1697617866381

Conclusion: yes, this is fine now. :-)
Thanks!

[mauritsvanrees]

Can be merged, but I wait a bit, because I think mr.roboto has a problem.
@gforcada, if you want to try out a fix and see if it works, you can merge this PR. :-)

[sneridagh]

@mauritsvanrees thanks! Although might seem not important, we have been already biten by this. Could we have a release asap? Thanks!

[gforcada]

Works again: plone/buildout.coredev@d4e5027 🎉

Sorry for the mr.roboto breakage 😓

[sneridagh]

@gforcada thanks!

[maethu]

The modified property was added in #149. But it's off by a factor of 1000. This leads to making the wrong decision about when to remove existing image scales.

The return value from the property is used to construct a DateTime, which interprets a numeric value as seconds.

self._p_time is in seconds but self._modified is in milliseconds, so needs to be divided by 1000.

omg, I did not realize that :-( Thanks for fixing it!

[mauritsvanrees]

@mauritsvanrees thanks! Although might seem not important, we have been already biten by this. Could we have a release asap? Thanks!

@sneridagh I have released 6.2.2:
https://pypi.org/project/plone.namedfile/6.2.2/

I will update coredev after releasing some more (unrelated) packages).

[sneridagh]

@davisagli @mauritsvanrees I can confirm that it does work like a charm :)
Thanks!