Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-28592

Create a manual_unenrol_users external function

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      This can be tested using admin/webservice/documentation.php to fill out parameters or running https://github.com/moodlehq/sample-ws-clients

      Simple test process:

      Enrol 4 users, then unenrol one, check that the 3 remain, then unenrol two more users, and check that one remains

      Error cases:

      (1) Unenrol a user that is not enrolled. Result: silent failure
      (2) Unenrol a user that does not exist. Result: error user doesn't exist
      (3) Unenrol a user from course that doesn't exist. Result: error course doesn't exist

      Show
      This can be tested using admin/webservice/documentation.php to fill out parameters or running https://github.com/moodlehq/sample-ws-clients Simple test process: Enrol 4 users, then unenrol one, check that the 3 remain, then unenrol two more users, and check that one remains Error cases: (1) Unenrol a user that is not enrolled. Result: silent failure (2) Unenrol a user that does not exist. Result: error user doesn't exist (3) Unenrol a user from course that doesn't exist. Result: error course doesn't exist
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_28_STABLE
    • Fixed Branches:
      MOODLE_29_STABLE
    • Pull Master Branch:
      MDL-28592-master

      Description

      The enrolment web service has a way through the manual plugin to enroll users, but it doesn't have a way to unenroll users. I've been working on writing an unenrol user function in moodleDir/enrol/manual/externallib.php and I can post what I have if you all feel like this is something that should be implemented.

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          Hi, Craig.

          Yes, that sounds quite logical.

          Please share what you create.

          Show
          salvetore Michael de Raadt added a comment - Hi, Craig. Yes, that sounds quite logical. Please share what you create.
          Hide
          cbaker118 Craig Baker added a comment -

          I started out trying to use course_enrolment_manager->get_user_enrolment_components like what's shown in enrol/users.php in the unenrol case, but those functions weren't working. After some digging I discovered $CFG->enrol_plugins_enabled did not exist, even though the entry exists in the config table. I ended up doing it using straight records calls. Its probably not the best way of doing things, so any advice is appreciated.

          	public static function manual_unenrol_users_parameters() {
          		return new external_function_parameters(
          			array(
          			    'enrolments' => new external_multiple_structure(
          				    new external_single_structure(
          					    array(
          						'userid' => new external_value(PARAM_INT, 'The user that is going to be enrolled'),
          						'courseid' => new external_value(PARAM_INT, 'The course to enrol the user role in'),
          					    )
          				    )
          			    )
          			)
          		);
          	}
           
              /**
               * Enrolment of users
               * Function throw an exception at the first error encountered.
               * @param array $enrolments  An array of user enrolment
               * @return null
               */
              public static function manual_unenrol_users($enrolments) {
                  global $DB, $CFG;
           
                  require_once($CFG->libdir . '/enrollib.php');
          	require('../../config.php');
           
                  $params = self::validate_parameters(self::manual_unenrol_users_parameters(),
                          array('enrolments' => $enrolments));
           
                  $transaction = $DB->start_delegated_transaction(); //rollback all enrolment if an error occurs
                                                                     //(except if the DB doesn't support it)
           
                  foreach ($params['enrolments'] as $enrolment) {
           
          		/*
          		I haven't tested the context check yet, 
          		but this is how I think it should look.
          		
          		// Ensure the current user is allowed to run this function in the enrolment context
          		$context = get_context_instance(CONTEXT_COURSE, $enrolment['courseid']);
          		self::validate_context($context);
           
          		//check that the user has the permission to manual enrol
          		require_capability('enrol/manual:unenrol', $context);
          		*/
          		$courseInfo = $DB->get_record('enrol', array('courseid'=>$enrolment['courseid'], 'enrol'=>'manual'), '*', MUST_EXIST); #course and manual enroll
          		$manualCourseID = $courseInfo->id; # number for course and manual enroll
           
          		$ue = $DB->get_record('user_enrolments', array('enrolid'=>$manualCourseID, 'userid'=>$enrolment['userid']));
           
          		$DB->delete_records('user_enrolments', array('id'=>$ue->id));
           
                  }
           
                  $transaction->allow_commit();
              }
           
              /**
               * Returns description of method result value
               * @return external_description
               */
              public static function manual_unenrol_users_returns() {
                  return null;
              }
           

          Show
          cbaker118 Craig Baker added a comment - I started out trying to use course_enrolment_manager->get_user_enrolment_components like what's shown in enrol/users.php in the unenrol case, but those functions weren't working. After some digging I discovered $CFG->enrol_plugins_enabled did not exist, even though the entry exists in the config table. I ended up doing it using straight records calls. Its probably not the best way of doing things, so any advice is appreciated. public static function manual_unenrol_users_parameters() { return new external_function_parameters( array( 'enrolments' => new external_multiple_structure( new external_single_structure( array( 'userid' => new external_value(PARAM_INT, 'The user that is going to be enrolled' ), 'courseid' => new external_value(PARAM_INT, 'The course to enrol the user role in' ), ) ) ) ) ); }   /** * Enrolment of users * Function throw an exception at the first error encountered. * @param array $enrolments An array of user enrolment * @return null */ public static function manual_unenrol_users($enrolments) { global $DB, $CFG;   require_once($CFG->libdir . '/enrollib.php' ); require( '../../config.php' );   $params = self::validate_parameters(self::manual_unenrol_users_parameters(), array( 'enrolments' => $enrolments));   $transaction = $DB->start_delegated_transaction(); //rollback all enrolment if an error occurs //(except if the DB doesn't support it)   foreach ($params[ 'enrolments' ] as $enrolment) {   /* I haven't tested the context check yet, but this is how I think it should look. // Ensure the current user is allowed to run this function in the enrolment context $context = get_context_instance(CONTEXT_COURSE, $enrolment['courseid']); self::validate_context($context);   //check that the user has the permission to manual enrol require_capability('enrol/manual:unenrol', $context); */ $courseInfo = $DB->get_record( 'enrol' , array( 'courseid' =>$enrolment[ 'courseid' ], 'enrol' => 'manual' ), '*' , MUST_EXIST); #course and manual enroll $manualCourseID = $courseInfo->id; # number for course and manual enroll   $ue = $DB->get_record( 'user_enrolments' , array( 'enrolid' =>$manualCourseID, 'userid' =>$enrolment[ 'userid' ]));   $DB->delete_records( 'user_enrolments' , array( 'id' =>$ue->id));   }   $transaction->allow_commit(); }   /** * Returns description of method result value * @return external_description */ public static function manual_unenrol_users_returns() { return null ; }  
          Hide
          rgillet Richard Gillette added a comment -

          I tested the context check this morning and it works

          Show
          rgillet Richard Gillette added a comment - I tested the context check this morning and it works
          Hide
          francoisj Francois J added a comment -

          I am interested in this webservice function, what about its implementation?

          Show
          francoisj Francois J added a comment - I am interested in this webservice function, what about its implementation?
          Hide
          jerome Jérôme Mouneyrac added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.
          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          jerome Jérôme Mouneyrac added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle Testing instructions are missing. MOODLE_28_STABLE (6 errors / 48 warnings) [branch: MDL-28592-M28 | CI Job ] phplint (0/0) , php (5/47) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , master (6 errors / 48 warnings) [branch: MDL-28592-master | CI Job ] phplint (0/0) , php (5/47) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          johno John Okely added a comment - - edited

          Thanks James. It'll be really good to get this web service added.

          1. I noticed the 28 branch, do you think this needs to be backported?
          2. Some things CIBot has pointed out:
            1. Missing some testing instructions
              You can tell the tester to use admin/webservice/documentation.php to fill out parameters for running https://github.com/moodlehq/sample-ws-clients
              Would be good to test success, and each of the errors
            2. Comments should always start with a capital letter and end with punctuation
            3. Add a space before and after operators such as + and =>
            4. Add a quick description to @param $enrolments
              Should explain that it is an array of course user and role ids
            5. Add a component to the commit message. To make it shorter, "MDL-28592 webservices: Add manual unenrol users function to web services" could work.
          3. I think it would be good to throw an exception when the manual plugin is not found just in case. There may be enrolments in the database which would cause a PHP error when the unenrol_user funciton of an non-existent $enrol instance is used.

            if (empty($enrol)) {
                throw new moodle_exception('manualpluginnotinstalled', 'enrol_manual');
            }


            after enrol/manual/externallib.php:194

          4. "// never to be false" comment can be removed. In general comment should be used to explain overall larger concepts that cannot be understood easily by looking at the code. If a comment explains one line, usually it isn't needed. enrol/manual/externallib.php:207
          5. Personally, I'd have one unit test for single and multiple, that way less time will be spent setting up. Enrol 4 users, then unenrol one, check that the 3 remain, then unenrol two more users, and check that one remains.
          6. You can use $this->setExpectedException('required_capability_exception'); etc instead of try catches
          7. If the enrol_cohort (cohort sync) was used, there's no way to unenrol the user. I could see that being problematic, but it should be fine.

          Thanks. Let me know if I can be of assistance.

          Show
          johno John Okely added a comment - - edited Thanks James. It'll be really good to get this web service added. I noticed the 28 branch, do you think this needs to be backported? Some things CIBot has pointed out: Missing some testing instructions You can tell the tester to use admin/webservice/documentation.php to fill out parameters for running https://github.com/moodlehq/sample-ws-clients Would be good to test success, and each of the errors Comments should always start with a capital letter and end with punctuation Add a space before and after operators such as + and => Add a quick description to @param $enrolments Should explain that it is an array of course user and role ids Add a component to the commit message. To make it shorter, " MDL-28592 webservices: Add manual unenrol users function to web services" could work. I think it would be good to throw an exception when the manual plugin is not found just in case. There may be enrolments in the database which would cause a PHP error when the unenrol_user funciton of an non-existent $enrol instance is used. if (empty($enrol)) { throw new moodle_exception('manualpluginnotinstalled', 'enrol_manual'); } after enrol/manual/externallib.php:194 "// never to be false" comment can be removed. In general comment should be used to explain overall larger concepts that cannot be understood easily by looking at the code. If a comment explains one line, usually it isn't needed. enrol/manual/externallib.php:207 Personally, I'd have one unit test for single and multiple, that way less time will be spent setting up. Enrol 4 users, then unenrol one, check that the 3 remain, then unenrol two more users, and check that one remains. You can use $this->setExpectedException('required_capability_exception'); etc instead of try catches If the enrol_cohort (cohort sync) was used, there's no way to unenrol the user. I could see that being problematic, but it should be fine. Thanks. Let me know if I can be of assistance.
          Hide
          jballard James Ballard added a comment -

          Thanks John, we'll try and pick these up shortly and update the patch.

          Probably doesn't need to be back-ported but we are currently working with 2.8 for our client releases which is where we need it.

          Show
          jballard James Ballard added a comment - Thanks John, we'll try and pick these up shortly and update the patch. Probably doesn't need to be back-ported but we are currently working with 2.8 for our client releases which is where we need it.
          Hide
          johno John Okely added a comment -

          Hey [~James Ballard] has any progress been made on this? There's only about a week before code freeze, after which we can't accept new features or API changes for 2.9. So if you are still working on it would be good to get this ready for integration soon

          Show
          johno John Okely added a comment - Hey [~James Ballard] has any progress been made on this? There's only about a week before code freeze, after which we can't accept new features or API changes for 2.9. So if you are still working on it would be good to get this ready for integration soon
          Hide
          jballard James Ballard added a comment -

          Thanks John Okely Ah! Probably going to get right to the wire on this. We've scheduled our developers to fix it from Monday (30th Mar).

          Show
          jballard James Ballard added a comment - Thanks John Okely Ah! Probably going to get right to the wire on this. We've scheduled our developers to fix it from Monday (30th Mar).
          Hide
          john.phoon John Phoon added a comment - - edited

          Updated the code to properly reflect Moodle's coding standards.

          Show
          john.phoon John Phoon added a comment - - edited Updated the code to properly reflect Moodle's coding standards.
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle

          • Testing instructions are missing.
          • MOODLE_28_STABLE [branch: MDL-28592-M28 | CI Job]
            • Info: the branch is based off moodle.git
            • Info: base commit 0a4b99f35bac5e8b44c9f01fb3eedbe353e5296a being used as initial commit.
            • Error: The MDL-28592-M28 branch at https://github.com/ninelanterns/moodle is very old (>60 days ago). Please rebase against current MOODLE_28_STABLE.
          • master [branch: MDL-28592-master | CI Job]
            • Info: the branch is based off moodle.git
            • Info: base commit 20d38830aed94d822079092a45ddfdd9274ed317 being used as initial commit.
            • Error: The MDL-28592-master branch at https://github.com/ninelanterns/moodle is very old (>60 days ago). Please rebase against current master.

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle Testing instructions are missing. MOODLE_28_STABLE [branch: MDL-28592-M28 | CI Job ] Info: the branch is based off moodle.git Info: base commit 0a4b99f35bac5e8b44c9f01fb3eedbe353e5296a being used as initial commit. Error: The MDL-28592 -M28 branch at https://github.com/ninelanterns/moodle is very old (>60 days ago). Please rebase against current MOODLE_28_STABLE. master [branch: MDL-28592-master | CI Job ] Info: the branch is based off moodle.git Info: base commit 20d38830aed94d822079092a45ddfdd9274ed317 being used as initial commit. Error: The MDL-28592 -master branch at https://github.com/ninelanterns/moodle is very old (>60 days ago). Please rebase against current master. More information about this report
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle Testing instructions are missing. master (1 errors / 1 warnings) [branch: MDL-28592-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          john.phoon John Phoon added a comment -

          Formatting error is thrown but it is similar with the other parts of the php script in enrol/manual/db/services

          Show
          john.phoon John Phoon added a comment - Formatting error is thrown but it is similar with the other parts of the php script in enrol/manual/db/services
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle master (2 errors / 0 warnings) [branch: MDL-28592-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          jballard James Ballard added a comment -

          Hi John Okely - our developer John Phoon has updated the changes now. As mentioned the PHP error, while accurate, mirrors the structure of the pre-existing service calls so we have left that as it is, but happy to add the space if required.

          Hopefully that covers what is needed

          Show
          jballard James Ballard added a comment - Hi John Okely - our developer John Phoon has updated the changes now. As mentioned the PHP error, while accurate, mirrors the structure of the pre-existing service calls so we have left that as it is, but happy to add the space if required. Hopefully that covers what is needed
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle master (1 errors / 0 warnings) [branch: MDL-28592-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          johno John Okely added a comment -

          Thanks John

          If you don't address a peer review point it's good if you can add a quick sentence saying why, so the integrator knows. And if I got anything wrong in the peer review I can learn (a peer review-review as it were )

          I'm going to push this forward to integration, thanks all.

          Show
          johno John Okely added a comment - Thanks John If you don't address a peer review point it's good if you can add a quick sentence saying why, so the integrator knows. And if I got anything wrong in the peer review I can learn (a peer review-review as it were ) I'm going to push this forward to integration, thanks all.
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle

          • master [branch: MDL-28592-master | CI Job]
            • Info: the branch is based off moodle.git
            • Info: base commit 1d3fd63f97de06d447f76591055b716436718df5 being used as initial commit.

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle master [branch: MDL-28592-master | CI Job ] Info: the branch is based off moodle.git Info: base commit 1d3fd63f97de06d447f76591055b716436718df5 being used as initial commit. More information about this report
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Hi,

          because of some coincidences & missing controls, last pre-check runs by CiBoT (maybe) did end incorrectly above.

          FYI, the issue has been tracked @ MDLSITE-3940, where we are introducing some extra validations to avoid this to happen again.

          Once applied, CiBoT checks will be re-executed again to have the results updated here.

          Sorry for the noise and the troubles, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Hi, because of some coincidences & missing controls, last pre-check runs by CiBoT (maybe) did end incorrectly above. FYI, the issue has been tracked @ MDLSITE-3940 , where we are introducing some extra validations to avoid this to happen again. Once applied, CiBoT checks will be re-executed again to have the results updated here. Sorry for the noise and the troubles, ciao
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28592 using repository: https://github.com/ninelanterns/moodle master (1 errors / 0 warnings) [branch: MDL-28592-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated to master - thanks

          Show
          poltawski Dan Poltawski added a comment - Integrated to master - thanks
          Hide
          jethac Jetha Chan added a comment -

          Thanks for working on this! Before I pass this - the error cases in the testing instructions:

          1. Unenrol a user that is not enrolled. When attempting to unenrol a user that isn't enrolled in the given course, this doesn't error, it just fails silently.
            • An error is generated as expected when attempting to unenrol a user that doesn't exist.
          2. Unenrol a user from course that doesn't exist. This errors as expected.

          If the testing instructions could be amended, I don't see any reason why this couldn't pass.

          Show
          jethac Jetha Chan added a comment - Thanks for working on this! Before I pass this - the error cases in the testing instructions: Unenrol a user that is not enrolled. When attempting to unenrol a user that isn't enrolled in the given course, this doesn't error, it just fails silently. An error is generated as expected when attempting to unenrol a user that doesn't exist. Unenrol a user from course that doesn't exist. This errors as expected. If the testing instructions could be amended, I don't see any reason why this couldn't pass.
          Hide
          jballard James Ballard added a comment -

          Thanks Jetha Chan, I've updated the testing instructions.

          Show
          jballard James Ballard added a comment - Thanks Jetha Chan , I've updated the testing instructions.
          Hide
          jethac Jetha Chan added a comment -

          Cheers James - marking as passed on master.

          Show
          jethac Jetha Chan added a comment - Cheers James - marking as passed on master.
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

          The craft of programming begins with empathy, not formatting or languages or tools or algorithms or data structures.
          – Kent Beck

          Show
          poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. The craft of programming begins with empathy, not formatting or languages or tools or algorithms or data structures. – Kent Beck

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/May/15