Code review fixes (#49)

* Add namespace path to pass OpenLMS code quality tests. * Return an empty contextlist * Update versions ready for release * Remove Moodle 3.4 testing
techfromsage · Oct 15, 2019 · ad0b1a2 · ad0b1a2
1 parent 3fcd787
commit ad0b1a2
Show file tree

Hide file tree

Showing 18 changed files with 95 additions and 96 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -28,10 +28,6 @@ matrix:
       env: MOODLE_BRANCH=MOODLE_36_STABLE DB=pgsql
     - php: 7.1
       env: MOODLE_BRANCH=MOODLE_36_STABLE DB=mysqli
-    - php: 7.2
-      env: MOODLE_BRANCH=MOODLE_34_STABLE DB=pgsql
-    - php: 7.2
-      env: MOODLE_BRANCH=MOODLE_34_STABLE DB=mysqli
     - php: 7.2
       env: MOODLE_BRANCH=MOODLE_35_STABLE DB=pgsql
     - php: 7.2

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/classes/event/aspirelists_launch.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/classes/event/aspirelists_launch.php
@@ -7,7 +7,7 @@
  * Provides a hook into the Moodle Event system for logging events.
  * This class implements the key functions needed to provide compatibility with https://docs.moodle.org/dev/Event_2
  * This class represents the event of a user launching an aspire list LTI link.
- * @package aspirelists\event
+ * @package mod_aspirelists\event
  */
 class aspire_lists_launch extends \core\event\base {
     /** @var  array */
@@ -20,7 +20,7 @@ protected function init() {
     }
 
     public static function get_name() {
-        return get_string('eventAspireLaunch', 'aspirelists');
+        return \get_string('eventAspireLaunch', 'aspirelists');
     }
 
     public function get_description() {

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/classes/event/course_module_viewed.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/classes/event/course_module_viewed.php
@@ -5,7 +5,7 @@
 /**
  * Class course_module_viewed
  * This provides an extension to the core course_module_viewed event to easily set legacy log data.
- * @package aspirelists\event
+ * @package mod_aspirelists\event
  */
 class course_module_viewed extends \core\event\course_module_viewed {
     /** @var  array */

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/classes/privacy/provider.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/classes/privacy/provider.php
@@ -57,6 +57,7 @@ public static function get_metadata(collection $collection) : collection {
     public static function get_contexts_for_userid(int $userid): contextlist
     {
         // This plugin does not directly handle user data in moodle
+        return new contextlist();
     }
 
     /**

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/db/upgrade.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/db/upgrade.php
@@ -128,11 +128,11 @@ function xmldb_aspirelists_upgrade($oldversion) {
         upgrade_mod_savepoint(true, 2017070400, 'aspirelists');
     }
 
-    if ($oldversion < 201805291000){
-        upgrade_mod_savepoint(true, 201805291000, 'aspirelists');
+    if ($oldversion < 2018052910){
+        upgrade_mod_savepoint(true, 2018052910, 'aspirelists');
     }
 
-    if ($oldversion < 201910031000){
-        upgrade_mod_savepoint(true, 201910031000, 'aspirelists');
+    if ($oldversion < 2019100310){
+        upgrade_mod_savepoint(true, 2019100310, 'aspirelists');
     }
 }
diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/lib.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/lib.php
@@ -112,7 +112,7 @@ function aspirelists_add_lti_properties(&$aspirelist)
 {
     global $CFG;
 
-    $pluginSettings = get_config('mod_aspirelists');
+    $pluginSettings = \get_config('mod_aspirelists');
 
     $aspirelist->toolurl = $pluginSettings->targetAspire . ASPIRELISTS_LTI_LAUNCH_PATH;
     $aspirelist->instructorchoiceacceptgrades = false;
@@ -125,7 +125,7 @@ function aspirelists_add_lti_properties(&$aspirelist)
     $aspirelist->servicesalt = uniqid('', true);
     if(function_exists('get_course'))
     {
-        $course = get_course($aspirelist->course);
+        $course = \get_course($aspirelist->course);
     } else {
         global $DB;
         $course = $DB->get_record('course', array('id' => $aspirelist->course), '*', MUST_EXIST);
@@ -161,7 +161,7 @@ function aspirelists_add_lti_properties(&$aspirelist)
     if(isset($aspirelist->showexpanded)){
         $customLTIParams[] = 'display_inline_expanded='.(string)$aspirelist->showexpanded;
     }
-    $plugin = get_config('mod_aspirelists');
+    $plugin = \get_config('mod_aspirelists');
     if(isset($plugin->version)){
         $customLTIParams[] = 'moodle_lti_plugin_version='.$plugin->version;
     }
@@ -216,9 +216,9 @@ function aspirelists_cm_info_dynamic(cm_info $cm) {
         $aspirelist = $cm->customdata;
         if(isset($aspirelist->showexpanded) && $aspirelist->showexpanded === '1')
         {
-            $afterLink = get_string('accordion_open', 'aspirelists');
+            $afterLink = \get_string('accordion_open', 'aspirelists');
         } else {
-            $afterLink = get_string('accordion_closed', 'aspirelists');
+            $afterLink = \get_string('accordion_closed', 'aspirelists');
         }
 
         $cm->set_after_link("<span class=\"aspirelists_inline_accordion\">" . $afterLink . "</span>");

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/load_js.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/load_js.php
@@ -6,7 +6,7 @@
     'moodle-mod_aspirelists-inline_display',
     'M.mod_aspirelists.inline_display.init_view',
     array(
-        get_string('accordion_open', 'aspirelists'),
-        get_string('accordion_closed', 'aspirelists')
+        \get_string('accordion_open', 'aspirelists'),
+        \get_string('accordion_closed', 'aspirelists')
     )
 );
diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/mod_form.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/mod_form.php
@@ -21,7 +21,7 @@
 class mod_aspirelists_mod_form extends mod_lti_mod_form {
      function definition() {
          global $CFG, $OUTPUT, $PAGE, $COURSE, $DB;
-         $pluginSettings = get_config('mod_aspirelists');
+         $pluginSettings = \get_config('mod_aspirelists');
 
          $launchUrl = $pluginSettings->targetAspire . ASPIRELISTS_LTI_LAUNCH_PATH;
          $ltiTool = lti_get_tool_by_url_match($launchUrl);
@@ -30,9 +30,9 @@ function definition() {
          $ltiPluginId = $DB->get_field('modules', 'id', array('name'=>$ltiPlugin->name));
 
          $mform =& $this->_form;
-         $mform->addElement('header', 'general', get_string('generalheader', 'aspirelists'));
-         $mform->addElement('text', 'name', get_string('section_title', 'aspirelists'));
-         $mform->setDefault('name', get_string('default_section_title', 'aspirelists'));
+         $mform->addElement('header', 'general', \get_string('generalheader', 'aspirelists'));
+         $mform->addElement('text', 'name', \get_string('section_title', 'aspirelists'));
+         $mform->setDefault('name', \get_string('default_section_title', 'aspirelists'));
          $mform->addRule('name', null, 'required', null, 'client');
          $mform->setType('name', PARAM_TEXT);
 
@@ -52,10 +52,10 @@ function definition() {
          }
          $mform->setAdvanced('showdescription');
          //-------------------------------------------------------
-         $mform->addElement('header', 'course_display', get_string('displayheader', 'aspirelists'));
-         $mform->addElement('select', 'display', get_string('display', 'aspirelists'),
-             array(ASPIRELISTS_DISPLAY_PAGE => get_string('displaypage', 'aspirelists'),
-                 ASPIRELISTS_DISPLAY_INLINE => get_string('displayinline', 'aspirelists')));
+         $mform->addElement('header', 'course_display', \get_string('displayheader', 'aspirelists'));
+         $mform->addElement('select', 'display', \get_string('display', 'aspirelists'),
+             array(ASPIRELISTS_DISPLAY_PAGE => \get_string('displaypage', 'aspirelists'),
+                 ASPIRELISTS_DISPLAY_INLINE => \get_string('displayinline', 'aspirelists')));
          $mform->addHelpButton('display', 'display', 'mod_aspirelists');
 
          if(method_exists($mform, 'setExpanded'))
@@ -64,12 +64,12 @@ function definition() {
          }
 
          // Adding option to show sub-folders expanded or collapsed by default.
-         $mform->addElement('advcheckbox', 'showexpanded', get_string('showexpanded', 'aspirelists'));
+         $mform->addElement('advcheckbox', 'showexpanded', \get_string('showexpanded', 'aspirelists'));
          $mform->addHelpButton('showexpanded', 'showexpanded', 'mod_aspirelists');
          $mform->setDefault('showexpanded', 0);
 
          $this->standard_coursemodule_elements();
-         $this->add_action_buttons(true, get_string('save_and_continue', 'aspirelists'), false);
+         $this->add_action_buttons(true, \get_string('save_and_continue', 'aspirelists'), false);
 
      }
 
@@ -85,6 +85,6 @@ function getPluginManager(){
             return plugin_manager::instance();
         }
         // After moodle 2.6
-        return core_plugin_manager::instance();
+        return \core_plugin_manager::instance();
     }
 }
diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/renderer.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/renderer.php
@@ -3,15 +3,15 @@
 
 class mod_aspirelists_renderer extends plugin_renderer_base {
     function display_aspirelists(stdClass $aspirelist){
-        $pluginSettings = get_config('mod_aspirelists');
+        $pluginSettings = \get_config('mod_aspirelists');
         if(!isset($pluginSettings->defaultInlineListHeight))
         {
             $pluginSettings->defaultInlineListHeight = "400px";
         }
         $output = '';
         if($aspirelist->showdescription)
         {
-            $output .= format_module_intro('aspirelists', $aspirelist, $aspirelist->cmid, false);
+            $output .= \format_module_intro('aspirelists', $aspirelist, $aspirelist->cmid, false);
         }
         if(isset($aspirelist->display) && $aspirelist->display === 1)
         {
@@ -24,7 +24,7 @@ function display_aspirelists(stdClass $aspirelist){
                 $srcType = 'data-intended-src';
             }
 
-            $output .= $this->output->container('<iframe id="aspirelists_inline_readings_' . $aspirelist->id . '" class="aspirelists_inline_list" width="100%" height="' . $pluginSettings->defaultInlineListHeight . '" '.$srcType.'="' . new moodle_url('/mod/aspirelists/launch.php?id='.$aspirelist->cmid) .'" style="' . $style . '"></iframe>');
+            $output .= $this->output->container('<iframe id="aspirelists_inline_readings_' . $aspirelist->id . '" class="aspirelists_inline_list" width="100%" height="' . $pluginSettings->defaultInlineListHeight . '" '.$srcType.'="' . new \moodle_url('/mod/aspirelists/launch.php?id='.$aspirelist->cmid) .'" style="' . $style . '"></iframe>');
             return $output;
         }
     }

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/settings.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/settings.php
@@ -3,23 +3,23 @@
 // Released under the LGPL Licence - http://www.gnu.org/licenses/lgpl.html. Anyone is free to change or redistribute this code.
 
 
-    $settings->add(new admin_setting_configtext('mod_aspirelists/targetAspire',get_string('config_targetAspire', 'mod_aspirelists'),get_string('config_targetAspire_desc', 'mod_aspirelists'),get_string('config_targetAspire_ex', 'mod_aspirelists')));
+    $settings->add(new \admin_setting_configtext('mod_aspirelists/targetAspire',\get_string('config_targetAspire', 'mod_aspirelists'),\get_string('config_targetAspire_desc', 'mod_aspirelists'),\get_string('config_targetAspire_ex', 'mod_aspirelists')));
 
     $options = array(
-        'modules'=>get_string('modules', 'mod_aspirelists'),
-        'courses'=>get_string('courses', 'mod_aspirelists'),
-        'units'=>get_string('units', 'mod_aspirelists'),
-        'programmes'=>get_string('programmes', 'mod_aspirelists'),
-        'subjects'=>get_string('subjects', 'mod_aspirelists'));
-
-    $settings->add(new admin_setting_configselect('mod_aspirelists/courseCodeField',
-        get_string('course_code_field', 'mod_aspirelists'), get_string('course_code_field_desc', 'mod_aspirelists'),
+        'modules'=>\get_string('modules', 'mod_aspirelists'),
+        'courses'=>\get_string('courses', 'mod_aspirelists'),
+        'units'=>\get_string('units', 'mod_aspirelists'),
+        'programmes'=>\get_string('programmes', 'mod_aspirelists'),
+        'subjects'=>\get_string('subjects', 'mod_aspirelists'));
+
+    $settings->add(new \admin_setting_configselect('mod_aspirelists/courseCodeField',
+        \get_string('course_code_field', 'mod_aspirelists'), \get_string('course_code_field_desc', 'mod_aspirelists'),
         'idnumber', array('idnumber'=>'idnumber','shortname'=>'shortname','fullname'=>'fullname')));
 
-    $settings->add(new admin_setting_configtext('mod_aspirelists/moduleCodeRegex',get_string('config_moduleCodeRegex', 'mod_aspirelists'), get_string('config_moduleCodeRegex_desc', 'mod_aspirelists'), get_string('config_moduleCodeRegex_ex', 'mod_aspirelists') ));
+    $settings->add(new \admin_setting_configtext('mod_aspirelists/moduleCodeRegex',\get_string('config_moduleCodeRegex', 'mod_aspirelists'), \get_string('config_moduleCodeRegex_desc', 'mod_aspirelists'), \get_string('config_moduleCodeRegex_ex', 'mod_aspirelists') ));
 
-    $settings->add(new admin_setting_configtext('mod_aspirelists/timePeriodRegex',get_string('config_timePeriodRegex', 'mod_aspirelists'), get_string('config_timePeriodRegex_desc', 'mod_aspirelists'), get_string('config_timePeriodRegex_ex', 'mod_aspirelists') ));
+    $settings->add(new \admin_setting_configtext('mod_aspirelists/timePeriodRegex',\get_string('config_timePeriodRegex', 'mod_aspirelists'), \get_string('config_timePeriodRegex_desc', 'mod_aspirelists'), \get_string('config_timePeriodRegex_ex', 'mod_aspirelists') ));
 
-    $settings->add(new admin_setting_configtext('mod_aspirelists/timePeriodMapping',get_string('config_timePeriodMapping', 'mod_aspirelists'), get_string('config_timePeriodMapping_desc', 'mod_aspirelists'), get_string('config_timePeriodMapping_ex', 'mod_aspirelists') ));
+    $settings->add(new \admin_setting_configtext('mod_aspirelists/timePeriodMapping',\get_string('config_timePeriodMapping', 'mod_aspirelists'), \get_string('config_timePeriodMapping_desc', 'mod_aspirelists'), \get_string('config_timePeriodMapping_ex', 'mod_aspirelists') ));
 
-    $settings->add(new admin_setting_configtext('mod_aspirelists/defaultInlineListHeight',get_string('config_defaultInlineListHeight', 'mod_aspirelists'), get_string('config_defaultInlineListHeight_desc', 'mod_aspirelists'), get_string('config_defaultInlineListHeight_default', 'mod_aspirelists') ));
+    $settings->add(new \admin_setting_configtext('mod_aspirelists/defaultInlineListHeight',\get_string('config_defaultInlineListHeight', 'mod_aspirelists'), \get_string('config_defaultInlineListHeight_desc', 'mod_aspirelists'), \get_string('config_defaultInlineListHeight_default', 'mod_aspirelists') ));
diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/tests/generator_test.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/tests/generator_test.php
@@ -71,12 +71,12 @@ public function test_generator() {
         $this->assertEquals(1, $record->display);
         $this->assertEquals(1, $record->showexpanded);
 
-        $cm = get_coursemodule_from_instance('aspirelists', $list->id);
+        $cm = \get_coursemodule_from_instance('aspirelists', $list->id);
         $this->assertEquals($list->id, $cm->instance);
         $this->assertEquals('aspirelists', $cm->modname);
         $this->assertEquals($course->id, $cm->course);
 
-        $context = context_module::instance($cm->id);
+        $context = \context_module::instance($cm->id);
         $this->assertEquals($list->cmid, $context->instanceid);
 
     }

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/version.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/version.php
@@ -7,12 +7,12 @@
     $plugin = new stdClass();
 }
 
-$plugin->version   = 201910031000; // Version for this plugin - based on the date and then an increment number
-$plugin->requires  = 2012062507; // See http://docs.moodle.org/dev/Moodle_Versions
+$plugin->version   = 2019101500; // Version for this plugin - based on the date and then an increment number
+$plugin->requires  = 2018051700; // Lowest Moodle version required. See http://docs.moodle.org/dev/Moodle_Versions
 $plugin->cron      = 0;
 $plugin->component = 'mod_aspirelists';
-$plugin->maturity  = MATURITY_BETA;
-$plugin->release   = '.0001';
+$plugin->maturity  = MATURITY_STABLE;
+$plugin->release   = 'v0.15'; // Release name to be same as github code release tag
 
 if (isset($CFG->version))
 {

diff --git a/moodle-activity-module-lti-wrapper/mod/aspirelists/view.php b/moodle-activity-module-lti-wrapper/mod/aspirelists/view.php
@@ -41,15 +41,15 @@
 require_once($CFG->dirroot . '/lib/completionlib.php');
 require_once($CFG->dirroot.'/mod/aspirelists/classes/event/course_module_viewed.php');
 
-$id = optional_param('id', 0, PARAM_INT); // Course Module ID, or
-$l  = optional_param('l', 0, PARAM_INT);  // aspirelists ID
+$id = \optional_param('id', 0, PARAM_INT); // Course Module ID, or
+$l  = \optional_param('l', 0, PARAM_INT);  // aspirelists ID
 
 if ($l) {  // Two ways to specify the module
     $list = $DB->get_record('aspirelists', array('id' => $l), '*', MUST_EXIST);
-    $cm = get_coursemodule_from_instance('aspirelists', $list->id, $list->course, false, MUST_EXIST);
+    $cm = \get_coursemodule_from_instance('aspirelists', $list->id, $list->course, false, MUST_EXIST);
 
 } else {
-    $cm = get_coursemodule_from_id('aspirelists', $id, 0, false, MUST_EXIST);
+    $cm = \get_coursemodule_from_id('aspirelists', $id, 0, false, MUST_EXIST);
     $list = $DB->get_record('aspirelists', array('id' => $cm->instance), '*', MUST_EXIST);
 }
 
@@ -68,10 +68,10 @@
 $list->cmid = $cm->id;
 
 $PAGE->set_cm($cm, $course); // set's up global $COURSE
-$context = context_module::instance($cm->id);
+$context = \context_module::instance($cm->id);
 $PAGE->set_context($context);
 
-$url = new moodle_url('/mod/lti/view.php', array('id'=>$cm->id));
+$url = new \moodle_url('/mod/lti/view.php', array('id'=>$cm->id));
 $PAGE->set_url($url);
 
 $launchcontainer = lti_get_launch_container($list, $toolconfig);
@@ -85,10 +85,10 @@
     $PAGE->set_pagelayout('incourse');
 }
 
-require_login($course);
+\require_login($course);
 
 // Mark viewed by user (if required).
-$completion = new completion_info($course);
+$completion = new \completion_info($course);
 $completion->set_module_viewed($cm);
 
 $event = \mod_aspirelists\event\course_module_viewed::create(
@@ -121,7 +121,7 @@
     echo "window.open('launch.php?id=".$cm->id."','aspirelists');";
     echo "//]]\n";
     echo "</script>\n";
-    echo "<p>".get_string("basiclti_in_new_window", "lti")."</p>\n";
+    echo "<p>".\get_string("basiclti_in_new_window", "lti")."</p>\n";
 } else {
     // Request the launch content with an iframe tag instead of the standard moodle LTI object tag
     echo '<iframe id="contentframe" height="600px" width="100%" type="text/html" src="launch.php?id='.$cm->id.'" frameborder="0"></iframe>';