diff --git a/admin/tool/uploadcourse/classes/course.php b/admin/tool/uploadcourse/classes/course.php index f082eabd40634..ff2283304d059 100644 --- a/admin/tool/uploadcourse/classes/course.php +++ b/admin/tool/uploadcourse/classes/course.php @@ -108,9 +108,6 @@ class tool_uploadcourse_course { static protected $importoptionsdefaults = array('canrename' => false, 'candelete' => false, 'canreset' => false, 'reset' => false, 'restoredir' => null, 'shortnametemplate' => null); - /** @var array special fields used when processing the enrolment methods. */ - static protected $enrolmentspecialfields = array('delete', 'disable', 'startdate', 'enddate', 'enrolperiod', 'role'); - /** * Constructor * @@ -658,18 +655,9 @@ public function prepare() { return false; } - // Getting the enrolment data. - $errors = array(); - $this->enrolmentdata = tool_uploadcourse_helper::get_enrolment_data($this->rawdata, $errors); - if (!empty($errors)) { - foreach ($errors as $key => $message) { - $this->error($key, $message); - } - return false; - } - // Saving data. $this->data = $coursedata; + $this->enrolmentdata = tool_uploadcourse_helper::get_enrolment_data($this->rawdata); // Restore data. // TODO Speed up things by not really extracting the backup just yet, but checking that @@ -781,20 +769,10 @@ protected function process_enrolment_data($course) { return; } - $cannotaddmethods = array(); $enrolmentplugins = tool_uploadcourse_helper::get_enrolment_plugins(); $instances = enrol_get_instances($course->id, false); foreach ($enrolmentdata as $enrolmethod => $method) { - $plugin = $enrolmentplugins[$enrolmethod]; - - // TODO MDL-48362 Abstract the logic to prevent it to be tied to the - // user interface. Ideally a plugin should have a method that returns - // whether or not a new instance can be added to the course rather than - // using enrol_plugin::get_newinstance_link() to figure that out. - $canadd = $plugin->get_newinstance_link($course->id); - - // TODO MDL-43820 Handle multiple instances of the same type. $instance = null; foreach ($instances as $i) { if ($i->enrol == $enrolmethod) { @@ -828,57 +806,30 @@ protected function process_enrolment_data($course) { } } } else { + $plugin = null; if (empty($instance)) { - - // Check if we can create a new instance of this enrolment method. - if (!$canadd) { - $cannotaddmethods[] = $enrolmethod; - continue; - } - - // Some plugins do not implement enrol_plugin::add_default_instance(), - // but we will try anyway and call enrol_plugin::add_instance() if needed. - $id = $plugin->add_default_instance($course); - if (empty($id)) { - $id = $plugin->add_instance($course); - } - + $plugin = $enrolmentplugins[$enrolmethod]; $instance = new stdClass(); - $instance->id = $id; + $instance->id = $plugin->add_default_instance($course); $instance->roleid = $plugin->get_config('roleid'); $instance->status = ENROL_INSTANCE_ENABLED; } else { + $plugin = $enrolmentplugins[$instance->enrol]; $plugin->update_status($instance, ENROL_INSTANCE_ENABLED); } // Now update values. foreach ($method as $k => $v) { - if (in_array($k, self::$enrolmentspecialfields)) { - // Skip the special import keys. - continue; - } $instance->{$k} = $v; } // Sort out the start, end and date. - if (isset($method['startdate'])) { - $instance->enrolstartdate = strtotime($method['startdate']); - } else if (!isset($instance->enrolstartdate)) { - $instance->enrolstartdate = 0; - } - if (isset($method['enddate'])) { - $instance->enrolenddate = strtotime($method['enddate']); - } else if (!isset($instance->enrolenddate)) { - $instance->enrolenddate = 0; - } + $instance->enrolstartdate = (isset($method['startdate']) ? strtotime($method['startdate']) : 0); + $instance->enrolenddate = (isset($method['enddate']) ? strtotime($method['enddate']) : 0); // Is the enrolment period set? - if (isset($method['enrolperiod'])) { - if (empty($method['enrolperiod'])) { - // Let's just make sure that it's 0. - $method['enrolperiod'] = 0; - } else if (preg_match('/^\d+$/', $method['enrolperiod'])) { - // Cast integers to integers. + if (isset($method['enrolperiod']) && ! empty($method['enrolperiod'])) { + if (preg_match('/^\d+$/', $method['enrolperiod'])) { $method['enrolperiod'] = (int) $method['enrolperiod']; } else { // Try and convert period to seconds. @@ -908,11 +859,6 @@ protected function process_enrolment_data($course) { $DB->update_record('enrol', $instance); } } - - if (!empty($cannotaddmethods)) { - $this->error('cannotaddenrolmentmethods', new lang_string('cannotaddenrolmentmethods', 'tool_uploadcourse', - implode(', ', $cannotaddmethods))); - } } /** diff --git a/admin/tool/uploadcourse/classes/helper.php b/admin/tool/uploadcourse/classes/helper.php index 0a3e12e96982e..2873f3b783f4e 100644 --- a/admin/tool/uploadcourse/classes/helper.php +++ b/admin/tool/uploadcourse/classes/helper.php @@ -134,10 +134,9 @@ public static function get_course_formats() { * ) * * @param array $data data to extract the enrolment data from. - * @param array $errors will be populated with errors found. * @return array */ - public static function get_enrolment_data($data, &$errors = array()) { + public static function get_enrolment_data($data) { $enrolmethods = array(); $enroloptions = array(); foreach ($data as $field => $value) { @@ -159,26 +158,16 @@ public static function get_enrolment_data($data, &$errors = array()) { // Combining enrolment methods and their options in a single array. $enrolmentdata = array(); - $unknownmethods = array(); - $methodsnotsupported = array(); if (!empty($enrolmethods)) { $enrolmentplugins = self::get_enrolment_plugins(); foreach ($enrolmethods as $key => $method) { if (!array_key_exists($method, $enrolmentplugins)) { - // Unknown enrolment method. - $unknownmethods[] = $method; + // Error! continue; } $enrolmentdata[$enrolmethods[$key]] = $enroloptions[$key]; } } - - // Logging errors related to enrolment methods. - if (!empty($unknownmethods)) { - $errors['unknownenrolmentmethods'] = new lang_string('unknownenrolmentmethods', 'tool_uploadcourse', - implode(', ', $unknownmethods)); - } - return $enrolmentdata; } diff --git a/admin/tool/uploadcourse/classes/processor.php b/admin/tool/uploadcourse/classes/processor.php index 27438467f301f..8963f7eee5edf 100644 --- a/admin/tool/uploadcourse/classes/processor.php +++ b/admin/tool/uploadcourse/classes/processor.php @@ -207,7 +207,6 @@ public function execute($tracker = null) { $course = $this->get_course($data); if ($course->prepare()) { $course->proceed(); - $outcome = 1; $status = $course->get_statuses(); if (array_key_exists('coursecreated', $status)) { @@ -218,19 +217,11 @@ public function execute($tracker = null) { $deleted++; } - // Errors can occur even though the course preparation returned true, often because - // some checks could not be done in course::prepare() because it requires the course to exist. - if ($course->has_errors()) { - $status += $course->get_errors(); - $errors++; - $outcome = 2; - } - $data = array_merge($data, $course->get_data(), array('id' => $course->get_id())); - $tracker->output($this->linenb, $outcome, $status, $data); + $tracker->output($this->linenb, true, $status, $data); } else { $errors++; - $tracker->output($this->linenb, 0, $course->get_errors(), $data); + $tracker->output($this->linenb, false, $course->get_errors(), $data); } } diff --git a/admin/tool/uploadcourse/classes/tracker.php b/admin/tool/uploadcourse/classes/tracker.php index 3ac78b4437fbc..0daa84527c5cc 100644 --- a/admin/tool/uploadcourse/classes/tracker.php +++ b/admin/tool/uploadcourse/classes/tracker.php @@ -136,8 +136,7 @@ public function results($total, $created, $updated, $deleted, $errors) { * Output one more line. * * @param int $line line number. - * @param int $outcome 0 for failure, 1 for success, 2 for success with errors. Use 2 when - * most of the process succeeded but there might have been outstanding errors. + * @param bool $outcome success or not? * @param array $status array of statuses. * @param array $data extra data to display. * @return void @@ -149,16 +148,9 @@ public function output($line, $outcome, $status, $data) { } if ($this->outputmode == self::OUTPUT_PLAIN) { - if ($outcome == 1) { - $ok = 'OK'; - } else if (!$outcome) { - $ok = 'NOK'; // Not OK. - } else { - $ok = 'EOK'; // Errors, but OK. - } $message = array( $line, - $ok, + $outcome ? 'OK' : 'NOK', isset($data['id']) ? $data['id'] : '', isset($data['shortname']) ? $data['shortname'] : '', isset($data['fullname']) ? $data['fullname'] : '', @@ -176,12 +168,10 @@ public function output($line, $outcome, $status, $data) { if (is_array($status)) { $status = implode(html_writer::empty_tag('br'), $status); } - if ($outcome == 1) { + if ($outcome) { $outcome = $OUTPUT->pix_icon('i/valid', ''); - } else if (!$outcome) { - $outcome = $OUTPUT->pix_icon('i/invalid', ''); } else { - $outcome = $OUTPUT->pix_icon('i/caution', ''); + $outcome = $OUTPUT->pix_icon('i/invalid', ''); } echo html_writer::start_tag('tr', array('class' => 'r' . $this->rownb % 2)); echo html_writer::tag('td', $line, array('class' => 'c' . $ci++)); diff --git a/admin/tool/uploadcourse/lang/en/tool_uploadcourse.php b/admin/tool/uploadcourse/lang/en/tool_uploadcourse.php index 64e96c500934b..96b847c5320e1 100644 --- a/admin/tool/uploadcourse/lang/en/tool_uploadcourse.php +++ b/admin/tool/uploadcourse/lang/en/tool_uploadcourse.php @@ -29,7 +29,6 @@ $string['allowresets'] = 'Allow resets'; $string['allowresets_help'] = 'Whether the reset field is accepted or not.'; $string['cachedef_helper'] = 'Helper caching'; -$string['cannotaddenrolmentmethods'] = 'Cannot add the enrolment methods: {$a}'; $string['cannotdeletecoursenotexist'] = 'Cannot delete a course that does not exist'; $string['cannotgenerateshortnameupdatemode'] = 'Cannot generate a shortname when updates are allowed'; $string['cannotreadbackupfile'] = 'Cannot read the backup file'; @@ -110,7 +109,6 @@ $string['shortnametemplate_help'] = 'The short name of the course is displayed in the navigation. You may use template syntax here (%f = fullname, %i = idnumber), or enter an initial value that is incremented.'; $string['templatefile'] = 'Restore from this file after upload'; $string['templatefile_help'] = 'Select a file to use as a template for the creation of all courses.'; -$string['unknownenrolmentmethods'] = 'Unknown enrolment methods: {$a}'; $string['unknownimportmode'] = 'Unknown import mode'; $string['updatemissing'] = 'Fill in missing items from CSV data and defaults'; $string['updatemode'] = 'Update mode'; diff --git a/admin/tool/uploadcourse/tests/course_test.php b/admin/tool/uploadcourse/tests/course_test.php index 9d12b80e19cb3..a173e721811d2 100644 --- a/admin/tool/uploadcourse/tests/course_test.php +++ b/admin/tool/uploadcourse/tests/course_test.php @@ -900,11 +900,8 @@ public function test_create_bad_category() { } public function test_enrolment_data() { - global $DB; $this->resetAfterTest(true); - $this->setAdminUser(); - // Adding an enrolment method to a new course. $mode = tool_uploadcourse_processor::MODE_CREATE_NEW; $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; $data = array('shortname' => 'c1', 'summary' => 'S', 'fullname' => 'FN', 'category' => '1'); @@ -924,135 +921,11 @@ public function test_enrolment_data() { $enroldata[$instance->enrol] = $instance; } - $this->assertFalse($co->has_errors()); $this->assertNotEmpty($enroldata['manual']); $this->assertEquals(ENROL_INSTANCE_ENABLED, $enroldata['manual']->status); $this->assertEquals(strtotime($data['enrolment_1_startdate']), $enroldata['manual']->enrolstartdate); $this->assertEquals(strtotime('1970-01-01 GMT + ' . $data['enrolment_1_enrolperiod']), $enroldata['manual']->enrolperiod); $this->assertEquals(strtotime('12th July 2013'), $enroldata['manual']->enrolenddate); - - // Updating a course's enrolment method. - $c2 = $this->getDataGenerator()->create_course(array('shortname' => 'c2')); - $enroldata = array(); - $instances = enrol_get_instances($c2->id, false); - foreach ($instances as $instance) { - $enroldata[$instance->enrol] = $instance; - } - $manual = $enroldata['manual']; - $manual->enrolstartdate = strtotime('1st January 2000'); - $manual->enrolenddate = strtotime('2nd January 2001'); - $manual->status = ENROL_INSTANCE_DISABLED; - $DB->update_record('enrol', $manual); - - $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; - $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; - $data = array('shortname' => 'c2'); - $data['enrolment_1'] = 'manual'; - $data['enrolment_1_role'] = 'teacher'; - $data['enrolment_1_enddate'] = '2nd August 2013'; - $data['enrolment_1_enrolperiod'] = '10 days'; - $co = new tool_uploadcourse_course($mode, $updatemode, $data); - $this->assertTrue($co->prepare()); - $co->proceed(); - - // Get the enrolment methods data. - $enroldata = array(); - $instances = enrol_get_instances($co->get_id(), false); - foreach ($instances as $instance) { - $enroldata[$instance->enrol] = $instance; - } - - $this->assertFalse($co->has_errors()); - $this->assertNotEmpty($enroldata['manual']); - $this->assertEquals(ENROL_INSTANCE_ENABLED, $enroldata['manual']->status); - $this->assertEquals($manual->enrolstartdate, $enroldata['manual']->enrolstartdate); - $this->assertEquals(strtotime('1970-01-01 GMT + ' . $data['enrolment_1_enrolperiod']), - $enroldata['manual']->enrolperiod); - $this->assertEquals(strtotime('11th January 2000'), $enroldata['manual']->enrolenddate); - - // Disabling an enrolment method. - $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; - $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; - $data = array('shortname' => 'c2'); - $data['enrolment_1'] = 'manual'; - $data['enrolment_1_disable'] = '1'; - $co = new tool_uploadcourse_course($mode, $updatemode, $data); - $this->assertTrue($co->prepare()); - $co->proceed(); - - $enroldata = array(); - $instances = enrol_get_instances($co->get_id(), false); - foreach ($instances as $instance) { - $enroldata[$instance->enrol] = $instance; - } - $this->assertFalse($co->has_errors()); - $this->assertNotEmpty($enroldata['manual']); - $this->assertEquals(ENROL_INSTANCE_DISABLED, $enroldata['manual']->status); - - // Deleting an enrolment method. - $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; - $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; - $data = array('shortname' => 'c2'); - $data['enrolment_1'] = 'manual'; - $data['enrolment_1_delete'] = '1'; - $co = new tool_uploadcourse_course($mode, $updatemode, $data); - $this->assertTrue($co->prepare()); - $co->proceed(); - - $enroldata = array(); - $instances = enrol_get_instances($co->get_id(), false); - foreach ($instances as $instance) { - $enroldata[$instance->enrol] = $instance; - } - $this->assertFalse($co->has_errors()); - $this->assertArrayNotHasKey('manual', $enroldata); - - // Trying to add a non-existing enrolment method. - $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; - $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; - $data = array('shortname' => 'c2'); - $data['enrolment_1'] = 'notexisting'; - $co = new tool_uploadcourse_course($mode, $updatemode, $data); - $this->assertFalse($co->prepare()); - $this->assertArrayHasKey('unknownenrolmentmethods', $co->get_errors()); - - // Trying to add a non-compatible enrolment method. - $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; - $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; - $data = array('shortname' => 'c2'); - $data['enrolment_1'] = 'database'; - $co = new tool_uploadcourse_course($mode, $updatemode, $data); - $this->assertTrue($co->prepare()); - $co->proceed(); - - $enroldata = array(); - $instances = enrol_get_instances($co->get_id(), false); - foreach ($instances as $instance) { - $enroldata[$instance->enrol] = $instance; - } - $this->assertTrue($co->has_errors()); - $this->assertArrayNotHasKey('database', $enroldata); - $this->assertArrayHasKey('cannotaddenrolmentmethods', $co->get_errors()); - - // Testing cohort enrolment method. - $cohort = $this->getDataGenerator()->create_cohort(); - $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; - $updatemode = tool_uploadcourse_processor::UPDATE_ALL_WITH_DATA_ONLY; - $data = array('shortname' => 'c2'); - $data['enrolment_1'] = 'cohort'; - $data['enrolment_1_customint1'] = $cohort->id; - $co = new tool_uploadcourse_course($mode, $updatemode, $data); - $this->assertTrue($co->prepare()); - $co->proceed(); - - $enroldata = array(); - $instances = enrol_get_instances($co->get_id(), false); - foreach ($instances as $instance) { - $enroldata[$instance->enrol] = $instance; - } - $this->assertFalse($co->has_errors()); - $this->assertArrayHasKey('cohort', $enroldata); - $this->assertEquals($cohort->id, $enroldata['cohort']->customint1); } public function test_idnumber_problems() { diff --git a/admin/tool/uploadcourse/tests/helper_test.php b/admin/tool/uploadcourse/tests/helper_test.php index 22569e3a00045..41a838c17535c 100644 --- a/admin/tool/uploadcourse/tests/helper_test.php +++ b/admin/tool/uploadcourse/tests/helper_test.php @@ -101,9 +101,7 @@ public function test_get_enrolment_data() { 'test1' => 'test1', ) ); - $errors = array(); - $this->assertSame(tool_uploadcourse_helper::get_enrolment_data($data, $errors), $expected); - $this->assertArrayHasKey('unknownenrolmentmethods', $errors); + $this->assertSame(tool_uploadcourse_helper::get_enrolment_data($data), $expected); } public function test_get_enrolment_plugins() {