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>';