From 423e5a5fee5e3643bd12bad73de5b0ea524d0c86 Mon Sep 17 00:00:00 2001 From: Peter Dias Date: Tue, 26 Oct 2021 11:20:33 +0800 Subject: [PATCH 1/2] MDL-72816 navigation: Make sure a primary node is active - Revert unnecessary primary_active_tab setters - Modify the navigation nodes to never change text based on default homepage - Make sure a tab is set active --- admin/search.php | 1 - course/index.php | 1 - lib/classes/navigation/views/primary.php | 86 +++++++++++++++------ lib/tests/navigation/views/primary_test.php | 6 +- theme/boost/tests/behat/primarynav.feature | 2 +- 5 files changed, 66 insertions(+), 30 deletions(-) diff --git a/admin/search.php b/admin/search.php index 88c2b35011e..1bcf3c38c17 100644 --- a/admin/search.php +++ b/admin/search.php @@ -60,7 +60,6 @@ if ($data = data_submitted() and confirm_sesskey() and isset($data->action) and } $PAGE->has_secondary_navigation_setter(false); -$PAGE->set_primary_active_tab('siteadminnode'); // and finally, if we get here, then there are matching settings and we have to print a form // to modify them diff --git a/course/index.php b/course/index.php index 6688b5471ff..910530f4351 100644 --- a/course/index.php +++ b/course/index.php @@ -65,7 +65,6 @@ $courserenderer = $PAGE->get_renderer('core', 'course'); $PAGE->set_heading($heading); $content = $courserenderer->course_category($categoryid); -$PAGE->set_primary_active_tab('courses'); $PAGE->set_secondary_active_tab('categorymain'); echo $OUTPUT->header(); diff --git a/lib/classes/navigation/views/primary.php b/lib/classes/navigation/views/primary.php index d970f337c81..5059f1beb3a 100644 --- a/lib/classes/navigation/views/primary.php +++ b/lib/classes/navigation/views/primary.php @@ -33,33 +33,46 @@ class primary extends view { * Initialise the primary navigation node */ public function initialise(): void { - global $CFG; if (during_initial_install() || $this->initialised) { return; } $this->id = 'primary_navigation'; - if (isloggedin() && !isguestuser()) { - $homepage = get_home_page(); - if ($homepage === HOMEPAGE_SITE) { - $this->add(get_string('home'), new \moodle_url('/'), self::TYPE_SYSTEM, - null, 'home', new \pix_icon('i/home', '')); - $this->rootnodes['home'] = $this->add(get_string('myhome'), new \moodle_url('/my/'), - self::TYPE_SETTING, null, 'myhome', new \pix_icon('i/dashboard', '')); - } else if ($homepage === HOMEPAGE_MY) { - $this->add(get_string('myhome'), new \moodle_url('/my/'), self::TYPE_SYSTEM, - null, 'myhome', new \pix_icon('i/dashboard', '')); - $this->rootnodes['home'] = $this->add(get_string('sitehome'), new \moodle_url('/'), - self::TYPE_SETTING, null, 'home', new \pix_icon('i/home', '')); - if (!empty($CFG->defaulthomepage) && ($CFG->defaulthomepage == HOMEPAGE_MY)) { + + // We do not need to change the text for the home/dashboard depending on the set homepage. + $sitehome = $this->add(get_string('home'), new \moodle_url('/'), self::TYPE_SYSTEM, + null, 'home', new \pix_icon('i/home', '')); + if (isloggedin() ) { + if (!isguestuser()) { + if (get_home_page() == HOMEPAGE_MY) { // We need to stop automatic redirection. - $this->rootnodes['home']->action->param('redirect', '0'); + $sitehome->action->param('redirect', '0'); } + + // Add the dashboard link. + $this->add(get_string('myhome'), new \moodle_url('/my/'), + self::TYPE_SETTING, null, 'myhome', new \pix_icon('i/dashboard', '')); } + + // Add the mycourses link. + $this->add(get_string('mycourses'), new \moodle_url('/my/courses.php'), self::TYPE_ROOTNODE, null, 'courses'); } - // Add the mycourses link. - $this->add(get_string('mycourses'), new \moodle_url('/my/courses.php'), self::TYPE_ROOTNODE, null, 'courses'); + if ($node = $this->get_site_admin_node()) { + // We don't need everything from the node just the initial link. + $this->add($node->text, $node->action(), self::TYPE_SITE_ADMIN, null, 'siteadminnode', $node->icon); + } + // Search and set the active node. + $this->set_active_node(); + $this->initialised = true; + } + + /** + * Get the site admin node if available. + * + * @return navigation_node|null + */ + private function get_site_admin_node(): ?navigation_node { // Add the site admin node. We are using the settingsnav so as to avoid rechecking permissions again. $settingsnav = $this->page->settingsnav; $node = $settingsnav->find('siteadministration', self::TYPE_SITE_ADMIN); @@ -68,14 +81,39 @@ class primary extends view { $node = $settingsnav->find('root', self::TYPE_SITE_ADMIN); } - if ($node) { - // We don't need everything from the node just the initial link. - $this->add($node->text, $node->action(), self::TYPE_SITE_ADMIN, null, 'siteadminnode', $node->icon); - } + return $node ?: null; + } - // Search and set the active node. - $this->search_and_set_active_node($this); - $this->initialised = true; + /** + * Find and set the active node. Initially searches based on URL/explicitly set active node. + * If nothing is found, it checks the following: + * - If the node is a site page, set 'Home' as active + * - If within a course context, set 'My courses' as active + * - If within a course category context, set 'Site Admin' (if available) else set 'Home' + * - Else if available set site admin as active + * - Fallback, set 'Home' as active + */ + private function set_active_node(): void { + $activenode = $this->search_and_set_active_node($this); + // If we haven't found an active node based on the standard search. Follow the criteria above. + if (!$activenode) { + $children = $this->get_children_key_list(); + $navactivenode = $this->page->navigation->find_active_node(); + $activekey = 'home'; + if (isset($navactivenode->parent) && $navactivenode->parent->text == get_string('sitepages')) { + $activekey = 'home'; + } else if (in_array($this->context->contextlevel, [CONTEXT_COURSE, CONTEXT_MODULE])) { + $activekey = 'courses'; + } else if (in_array('siteadminnode', $children) && $node = $this->get_site_admin_node()) { + if ($this->context->contextlevel == CONTEXT_COURSECAT || $node->search_for_active_node()) { + $activekey = 'siteadminnode'; + } + } + + if ($activekey && $activenode = $this->find($activekey, null)) { + $activenode->make_active(); + } + } } /** diff --git a/lib/tests/navigation/views/primary_test.php b/lib/tests/navigation/views/primary_test.php index 0564a13321f..b4193c975e2 100644 --- a/lib/tests/navigation/views/primary_test.php +++ b/lib/tests/navigation/views/primary_test.php @@ -61,9 +61,9 @@ class primary_test extends \advanced_testcase { */ public function test_setting_initialise_provider() { return [ - 'Testing as a guest user' => ['guest', ['courses']], - 'Testing as an admin' => ['admin', ['myhome', 'home', 'courses', 'siteadminnode']], - 'Testing as a regular user' => ['user', ['myhome', 'home', 'courses']] + 'Testing as a guest user' => ['guest', ['home', 'courses']], + 'Testing as an admin' => ['admin', ['home', 'myhome', 'courses', 'siteadminnode']], + 'Testing as a regular user' => ['user', ['home', 'myhome', 'courses']] ]; } diff --git a/theme/boost/tests/behat/primarynav.feature b/theme/boost/tests/behat/primarynav.feature index d4a03a26ec7..d170c2164eb 100644 --- a/theme/boost/tests/behat/primarynav.feature +++ b/theme/boost/tests/behat/primarynav.feature @@ -21,7 +21,7 @@ Feature: Primary navigation Examples: | defaulthomepageset | homepage | sitehome | linkelement | | 0 | Home | Home | //a[contains(@class, 'nav-link active') and contains(@tabindex, 0) and not(contains(@href, 'redirect=0'))] | - | 1 | Dashboard | Site home | //a[contains(@class, 'nav-link') and contains(@tabindex, -1) and (contains(@href, 'redirect=0'))] | + | 1 | Dashboard | Home | //a[contains(@class, 'nav-link') and contains(@tabindex, 0) and (contains(@href, 'redirect=0'))] | @javascript @theme_boost Scenario Outline: Admin sets defaulthomepage to 2 and verify the landing page based on user preference set From a7a5f6626acce1c507a4a4e64083f2d19d28686e Mon Sep 17 00:00:00 2001 From: Peter Dias Date: Wed, 27 Oct 2021 10:14:51 +0800 Subject: [PATCH 2/2] MDL-72816 navigation: Make sure a secondary node is active - Clone nodes shared between nav trees before utilising it. --- lib/classes/navigation/views/secondary.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/classes/navigation/views/secondary.php b/lib/classes/navigation/views/secondary.php index b933db44841..591a530c073 100644 --- a/lib/classes/navigation/views/secondary.php +++ b/lib/classes/navigation/views/secondary.php @@ -352,7 +352,7 @@ class secondary extends view { $othernode = $this->get_first_action_for_node($other); // Get the first node and check whether it's been added already. if ($othernode && !$this->get($othernode->key)) { - $this->add_node($othernode); + $this->add_node(clone $othernode); } } } @@ -481,9 +481,9 @@ class secondary extends view { } else { $child->action = new \moodle_url("/admin/search.php", [], "link$child->key"); } - $this->add_node($child); + $this->add_node(clone $child); } else { - $siteadminnode->add_node($child); + $siteadminnode->add_node(clone $child); } } } @@ -502,10 +502,10 @@ class secondary extends view { if (is_string($key)) { $parentnode = $nodes[floor($key)] ?? null; if ($parentnode) { - $parentnode->add_node($node); + $parentnode->add_node(clone $node); } } else { - $this->add_node($node); + $this->add_node(clone $node); } } } @@ -534,7 +534,7 @@ class secondary extends view { // Confirm we have a valid object to add. if ($leftovernode) { - $this->add_node($leftovernode); + $this->add_node(clone $leftovernode); } } }