Moodle
  1. Moodle
  2. MDL-10161

get_course_users and get_course_students don't work the same as they used to for site level

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.7, 1.8
    • Fix Version/s: 2.0
    • Component/s: Roles / Access
    • Labels:
      None
    • Affected Branches:
      MOODLE_17_STABLE, MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      3863

      Description

      "get_course_users" and "get_course_students" have been moved to "deprecatedlib", but continue to be used by a number of existing and new code. Unfortunately, when they were modified to use the new roles and capabilities functions they lost some of their existing function.

      The "get_course_students" previously would return all users who were considered students at the site level. This was dependent on the "$CFG->allusersaresitestudents" variable being set to true, and excluded users specifically assigned as teachers to the site course. Now, it returns users who have the "moodle/legacy:student" capability. This works if you manually assign users to the 'student' role at the site level. However, the "$CFG->allusersaresitestudents" has been replaced by the "user policies" setting "defaultuserroleid". If this is set to "student", "get_course_students" does not return all the users it should.

      Likewise, "get_course_users" previously used the old "get_course_students" and "get_course_teachers". Now, it gets all users with the "moodle/course:view" capability. This capability doesn't seem to get set for a user unless they are specifically given a role with that capability. Setting the "defaultuserroleid" does not do this.

      These functions need to be updated to handle the "defaultuserroleid" setting for the site course. And, whatever they are replaced with (after they are removed from "deprecatedlib") also needs to be able to handle this.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          I agree that it would be good to fix the deprecated the legacy functions somehow and deal with it properly in 2.0 because it might need some greater changes in roles/enrolments.

          Please attach any patches and I will help with review and testing

          Show
          Petr Škoda added a comment - I agree that it would be good to fix the deprecated the legacy functions somehow and deal with it properly in 2.0 because it might need some greater changes in roles/enrolments. Please attach any patches and I will help with review and testing
          Hide
          Mike Churchward added a comment - - edited

          I was thinking of something in line of:

          /**

          • Returns all the users of a course: students and teachers
            *
          • @param int $courseid The course in question.
          • @param string $sort ?
          • @param string $exceptions ?
          • @param string $fields A comma separated list of fields to be returned from the chosen table.
          • @return object
          • @todo Finish documenting this function
            */
            function get_course_users($courseid, $sort='ul.timeaccess DESC', $exceptions='', $fields='') {
            global $CFG;

          $context = get_context_instance(CONTEXT_COURSE, $courseid);

          /// If the course id is the SITEID, we need to return all the users if the "defaultuserroleid"
          /// has the capbility of accessing the site course.
          if (($courseid == SITEID) && isset($CFG->defaultuserroleid)) {
          if ($roles = get_roles_with_capability('moodle/course:view', CAP_ALLOW, $context)) {
          $hascap = false;
          foreach ($roles as $role) {
          if ($role->id == $CFG->defaultuserroleid)

          { $hascap = true; break; }

          }
          if ($hascap) {
          if (empty($fields))

          { $fields = '*'; }

          return get_users(true, '', true, $exceptions, 'lastname ASC', '', '', '', '', $fields);
          } else

          { return false; }

          } else

          { return false; }

          }
          return get_users_by_capability($context, 'moodle/course:view', 'u.*, ul.timeaccess as lastaccess', $sort, '','','',$exceptions, false);

          }

          for get_course_users.

          Show
          Mike Churchward added a comment - - edited I was thinking of something in line of: /** Returns all the users of a course: students and teachers * @param int $courseid The course in question. @param string $sort ? @param string $exceptions ? @param string $fields A comma separated list of fields to be returned from the chosen table. @return object @todo Finish documenting this function */ function get_course_users($courseid, $sort='ul.timeaccess DESC', $exceptions='', $fields='') { global $CFG; $context = get_context_instance(CONTEXT_COURSE, $courseid); /// If the course id is the SITEID, we need to return all the users if the "defaultuserroleid" /// has the capbility of accessing the site course. if (($courseid == SITEID) && isset($CFG->defaultuserroleid)) { if ($roles = get_roles_with_capability('moodle/course:view', CAP_ALLOW, $context)) { $hascap = false; foreach ($roles as $role) { if ($role->id == $CFG->defaultuserroleid) { $hascap = true; break; } } if ($hascap) { if (empty($fields)) { $fields = '*'; } return get_users(true, '', true, $exceptions, 'lastname ASC', '', '', '', '', $fields); } else { return false; } } else { return false; } } return get_users_by_capability($context, 'moodle/course:view', 'u.*, ul.timeaccess as lastaccess', $sort, '','','',$exceptions, false); } for get_course_users.
          Hide
          Petr Škoda added a comment -

          it should IMO work, though we could add some configuration switch to prevent potential problems on large sites - such as the old one - readd $CFG->allusersaresitestudents ?
          also the isset($CFG->defaultuserroleid)) might be better with empty in case the id is 0

          Show
          Petr Škoda added a comment - it should IMO work, though we could add some configuration switch to prevent potential problems on large sites - such as the old one - readd $CFG->allusersaresitestudents ? also the isset($CFG->defaultuserroleid)) might be better with empty in case the id is 0
          Hide
          Mike Churchward added a comment -

          I think a config variable like you are suggesting is a good idea. Should this go into the 'config' file or the admin interface (I'm thinking the UI is better)? The places that use these functions really need to be updated to handle large amounts of users, but that will take a lot more time. By the way, this would also be a problem now, if a course had an unusually large number of enrolees.

          I would suggest a more obvious config variable. Something like '$CFG->nodefaultuserrolelists' set to TRUE to prevent 'get_course_users and 'get_site_users' from returning all the users.

          Thoughts?

          mike

          Show
          Mike Churchward added a comment - I think a config variable like you are suggesting is a good idea. Should this go into the 'config' file or the admin interface (I'm thinking the UI is better)? The places that use these functions really need to be updated to handle large amounts of users, but that will take a lot more time. By the way, this would also be a problem now, if a course had an unusually large number of enrolees. I would suggest a more obvious config variable. Something like '$CFG->nodefaultuserrolelists' set to TRUE to prevent 'get_course_users and 'get_site_users' from returning all the users. Thoughts? mike
          Hide
          Mike Churchward added a comment -

          Petr. I committed changes to HEAD for this. Updated 'deprecatedlib' as well as the user policy admin page and the admin language file. I think we should merge this into 1.8 too.

          mike

          Show
          Mike Churchward added a comment - Petr. I committed changes to HEAD for this. Updated 'deprecatedlib' as well as the user policy admin page and the admin language file. I think we should merge this into 1.8 too. mike
          Hide
          Martín Langhoff added a comment -

          Peter Bulmer @ catalyst is also looking at this, but I disagree that this needs a patch.

          This can be achieved with an override to the default role for the logged in user that provides moodle/course:view . So at the most we need to "move" the setting from $CFG to an override, and provide a hint that we moved so that admins looking for it find a note that explains how to manage it now, in the brave new world of roles and capabilities.

          Show
          Martín Langhoff added a comment - Peter Bulmer @ catalyst is also looking at this, but I disagree that this needs a patch. This can be achieved with an override to the default role for the logged in user that provides moodle/course:view . So at the most we need to "move" the setting from $CFG to an override, and provide a hint that we moved so that admins looking for it find a note that explains how to manage it now, in the brave new world of roles and capabilities.
          Hide
          Petr Škoda added a comment -

          Hello,
          this was reimplemented in 2.O again, it is now a bit more similar to the old style teachers&users in 1.6.

          Please test and file new issue if necessary, thanks for the report.

          Petr Škoda

          Show
          Petr Škoda added a comment - Hello, this was reimplemented in 2.O again, it is now a bit more similar to the old style teachers&users in 1.6. Please test and file new issue if necessary, thanks for the report. Petr Škoda

            People

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

              Dates

              • Created:
                Updated:
                Resolved: