Moodle
  1. Moodle
  2. MDL-37762

Breadcrumb not showing categories on course page

    Details

    • Rank:
      47489

      Description

      I've updated to 2.4 and found that this problem has returned.

      Create course categories category / subcategory / subsubcategory and course inside it.
      When you're in bottom category, breadcrumb shows "start > category > subcategory > subsubcategory ".
      When you's re inside course in 2.4 breadcrumb shows "start > course", while in 2.3 it was "start > category > subcategory > subsubcategory > course".
      This is unexpected and unwanted behaviour, as it is a regression of MDL-2347.

      1. 0001-MDL-37762-dirty-fix.patch
        2 kB
        Vadim Dvorovenko
      1. 2,4 breadcrumbs (or lack of them).jpg
        16 kB
      2. Before.jpg
        27 kB

        Issue Links

          Activity

          Hide
          Anne-Marie Lamontagne added a comment -

          I wouldn't use the word "stupid", but I really need my breadcrumb to work, and my technician cannot find the problem, I hope somebody can help us here... or at the Canadian Moodlemoot next week. Have a nice day!

          Show
          Anne-Marie Lamontagne added a comment - I wouldn't use the word "stupid", but I really need my breadcrumb to work, and my technician cannot find the problem, I hope somebody can help us here... or at the Canadian Moodlemoot next week. Have a nice day!
          Hide
          Derek Chirnside added a comment -

          I think you will find this is an unintended consequence of something Vadim, and is not intentional.

          But I am interested in any developer comment. Please ignore the slightly emotive and negative tone of the issue description.

          -Derek

          Show
          Derek Chirnside added a comment - I think you will find this is an unintended consequence of something Vadim, and is not intentional. But I am interested in any developer comment. Please ignore the slightly emotive and negative tone of the issue description. -Derek
          Hide
          Derek Chirnside added a comment -

          This is the NEW 2.4 breadcrumbs

          Show
          Derek Chirnside added a comment - This is the NEW 2.4 breadcrumbs
          Hide
          Derek Chirnside added a comment -

          The courses link in the breadcrumbs was always not clickable (this is a feature, not a bug) but tere were links to categories visible. These are the links that have gone.

          Show
          Derek Chirnside added a comment - The courses link in the breadcrumbs was always not clickable (this is a feature, not a bug) but tere were links to categories visible. These are the links that have gone.
          Hide
          Vadim Dvorovenko added a comment -

          This patch partially resolves problem, but it remove current course item in navigation

          Show
          Vadim Dvorovenko added a comment - This patch partially resolves problem, but it remove current course item in navigation
          Hide
          Vadim Dvorovenko added a comment -

          Anne-Marie, i've attached simply patch - it partly resolves problem.
          Currently breadcrumb way is taken from navigation block. So, if you see course under short "current course" path in navigation block, breadcrumb will show short way. Disabling current_course item leads to long path in navigation and in breadcrumb, like in 2.3. But long way in navigation is not very good too.
          Ability to have different ways in navigation block and in breadcrumb needs deep changes in navigation core. Also, such changes may contradict with moodle navigation philosophy - different ways may delude users.

          Show
          Vadim Dvorovenko added a comment - Anne-Marie, i've attached simply patch - it partly resolves problem. Currently breadcrumb way is taken from navigation block. So, if you see course under short "current course" path in navigation block, breadcrumb will show short way. Disabling current_course item leads to long path in navigation and in breadcrumb, like in 2.3. But long way in navigation is not very good too. Ability to have different ways in navigation block and in breadcrumb needs deep changes in navigation core. Also, such changes may contradict with moodle navigation philosophy - different ways may delude users.
          Hide
          Derek Chirnside added a comment -

          Bump. Has anything happened with this? Maybe a duplicate tracker item?

          -Derek

          Show
          Derek Chirnside added a comment - Bump. Has anything happened with this? Maybe a duplicate tracker item? -Derek
          Hide
          Paul Nicholls added a comment -

          Updated title and description to make its tone more neutral. Pull details added - it's a pretty quick attempt at this, so the peer reviewer will hopefully have a better understanding of the navigation system than I do and be able to confirm whether it's a sensible approach. My commit message is also a one-liner; I think it's reasonably self-explanatory, and I'm already late starting my weekend, so I don't have time to write anything longer

          I also removed the part of the description about the "current course" node - it's a separate issue, and should be submitted as a new ticket (if there isn't already one for that issue).

          Show
          Paul Nicholls added a comment - Updated title and description to make its tone more neutral. Pull details added - it's a pretty quick attempt at this, so the peer reviewer will hopefully have a better understanding of the navigation system than I do and be able to confirm whether it's a sensible approach. My commit message is also a one-liner; I think it's reasonably self-explanatory, and I'm already late starting my weekend, so I don't have time to write anything longer I also removed the part of the description about the "current course" node - it's a separate issue, and should be submitted as a new ticket (if there isn't already one for that issue).
          Hide
          Julia added a comment -

          We used to have a discussion about breadcrumbs in usability forum.
          https://tracker.moodle.org/browse/MDL-34487
          Perhaps it's time to provide more options to configure breadcrumbs.

          Show
          Julia added a comment - We used to have a discussion about breadcrumbs in usability forum. https://tracker.moodle.org/browse/MDL-34487 Perhaps it's time to provide more options to configure breadcrumbs.
          Hide
          Paul Nicholls added a comment -

          Re-requesting peer review with Sam as reviewer - I couldn't find a way to set the reviewer while it was awaiting peer review.

          Show
          Paul Nicholls added a comment - Re-requesting peer review with Sam as reviewer - I couldn't find a way to set the reviewer while it was awaiting peer review.
          Hide
          Sam Hemelryk added a comment -

          Hi Paul,

          I've just being looking at your changes now.
          In general the code is good thanks. I've produced a couple of branches based upon your code just making a few tweaks.
          Would you mind having a look at them and seeing what you think:

          Really the only noteable change is that I've added a condition before we add categories to ensure that $CFG->navshowcategories is enabled.

          If you're happy with the tweaking I've done I think this can be pushed up for integration.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Paul, I've just being looking at your changes now. In general the code is good thanks. I've produced a couple of branches based upon your code just making a few tweaks. Would you mind having a look at them and seeing what you think: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-37762-m25 https://github.com/samhemelryk/moodle/compare/MOODLE_24_STABLE...wip-MDL-37762-m24 Really the only noteable change is that I've added a condition before we add categories to ensure that $CFG->navshowcategories is enabled. If you're happy with the tweaking I've done I think this can be pushed up for integration. Many thanks Sam
          Hide
          Paul Nicholls added a comment -

          Hi Sam,
          That looks good (and makes sense) to me. Hopefully it'll help address some of the requests for more flexibility - though tying it to the existing $CFG->navshowcategories option might not please everyone (I'm not really sure why anyone would want categories in the breadcrumbs but not the navigation itself - and I don't think it's worth adding yet more settings for such minute details; if anyone's really that bothered by it, it'll be easy enough for them to make a very minor change to this new code to do what they want).

          Please go ahead and push it up for integration (I don't have permission to do so).

          -Paul

          Show
          Paul Nicholls added a comment - Hi Sam, That looks good (and makes sense) to me. Hopefully it'll help address some of the requests for more flexibility - though tying it to the existing $CFG->navshowcategories option might not please everyone (I'm not really sure why anyone would want categories in the breadcrumbs but not the navigation itself - and I don't think it's worth adding yet more settings for such minute details; if anyone's really that bothered by it, it'll be easy enough for them to make a very minor change to this new code to do what they want). Please go ahead and push it up for integration (I don't have permission to do so). -Paul
          Hide
          Sam Hemelryk added a comment -

          Thanks Paul, this is up for integration now.

          Show
          Sam Hemelryk added a comment - Thanks Paul, this is up for integration now.
          Hide
          Damyon Wiese added a comment -

          Thanks Sam and Paul,

          I've integrated this now for master and 24. I had a quick look to see if a unit test could be added for it - but lib/tests/navigationlib_test.php is suffering from multiple test classes in a single file issue.

          If you think a unit test could be added without an unreasonable amount of effort please add an issue Sam.

          Show
          Damyon Wiese added a comment - Thanks Sam and Paul, I've integrated this now for master and 24. I had a quick look to see if a unit test could be added for it - but lib/tests/navigationlib_test.php is suffering from multiple test classes in a single file issue. If you think a unit test could be added without an unreasonable amount of effort please add an issue Sam.
          Hide
          Damyon Wiese added a comment - - edited

          Reported by Dan,

          Clicking on a category in the navbar (master only) gives warning:

          Please use URL /course/index.php?categoryid=XXX instead of /course/category.php?id=XXX

          (will add patch)

          Show
          Damyon Wiese added a comment - - edited Reported by Dan, Clicking on a category in the navbar (master only) gives warning: Please use URL /course/index.php?categoryid=XXX instead of /course/category.php?id=XXX (will add patch)
          Hide
          Dan Poltawski added a comment -

          I've put a fix here:
          git pull git://github.com/danpoltawski/moodle.git MDL-37762-category-fixup

          Show
          Dan Poltawski added a comment - I've put a fix here: git pull git://github.com/danpoltawski/moodle.git MDL-37762 -category-fixup
          Hide
          Damyon Wiese added a comment -

          Pulled that in - thanks Dan.

          Show
          Damyon Wiese added a comment - Pulled that in - thanks Dan.
          Hide
          Marina Glancy added a comment -

          Please also note that all course categories are already loaded in moodle_page for the current course, you do not need to load them again using coursecat, just loop through

          $this->page->categories
          
          Show
          Marina Glancy added a comment - Please also note that all course categories are already loaded in moodle_page for the current course, you do not need to load them again using coursecat, just loop through $ this ->page->categories
          Hide
          Marina Glancy added a comment -

          Also this line
          https://github.com/samhemelryk/moodle/compare/master...wip-MDL-37762-m25#L0R3067

          $categories[] = $this->page->navigation->get('courses'); 
          

          must check if the node 'courses' is present, because it can be disabled in settings.

          Show
          Marina Glancy added a comment - Also this line https://github.com/samhemelryk/moodle/compare/master...wip-MDL-37762-m25#L0R3067 $categories[] = $ this ->page->navigation->get('courses'); must check if the node 'courses' is present, because it can be disabled in settings.
          Hide
          Dan Poltawski added a comment - - edited

          I'm failing this due to the problem caused described in MDL-39127

          Thanks to Marina, it seems this:

          $this->page->navigation->get('courses');
          

          Is returning false.

          Show
          Dan Poltawski added a comment - - edited I'm failing this due to the problem caused described in MDL-39127 Thanks to Marina, it seems this: $ this ->page->navigation->get('courses'); Is returning false.
          Hide
          Marina Glancy added a comment -
          Show
          Marina Glancy added a comment - MDL-39127
          Hide
          Marina Glancy added a comment - - edited

          When logged in as student:

          Fatal error: Class 'coursecat' not found in /home/marina/repositories/int_master/integration/lib/navigationlib.php on line 3066 
          Call Stack: 0.0005 795784 
          1. {main}() /home/marina/repositories/int_master/integration/course/view.php:0 0.4339 49922608 
          2. core_renderer->header() /home/marina/repositories/int_master/integration/course/view.php:240 0.4462 51102176 
          3. core_renderer->render_page_layout() /home/marina/repositories/int_master/integration/lib/outputrenderers.php:771 0.4465 51220496 
          4. include('/home/marina/repositories/int_master/integration/theme/base/layout/general.php') /home/marina/repositories/int_master/integration/lib/outputrenderers.php:841 0.6811 72531648 
          5. core_renderer->navbar() /home/marina/repositories/int_master/integration/theme/base/layout/general.php:77 0.6811 72531832 
          6. navbar->get_items() /home/marina/repositories/int_master/integration/lib/outputrenderers.php:2638 0.6813 72532424 
          7. navbar->get_course_categories() /home/marina/repositories/int_master/integration/lib/navigationlib.php:3027
          
          Show
          Marina Glancy added a comment - - edited When logged in as student: Fatal error: Class 'coursecat' not found in /home/marina/repositories/int_master/integration/lib/navigationlib.php on line 3066 Call Stack: 0.0005 795784 1. {main}() /home/marina/repositories/int_master/integration/course/view.php:0 0.4339 49922608 2. core_renderer->header() /home/marina/repositories/int_master/integration/course/view.php:240 0.4462 51102176 3. core_renderer->render_page_layout() /home/marina/repositories/int_master/integration/lib/outputrenderers.php:771 0.4465 51220496 4. include('/home/marina/repositories/int_master/integration/theme/base/layout/general.php') /home/marina/repositories/int_master/integration/lib/outputrenderers.php:841 0.6811 72531648 5. core_renderer->navbar() /home/marina/repositories/int_master/integration/theme/base/layout/general.php:77 0.6811 72531832 6. navbar->get_items() /home/marina/repositories/int_master/integration/lib/outputrenderers.php:2638 0.6813 72532424 7. navbar->get_course_categories() /home/marina/repositories/int_master/integration/lib/navigationlib.php:3027
          Hide
          Marina Glancy added a comment -

          well, except for all problems listed above.
          The comment to $CFG->navshowcategories says:
          "Show course categories in the navigation bar and navigation blocks. This does not occur with courses the user is currently enrolled in, they will still be listed under mycourses without categories."

          so at least string must have been changed.

          But at the same time, navbar should be the same as navigation block. It looks awkward when in navigation block course in under 'Current course' and navbar is completely different.

          And also, as Martin said 7 years ago in the comment to MDL-2347 - what is convenient for admins is not convenient for students. Maybe capabilities is the way to go here.

          It really needs some more thinking.

          Show
          Marina Glancy added a comment - well, except for all problems listed above. The comment to $CFG->navshowcategories says: "Show course categories in the navigation bar and navigation blocks. This does not occur with courses the user is currently enrolled in, they will still be listed under mycourses without categories." so at least string must have been changed. But at the same time, navbar should be the same as navigation block. It looks awkward when in navigation block course in under 'Current course' and navbar is completely different. And also, as Martin said 7 years ago in the comment to MDL-2347 - what is convenient for admins is not convenient for students. Maybe capabilities is the way to go here. It really needs some more thinking.
          Hide
          Vadim Dvorovenko added a comment -

          > what is convenient for admins is not convenient for students
          But one thing has changed from 1.4 - now it's more difficult to determine, who is admin and who is teacher and who is student.
          We've got teachers that have access to more then 100 courses, they preffer (like admins) full path in breadcrumbs.
          I think some way to determine who is who, is to check mycourses count - if the're less than 20 (or other configurable value), we show short path, otherwise we show fullpath. This logic seems very pretty for me also on frontpage - if less then 20 - show mycourses list, if more - show courses three. With ability to code frontpage plugin (MDL-37731) the behavior will be more customizable.
          Maybee we should make ability to make breadcrumb and navigation tree plugins? Such plugin can contain some code, that looks at breadcrumb or navtree, and expand or remove some items depending on custom logic. It can also determine, how to trim long names and so on

          Show
          Vadim Dvorovenko added a comment - > what is convenient for admins is not convenient for students But one thing has changed from 1.4 - now it's more difficult to determine, who is admin and who is teacher and who is student. We've got teachers that have access to more then 100 courses, they preffer (like admins) full path in breadcrumbs. I think some way to determine who is who, is to check mycourses count - if the're less than 20 (or other configurable value), we show short path, otherwise we show fullpath. This logic seems very pretty for me also on frontpage - if less then 20 - show mycourses list, if more - show courses three. With ability to code frontpage plugin ( MDL-37731 ) the behavior will be more customizable. Maybee we should make ability to make breadcrumb and navigation tree plugins? Such plugin can contain some code, that looks at breadcrumb or navtree, and expand or remove some items depending on custom logic. It can also determine, how to trim long names and so on
          Hide
          Sam Hemelryk added a comment -

          Alrighty, MDL-39127 has a patch up now to clean things up.
          It uses $this->page->categories now instead (yay for that being there).
          Shame the coursecat class isn't usable be surely we can work on that in the future.

          In regards to the discussion about the navbar not matching the navigation, that is an interesting one.
          When I first saw this issue I had the same reaction that Marina had.
          The navigation was designed to illustrate the structure of a Moodle site. In planning that we decided that the navbar should only ever show the path highlighted in the navigation. If something was missing from the navbar, it would be missing from the navigation and we'd need to either fix the structure of the navigation or make an exception when displaying the page.
          Back to this issue however I decided that this change was a worthwhile change. Not necessarily the "most accurate" of changes, but a good change necessitated because of the damned navigation settings.
          If I were able to re-do the navigation I would have generated the navigation consistently without any of the navigation settings that we have now.
          Then within the navigation block I'd introduce some means to fully control how things are displayed, allowing admins to configure things to their likings.
          The navbar could become a block allowing control of that as well.
          However as we have these navigation settings and I don't have time to rebuild things I think this is the next best approach.
          Certainly as well as this issue several people have mentioned to me they wanted this change made.

          Many thanks and sorry letting those issues slip in.
          Sam

          Show
          Sam Hemelryk added a comment - Alrighty, MDL-39127 has a patch up now to clean things up. It uses $this->page->categories now instead (yay for that being there). Shame the coursecat class isn't usable be surely we can work on that in the future. In regards to the discussion about the navbar not matching the navigation, that is an interesting one. When I first saw this issue I had the same reaction that Marina had. The navigation was designed to illustrate the structure of a Moodle site. In planning that we decided that the navbar should only ever show the path highlighted in the navigation. If something was missing from the navbar, it would be missing from the navigation and we'd need to either fix the structure of the navigation or make an exception when displaying the page. Back to this issue however I decided that this change was a worthwhile change. Not necessarily the "most accurate" of changes, but a good change necessitated because of the damned navigation settings. If I were able to re-do the navigation I would have generated the navigation consistently without any of the navigation settings that we have now. Then within the navigation block I'd introduce some means to fully control how things are displayed, allowing admins to configure things to their likings. The navbar could become a block allowing control of that as well. However as we have these navigation settings and I don't have time to rebuild things I think this is the next best approach. Certainly as well as this issue several people have mentioned to me they wanted this change made. Many thanks and sorry letting those issues slip in. Sam
          Hide
          Marina Glancy added a comment -

          Created a follow-up issue MDL-39142 to consider more settings in hacking navbar

          Show
          Marina Glancy added a comment - Created a follow-up issue MDL-39142 to consider more settings in hacking navbar
          Hide
          Frédéric Massart added a comment -

          Passing, as tested in MDL-39127.

          Show
          Frédéric Massart added a comment - Passing, as tested in MDL-39127 .
          Hide
          Dan Poltawski added a comment -

          Blooming Marvelous! It's time for a knees up - your changes are upstream!

          Thanks for making Moodle better!

          Toodle pip

          Show
          Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

            People

            • Votes:
              14 Vote for this issue
              Watchers:
              21 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: