MDL-50328 tool_lp: Unit test failure and little clean-up of plans page

admin/tool/lp/classes/api.php

@@ -520,7 +520,9 @@ class api {
     /**
      * Fetches all the relevant contexts.
      *
-     * Note: This currently only supports system and category contexts.
+     * Note: This currently only supports system, category and user contexts. However user contexts
+     * behave a bit differently and will fallback on the system context. This is what makes the most
+     * sense because a user context does not have descendants, and only has system as a parent.
      *
      * @param context $context The context to start from.
      * @param string $includes Defines what other contexts to find.
@@ -537,9 +539,10 @@ class api {
         if (!in_array($includes, array('children', 'parents', 'self'))) {
             throw new coding_exception('Invalid parameter value for \'includes\'.');
         }
+
         // If context user swap it for the context_system.
         if ($context->contextlevel == CONTEXT_USER) {
-            $context = $systemcontext = context_system::instance();
+            $context = context_system::instance();
         }
 
         $contexts = array($context->id => $context);
@@ -1364,27 +1367,25 @@ class api {
      * @return bool
      */
     public static function add_competency_to_plan($planid, $competencyid) {
-        // First we do a permissions check.
         $plan = new plan($planid);
-        $plan->read();
-        if (!$plan->can_manage()) {
-            $context = context_user::instance($plan->get_userid());
-            throw new required_capability_exception($context, 'tool/lp:planmanage', 'nopermissions', '');
-        }
 
-        if ($plan->is_based_on_template()) {
+        // First we do a permissions check.
+        if (!$plan->can_manage()) {
+            throw new required_capability_exception($plan->get_context(), 'tool/lp:planmanage', 'nopermissions', '');
+
+        } else if ($plan->is_based_on_template()) {
             throw new coding_exception('A competency can not be added to a learning plan based on a template');
         }
 
-        $record = new stdClass();
-        $record->planid = $planid;
-        $record->competencyid = $competencyid;
-
-        $exists = plan_competency::get_records(array('planid' => $planid, 'competencyid' => $competencyid));
+        $exists = plan_competency::get_record(array('planid' => $planid, 'competencyid' => $competencyid));
         if (!$exists) {
+            $record = new stdClass();
+            $record->planid = $planid;
+            $record->competencyid = $competencyid;
             $plancompetency = new plan_competency(0, $record);
             $plancompetency->create();
         }
+
         return true;
     }
 
@@ -1396,22 +1397,17 @@ class api {
      * @return bool
      */
     public static function remove_competency_from_plan($planid, $competencyid) {
-        // First we do a permissions check.
         $plan = new plan($planid);
-        $plan->read();
+
+        // First we do a permissions check.
         if (!$plan->can_manage()) {
             $context = context_user::instance($plan->get_userid());
             throw new required_capability_exception($context, 'tool/lp:planmanage', 'nopermissions', '');
-        }
 
-        if ($plan->is_based_on_template()) {
+        } else if ($plan->is_based_on_template()) {
             throw new coding_exception('A competency can not be removed from a learning plan based on a template');
         }
 
-        $record = new stdClass();
-        $record->planid = $planid;
-        $record->competencyid = $competencyid;
-
         $link = plan_competency::get_record(array('planid' => $planid, 'competencyid' => $competencyid));
         if ($link) {
             return $link->delete();
@@ -1430,15 +1426,14 @@ class api {
      * @return boolean
      */
     public static function reorder_plan_competency($planid, $competencyidfrom, $competencyidto) {
-        // First we do a permissions check.
         $plan = new plan($planid);
-        $plan->read();
+
+        // First we do a permissions check.
         if (!$plan->can_manage()) {
             $context = context_user::instance($plan->get_userid());
             throw new required_capability_exception($context, 'tool/lp:planmanage', 'nopermissions', '');
-        }
 
-        if ($plan->is_based_on_template()) {
+        } else if ($plan->is_based_on_template()) {
             throw new coding_exception('A competency can not be reordered in a learning plan based on a template');
         }
 

admin/tool/lp/classes/external.php

@@ -868,9 +868,10 @@ class external extends external_api {
     /**
      * Returns the external structure of a full user_competency record.
      *
+     * @param int $fordisplay When true, additional fields for display purposes will be added.
      * @return \external_single_structure
      */
-    protected static function get_user_competency_external_structure() {
+    protected static function get_user_competency_external_structure($fordisplay = false) {
         $id = new external_value(
             PARAM_INT,
             'Database record id'
@@ -912,20 +913,6 @@ class external extends external_api {
             'User who modified this record last'
         );
 
-        // Extra params.
-        $gradename = new external_value(
-            PARAM_TEXT,
-            'User competency status name'
-        );
-        $proficiencyname = new external_value(
-            PARAM_TEXT,
-            'User competency proficiency name'
-        );
-        $statusname = new external_value(
-            PARAM_TEXT,
-            'User competency status name'
-        );
-
         $returns = array(
             'id' => $id,
             'userid' => $userid,
@@ -937,11 +924,26 @@ class external extends external_api {
             'timecreated' => $timecreated,
             'timemodified' => $timemodified,
             'usermodified' => $usermodified,
-            'gradename' => $gradename,
-            'proficiencyname' => $proficiencyname,
-            'statusname' => $statusname,
         );
 
+        if ($fordisplay) {
+            $gradename = new external_value(
+                PARAM_TEXT,
+                'User competency status name'
+            );
+            $proficiencyname = new external_value(
+                PARAM_TEXT,
+                'User competency proficiency name'
+            );
+            $statusname = new external_value(
+                PARAM_TEXT,
+                'User competency status name'
+            );
+            $returns['gradename'] = $gradename;
+            $returns['proficiencyname'] = $proficiencyname;
+            $returns['statusname'] = $statusname;
+        }
+
         return new external_single_structure($returns);
     }
 
@@ -3210,18 +3212,16 @@ class external extends external_api {
      * @return \external_description
      */
     public static function data_for_plan_competencies_page_returns() {
-
         return new external_single_structure(array (
             'planid' => new external_value(PARAM_INT, 'Learning Plan id'),
             'canmanage' => new external_value(PARAM_BOOL, 'User can manage learning plan'),
             'competencies' => new external_multiple_structure(
-                    new external_single_structure(array(
-                        'competency' => self::get_competency_external_structure(),
-                        'usercompetency' => self::get_user_competency_external_structure(),
-                    ))
+                new external_single_structure(array(
+                    'competency' => self::get_competency_external_structure(),
+                    'usercompetency' => self::get_user_competency_external_structure(true),
+                ))
             )
         ));
-
     }
 
     /**

admin/tool/lp/classes/persistent.php

@@ -653,8 +653,7 @@ abstract class persistent {
      * Load a single record.
      *
      * @param array $filters Filters to apply.
-     *
-     * @return \tool_lp\persistent
+     * @return false|\tool_lp\persistent
      */
     public static function get_record($filters = array()) {
         global $DB;

admin/tool/lp/version.php

@@ -25,7 +25,7 @@
 defined('MOODLE_INTERNAL') || die();
 
 
-$plugin->version   = 2015052425; // The current plugin version (Date: YYYYMMDDXX).
+$plugin->version   = 2015052426; // 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).