Skip to content

Commit

Permalink
MDL-73645 theme_boost: Keep section breadcumb item in specific layouts
Browse files Browse the repository at this point in the history 
Avoids removing the section breadcrumb nodes in the course module
context when the course uses the 'One section per page' layout.
Additionaly, it also removes the Courses and Category breadcrumb
nodes in the module contexts to be consistent with the way the
breadcrumbs are displayed in the course context.
  • Loading branch information
Mihail Geshoski committed Mar 2, 2022
1 parent 3e24811 commit 2a09a3f
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 31 deletions.
22 changes: 18 additions & 4 deletions theme/boost/classes/boostnavbar.php
View file
Expand Up @@ -55,6 +55,9 @@ public function __construct(\moodle_page $page) {
protected function prepare_nodes_for_boost(): void {
global $PAGE;

// Defines whether section items with an action should be removed by default.
$removesections = true;

if ($this->page->context->contextlevel == CONTEXT_COURSE) {
// Remove any duplicate navbar nodes.
$this->remove_duplicate_items();
Expand Down Expand Up @@ -90,6 +93,14 @@ protected function prepare_nodes_for_boost(): void {
// Remove 'My courses' if we are in the module context.
if ($this->page->context->contextlevel == CONTEXT_MODULE) {
$this->remove('mycourses');
$this->remove('courses');
// Remove the course category breadcrumb node.
$this->remove($this->page->course->category);
$courseformat = course_get_format($this->page->course)->get_course();
// Section items can be only removed if a course layout (coursedisplay) is not explicitly set in the
// given course format or the set course layout is not 'One section per page'.
$removesections = !isset($courseformat->coursedisplay) ||
$courseformat->coursedisplay != COURSE_DISPLAY_MULTIPAGE;
}

if (!is_null($this->get_item('root'))) { // We are in site administration.
Expand All @@ -107,7 +118,7 @@ protected function prepare_nodes_for_boost(): void {
$mycoursesnode->text = get_string('mycourses');
}

$this->remove_no_link_items();
$this->remove_no_link_items($removesections);

// Don't display the navbar if there is only one item. Apparently this is bad UX design.
if ($this->item_count() <= 1) {
Expand Down Expand Up @@ -212,13 +223,16 @@ public function get_penultimate_item(): ?\breadcrumb_navigation_node {
}

/**
* Remove items that are categories or have no actions associated with them.
* Remove items that have no actions associated with them and optionally remove items that are sections.
*
* The only exception is the last item in the list which may not have a link but needs to be displayed.
*
* @param bool $removesections Whether section items should be also removed (only applies when they have an action)
*/
protected function remove_no_link_items(): void {
protected function remove_no_link_items(bool $removesections = true): void {
foreach ($this->items as $key => $value) {
if (!$value->is_last() && (!$value->has_action() || $value->type == \navigation_node::TYPE_SECTION)) {
if (!$value->is_last() &&
(!$value->has_action() || ($value->type == \navigation_node::TYPE_SECTION && $removesections))) {
unset($this->items[$key]);
}
}
Expand Down
113 changes: 86 additions & 27 deletions theme/boost/tests/boostnavbar_test.php
View file
Expand Up @@ -33,12 +33,13 @@ class boostnavbar_test extends \advanced_testcase {
*/
public function remove_no_link_items_provider(): array {
return [
'All nodes have links links including leaf node' => [
'All nodes have links links including leaf node. Set to remove section nodes.' => [
[
'node1' => true,
'node2' => true,
'node3' => true,
'node1' => ['hasaction' => true, 'issection' => false],
'node2' => ['hasaction' => true, 'issection' => false],
'node3' => ['hasaction' => true, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
Expand All @@ -48,12 +49,13 @@ public function remove_no_link_items_provider(): array {
'node3' => true,
]
],
'Only some parent nodes have links. Leaf node has a link.' => [
'Only some parent nodes have links. Leaf node has a link. Set to remove section nodes.' => [
[
'node1' => false,
'node2' => true,
'node3' => true,
'node1' => ['hasaction' => false, 'issection' => false],
'node2' => ['hasaction' => true, 'issection' => false],
'node3' => ['hasaction' => true, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
Expand All @@ -62,25 +64,27 @@ public function remove_no_link_items_provider(): array {
'node3' => true,
]
],
'All parent nodes do not have links. Leaf node has a link.' => [
'All parent nodes do not have links. Leaf node has a link. Set to remove section nodes.' => [
[
'node1' => false,
'node2' => false,
'node3' => true,
'node1' => ['hasaction' => false, 'issection' => false],
'node2' => ['hasaction' => false, 'issection' => false],
'node3' => ['hasaction' => true, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
'tc_1' => true,
'node3' => true,
]
],
'All parent nodes have links. Leaf node does not has a link.' => [
'All parent nodes have links. Leaf node does not has a link. Set to remove section nodes.' => [
[
'node1' => true,
'node2' => true,
'node3' => false,
'node1' => ['hasaction' => true, 'issection' => false],
'node2' => ['hasaction' => true, 'issection' => false],
'node3' => ['hasaction' => false, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
Expand All @@ -90,31 +94,84 @@ public function remove_no_link_items_provider(): array {
'node3' => false,
]
],
'All parent nodes do not have links. Leaf node does not has a link.' => [
'All parent nodes do not have links. Leaf node does not has a link. Set to remove section nodes.' => [
[
'node1' => false,
'node2' => false,
'node3' => false,
'node1' => ['hasaction' => false, 'issection' => false],
'node2' => ['hasaction' => false, 'issection' => false],
'node3' => ['hasaction' => false, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
'tc_1' => true,
'node3' => false,
]
],
'Some parent nodes do not have links. Leaf node does not has a link.' => [
'Some parent nodes do not have links. Leaf node does not has a link. Set to remove section nodes.' => [
[
'node1' => ['hasaction' => true, 'issection' => false],
'node2' => ['hasaction' => false, 'issection' => false],
'node3' => ['hasaction' => false, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
'tc_1' => true,
'node1' => true,
'node2' => false,
'node3' => false,
]
],
'All nodes have links links including leaf node and section nodes. Set to remove section nodes.' => [
[
'node1' => ['hasaction' => true, 'issection' => false],
'node2' => ['hasaction' => true, 'issection' => false],
'sectionnode1' => ['hasaction' => true, 'issection' => true],
'node3' => ['hasaction' => true, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
'tc_1' => true,
'node1' => true,
'node3' => false,
'node2' => true,
'node3' => true,
]
],
'All nodes have links links including leaf node and section nodes. Set to not remove section nodes.' => [
[
'node1' => ['hasaction' => true, 'issection' => false],
'node2' => ['hasaction' => true, 'issection' => false],
'sectionnode1' => ['hasaction' => true, 'issection' => true],
'node3' => ['hasaction' => true, 'issection' => false],
],
false,
[
'Home' => true,
'Courses' => true,
'tc_1' => true,
'node1' => true,
'node2' => true,
'sectionnode1' => true,
'node3' => true,
]
],
'Only some parent nodes have links. Section node does not have a link. Set to not remove section nodes.' => [
[
'node1' => ['hasaction' => false, 'issection' => false],
'node2' => ['hasaction' => true, 'issection' => false],
'sectionnode1' => ['hasaction' => false, 'issection' => true],
'node3' => ['hasaction' => true, 'issection' => false],
],
true,
[
'Home' => true,
'Courses' => true,
'tc_1' => true,
'node2' => true,
'node3' => true,
]
]
];
Expand All @@ -124,10 +181,11 @@ public function remove_no_link_items_provider(): array {
*
* @dataProvider remove_no_link_items_provider
* @param array $setup
* @param bool $removesectionnodes Whether to remove the section nodes with an associated action.
* @param array $expected
* @throws \ReflectionException
*/
public function test_remove_no_link_items(array $setup, array $expected) {
public function test_remove_no_link_items(array $setup, bool $removesectionnodes, array $expected) {
global $PAGE;

$this->resetAfterTest();
Expand All @@ -139,8 +197,9 @@ public function test_remove_no_link_items(array $setup, array $expected) {
$page->set_url(new \moodle_url('/course/view.php', array('id' => $course->id)));
// A dummy url to use. We don't care where it's pointing to.
$url = new \moodle_url('/');
foreach ($setup as $node => $hasaction) {
$page->navbar->add($node, $hasaction ? $url : null);
foreach ($setup as $key => $value) {
$page->navbar->add($key, $value['hasaction'] ? $url : null,
$value['issection'] ? \navigation_node::TYPE_SECTION : null);
}

$boostnavbar = $this->getMockBuilder(boostnavbar::class)
Expand All @@ -156,7 +215,7 @@ public function test_remove_no_link_items(array $setup, array $expected) {
// Make the call to the function.
$rcm = $rc->getMethod('remove_no_link_items');
$rcm->setAccessible(true);
$rcm->invoke($boostnavbar);
$rcm->invoke($boostnavbar, $removesectionnodes);

// Get the value for the class variable that the function modifies.
$values = $rcp->getValue($boostnavbar);
Expand Down

0 comments on commit 2a09a3f

Please sign in to comment.