Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dont preserve mtime when uploading trough the web interface #3822

Merged
merged 1 commit into from
Mar 13, 2017

Conversation

icewind1991
Copy link
Member

While mtime preservation is expected behaviour when using the sync client I think preserving mtime while uploading trough the web interface is not expected.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Mar 13, 2017
@icewind1991 icewind1991 added this to the Nextcloud 12.0 milestone Mar 13, 2017
@mention-bot
Copy link

@icewind1991, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schiessle, @butonic and @luckydonald to be potential reviewers.

Signed-off-by: Robin Appelman <robin@icewind.nl>
@codecov-io
Copy link

Codecov Report

Merging #3822 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3822   +/-   ##
=========================================
  Coverage     54.29%   54.29%           
  Complexity    21053    21053           
=========================================
  Files          1302     1302           
  Lines         80280    80280           
  Branches       1254     1254           
=========================================
  Hits          43586    43586           
  Misses        36694    36694

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4395cf4...ad4b5c8. Read the comment docs.

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I do second this. Just had a customer last week complaining about this behaviour as well :)

@icewind1991 icewind1991 merged commit c1cf872 into master Mar 13, 2017
@icewind1991 icewind1991 deleted the webui-upoad-no-mtime branch March 13, 2017 14:49
@luckydonald
Copy link
Contributor

Please don't bump me.

@nickvergessen
Copy link
Member

@luckydonald sorry, you already saw that mention bot ignores the ignore list.
If you don't want to be bothered anymore, please make use of this github feature:

bildschirmfoto vom 2017-03-23 17-09-34

Copy link

@wowangus wowangus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that not preserving mtime might very likely will affect the integrity of the data being uploaded and affect the truth of last modification timestamp of the file.

there should not be any differences between uploading through web interface, or desktop sync, or mobile apps. mtime should always be kept for sack of telling the truth about when is it eventually modified even you revisit and review a file 1/3/5/10/15 years later.

@wowangus
Copy link

@icewind1991 @nickvergessen Please reconsider the effect of not preserving mtime of a file being upload through the web ui. Preserving mtime is always expected when transferring a file from/to internal drive/flash drive/network drive/memory card/mobile phone.

Consider the following scenario:

  1. USER_A have a batch of document/image/video, taking photos as example in this case, that are taken/edited/modified 4~6 months ago.
  2. USER_A recently uploads those photos through web ui to Nextcloud.
  3. USER_A tries to sort uploaded photos by their last modified date in web ui or mobile app or sync client's folder, and expects that would behave similarly as how file explorer does with the original folder.
  4. USER_A however cannot get the expected sorting, instead those files are now sorted by their uncertain sequence of upload, because the mtime is not retained/preserved at/as of web-ui uploading, and mtime of those files are lost then replaced with their upload time (e.g. if connection is slow, upload time would take quite a while, probably like 09:00 for photo002, 09:01 for photo001, 09:02 for photo003).

Please reconsider the effect of not preserving mtime and consider reverting the merged changes.

I have also #3572 and nextcloud/files_retention#23 about the issue regarding file retention module. The issue there should be fixed by making use of "upload datetime timestamp" for the retention decision logic instead. That issue should not be fixed by abandoning mtime as of web-ui uploading.

Thanks for your attention!

@nickvergessen
Copy link
Member

I totally agree

@f11h
Copy link

f11h commented Sep 3, 2017

Is there any chance that this "feature" will be reverted to old behaviour?
Since we use NC12 my users are a little bit confused about the "faked changed times" (some of them are only using WEB-UI)

Currently I've reverted the corresponding js-file to the old version, but this isn't the way I should do it 😃

@wowangus
Copy link

wowangus commented Sep 4, 2017

Totally understood the confusion caused by the false "last-modified" timestamp.

The real last-modified timestamp of any particular uploaded files should always be retained throughout the journey and workflow between front-end and back-end.

Hope communities could see the issues and consequences of the behaviour and revert it.

@zestoi
Copy link

zestoi commented Sep 14, 2017

i just added back in the code that had been removed from apps/files/js/file-upload.js as i need the file modified timestamp not when it was uploaded. guess this isnt the best thing to do but works fine for me now.

@icewind1991
Copy link
Member Author

While I still maintain the opinion that the current behaviour makes the most sense as a default, I understand that the old behaviour has valid use cases.

To "solve" the problem I created a small app that reverts the behaviour to the pre-12 behaviour: https://github.com/icewind1991/files_upload_mtime

The app isn't the cleanest way to handle it (dynamically modifying core js...) but it at least removes the need to manually modify the js.

@f11h
Copy link

f11h commented Sep 17, 2017

Hi icewind1991, thanks a lot, this really helps me. I think this is the most elegant way we could do it :)

@Frogging101
Copy link

I think this behaviour should at least be made optional. I expect a file storage system to preserve metadata when I upload files to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants