moodle-an-hochschulen / moodle-local_boostnavigation

Moodle plugin which tries to overcome some fixed appearance behaviours of Boost's nav drawer in a clean way
GNU General Public License v3.0
39 stars 28 forks source link

Bugfix: Fix error when participants node removed and viewing user logs #61

Closed zpottie closed 4 years ago

zpottie commented 4 years ago

Hi there,

This pull request ensures that a user's logs pages do not error when the 'Remove participants node' setting (removeparticipantscoursenode) is enabled. Removing the node completely causes an error when, for example, going to participants in a course and clicking on a user's name and viewing their logs for that day: RemoveParticipantsEnabled

This pull request tries to address this by checking if we're on the user logs report page and then uses the node's mainnavonly field to hide it instead.

Another way to deal with it possibly is to not remove it if on the user logs page - and leave it up to the admin to remove the 'viewparticipants' permission from users they don't want to have the 'Participants' node in the breadcrumbs. That fix might look something like the below, in the file: wwwroot/local/boostnavigation/lib.php

215     // Only proceed if we are inside a course and we are _not_ on the frontpage or user logs page.
216     if ($PAGE->context->get_course_context(false) == true && $COURSE->id != SITEID && strpos($PAGE->url, 'report/log/user.php') === false) {
217         if ($participantsnode = $navigation->find('participants', global_navigation::TYPE_CONTAINER)) {
218             // Remove participants node (Just hiding it with the showinflatnavigation attribute does not work here).
219             $participantsnode->remove();
220         }
221     }

The extra condition on line 216 prevents it from being removed on the user' logs report page.

Let me know if you'd prefer the solution above or have other suggestions and I'll update the pull request.

Hope this helps.

Thanks, Zander

zpottie commented 4 years ago

Not sure why Travis failed, says the job ran for too long...

abias commented 4 years ago

Hi @zpottie,

thanks for highlighting this issue and for providing a straightforward solution. I will merge this now and will add some comments and a Behat test in a subsequent commit.

Cheers, Alex