Moodle
  1. Moodle
  2. MDL-31548

Hidden courses no longer show for Adminstrators viewing user profiles

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Open the profile of a student enrolled in a number of courses (Site admin > Users > Browse Users > Pick a user)
      3. In another tab, navigate to list of courses (Site admin > Courses > Add/edit courses > Pick category)
      4. Click the eye icon to hide a course that the student is enrolled in
      5. Note the effect on the student's profile, it should turn grey, but still appear
      6. Visit the same students course profile page (user/view.php?id=x&course=y) and note that the same pattern is followed as above.
      Show
      Log in as admin Open the profile of a student enrolled in a number of courses (Site admin > Users > Browse Users > Pick a user) In another tab, navigate to list of courses (Site admin > Courses > Add/edit courses > Pick category) Click the eye icon to hide a course that the student is enrolled in Note the effect on the student's profile, it should turn grey, but still appear Visit the same students course profile page (user/view.php?id=x&course=y) and note that the same pattern is followed as above.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31548-master
    • Rank:
      38103

      Description

      In Moodle 1.X, Administrators were able to view all courses a user was enrolled in (including hidden courses) by looking at their profile. It looks like this feature was added to Moodle 1.8 as a result of this bug report: http://tracker.moodle.org/browse/MDL-12855

      I am using Moodle 2.1.1+ and it appears this feature has been lost - hidden courses no longer appear to administrators in a user profile.

      To me, this is an essential feature of Moodle - checking an individual user's enrolments is much harder/longer without it. It would be great to have it back!

      1. MDL-31548.patch
        0.9 kB
        Iurii Kucherov
      1. View in 1.9.jpg
        120 kB

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that.

          I was able to reproduce that. In 1.9 the link to hidden courses was a faded grey, but now they don't appear at all.

          Show
          Michael de Raadt added a comment - Thanks for reporting that. I was able to reproduce that. In 1.9 the link to hidden courses was a faded grey, but now they don't appear at all.
          Hide
          Iurii Kucherov added a comment -

          Hi.

          There is a problem with 'function enrol_get_users_courses' in enrollib.php. This function gets all courses for user and checks whether to show it or not.
          If administrator is logged in it doesn't work appropriate. The problem is that has_capability method is checking rights for user who is enrolled in the course but it should check rights for logged in user.

          — enrollib.php.original 2012-02-12 00:26:02.752760133 +0200
          +++ enrollib.php 2012-02-12 00:26:13.669765449 +0200
          @@ -773,7 +773,7 @@
          unset($courses[$id]);
          continue;
          }

          • if (!has_capability('moodle/course:viewhiddencourses', $context, $userid)) {
            + if (!has_capability('moodle/course:viewhiddencourses', $context, $USER->id)) { unset($courses[$id]); continue; }
          Show
          Iurii Kucherov added a comment - Hi. There is a problem with 'function enrol_get_users_courses' in enrollib.php. This function gets all courses for user and checks whether to show it or not. If administrator is logged in it doesn't work appropriate. The problem is that has_capability method is checking rights for user who is enrolled in the course but it should check rights for logged in user. — enrollib.php.original 2012-02-12 00:26:02.752760133 +0200 +++ enrollib.php 2012-02-12 00:26:13.669765449 +0200 @@ -773,7 +773,7 @@ unset($courses [$id] ); continue; } if (!has_capability('moodle/course:viewhiddencourses', $context, $userid)) { + if (!has_capability('moodle/course:viewhiddencourses', $context, $USER->id)) { unset($courses[$id]); continue; }
          Hide
          Iurii Kucherov added a comment -

          well formatted diff

          diff -u enrollib.php.original enrollib.php
          --- enrollib.php.original	2012-02-12 00:26:02.752760133 +0200
          +++ enrollib.php	2012-02-12 00:26:13.669765449 +0200
          @@ -773,7 +773,7 @@
                               unset($courses[$id]);
                               continue;
                           }
          -                if (!has_capability('moodle/course:viewhiddencourses', $context, $userid)) {
          +                if (!has_capability('moodle/course:viewhiddencourses', $context, $USER->id)) {
                               unset($courses[$id]);
                               continue;
                           }
          
          
          Show
          Iurii Kucherov added a comment - well formatted diff diff -u enrollib.php.original enrollib.php --- enrollib.php.original 2012-02-12 00:26:02.752760133 +0200 +++ enrollib.php 2012-02-12 00:26:13.669765449 +0200 @@ -773,7 +773,7 @@ unset($courses[$id]); continue ; } - if (!has_capability('moodle/course:viewhiddencourses', $context, $userid)) { + if (!has_capability('moodle/course:viewhiddencourses', $context, $USER->id)) { unset($courses[$id]); continue ; }
          Hide
          Iurii Kucherov added a comment -

          Oh I forgot to add global $USER

          So the last variant is

          --- enrollib.php.original	2012-02-12 00:26:02.752760133 +0200
          +++ enrollib.php	2012-02-14 00:02:58.945752600 +0200
          @@ -690,7 +690,7 @@
            * @return array
            */
           function enrol_get_users_courses($userid, $onlyactive = false, $fields = NULL, $sort = 'visible DESC,sortorder ASC') {
          -    global $DB;
          +    global $DB, $USER;
           
               // Guest account does not have any courses
               if (isguestuser($userid) or empty($userid)) {
          @@ -773,7 +773,7 @@
                               unset($courses[$id]);
                               continue;
                           }
          -                if (!has_capability('moodle/course:viewhiddencourses', $context, $userid)) {
          +                if (!has_capability('moodle/course:viewhiddencourses', $context, $USER->id)) {
                               unset($courses[$id]);
                               continue;
                           }
          
          Show
          Iurii Kucherov added a comment - Oh I forgot to add global $USER So the last variant is --- enrollib.php.original 2012-02-12 00:26:02.752760133 +0200 +++ enrollib.php 2012-02-14 00:02:58.945752600 +0200 @@ -690,7 +690,7 @@ * @return array */ function enrol_get_users_courses($userid, $onlyactive = false, $fields = NULL, $sort = 'visible DESC,sortorder ASC') { - global $DB; + global $DB, $USER; // Guest account does not have any courses if (isguestuser($userid) or empty($userid)) { @@ -773,7 +773,7 @@ unset($courses[$id]); continue; } - if (!has_capability('moodle/course:viewhiddencourses', $context, $userid)) { + if (!has_capability('moodle/course:viewhiddencourses', $context, $USER->id)) { unset($courses[$id]); continue; }
          Show
          Iurii Kucherov added a comment - Added link to github https://github.com/yuyokk/moodle/commit/7b32312fe14d711e28a06fb8a22692025b8b6d99
          Hide
          Ankit Agarwal added a comment -

          @Iurii Kucherov
          Thanks for the patch.
          I have edited the commit msg a little.

          Sending this for review.

          @integrators
          This probabily should go into master only, since this function is used at many places. It should be clean cherry-pick if need to be backported.

          Thanks

          Show
          Ankit Agarwal added a comment - @Iurii Kucherov Thanks for the patch. I have edited the commit msg a little. Sending this for review. @integrators This probabily should go into master only, since this function is used at many places. It should be clean cherry-pick if need to be backported. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          This looks good plus should be backported.

          +1 for integration.

          Show
          Rossiani Wijaya added a comment - Hi Ankit, This looks good plus should be backported. +1 for integration.
          Hide
          Ankit Agarwal added a comment -

          Thanks Rosie for the review.
          Updated stable branches
          Sending for integration
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Rosie for the review. Updated stable branches Sending for integration Thanks
          Hide
          Petr Škoda added a comment -

          Hello, this patch changes API, it may fix problem on profiles but it would break other uses of this function, sorry, it can not be integrated.

          I am proposing to add new parameter that describes who is going to look at the list of courses or alternatively option to not verify visibility at all.

          Show
          Petr Škoda added a comment - Hello, this patch changes API, it may fix problem on profiles but it would break other uses of this function, sorry, it can not be integrated. I am proposing to add new parameter that describes who is going to look at the list of courses or alternatively option to not verify visibility at all.
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,

          I was having a chat with Ankit about another issue and he asked me what I thought of this issue.
          You are 100% correct in that the current solution can't go in as it changes the API, however I am unsure whether the new param would be the best way to proceed.
          Here's how I see it at the moment (and perhaps I am missing something).
          If we add a new param we'd have to make it default to null, and then if it is null within the function assign $userid to it. That would ensure things continued to work as they do now, but with the change is scope we are looking for.
          However its that change in scope that I'm not sure about.
          This new param changes the function from "Returning all courses the requested user is enrolled in and can view" to "Return all courses the requrested user is enrolled in and the current user can see".

          I just wonder whether we would be better off splitting this into two functions to make it clearer exactly what is going on, having a enrol_get_users_courses_for_display may help make the difference in functionality easier to understand.

          Anyway food for thought that one.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, I was having a chat with Ankit about another issue and he asked me what I thought of this issue. You are 100% correct in that the current solution can't go in as it changes the API, however I am unsure whether the new param would be the best way to proceed. Here's how I see it at the moment (and perhaps I am missing something). If we add a new param we'd have to make it default to null, and then if it is null within the function assign $userid to it. That would ensure things continued to work as they do now, but with the change is scope we are looking for. However its that change in scope that I'm not sure about. This new param changes the function from "Returning all courses the requested user is enrolled in and can view" to "Return all courses the requrested user is enrolled in and the current user can see". I just wonder whether we would be better off splitting this into two functions to make it clearer exactly what is going on, having a enrol_get_users_courses_for_display may help make the difference in functionality easier to understand. Anyway food for thought that one. Cheers Sam
          Hide
          Petr Škoda added a comment -

          Yeah, new function might be the best and least confusing solution.

          Show
          Petr Škoda added a comment - Yeah, new function might be the best and least confusing solution.
          Hide
          Ankit Agarwal added a comment -

          Thanks guys for feedback..
          I will create a new function enrol_get_users_courses_display that uses logged in user's id to make the cap check and also create another issue to review all existing uses of enrol_get_users_courses and replace with enrol_get_users_courses_display if applicable.

          Show
          Ankit Agarwal added a comment - Thanks guys for feedback.. I will create a new function enrol_get_users_courses_display that uses logged in user's id to make the cap check and also create another issue to review all existing uses of enrol_get_users_courses and replace with enrol_get_users_courses_display if applicable.
          Hide
          Rossiani Wijaya added a comment -

          Patch looks good Ankit.

          Show
          Rossiani Wijaya added a comment - Patch looks good Ankit.
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) 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
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) 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
          Sam Hemelryk added a comment -

          Hi Ankit,

          I've just been looking at this now. I'm not entirely sure about the name of this new function and think I will discuss it with Eloy, so there will be a delay on the integration of this issue sorry.
          I'll also discuss with Eloy whether we should backport this or not, as its a bug ideally we will backport it otherwise we will still need to find a fix for the stable branches.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit, I've just been looking at this now. I'm not entirely sure about the name of this new function and think I will discuss it with Eloy, so there will be a delay on the integration of this issue sorry. I'll also discuss with Eloy whether we should backport this or not, as its a bug ideally we will backport it otherwise we will still need to find a fix for the stable branches. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit,

          The more I looked at this the more I didn't like this new function.
          I had a very good chat with Eloy just now in which we discussed the function and its uses and we think we've come up with a good plan.
          While discussing I think Eloy nailed the current situation in saying that really the function should not be making those checks, in doing so it is mixing enrolment logic and visualisation.
          What I really didn't like about the current solution is the awful amount of duplication that would be going on.
          Also in further reviewing the uses of the current function both Eloy and myself found all the uses we looked at (by no means all) would need to reviewed as most needed to be converted to the new function and several were making the same checks again anyway.

          The plan that we have come up with is as follows:

          1. Create a new function called enrol_get_all_users_courses or something and have it do exactly what the existing function is doing but without the checks (without any checks!)
          2. Change the existing function to call the new function and perform the checks it wants.
          3. Review all uses of the existing function and change to the new function where required. At the same time check that they are making the checks that they require.

          In doing this we create a function that returns the courses the user is enrolled in. We perform the checks required for the area using the code. We avoid duplication of code. And hopefully things are clearer.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit, The more I looked at this the more I didn't like this new function. I had a very good chat with Eloy just now in which we discussed the function and its uses and we think we've come up with a good plan. While discussing I think Eloy nailed the current situation in saying that really the function should not be making those checks, in doing so it is mixing enrolment logic and visualisation. What I really didn't like about the current solution is the awful amount of duplication that would be going on. Also in further reviewing the uses of the current function both Eloy and myself found all the uses we looked at (by no means all) would need to reviewed as most needed to be converted to the new function and several were making the same checks again anyway. The plan that we have come up with is as follows: Create a new function called enrol_get_all_users_courses or something and have it do exactly what the existing function is doing but without the checks (without any checks!) Change the existing function to call the new function and perform the checks it wants. Review all uses of the existing function and change to the new function where required. At the same time check that they are making the checks that they require. In doing this we create a function that returns the courses the user is enrolled in. We perform the checks required for the area using the code. We avoid duplication of code. And hopefully things are clearer. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          Hi Sam,
          Makes sense to me. But this solution is only for master right? or we can introduce the new Api in stable as well?

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, Makes sense to me. But this solution is only for master right? or we can introduce the new Api in stable as well? Thanks
          Hide
          Sam Hemelryk added a comment -

          New API will be fine to backport, but it is essential that we don't change the way the current API functions.

          Show
          Sam Hemelryk added a comment - New API will be fine to backport, but it is essential that we don't change the way the current API functions.
          Hide
          Rossiani Wijaya added a comment -

          This looks good Ankit.

          You might want to fix the url for 2.1

          Show
          Rossiani Wijaya added a comment - This looks good Ankit. You might want to fix the url for 2.1
          Hide
          Ankit Agarwal added a comment -

          Thanks Rosie for the review.
          Sending for integration
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration Thanks
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Ankit Agarwal added a comment -

          rebased!

          Show
          Ankit Agarwal added a comment - rebased!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, Ankit,

          definitively the split sounds spot. And the 2 uses changed also look ok, as far as both perform, later, the desired 'moodle/course:viewhiddencourses' cap check for $USER.

          Also the enrol_get_users_courses() function continues behaving exactly the same, so BC use is guaranteed.

          So I'm going to integrate this right now, thanks!

          Apart from that there are some points I'm don't get about the original (and preserved) logic:

          • Why the capability checks are only performed if $onlyactive. Beyond my imagination, really.
          • The enrol_get_users_courses() function is only suitable for returning information about one user to be displayed by that same user, as far as the capability checks are only performed for that user.

          Both points make that function really tricky, as far as it return different results based on the initially unrelated $onlyactive parameter. More yet, the use of such parameter invalidates the use of the function for anybody not being the same $userid. And those "tricks" are not really obvious in the phpdocs.

          IMO, in the long term, we should go to some standard way in a lot of APIs to be able to specify always the user we want to check caps, defaulting to $USER if not specified. Or, else, separate clearly the execution logic from the perms check.

          But all this is literature... just writing it down to share my feeling... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, Ankit, definitively the split sounds spot. And the 2 uses changed also look ok, as far as both perform, later, the desired 'moodle/course:viewhiddencourses' cap check for $USER. Also the enrol_get_users_courses() function continues behaving exactly the same, so BC use is guaranteed. So I'm going to integrate this right now, thanks! Apart from that there are some points I'm don't get about the original (and preserved) logic: Why the capability checks are only performed if $onlyactive. Beyond my imagination, really. The enrol_get_users_courses() function is only suitable for returning information about one user to be displayed by that same user, as far as the capability checks are only performed for that user. Both points make that function really tricky, as far as it return different results based on the initially unrelated $onlyactive parameter. More yet, the use of such parameter invalidates the use of the function for anybody not being the same $userid. And those "tricks" are not really obvious in the phpdocs. IMO, in the long term, we should go to some standard way in a lot of APIs to be able to specify always the user we want to check caps, defaulting to $USER if not specified. Or, else, separate clearly the execution logic from the perms check. But all this is literature... just writing it down to share my feeling... ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22 & master), thanks!

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

          Hi Eloy,
          I completely agree with you. I was also not able to think of any situation in which we are actually going to use enrol_get_users_courses(). I guess it was intended to use $USER instead.

          Anyways it would be great to deprecate this function in 2.3 and remove it completely later on.

          What we can do is:-
          -> deprecate enrol_get_users_courses()
          -> Introduce a function similar to it, which can accept a $user param to check cap (defaults to $USER) OR take the cap check logic completely out of the function.
          -> Review all present instances to make appropriate changes

          Let me know your views, I will create a new issue for the same, once we all agree on what needs to be done.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Eloy, I completely agree with you. I was also not able to think of any situation in which we are actually going to use enrol_get_users_courses(). I guess it was intended to use $USER instead. Anyways it would be great to deprecate this function in 2.3 and remove it completely later on. What we can do is:- -> deprecate enrol_get_users_courses() -> Introduce a function similar to it, which can accept a $user param to check cap (defaults to $USER) OR take the cap check logic completely out of the function. -> Review all present instances to make appropriate changes Let me know your views, I will create a new issue for the same, once we all agree on what needs to be done. Thanks
          Hide
          Adrian Greeve added a comment -

          Tested in versions 2.1, 2.2 and master. The course information in the profile pages now shows the hidden courses.
          Thanks.

          Show
          Adrian Greeve added a comment - Tested in versions 2.1, 2.2 and master. The course information in the profile pages now shows the hidden courses. Thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: