Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36900

"Create folder" dialog closes without error message in file manager when no folder name is given

    Details

    • Testing Instructions:
      Hide
      1. Go to your private files
      2. Click new folder
        • Make sure the textbox displays "New folder"
        • Make sure the text is selected
        • Hit enter and make sure the folder is created
      3. Click new folder again
        • Make sure the textbox displays "New folder (1)"
        • Click "Create" and make sure the folder is created
      4. Click new folder again
        • Make sure the textbox displays "New folder (2)"
        • Delete the content of the textbox
        • Make sure the 'Create' button is disabled
        • Make sure hitting 'Enter' key does not do anything

      Drag and Drop

      1. Upload a file to your private files via drag and drop (say test.txt)
      2. Upload the same file via drag and drop
      3. Make sure the file name of the newly uploaded file is 'test (1).txt'
      Show
      Go to your private files Click new folder Make sure the textbox displays "New folder" Make sure the text is selected Hit enter and make sure the folder is created Click new folder again Make sure the textbox displays "New folder (1)" Click "Create" and make sure the folder is created Click new folder again Make sure the textbox displays "New folder (2)" Delete the content of the textbox Make sure the 'Create' button is disabled Make sure hitting 'Enter' key does not do anything Drag and Drop Upload a file to your private files via drag and drop (say test.txt) Upload the same file via drag and drop Make sure the file name of the newly uploaded file is 'test (1).txt'
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36900-master

      Description

      When using the "Create folder" dialog in the file manager, if one clicks on the "Create folder" button without first providing a folder name the dialog is hidden and process cancelled. It would be preferable if the user received an error message similar to that during the folder rename process if no name is provided.

      Reproduction steps:

      1. Click on the "Create folder" link in the file manager (anywhere - private files, File Resource modedit, etc)
      2. Leave the "Enter a name:" text box empty or add spaces, but no other characters
      3. Click the "Create folder" button

      Actual result: New folder creation dialog automatically closes when user clicks "Create folder" button without entering the folder name.
      Expected Result: User should be alerted with appropriate message when folder name not entered and clicks on Create folder button.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              sbc24 Sam Chaffee added a comment -

              Attaching the diff for a proposed fix. This uses the same string that the renaming folder validation error uses.

              Show
              sbc24 Sam Chaffee added a comment - Attaching the diff for a proposed fix. This uses the same string that the renaming folder validation error uses.
              Hide
              salvetore Michael de Raadt added a comment -

              It looks like Marina is already looking at this.

              Show
              salvetore Michael de Raadt added a comment - It looks like Marina is already looking at this.
              Hide
              marina Marina Glancy added a comment -

              I was not actually looking at it, just found this issue accidentally because it had a strange status (seemed that I was both assignee and doing peer review).
              I don't personally think this should be classified as a bug but rather as an improvement:
              When user clicks "Create folder" the popup should appear with default name "New folder" (or "New folder (X)") which is selected, so if user clicks "Create", a folder with default name gets created. At the same time if user starts to type a new name the default value just disappears because it was selected and focused.
              In this case we can display error message if user erased a default name but did not type anything instead.

              Added Fred as a watcher here because he is master of "File (2)" names

              Show
              marina Marina Glancy added a comment - I was not actually looking at it, just found this issue accidentally because it had a strange status (seemed that I was both assignee and doing peer review). I don't personally think this should be classified as a bug but rather as an improvement: When user clicks "Create folder" the popup should appear with default name "New folder" (or "New folder (X)") which is selected, so if user clicks "Create", a folder with default name gets created. At the same time if user starts to type a new name the default value just disappears because it was selected and focused. In this case we can display error message if user erased a default name but did not type anything instead. Added Fred as a watcher here because he is master of "File (2)" names
              Hide
              fred Frédéric Massart added a comment -

              I am personally not a fan of error messages and alert popups or so... especially when it is clear that the user did something wrong.
              Why not "disabling" the "Create" button as long as the field does not contain a valid folder name?
              +1 for a default folder name.

              Show
              fred Frédéric Massart added a comment - I am personally not a fan of error messages and alert popups or so... especially when it is clear that the user did something wrong. Why not "disabling" the "Create" button as long as the field does not contain a valid folder name? +1 for a default folder name.
              Hide
              marina Marina Glancy added a comment -

              Fred, disabling button is a great idea! (End disabling "Enter" key as well?)
              Anyway, assigning it to you. Thanks!

              Show
              marina Marina Glancy added a comment - Fred, disabling button is a great idea! (End disabling "Enter" key as well?) Anyway, assigning it to you. Thanks!
              Hide
              fred Frédéric Massart added a comment -

              Pushing for peer review. While fixing this, I actually realised that this should maybe have been closed as 'Not a bug' as a user should not expect anything when creating an folder without providing a name. But as I had spent some time on it already... here is the patch.

              So now:

              • the button gets disabled if the folder name is not valid
              • the field is populated with a default folder name
              • if the default folder name is taken, we increment it

              I rebased this issue on MDL-37641, and moved the JS logic of incrementing a file name from D&D upload, to JavaScript static so that it's reusable and not duplicated.

              Cheers!
              Fred

              Show
              fred Frédéric Massart added a comment - Pushing for peer review. While fixing this, I actually realised that this should maybe have been closed as 'Not a bug' as a user should not expect anything when creating an folder without providing a name. But as I had spent some time on it already... here is the patch. So now: the button gets disabled if the folder name is not valid the field is populated with a default folder name if the default folder name is taken, we increment it I rebased this issue on MDL-37641 , and moved the JS logic of incrementing a file name from D&D upload, to JavaScript static so that it's reusable and not duplicated. Cheers! Fred
              Hide
              marina Marina Glancy added a comment -

              Nice, I like this solution very much!

              Show
              marina Marina Glancy added a comment - Nice, I like this solution very much!
              Hide
              fred Frédéric Massart added a comment -

              Thanks! Pushing for integration.

              Show
              fred Frédéric Massart added a comment - Thanks! Pushing for integration.
              Hide
              fred Frédéric Massart added a comment -

              Assigning to Damyon as he integrated MDL-37641.

              Show
              fred Frédéric Massart added a comment - Assigning to Damyon as he integrated MDL-37641 .
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi Fred I'm reopening this because it does not seem to be properly rebased and also, given the amount of changes recently landing to repos/js, it's conflicting way too much.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi Fred I'm reopening this because it does not seem to be properly rebased and also, given the amount of changes recently landing to repos/js, it's conflicting way too much.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              fred Frédéric Massart added a comment -

              Rebased and resubmitting for integration.

              Show
              fred Frédéric Massart added a comment - Rebased and resubmitting for integration.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
              Hide
              dmonllao David Monllaó added a comment -

              It passes, works as expected, about the drag & drop section of the test I'm asked to confirm the file name and the default value is XXXX (1).txt

              Show
              dmonllao David Monllaó added a comment - It passes, works as expected, about the drag & drop section of the test I'm asked to confirm the file name and the default value is XXXX (1).txt
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

              Thanks!

              PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

                People

                • Votes:
                  0 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/May/13