Moodle
  1. Moodle
  2. MDL-31197

Unavailable course displays incorrect breadcrumbs

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Course, Navigation
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a new course. Set it to 'unavailable to students'.
      2. Enrol a test student user on the course.
      3. Log in as that user and visit the course.
      The error message 'This course is currently unavailable to students' appears.

      Expected behaviour:
      Breadcrumb trail does not include the unavailable course, nor any other course.

      Actual behaviour:
      Breadcrumb trail includes another course.

      Show
      1. Create a new course. Set it to 'unavailable to students'. 2. Enrol a test student user on the course. 3. Log in as that user and visit the course. The error message 'This course is currently unavailable to students' appears. Expected behaviour: Breadcrumb trail does not include the unavailable course, nor any other course. Actual behaviour: Breadcrumb trail includes another course.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-31197-m23

      Description

      When a student looks at an unavailable course and gets the 'This course is currently unavailable to students' error page, that page shows breadcrumbs to another (apparently random) course.

      I assume that the current course name is intentionally not displayed, in order to avoid revealing the name of the course when it is unavailable. If this is the case, then correct behaviour would be for the breadcrumbs on this page to be based on the site course and not include a course.

        Gliffy Diagrams

          Activity

          Hide
          Sam Marshall added a comment -

          Note: This was reported to me by staff here. I don't plan to fix it immediately, but may fix it in a few months if nobody else does!

          Show
          Sam Marshall added a comment - Note: This was reported to me by staff here. I don't plan to fix it immediately, but may fix it in a few months if nobody else does!
          Hide
          Sam Hemelryk added a comment -

          Thanks for the report Sam. Marking this triaged now.

          Show
          Sam Hemelryk added a comment - Thanks for the report Sam. Marking this triaged now.
          Hide
          Sam Hemelryk added a comment -

          Ok I've just been having a look into this now.
          What is happening:

          • require_login is setting $PAGE->course to the unavailable course
          • require_login then calls notice to display a notice about the course being unavailable
          • notice is causing theme and output to initialise navigation
          • because the page course is unavailable it doesn't get added to the navigation
          • because its not added to the navigation the active node isn't found
          • because the active node isn't found the navigation tries to loosely match the first node using the same URL (used to catch argument differences)
          • This causes the first courses in the my courses branch to be marked as the active node.

          I'll look for a solution now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've just been having a look into this now. What is happening: require_login is setting $PAGE->course to the unavailable course require_login then calls notice to display a notice about the course being unavailable notice is causing theme and output to initialise navigation because the page course is unavailable it doesn't get added to the navigation because its not added to the navigation the active node isn't found because the active node isn't found the navigation tries to loosely match the first node using the same URL (used to catch argument differences) This causes the first courses in the my courses branch to be marked as the active node. I'll look for a solution now. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ok got a solution, the navigation nodes active URL now gets overridden to the wwwroot which is also used for the notice button.

          Putting this up for peer-review now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok got a solution, the navigation nodes active URL now gets overridden to the wwwroot which is also used for the notice button. Putting this up for peer-review now. Cheers Sam
          Hide
          Sam Marshall added a comment -

          Solution looks correct so +1 from me - although not sure I am actually qualified to review it

          Show
          Sam Marshall added a comment - Solution looks correct so +1 from me - although not sure I am actually qualified to review it
          Hide
          Rajesh Taneja added a comment -

          Code look spot-on Sam
          Few things:

          1. Can this check go in navigationlib ? Shouldn't this be handled within navigation lib?
          2. Accessing unavailable course page doesn't show any navigation block and hide header and footer, is it intended?
          3. As this is an error, should this be shown as error rather then notice?
          Show
          Rajesh Taneja added a comment - Code look spot-on Sam Few things: Can this check go in navigationlib ? Shouldn't this be handled within navigation lib? Accessing unavailable course page doesn't show any navigation block and hide header and footer, is it intended? As this is an error, should this be shown as error rather then notice?
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          100% can't go into navigation. There is a lot of logic there to keep everything humming along and there are hundreds of areas where like this it needs a helping hand. Adding them as exceptions to the normal path would mean shifting a lot of checking into navigation lib, something we would like to avoid.

          The unavailable course page doesn't show blocks because the notice is being created during require login and no page setup has been done before then. We could start page setup earlier but to be truthful I don't think it is worth it, better to keep the code clean and obvious.

          Showing this as a notice is fine as well as far as I am concerned. While users can get here by manually changing the id on the URL to a course that is unavilable it is much more likely that they will get here because of things like the teacher having just made the course unavailable, or having clicked on an old bookmarked link (or one that was sent to them). I think this warrants a notice rather than an error, remembering of course an error really is just a more aggressive notice.

          Sam Sorry I looked at your review and thought hell year you are qualified to review, but then completely forgot to act upon it sorry!

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, 100% can't go into navigation. There is a lot of logic there to keep everything humming along and there are hundreds of areas where like this it needs a helping hand. Adding them as exceptions to the normal path would mean shifting a lot of checking into navigation lib, something we would like to avoid. The unavailable course page doesn't show blocks because the notice is being created during require login and no page setup has been done before then. We could start page setup earlier but to be truthful I don't think it is worth it, better to keep the code clean and obvious. Showing this as a notice is fine as well as far as I am concerned. While users can get here by manually changing the id on the URL to a course that is unavilable it is much more likely that they will get here because of things like the teacher having just made the course unavailable, or having clicked on an old bookmarked link (or one that was sent to them). I think this warrants a notice rather than an error, remembering of course an error really is just a more aggressive notice. Sam Sorry I looked at your review and thought hell year you are qualified to review, but then completely forgot to act upon it sorry! Cheers Sam
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for the clarification Sam.
          +1 from me

          Show
          Rajesh Taneja added a comment - - edited Thanks for the clarification Sam. +1 from me
          Hide
          Sam Hemelryk added a comment -

          Up for integration now.

          Show
          Sam Hemelryk added a comment - Up for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Ankit Agarwal added a comment -

          Works as expected!
          Thanks

          Show
          Ankit Agarwal added a comment - Works as expected! Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: