-
Bug
-
Resolution: Unresolved
-
Minor
-
None
-
4.3.7, 4.4.3
-
MOODLE_403_STABLE, MOODLE_404_STABLE
The algorithm implemented to reorder nodes has at least two flaws in it.
Flaw 1
- Create a single activity format course (use a quiz as the activity)
- Browse to the course
- Under the "Activity" dropdown, observe the URL of the question bank
- Under the "Course" dropdown, observe the URL of the question bank
- Notice that the URLs are the same - the should be different, one should take you to the question bank in course context, and the other one should take you to the question bank in module context
Flaw 2 (originally MDL-80779)
- Create a normal course with a quiz
- Browse to the quiz and observe the "Results" item in secondary navigation
- Install the local_navigationobliterator plugin attached to this issue (unzip the contents in to your site's "local" directory)
- Upgrade the site: php admin/cli/upgrade.php
- Look at the quiz again
- Notice that the "Results" tab is gone
Impacts
Flaw 1 has a significant impact on accessibility, as when you click the "Question bank" item under course, it takes you to the "Activity" question bank, with the "Activity" menu being highlighted (you may note that the checkmark is missing from the "Question bank" item in the dropdown - this is a separate issue which should he handled in MDL-83107). This is an issue for anyone using assistive technologies. It also renders an entire part of the course inaccessible via the menus.
Flaw 2 has a lesser impact, but an impact nonetheless on any plugin that attempts to customise the navigation by adding nodes.
Analysis
NB: A file, qbank-single-activity.zip, has been attached to this issue with contains a dump of the navigation tree state when view the quiz module in a single activity course. Please see the "Supporting utilities" section of the parent epic to understand how to interact with these files.
Flaw 1 is due to the existence of two questionbank nodes in settingsnav when in single activity format (one for the course and one for the module). Start the navigation node player script using the files from qbank-single-activity.zip and skip to frame 252. If you use 'h' to highlight "questionbank" you'll see there are two, the first one is for the module level question bank. When plugins specify the sort order the want for nodes, they are only given the ability to specify a key. Core's implementation of the secondary navigation view specifies that it wants to reorder the questionbank node (see here) - the issue is that the code responsible for finding this node just does a naive ->find and doesn't account for the case when there is more than one matching node (see here), so the first one always gets returned regardless of whether the reordering is being done for the Activity dropdown or the Course dropdown.
Flaw 2 is due to a case where multiple nodes having the same sort order. The obliterator plugin adds a node called mod_quiz_groupoverrides which happens to be given a sort order of 4 in the default secondary navigation view implementation. However when looking at a quiz module, quiz's secondary navigation view implementation is used, and it merges the default sort orders with its own. Quiz's implementation specifies the report node as having sort order 4 also.
The result of the merge is that there are now two nodes both wanting sort order 4. And the code responsible for sorting the nodes uses these sort orders as the key of an array, and the code explicitly skips any nodes that have already been set in that array.
Note that a patch for flaw 2 does exist in MDL-80779 which may be re-used here. But we should also address flaw 1 at the same time.