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

      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.

        Gliffy Diagrams

          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: