Moodle
  1. Moodle
  2. MDL-29224

Navigation Block doesn't respect a URL activities settings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      add a URL to the main menu block on the site homepage - set it to automatic.
      Click on the link in the main menu block - make sure it auto redirects to the page and doesn't show an intermediary splash page.
      Go back to Homepage and select the same link but within the site pages area of the Navigation block - make sure the page redirects and doesn't show an intermediary splash page

      Show
      add a URL to the main menu block on the site homepage - set it to automatic. Click on the link in the main menu block - make sure it auto redirects to the page and doesn't show an intermediary splash page. Go back to Homepage and select the same link but within the site pages area of the Navigation block - make sure the page redirects and doesn't show an intermediary splash page
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-29224-master
    • Rank:
      18767

      Description

      If you add a URL to the main menu block on the site home page a link to that same URL is shown in the Navigation block under site pages.

      Problem is the behavior of this link differs to the one in the main menu block.
      click on link in main menu you are redirected to the page correctly.
      click on the link in navigation block under site pages you are taken to a splash page that displays the url to click on.

      The main menu block has some extra js on the "onclick" event for the url that has the param redirect=1 which is missing in the navigation block.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this, Dan.

          Show
          Michael de Raadt added a comment - Thanks for reporting this, Dan.
          Hide
          Sam Hemelryk added a comment -

          Hi Dan,

          I've just been looking at this now and havn't been able to replicate it in either 2.1.1+ or master.
          From what I can see the new cm_info object is responsible for producing those URL's and there's no premis I could find for the redirect param you mention.
          Would you mind giving it another shot for me and seeing if you are able to replicate it yourself still?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dan, I've just been looking at this now and havn't been able to replicate it in either 2.1.1+ or master. From what I can see the new cm_info object is responsible for producing those URL's and there's no premis I could find for the redirect param you mention. Would you mind giving it another shot for me and seeing if you are able to replicate it yourself still? Cheers Sam
          Hide
          Dan Marsden added a comment -

          hmm - looks like the "automatic" setting for urls has been broken - just added a link on demo.moodle.net and I'm getting a splash page when clicking on the link in the block too.

          Show
          Dan Marsden added a comment - hmm - looks like the "automatic" setting for urls has been broken - just added a link on demo.moodle.net and I'm getting a splash page when clicking on the link in the block too.
          Hide
          Dan Marsden added a comment -

          sorry - it's working but I had something weird going on with my browser

          I can confirm this on demo.moodle.net as still an issue. (haven't checked master code)

          Step 1 - add Main menu block to homepage
          step 2 - add link to a course within the site as a url - eg on demo.moodle.net add a url for "http://demo.moodle.net/course/view.php?id=5"

          the link inside the block will take you directly to that page listed

          but... if you click on the link in the navigation block you are taken to a splash page.

          Show
          Dan Marsden added a comment - sorry - it's working but I had something weird going on with my browser I can confirm this on demo.moodle.net as still an issue. (haven't checked master code) Step 1 - add Main menu block to homepage step 2 - add link to a course within the site as a url - eg on demo.moodle.net add a url for "http://demo.moodle.net/course/view.php?id=5" the link inside the block will take you directly to that page listed but... if you click on the link in the navigation block you are taken to a splash page.
          Hide
          Dan Marsden added a comment -

          here's a paste of the html in the nav block and main menu block so you can see the difference:
          http://paste.dollyfish.net.nz/b5709b

          Show
          Dan Marsden added a comment - here's a paste of the html in the nav block and main menu block so you can see the difference: http://paste.dollyfish.net.nz/b5709b
          Hide
          Sam Hemelryk added a comment -

          Thanks Dan - perfectly illustrates the problem.

          That is the first time I've seen the onclick property ... puke
          I've worked out a solution now that allows us to use this attribute and doesn't involve introducing more inline JS actions instead uses YUI and action links.
          I've put it up for peer-review now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Dan - perfectly illustrates the problem. That is the first time I've seen the onclick property ... puke I've worked out a solution now that allows us to use this attribute and doesn't involve introducing more inline JS actions instead uses YUI and action links. I've put it up for peer-review now. Cheers Sam
          Hide
          Dan Marsden added a comment -

          yeah - I didn't like it much either - I thought it would have been better to add a check on view.php and then do a redirect before printing any data... but that might be a bit more complicated than your patch! - thanks.

          Show
          Dan Marsden added a comment - yeah - I didn't like it much either - I thought it would have been better to add a check on view.php and then do a redirect before printing any data... but that might be a bit more complicated than your patch! - thanks.
          Hide
          Sam Hemelryk added a comment -

          Putting this up for integration now

          Show
          Sam Hemelryk added a comment - Putting this up for integration now
          Hide
          Aparup Banerjee added a comment -

          lol @ pukes
          no 100% idea but here's to hoping no undesired propagation halts occur with 'strpos($activity->onclick, 'return false')' , anyway i can't imagine URL gets too complex.

          Integrated and up for testing!

          Show
          Aparup Banerjee added a comment - lol @ pukes no 100% idea but here's to hoping no undesired propagation halts occur with 'strpos($activity->onclick, 'return false')' , anyway i can't imagine URL gets too complex. Integrated and up for testing!
          Hide
          Rossiani Wijaya added a comment -

          With display is set to 'automatic'

          From navigation block, link redirects properly

          However, selecting from main menu block, it goes to splash page that displays the url to click on.

          This happens in all 3 branches. I asked Apu to test it on his distribution and got the same result.

          I'm failing this test. sorry.

          Show
          Rossiani Wijaya added a comment - With display is set to 'automatic' From navigation block, link redirects properly However, selecting from main menu block, it goes to splash page that displays the url to click on. This happens in all 3 branches. I asked Apu to test it on his distribution and got the same result. I'm failing this test. sorry.
          Hide
          Dan Marsden added a comment -

          Hi Rosie,

          I haven't checked the patch... but is the URL you are adding a url to a page within the Moodle site or is it an external link - eg google?

          The original report was related to links to pages inside the Moodle site. - External links like Google were still showing the splash page - which may be a seperate bug? - or expected behaviour?

          Show
          Dan Marsden added a comment - Hi Rosie, I haven't checked the patch... but is the URL you are adding a url to a page within the Moodle site or is it an external link - eg google? The original report was related to links to pages inside the Moodle site. - External links like Google were still showing the splash page - which may be a seperate bug? - or expected behaviour?
          Hide
          Rossiani Wijaya added a comment -

          Hi Dan,

          I tested using both moodle site and external links. Both links have the same result.
          These are the link I used to test:
          1. moodle site: http://rwijaya.moodle.local/course/view.php?id=2 and /course/view.php?id=2
          2. external : http://cnn.com

          Show
          Rossiani Wijaya added a comment - Hi Dan, I tested using both moodle site and external links. Both links have the same result. These are the link I used to test: 1. moodle site: http://rwijaya.moodle.local/course/view.php?id=2 and /course/view.php?id=2 2. external : http://cnn.com
          Hide
          Sam Hemelryk added a comment -

          I've just been testing this as well, indeed Rosie is correct there is a bug in play here - although after a bit of testing it is not brought out by these changes but in fact by something that has changed within master only.

          Testing in MOODLE_20_STABLE and MOODLE_21_STABLE things work perfectly as expected, and behind the scenes with a bit of investigation into the cm object we see the following:

          $cm->extra = onclick="window.location.href ='http://integration.sam.local/mod/url/view.php?id=3&redirect=1';return false;";
          $cm->get_on_click() = '';
          

          In master we can see that the main menu block no longer jumps straight to the URL whereas all other areas do. Inspecting $cm again we now see the following:

          $cm->extra = '';
          $cm->get_on_click() = window.location.href ='http://integration.sam.local/mod/url/view.php?id=3&redirect=1';return false;
          

          So pretty obvious problem here, in master (obviously because someone realised onclick was being set on extra) things have been switched over.

          Looking at how the method print_section() is working it includes $cm->extra and then if there is a result from get_on_click() it builds that into JS event and uses it as well - essentially it takes into account both.
          The site main menu presently only looks at $cm->extra thus in master the onclick has stopped working.
          The navigation uses get_on_click() only.

          I think after looking at this that this is a new bug (regression) within the site_main_menu block on master only. Personally I'd rather see a new issue created to fix it than try to morph this bug into fixing both issues now that it has been integrated. However as the integrator Apu it is you call and I'll be happy either way.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - I've just been testing this as well, indeed Rosie is correct there is a bug in play here - although after a bit of testing it is not brought out by these changes but in fact by something that has changed within master only. Testing in MOODLE_20_STABLE and MOODLE_21_STABLE things work perfectly as expected, and behind the scenes with a bit of investigation into the cm object we see the following: $cm->extra = onclick= "window.location.href ='http: //integration.sam.local/mod/url/view.php?id=3&redirect=1'; return false ;" ; $cm->get_on_click() = ''; In master we can see that the main menu block no longer jumps straight to the URL whereas all other areas do. Inspecting $cm again we now see the following: $cm->extra = ''; $cm->get_on_click() = window.location.href ='http: //integration.sam.local/mod/url/view.php?id=3&redirect=1'; return false ; So pretty obvious problem here, in master (obviously because someone realised onclick was being set on extra) things have been switched over. Looking at how the method print_section() is working it includes $cm->extra and then if there is a result from get_on_click() it builds that into JS event and uses it as well - essentially it takes into account both. The site main menu presently only looks at $cm->extra thus in master the onclick has stopped working. The navigation uses get_on_click() only. I think after looking at this that this is a new bug (regression) within the site_main_menu block on master only. Personally I'd rather see a new issue created to fix it than try to morph this bug into fixing both issues now that it has been integrated. However as the integrator Apu it is you call and I'll be happy either way. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 for followup and keep integration (up to Apa to decide, anyway)

          Niao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 for followup and keep integration (up to Apa to decide, anyway) Niao
          Hide
          Aparup Banerjee added a comment - - edited

          I'm ok for it to be kept in considering its not causing more regressions and will be followed up. (link the issue)
          Rosie, resetting this for testing, could you verify by testing that it works for STABLE as Sam reckons.

          ps: not a flaw seen in the code either
          pps: this time i tested with 'internal' url (entered into 'external url field lol) hehe works for me

          Show
          Aparup Banerjee added a comment - - edited I'm ok for it to be kept in considering its not causing more regressions and will be followed up. (link the issue) Rosie, resetting this for testing, could you verify by testing that it works for STABLE as Sam reckons. ps: not a flaw seen in the code either pps: this time i tested with 'internal' url (entered into 'external url field lol) hehe works for me
          Hide
          Sam Hemelryk added a comment -

          Alright - Rosie has done an excellent job testing this little but terrifying bug! (Rosie gets the gold star today for sure!)

          So Apu first up:
          Rosie found that the navigation changes weren't working in 20 and 21 stable because cm->extra contains the onclick event rather than cm->get_on_click.
          I have created two new branches that should merge straight onto integration for you to get (if you have trouble merging let me know and I'll get you the commit hashes so you can cherry-pick):
          Repo: git://github.com/samhemelryk/moodle.git
          20 Branch: wip-MDL-29224-m20-integration
          21 Branch: wip-MDL-29224-m21-integration

          While testing this there have been two other bugs found, one with the main menu block, and the other with the url module. They are as follows:

          1. The site main menu block in master uses $cm->extra however in master the onclick is handled properly and the site main menu block needs to use both $cm->extra and $cm->get_on_click(). To the developer have a look at what print_section does.
          2. The URL module in all branches should include URL's ending with '/' when searching for internal links. Without this check links to something like http://yoursite.local/moodle/course/ will be processed incorrectly and avoid having the onclick/extra fields set.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Alright - Rosie has done an excellent job testing this little but terrifying bug! (Rosie gets the gold star today for sure!) So Apu first up: Rosie found that the navigation changes weren't working in 20 and 21 stable because cm->extra contains the onclick event rather than cm->get_on_click. I have created two new branches that should merge straight onto integration for you to get (if you have trouble merging let me know and I'll get you the commit hashes so you can cherry-pick): Repo: git://github.com/samhemelryk/moodle.git 20 Branch: wip- MDL-29224 -m20-integration 21 Branch: wip- MDL-29224 -m21-integration While testing this there have been two other bugs found, one with the main menu block, and the other with the url module. They are as follows: The site main menu block in master uses $cm->extra however in master the onclick is handled properly and the site main menu block needs to use both $cm->extra and $cm->get_on_click(). To the developer have a look at what print_section does. The URL module in all branches should include URL's ending with '/' when searching for internal links. Without this check links to something like http://yoursite.local/moodle/course/ will be processed incorrectly and avoid having the onclick/extra fields set. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Thanks Sam, integrated those additional patches for stable 2.x after preliminary testing with Rosie , up for proper integration testing now.

          Show
          Aparup Banerjee added a comment - Thanks Sam, integrated those additional patches for stable 2.x after preliminary testing with Rosie , up for proper integration testing now.
          Hide
          Rossiani Wijaya added a comment -

          Re-tested the issue with Sam's latest patch and it works great.

          Thanks Sam for quickly updating the patch and Apu for integrated the patch immediately.

          ps: master branch will be fixed separately from this issue.

          Test passed.

          *claiming gold star from Sam. YAYYYY!!!!

          Show
          Rossiani Wijaya added a comment - Re-tested the issue with Sam's latest patch and it works great. Thanks Sam for quickly updating the patch and Apu for integrated the patch immediately. ps: master branch will be fixed separately from this issue. Test passed. *claiming gold star from Sam. YAYYYY!!!!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work developing and testing this. It has been spread to cvs and git upstream repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work developing and testing this. It has been spread to cvs and git upstream repositories. Closing, ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: