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
    • Rank:
      37645

      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.

        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: