From f446b2e133e1b5fdf1f85778feee634713ff1af0 Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Tue, 9 Feb 2016 17:01:09 +0800 Subject: [PATCH] MDL-52534 cbe: Fixes from peer review (linking competencies to activities) --- .../moodle2/restore_tool_lp_plugin.class.php | 41 +++++++++- admin/tool/lp/classes/api.php | 77 +++++++++++++------ .../course_competencies_form_element.php | 6 +- admin/tool/lp/classes/course_competency.php | 10 --- .../course_competency_rule_form_element.php | 3 + .../lp/classes/course_module_competency.php | 37 ++------- admin/tool/lp/classes/external.php | 7 +- admin/tool/lp/lib.php | 24 ++++-- .../form_element_course_competencies.mustache | 1 - admin/tool/lp/tests/api_test.php | 6 +- admin/tool/lp/version.php | 2 +- 11 files changed, 130 insertions(+), 84 deletions(-) delete mode 100644 admin/tool/lp/templates/form_element_course_competencies.mustache diff --git a/admin/tool/lp/backup/moodle2/restore_tool_lp_plugin.class.php b/admin/tool/lp/backup/moodle2/restore_tool_lp_plugin.class.php index b0fa8796e15..b4b5fdbbafe 100644 --- a/admin/tool/lp/backup/moodle2/restore_tool_lp_plugin.class.php +++ b/admin/tool/lp/backup/moodle2/restore_tool_lp_plugin.class.php @@ -64,6 +64,44 @@ class restore_tool_lp_plugin extends restore_tool_plugin { * * @param array $data The data. */ + public function process_course_competency($data) { + global $DB; + + $data = (object) $data; + + // Mapping the competency by ID numbers. + $framework = \tool_lp\competency_framework::get_record(array('idnumber' => $data->frameworkidnumber)); + if (!$framework) { + return; + } + $competency = \tool_lp\competency::get_record(array('idnumber' => $data->idnumber, + 'competencyframeworkid' => $framework->get_id())); + if (!$competency) { + return; + } + + $params = array( + 'competencyid' => $competency->get_id(), + 'courseid' => $this->task->get_courseid() + ); + $query = 'competencyid = :competencyid AND courseid = :courseid'; + $existing = \tool_lp\course_competency::record_exists_select($query, $params); + + if (!$existing) { + // Sortorder is ignored by precaution, anyway we should walk through the records in the right order. + $record = (object) $params; + $record->ruleoutcome = $data->ruleoutcome; + $coursecompetency = new \tool_lp\course_competency(0, $record); + $coursecompetency->create(); + } + + } + + /** + * Process a course module competency. + * + * @param array $data The data. + */ public function process_course_module_competency($data) { global $DB; @@ -84,7 +122,8 @@ class restore_tool_lp_plugin extends restore_tool_plugin { 'competencyid' => $competency->get_id(), 'cmid' => $this->task->get_moduleid() ); - $existing = \tool_lp\course_module_competency::record_exists_select('competencyid = :competencyid AND cmid = :cmid', $params); + $query = 'competencyid = :competencyid AND cmid = :cmid'; + $existing = \tool_lp\course_module_competency::record_exists_select($query, $params); if (!$existing) { // Sortorder is ignored by precaution, anyway we should walk through the records in the right order. diff --git a/admin/tool/lp/classes/api.php b/admin/tool/lp/classes/api.php index e120ab13da7..7b56855377f 100644 --- a/admin/tool/lp/classes/api.php +++ b/admin/tool/lp/classes/api.php @@ -45,6 +45,36 @@ use required_capability_exception; */ class api { + /** + * Validate if current user have acces to the course_module if hidden. + * + * @param mixed $cmmixed The cm_info class, course module record or its ID. + * @param bool $throwexception Throw an exception or not. + * @return bool + */ + protected static function validate_course_module($cmmixed, $throwexception = true) { + $cm = $cmmixed; + if (!is_object($cm)) { + $cmrecord = get_coursemodule_from_id(null, $cmmixed); + $modinfo = get_fast_modinfo($cmrecord->course); + $cm = $modinfo->get_cm($cmmixed); + } else if (!$cm instanceof cm_info) { + // Assume we got a course module record. + $modinfo = get_fast_modinfo($cm->course); + $cm = $modinfo->get_cm($cm->id); + } + + if (!$cm->uservisible) { + if ($throwexception) { + throw new require_login_exception('Course module is hidden'); + } else { + return false; + } + } + + return true; + } + /** * Validate if current user have acces to the course if hidden. * @@ -812,7 +842,7 @@ class api { * * @param int $competencyid The id of the competency to check. * @param int $courseid The id of the course to check. - * @return array[stdClass] Array of stdClass containing course module records. + * @return array[int] Array of course modules ids. */ public static function list_course_modules_using_competency($competencyid, $courseid) { @@ -828,8 +858,10 @@ class api { } $cmlist = course_module_competency::list_course_modules($competencyid, $courseid); - foreach ($cmlist as $id => $cm) { - array_push($result, $cm); + foreach ($cmlist as $cmid) { + if (self::validate_course_module($cmid, false)) { + array_push($result, $cmid); + } } return $result; @@ -847,8 +879,8 @@ class api { $cm = get_coursemodule_from_id('', $cmorid, 0, true, MUST_EXIST); } - // Check the user have access to the course. - self::validate_course($cm->course); + // Check the user have access to the course module. + self::validate_course_module($cm); $context = context_module::instance($cm->id); $capabilities = array('tool/lp:coursecompetencyread', 'tool/lp:coursecompetencymanage'); @@ -857,7 +889,6 @@ class api { } $result = array(); - self::validate_course($cm->course); $cmclist = course_module_competency::list_course_module_competencies($cm->id); foreach ($cmclist as $id => $cmc) { @@ -1054,8 +1085,8 @@ class api { $cm = get_coursemodule_from_id('', $cmorid, 0, true, MUST_EXIST); } - // Check the user have access to the course. - self::validate_course($cm->course); + // Check the user have access to the course module. + self::validate_course_module($cm); $context = context_module::instance($cm->id); $capabilities = array('tool/lp:coursecompetencyread', 'tool/lp:coursecompetencymanage'); @@ -1261,8 +1292,8 @@ class api { $cm = get_coursemodule_from_id('', $cmorid, 0, true, MUST_EXIST); } - // Check the user have access to the course. - self::validate_course($cm->course); + // Check the user have access to the course module. + self::validate_course_module($cm); // First we do a permissions check. $context = context_module::instance($cm->id); @@ -1309,8 +1340,8 @@ class api { if (!is_object($cmorid)) { $cm = get_coursemodule_from_id('', $cmorid, 0, true, MUST_EXIST); } - // Check the user have access to the course. - self::validate_course($cm->course); + // Check the user have access to the course module. + self::validate_course_module($cm); // First we do a permissions check. $context = context_module::instance($cm->id); @@ -1322,10 +1353,9 @@ class api { $record->competencyid = $competencyid; $competency = new competency($competencyid); - $exists = course_module_competency::get_records(array('cmid' => $cm->id, 'competencyid' => $competencyid)); + $exists = course_module_competency::get_record(array('cmid' => $cm->id, 'competencyid' => $competencyid)); if ($exists) { - $coursemodulecompetency = array_pop($exists); - return $coursemodulecompetency->delete(); + return $exists->delete(); } return false; } @@ -1345,8 +1375,8 @@ class api { if (!is_object($cmorid)) { $cm = get_coursemodule_from_id('', $cmorid, 0, true, MUST_EXIST); } - // Check the user have access to the course. - self::validate_course($cm->course); + // Check the user have access to the course module. + self::validate_course_module($cm); // First we do a permissions check. $context = context_module::instance($cm->id); @@ -1403,8 +1433,7 @@ class api { $cm = get_coursemodule_from_id('', $coursemodulecompetency->get_cmid(), 0, true, MUST_EXIST); - $courseid = $cm->course; - self::validate_course($courseid); + self::validate_course_module($cm); $context = context_module::instance($cm->id); require_capability('tool/lp:coursecompetencymanage', $context); @@ -1473,10 +1502,14 @@ class api { $competency = new competency($competencyid); $coursecompetency = new course_competency(); - $exists = $coursecompetency->get_records(array('courseid' => $courseid, 'competencyid' => $competencyid)); + $exists = $coursecompetency->get_record(array('courseid' => $courseid, 'competencyid' => $competencyid)); if ($exists) { - $competency = array_pop($exists); - return $competency->delete(); + // Delete all course_module_competencies for this competency in this course. + $cmcs = course_module_competency::list_course_module_competencies($competencyid, $courseid); + foreach ($cmcs as $cmc) { + $cmc->delete(); + } + return $exists->delete(); } return false; } diff --git a/admin/tool/lp/classes/course_competencies_form_element.php b/admin/tool/lp/classes/course_competencies_form_element.php index f9e199b8a74..d9a30a4d5e9 100644 --- a/admin/tool/lp/classes/course_competencies_form_element.php +++ b/admin/tool/lp/classes/course_competencies_form_element.php @@ -65,8 +65,10 @@ class tool_lp_course_competencies_form_element extends MoodleQuickForm_autocompl if (!empty($options['cmid'])) { $current = \tool_lp\api::list_course_module_competencies_in_course_module($options['cmid']); - $getid = function($coursemodulecompetency) { return $coursemodulecompetency->get_competencyid(); }; - $ids = array_map($getid, $current); + $ids = array(); + foreach ($current as $coursemodulecompetency) { + array_push($ids, $coursemodulecompetency->get_competencyid()); + } $this->setValue($ids); } diff --git a/admin/tool/lp/classes/course_competency.php b/admin/tool/lp/classes/course_competency.php index 9aae02805d9..01ea88d69c9 100644 --- a/admin/tool/lp/classes/course_competency.php +++ b/admin/tool/lp/classes/course_competency.php @@ -339,16 +339,6 @@ class course_competency extends persistent { $table = '{' . self::TABLE . '}'; $sql = "UPDATE $table SET sortorder = sortorder -1 WHERE courseid = ? AND sortorder > ?"; $DB->execute($sql, array($this->get_courseid(), $this->get_sortorder())); - - // Delete all course_module_competencies for this competency in this course. - $cmids = course_module_competency::list_course_modules($this->get_competencyid(), $this->get_courseid()); - if (!empty($cmids)) { - list($in, $params) = $DB->get_in_or_equal(array_keys($cmids), SQL_PARAMS_NAMED); - $params['compid'] = $this->get_competencyid(); - $DB->delete_records_select(course_module_competencies::TABLE, - 'cmid ' . $in . ' AND competencyid = :compid', - $params); - } } /** diff --git a/admin/tool/lp/classes/course_competency_rule_form_element.php b/admin/tool/lp/classes/course_competency_rule_form_element.php index 53724aa855d..9fb72a2055f 100644 --- a/admin/tool/lp/classes/course_competency_rule_form_element.php +++ b/admin/tool/lp/classes/course_competency_rule_form_element.php @@ -64,6 +64,9 @@ class tool_lp_course_competency_rule_form_element extends MoodleQuickForm_select $cmid = $options['cmid']; $current = \tool_lp\api::list_course_module_competencies_in_course_module($cmid); + + // Note: We just pick the outcome set on the first course_module_competency - because in our UI are are + // forcing them to be all the same for each activity. if (!empty($current)) { $one = array_pop($current); $this->setValue($one->get_ruleoutcome()); diff --git a/admin/tool/lp/classes/course_module_competency.php b/admin/tool/lp/classes/course_module_competency.php index 02d884113b2..1eb5d7b702d 100644 --- a/admin/tool/lp/classes/course_module_competency.php +++ b/admin/tool/lp/classes/course_module_competency.php @@ -162,18 +162,18 @@ class course_module_competency extends persistent { * Return the module IDs and visible flags that include this competency in a single course. * * @param int $competencyid The competency id - * @return array containing cmid and visible. + * @return array of ints (cmids) */ public static function list_course_modules($competencyid, $courseid) { global $DB; - $results = $DB->get_records_sql('SELECT coursemodules.id as id, coursemodules.visible as visible + $results = $DB->get_records_sql('SELECT coursemodules.id as id FROM {' . self::TABLE . '} modcomp JOIN {course_modules} coursemodules ON modcomp.cmid = coursemodules.id WHERE modcomp.competencyid = ? AND coursemodules.course = ?', array($competencyid, $courseid)); - return $results; + return array_keys($results); } /** @@ -210,10 +210,10 @@ class course_module_competency extends persistent { FROM {' . competency::TABLE . '} comp JOIN {' . self::TABLE . '} coursemodulecomp ON coursemodulecomp.competencyid = comp.id - WHERE coursemodulecomp.cmid = ?'; + WHERE coursemodulecomp.cmid = ? + ORDER BY coursemodulecomp.sortorder ASC'; $params = array($cmid); - $sql .= ' ORDER BY coursemodulecomp.sortorder ASC'; $results = $DB->get_recordset_sql($sql, $params); $instances = array(); foreach ($results as $result) { @@ -267,29 +267,6 @@ class course_module_competency extends persistent { $DB->execute($sql, array($this->get_cmid(), $this->get_sortorder())); } - /** - * Get the specified mod_competency in this course. - * - * @param int $cmid The course module id - * @param int $competencyid The competency id - * @return course_module_competency - */ - public static function get_course_module_competency($cmid, $competencyid) { - global $DB; - - $sql = 'SELECT crsmodcomp.* - FROM {' . self::TABLE . '} crsmodcomp - WHERE crsmodcomp.cmid = ? AND crsmodcomp.competencyid = ?'; - $params = array($cmid, $competencyid); - - $result = $DB->get_record_sql($sql, $params); - if (!$result) { - throw new coding_exception('The competency does not belong to this course module: ' . $competencyid . ', ' . $cmid); - } - - return new course_module_competency(0, $result); - } - /** * List the course_module_competencies in this course module. * @@ -303,10 +280,10 @@ class course_module_competency extends persistent { FROM {' . self::TABLE . '} coursemodcomp JOIN {' . competency::TABLE . '} comp ON coursemodcomp.competencyid = comp.id - WHERE coursemodcomp.cmid = ?'; + WHERE coursemodcomp.cmid = ? + ORDER BY coursemodcomp.sortorder ASC'; $params = array($cmid); - $sql .= ' ORDER BY coursemodcomp.sortorder ASC'; $results = $DB->get_recordset_sql($sql, $params); $instances = array(); foreach ($results as $result) { diff --git a/admin/tool/lp/classes/external.php b/admin/tool/lp/classes/external.php index a3927c009cb..6fb3eab6312 100644 --- a/admin/tool/lp/classes/external.php +++ b/admin/tool/lp/classes/external.php @@ -1511,7 +1511,6 @@ class external extends external_api { return new external_value(PARAM_INT, 'The number of competencies in this course.'); } - /** /** * Returns description of list_course_module_competencies() parameters. * @@ -1543,8 +1542,7 @@ class external extends external_api { 'cmid' => $cmid )); - $cm = course_module_instance_from_id($params['cmid']); - $context = context_module::instance($cm->id); + $context = context_module::instance($params['cmid']); self::validate_context($context); $output = $PAGE->get_renderer('tool_lp'); @@ -1626,7 +1624,8 @@ class external extends external_api { $coursemodules = api::list_course_modules_using_competency($params['competencyid'], $params['courseid']); $result = array(); - foreach ($coursemodules as $cmrecord) { + foreach ($coursemodules as $cmid) { + $cmrecord = get_coursemodule_from_id(null, $cmid); $context = context_module::instance($cmrecord->id); $exporter = new course_module_summary_exporter($cmrecord, array('context' => $context)); $coursemodulesummary = $exporter->export($output); diff --git a/admin/tool/lp/lib.php b/admin/tool/lp/lib.php index d565b7d73c3..6746138a2b1 100644 --- a/admin/tool/lp/lib.php +++ b/admin/tool/lp/lib.php @@ -435,11 +435,15 @@ function tool_lp_coursemodule_standard_elements($formwrapper, $mform) { function tool_lp_coursemodule_edit_post_actions($data, $course) { $existing = \tool_lp\api::list_course_module_competencies_in_course_module($data->coursemodule); - $getid = function($value) { return $value->get_competencyid(); }; - $existingids = array_map($getid, $existing); + $existingids = array(); + foreach ($existing as $cmc) { + array_push($existingids, $cmc->get_competencyid()); + } - $removed = array_diff($existingids, $data->competencies); - $added = array_diff($data->competencies, $existingids); + $newids = isset($data->competencies) ? $data->competencies : array(); + + $removed = array_diff($existingids, $newids); + $added = array_diff($newids, $existingids); foreach ($removed as $removedid) { \tool_lp\api::remove_competency_from_course_module($data->coursemodule, $removedid); @@ -448,9 +452,13 @@ function tool_lp_coursemodule_edit_post_actions($data, $course) { \tool_lp\api::add_competency_to_course_module($data->coursemodule, $addedid); } - $current = \tool_lp\api::list_course_module_competencies_in_course_module($data->coursemodule); - // Now update the rules for each course_module_competency. - foreach ($current as $coursemodulecompetency) { - \tool_lp\api::set_course_module_competency_ruleoutcome($coursemodulecompetency, $data->competency_rule); + if (isset($data->competency_rule)) { + // Now update the rules for each course_module_competency. + $current = \tool_lp\api::list_course_module_competencies_in_course_module($data->coursemodule); + foreach ($current as $coursemodulecompetency) { + \tool_lp\api::set_course_module_competency_ruleoutcome($coursemodulecompetency, $data->competency_rule); + } } + + return $data; } diff --git a/admin/tool/lp/templates/form_element_course_competencies.mustache b/admin/tool/lp/templates/form_element_course_competencies.mustache deleted file mode 100644 index ef7ab584941..00000000000 --- a/admin/tool/lp/templates/form_element_course_competencies.mustache +++ /dev/null @@ -1 +0,0 @@ - diff --git a/admin/tool/lp/tests/api_test.php b/admin/tool/lp/tests/api_test.php index 914a7cd6dc7..cb29d4893ea 100644 --- a/admin/tool/lp/tests/api_test.php +++ b/admin/tool/lp/tests/api_test.php @@ -2347,11 +2347,7 @@ class tool_lp_api_testcase extends advanced_testcase { $cm = get_coursemodule_from_instance('page', $page->id); // Add a link and list again. $ccm = api::add_competency_to_course_module($cm, $c->get_id()); - $one = (object) array( - 'id' => $cm->id, - 'visible' => true - ); - $expected = array($one); + $expected = array($cm->id); $result = api::list_course_modules_using_competency($c->get_id(), $course->id); $this->assertEquals($expected, $result); diff --git a/admin/tool/lp/version.php b/admin/tool/lp/version.php index 051d7ef803b..2fc382c3807 100644 --- a/admin/tool/lp/version.php +++ b/admin/tool/lp/version.php @@ -25,6 +25,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2015111050; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2015111051; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2014110400; // Requires this Moodle version. $plugin->component = 'tool_lp'; // Full name of the plugin (used for diagnostics).