Description

To replicate:

1. Create a single activity format course (use a quiz as the activity)
2. Browse to the course
3. Select "Activity" > "Results"
4. Open the "Activity" drop down again and note that there is no tick next to "Results"
5. Another issue here is that there should be a tertiary drop down to access the other quiz reports - not sure why it's not there? It is in multitopic format courses

This happens with other nodes too:

• "Activity" > "Question bank" has the same issue.
• "Activity" > "Logs" (side note: you end up in course context when clicking that - does the report not set its context correctly?).
• "Activity" > "Locally assigned roles" then select "Check permissions" from the tertiary drop down
• Probably more, in the analysis section it'll become clear what the pattern to problematic nodes is

Analysis

NB: A file, results-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.

The code responsible for activating the appropriate nodes is here - the original intention of that code appears to be to find the active node in the secondary navigation tree, then reactivate all its parents (in line with the process explained in the "Navigation overhaul" section of the parent epic). But the implementation is short-sighted and only accounts for the case when there are two levels of nodes.

The implementation itself is a depth first search but with a condition to halt searching deeper nodes. The search will stop exploring deeper nodes of a branch in two situations:

1. The current node key matches an "active key"
2. The current node is active

In many cases, the issue is not apparent as the code behaves like so (* denotes active node state in any diagrams):

1. Assume were are on a page which causes nodes "A" and "B" to become active (or somewhere has called $PAGE->set_secondary_active_tab('A')).
2. Assume the secondary navigation view will reorder everything to the state S->A->B* where S is the secondary navigation view node (or maybe the original nodes were ordered this way)
3. A will be inactive because the act of adding B to it does so (preexisting behaviour from time immemorial)
4. When the secondary navigation view initiates active_node_scan, the scan will get to A, which triggers the termination condition (either because A is actually active, or because we specified we want it to be via $PAGE->set_secondary_active_tab('A'))
   5. A will be activated, and B will never be visited

This is OK because B was never deactivated. So the final state of the tree is S->A*->B* - exactly what we want.

However if there are 3 or more levels of nodes, we run in to issues. Consider:

1. Assume were are on a page which causes nodes "A", "B", and "C" to become active (or somewhere has called $PAGE->set_secondary_active_tab('A')).
2. Assume the secondary navigation view will reorder everything to the state S->A->B->C* where S is the secondary navigation view node (or maybe the original nodes were ordered this way)
3. A and B are now inactive (adding B to A makes A inactive and adding C to B makes B inactive)
4. When the secondary navigation view initiates active_node_scan, it will get to A, which triggers the termination condition (either because A is actually active, or because we specified we want it to be via $PAGE->set_secondary_active_tab('A'))
5. A will be activated, but B and C will never be visited and just retain their original state, i.e., we end up with S->A*>B>C*

This is not OK because now the user will not be shown that B is active in the drop down menu. This is precisely what happens with "Activity" > "Results". The following script can be used to see the issue locally:

<?php

use core\url;

use core\context\course;

use core\navigation\views\view;

require_once('config.php');

$PAGE->set_context(course::instance(2));

$PAGE->set_url('/whatever');

$mknode = fn($key) => navigation_node::create("Node $key", null, navigation_node::TYPE_CUSTOM, null, $key);

$printnode = fn($node) => $node->key . ($node->isactive ? '*' : '');

$myview = new class($PAGE) extends view {

    public function initialise(): void {}

    public function scan($node): void {

        parent::scan_for_active_node($node);

    }

};

$printtree = function($node, string $pre = '') use (&$printtree, $printnode) {

    $children = iterator_to_array($node->children);

    $islast = fn($node): bool => $children[array_key_last($children)]->key === $node->key;

    $nodeline = fn($node): string => $pre . ($islast($node) ? '└' : '├') . $printnode($node);

    $childline = fn($node): string => $node->has_children() ? $printtree($node, $pre . ($islast($node) ? ' ' : '│')) : '';

    $merge = fn(string $nodes, $node): string => $nodes . $nodeline($node) . "\n" . $childline($node);

    return array_reduce($children, $merge, empty($pre) ? ($printnode($node)  . "\n") : '');

};

echo '<pre>';

[$A, $B, $C, $D, $E] = array_map($mknode, ['A', 'B', 'C', 'D', 'E']);

// Make A, B, and C active on this page, and set initial active state.

$A->action = new url('/whatever');

$B->action = new url('/whatever');

$C->action = new url('/whatever');

$A->make_active();

$B->make_active();

$C->make_active();

$A->add_node($B);

$A->add_node($D);

$A->add_node($E);

$B->add_node($C);

echo "Adding B to A deactivated A, and adding C to B deactivated B. Correct behaviour under old navigation paradigm.\n";

echo $printtree($A);

echo "Now calling scan_for_active_node, we should see the ancestors of C reactivated. But the scan stops at node A and never reaches B.\n";

$myview->scan($A);

echo $printtree($A);

echo  "Now reset evertything and change A and B to just be regular nodes that don't activate on this page.\n";

$A->action = null;

$B->action = null;

$A->make_inactive();

$B->make_inactive();

echo $printtree($A);

echo "Run the node scan again. All ancestors of C will be reactivated because the full tree was searched.\n";

$myview->scan($A);

echo $printtree($A);

echo '</pre>';

Additionally, we can use the navigation node debugging tool to see what's happening. Start the navigation node player script using the files from results-single-activity.zip and skip to frame 487. If you look in the secondary navigation tree, the node quiz_report_overview is active (this node becomes active when pressing the "Results" item in the drop down, but it's actually a child of that results node). At this point, by pressing right you can see what the node scan is doing. At frame 840 it will start to look at the "activity" node, then immediately after on frame 841, you can see it doesn't bother to search the children of activity, because the forced secondary active tab is "activity" (the player will show the specific key if it's set, up the top under "Page info>") - as mentioned in MDL-83107, in a single activity format course core's secondary navigation view implementation activates those tabs (see here and here). Meaning that quiz_report_overview stays active, but quiz_report (the "Results" item) is never activated and thus the check mark never appears in the UI.

It appears like a bunch of ideas got conflated when implementing the node scan functionality. Taking a step back, the node scan function appears to have two requirements:

1. Find whatever node is the active node in the tree (there should only be one) and activate all ancestors of that node
2. If a secondary active tab has been specified, activate that

These two requirements are subtly at odds with each other. Assume we have the following tree before the node scan (which should always be the case (i.e., there is only one active node)):

S

├A

│├B

││├C

││└D*

│└E

│ └F

└G

There are a few situations to consider:

1. What should happen when we want to "force activate" A? Should B and D become active? Or just A and nothing else?
2. What should happen if we want to "force activate" C? Surely D should be inactive, but what aouut B and A? Should they become active?
3. What about if we want to activate F? Surely D should deactivate, but what about E and A? Should they become active?

Potential solution

One approach that feels like it makes sense is based on the navigation overhaul section:

1. Find the currently active node (should always exist)
2. If we do not have "forced active" key:
   1. Activate all of its ancestors
3. If we do have a "forced active" key:
   1. Scan for the forced active key
   2. If we find it:
      1. If it is an ancestor of the active node from 1. activate all ancestors of the active node
      2. If it is not an ancestor of the active node from 1. activate it and its ancestors and deactivate the active node from 1.
   3. If we don't find it, activate ancestors of the active node from 1.

The above algorithm would result in the following behavior for each of the following scenarios:

Without forced active key

S                             S                         S

├A                            ├A                        ├A* 

│├B                           │├B                       │├B*

││├C   Find active node (D)   ││├C   Activate lineage   ││├C

││└D* ======================> ││└D* ==================> ││└D*

│└E                           │└E                       │└E

│ └F                          │ └F                      │ └F

└G                            └G                        └G

With a forced active key

To see that this works in a general way, here are a few examples:

Forced key is a sibling ("C" in this example)

S                             S                              S                             S                                S  

├A                            ├A                             ├A                            ├A*                              ├A* 

│├B                           │├B                            │├B                           │├B*                             │├B*

││├C   Find active node (D)   ││├C   Find desired node (C)   ││├C   Activate C's lineage   ││├C*  Deactivate old node (D)   ││├C*

││└D* ======================> ││└D* =======================> ││└D* ======================> ││└D* =========================> ││└D

│└E                           │└E                            │└E                           │└E                              │└E 

│ └F                          │ └F                           │ └F                          │ └F                             │ └F

└G                            └G                             └G                            └G                               └G

Forced key is a first cousin ("E" in this example)

S                             S                              S                             S                                S  

├A                            ├A                             ├A                            ├A*                              ├A* 

│├B                           │├B                            │├B                           │├B*                             │├B*

││├C   Find active node (D)   ││├C   Find desired node (C)   ││├C   Activate C's lineage   ││├C*  Deactivate old node (D)   ││├C*

││└D* ======================> ││└D* =======================> ││└D* ======================> ││└D* =========================> ││└D

│└E                           │└E                            │└E                           │└E                              │└E 

│ └F                          │ └F                           │ └F                          │ └F                             │ └F

└G                            └G                             └G                            └G                               └G

Forced key is a distant relation of active node ("G" in this example)

S                             S                              S                             S                                S  

├A                            ├A                             ├A                            ├A*                              ├A* 

│├B                           │├B                            │├B                           │├B*                             │├B*

││├C   Find active node (D)   ││├C   Find desired node (C)   ││├C   Activate C's lineage   ││├C*  Deactivate old node (D)   ││├C*

││└D* ======================> ││└D* =======================> ││└D* ======================> ││└D* =========================> ││└D

│└E                           │└E                            │└E                           │└E                              │└E 

│ └F                          │ └F                           │ └F                          │ └F                             │ └F

└G                            └G                             └G                            └G                               └G

Forced key is an ancestor of active node ("A" in this example)

This is a special case (3.1.1 in the algorithm), we want to activate a node that has a descendant already active, so we shouldn't deactivate that and just let the ancestors of the active node be activated as normal, as the end result will activate the tab we want.

Forced key is an ancestor of active node (A)

S                             S                              S                             S

├A                            ├A                             ├A                            ├A*

│├B                           │├B                            │├B                           │├B*

││├C   Find active node (D)   ││├C   Find desired node (A)   ││├C   Activate D's lineage   ││├C

││└D* ======================> ││└D* =======================> ││└D* ======================> ││└D*

│└E                           │└E                            │└E                           │└E

│ └F                          │ └F                           │ └F                          │ └F

└G                            └G                             └G                            └G

Forced key doesn't exist ("H" in this example)

S                             S                              S                             S

├A                            ├A                             ├A                            ├A*

│├B                           │├B                            │├B                           │├B*

││├C   Find active node (D)   ││├C   Find desired node (H)   ││├C   Activate D's lineage   ││├C

││└D* ======================> ││└D* =======================> ││└D* ======================> ││└D*

│└E                           │└E                            │└E                           │└E

│ └F                          │ └F                           │ └F                          │ └F

└G                            └G                             └G                            └G

This behaviour seems the most desirable and consistent to me. Further discussion with HQ may be needed to ensure this approach truly is desirable.