Moodle
  1. Moodle
  2. MDL-31015

File/URL resource: 'Open' option does not work in all situations

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Resource
    • Labels:
    • Testing Instructions:
      Hide

      Note: For behaviour to be exactly as described, you have to be using a browser/computer which is capable of displaying PDFs directly in the browser. (This is default for most sysetms.) Otherwise it will download instead.

      Note: Checks marked are unchanged from existing behaviour.

      1. On a test course, ensure that completion is enabled, then add the following activities, all of which should have the default (manual) completion type so they get a tickbox:
      a) A File resource containing a PDF set to display type 'Open'.
      b) A File resource containing a PDF set to display type 'Force download'.
      c) A URL resource (to whatever link) set to display type 'Open'.

      2. Click on the file (a), second file (b), and URL (c) from the main course page. (After the first file and URL you will need to click Back to return to the page.)
      + The PDF (a) should open directly in same browser window without an intermediate page.
      + The PDF (b) should show as a download prompt dialog.
      + When showing the download prompt the browser should NOT briefly display a new tab.
      + The URL (c) should open directly in the same browser window.

      3. Ctrl/Command-click the three links.
      + (a) and (c) should open directly in new tabs.
      + (b) should (in Firefox) show a new empty tab then the download prompt. (Other browsers might get rid of the tab afterwards or something, not sure, but Firefox doesn't. Its call anyhow.)

      4. Expand the navigation block and click on the three links.
      + Results should be the same as in step 2.

      5. Go to the course / Reports / Activity completion. From the completion table, click on the three links.
      + Results should be the same as in step 2.

      Following tests originally from MDL-31247, which this is supposed to fix too

      Using an embeddable media type

      • create a new file resource and upload a PDF
      • set the 'Display' setting to 'Automatic' and save changes
      • click the link for the resource
        • the PDF should embed
      • update the resource and set the Display to 'Embed' and save changes
      • click the link for the resource
        • the PDF should embed
      • update the resource and set the Display to 'Force Download' and save changes
      • click the link for the resource
        • a new window may (or may not) be opened and the file will be downloaded. The browser window should close after accepting the download
      • update the resource and set the Display to 'Open' and save changes
      • click the link for the resource
        • the PDF should open (or download if the browser doesn't support opening in the browser window)
      • update the resource and set the Display to 'In pop-up' and save changes
      • click the link for the resource
        • the PDF should open in a popup window

      Using an non-embeddable media type

      • create a new file resource and upload a zip file
      • set the 'Display' setting to 'Automatic' and save changes
      • click the link for the resource
        • a new window may (or may not) be opened and the file will be downloaded. The browser window should close after accepting the download
      • update the resource and set the Display to 'Embed' and save changes
      • click the link for the resource
        • the file information will be displayed with a download link
        • the download link should download the file
      • update the resource and set the Display to 'Force Download' and save changes
      • click the link for the resource
        • a new window may (or may not) be opened and the file will be downloaded. The browser window should close after accepting the download
      • update the resource and set the Display to 'Open' and save changes
      • click the link for the resource
        • the file should be downloaded
      • update the resource and set the Display to 'In pop-up' and save changes
      • click the link for the resource
        • A popup window will be opened, the file should download, the popup doesn't close (this sucks, but is the existing behaviour)

      Make sure this is tested in all supported browsers

      Show
      Note: For behaviour to be exactly as described, you have to be using a browser/computer which is capable of displaying PDFs directly in the browser. (This is default for most sysetms.) Otherwise it will download instead. Note: Checks marked are unchanged from existing behaviour. 1. On a test course, ensure that completion is enabled, then add the following activities, all of which should have the default (manual) completion type so they get a tickbox: a) A File resource containing a PDF set to display type 'Open'. b) A File resource containing a PDF set to display type 'Force download'. c) A URL resource (to whatever link) set to display type 'Open'. 2. Click on the file (a), second file (b), and URL (c) from the main course page. (After the first file and URL you will need to click Back to return to the page.) + The PDF (a) should open directly in same browser window without an intermediate page. + The PDF (b) should show as a download prompt dialog. + When showing the download prompt the browser should NOT briefly display a new tab. + The URL (c) should open directly in the same browser window. 3. Ctrl/Command-click the three links. + (a) and (c) should open directly in new tabs. + (b) should (in Firefox) show a new empty tab then the download prompt. (Other browsers might get rid of the tab afterwards or something, not sure, but Firefox doesn't. Its call anyhow.) 4. Expand the navigation block and click on the three links. + Results should be the same as in step 2. 5. Go to the course / Reports / Activity completion. From the completion table, click on the three links. + Results should be the same as in step 2. Following tests originally from MDL-31247 , which this is supposed to fix too Using an embeddable media type create a new file resource and upload a PDF set the 'Display' setting to 'Automatic' and save changes click the link for the resource the PDF should embed update the resource and set the Display to 'Embed' and save changes click the link for the resource the PDF should embed update the resource and set the Display to 'Force Download' and save changes click the link for the resource a new window may (or may not) be opened and the file will be downloaded. The browser window should close after accepting the download update the resource and set the Display to 'Open' and save changes click the link for the resource the PDF should open (or download if the browser doesn't support opening in the browser window) update the resource and set the Display to 'In pop-up' and save changes click the link for the resource the PDF should open in a popup window Using an non-embeddable media type create a new file resource and upload a zip file set the 'Display' setting to 'Automatic' and save changes click the link for the resource a new window may (or may not) be opened and the file will be downloaded. The browser window should close after accepting the download update the resource and set the Display to 'Embed' and save changes click the link for the resource the file information will be displayed with a download link the download link should download the file update the resource and set the Display to 'Force Download' and save changes click the link for the resource a new window may (or may not) be opened and the file will be downloaded. The browser window should close after accepting the download update the resource and set the Display to 'Open' and save changes click the link for the resource the file should be downloaded update the resource and set the Display to 'In pop-up' and save changes click the link for the resource A popup window will be opened, the file should download, the popup doesn't close (this sucks, but is the existing behaviour) Make sure this is tested in all supported browsers
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31015-master
    • Rank:
      37421

      Description

      In File and URL resources, one of the display options is 'Open'. When you choose this option, the resource is supposed to open directly without an intermediate page.

      For example, if the file is a PDF, you can have an entry on the main course page that says 'Book 1 (PDF)' and when users click it, the PDF loads immediately in their browser. Similarly, you could have an entry on the main course page that says 'BBC News' and when users click it they are taken immediately to the BBC news website.

      This works by using JavaScript on the main page to load the module's view.php with the extra parameter redirect=1.

      While this works fine on the main page when clicking the link normally it does not work in any other situation. For example:

      • If you click the links in the navigation block, you get the intermediate page.
      • If you middle-click the links to open in a new tab (which bypasses the JavaScript), you get the intermediate page.
      • If you use a link from pretty much any other location in Moodle (these tend to be fairly obscure places), such as the activity completion report, you get the intermediate page.
      • If you use some link in another module (we have a couple of OU custom ones that do this) which redirects to the standard view.php for mod/resource or mod/url, you get the intermediate page.

      This is currently our most frequently reported 'bug' in our Moodle 2 system. Users think 'I just clicked the link to open it - now you're giving me another screen that says please click the link to open it? wtf?'

      It should be easy to solve this problem: in cases where the display type is 'open', automatically default to redirect=1. As there is then no benefit to having the JavaScript, we can also remove the JavaScript (for this case).

      The same applies, but is less annoying, for the 'force download' option. In this case there is some slightly confusing JavaScript which (at least in Firefox) opens a new tab very briefly before the download starts. There is a slight performance benefit to this as it goes directly to the file rather than to the view.php, but it doesn't seem like a significant one and makes the code more complicated, so I suggest removing this too.

      JavaScript is of course still required for the 'pop up' option which I don't propose changing. That one makes a bit more sense to click twice if you have to. There may also be justification for making sure the JavaScript actually works in certain locations (e.g. navigation), but I think the most important fix is to make it work without JavaScript because that solves every case - improving the JS is a 'nice-to-have' for popups.

      Benefits:

      • Makes 'Open' and 'Force download' options on File and (in former case) URL work correctly however the File/URL is accessed.
      • Simplifies code. (Actually reduces line count!)

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Here is the change. I think this is really more a bug than just an improvement, so I've included patches back to 2.1 - the 2.1 version did not cherry-pick cleanly due to other changes, but I did briefly test it after handling the conflict. I'm not sure what the current policy is - as this is not a critical bug it might be preferable to only apply to 2.2 (or even only master), obviously up to HQ to decide.

          Show
          Sam Marshall added a comment - Here is the change. I think this is really more a bug than just an improvement, so I've included patches back to 2.1 - the 2.1 version did not cherry-pick cleanly due to other changes, but I did briefly test it after handling the conflict. I'm not sure what the current policy is - as this is not a critical bug it might be preferable to only apply to 2.2 (or even only master), obviously up to HQ to decide.
          Hide
          Michael de Raadt added a comment -

          Linked to MDL-29987. I'm not sure if this is a duplicate, but the problems seem related.

          Show
          Michael de Raadt added a comment - Linked to MDL-29987 . I'm not sure if this is a duplicate, but the problems seem related.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this a providing a solution.

          I'll try to get you a peer reviewer during the Scrum.

          Show
          Michael de Raadt added a comment - Thanks for working on this a providing a solution. I'll try to get you a peer reviewer during the Scrum.
          Hide
          Tim Hunt added a comment -

          I just had a quick look at the code, and it looks like a good change to me. +1

          I think the best person to peer review this, if he has time, would be Petr, because he originally did resourcelib.php.

          Show
          Tim Hunt added a comment - I just had a quick look at the code, and it looks like a good change to me. +1 I think the best person to peer review this, if he has time, would be Petr, because he originally did resourcelib.php.
          Hide
          Sam Hemelryk added a comment -

          Added Petr as a watcher.. want to check this out dude?

          Show
          Sam Hemelryk added a comment - Added Petr as a watcher.. want to check this out dude?
          Hide
          Sam Hemelryk added a comment -

          Hmm this same area of code has been modified by MDL-31247 and duplicate issue MDL-29272.
          Sam perhaps you'd care to have a look at MDL-31247 before this gets up for integration, if you've no interest or no time not a prob we can look at everything during integration and see about what can be done and where the best solution lies.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hmm this same area of code has been modified by MDL-31247 and duplicate issue MDL-29272 . Sam perhaps you'd care to have a look at MDL-31247 before this gets up for integration, if you've no interest or no time not a prob we can look at everything during integration and see about what can be done and where the best solution lies. Cheers Sam
          Hide
          Sam Marshall added a comment -

          oh dear!

          I looked at MDL-31247. I think my solution is (a) equivalent and (b) better [in removing all JS in these cases] regarding mod/resource/lib.php.

          I think my solution is also slightly tidier (at least a smaller change) in mod/resource/view.php, and think it will achieve the same affect - the 'true' parameter I've set in the case where FORCE_DOWNLOAD is set, should do the same as their redirect to url. (Supposedly - I ddi not actually test it from this perspective.)

          So my suggestion would be to integrate my fix instead of the other one... however it might be a good idea to see what the developer of the other fix thinks as I can imagine a possible difference of opinion there...

          Show
          Sam Marshall added a comment - oh dear! I looked at MDL-31247 . I think my solution is (a) equivalent and (b) better [in removing all JS in these cases] regarding mod/resource/lib.php. I think my solution is also slightly tidier (at least a smaller change) in mod/resource/view.php, and think it will achieve the same affect - the 'true' parameter I've set in the case where FORCE_DOWNLOAD is set, should do the same as their redirect to url. (Supposedly - I ddi not actually test it from this perspective.) So my suggestion would be to integrate my fix instead of the other one... however it might be a good idea to see what the developer of the other fix thinks as I can imagine a possible difference of opinion there...
          Hide
          Andrew Nicols added a comment -

          Hmmm, oh dear indeed.

          Personally, I prefer mine because it moves to using a moodle_url for the redirect. Additionally, it also stops using file_encode_url which is now deprecated.

          Additionally, at present with Force Download as the display type, it's currently possible to view the resource information page if visiting the link directly - e.g. in the 'Recent Activity' blocktype. This would also break.
          As a result with your suggested fix, using the 'Save and display' button doesn't redirect you and causes the file to download instead – This could really confuse users.

          That said, it's probably possible to remove the JavaScript. I did consider it, but kept coming up with bizarre edge case reasons not to do so, then excluding those, and finding another. I decided against removing the JS in the end as there's probably some situation where having it is useful and I just haven't thought of it yet

          Show
          Andrew Nicols added a comment - Hmmm, oh dear indeed. Personally, I prefer mine because it moves to using a moodle_url for the redirect. Additionally, it also stops using file_encode_url which is now deprecated. Additionally, at present with Force Download as the display type, it's currently possible to view the resource information page if visiting the link directly - e.g. in the 'Recent Activity' blocktype. This would also break. As a result with your suggested fix, using the 'Save and display' button doesn't redirect you and causes the file to download instead – This could really confuse users. That said, it's probably possible to remove the JavaScript. I did consider it, but kept coming up with bizarre edge case reasons not to do so, then excluding those, and finding another. I decided against removing the JS in the end as there's probably some situation where having it is useful and I just haven't thought of it yet
          Hide
          Andrew Nicols added a comment -

          Following on from my previous comment:

          If you remove the JS and, following the reasoning that it's important to still make it possible to get to the resource view page (primarily for the Save and View option), then you have to somehow find a way of changing the URL for certain situations to something that will automatically download or to not download – my solution was to use the redirect param to mod/resource/view.php.
          At present, the URL for an activity module is obtained using $mod->get_url(). Not sure whether the ->url is used anywhere or everywhere else though - still trying to dig down and work it out.

          Patch-wise, I also notice that in mod/resource/view.php you've also moved the resource_get_final_display_type() call further up in the code. I discovered when I did this, that the result of this will differ depending upon whether $resource->mainfile is set.
          I think it only changed when the displaytype was set to Automatic, and changed whether an embeddable type was correctly embedded following the change.

          Show
          Andrew Nicols added a comment - Following on from my previous comment: If you remove the JS and, following the reasoning that it's important to still make it possible to get to the resource view page (primarily for the Save and View option), then you have to somehow find a way of changing the URL for certain situations to something that will automatically download or to not download – my solution was to use the redirect param to mod/resource/view.php. At present, the URL for an activity module is obtained using $mod->get_url(). Not sure whether the ->url is used anywhere or everywhere else though - still trying to dig down and work it out. Patch-wise, I also notice that in mod/resource/view.php you've also moved the resource_get_final_display_type() call further up in the code. I discovered when I did this, that the result of this will differ depending upon whether $resource->mainfile is set. I think it only changed when the displaytype was set to Automatic, and changed whether an embeddable type was correctly embedded following the change.
          Hide
          Sam Marshall added a comment -

          1. Imo, it's reasonable for 'save and display' to download the file immediately if it is set to force download. You're creating a File object, save and display results in the file... Doesn't seem surprising

          2. Imo, the ability to access the view page via 'Recent activity' (or navigation, or whatever) is a bug, not a feature. If that was intended then it should be given as a distinct option with different text like (File details) or something. If the link has the same text etc as the main page one then it should open the same way, imo.

          3. In the cases where this change skips it, I don't think the view page ever provides any useful information (just has link you need to click to get the file/URL). Supposing it did provide useful information, I don't think it would be acceptable to have it so that users don't see that information if they click the link via the most obvious method (main page), would it?

          Regarding get_final_display_type - oh dear, OK I may have broken that, can check...

          Show
          Sam Marshall added a comment - 1. Imo, it's reasonable for 'save and display' to download the file immediately if it is set to force download. You're creating a File object, save and display results in the file... Doesn't seem surprising 2. Imo, the ability to access the view page via 'Recent activity' (or navigation, or whatever) is a bug, not a feature. If that was intended then it should be given as a distinct option with different text like (File details) or something. If the link has the same text etc as the main page one then it should open the same way, imo. 3. In the cases where this change skips it, I don't think the view page ever provides any useful information (just has link you need to click to get the file/URL). Supposing it did provide useful information, I don't think it would be acceptable to have it so that users don't see that information if they click the link via the most obvious method (main page), would it? Regarding get_final_display_type - oh dear, OK I may have broken that, can check...
          Hide
          Sam Marshall added a comment -

          I have rebased to current code (all 3 branches) and applied fixes based on Andrew's suggestions:

          1) Changed file_encode_url to use the current non-deprecated moodle_url function.

          2) Fixed bug he pointed out where $file->mainfile was not set before calling function.

          3) Despite what I said in last comment he is totally right about 'save and display' behaviour being confusing I changed it so that it does not automatically redirect when coming from the modedit form.

          Just to reiterate, the main effect of this change (different to MDL-31247) is that it removes the intermediate page in the cases where previously, the intermediate page was removed if you clicked from the main page but was still present if you clicked from navigation, ctrl-clicked, or did something else.

          And the main rationale for this is that (a) this behaviour annoys our users a lot, and (b) given that the intermediate page does not display in the 'main' case (clicking the link from the course page), using it for anything important would be a very bad idea anyway, so there is no need to display it.

          Show
          Sam Marshall added a comment - I have rebased to current code (all 3 branches) and applied fixes based on Andrew's suggestions: 1) Changed file_encode_url to use the current non-deprecated moodle_url function. 2) Fixed bug he pointed out where $file->mainfile was not set before calling function. 3) Despite what I said in last comment he is totally right about 'save and display' behaviour being confusing I changed it so that it does not automatically redirect when coming from the modedit form. Just to reiterate, the main effect of this change (different to MDL-31247 ) is that it removes the intermediate page in the cases where previously, the intermediate page was removed if you clicked from the main page but was still present if you clicked from navigation, ctrl-clicked, or did something else. And the main rationale for this is that (a) this behaviour annoys our users a lot, and (b) given that the intermediate page does not display in the 'main' case (clicking the link from the course page), using it for anything important would be a very bad idea anyway, so there is no need to display it.
          Hide
          Sam Marshall added a comment -

          note: we are about to deploy this on our systems on Tuesday. (Our policy is to wait until things are approved and included in core before patching them into our own system, but this one is so important that we can't wait for it.) So we should get some extra bonus live-system testing.

          Show
          Sam Marshall added a comment - note: we are about to deploy this on our systems on Tuesday. (Our policy is to wait until things are approved and included in core before patching them into our own system, but this one is so important that we can't wait for it.) So we should get some extra bonus live-system testing.
          Hide
          Aparup Banerjee added a comment - - edited

          looks good to me but perhaps Andrew Nicols should peer review this actually

          ps: the test instructions @ MDL-31247 could be merged here perhaps?

          Show
          Aparup Banerjee added a comment - - edited looks good to me but perhaps Andrew Nicols should peer review this actually ps: the test instructions @ MDL-31247 could be merged here perhaps?
          Hide
          Sam Marshall added a comment -

          Andrew did review it (he spotted 2 things wrong), dunno if he wants to check again. Agree re merging test instructions.

          Show
          Sam Marshall added a comment - Andrew did review it (he spotted 2 things wrong), dunno if he wants to check again. Agree re merging test instructions.
          Hide
          Andrew Nicols added a comment -

          The new patch looks good to me and addresses the 'Save and Display' issue I was concerned about.

          I've tested various combinations and I think it's good to go

          Show
          Andrew Nicols added a comment - The new patch looks good to me and addresses the 'Save and Display' issue I was concerned about. I've tested various combinations and I think it's good to go
          Hide
          Sam Marshall added a comment -

          Thank you Andrew. Submitting for integration review.

          Show
          Sam Marshall added a comment - Thank you Andrew. Submitting for integration review.
          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
          Sam Marshall added a comment -

          Rebased (all 3 branches).

          Show
          Sam Marshall added a comment - Rebased (all 3 branches).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks perfect, integrated (21, 22 & master). Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Looks perfect, integrated (21, 22 & master). Thanks!
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: