Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            mchurch 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
            mchurch 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            mchurch 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
            mchurch 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
            mchurch 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
            mchurch 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
            martinlanghoff 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
            martinlanghoff 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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:
                  Fix Release Date:
                  24/Nov/10