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
    • Rank:
      46434

      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.

        Issue Links

          Activity

          Hide
          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
          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
          Michael de Raadt added a comment -

          It looks like Marina is already looking at this.

          Show
          Michael de Raadt added a comment - It looks like Marina is already looking at this.
          Hide
          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 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
          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
          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 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 Glancy added a comment - Fred, disabling button is a great idea! (End disabling "Enter" key as well?) Anyway, assigning it to you. Thanks!
          Hide
          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
          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 Glancy added a comment -

          Nice, I like this solution very much!

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

          Thanks! Pushing for integration.

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

          Assigning to Damyon as he integrated MDL-37641.

          Show
          Frédéric Massart added a comment - Assigning to Damyon as he integrated MDL-37641 .
          Hide
          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
          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
          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
          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 added a comment -

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

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

          Rebased and resubmitting for integration.

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

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          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
          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
          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
          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:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: