Moodle
  1. Moodle
  2. MDL-28965

Description for "Show all courses" navigation setting needs to be updated

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2.5, 2.3.2, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      1. search for "Show all courses" setting
      2. make sure you see an updated description for the setting saying it is limited by "navcourselimit"
    • Workaround:
      Hide

      Set "Course limit" to a number greater than the number of courses in your Moodle instance, and all the courses will always be listed.

      Show
      Set "Course limit" to a number greater than the number of courses in your Moodle instance, and all the courses will always be listed.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-28965-master
    • Rank:
      18512

      Description

      Under "Site administration > Appearance > Navigation" there is an admin setting $CFG->navshowallcourses, labeled "Show all courses". Its help text reads "Setting this ensures that all courses on the site are shown in the navigation at all times."

      However, if you activate this setting, it still limits the number of courses shown in the Navigation block to the number set in the "Course limit" field, $CFG->navcourselimit.

      The help text for $CFG->navshowallcourses suggests that if it's ticked, you should see all the courses all the time, effectively overriding $CFG->navcourselimit. We should implement that, or alternately, update the help text so that it more accurately describes what $CFG->navshowallcourses is supposed to be doing.

      Replication instructions:
      1. Create a Moodle instance with 10 courses in it
      2. Go to "Site Administration > Appearance > Navigation".
      3. Tick the box for "Show all courses"
      4. Set "Course limit" to 5.
      5. Enter a course and look at the Navigation block

      Actual result: You will see only 5 courses listed in the Navigation block

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          That does seem like a bit of a conflict.

          Show
          Michael de Raadt added a comment - That does seem like a bit of a conflict.
          Hide
          Ankit Agarwal added a comment - - edited

          This is the description at present : -
          navshowallcourses : If enabled users will see courses they are enrolled in both within the My Courses branch and the course structure. When disabled users with enrolments will only see the My Courses branch of the navigaiton.
          navcourselimit : Limits the number of courses shown to the user when they are either not logged in or are not enrolled in any courses.

          Consider the following case:-
          There are three courses 1,2,3
          Student A is enrolled in 1 and 2
          user B is not enrolled in any course

          navcourselimit = 1
          navshowallcourses is checked

          A can see:-
          My courses : 1,2
          Courses : -1,2,3

          B can see:-
          Courses :- 1 (+ the current course if other than 1)

          Not logged in user sees :-
          courses:-1

          It seems perfectly right to me and as per the setting description. Although the description can ofcourse be edited to make it more clear. Adding sam as watcher to get more feedback.

          Show
          Ankit Agarwal added a comment - - edited This is the description at present : - navshowallcourses : If enabled users will see courses they are enrolled in both within the My Courses branch and the course structure. When disabled users with enrolments will only see the My Courses branch of the navigaiton. navcourselimit : Limits the number of courses shown to the user when they are either not logged in or are not enrolled in any courses. Consider the following case:- There are three courses 1,2,3 Student A is enrolled in 1 and 2 user B is not enrolled in any course navcourselimit = 1 navshowallcourses is checked A can see:- My courses : 1,2 Courses : -1,2,3 B can see:- Courses :- 1 (+ the current course if other than 1) Not logged in user sees :- courses:-1 It seems perfectly right to me and as per the setting description. Although the description can ofcourse be edited to make it more clear. Adding sam as watcher to get more feedback.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I think this would best be tackled by improving the description of that setting to make it clear that course limit is still respected and imposed.
          That course limit setting is ultra important as it prevents certain death on larger sites should play around with the navigation settings without considering the consequences.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I think this would best be tackled by improving the description of that setting to make it clear that course limit is still respected and imposed. That course limit setting is ultra important as it prevents certain death on larger sites should play around with the navigation settings without considering the consequences. Many thanks Sam
          Hide
          Ankit Agarwal added a comment -

          Thanks Sam for the feedback
          I have updated the string, sending this for peer-review.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Sam for the feedback I have updated the string, sending this for peer-review. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          I reviewed the patch for 2.2, 2.3 and 2.4. It works as expected.

          checklist:
          [-] Syntax
          [y] Output
          [y] Whitespace
          [y] Language
          [-] Databases
          [y] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rossiani Wijaya added a comment - Hi Ankit, I reviewed the patch for 2.2, 2.3 and 2.4. It works as expected. checklist: [-] Syntax [y] Output [y] Whitespace [y] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Ankit Agarwal added a comment -

          Thanks for the feedback Rosie,
          Sending for integration.

          Show
          Ankit Agarwal added a comment - Thanks for the feedback Rosie, Sending for integration.
          Hide
          Aparup Banerjee added a comment -

          Thanks, integrated into 22 23 and master.

          Show
          Aparup Banerjee added a comment - Thanks, integrated into 22 23 and master.
          Hide
          Rajesh Taneja added a comment -

          Hello Aparup,

          I think this is not integrated on 22 and 23. I can see this on Master only.

          Show
          Rajesh Taneja added a comment - Hello Aparup, I think this is not integrated on 22 and 23. I can see this on Master only.
          Hide
          Aparup Banerjee added a comment -

          Thanks for that pick up Raj! those are now pushed up into 22 and 23.

          Show
          Aparup Banerjee added a comment - Thanks for that pick up Raj! those are now pushed up into 22 and 23.
          Hide
          Rajesh Taneja added a comment -

          Looks Great, Ankit and Aparup.

          Thanks for fixing this.

          Show
          Rajesh Taneja added a comment - Looks Great, Ankit and Aparup. Thanks for fixing this.
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: