Moodle
  1. Moodle
  2. MDL-26388

Handle duplicate files uploaded through filepicker

    Details

    • Testing Instructions:
      Hide

      Hello testers
      This issue needs to test in html editor and file manager.

      HTML Editor
      1. Adding a picture to html editor with name for example: test.jpg
      2. Adding another picture with the same name by using upload/local/recent/private repository plugins, you should see a dialog pop up asking you want to overwrite/rename or cancel, try all options to see if it works as expected.

      File manager, for example in your private file management page
      1. Adding a picture to filemanager with name for example: test.jpg
      2. Adding another picture with the same name using upload/local/recent/private repository plugins, you should see a dialog pop up asking you want to overwrite/rename or cancel, try all options to see if it works as expected.

      Show
      Hello testers This issue needs to test in html editor and file manager. HTML Editor 1. Adding a picture to html editor with name for example: test.jpg 2. Adding another picture with the same name by using upload/local/recent/private repository plugins, you should see a dialog pop up asking you want to overwrite/rename or cancel, try all options to see if it works as expected. File manager, for example in your private file management page 1. Adding a picture to filemanager with name for example: test.jpg 2. Adding another picture with the same name using upload/local/recent/private repository plugins, you should see a dialog pop up asking you want to overwrite/rename or cancel, try all options to see if it works as expected.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull Master Branch:
      s9_MDL-26388_filepicker_existing_files_master
    • Rank:
      16088

      Description

      User are not able to:
      1/ delete "uploaded" file
      2/ user can not find out what files were already uploaded
      3/ users can not link again already existing image

      The reason is that there is no way to manage the files that are uploaded to the file area referenced from the text in tinymce editor. Attempts to work around this missing feature (overriding of files, link from forms, etc.) were only creating regressions.

      Possible solution - add a new tab "Used files" to the file picker, this would solve all 3 issues above. I have proposed several different solutions before, but I think this is the best one.

      1. 2011-04-11 12.01.bmml
        1 kB
        Dongsheng Cai
      1. 2011-04-11 12.01.png
        13 kB
      2. mdl_26388.jpg
        170 kB
      3. picker.png
        23 kB
      4. picker2.png
        307 kB
      5. safari.png
        46 kB

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          Definitely! Please change the type of the issue from Improvement to Bug as it really is that. Just one question.

          If "Existing files" (or Current files or whatever the final name would be) behaves as a repository, how will the user be able to delete a file? Does filepicker allows such operations in the right part of the window?

          Show
          David Mudrak added a comment - Definitely! Please change the type of the issue from Improvement to Bug as it really is that. Just one question. If "Existing files" (or Current files or whatever the final name would be) behaves as a repository, how will the user be able to delete a file? Does filepicker allows such operations in the right part of the window?
          Hide
          Petr Škoda added a comment -

          The "Used files" are all files that are carried along together with each forum post - if you backup forum and restore it on another server all those files will be available there.

          Please note that you can not link the "Used files" outside of the current TinyMCE editor, it is a local file area only for one post, nothing else.

          Show
          Petr Škoda added a comment - The "Used files" are all files that are carried along together with each forum post - if you backup forum and restore it on another server all those files will be available there. Please note that you can not link the "Used files" outside of the current TinyMCE editor, it is a local file area only for one post, nothing else.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 for 2.1.

          The only conflicting point is that the htmlarea or whatever should be immediately (eh, I wrote it perfectly, lol) refreshed to represent the current status in case of deletion of really used file (because the named "used files" tab isn't really used, but stored).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 for 2.1. The only conflicting point is that the htmlarea or whatever should be immediately (eh, I wrote it perfectly, lol) refreshed to represent the current status in case of deletion of really used file (because the named "used files" tab isn't really used, but stored). Ciao
          Hide
          Sam Marshall added a comment -

          Agree this is a good feature.

          I would suggest changing the name from 'Used files' as I think this will confuse. Unfortunately I am having trouble thinking of a good solution that isn't quite long. Something like 'Files associated with this text area'. That's a bit long. Perhaps a tab like 'Current files' or 'Existing files' and then a single sentence below to explain once you actually click onto that tab, like:

          'The list below shows all files that are already contained within this text area.'

          (it's the 'within this text area' bit that is not obvious from the name...)

          However I don't think this is a silver bullet - the behaviour when you upload a file with a name that already exists is not a pleasant interface and should ideally be improved even if this feature is in place.

          One option for that would be to change the popup so it offers 'Rename' and 'Cancel' - the existing one just says 'ok' and dumps you out with no clue as to what to do. Rename is always what you want to do. Deleting the file and uploading again with the same name is a bad idea; you can't reuse the same name as cache will cause horrible confusion if you do that...

          (btw I think it would be better if moodle automatically changed all file names upon upload so that if you upload frog.doc it would become frog.8ba0df34.doc, i.e. add 8 chars from the start of contenthash to ensure that if the file ever changes it will get a new name... but that's another issue.)

          Show
          Sam Marshall added a comment - Agree this is a good feature. I would suggest changing the name from 'Used files' as I think this will confuse. Unfortunately I am having trouble thinking of a good solution that isn't quite long. Something like 'Files associated with this text area'. That's a bit long. Perhaps a tab like 'Current files' or 'Existing files' and then a single sentence below to explain once you actually click onto that tab, like: 'The list below shows all files that are already contained within this text area.' (it's the 'within this text area' bit that is not obvious from the name...) However I don't think this is a silver bullet - the behaviour when you upload a file with a name that already exists is not a pleasant interface and should ideally be improved even if this feature is in place. One option for that would be to change the popup so it offers 'Rename' and 'Cancel' - the existing one just says 'ok' and dumps you out with no clue as to what to do. Rename is always what you want to do. Deleting the file and uploading again with the same name is a bad idea; you can't reuse the same name as cache will cause horrible confusion if you do that... (btw I think it would be better if moodle automatically changed all file names upon upload so that if you upload frog.doc it would become frog.8ba0df34.doc, i.e. add 8 chars from the start of contenthash to ensure that if the file ever changes it will get a new name... but that's another issue.)
          Hide
          Petr Škoda added a comment -

          I agree that popup confirmation with rename/overwrite would be nice.

          My point was that you may not decide easily if you do not know what files are there already, if there was a file manager you could simply delete the file and upload a new one.

          There is a little internal problem, JS filepicker UI is used from the filepicker forms element too - in that case it should always be overwriting the file because only one is expected (at present it sometimes throws the error instead).

          Show
          Petr Škoda added a comment - I agree that popup confirmation with rename/overwrite would be nice. My point was that you may not decide easily if you do not know what files are there already, if there was a file manager you could simply delete the file and upload a new one. There is a little internal problem, JS filepicker UI is used from the filepicker forms element too - in that case it should always be overwriting the file because only one is expected (at present it sometimes throws the error instead).
          Hide
          Mark Emery added a comment -

          Our moodle 2.0.2 has been rendered unusable by this bug. All our course images broke on friday afternoon. we have no idea why, the images are still there in the recent file list but we cannot repair the links because choosing an existing file gives the file exists error.
          The main concern is why every image link in all courses broke at once when nothing in the system changed that we know of.

          Show
          Mark Emery added a comment - Our moodle 2.0.2 has been rendered unusable by this bug. All our course images broke on friday afternoon. we have no idea why, the images are still there in the recent file list but we cannot repair the links because choosing an existing file gives the file exists error. The main concern is why every image link in all courses broke at once when nothing in the system changed that we know of.
          Hide
          Dongsheng Cai added a comment -

          Added UI Mockup: <2011-04-11 12.01>

          Show
          Dongsheng Cai added a comment - Added UI Mockup: <2011-04-11 12.01>
          Hide
          Brandon Horn added a comment -

          Does the proposed solution allow used files to be deleted? Currently, once a file has been uploaded to an assignment, it can't be deleted. This is a significant disk space leak when large files are attached to assignments.

          Show
          Brandon Horn added a comment - Does the proposed solution allow used files to be deleted? Currently, once a file has been uploaded to an assignment, it can't be deleted. This is a significant disk space leak when large files are attached to assignments.
          Hide
          Dongsheng Cai added a comment -

          Brandon

          The proposed solution allow to overwrite existing files or rename it.

          Show
          Dongsheng Cai added a comment - Brandon The proposed solution allow to overwrite existing files or rename it.
          Hide
          Elena Ivanova added a comment -

          imho, we really need a solution that allows delete them )
          There can be thousands of those files, embedded in the labels, webpages, HTML blocks, etc, etc.

          Show
          Elena Ivanova added a comment - imho, we really need a solution that allows delete them ) There can be thousands of those files, embedded in the labels, webpages, HTML blocks, etc, etc.
          Hide
          Dongsheng Cai added a comment -

          Elena
          I totally agree with you, to manage all these files, we need to change a lot on current UI, it's planed on 2.1, but for now we need a temporary solution to handle the duplicated files.

          Show
          Dongsheng Cai added a comment - Elena I totally agree with you, to manage all these files, we need to change a lot on current UI, it's planed on 2.1, but for now we need a temporary solution to handle the duplicated files.
          Hide
          Elena Ivanova added a comment -

          ahh, ok. So it is on the roadmap already

          Show
          Elena Ivanova added a comment - ahh, ok. So it is on the roadmap already
          Hide
          Aparup Banerjee added a comment -

          Hi Dongsheng,

          1) 'being' should be 'been' (x2) in :
          $string['fileexistsdialog_editor'] = 'A file with that name has already being attached to the text you are editing.';
          (and the next line too.)

          2)abstract class repository { ...
          should has_moodle_files() be abstracted as well ? if we're to depend on it, i think its better if we force the re-implementation onto the plugin developer.

          3)i don't think so but just checking: draftfile_exists() ...
          does $fs->get_file() actually retrieve the file into a short lived memory? (a really large file over NAS wouldn't perform well just for file exists test. especially in the while loop in get_unused_filename() )

          4) append_suffix() : do we want to limit file name length anytime (file systems bc)? also, incrementing suffix will allow more names than appending suffix.

          5) overwrite_existing_draftfile() : can we delete the first draftfile after the second draft file ($newfile) is created successfully? i mean if the $newfile fails to be created, would the original draft file be lost to moodle (for handling differently)? if itemid reuse is the problem, can we change the 'symlink' successfully before deleting the actual file.

          6) not yet tested, will test and feedback soon.

          Show
          Aparup Banerjee added a comment - Hi Dongsheng, 1) 'being' should be 'been' (x2) in : $string ['fileexistsdialog_editor'] = 'A file with that name has already being attached to the text you are editing.'; (and the next line too.) 2)abstract class repository { ... should has_moodle_files() be abstracted as well ? if we're to depend on it, i think its better if we force the re-implementation onto the plugin developer. 3)i don't think so but just checking: draftfile_exists() ... does $fs->get_file() actually retrieve the file into a short lived memory? (a really large file over NAS wouldn't perform well just for file exists test. especially in the while loop in get_unused_filename() ) 4) append_suffix() : do we want to limit file name length anytime (file systems bc)? also, incrementing suffix will allow more names than appending suffix. 5) overwrite_existing_draftfile() : can we delete the first draftfile after the second draft file ($newfile) is created successfully? i mean if the $newfile fails to be created, would the original draft file be lost to moodle (for handling differently)? if itemid reuse is the problem, can we change the 'symlink' successfully before deleting the actual file. 6) not yet tested, will test and feedback soon.
          Hide
          Dongsheng Cai added a comment -

          1. Fixed
          2. we shouldn't encourage people create repository plugin host moodle files, better to make it return false by default
          3. not really, get_file only deals with db records
          4.
          5. changed the logic, if temp file exists successfully, old file will be deleted. Remember we need to delete the old file to release the file name, so it has to be before create_file_from_storedfile

          Show
          Dongsheng Cai added a comment - 1. Fixed 2. we shouldn't encourage people create repository plugin host moodle files, better to make it return false by default 3. not really, get_file only deals with db records 4. 5. changed the logic, if temp file exists successfully, old file will be deleted. Remember we need to delete the old file to release the file name, so it has to be before create_file_from_storedfile
          Hide
          Aparup Banerjee added a comment -

          4) i've just tried uploading during test (which is working so far!).
          just wanted to note the very confusing situation : upload my test files called 'test.jpg' a few times then upload my 'test_2.jpg' has gotten me to a point i don't know which file is which :-D

          Show
          Aparup Banerjee added a comment - 4) i've just tried uploading during test (which is working so far!). just wanted to note the very confusing situation : upload my test files called 'test.jpg' a few times then upload my 'test_2.jpg' has gotten me to a point i don't know which file is which :-D
          Hide
          Aparup Banerjee added a comment -

          ok all tested as per instructions.
          (there was a glitch (broken img) while changing testing from uploading to using recent files using the same test file, but i can't reproduce it)

          works for me!

          Show
          Aparup Banerjee added a comment - ok all tested as per instructions. (there was a glitch (broken img) while changing testing from uploading to using recent files using the same test file, but i can't reproduce it) works for me!
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've looked through this now and had a play with it.
          It gets my +1 for integration, it is a big improvement to the way things were.
          I did spot a couple of minor whitespace things within the JS changes however I am happy to fix them up during integration.
          I'll talk with Eloy about it this afternoon and providing he is happy as well it will be integrated.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've looked through this now and had a play with it. It gets my +1 for integration, it is a big improvement to the way things were. I did spot a couple of minor whitespace things within the JS changes however I am happy to fix them up during integration. I'll talk with Eloy about it this afternoon and providing he is happy as well it will be integrated. Cheers Sam
          Hide
          Dongsheng Cai added a comment -

          Thanks Sam

          Just noticed this issue got 40 votes!

          Show
          Dongsheng Cai added a comment - Thanks Sam Just noticed this issue got 40 votes!
          Hide
          Sam Hemelryk added a comment -

          Thanks Dongsheng, this has been integrated now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Dongsheng, this has been integrated now. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          Yay and there was much rejoicing!

          Show
          Martin Dougiamas added a comment - Yay and there was much rejoicing!
          Hide
          Helen Foster added a comment -

          Just tested in the HTML editor as follows:

          1. Create a new blog entry
          2. In the blog entry body insert an image
          3. Using the file picker upload a file test.jpg
          4. Save changes
          5. Create another new blog entry
          6. In the blog entry body insert an image
          7. In the file picker select recent files and then the file test.jpg

          The insert/edit image dialogue box then has the image URL filled in as "undefined" which then results in a broken link if the image is inserted into the blog entry.

          I've tried on the QA testing site and my local 2.0 site and obtain the same results. I don't get a dialogue box asking whether the file should be overwritten.

          Sam or Dongsheng, please tell me what I am doing wrong in my testing, otherwise I will have to fail this issue.

          Show
          Helen Foster added a comment - Just tested in the HTML editor as follows: 1. Create a new blog entry 2. In the blog entry body insert an image 3. Using the file picker upload a file test.jpg 4. Save changes 5. Create another new blog entry 6. In the blog entry body insert an image 7. In the file picker select recent files and then the file test.jpg The insert/edit image dialogue box then has the image URL filled in as "undefined" which then results in a broken link if the image is inserted into the blog entry. I've tried on the QA testing site and my local 2.0 site and obtain the same results. I don't get a dialogue box asking whether the file should be overwritten. Sam or Dongsheng, please tell me what I am doing wrong in my testing, otherwise I will have to fail this issue.
          Hide
          Dongsheng Cai added a comment -

          Hi Helen

          Aparup fixed it by reseting the browser cache.

          Regards
          Dongsheng

          Show
          Dongsheng Cai added a comment - Hi Helen Aparup fixed it by reseting the browser cache. Regards Dongsheng
          Hide
          Helen Foster added a comment -

          Sorry I have to fail this because resetting the browser cache does NOT fix this problem. I've updated the QA site from the integration server and tried Firefox and Chrome. The problem of an undefined image URL remains.

          Please note that this problem occurs when attempting to use the same file in a NEW blog entry. If I try editing the original blog entry and adding the same file to it a second time then I obtain a different problem (using Chrome): after selecting the file, the file picker loading box appears on top of the insert/edit image box which appears on top of the file exists dialogue box.

          Show
          Helen Foster added a comment - Sorry I have to fail this because resetting the browser cache does NOT fix this problem. I've updated the QA site from the integration server and tried Firefox and Chrome. The problem of an undefined image URL remains. Please note that this problem occurs when attempting to use the same file in a NEW blog entry. If I try editing the original blog entry and adding the same file to it a second time then I obtain a different problem (using Chrome): after selecting the file, the file picker loading box appears on top of the insert/edit image box which appears on top of the file exists dialogue box.
          Hide
          Dongsheng Cai added a comment -

          Sam
          I cannot reopen this issue from my end.
          I just committed the fix on top of my branches.

          Show
          Dongsheng Cai added a comment - Sam I cannot reopen this issue from my end. I just committed the fix on top of my branches.
          Hide
          Sam Hemelryk added a comment -

          Thanks for spotting that Helen and thanks for getting a quick fix Donghseng.
          This is a very popular issue so I have integrated the fix immediately rather than revert this for next week.

          Helen if you have a moment could you please test this again.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for spotting that Helen and thanks for getting a quick fix Donghseng. This is a very popular issue so I have integrated the fix immediately rather than revert this for next week. Helen if you have a moment could you please test this again. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've tried the steps commented by Helen above, two different blog entries using the same image (uploaded in the fist entry and picked from recent in the second), and it has worked perfecty.

          Then I've tried to add the same image twice in the same blog entry and I get the filepicker on top, with the spinning wheel image and, the overwrite dialog dimmed and behind.

          Only if I close the filepicker and cancel the "add/edit image" dialog I can see the overwite dialog: "A file with that name has already been attached to the text you are editing, overwrite/rename/cancel".

          Using Safari (Mac) here with all caches purged before testing.

          Going to try in other browsers.

          Show
          Eloy Lafuente (stronk7) added a comment - I've tried the steps commented by Helen above, two different blog entries using the same image (uploaded in the fist entry and picked from recent in the second), and it has worked perfecty. Then I've tried to add the same image twice in the same blog entry and I get the filepicker on top, with the spinning wheel image and, the overwrite dialog dimmed and behind. Only if I close the filepicker and cancel the "add/edit image" dialog I can see the overwite dialog: "A file with that name has already been attached to the text you are editing, overwrite/rename/cancel". Using Safari (Mac) here with all caches purged before testing. Going to try in other browsers.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Working ok on Chrome (I get the overwrite dialog on top and all options - overwite, rename and cancel - seem to be working ok)

          Show
          Eloy Lafuente (stronk7) added a comment - Working ok on Chrome (I get the overwrite dialog on top and all options - overwite, rename and cancel - seem to be working ok)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Working on FF too ok, going to clean all caches in safari and try again

          Show
          Eloy Lafuente (stronk7) added a comment - Working on FF too ok, going to clean all caches in safari and try again
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Have cleaned all caches in Safari, disabled them, restarted it, purged again all moodle chaches + theme cache and I continue getting the same problem, the "overwrite dialog" is shown under (below) both the file picker and the "add/edit image" dialogs.

          Attaching image (safari.png), ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Have cleaned all caches in Safari, disabled them, restarted it, purged again all moodle chaches + theme cache and I continue getting the same problem, the "overwrite dialog" is shown under (below) both the file picker and the "add/edit image" dialogs. Attaching image (safari.png), ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Exactly same behavior with Safari & file managers, the "overwrite dialog" is shown below the file picker. Although if you move it, buttons work ok.

          Show
          Eloy Lafuente (stronk7) added a comment - Exactly same behavior with Safari & file managers, the "overwrite dialog" is shown below the file picker. Although if you move it, buttons work ok.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Confirmed same behavior with Safari on qa.moodle.net too.

          Show
          Eloy Lafuente (stronk7) added a comment - Confirmed same behavior with Safari on qa.moodle.net too.
          Hide
          Michael Blake added a comment -

          safari screenshot

          Show
          Michael Blake added a comment - safari screenshot
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I'm passing this as far as it works under IE, FF and Chrome.

          I've created MDL-27381 for the detected Safari problem. Would be hyper-cool to have it fixed before release tomorrow or, if not, in the minimum number of weeks (aka 1 )

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I'm passing this as far as it works under IE, FF and Chrome. I've created MDL-27381 for the detected Safari problem. Would be hyper-cool to have it fixed before release tomorrow or, if not, in the minimum number of weeks (aka 1 )
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed. Many thanks!

          Plz, take a look to the Safari followup. It would be awesome if we can integrate it tomorrow, so 2.0.3 will get it working. If it's not possible for tomorrow... then ASAP. TIA!

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed. Many thanks! Plz, take a look to the Safari followup. It would be awesome if we can integrate it tomorrow, so 2.0.3 will get it working. If it's not possible for tomorrow... then ASAP. TIA!
          Hide
          Petr Škoda added a comment - - edited

          This issue was NOT fixed sorry! You have hijacked my bug report with all the votes and fixed a different minor problem!!!!

          I do not know who the moodle.com user was but changing "no management of files uploaded through tinymce filepicker" to "Handle duplicate files uploaded through filepicker" is completely wrong.

          Please reopen this issue and change the title back.

          Show
          Petr Škoda added a comment - - edited This issue was NOT fixed sorry! You have hijacked my bug report with all the votes and fixed a different minor problem!!!! I do not know who the moodle.com user was but changing "no management of files uploaded through tinymce filepicker" to "Handle duplicate files uploaded through filepicker" is completely wrong. Please reopen this issue and change the title back.
          Hide
          Robert Puffer added a comment -

          It really sucks when folks hijack an issue and change the subject line.

          Show
          Robert Puffer added a comment - It really sucks when folks hijack an issue and change the subject line.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 to clone in order to continue with the original issue in another place, keeping this as is now.

          Show
          Eloy Lafuente (stronk7) added a comment - +1 to clone in order to continue with the original issue in another place, keeping this as is now.
          Hide
          Martin Dougiamas added a comment -

          Yes, I agree it was an error to rename this issue. I'll clone a new one to cover file management.

          Show
          Martin Dougiamas added a comment - Yes, I agree it was an error to rename this issue. I'll clone a new one to cover file management.

            People

            • Votes:
              42 Vote for this issue
              Watchers:
              19 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: