Moodle
  1. Moodle
  2. MDL-37329

navshowmycoursecategories doesn't work as expected

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4.2
    • Component/s: My home
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as admin
      2. Set navshowmycoursecategories to on
      3. Set navshowcategories to on
      4. Set navshowallcourses to off
      5. Create few categories and sub-categories.
      6. Add few courses to categories and sub-categories
      7. Enrol user1 to few courses (Make sure user1 is not enrolled in all courses)
      8. Login as user1 and click my courses in navigation node.
      9. Make sure user can only see his/her enrolled courses only.
      Show
      Login as admin Set navshowmycoursecategories to on Set navshowcategories to on Set navshowallcourses to off Create few categories and sub-categories. Add few courses to categories and sub-categories Enrol user1 to few courses (Make sure user1 is not enrolled in all courses) Login as user1 and click my courses in navigation node. Make sure user can only see his/her enrolled courses only.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-mdl-37329-new
    • Rank:
      46943

      Description

      Enable navshowmycoursecategories and navshowcategories in Site Administration --> Appearance --> Navigation
      Let navshowallcourses disabled.

      When you click on "My Courses" everything is in a category (so far it is expected), when you click on a category you will see all courses in this category not just those you are enrolled in (expected was that you just see the courses you are enrolled in, but have somehow the ability to switch the display to show all courses if needed i.e. for enrollment in a new course).

      1. AdminMyCourses.png
        154 kB
      2. Categories.png
        161 kB
      3. MyCourses.png
        136 kB
      4. Schermafbeelding 2013-04-05 om 11.57.16.png
        27 kB
      5. Schermafbeelding 2013-04-05 om 12.03.34.png
        56 kB
      6. Schermafbeelding 2013-04-05 om 12.04.15.png
        30 kB

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Rajesh Taneja added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Rajesh Taneja added a comment -

          I did some draft work on this, it seems we need a new node type to differentiate category and enroled category.

          Adding Sam as peer-reviewer to get his feedback on my draft work. This is not yet complete, but need to check with Sam, if this is correct way to go.

          Show
          Rajesh Taneja added a comment - I did some draft work on this, it seems we need a new node type to differentiate category and enroled category. Adding Sam as peer-reviewer to get his feedback on my draft work. This is not yet complete, but need to check with Sam, if this is correct way to go.
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          The direction looks good thanks.

          Worth noting, not sure what you're aware of so I'll just note things:

          1. global_navigation::add_category just needs it phpdocs tweaked.
          2. blocks/navigation/yui/navigation/navigation.js
            1. Should define NODETYPE.MYCATEGORY
            2. Look for all uses of NODETYPE.CATEGORY and extend as required to include MYCATEGORY. (Just the view all courses links, not sure if they need updating actually.

          Anyway good work so far thanks.

          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, The direction looks good thanks. Worth noting, not sure what you're aware of so I'll just note things: global_navigation::add_category just needs it phpdocs tweaked. blocks/navigation/yui/navigation/navigation.js Should define NODETYPE.MYCATEGORY Look for all uses of NODETYPE.CATEGORY and extend as required to include MYCATEGORY. (Just the view all courses links, not sure if they need updating actually. Anyway good work so far thanks. Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for initial review Sam,

          Just noting as discussed with you, this will only go back till 2.4 because:

          1. API change
          2. It will be tedious to get this on 2.3-
          Show
          Rajesh Taneja added a comment - Thanks for initial review Sam, Just noting as discussed with you, this will only go back till 2.4 because: API change It will be tedious to get this on 2.3-
          Hide
          Rajesh Taneja added a comment -

          Hello Sam,

          Can you please review this code. Also, I am not sure about the const. Is it fine to use 11.

          FYI: I tried giving 100 to it but seems got stuck in some logic there.

          Show
          Rajesh Taneja added a comment - Hello Sam, Can you please review this code. Also, I am not sure about the const. Is it fine to use 11. FYI: I tried giving 100 to it but seems got stuck in some logic there.
          Hide
          Frédéric Massart added a comment -

          Hi Raj,

          great patch, thanks. Just a few minor comments here:

          • There are a couple of lines which are too long.
          • navigation.js:409 I know you don't introduce this, but it looks like we're expecting a double == there, not a single one.
          • navigation.js:1693 Could you add the @return doc block as you're modifying this?
          • navigation.js:1694 I'm not sure about the approach of having a parameter just for MyHome. What if in the future we need to add categories or another type? It is really unlikely but would that make sense to pass the nodetype via parameter, and assume the default is TYPE_CATEGORY instead? Please feel free to tell me that I'm overkilling it.

          Many thanks!
          Fred

          Show
          Frédéric Massart added a comment - Hi Raj, great patch, thanks. Just a few minor comments here: There are a couple of lines which are too long. navigation.js:409 I know you don't introduce this, but it looks like we're expecting a double == there, not a single one. navigation.js:1693 Could you add the @return doc block as you're modifying this? navigation.js:1694 I'm not sure about the approach of having a parameter just for MyHome. What if in the future we need to add categories or another type? It is really unlikely but would that make sense to pass the nodetype via parameter, and assume the default is TYPE_CATEGORY instead? Please feel free to tell me that I'm overkilling it. Many thanks! Fred
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          1. All lines are between 130-180, so seems fine to me.
          2. Error at 409 should be fixed in separate issue as I am not sure this issue will be backported.
          3. I think you mean navigationlib.php. As there was no valid return, it is not required. Anyway I have added one now.
          4. Thanks. Make sense, I have replaced bool with int.

          Back for your review.

          Show
          Rajesh Taneja added a comment - Thanks Fred, All lines are between 130-180, so seems fine to me. Error at 409 should be fixed in separate issue as I am not sure this issue will be backported. I think you mean navigationlib.php. As there was no valid return, it is not required. Anyway I have added one now. Thanks. Make sense, I have replaced bool with int. Back for your review.
          Hide
          Frédéric Massart added a comment -

          Thanks Raj, I think it's good for integration! Cheers

          Show
          Frédéric Massart added a comment - Thanks Raj, I think it's good for integration! Cheers
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred

          Pushing for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Fred Pushing for integration review.
          Hide
          Damyon Wiese added a comment -

          This all looks fine to me thanks Raj,

          I'll push first thing tomorrow - I'm about to head off and I don't want to break the build and run.

          Show
          Damyon Wiese added a comment - This all looks fine to me thanks Raj, I'll push first thing tomorrow - I'm about to head off and I don't want to break the build and run.
          Hide
          Damyon Wiese added a comment -

          Thanks Raj I have pushed this to master and cherrypicked to 24.

          The patch was good - nice work.

          Show
          Damyon Wiese added a comment - Thanks Raj I have pushed this to master and cherrypicked to 24. The patch was good - nice work.
          Hide
          Andrew Davis added a comment -

          Something is wrong here. I'm attaching some screenshots.

          As specified in the testing instructions:
          navshowallcourses is off.
          navshowcategories is on.
          navshowmycoursecategories is on.

          While logged in as a student, if you expand the my courses node its not displaying the student's courses and their associated categories.

          As admin you can see the courses you're enrolled in but for some reason a category not containing a course the user is enrolled in is displayed.

          Show
          Andrew Davis added a comment - Something is wrong here. I'm attaching some screenshots. As specified in the testing instructions: navshowallcourses is off. navshowcategories is on. navshowmycoursecategories is on. While logged in as a student, if you expand the my courses node its not displaying the student's courses and their associated categories. As admin you can see the courses you're enrolled in but for some reason a category not containing a course the user is enrolled in is displayed.
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Andrew.

          MY bad. Courses were supposed to be added to category and not MY_HOME.

          Have added another commit to fix this.

          As per empty categories, we show all categories, which user has access to (Probably another issue)

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Andrew. MY bad. Courses were supposed to be added to category and not MY_HOME. Have added another commit to fix this. As per empty categories, we show all categories, which user has access to (Probably another issue)
          Hide
          Damyon Wiese added a comment -

          I have pushed the patch to 24 and master - ready for restesting.

          Show
          Damyon Wiese added a comment - I have pushed the patch to 24 and master - ready for restesting.
          Hide
          Andrew Davis added a comment -

          Looks ok now. Passing.

          Show
          Andrew Davis added a comment - Looks ok now. Passing.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon
          Hide
          Rob Rutten added a comment -

          User sees sub category even though he is not assigned to any course in that sub category

          Show
          Rob Rutten added a comment - User sees sub category even though he is not assigned to any course in that sub category
          Hide
          Rob Rutten added a comment -

          Navigation settings

          Show
          Rob Rutten added a comment - Navigation settings
          Hide
          Rob Rutten added a comment -

          Category setup. Each category has one course.

          Show
          Rob Rutten added a comment - Category setup. Each category has one course.
          Hide
          Rob Rutten added a comment -

          I have Moodle version 2.4.3 (Build: 20130328) running and still have this problem. I have navshowcategories and navshowmycoursecategories enabled (navshowallcourse is disabled) - see attached screenshot.

          I have a top category with two sub categories, the user is assigned to a course in the top category and the first sub category (called "top course" and "course 1"). The user is not assigned to "course 2" in the second sub category. However I still see the second sub category in the navigation menu of "My courses".

          This situation is depicted in the secondscreen shot. I included the course overview block so you can see the user really is not assigned to the course in the second subcategory.

          As a final remark. I really don't like the fact that the course in the top category ("Top course") is shown after the sub categories. This used to be the other way around...

          Show
          Rob Rutten added a comment - I have Moodle version 2.4.3 (Build: 20130328) running and still have this problem. I have navshowcategories and navshowmycoursecategories enabled (navshowallcourse is disabled) - see attached screenshot. I have a top category with two sub categories, the user is assigned to a course in the top category and the first sub category (called "top course" and "course 1"). The user is not assigned to "course 2" in the second sub category. However I still see the second sub category in the navigation menu of "My courses". This situation is depicted in the secondscreen shot. I included the course overview block so you can see the user really is not assigned to the course in the second subcategory. As a final remark. I really don't like the fact that the course in the top category ("Top course") is shown after the sub categories. This used to be the other way around...
          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this Rob,

          It seems to be a regression from MDL-32975, Have opened another issue to rectify this (MDL-39014)

          Show
          Rajesh Taneja added a comment - Thanks for reporting this Rob, It seems to be a regression from MDL-32975 , Have opened another issue to rectify this ( MDL-39014 )
          Hide
          Marina Glancy added a comment -

          Raj, it seems that this line should have introduced regression:
          https://github.com/moodle/moodle/blame/MOODLE_24_STABLE/lib/navigationlib.php#L1545

          Show
          Marina Glancy added a comment - Raj, it seems that this line should have introduced regression: https://github.com/moodle/moodle/blame/MOODLE_24_STABLE/lib/navigationlib.php#L1545
          Hide
          Rajesh Taneja added a comment -

          Thanks Marina,

          I will check this.

          Show
          Rajesh Taneja added a comment - Thanks Marina, I will check this.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: