From 71ffad7ea617f15fddb95cb813048d9d61864309 Mon Sep 17 00:00:00 2001 From: Peter Burnett Date: Fri, 5 Mar 2021 14:54:31 +1000 Subject: [PATCH 1/2] MDL-71031 webservices: Extended gradecategory creation API This adds a new webservice that creates gradecategories as a batch, and deprecates the old single creation call, which is superseded. It also patches a few small issues in the single WS, for any integrations currently relying on that webservice. --- .../external/create_gradecategories.php | 241 ++++++++++++++++++ ...s_external_create_gradecategories_test.php | 148 +++++++++++ lib/classes/grades_external.php | 113 ++------ lib/db/services.php | 10 +- lib/upgrade.txt | 2 + version.php | 2 +- 6 files changed, 428 insertions(+), 88 deletions(-) create mode 100644 grade/classes/external/create_gradecategories.php create mode 100644 grade/tests/grades_external_create_gradecategories_test.php diff --git a/grade/classes/external/create_gradecategories.php b/grade/classes/external/create_gradecategories.php new file mode 100644 index 00000000000..3252151d971 --- /dev/null +++ b/grade/classes/external/create_gradecategories.php @@ -0,0 +1,241 @@ +. + +/** + * Create gradecategories webservice. + * + * @package core_grades + * @copyright 2021 Peter Burnett + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @since Moodle 3.11 + */ + +namespace core_grades\external; +defined('MOODLE_INTERNAL') || die; +use \external_function_parameters, + \external_value, + \external_single_structure, + \external_multiple_structure, + \external_warnings; + +require_once("$CFG->libdir/externallib.php"); +require_once("$CFG->libdir/gradelib.php"); +require_once("$CFG->dirroot/grade/edit/tree/lib.php"); + +/** + * Parameter, returns and webservice definitions for create_gradecategories. + */ +class create_gradecategories extends \external_api { + /** + * Returns description of method parameters + * + * @return external_function_parameters + * @since Moodle 3.11 + */ + public static function create_gradecategories_parameters() { + return new external_function_parameters( + [ + 'courseid' => new external_value(PARAM_INT, 'id of course', VALUE_REQUIRED), + 'categories' => new external_multiple_structure( + new external_single_structure([ + 'fullname' => new external_value(PARAM_TEXT, 'fullname of category', VALUE_REQUIRED), + 'options' => new external_single_structure([ + 'aggregation' => new external_value(PARAM_INT, 'aggregation method', VALUE_OPTIONAL), + 'aggregateonlygraded' => new external_value(PARAM_BOOL, 'exclude empty grades', VALUE_OPTIONAL), + 'aggregateoutcomes' => new external_value(PARAM_BOOL, 'aggregate outcomes', VALUE_OPTIONAL), + 'droplow' => new external_value(PARAM_INT, 'drop low grades', VALUE_OPTIONAL), + 'itemname' => new external_value(PARAM_TEXT, 'the category total name', VALUE_OPTIONAL), + 'iteminfo' => new external_value(PARAM_TEXT, 'the category iteminfo', VALUE_OPTIONAL), + 'idnumber' => new external_value(PARAM_TEXT, 'the category idnumber', VALUE_OPTIONAL), + 'gradetype' => new external_value(PARAM_INT, 'the grade type', VALUE_OPTIONAL), + 'grademax' => new external_value(PARAM_INT, 'the grade max', VALUE_OPTIONAL), + 'grademin' => new external_value(PARAM_INT, 'the grade min', VALUE_OPTIONAL), + 'gradepass' => new external_value(PARAM_INT, 'the grade to pass', VALUE_OPTIONAL), + 'display' => new external_value(PARAM_INT, 'the display type', VALUE_OPTIONAL), + 'decimals' => new external_value(PARAM_INT, 'the decimal count', VALUE_OPTIONAL), + 'hiddenuntil' => new external_value(PARAM_INT, 'grades hidden until', VALUE_OPTIONAL), + 'locktime' => new external_value(PARAM_INT, 'lock grades after', VALUE_OPTIONAL), + 'weightoverride' => new external_value(PARAM_BOOL, 'weight adjusted', VALUE_OPTIONAL), + 'aggregationcoef2' => new external_value(PARAM_RAW, 'weight coefficient', VALUE_OPTIONAL), + 'parentcategoryid' => new external_value(PARAM_INT, 'The parent category id', VALUE_OPTIONAL), + 'parentcategoryidnumber' => new external_value(PARAM_TEXT, + 'the parent category idnumber', VALUE_OPTIONAL), + ], 'optional category data', VALUE_DEFAULT, []), + ], 'Category to create', VALUE_REQUIRED) + , 'Categories to create', VALUE_REQUIRED) + ] + ); + } + + /** + * Creates gradecategories inside of the specified course. + * + * @param int $courseid the courseid to create the gradecategory in. + * @param array $categories the categories to create. + * @return array array of created categoryids and warnings. + */ + public static function create_gradecategories(int $courseid, array $categories) { + $params = self::validate_parameters(self::create_gradecategories_parameters(), + ['courseid' => $courseid, 'categories' => $categories]); + + // Now params are validated, update the references. + $courseid = $params['courseid']; + $categories = $params['categories']; + + // Check that the context and permissions are OK. + $context = \context_course::instance($courseid); + self::validate_context($context); + require_capability('moodle/grade:manage', $context); + + return self::create_gradecategories_from_data($courseid, $categories); + } + + /** + * Returns description of method result value + * + * @return external_description + * @since Moodle 3.11 + */ + public static function create_gradecategories_returns() { + return new external_single_structure([ + 'categoryids' => new external_multiple_structure( + new external_value(PARAM_INT, 'created cateogry ID') + ), + 'warnings' => new external_warnings(), + ]); + } + + /** + * Takes an array of categories and creates the inside the category tree for the supplied courseid. + * + * @param int $courseid the courseid to create the categories inside of. + * @param array $categories the categories to create. + * @return array array of results and warnings. + */ + public static function create_gradecategories_from_data(int $courseid, array $categories): array { + global $CFG, $DB; + + $defaultparentcat = \grade_category::fetch_course_category($courseid); + // Setup default data so WS call needs to contain only data to set. + // This is not done in the Parameters, so that the array of options can be optional. + $defaultdata = [ + 'aggregation' => grade_get_setting($courseid, 'aggregation', $CFG->grade_aggregation, true), + 'aggregateonlygraded' => 1, + 'aggregateoutcomes' => 0, + 'droplow' => 0, + 'grade_item_itemname' => '', + 'grade_item_iteminfo' => '', + 'grade_item_idnumber' => '', + 'grade_item_gradetype' => GRADE_TYPE_VALUE, + 'grade_item_grademax' => 100, + 'grade_item_grademin' => 1, + 'grade_item_gradepass' => 1, + 'grade_item_display' => GRADE_DISPLAY_TYPE_DEFAULT, + // Hack. This must be -2 to use the default setting. + 'grade_item_decimals' => -2, + 'grade_item_hiddenuntil' => 0, + 'grade_item_locktime' => 0, + 'grade_item_weightoverride' => 0, + 'grade_item_aggregationcoef2' => 0, + 'parentcategory' => $defaultparentcat->id + ]; + + // Most of the data items need boilerplate prepended. These are the exceptions. + $ignorekeys = [ + 'aggregation', + 'aggregateonlygraded', + 'aggregateoutcomes', + 'droplow', + 'parentcategoryid', + 'parentcategoryidnumber' + ]; + + $createdcats = []; + foreach ($categories as $category) { + // Setup default data so WS call needs to contain only data to set. + // This is not done in the Parameters, so that the array of options can be optional. + $data = $defaultdata; + $data['fullname'] = $category['fullname']; + + foreach ($category['options'] as $key => $value) { + if (!in_array($key, $ignorekeys)) { + $fullkey = 'grade_item_' . $key; + $data[$fullkey] = $value; + } else { + $data[$key] = $value; + } + } + + // Handle parent category special case. + // This figures the parent category id from the provided id OR idnumber. + if (array_key_exists('parentcategoryid', $category['options']) && $parentcat = $DB->get_record('grade_categories', + ['id' => $category['options']['parentcategoryid'], 'courseid' => $courseid])) { + $data['parentcategory'] = $parentcat->id; + } else if (array_key_exists('parentcategoryidnumber', $category['options']) && + $parentcatgradeitem = $DB->get_record('grade_items', [ + 'itemtype' => 'category', + 'courseid' => $courseid, + 'idnumber' => $category['options']['parentcategoryidnumber'] + ], '*', IGNORE_MULTIPLE)) { + if ($parentcat = $DB->get_record('grade_categories', + ['courseid' => $courseid, 'id' => $parentcatgradeitem->iteminstance])) { + $data['parentcategory'] = $parentcat->id; + } + } + + // Create new gradecategory item. + $gradecategory = new \grade_category(['courseid' => $courseid], false); + $gradecategory->apply_default_settings(); + $gradecategory->apply_forced_settings(); + + // Data Validation. + if (array_key_exists('grade_item_gradetype', $data) and $data['grade_item_gradetype'] == GRADE_TYPE_SCALE) { + if (empty($data['grade_item_scaleid'])) { + $warnings[] = ['item' => 'scaleid', 'warningcode' => 'invalidscale', + 'message' => get_string('missingscale', 'grades')]; + } + } + if (array_key_exists('grade_item_grademin', $data) and array_key_exists('grade_item_grademax', $data)) { + if (($data['grade_item_grademax'] != 0 OR $data['grade_item_grademin'] != 0) AND + ($data['grade_item_grademax'] == $data['grade_item_grademin'] OR + $data['grade_item_grademax'] < $data['grade_item_grademin'])) { + $warnings[] = ['item' => 'grademax', 'warningcode' => 'invalidgrade', + 'message' => get_string('incorrectminmax', 'grades')]; + } + } + + if (!empty($warnings)) { + return ['categoryids' => [], 'warnings' => $warnings]; + } + + // Now call the update function with data. Transactioned so the gradebook isn't broken on bad data. + // This is done per-category so that children can correctly discover the parent categories. + try { + $transaction = $DB->start_delegated_transaction(); + \grade_edit_tree::update_gradecategory($gradecategory, (object) $data); + $transaction->allow_commit(); + $createdcats[] = $gradecategory->id; + } catch (\Exception $e) { + // If the submitted data was broken for any reason. + $warnings['database'] = $e->getMessage(); + $transaction->rollback(); + return ['warnings' => $warnings]; + } + } + + return['categoryids' => $createdcats, 'warnings' => []]; + } +} diff --git a/grade/tests/grades_external_create_gradecategories_test.php b/grade/tests/grades_external_create_gradecategories_test.php new file mode 100644 index 00000000000..fce8fa217fb --- /dev/null +++ b/grade/tests/grades_external_create_gradecategories_test.php @@ -0,0 +1,148 @@ +. + +/** + * Unit tests for the core_grades\external\create_gradecategories webservice. + * + * @package core_grades + * @category external + * @copyright 2021 Peter Burnett + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @since Moodle 3.11 + */ + +defined('MOODLE_INTERNAL') || die(); +global $CFG; +require_once($CFG->dirroot . '/webservice/tests/helpers.php'); + +use \core_grades\external\create_gradecategories; + +/** + * create_gradecategories unit tests. + */ +class create_gradecategories_testcase extends externallib_advanced_testcase { + + /** + * Test create_gradecategories. + * + * @return void + */ + public function test_create_gradecategories() { + global $DB; + $this->resetAfterTest(true); + $course = $this->getDataGenerator()->create_course(); + $this->setAdminUser(); + + // Test the most basic gradecategory creation. + $status1 = create_gradecategories::create_gradecategories($course->id, + [['fullname' => 'Test Category 1', 'options' => []]]); + + $courseparentcat = grade_category::fetch_course_category($course->id); + $record1 = $DB->get_record('grade_categories', ['id' => $status1['categoryids'][0]]); + $this->assertEquals('Test Category 1', $record1->fullname); + // Confirm that the parent category for this category is the top level category for the course. + $this->assertEquals($courseparentcat->id, $record1->parent); + $this->assertEquals(2, $record1->depth); + + // Now create a category as a child of the newly created category. + $status2 = create_gradecategories::create_gradecategories($course->id, + [['fullname' => 'Test Category 2', 'options' => ['parentcategoryid' => $record1->id]]]); + $record2 = $DB->get_record('grade_categories', ['id' => $status2['categoryids'][0]]); + $this->assertEquals($record1->id, $record2->parent); + $this->assertEquals(3, $record2->depth); + // Check the path is correct. + $this->assertEquals('/' . implode('/', [$courseparentcat->id, $record1->id, $record2->id]) . '/', $record2->path); + + // Now create a category with some customised data and check the returns. This customises every value. + $customopts = [ + 'aggregation' => GRADE_AGGREGATE_MEAN, + 'aggregateonlygraded' => 0, + 'aggregateoutcomes' => 1, + 'droplow' => 1, + 'itemname' => 'item', + 'iteminfo' => 'info', + 'idnumber' => 'idnumber', + 'gradetype' => GRADE_TYPE_TEXT, + 'grademax' => 5, + 'grademin' => 2, + 'gradepass' => 3, + 'display' => GRADE_DISPLAY_TYPE_LETTER, + // Hack. This must be -2 to use the default setting. + 'decimals' => 3, + 'hiddenuntil' => time(), + 'locktime' => time(), + 'weightoverride' => 1, + 'aggregationcoef2' => 20, + 'parentcategoryid' => $record2->id + ]; + + $status3 = create_gradecategories::create_gradecategories($course->id, + [['fullname' => 'Test Category 3', 'options' => $customopts]]); + $cat3 = new grade_category(['courseid' => $course->id, 'id' => $status3['categoryids'][0]], true); + $cat3->load_grade_item(); + + // Lets check all of the data is in the right shape. + $this->assertEquals(GRADE_AGGREGATE_MEAN, $cat3->aggregation); + $this->assertEquals(0, $cat3->aggregateonlygraded); + $this->assertEquals(1, $cat3->aggregateoutcomes); + $this->assertEquals(1, $cat3->droplow); + $this->assertEquals('item', $cat3->grade_item->itemname); + $this->assertEquals('info', $cat3->grade_item->iteminfo); + $this->assertEquals('idnumber', $cat3->grade_item->idnumber); + $this->assertEquals(GRADE_TYPE_TEXT, $cat3->grade_item->gradetype); + $this->assertEquals(5, $cat3->grade_item->grademax); + $this->assertEquals(2, $cat3->grade_item->grademin); + $this->assertEquals(3, $cat3->grade_item->gradepass); + $this->assertEquals(GRADE_DISPLAY_TYPE_LETTER, $cat3->grade_item->display); + $this->assertEquals(3, $cat3->grade_item->decimals); + $this->assertGreaterThanOrEqual($cat3->grade_item->hidden, time()); + $this->assertGreaterThanOrEqual($cat3->grade_item->locktime, time()); + $this->assertEquals(1, $cat3->grade_item->weightoverride); + // Coefficient is converted to percentage. + $this->assertEquals(0.2, $cat3->grade_item->aggregationcoef2); + $this->assertEquals($record2->id, $cat3->parent); + + // Now test creating 2 in parallel, and nesting them. + $status4 = create_gradecategories::create_gradecategories($course->id, [ + [ + 'fullname' => 'Test Category 4', + 'options' => [ + 'idnumber' => 'secondlevel' + ], + ], + [ + 'fullname' => 'Test Category 5', + 'options' => [ + 'idnumber' => 'thirdlevel', + 'parentcategoryidnumber' => 'secondlevel' + ], + ], + ]); + + $secondlevel = $DB->get_record('grade_categories', ['id' => $status4['categoryids'][0]]); + $thirdlevel = $DB->get_record('grade_categories', ['id' => $status4['categoryids'][1]]); + + // Confirm that the parent category for secondlevel is the top level category for the course. + $this->assertEquals($courseparentcat->id, $secondlevel->parent); + $this->assertEquals(2, $record1->depth); + + // Confirm that the parent category for thirdlevel is the secondlevel category. + $this->assertEquals($secondlevel->id, $thirdlevel->parent); + $this->assertEquals(3, $thirdlevel->depth); + // Check the path is correct. + $this->assertEquals('/' . implode('/', [$courseparentcat->id, $secondlevel->id, $thirdlevel->id]) . '/', $thirdlevel->path); + } +} diff --git a/lib/classes/grades_external.php b/lib/classes/grades_external.php index 559c657d65a..31e3cead682 100644 --- a/lib/classes/grades_external.php +++ b/lib/classes/grades_external.php @@ -575,6 +575,10 @@ class core_grades_external extends external_api { /** * Returns description of method parameters * + * @deprecated since Moodle 3.11 MDL-71031 - please do not use this function any more. + * @todo MDL-71325 This will be deleted in Moodle 4.3. + * @see core_grades\external\create_gradecategories::create_gradecategories() + * * @return external_function_parameters * @since Moodle 3.10 */ @@ -611,6 +615,10 @@ class core_grades_external extends external_api { /** * Creates a gradecategory inside of the specified course. * + * @deprecated since Moodle 3.11 MDL-71031 - please do not use this function any more. + * @todo MDL-71325 This will be deleted in Moodle 4.3. + * @see core_grades\external\create_gradecategories::create_gradecategories() + * * @param int $courseid the courseid to create the gradecategory in. * @param string $fullname the fullname of the grade category to create. * @param array $options array of options to set. @@ -633,98 +641,22 @@ class core_grades_external extends external_api { self::validate_context($context); require_capability('moodle/grade:manage', $context); - $defaultparentcat = new grade_category(['courseid' => $courseid, 'depth' => 1], true); + $categories = []; + $categories[] = ['fullname' => $fullname, 'options' => $options]; + // Call through to webservice class for multiple creations, + // Where the majority of the this functionality moved with the deprecation of this function. + $result = \core_grades\external\create_gradecategories::create_gradecategories_from_data($courseid, $categories); - // Setup default data so WS call needs to contain only data to set. - // This is not done in the Parameters, so that the array of options can be optional. - $data = [ - 'fullname' => $fullname, - 'aggregation' => grade_get_setting($courseid, 'displaytype', $CFG->grade_displaytype), - 'aggregateonlygraded' => 1, - 'aggregateoutcomes' => 0, - 'droplow' => 0, - 'grade_item_itemname' => '', - 'grade_item_iteminfo' => '', - 'grade_item_idnumber' => '', - 'grade_item_gradetype' => GRADE_TYPE_VALUE, - 'grade_item_grademax' => 100, - 'grade_item_grademin' => 1, - 'grade_item_gradepass' => 1, - 'grade_item_display' => GRADE_DISPLAY_TYPE_DEFAULT, - // Hack. This must be -2 to use the default setting. - 'grade_item_decimals' => -2, - 'grade_item_hiddenuntil' => 0, - 'grade_item_locktime' => 0, - 'grade_item_weightoverride' => 0, - 'grade_item_aggregationcoef2' => 0, - 'parentcategory' => $defaultparentcat->id - ]; - - // Most of the data items need boilerplate prepended. These are the exceptions. - $ignorekeys = ['aggregation', 'aggregateonlygraded', 'aggregateoutcomes', 'droplow', 'parentcategoryid', 'parentcategoryidnumber']; - foreach ($options as $key => $value) { - if (!in_array($key, $ignorekeys)) { - $fullkey = 'grade_item_' . $key; - $data[$fullkey] = $value; - } else { - $data[$key] = $value; - } - } - - // Handle parent category special case. - if (array_key_exists('parentcategoryid', $options) && $parentcat = $DB->get_record('grade_categories', - ['id' => $options['parentcategoryid'], 'courseid' => $courseid])) { - $data['parentcategory'] = $parentcat->id; - } else if (array_key_exists('parentcategoryidnumber', $options) && $parentcatgradeitem = $DB->get_record('grade_items', - ['itemtype' => 'category', 'idnumber' => $options['parentcategoryidnumber']], '*', IGNORE_MULTIPLE)) { - if ($parentcat = $DB->get_record('grade_categories', ['courseid' => $courseid, 'id' => $parentcatgradeitem->iteminstance])) { - $data['parentcategory'] = $parentcat->id; - } - } - - // Create new gradecategory item. - $gradecategory = new grade_category(['courseid' => $courseid], false); - $gradecategory->apply_default_settings(); - $gradecategory->apply_forced_settings(); - - // Data Validation. - if (array_key_exists('grade_item_gradetype', $data) and $data['grade_item_gradetype'] == GRADE_TYPE_SCALE) { - if (empty($data['grade_item_scaleid'])) { - $warnings[] = ['item' => 'scaleid', 'warningcode' => 'invalidscale', - 'message' => get_string('missingscale', 'grades')]; - } - } - if (array_key_exists('grade_item_grademin', $data) and array_key_exists('grade_item_grademax', $data)) { - if (($data['grade_item_grademax'] != 0 OR $data['grade_item_grademin'] != 0) AND - ($data['grade_item_grademax'] == $data['grade_item_grademin'] OR - $data['grade_item_grademax'] < $data['grade_item_grademin'])) { - $warnings[] = ['item' => 'grademax', 'warningcode' => 'invalidgrade', - 'message' => get_string('incorrectminmax', 'grades')]; - } - } - - if (!empty($warnings)) { - return ['categoryid' => null, 'warnings' => $warnings]; - } - - // Now call the update function with data. Transactioned so the gradebook isn't broken on bad data. - try { - $transaction = $DB->start_delegated_transaction(); - grade_edit_tree::update_gradecategory($gradecategory, (object) $data); - $transaction->allow_commit(); - } catch (Exception $e) { - // If the submitted data was broken for any reason. - $warnings['database'] = $e->getMessage(); - $transaction->rollback(); - return ['warnings' => $warnings]; - } - - return['categoryid' => $gradecategory->id, 'warnings' => []]; + return['categoryid' => $result['categoryids'][0], 'warnings' => []]; } /** * Returns description of method result value * + * @deprecated since Moodle 3.11 MDL-71031 - please do not use this function any more. + * @todo MDL-71325 This will be deleted in Moodle 4.3. + * @see core_grades\external\create_gradecategories::create_gradecategories() + * * @return external_description * @since Moodle 3.10 */ @@ -734,4 +666,13 @@ class core_grades_external extends external_api { 'warnings' => new external_warnings(), ]); } + + /** + * Marking the method as deprecated. See MDL-71031 for details. + * @since Moodle 3.11 + * @return bool + */ + public static function create_gradecategory_is_deprecated() { + return true; + } } diff --git a/lib/db/services.php b/lib/db/services.php index 6c95c4c2d1a..f0bd423db54 100644 --- a/lib/db/services.php +++ b/lib/db/services.php @@ -935,7 +935,15 @@ $functions = array( 'core_grades_create_gradecategory' => array ( 'classname' => 'core_grades_external', 'methodname' => 'create_gradecategory', - 'description' => 'Create a grade category inside a course gradebook.', + 'description' => '** DEPRECATED ** Please do not call this function any more. Use core_grades_create_gradecategories. + Create a grade category inside a course gradebook.', + 'type' => 'write', + 'capabilities' => 'moodle/grade:manage', + ), + 'core_grades_create_gradecategories' => array ( + 'classname' => 'core_grades\external\create_gradecategories', + 'methodname' => 'create_gradecategories', + 'description' => 'Create grade categories inside a course gradebook.', 'type' => 'write', 'capabilities' => 'moodle/grade:manage', ), diff --git a/lib/upgrade.txt b/lib/upgrade.txt index b6b195a45c5..136c14e096b 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -123,6 +123,8 @@ information provided here is intended especially for developers. - I should see "##tomorrow noon##%A, %d %B %Y, %I:%M %p##" * External functions implementation classes should use 'execute' as the method name, in which case the 'methodname' property should not be specified in db/services.php file. +* The core_grades_create_gradecategory webservice has been deprecated in favour of core_grades_create_gradecategories, which is + functionally identical but allows for parallel gradecategory creations by supplying a data array to the webservice. === 3.10 === * PHPUnit has been upgraded to 8.5. That comes with a few changes: diff --git a/version.php b/version.php index d14d637af70..8cdb4438328 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2021052500.79; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2021052500.80; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.0dev (Build: 20210416)'; // Human-friendly version name From be927f0327e3b43424b5d52f4d708f9b31a5bcec Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Fri, 16 Apr 2021 00:13:54 +0200 Subject: [PATCH 2/2] MDL-71031 external: Minor tweaks towards coding style and external rules - Some small namespace/use/MOODLE_INTERNAL/requires reorganization. - Move file phpdocs to class phpdocs. - Move external method names to be new "execute" default ones. - Move unit tests to matching core_grades\external namespace. - Add missing clean_returnvalue() calls. --- .../external/create_gradecategories.php | 44 +++++++++---------- .../create_gradecategories_test.php} | 39 +++++++++------- lib/db/services.php | 1 - 3 files changed, 44 insertions(+), 40 deletions(-) rename grade/tests/{grades_external_create_gradecategories_test.php => external/create_gradecategories_test.php} (84%) diff --git a/grade/classes/external/create_gradecategories.php b/grade/classes/external/create_gradecategories.php index 3252151d971..924bf4d9eca 100644 --- a/grade/classes/external/create_gradecategories.php +++ b/grade/classes/external/create_gradecategories.php @@ -14,6 +14,21 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . +namespace core_grades\external; + +use external_api; +use external_function_parameters; +use external_value; +use external_single_structure; +use external_multiple_structure; +use external_warnings; + +defined('MOODLE_INTERNAL') || die; + +require_once("$CFG->libdir/externallib.php"); +require_once("$CFG->libdir/gradelib.php"); +require_once("$CFG->dirroot/grade/edit/tree/lib.php"); + /** * Create gradecategories webservice. * @@ -22,30 +37,14 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later * @since Moodle 3.11 */ - -namespace core_grades\external; -defined('MOODLE_INTERNAL') || die; -use \external_function_parameters, - \external_value, - \external_single_structure, - \external_multiple_structure, - \external_warnings; - -require_once("$CFG->libdir/externallib.php"); -require_once("$CFG->libdir/gradelib.php"); -require_once("$CFG->dirroot/grade/edit/tree/lib.php"); - -/** - * Parameter, returns and webservice definitions for create_gradecategories. - */ -class create_gradecategories extends \external_api { +class create_gradecategories extends external_api { /** * Returns description of method parameters * * @return external_function_parameters * @since Moodle 3.11 */ - public static function create_gradecategories_parameters() { + public static function execute_parameters(): external_function_parameters { return new external_function_parameters( [ 'courseid' => new external_value(PARAM_INT, 'id of course', VALUE_REQUIRED), @@ -86,9 +85,10 @@ class create_gradecategories extends \external_api { * @param int $courseid the courseid to create the gradecategory in. * @param array $categories the categories to create. * @return array array of created categoryids and warnings. + * @since Moodle 3.11 */ - public static function create_gradecategories(int $courseid, array $categories) { - $params = self::validate_parameters(self::create_gradecategories_parameters(), + public static function execute(int $courseid, array $categories): array { + $params = self::validate_parameters(self::execute_parameters(), ['courseid' => $courseid, 'categories' => $categories]); // Now params are validated, update the references. @@ -106,10 +106,10 @@ class create_gradecategories extends \external_api { /** * Returns description of method result value * - * @return external_description + * @return external_single_structure * @since Moodle 3.11 */ - public static function create_gradecategories_returns() { + public static function execute_returns(): external_single_structure { return new external_single_structure([ 'categoryids' => new external_multiple_structure( new external_value(PARAM_INT, 'created cateogry ID') diff --git a/grade/tests/grades_external_create_gradecategories_test.php b/grade/tests/external/create_gradecategories_test.php similarity index 84% rename from grade/tests/grades_external_create_gradecategories_test.php rename to grade/tests/external/create_gradecategories_test.php index fce8fa217fb..dfde44b8e5f 100644 --- a/grade/tests/grades_external_create_gradecategories_test.php +++ b/grade/tests/external/create_gradecategories_test.php @@ -14,6 +14,17 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . +namespace core_grades\external; + +use core_grades\external\create_gradecategories; +use external_api; + +defined('MOODLE_INTERNAL') || die; + +global $CFG; + +require_once($CFG->dirroot . '/webservice/tests/helpers.php'); + /** * Unit tests for the core_grades\external\create_gradecategories webservice. * @@ -23,17 +34,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later * @since Moodle 3.11 */ - -defined('MOODLE_INTERNAL') || die(); -global $CFG; -require_once($CFG->dirroot . '/webservice/tests/helpers.php'); - -use \core_grades\external\create_gradecategories; - -/** - * create_gradecategories unit tests. - */ -class create_gradecategories_testcase extends externallib_advanced_testcase { +class create_gradecategories_testcase extends \externallib_advanced_testcase { /** * Test create_gradecategories. @@ -47,10 +48,11 @@ class create_gradecategories_testcase extends externallib_advanced_testcase { $this->setAdminUser(); // Test the most basic gradecategory creation. - $status1 = create_gradecategories::create_gradecategories($course->id, + $status1 = create_gradecategories::execute($course->id, [['fullname' => 'Test Category 1', 'options' => []]]); + $status1 = external_api::clean_returnvalue(create_gradecategories::execute_returns(), $status1); - $courseparentcat = grade_category::fetch_course_category($course->id); + $courseparentcat = \grade_category::fetch_course_category($course->id); $record1 = $DB->get_record('grade_categories', ['id' => $status1['categoryids'][0]]); $this->assertEquals('Test Category 1', $record1->fullname); // Confirm that the parent category for this category is the top level category for the course. @@ -58,8 +60,9 @@ class create_gradecategories_testcase extends externallib_advanced_testcase { $this->assertEquals(2, $record1->depth); // Now create a category as a child of the newly created category. - $status2 = create_gradecategories::create_gradecategories($course->id, + $status2 = create_gradecategories::execute($course->id, [['fullname' => 'Test Category 2', 'options' => ['parentcategoryid' => $record1->id]]]); + $status2 = external_api::clean_returnvalue(create_gradecategories::execute_returns(), $status2); $record2 = $DB->get_record('grade_categories', ['id' => $status2['categoryids'][0]]); $this->assertEquals($record1->id, $record2->parent); $this->assertEquals(3, $record2->depth); @@ -89,9 +92,10 @@ class create_gradecategories_testcase extends externallib_advanced_testcase { 'parentcategoryid' => $record2->id ]; - $status3 = create_gradecategories::create_gradecategories($course->id, + $status3 = create_gradecategories::execute($course->id, [['fullname' => 'Test Category 3', 'options' => $customopts]]); - $cat3 = new grade_category(['courseid' => $course->id, 'id' => $status3['categoryids'][0]], true); + $status3 = external_api::clean_returnvalue(create_gradecategories::execute_returns(), $status3); + $cat3 = new \grade_category(['courseid' => $course->id, 'id' => $status3['categoryids'][0]], true); $cat3->load_grade_item(); // Lets check all of the data is in the right shape. @@ -116,7 +120,7 @@ class create_gradecategories_testcase extends externallib_advanced_testcase { $this->assertEquals($record2->id, $cat3->parent); // Now test creating 2 in parallel, and nesting them. - $status4 = create_gradecategories::create_gradecategories($course->id, [ + $status4 = create_gradecategories::execute($course->id, [ [ 'fullname' => 'Test Category 4', 'options' => [ @@ -131,6 +135,7 @@ class create_gradecategories_testcase extends externallib_advanced_testcase { ], ], ]); + $status4 = external_api::clean_returnvalue(create_gradecategories::execute_returns(), $status4); $secondlevel = $DB->get_record('grade_categories', ['id' => $status4['categoryids'][0]]); $thirdlevel = $DB->get_record('grade_categories', ['id' => $status4['categoryids'][1]]); diff --git a/lib/db/services.php b/lib/db/services.php index f0bd423db54..4cfda198c3e 100644 --- a/lib/db/services.php +++ b/lib/db/services.php @@ -942,7 +942,6 @@ $functions = array( ), 'core_grades_create_gradecategories' => array ( 'classname' => 'core_grades\external\create_gradecategories', - 'methodname' => 'create_gradecategories', 'description' => 'Create grade categories inside a course gradebook.', 'type' => 'write', 'capabilities' => 'moodle/grade:manage',