Moodle
  1. Moodle
  2. MDL-26250

Create a web service function that enrols users to a certain course

    Details

    • Testing Instructions:
      Hide

      This web service function is for developer so it needs to be tested by developer.

      In order to test, create a web service client and call the web service function. A good test is to try to to enrol two users in two differents courses.

      Look at http://docs.moodle.org/en/Web_Services for information how to create a web service client.

      Show
      This web service function is for developer so it needs to be tested by developer. In order to test, create a web service client and call the web service function. A good test is to try to to enrol two users in two differents courses. Look at http://docs.moodle.org/en/Web_Services for information how to create a web service client.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull from Repository:
      git@github.com:mouneyrac/moodle.git
    • Pull Master Branch:
      MDL-26250-final6

      Description

      Something like that (I'm just writing this on the fly, need to finish):

      moodle_enrol_users:
      ------------------

      public static function enrol_users_parameters() {
              return new external_function_parameters(
                  array(
                      'enrolments' => new external_multiple_structure(
                          new external_single_structure(
                              array(
                                  'roleid'    => new external_value(PARAM_INT, 'Role to assign to the user'),
                                  'userid'    => new external_value(PARAM_INT, 'The user that is going to be enrolled'),
                                  'contextid' => new external_value(PARAM_INT, 'The context to enrol the user role in'),
                              )
                          )
                      )
                  )
              );
          }
       
      /**
           * Manual enrolment of users
           *
           * @param array $enrolments  An array of manual user enrolment
           * @return null
           */
          public static function enrol_users($enrolments) {
              global $DB, $USER;
       
              $params = self::validate_parameters(self::enrol_users_parameters(), array('enrolments'=>$enrolments));
       
              $transaction = $DB->start_delegated_transaction();
       
              foreach ($params['enrolments'] as $enrolment) {
                  // Ensure the current user is allowed to run this function in the enrolment context
                  $context = get_context_instance_by_id($enrolment['contextid']);
                  self::validate_context($context);
                  require_capability('moodle/role:assign', $context); //TODO: there is probably a capability related to enrolment, look for it and check on it it.
       
                  $instance = $DB->get_record('enrol', array('courseid' => $context->instanceid, 'enrol' => 'manual'));
                  $enrol = enrol_get_plugin('manual');
                  $enrol->enrol_user($instance, $enrolment['userid'], $enrolment['roleid']);
       
                  //TODO: maybe return a array of success/fail
              }
       
              $transaction->allow_commit();
          }
       
       /**
           * Returns description of method result value
           * @return external_description
           */
          public static function enrol_users_returns() {
              return null;
          }
       

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Martin Dougiamas added a comment -

            Definitely definitely should be done ASAP. This is a huge hole in WS at the moments.

            Show
            Martin Dougiamas added a comment - Definitely definitely should be done ASAP. This is a huge hole in WS at the moments.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Just a note: this function should accept any enrolment method (default to manual). If the enrolment plugin not installed Moodle throw an exception.

            Show
            Jérôme Mouneyrac added a comment - - edited Just a note: this function should accept any enrolment method (default to manual). If the enrolment plugin not installed Moodle throw an exception.
            Hide
            Petr Skoda added a comment -

            This can not be implemented for all plugins! Please have a look at the enrol UI code - it has to find some plugin that actually allows manual enrolments, otherwise you might create a big mess in the enrol tables...

            Ideally this should be implemented inside each enrol plugin that makes this possible.

            Show
            Petr Skoda added a comment - This can not be implemented for all plugins! Please have a look at the enrol UI code - it has to find some plugin that actually allows manual enrolments, otherwise you might create a big mess in the enrol tables... Ideally this should be implemented inside each enrol plugin that makes this possible.
            Hide
            Petr Skoda added a comment -

            Reopening, see the PULL request for details, thanks.

            Show
            Petr Skoda added a comment - Reopening, see the PULL request for details, thanks.
            Hide
            Heiko Schach added a comment -

            Hello, good to see there's progress with the web service enrolment functions.

            Could this function be made to work directly with mdl_course:id?

            As of now, enrolling users through web services with a course id would be a two-step process. One has to find out the course's context id with a web service function described in MDL-26251 before the enrolment can be done.
            Using mdl_course:id (mdl_context:instanceid) directly would be more straightforward as external systems wouldn't need to bother with Moodle's internals in the mdl_context -table.

            How does this function relate to MDL-21524 Enrolment functions?

            Show
            Heiko Schach added a comment - Hello, good to see there's progress with the web service enrolment functions. Could this function be made to work directly with mdl_course:id? As of now, enrolling users through web services with a course id would be a two-step process. One has to find out the course's context id with a web service function described in MDL-26251 before the enrolment can be done. Using mdl_course:id (mdl_context:instanceid) directly would be more straightforward as external systems wouldn't need to bother with Moodle's internals in the mdl_context -table. How does this function relate to MDL-21524 Enrolment functions?
            Hide
            Jérôme Mouneyrac added a comment -

            Hi Heiko,
            you are right about courseid. If you look at the pull request you'll see the patch I suggested finally uses courseid (not contextid).
            This web service function being a web service enrolment function it is related to the task MDL-21524 (being itself a subtask of MDL-20806). It sounds like a tracking game, hehe.

            Hopefully we'll get this function into core very soon.

            Show
            Jérôme Mouneyrac added a comment - Hi Heiko, you are right about courseid. If you look at the pull request you'll see the patch I suggested finally uses courseid (not contextid). This web service function being a web service enrolment function it is related to the task MDL-21524 (being itself a subtask of MDL-20806 ). It sounds like a tracking game, hehe. Hopefully we'll get this function into core very soon.
            Show
            Jérôme Mouneyrac added a comment - Please Petr can you review https://github.com/mouneyrac/moodle/commit/4cf01237c9018af0ffed8029d28b4250a13328a9 thanks
            Hide
            Jérôme Mouneyrac added a comment -
            Show
            Jérôme Mouneyrac added a comment - + throw an exception if an enrolment failed: https://github.com/mouneyrac/moodle/commit/3786e287f0026bfc67bc460fef51d8a6b3b6ed55
            Hide
            Jérôme Mouneyrac added a comment -

            Created a PULL request to accelerate the review

            Show
            Jérôme Mouneyrac added a comment - Created a PULL request to accelerate the review
            Hide
            Petr Skoda added a comment -

            Reopening, please see PULL request for details. Thanks

            Show
            Petr Skoda added a comment - Reopening, please see PULL request for details. Thanks
            Hide
            Jérôme Mouneyrac added a comment - - edited

            1/ there is still hardcoded plugin name: require_capability('enrol/manual:enrol', $context);

            In this case which capability to check? I read allow_enrol() and user_enrol() I didn't see any of these functions doing capability checking?

            2/ there are more parameters in the enrol function, I suppose they might be handy too sometimes: $roleid = NULL, $timestart = 0, $timeend = 0, $status = NULL

            $roleid is used, I'll add the three others

            Show
            Jérôme Mouneyrac added a comment - - edited 1/ there is still hardcoded plugin name: require_capability('enrol/manual:enrol', $context); In this case which capability to check? I read allow_enrol() and user_enrol() I didn't see any of these functions doing capability checking? 2/ there are more parameters in the enrol function, I suppose they might be handy too sometimes: $roleid = NULL, $timestart = 0, $timeend = 0, $status = NULL $roleid is used, I'll add the three others
            Hide
            Jérôme Mouneyrac added a comment -

            ok for 1) I found it:

             && has_capability('enrol/'.$plugin->get_name().':enrol', $context)
            

            so don't worry I'll create a new fixed pull request

            Show
            Jérôme Mouneyrac added a comment - ok for 1) I found it: && has_capability('enrol/'.$plugin->get_name().':enrol', $context) so don't worry I'll create a new fixed pull request
            Hide
            Petr Skoda added a comment -

            reopening, please see the PULL request for details.

            Show
            Petr Skoda added a comment - reopening, please see the PULL request for details.
            Show
            Jérôme Mouneyrac added a comment - removed all references to manual: https://github.com/mouneyrac/moodle/commit/32a94bfb09688fa3395abb79433613507e4b55fd
            Hide
            Jérôme Mouneyrac added a comment -

            integration git server has just been updated, PULL request need to be recreated.

            Show
            Jérôme Mouneyrac added a comment - integration git server has just been updated, PULL request need to be recreated.
            Hide
            Helen Foster added a comment -

            Reopening - please see PULL-515 for reasons. Removing STABLE sprint 8 as fix version.

            Jerome, thanks for your work on this issue. Looking forward to a new pull request

            Show
            Helen Foster added a comment - Reopening - please see PULL-515 for reasons. Removing STABLE sprint 8 as fix version. Jerome, thanks for your work on this issue. Looking forward to a new pull request
            Hide
            Jérôme Mouneyrac added a comment -

            I put it back in Sprint Helen.

            Petr can you review the changes (I switch the generic function into a specific manual enrol function):
            https://github.com/mouneyrac/moodle/commit/2c56acb212bfe14da6b1f733c25c77ec2ba5a148

            Show
            Jérôme Mouneyrac added a comment - I put it back in Sprint Helen. Petr can you review the changes (I switch the generic function into a specific manual enrol function): https://github.com/mouneyrac/moodle/commit/2c56acb212bfe14da6b1f733c25c77ec2ba5a148
            Hide
            Martin Dougiamas added a comment -

            Reopening as this is not integrated yet.

            Show
            Martin Dougiamas added a comment - Reopening as this is not integrated yet.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            This is the current user_can_assign($context, $targetroleid) core function that I called in my last rejected pull request:

            /**
             * Checks if a user can assign users to a particular role in this context
             *
             * @param object $context
             * @param int $targetroleid - the id of the role you want to assign users to
             * @return boolean
             */
            function user_can_assign($context, $targetroleid) {
                global $DB;
             
                // first check if user has override capability
                // if not return false;
                if (!has_capability('moodle/role:assign', $context)) {
                    return false;
                }
                // pull out all active roles of this user from this context(or above)
                if ($userroles = get_user_roles($context)) {
                    foreach ($userroles as $userrole) {
                        // if any in the role_allow_override table, then it's ok
                        if ($DB->get_record('role_allow_assign', array('roleid'=>$userrole->roleid, 'allowassign'=>$targetroleid))) {
                            return true;
                        }
                    }
                }
             
                return false;
            }
            

            This function will return false for admin if it doesn't go through the if condition. It seemsto me it is a function that return true if a user can assign a role to a context (phpdoc/function name). It's why it seemed to me as admin should always be able to assign, I proposed a little fix.

            I think either this function is confusing developers, either I'm not that wrong with my previous fix I guess it's the first one (wrong name/phpdoc) when I read Petr comment.

            It is that correct?

            Note that I knew about get_assignable_roles() function but user_can_assign() looked to be matching perfectly what I wanted without go through all assignables roles to see if the user has it.

            Show
            Jérôme Mouneyrac added a comment - - edited This is the current user_can_assign($context, $targetroleid) core function that I called in my last rejected pull request: /** * Checks if a user can assign users to a particular role in this context * * @param object $context * @param int $targetroleid - the id of the role you want to assign users to * @return boolean */ function user_can_assign($context, $targetroleid) { global $DB;   // first check if user has override capability // if not return false; if (!has_capability('moodle/role:assign', $context)) { return false; } // pull out all active roles of this user from this context(or above) if ($userroles = get_user_roles($context)) { foreach ($userroles as $userrole) { // if any in the role_allow_override table, then it's ok if ($DB->get_record('role_allow_assign', array('roleid'=>$userrole->roleid, 'allowassign'=>$targetroleid))) { return true; } } }   return false; } This function will return false for admin if it doesn't go through the if condition. It seemsto me it is a function that return true if a user can assign a role to a context (phpdoc/function name). It's why it seemed to me as admin should always be able to assign, I proposed a little fix. I think either this function is confusing developers, either I'm not that wrong with my previous fix I guess it's the first one (wrong name/phpdoc) when I read Petr comment. It is that correct? Note that I knew about get_assignable_roles() function but user_can_assign() looked to be matching perfectly what I wanted without go through all assignables roles to see if the user has it.
            Hide
            Petr Skoda added a comment -

            user_can_assign() misses very important limitation introduced in 2.0 - allowed context levels, the admin fix was ok, but not enough

            Show
            Petr Skoda added a comment - user_can_assign() misses very important limitation introduced in 2.0 - allowed context levels, the admin fix was ok, but not enough
            Hide
            Jérôme Mouneyrac added a comment -

            no problem, I'll use get_assignable_roles(), write an issue for the cache improvement and push a new patch to github.

            Show
            Jérôme Mouneyrac added a comment - no problem, I'll use get_assignable_roles(), write an issue for the cache improvement and push a new patch to github.
            Hide
            Jérôme Mouneyrac added a comment -

            I'll write an issue for user_can_assign too

            Show
            Jérôme Mouneyrac added a comment - I'll write an issue for user_can_assign too
            Hide
            Jérôme Mouneyrac added a comment -

            I just understood that I was omitting that roles can be disallowed to some courses, and though that we are admin or not. Petr just explained me by chat. Thanks.

            Show
            Jérôme Mouneyrac added a comment - I just understood that I was omitting that roles can be disallowed to some courses, and though that we are admin or not. Petr just explained me by chat. Thanks.
            Hide
            Jérôme Mouneyrac added a comment -

            Done

            Show
            Jérôme Mouneyrac added a comment - Done
            Hide
            Jérôme Mouneyrac added a comment -

            Rosie, you have been randomly selected for peer reviewing

            Show
            Jérôme Mouneyrac added a comment - Rosie, you have been randomly selected for peer reviewing
            Hide
            Rossiani Wijaya added a comment -

            Hi Jerome,

            The patch looks great and it works.
            Just a side note, on testwebservice.php, you probably wants to add some indication on the lines that required value changes.

            Rosie

            Show
            Rossiani Wijaya added a comment - Hi Jerome, The patch looks great and it works. Just a side note, on testwebservice.php, you probably wants to add some indication on the lines that required value changes. Rosie
            Hide
            Jérôme Mouneyrac added a comment -

            Good for integration.

            Show
            Jérôme Mouneyrac added a comment - Good for integration.
            Hide
            Petr Skoda added a comment -

            enrol/manual services should be listed in enrol/manual/db/services.php

            I did not review the rest...

            Show
            Petr Skoda added a comment - enrol/manual services should be listed in enrol/manual/db/services.php I did not review the rest...
            Hide
            Sam Hemelryk added a comment -

            Hi Jerome,
            I've just been looking at this now and have noted the following:

            1. externallib.php line:88 The require capability call should really be performed before the get record call for instance (very minor).
            2. externallib.php line:91 The record exists call is superfluous because the very next check is against assignable roles.
            3. services.php: Shouldn't the definition be added to enrol/manual/db/services.php instead?
            4. testwebservice.php: Is it possible/practical to move the enrol_manual_enrol_users tests to enrol/manual/simpletest/... ?

            The last two points are just about positioning, it'd be great if it were possible to move them as it would provide a great code example for those interested in writing web services for enrolment plugins and the like.

            The other thing I noticed however I am unsure whether it is intentional or not is that when you are fetching the manual enrolment instance you are not checking whether is active or not, or even whether the manual plugin as a whole is enabled.
            Perhaps a better solution would be to use enrol_get_instance($courseid) and then iterate the resulting instances until you find $instance->enrol == 'manual' or you get to the end of the array?
            That was you get all of those checks included, you are sure to get the first enabled manual enrolment plugin instance.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Jerome, I've just been looking at this now and have noted the following: externallib.php line:88 The require capability call should really be performed before the get record call for instance (very minor). externallib.php line:91 The record exists call is superfluous because the very next check is against assignable roles. services.php: Shouldn't the definition be added to enrol/manual/db/services.php instead? testwebservice.php: Is it possible/practical to move the enrol_manual_enrol_users tests to enrol/manual/simpletest/... ? The last two points are just about positioning, it'd be great if it were possible to move them as it would provide a great code example for those interested in writing web services for enrolment plugins and the like. The other thing I noticed however I am unsure whether it is intentional or not is that when you are fetching the manual enrolment instance you are not checking whether is active or not, or even whether the manual plugin as a whole is enabled. Perhaps a better solution would be to use enrol_get_instance($courseid) and then iterate the resulting instances until you find $instance->enrol == 'manual' or you get to the end of the array? That was you get all of those checks included, you are sure to get the first enabled manual enrolment plugin instance. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Forgot to add that while I didn't actually tested it all looked otherwise very good. Thanks for moving the bulk of it to the manual enrolment plugin by the way.
            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Forgot to add that while I didn't actually tested it all looked otherwise very good. Thanks for moving the bulk of it to the manual enrolment plugin by the way. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Hi Jerome,

            I've just been reviewing this and giving it a test.
            Everything worked very well and I've attached a copy of the test client I used (note if you are testing it you will need to create the web service and then set the params allowed by the script).

            The code looks good the only thing I noted was that in a couple of places within externallib.php you indented 2 spaces rather than 4, but that could probably be fixed on integration.

            In regards to integration I'll talk to Eloy today when he comes on, if we agree that it is fine to go in today then I'll integrate it this evening, otherwise it will have to wait till next Wednesday.
            Is there any urgency to get this into 2.0.3?

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Jerome, I've just been reviewing this and giving it a test. Everything worked very well and I've attached a copy of the test client I used (note if you are testing it you will need to create the web service and then set the params allowed by the script). The code looks good the only thing I noted was that in a couple of places within externallib.php you indented 2 spaces rather than 4, but that could probably be fixed on integration. In regards to integration I'll talk to Eloy today when he comes on, if we agree that it is fine to go in today then I'll integrate it this evening, otherwise it will have to wait till next Wednesday. Is there any urgency to get this into 2.0.3? Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Thanks Jerome this has been integrated to master now.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Jerome this has been integrated to master now. Cheers Sam
            Hide
            Eloy Lafuente (stronk7) added a comment -

            wow, wow, wow, was just reading the forums when I arrived here. It seems that this issue was missing the "ci" label, hence it doesn't appear in the integration dashboard, hence has been integrated and sent upstream WITHOUT testing!

            Also it's missing the corresponding pullweek-2011-18 label.

            Plz, fix and test.

            Sidenote: I'll try to improve the filter used by the dashboard so any issue in statuses like this will be shown too, no matter if they have the "ci" label or no.

            Show
            Eloy Lafuente (stronk7) added a comment - wow, wow, wow, was just reading the forums when I arrived here. It seems that this issue was missing the "ci" label, hence it doesn't appear in the integration dashboard, hence has been integrated and sent upstream WITHOUT testing! Also it's missing the corresponding pullweek-2011-18 label. Plz, fix and test. Sidenote: I'll try to improve the filter used by the dashboard so any issue in statuses like this will be shown too, no matter if they have the "ci" label or no.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Done, now the dashboard reveals this issue because I've changed the "Weekly: Current integration" filter. Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Done, now the dashboard reveals this issue because I've changed the "Weekly: Current integration" filter. Ciao
            Hide
            Sam Hemelryk added a comment -

            Hi Rosie,

            Jerome mentioned you'd tested this previously and that you knew how web services worked.
            Could you please test this again for us. It has been integrated on master and MOODLE_20_STABLE.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Rosie, Jerome mentioned you'd tested this previously and that you knew how web services worked. Could you please test this again for us. It has been integrated on master and MOODLE_20_STABLE. Cheers Sam
            Hide
            Rossiani Wijaya added a comment -

            Sure, no problem.

            Testing it right now.

            Show
            Rossiani Wijaya added a comment - Sure, no problem. Testing it right now.
            Hide
            Rossiani Wijaya added a comment -

            Hi Jerome,

            The function works fine.

            However, testing it from the testwebservice page, the enrollment service works fine as long as all the course has manual enrollment enable. On the other hand, if one of the courses has disabled manual enrollment, the following error occurs:
            Fatal error: Call to a member function unenrol_user() on a non-object in /moodle/webservice/simpletest/testwebservice.php on line 304

            Note:
            I discussed the issue with Jerome and its a bug that occurs in unittest only.

            I will create a separate issue to fix the unittest.

            Marking this as passed.

            Show
            Rossiani Wijaya added a comment - Hi Jerome, The function works fine. However, testing it from the testwebservice page, the enrollment service works fine as long as all the course has manual enrollment enable. On the other hand, if one of the courses has disabled manual enrollment, the following error occurs: Fatal error: Call to a member function unenrol_user() on a non-object in /moodle/webservice/simpletest/testwebservice.php on line 304 Note: I discussed the issue with Jerome and its a bug that occurs in unittest only. I will create a separate issue to fix the unittest. Marking this as passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This is now part of Moodle upstream, you did it possible, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - This is now part of Moodle upstream, you did it possible, thanks!
            Hide
            Lorenz Ulrich added a comment -

            Could it be that FIX VERSION of this issue is wrong? I can't find these changes in 2.0.3.

            Show
            Lorenz Ulrich added a comment - Could it be that FIX VERSION of this issue is wrong? I can't find these changes in 2.0.3.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: