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.

            People

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

              Dates

              • Created:
                Updated: