Moodle
  1. Moodle
  2. MDL-31656

Downloading a file that was just uploaded into a form should not navigate away from page. Triggers form changed warnings.

    Details

    • Testing Instructions:
      Hide

      Note to tester: This change is related to MDL-31660 and MDL-31655. To aid your testing, you may want to test all three together. These instructions are additive

      Uploading and Downloading a file
      • In a form with a file picker (new discussion for example)
      • Upload a file using the 'Add' button and the filepicker
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • From the menu on the file, choose Download
        • Confirm that the file downloads
        • Confirm that no warning was given
      Show
      Note to tester: This change is related to MDL-31660 and MDL-31655 . To aid your testing, you may want to test all three together. These instructions are additive Uploading and Downloading a file In a form with a file picker (new discussion for example) Upload a file using the 'Add' button and the filepicker Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay From the menu on the file, choose Download Confirm that the file downloads Confirm that no warning was given
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31656-master-3

      Description

      While testing MDL-31114 with MDL-31315 patch in integration.git, i noticed that after i uploaded a file to a form and if i then (just to be a bandwidth hog) tried to download it again, i would be prompted to stay or leave the page.

      If i chose 'stay' i wouldn't get any download happening. If i chose 'leave' the file would get downloaded.

      I think primarily that having the download option in files just uploaded is slightly ridiculous but i understand it is a valid option to have.

      Still, if we have the download option, i think it shouldn't be triggering the 'form has changed' warning prompts.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Aparup Banerjee added a comment -

            also consider while on any editing page with a form and file picker, after editing, then attempting to download a file would trigger a confusing prompt when it appears not to be navigating away to another page.

            Show
            Aparup Banerjee added a comment - also consider while on any editing page with a form and file picker, after editing, then attempting to download a file would trigger a confusing prompt when it appears not to be navigating away to another page.
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew, I've made you assignee on this issue.
            If you're unable to get to it no probs but could you please assign it to moodle.com so that it can go through our normal processes.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, I've made you assignee on this issue. If you're unable to get to it no probs but could you please assign it to moodle.com so that it can go through our normal processes. Cheers Sam
            Hide
            Andrew Nicols added a comment -

            There is a very sensible rationale for having this warning on file uploads – assignments.

            The reason that the original change was implemented here, and at some of the other institutions who have commented on the original bug, was that many students upload files in an assignment activity, but don't realise that they must save their changes first.

            Therefore I'd say that we shouldn't remove the warning for file elements. I do agree though that we shouldn't navigate away to perform the download.

            Show
            Andrew Nicols added a comment - There is a very sensible rationale for having this warning on file uploads – assignments. The reason that the original change was implemented here, and at some of the other institutions who have commented on the original bug, was that many students upload files in an assignment activity, but don't realise that they must save their changes first. Therefore I'd say that we shouldn't remove the warning for file elements. I do agree though that we shouldn't navigate away to perform the download.
            Hide
            Andrew Nicols added a comment -

            I've changed the filemanager code to spawn a new window rather than adjust the URL of the current window.
            This has the effect that a browser tab/window will momentarily pop up when using the Download option, but the benefit that warnings are no longer displayed.
            The 'Download Folder' option already works in this manner.

            Show
            Andrew Nicols added a comment - I've changed the filemanager code to spawn a new window rather than adjust the URL of the current window. This has the effect that a browser tab/window will momentarily pop up when using the Download option, but the benefit that warnings are no longer displayed. The 'Download Folder' option already works in this manner.
            Hide
            Andrew Nicols added a comment -

            Rebased on weekly

            Show
            Andrew Nicols added a comment - Rebased on weekly
            Hide
            Dan Poltawski added a comment -

            Thanks for the patch! My review comments:

            1/ Could you add a bit more detail in the commit message to explain why you are making the change to open in a new window (to prevent the form changed warnings)

            2/ I don't really see why you changed the way the menu items array is populated. So unless there is a reason I wouldn't change it and introduce the extra variable and steps.

            3/ I think the downloadfile function name could be misleading to future developers (it might suggest the JS downloads the file, when its actually only opening a window) so it'd be good if we could think of another name to prevent confusion.

            Show
            Dan Poltawski added a comment - Thanks for the patch! My review comments: 1/ Could you add a bit more detail in the commit message to explain why you are making the change to open in a new window (to prevent the form changed warnings) 2/ I don't really see why you changed the way the menu items array is populated. So unless there is a reason I wouldn't change it and introduce the extra variable and steps. 3/ I think the downloadfile function name could be misleading to future developers (it might suggest the JS downloads the file, when its actually only opening a window) so it'd be good if we could think of another name to prevent confusion.
            Hide
            Martin Dougiamas added a comment -

            Ah, yes I just discovered this problem in the "Private files" filemanager element. +1 to fix

            Show
            Martin Dougiamas added a comment - Ah, yes I just discovered this problem in the "Private files" filemanager element. +1 to fix
            Hide
            Andrew Nicols added a comment -

            Hi Dan,

            Thanks for the feedback. I've done the following:

            • Changed back to defining the hash in the menuitems array as before to reduce the diff
            • Renamed the function to open_file_in_new_window (to use the correct style)
            • Added a note to the commit message

            Andrew

            Show
            Andrew Nicols added a comment - Hi Dan, Thanks for the feedback. I've done the following: Changed back to defining the hash in the menuitems array as before to reduce the diff Renamed the function to open_file_in_new_window (to use the correct style) Added a note to the commit message Andrew
            Hide
            Andrew Nicols added a comment -

            Resubmitting for peer review

            Show
            Andrew Nicols added a comment - Resubmitting for peer review
            Hide
            Dan Poltawski added a comment -

            Looks good to me!

            Show
            Dan Poltawski added a comment - Looks good to me!
            Hide
            Andrew Nicols added a comment -

            Waiting for the updated MDL-31660 to go through Peer Review before submitting for integration

            Show
            Andrew Nicols added a comment - Waiting for the updated MDL-31660 to go through Peer Review before submitting for integration
            Hide
            Dan Poltawski added a comment -

            Thanks for the note on the testing instructions Andrew - thats really helpful for us

            Show
            Dan Poltawski added a comment - Thanks for the note on the testing instructions Andrew - thats really helpful for us
            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
            Andrew Nicols added a comment -

            Rebases cleanly onto latest master

            Show
            Andrew Nicols added a comment - Rebases cleanly onto latest master
            Hide
            Sam Hemelryk added a comment -

            Thanks Andrew, this has been integrated now.
            I wasn't 100% sure whether this should be backported or not so I've opted to integrate only on master.
            If you think it should be backported please let me know.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now. I wasn't 100% sure whether this should be backported or not so I've opted to integrate only on master. If you think it should be backported please let me know. Cheers Sam
            Hide
            Andrew Nicols added a comment -

            Hi Sam,

            I agree - I don't see any real benefit from backporting this to stable branches.

            Thanks,

            Andrew.

            Show
            Andrew Nicols added a comment - Hi Sam, I agree - I don't see any real benefit from backporting this to stable branches. Thanks, Andrew.
            Hide
            Andrew Davis added a comment -

            Works as described. Passing.

            Show
            Andrew Davis added a comment - Works as described. Passing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

            icao_reverse('arreis olik rebemevon afla letoh ognat');
            

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: