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

      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.

        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: