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

          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: