Moodle
  1. Moodle
  2. MDL-28592

Create a manual_unenrol_users external function

    Details

    • Type: New Feature New Feature
    • Status: Peer review in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.0.2, 2.8.2, 2.9
    • Fix Version/s: DEV backlog
    • Component/s: Enrolments, Web Services
    • Labels:

      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
          Michael de Raadt added a comment -

          Hi, Craig.

          Yes, that sounds quite logical.

          Please share what you create.

          Show
          Michael de Raadt added a comment - Hi, Craig. Yes, that sounds quite logical. Please share what you create.
          Hide
          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
          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
          Richard Gillette added a comment -

          I tested the context check this morning and it works

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

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

          Show
          Francois J added a comment - I am interested in this webservice function, what about its implementation?
          Hide
          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
          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 added a comment -

          Fails against automated checks.

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

          More information about this report

          Show
          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
          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
          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
          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
          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
          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
          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
          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
          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).

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated: