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

navshowmycoursecategories doesn't work as expected

    Details

    • 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

      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).

        Gliffy Diagrams

        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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja 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
            samhemelryk 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
            samhemelryk 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja 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
            fred 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
            fred 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
            rajeshtaneja 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
            rajeshtaneja 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
            fred Frédéric Massart added a comment -

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

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

            Thanks Fred

            Pushing for integration review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Fred Pushing for integration review.
            Hide
            damyon 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 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 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 Damyon Wiese added a comment - Thanks Raj I have pushed this to master and cherrypicked to 24. The patch was good - nice work.
            Hide
            andyjdavis 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
            andyjdavis 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
            rajeshtaneja 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
            rajeshtaneja 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 Damyon Wiese added a comment -

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

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

            Looks ok now. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Looks ok now. Passing.
            Hide
            damyon 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 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
            roebidoebi Rob Rutten added a comment -

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

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

            Navigation settings

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

            Category setup. Each category has one course.

            Show
            roebidoebi Rob Rutten added a comment - Category setup. Each category has one course.
            Hide
            roebidoebi 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
            roebidoebi 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
            rajeshtaneja 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
            rajeshtaneja 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 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 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
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Marina,

            I will check this.

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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Mar/13