Moodle
  1. Moodle
  2. MDL-29174

Modules' navigation nodes incorrectly flagged as branches due to stubs

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.2.3
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      1. Add one each of the following types of module to a course: Folder, IMS CP, Lesson, Page, Scorm, Survey, URL.

      2. Look at the navigation block for the added modules. Verify that none of these entries have an expand arrow, and they are all shown as 'leaf' nodes.

      Show
      1. Add one each of the following types of module to a course: Folder, IMS CP, Lesson, Page, Scorm, Survey, URL. 2. Look at the navigation block for the added modules. Verify that none of these entries have an expand arrow, and they are all shown as 'leaf' nodes.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29174-master
    • Rank:
      18722

      Description

      Lines 1591 and 1720 of lib/navigationlib.php flag modules' navigation nodes as being branches (navigation_node::NODETYPE_BRANCH) purely because the module defines a modname_extend_navigation function - although at least in some cases (such as mod/page), the function is just a stub which attempts to set the node to navigation_mode::NODETYPE_LEAF.

      Is there any point in setting them to NODETYPE_BRANCH purely based on having the _extend_navigation function, given that if the _extend_navigation function adds any nodes, the add_node function will flag the node as NODETYPE_BRANCH anyway? Leaving it to add_node to flag it as a branch means that if the _extend_navigation function is either a stub (i.e. about half the standard modules including page, folder, lesson, url and survey) or has conditional logic controlling the addition of child nodes, it'll be correctly flagged as NODETYPE_LEAF unless children are added.

      Incorrectly flagging module nodes with no children as branches results in these nodes having expansion arrows in the navigation block which, when clicked, don't expand the navigation (as there's nothing to add under that node). They should really be drawn without the expansion arrow, as there are no children to expand to.

      You can see this in action by adding a module which contains a stub, such as a Page, to a course. Check the module's node in the navigation block - it'll have an expansion arrow, but won't give any new content if you try to expand it. Adding a forum, which does NOT have the stub (or a full _expand_navigation function), results in a node with no expansion arrow, as expected.

        Activity

        Hide
        Rajesh Taneja added a comment -

        Hello Paul and Sam,

        Current patch doesn't seem to solve the actual issue. In case of page it will be fine but in case of quiz it fails.

        Actually, there seems to be a wrong design usage in page... Following function should be removed.

        /mod/page/lib.php
        function page_extend_navigation($navigation, $course, $module, $cm) {
            /**
             * This is currently just a stub so that it can be easily expanded upon.
             * When expanding just remove this comment and the line below and then add
             * you content.
             */
            $navigation->nodetype = navigation_node::NODETYPE_LEAF;
        }
        

        or
        page_extend_navigation should return bool and module_extends_navigation should check for it.

        lib/navigationlib.php
        $extendingmodules[$modname] = (function_exists($function));
        
        Show
        Rajesh Taneja added a comment - Hello Paul and Sam, Current patch doesn't seem to solve the actual issue. In case of page it will be fine but in case of quiz it fails. Actually, there seems to be a wrong design usage in page... Following function should be removed. /mod/page/lib.php function page_extend_navigation($navigation, $course, $module, $cm) { /** * This is currently just a stub so that it can be easily expanded upon. * When expanding just remove this comment and the line below and then add * you content. */ $navigation->nodetype = navigation_node::NODETYPE_LEAF; } or page_extend_navigation should return bool and module_extends_navigation should check for it. lib/navigationlib.php $extendingmodules[$modname] = (function_exists($function));
        Hide
        Paul Nicholls added a comment -

        Ah - you're right, it doesn't work for activities/resources which genuinely do add nodes (such as quizzes). I confused myself through a combination of navigation caching and using a theme which uses an overridden renderer to include all child nodes (i.e. no AJAX expansion required). The solution's not quite as simple as you suggest, though.

        Removing the stub from the page module (and other affected modules) still wouldn't address the underlying issue, so wouldn't fix the case where a module can add items, but doesn't have any to add (due to the current user not having sufficient capabilities to view any of the items, for example); module_extends_navigation can't call the modname_extend_navigation function as it only takes a module name (whereas the modname_extend_navigation function takes other params as it actually does the extending).

        Basically, the whole premise of flagging an activity/resource node as a branch or leaf based solely on whether the module has a modname_extend_navigation function is flawed. Perhaps we need a second function, modname_will_extend_navigation, which takes the same parameters as modname_extend_navigation but doesn't actually modify anything, instead returning a boolean value. This could then be called instead of blindly flagging the node as a branch - this would still allow AJAX expansion while accurately flagging nodes as branches or leaves. It could be kept reasonably efficient by making sure that you return true as soon as you find a child node which would be added, in order to avoid too much work to determine whether it'd add anything - and any module which knows that it'll always add at least one child can simply return true. As a fallback, we could use the existing blind flagging as a branch in cases where a module has a modname_extend_navigation function but no modname_will_extend_navigation function.

        Show
        Paul Nicholls added a comment - Ah - you're right, it doesn't work for activities/resources which genuinely do add nodes (such as quizzes). I confused myself through a combination of navigation caching and using a theme which uses an overridden renderer to include all child nodes (i.e. no AJAX expansion required). The solution's not quite as simple as you suggest, though. Removing the stub from the page module (and other affected modules) still wouldn't address the underlying issue, so wouldn't fix the case where a module can add items, but doesn't have any to add (due to the current user not having sufficient capabilities to view any of the items, for example); module_extends_navigation can't call the modname_extend_navigation function as it only takes a module name (whereas the modname_extend_navigation function takes other params as it actually does the extending). Basically, the whole premise of flagging an activity/resource node as a branch or leaf based solely on whether the module has a modname_extend_navigation function is flawed. Perhaps we need a second function, modname_will_extend_navigation, which takes the same parameters as modname_extend_navigation but doesn't actually modify anything, instead returning a boolean value. This could then be called instead of blindly flagging the node as a branch - this would still allow AJAX expansion while accurately flagging nodes as branches or leaves. It could be kept reasonably efficient by making sure that you return true as soon as you find a child node which would be added, in order to avoid too much work to determine whether it'd add anything - and any module which knows that it'll always add at least one child can simply return true. As a fallback, we could use the existing blind flagging as a branch in cases where a module has a modname_extend_navigation function but no modname_will_extend_navigation function.
        Hide
        Paul Nicholls added a comment -

        I can't seem to find a way to request another peer review - I've created a new branch, MDL-29174_will_extend_master, which implements my suggestion as above (modname_will_extend_navigation functions). The first commit adds a new function to navigationlib and tweaks other functions there to use it, and the second commit adds the modname_will_extend_navigation functions to all core modules.

        I haven't bothered with branches for 2.0 and 2.1 at this stage, but I imagine it should cherry-pick over cleanly - if not, we can cross that bridge when we get to it.

        Show
        Paul Nicholls added a comment - I can't seem to find a way to request another peer review - I've created a new branch, MDL-29174 _will_extend_master, which implements my suggestion as above (modname_will_extend_navigation functions). The first commit adds a new function to navigationlib and tweaks other functions there to use it, and the second commit adds the modname_will_extend_navigation functions to all core modules. I haven't bothered with branches for 2.0 and 2.1 at this stage, but I imagine it should cherry-pick over cleanly - if not, we can cross that bridge when we get to it.
        Hide
        Sam Hemelryk added a comment -

        Hi Paul,

        I think the reason that you can no longer put it up for peer-review is that the permissions were locked down in tracker the other day to prevent anonymous users from changing the state of issues, I think to do this they limited it just members of the developers group.
        There are two things that we can do now:

        1. Add a comment detailing the location of your patches (git repo, branches, and diff URL's) and then either Raj or myself can put it up for peer-review and/or review it straight away and put it up for integration.
        2. See if we can get you added to the developers group. I'm not sure myself what the rules are on getting added to that group however we can always ask. I'll add Michael to have a look at this issue.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Paul, I think the reason that you can no longer put it up for peer-review is that the permissions were locked down in tracker the other day to prevent anonymous users from changing the state of issues, I think to do this they limited it just members of the developers group. There are two things that we can do now: Add a comment detailing the location of your patches (git repo, branches, and diff URL's) and then either Raj or myself can put it up for peer-review and/or review it straight away and put it up for integration. See if we can get you added to the developers group. I'm not sure myself what the rules are on getting added to that group however we can always ask. I'll add Michael to have a look at this issue. Cheers Sam
        Hide
        Michael de Raadt added a comment -

        Hi, Paul.

        I've added you to the developers group, so you should now be able to request peer reviews.

        Michael;

        Show
        Michael de Raadt added a comment - Hi, Paul. I've added you to the developers group, so you should now be able to request peer reviews. Michael;
        Hide
        Paul Nicholls added a comment -

        New approach - use a modname_will_extend_navigation method (if present) to determine whether it's a branch or a leaf. Method has been added to all core modules.

        Show
        Paul Nicholls added a comment - New approach - use a modname_will_extend_navigation method (if present) to determine whether it's a branch or a leaf. Method has been added to all core modules.
        Hide
        Paul Nicholls added a comment -

        Thanks, Michael and Sam.

        Michael, the "developers" group seems to have given me several other privileges which I didn't have before - perhaps a new group ("external developers" perhaps?) is needed here to provide some of the capabilities (such as requesting a peer review)?

        Cheers,
        Paul

        Show
        Paul Nicholls added a comment - Thanks, Michael and Sam. Michael, the "developers" group seems to have given me several other privileges which I didn't have before - perhaps a new group ("external developers" perhaps?) is needed here to provide some of the capabilities (such as requesting a peer review)? Cheers, Paul
        Hide
        Sam Hemelryk added a comment -

        Hehe one of the other advantages to the developers group is that we can now assign this to you Paul

        Show
        Sam Hemelryk added a comment - Hehe one of the other advantages to the developers group is that we can now assign this to you Paul
        Hide
        Sam Hemelryk added a comment -

        Ping - Raj you've had this in peer-review for quite some time now. Wanna come back to it ?

        Show
        Sam Hemelryk added a comment - Ping - Raj you've had this in peer-review for quite some time now. Wanna come back to it ?
        Hide
        Paul Nicholls added a comment -

        Oops - I may have inadvertently set this to peer review in progress rather than requesting a peer review, due to the changes related to adding me to the developer group. If so, can anyone explain how I should request a peer review now?

        Show
        Paul Nicholls added a comment - Oops - I may have inadvertently set this to peer review in progress rather than requesting a peer review, due to the changes related to adding me to the developer group. If so, can anyone explain how I should request a peer review now?
        Hide
        Rajesh Taneja added a comment -

        Sorry Paul,

        I missed this issue. I am looking at it now and will provide review soon

        Thanks for all the hard work.

        Show
        Rajesh Taneja added a comment - Sorry Paul, I missed this issue. I am looking at it now and will provide review soon Thanks for all the hard work.
        Hide
        Rajesh Taneja added a comment -

        Code looks promising Paul, but I am not sure how it's going to effect the performance and if there can be a better approach to this.
        I am assigning this to Sam as he is the expert in navigation.

        Show
        Rajesh Taneja added a comment - Code looks promising Paul, but I am not sure how it's going to effect the performance and if there can be a better approach to this. I am assigning this to Sam as he is the expert in navigation.
        Hide
        Sam Hemelryk added a comment -

        Hi Paul,

        I've just been looking at this now, sorry for the wait unfortunately as it was marked as Peer-review in progress it slipped under my radar.

        In general I'm not sure about the solution you've come up with.
        It is certainly more accurate however the downside to such a change is the performance implications that are introduced by looking to each module during the building of the course structure by the navigation. Of particular concern is that for many of the modules it would add several database calls to the page per module within a course.

        When the navigation was created originally we decided to limit the generation and guess for the modules as the performance hit checking for every module in a course was huge, particularly in large systems and courses with many modules.
        I think what ever solution we find for this has to be thoughtful of that. Certainly as the performance of the navigation is already a hot topic.

        As for the code you have presently it looks pretty good thanks.
        Looking at it now I think I'd almost be tempted to now implement a new callback function but change the arguments being passed to module_extends_navigation so that the existing extends navigation callback can be improved to allow modules to check more things.

        At this point I think this issue needs a little more thought and work, really I think we need to have a look for solutions that will allow us to achieve greater accuracy without impacting the performance of the site in a serious way.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Paul, I've just been looking at this now, sorry for the wait unfortunately as it was marked as Peer-review in progress it slipped under my radar. In general I'm not sure about the solution you've come up with. It is certainly more accurate however the downside to such a change is the performance implications that are introduced by looking to each module during the building of the course structure by the navigation. Of particular concern is that for many of the modules it would add several database calls to the page per module within a course. When the navigation was created originally we decided to limit the generation and guess for the modules as the performance hit checking for every module in a course was huge, particularly in large systems and courses with many modules. I think what ever solution we find for this has to be thoughtful of that. Certainly as the performance of the navigation is already a hot topic. As for the code you have presently it looks pretty good thanks. Looking at it now I think I'd almost be tempted to now implement a new callback function but change the arguments being passed to module_extends_navigation so that the existing extends navigation callback can be improved to allow modules to check more things. At this point I think this issue needs a little more thought and work, really I think we need to have a look for solutions that will allow us to achieve greater accuracy without impacting the performance of the site in a serious way. Cheers Sam
        Hide
        Paul Nicholls added a comment - - edited

        Hi Sam,
        The _will_extend_navigation() functions I've suggested are based on the modules' respective _extend_navigation() functions - any which use excessive database queries could certainly be simplified to reduce the performance overhead (at the loss of some accuracy, perhaps - but it still ought to be more accurate than it is at present). We could also move the fetching of the activity record (i.e. $activityrecord = $DB->get_record($cm->modname, array('id' => $cm->instance), '*', MUST_EXIST); ) into the _will_extend_navigation functions which actually make use of it, so that it only happens if necessary.

        Cheers,
        Paul

        Show
        Paul Nicholls added a comment - - edited Hi Sam, The _will_extend_navigation() functions I've suggested are based on the modules' respective _extend_navigation() functions - any which use excessive database queries could certainly be simplified to reduce the performance overhead (at the loss of some accuracy, perhaps - but it still ought to be more accurate than it is at present). We could also move the fetching of the activity record (i.e. $activityrecord = $DB->get_record($cm->modname, array('id' => $cm->instance), '*', MUST_EXIST); ) into the _will_extend_navigation functions which actually make use of it, so that it only happens if necessary. Cheers, Paul
        Hide
        Paul Nicholls added a comment -

        Hi Sam,
        Any thoughts on the above? Continuing from my previous comment, I did try to minimise the number of database queries the _will_extend_navigation functions use - i.e. returning a result as soon as we're sure one way or the other. If there are any in particular which you think are using too many DB queries, we could simplify those to reduce performance impact (at the cost of accuracy).

        If you're still concerned about the performance impact, perhaps we can just nuke the _extend_navigation stubs in modules which don't actually extend the nav - while that wouldn't fix the underlying issue, it'd at least make those modules get flagged correctly. Some other modules would still be flagged as branches rather than leaves in some circumstances, though.

        Cheers,
        Paul

        Show
        Paul Nicholls added a comment - Hi Sam, Any thoughts on the above? Continuing from my previous comment, I did try to minimise the number of database queries the _will_extend_navigation functions use - i.e. returning a result as soon as we're sure one way or the other. If there are any in particular which you think are using too many DB queries, we could simplify those to reduce performance impact (at the cost of accuracy). If you're still concerned about the performance impact, perhaps we can just nuke the _extend_navigation stubs in modules which don't actually extend the nav - while that wouldn't fix the underlying issue, it'd at least make those modules get flagged correctly. Some other modules would still be flagged as branches rather than leaves in some circumstances, though. Cheers, Paul
        Hide
        Sam Marshall added a comment -

        Sorry to confuse matters by having another sam jump in - but our users are complaining about this now, we found the issue, and I think there are two separate problems here.

        Problem 1 - Stupid stubs in the code cause 90% (rough estimate) of poor behaviour regarding incorrectly-added branches in the navigation.

        Problem 2 - In 10% of the poor behaviour there are modules which CAN add things to the navigation, but do not always do so, e.g. depending on user permissions or module settings. This is a complex problem to solve because it might involve database queries which are not acceptable at this point of code.

        The title of this bug relates to Problem 1.

        So can we please solve 90% of the problem, under this issue number, by deleting the unnecessary stubs? Then leave the other 10% to a new issue if necessary.

        I am happy to get Ray (a developer on my team) to code and test the deleting-stubs fix for 2.1, 2.2, and master.

        Show
        Sam Marshall added a comment - Sorry to confuse matters by having another sam jump in - but our users are complaining about this now, we found the issue, and I think there are two separate problems here. Problem 1 - Stupid stubs in the code cause 90% (rough estimate) of poor behaviour regarding incorrectly-added branches in the navigation. Problem 2 - In 10% of the poor behaviour there are modules which CAN add things to the navigation, but do not always do so, e.g. depending on user permissions or module settings. This is a complex problem to solve because it might involve database queries which are not acceptable at this point of code. The title of this bug relates to Problem 1. So can we please solve 90% of the problem, under this issue number, by deleting the unnecessary stubs? Then leave the other 10% to a new issue if necessary. I am happy to get Ray (a developer on my team) to code and test the deleting-stubs fix for 2.1, 2.2, and master.
        Hide
        Ray Guo added a comment -

        As Sam Marshall suggested, I have fixed this issue by removing the empty modulename_extent_navigation() functions from the modulename/lib.php.
        There are total 7 modules which has the stub removed. They are folder, imscp ,lesson, page, scorm, survey and url.

        I have fixed this bug in both master and MOODLE_22_STABLE.

        Pull from Repository: git://github.com/raymanuk/moodle.git
        Pull Master Branch: MDL-29174-master
        Pull Master Diff URL:
        https://github.com/raymanuk/moodle/compare/master...MDL-29174-master

        Pull 2.2 Branch: MDL-29174_MOODLE_22
        Pull 2.2 Diff URL:
        https://github.com/raymanuk/moodle/compare/MOODLE_22_STABLE...MDL-29174_MOODLE_22

        Testing Instructions:
        Add url activity on your course (any of the 7 affected modules). On the navigation block of course main page,
        you will see a expandable triangle button before the url activity.
        When you click triangle, nothing is being expanded.
        The correct behaviour should be that there should not be an expandable triangle button
        if the activity has nothing to expand.

        Show
        Ray Guo added a comment - As Sam Marshall suggested, I have fixed this issue by removing the empty modulename_extent_navigation() functions from the modulename/lib.php. There are total 7 modules which has the stub removed. They are folder, imscp ,lesson, page, scorm, survey and url. I have fixed this bug in both master and MOODLE_22_STABLE. Pull from Repository: git://github.com/raymanuk/moodle.git Pull Master Branch: MDL-29174 -master Pull Master Diff URL: https://github.com/raymanuk/moodle/compare/master...MDL-29174-master Pull 2.2 Branch: MDL-29174 _MOODLE_22 Pull 2.2 Diff URL: https://github.com/raymanuk/moodle/compare/MOODLE_22_STABLE...MDL-29174_MOODLE_22 Testing Instructions: Add url activity on your course (any of the 7 affected modules). On the navigation block of course main page, you will see a expandable triangle button before the url activity. When you click triangle, nothing is being expanded. The correct behaviour should be that there should not be an expandable triangle button if the activity has nothing to expand.
        Hide
        Ray Guo added a comment -

        As Sam Marshall suggested, I have fixed this issue by removing the empty modulename_extent_navigation() functions from the modulename/lib.php.
        There are total 7 modules which has the stub removed. They are folder, imscp ,lesson, page, scorm, survey and url.

        I have fixed this bug in both master and MOODLE_22_STABLE.

        Pull from Repository: git://github.com/raymanuk/moodle.git
        Pull Master Branch: MDL-29174-master
        Pull Master Diff URL:
        https://github.com/raymanuk/moodle/compare/master...MDL-29174-master

        Pull 2.2 Branch: MDL-29174_MOODLE_22
        Pull 2.2 Diff URL:
        https://github.com/raymanuk/moodle/compare/MOODLE_22_STABLE...MDL-29174_MOODLE_22

        Testing Instructions:
        Add url activity on your course. On the navigation block of course main page,
        you will see a expandable triangle button before the url activity.
        When you click triangle, nothing is being expanded. The correct behaviour
        should be that there should not be an expandable triangle button
        if the activity has nothing to expand.

        Show
        Ray Guo added a comment - As Sam Marshall suggested, I have fixed this issue by removing the empty modulename_extent_navigation() functions from the modulename/lib.php. There are total 7 modules which has the stub removed. They are folder, imscp ,lesson, page, scorm, survey and url. I have fixed this bug in both master and MOODLE_22_STABLE. Pull from Repository: git://github.com/raymanuk/moodle.git Pull Master Branch: MDL-29174 -master Pull Master Diff URL: https://github.com/raymanuk/moodle/compare/master...MDL-29174-master Pull 2.2 Branch: MDL-29174 _MOODLE_22 Pull 2.2 Diff URL: https://github.com/raymanuk/moodle/compare/MOODLE_22_STABLE...MDL-29174_MOODLE_22 Testing Instructions: Add url activity on your course. On the navigation block of course main page, you will see a expandable triangle button before the url activity. When you click triangle, nothing is being expanded. The correct behaviour should be that there should not be an expandable triangle button if the activity has nothing to expand.
        Hide
        Sam Marshall added a comment -

        Moving Ray's fix into the metadata for the bug, let's see if we can get this one through.

        So as not to lose it, the previous suggested fix (more comprehensive but with possible performance problems) was:

        git://github.com/MaxThrax/moodle.git
        MDL-29174_will_extend_master
        https://github.com/MaxThrax/moodle/compare/master...MDL-29174_will_extend_master

        Show
        Sam Marshall added a comment - Moving Ray's fix into the metadata for the bug, let's see if we can get this one through. So as not to lose it, the previous suggested fix (more comprehensive but with possible performance problems) was: git://github.com/MaxThrax/moodle.git MDL-29174 _will_extend_master https://github.com/MaxThrax/moodle/compare/master...MDL-29174_will_extend_master
        Hide
        Sam Marshall added a comment -

        Change looks OK to me...

        Show
        Sam Marshall added a comment - Change looks OK to me...
        Hide
        Sam Marshall added a comment -

        Submitting for integration.

        As noted this proposed fix resolves the majority of problems caused by this feature by just removing unnecessary stub functions from core modules. A previously-proposed more significant improvement, which allows for modules that can [depending on the situation] either be expandable or not, is both more complex and less important, and can hopefully be addressed later in another issue.

        Show
        Sam Marshall added a comment - Submitting for integration. As noted this proposed fix resolves the majority of problems caused by this feature by just removing unnecessary stub functions from core modules. A previously-proposed more significant improvement, which allows for modules that can [depending on the situation] either be expandable or not, is both more complex and less important, and can hopefully be addressed later in another issue.
        Hide
        Dan Poltawski 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
        Dan Poltawski 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 Hemelryk added a comment -

        Thanks everyone, these changes have been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks everyone, these changes have been integrated now.
        Hide
        Ankit Agarwal added a comment -

        working as expected
        Thanks

        Show
        Ankit Agarwal added a comment - working as expected Thanks
        Hide
        Dan Poltawski added a comment -

        Bonza mate!

        Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

        Hooroo

        Show
        Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

          People

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

            Dates

            • Created:
              Updated:
              Resolved: