Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37762

Breadcrumb not showing categories on course page

    Details

    • Testing Instructions:
      Hide
      1. Log in as a admin.
      2. Ensure $CFG->navshowcategories is enabled.
      3. Log in as a student.
      4. Browse to a course.
      5. Check the category path for the course is shown in the navbar.
      6. Log in as an admin and disable $CFG->navshowcategories.
      7. Log in as a student.
      8. Browse to the same course.
      9. Check that the category path is no longer shown.
      Show
      Log in as a admin. Ensure $CFG->navshowcategories is enabled. Log in as a student. Browse to a course. Check the category path for the course is shown in the navbar. Log in as an admin and disable $CFG->navshowcategories. Log in as a student. Browse to the same course. Check that the category path is no longer shown.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-37762-m25

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              amlamontagne 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
              amlamontagne 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
              derekcx 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
              derekcx 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
              derekcx Derek Chirnside added a comment -

              This is the NEW 2.4 breadcrumbs

              Show
              derekcx Derek Chirnside added a comment - This is the NEW 2.4 breadcrumbs
              Hide
              derekcx 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
              derekcx 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
              vadimon Vadim Dvorovenko added a comment -

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

              Show
              vadimon Vadim Dvorovenko added a comment - This patch partially resolves problem, but it remove current course item in navigation
              Hide
              vadimon 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
              vadimon 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
              derekcx Derek Chirnside added a comment -

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

              -Derek

              Show
              derekcx Derek Chirnside added a comment - Bump. Has anything happened with this? Maybe a duplicate tracker item? -Derek
              Hide
              paul.n 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.n 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
              doreen 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
              doreen 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.n 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.n 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
              samhemelryk 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
              samhemelryk 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.n 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.n 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
              samhemelryk Sam Hemelryk added a comment -

              Thanks Paul, this is up for integration now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Paul, this is up for integration now.
              Hide
              damyon 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 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 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 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski 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 Damyon Wiese added a comment -

              Pulled that in - thanks Dan.

              Show
              damyon Damyon Wiese added a comment - Pulled that in - thanks Dan.
              Hide
              marina 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 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 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 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
              poltawski 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
              poltawski 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 Marina Glancy added a comment -
              Show
              marina Marina Glancy added a comment - MDL-39127
              Hide
              marina 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 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 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 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
              vadimon 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
              vadimon 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
              samhemelryk 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
              samhemelryk 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 Marina Glancy added a comment -

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

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

              Passing, as tested in MDL-39127.

              Show
              fred Frédéric Massart added a comment - Passing, as tested in MDL-39127 .
              Hide
              poltawski 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
              poltawski 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:
                    Fix Release Date:
                    13/May/13