Moodle
  1. Moodle
  2. MDL-39177

Overwriting files does not observe the "alias" attribute always...

    Details

    • Testing Instructions:
      Hide

      Sorry for the amount of tests but there is a lot of code duplication and I want to make sure that all options work.

      To test under 23_STABLE, 24_STABLE and 25_STABLE!

      Test 1. Basic overwrite a file

      1. Edit any page with filemanager (for example Private files)
      2. Add a file from repository (Server file, Dropbox, etc) as a copy
      3. Add the same file again but as a shortcut
      4. Make sure file looks like shortcut
      5. Save file area and return to editing
      6. Make sure file is a shortcut

      Repeat the same test but save filearea after #2

      Test 2. Renaming and moving source file

      1. Add file to private files, create a shortcut to it from somewhere else (for example file resource on frontpage)
      2. Edit private files and rename the source file
      3. Make sure it no longer looks like a source
      4. Make sure the shortcut is converted to the copy

      Repeat the same with moving file to the folder or renaming containing folder

      Test 3. Overwrite vs Delete+replace

      1. Add five files to private files, create shortcuts to all of them from somewhere else
      2. Edit private files. Overwrite 1st file by uploading, overwrite 2nd file by picking it from any repository (as a copy), overwrite 3rd file using drag&drop.
      3. Remove the 4th file and upload another file with the same name as removed
      4. Rename the 5th file twice restoring original name
      5. Make sure first 3 files are still listed as sources and 4th & 5th are not
      6. Save file area and open it for editing again
      7. Make sure first 3 files are listed as sources and 4th & 5th are not
      8. Make sure shortcuts to the first 3 files are updated (new thumbnail, content is correct, ...)

      Test 4. Non-JS filemanager

      1. Disable Javascript
      2. Edit any form containing filearea (for example private files) and make sure basic operations work (look at moving and deleting the files). There is no support for file references and no possibility to overwrite a file in non-js filemanager.
      3. If something is not working as expected please make sure it is a regression from this issue before failing it

      Test 5. Attemp to create double reference

      1. Add a file to private files and create a shortcut to it from somewhere else
      2. Edit private files again, try to overwrite the source with a shortcut (like in Test 1 but with when file is a source of a shortcut)
      3. Make sure the popup appears and action is not possible
      4. Add another file to private files, save it and return to editing
      5. In another window create a shortcut to this file, save.
      6. Return to window with private files and overwrite the file with shortcut (no message appears because in this case we don't know that references exist)
      7. Save private files, make sure the exception is thrown and file is not substituted with shortcut

      Test 6. Unzipping

      1. Add archive file to the private files (must contain subfolders)
      2. Extract (unzip) archive file and make sure that files are extracted properly
      3. Save private files
      4. Create a shortcut to one of the extracted files from other filearea, save it
      5. Edit private files, overwrite the source file, save private files, make sure shortcut is updated
      6. Delete or modify some other extracted files, add more files to extracted subfolders, save private files
      7. Extract archive again and make sure everything is overwritten correctly, shortcut is still a shortcut and it is updated

      Note: there is no warning when files get overwritten during unzip. It is not related to this issue.

      Test 7. Overwriting file by another user

      1. Use any filearea where several users have access to (for example Folder resource in a course with several teachers)
      2. Upload a file as user1, save filearea (use the file here that has never previously been used)
      3. Open any textarea or filemanager, open filepicker and make sure user1 can see this file in his recent files
      4. Login as user2, upload (by overwriting) different file with the same name, save filearea
      5. Make sure user2 can see the new file in his recent files
      6. Make sure user1 can not see any of those two files in his recent files
      Show
      Sorry for the amount of tests but there is a lot of code duplication and I want to make sure that all options work. To test under 23_STABLE, 24_STABLE and 25_STABLE! Test 1. Basic overwrite a file Edit any page with filemanager (for example Private files) Add a file from repository (Server file, Dropbox, etc) as a copy Add the same file again but as a shortcut Make sure file looks like shortcut Save file area and return to editing Make sure file is a shortcut Repeat the same test but save filearea after #2 Test 2. Renaming and moving source file Add file to private files, create a shortcut to it from somewhere else (for example file resource on frontpage) Edit private files and rename the source file Make sure it no longer looks like a source Make sure the shortcut is converted to the copy Repeat the same with moving file to the folder or renaming containing folder Test 3. Overwrite vs Delete+replace Add five files to private files, create shortcuts to all of them from somewhere else Edit private files. Overwrite 1st file by uploading, overwrite 2nd file by picking it from any repository (as a copy), overwrite 3rd file using drag&drop. Remove the 4th file and upload another file with the same name as removed Rename the 5th file twice restoring original name Make sure first 3 files are still listed as sources and 4th & 5th are not Save file area and open it for editing again Make sure first 3 files are listed as sources and 4th & 5th are not Make sure shortcuts to the first 3 files are updated (new thumbnail, content is correct, ...) Test 4. Non-JS filemanager Disable Javascript Edit any form containing filearea (for example private files) and make sure basic operations work (look at moving and deleting the files). There is no support for file references and no possibility to overwrite a file in non-js filemanager. If something is not working as expected please make sure it is a regression from this issue before failing it Test 5. Attemp to create double reference Add a file to private files and create a shortcut to it from somewhere else Edit private files again, try to overwrite the source with a shortcut (like in Test 1 but with when file is a source of a shortcut) Make sure the popup appears and action is not possible Add another file to private files, save it and return to editing In another window create a shortcut to this file, save. Return to window with private files and overwrite the file with shortcut (no message appears because in this case we don't know that references exist) Save private files, make sure the exception is thrown and file is not substituted with shortcut Test 6. Unzipping Add archive file to the private files (must contain subfolders) Extract (unzip) archive file and make sure that files are extracted properly Save private files Create a shortcut to one of the extracted files from other filearea, save it Edit private files, overwrite the source file, save private files, make sure shortcut is updated Delete or modify some other extracted files, add more files to extracted subfolders, save private files Extract archive again and make sure everything is overwritten correctly, shortcut is still a shortcut and it is updated Note: there is no warning when files get overwritten during unzip. It is not related to this issue. Test 7. Overwriting file by another user Use any filearea where several users have access to (for example Folder resource in a course with several teachers) Upload a file as user1, save filearea (use the file here that has never previously been used) Open any textarea or filemanager, open filepicker and make sure user1 can see this file in his recent files Login as user2, upload (by overwriting) different file with the same name, save filearea Make sure user2 can see the new file in his recent files Make sure user1 can not see any of those two files in his recent files
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
      wip-MDL-39177-m24
    • Pull Master Branch:
      wip-MDL-39177-master
    • Rank:
      49780

      Description

      While testing some other repository issues I came to this situation:

      1) with some repository defined (mine was a filesystem system-wide repo).
      2) got to any area accepting aliases (user private files is ok).
      3) and add the file xxxx.yyy (making a copy of the file)
      4) then add again the same file (now creating an alias)
      5) The overwrite/rename dialog will be shown.
      6) Select "overwrite", so your second added file wins.
      7) Save changes to ensure there isn't any refresh visual glitch.
      8) The file xxxx.yyy file should be an alias. It's a copied file.

      Note that the opposite also happens, first alias, then copy and overwrite should lead to copy, but it remains as alias.

      Somehow the overwrite feature is now alias-aware.

      No problems detected when renaming is selected instead of overwrite.

      Ciao


      The complete list of problems:

      1. Filemanager was not refreshed after overwriting a file (this is issue MDL-33719).
      2. The 'original' attribute of draftfile was lost when file was overwritten, therefore the list of shortcuts no longer appeared (but it was not obvious becase filemanager was not refreshed).
      3. The 'original' attribute of draftfile was ignored when draftarea was saved. Which sort of compensated #2 but if file was deleted and replaced it was treated the same as if it was overwritten (even though filemanger UI distinguished those cases)
      4. When saving filearea the maxbytes check did not apply to the updated files
      5. It was not possible to replace the file with a reference (actually this is what the original issue text was about). So after it has became possible we need to make sure that double references are not created.
      6. When files are unzipped, they are overwritten without warning, source was not specified, original was lost.
      7. When one user overwrites the file of another user, the files.userid field was not updated and the new file appeared in the 'Recent file' of the wrong user

      To add to this: there is/was a lot of code duplication and overwriting files when picking from repository, uploading, drag&drop and/or unzipping are treated differently. JS and non-JS filemanager treat(ed) rename/move also differently.

        Issue Links

          Activity

          Hide
          Jason Hardin added a comment -

          THis also breaks when using the file picker within the HTML editor. Nothing is shown to the user that this doesn't work. Deleting the file first then aliasing a file of the same name does not resolve the issue either. Work arounds are toe alias and rename or delete the file, upload a differently named file, save, remove differently named file and alias the new name.

          Show
          Jason Hardin added a comment - THis also breaks when using the file picker within the HTML editor. Nothing is shown to the user that this doesn't work. Deleting the file first then aliasing a file of the same name does not resolve the issue either. Work arounds are toe alias and rename or delete the file, upload a differently named file, save, remove differently named file and alias the new name.
          Hide
          Chris Follin added a comment -
          Show
          Chris Follin added a comment - We're seeing this in 2.3.3 and it's not minor. It's the same as the comment here: https://tracker.moodle.org/browse/MDL-33719?focusedCommentId=174079&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-174079
          Hide
          Michael de Raadt added a comment -

          This and the linked issue should probably be addressed at the same time.

          Show
          Michael de Raadt added a comment - This and the linked issue should probably be addressed at the same time.
          Hide
          Michael de Raadt added a comment -

          Marina: I've added you as a watcher as you are involved in the linked issue.

          Show
          Michael de Raadt added a comment - Marina: I've added you as a watcher as you are involved in the linked issue.
          Hide
          Kris Stokking added a comment -

          Is there any way at all to know that the alias was lost, so that we can recover the links? I am very concerned that our clients who have spent the entire semester linking files will go to update those files over the summer and find out that the work was lost.

          Show
          Kris Stokking added a comment - Is there any way at all to know that the alias was lost, so that we can recover the links? I am very concerned that our clients who have spent the entire semester linking files will go to update those files over the summer and find out that the work was lost.
          Hide
          Marina Glancy added a comment -

          I'm working on this issue now.
          Just to update the watchers on progress: Yes, there is a bug that reference is not updated when file is overriden. But when we allow it we need to be sure not to create circular or double references. So there should be one check when user tries to overwrite the file, and another check when user saves draft filearea. Which involves UI change, etc.

          Show
          Marina Glancy added a comment - I'm working on this issue now. Just to update the watchers on progress: Yes, there is a bug that reference is not updated when file is overriden. But when we allow it we need to be sure not to create circular or double references. So there should be one check when user tries to overwrite the file, and another check when user saves draft filearea. Which involves UI change, etc.
          Hide
          Sam Hemelryk added a comment -

          Adding a link to MDL-33719 as a blocker

          Show
          Sam Hemelryk added a comment - Adding a link to MDL-33719 as a blocker
          Hide
          Marina Glancy added a comment -

          I added two commits for handling unzipping files from archive. Function file_storage::get_unused_dirname() probably needs to be added to master only

          Show
          Marina Glancy added a comment - I added two commits for handling unzipping files from archive. Function file_storage::get_unused_dirname() probably needs to be added to master only
          Hide
          Frédéric Massart added a comment - - edited

          Thanks Marina, this looks good to me.

          There are a few minor coding style issues, and I'm really not a fan of assignment and logic within an if condition, but that's just me.

          I think your testing instructions are covering a wide range of possible issues. I have myself tested a bit and things looked alright. Just noting that it's possible to reference from Private files to Private files, and that in the testing instructions you should perhaps add that for Dropbox (or others) there is a caching period during which the thumbnail or content won't be updated.

          In regard with the source special treatment, I think we would, at least, need a method that clarifies all that. Draft files having 2 properties (source and original) and non-draftfiles only having source is rather confusing. I agree that a new DB field is the solution, but meantime having:

          • get_source() returning the right source depending if draft or not
          • get_original() returning the original, if any, of a draft file, and an exception when it's not a draft file

          Oh, this is not new, but there is no confirmation when overwriting a file during zip extraction. Perhaps we should simply use get_unused_filename() when it happens. Or not... I don't know what is the common practices by operating systems when it comes to auto extraction in current directory.

          Anyway, your patch already cleans up a big mess! Thanks

          Fred

          Show
          Frédéric Massart added a comment - - edited Thanks Marina, this looks good to me. There are a few minor coding style issues, and I'm really not a fan of assignment and logic within an if condition, but that's just me. I think your testing instructions are covering a wide range of possible issues. I have myself tested a bit and things looked alright. Just noting that it's possible to reference from Private files to Private files, and that in the testing instructions you should perhaps add that for Dropbox (or others) there is a caching period during which the thumbnail or content won't be updated. In regard with the source special treatment, I think we would, at least, need a method that clarifies all that. Draft files having 2 properties (source and original) and non-draftfiles only having source is rather confusing. I agree that a new DB field is the solution, but meantime having: get_source() returning the right source depending if draft or not get_original() returning the original, if any, of a draft file, and an exception when it's not a draft file Oh, this is not new, but there is no confirmation when overwriting a file during zip extraction. Perhaps we should simply use get_unused_filename() when it happens. Or not... I don't know what is the common practices by operating systems when it comes to auto extraction in current directory. Anyway, your patch already cleans up a big mess! Thanks Fred
          Hide
          Marina Glancy added a comment -

          Hi Fred
          thanks for review. I'm very happy we both spent a lot of time on communicating about it and now at least one more current employee of Moodle HQ knows how this all mess works (or tries to work).

          get_source() may only be modified in master because it's part of API. But not 2.5 any more. And if it is modified in 2.6 we may as well add new field to files table (and btw remove two others: MDL-33416)

          re unzip: I was surprised the issue does not exist yet. It should get a prompt similar to drag&drop of multiple files. Btw, just found another interesting issue about unzip: MDL-32457

          ok, I'm preparing now the 2.3 and 2.4 branches

          Show
          Marina Glancy added a comment - Hi Fred thanks for review. I'm very happy we both spent a lot of time on communicating about it and now at least one more current employee of Moodle HQ knows how this all mess works (or tries to work). get_source() may only be modified in master because it's part of API. But not 2.5 any more. And if it is modified in 2.6 we may as well add new field to files table (and btw remove two others: MDL-33416 ) re unzip: I was surprised the issue does not exist yet. It should get a prompt similar to drag&drop of multiple files. Btw, just found another interesting issue about unzip: MDL-32457 ok, I'm preparing now the 2.3 and 2.4 branches
          Hide
          Marina Glancy added a comment -

          reopening. Just thought about overwriting option during restore - have to check that as well....

          Show
          Marina Glancy added a comment - reopening. Just thought about overwriting option during restore - have to check that as well....
          Hide
          Marina Glancy added a comment -

          Added new commit handling changed files.userid

          TO INTEGRATORS: note that the branches also contain commit for MDL-33719

          Show
          Marina Glancy added a comment - Added new commit handling changed files.userid TO INTEGRATORS: note that the branches also contain commit for MDL-33719
          Hide
          Marina Glancy added a comment -

          btw when testing overwriting during restore I found a different issue MDL-39765

          Show
          Marina Glancy added a comment - btw when testing overwriting during restore I found a different issue MDL-39765
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note that by integrating and testing this, MDL-33719 also becomes integrated and tested, confirmed by Marina. Keep them moving together.

          Show
          Eloy Lafuente (stronk7) added a comment - Note that by integrating and testing this, MDL-33719 also becomes integrated and tested, confirmed by Marina. Keep them moving together.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24, 25 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, 25 & master), thanks!
          Hide
          Ankit Agarwal added a comment -

          All tests pass on master.

          Show
          Ankit Agarwal added a comment - All tests pass on master.
          Hide
          Rossiani Wijaya added a comment -

          I tested 2.3 for Ankit.

          I noticed for test 6: if the zip file contains a folder within the subfolder, the unzip functionality doesn't work. I tested on 2.3 stable version, it works there.

          Also Ankit will do test 5 because it didn't throw any exception error on my distribution.

          Other than that, the rest looks good.

          Show
          Rossiani Wijaya added a comment - I tested 2.3 for Ankit. I noticed for test 6: if the zip file contains a folder within the subfolder, the unzip functionality doesn't work. I tested on 2.3 stable version, it works there. Also Ankit will do test 5 because it didn't throw any exception error on my distribution. Other than that, the rest looks good.
          Hide
          Marina Glancy added a comment -

          Rossie, I will check it tonight, thanks

          Show
          Marina Glancy added a comment - Rossie, I will check it tonight, thanks
          Hide
          Marina Glancy added a comment -

          Rossie, I could not reproduce this with unzip. Can you attach an archive file that does not unzip?

          Re test 5 - you said it did not throw an exception. Did you actually manage to create a file that is both source of shortcut and a shortcut itself?

          Show
          Marina Glancy added a comment - Rossie, I could not reproduce this with unzip. Can you attach an archive file that does not unzip? Re test 5 - you said it did not throw an exception. Did you actually manage to create a file that is both source of shortcut and a shortcut itself?
          Hide
          Rossiani Wijaya added a comment -

          Hi Marina,

          I have attached the zip file.

          Test #5: yes I'm able to create both source of shortcut and shortcut itself.

          Just wanted to make it clear, I tested this on 2.3.

          Show
          Rossiani Wijaya added a comment - Hi Marina, I have attached the zip file. Test #5: yes I'm able to create both source of shortcut and shortcut itself. Just wanted to make it clear, I tested this on 2.3.
          Hide
          Ankit Agarwal added a comment - - edited

          The Zip Rosie attached is failing in integration master as well, where as it works in stable master. Zip with single level sub-folder works on both.

          Show
          Ankit Agarwal added a comment - - edited The Zip Rosie attached is failing in integration master as well, where as it works in stable master. Zip with single level sub-folder works on both.
          Hide
          Ankit Agarwal added a comment -

          Am failing this for the time being, so the issue can be further investigated before we proceed with this patch, if it is found the issue is caused by some other issue in integration, we can proceed with this.
          Thanks

          Show
          Ankit Agarwal added a comment - Am failing this for the time being, so the issue can be further investigated before we proceed with this patch, if it is found the issue is caused by some other issue in integration, we can proceed with this. Thanks
          Hide
          Marina Glancy added a comment -

          Thanks Rossie for picking it. The problem was not because it was a single folder but because the zip contained the folder with the same name as zip file (and I used it as a temporary unzip folder), so I removed it on cleanup.
          TO INTEGRATORS: I added a commit in all branches that does a proper unzip cleanup.
          TO TESTERS: please only re-test 6 (unzipping), nothing else is affected with the last commit.
          Thanks

          Show
          Marina Glancy added a comment - Thanks Rossie for picking it. The problem was not because it was a single folder but because the zip contained the folder with the same name as zip file (and I used it as a temporary unzip folder), so I removed it on cleanup. TO INTEGRATORS: I added a commit in all branches that does a proper unzip cleanup. TO TESTERS: please only re-test 6 (unzipping), nothing else is affected with the last commit. Thanks
          Hide
          Marina Glancy added a comment -

          interesting fact: if a zip file test.zip contains a file named test.zip (which is crazy but still), the extracted file will overwrite the file being extracted. [unrelated to this issue]

          Show
          Marina Glancy added a comment - interesting fact: if a zip file test.zip contains a file named test.zip (which is crazy but still), the extracted file will overwrite the file being extracted. [unrelated to this issue]
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Silly question and why are we using the name of the zip as temp unzip storage? IMO it's one of the worst possibilities (more chances of collision with zip contents). Wouldn't be something "random" like a hash of something far better for temp unzip ?

          In any case... I'm looking and applying your extra commit now to all branches...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Silly question and why are we using the name of the zip as temp unzip storage? IMO it's one of the worst possibilities (more chances of collision with zip contents). Wouldn't be something "random" like a hash of something far better for temp unzip ? In any case... I'm looking and applying your extra commit now to all branches... Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Extra commit applied to all branches (to 25_STABLE by cherry-picking from master). Sending back to testing... thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Extra commit applied to all branches (to 25_STABLE by cherry-picking from master). Sending back to testing... thanks!
          Hide
          Rossiani Wijaya added a comment -

          Hi Marina,

          Thank you for fixing this.

          I re-tested for 2.3 and it works as expected.

          I will leave it to Ankit to finalize the decision for this test.

          Show
          Rossiani Wijaya added a comment - Hi Marina, Thank you for fixing this. I re-tested for 2.3 and it works as expected. I will leave it to Ankit to finalize the decision for this test.
          Hide
          Ankit Agarwal added a comment -

          Marina, in test 6 step 7, it is correct behaviour if the "Extra files" stay in, after unzipping the archive second time?

          Show
          Ankit Agarwal added a comment - Marina, in test 6 step 7, it is correct behaviour if the "Extra files" stay in, after unzipping the archive second time?
          Hide
          Marina Glancy added a comment -

          yes of course

          Show
          Marina Glancy added a comment - yes of course
          Hide
          Marina Glancy added a comment -

          Eloy, answering your question. Because hopefully at some point we will start giving user a warning that after unzip some files are going to be overwritten. And an option will be to unzip class to the not-yet-existing folder.

          Show
          Marina Glancy added a comment - Eloy, answering your question. Because hopefully at some point we will start giving user a warning that after unzip some files are going to be overwritten. And an option will be to unzip class to the not-yet-existing folder.
          Hide
          Ankit Agarwal added a comment -

          24 is same as other branches, as suggested by Marina, testing on 23 and master is sufficient.
          Passing!

          Show
          Ankit Agarwal added a comment - 24 is same as other branches, as suggested by Marina, testing on 23 and master is sufficient. Passing!
          Hide
          Damyon Wiese added a comment -

          Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone.

          Closing as Fixed!

          Show
          Damyon Wiese added a comment - Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone. Closing as Fixed!
          Hide
          Plecto Rincus added a comment -

          Seems to have reappeared (or having a similirar bug) in version 2.4.6 of Moodle.
          Check here. https://tracker.moodle.org/browse/MDL-42153

          Show
          Plecto Rincus added a comment - Seems to have reappeared (or having a similirar bug) in version 2.4.6 of Moodle. Check here. https://tracker.moodle.org/browse/MDL-42153
          Hide
          Marina Glancy added a comment -

          Piecto, those two issue have nothing in common

          Show
          Marina Glancy added a comment - Piecto, those two issue have nothing in common

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: