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
    • Rank:
      15960

      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;
          }
      
      

        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 Škoda 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 Škoda 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 Škoda added a comment -

          Reopening, see the PULL request for details, thanks.

          Show
          Petr Škoda 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 Škoda added a comment -

          Reopening, please see PULL request for details. Thanks

          Show
          Petr Škoda 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 Škoda added a comment -

          reopening, please see the PULL request for details.

          Show
          Petr Škoda 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 Škoda 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 Škoda 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 Škoda added a comment -

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

          I did not review the rest...

          Show
          Petr Škoda 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: