MDL-59368 groups: Peer review fixes

* Fix coalesce on postgres. * The edit icon's alt shows the HTML entities causing * enrol/tests/behat/add_to_group.feature Contains a '@doit' tag which I assume is there from testing. * group/classes/output/user_groups_editable.php ** Missing MOODLE_INTERNAL check. ** Unused variables context_system and moodle_exception. ** The PHPDocs for the constructors are wrong. ** export_for_template() returns an array, not stdClass (according to parent docs). ** Change moodle_exception to coding_exception at the beginning. * group/lib.php ** Missing docs for core_group_inplace_editable(). * user/classes/participants_table.php ** The docs for the class variables $groups, $course and $context need a '\' beforehand as they are in a namespace. ** I would prefer $this->context = $context; and $this->groups = ... to be done at the end of the constructor with the other class variable assignments. ** You could get rid of the class variable courseid if we are setting course and use $this->course->id instead. ** The function col_groups has @param \stdclass $row but it should be $data * lib/amd/src/form-autocomplete.js and lib/amd/src/inplace_editable.js ** Some issues here CiBot has pointed out. * lib/classes/output/inplace_editable.php ** @see should be on a new line.
davidknu · Jul 12, 2017 · f3ecea3 · f3ecea3
1 parent f746a07
commit f3ecea3
Show file tree

Hide file tree

Showing 10 changed files with 39 additions and 25 deletions.
diff --git a/enrol/tests/behat/add_to_group.feature b/enrol/tests/behat/add_to_group.feature
@@ -1,4 +1,4 @@
-@core_enrol @core_group @doit
+@core_enrol @core_group
 Feature: Users can be added to multiple groups at once
   In order to manage group membership effectively
   As a user

diff --git a/group/classes/output/user_groups_editable.php b/group/classes/output/user_groups_editable.php
@@ -24,11 +24,12 @@
 
 namespace core_group\output;
 
-use context_system;
 use context_course;
 use core_user;
 use core_external;
-use moodle_exception;
+use coding_exception;
+
+defined('MOODLE_INTERNAL') || die();
 
 require_once($CFG->dirroot . '/group/lib.php');
 
@@ -49,7 +50,11 @@ class user_groups_editable extends \core\output\inplace_editable {
     /**
      * Constructor.
      *
-     * @param \stdClass|core_tag_tag $tag
+     * @param \stdClass $course The current course
+     * @param \context $context The course context
+     * @param \stdClass $user The current user
+     * @param \stdClass[] $coursegroups The list of course groups from groups_get_all_groups with membership.
+     * @param string $value JSON Encoded list of group ids.
      */
     public function __construct($course, $context, $user, $coursegroups, $value) {
         // Check capabilities to get editable value.
@@ -83,7 +88,7 @@ public function __construct($course, $context, $user, $coursegroups, $value) {
      * Export this data so it can be used as the context for a mustache template.
      *
      * @param \renderer_base $output
-     * @return \stdClass
+     * @return array
      */
     public function export_for_template(\renderer_base $output) {
         $listofgroups = [];

diff --git a/group/lib.php b/group/lib.php
@@ -1097,6 +1097,14 @@ function groups_sync_with_enrolment($enrolname, $courseid = 0, $gidfield = 'cust
     return $affectedusers;
 }
 
+/**
+ * Callback for inplace editable API.
+ *
+ * @param string $itemtype - Only user_groups is supported.
+ * @param string $itemid - Userid and groupid separated by a :
+ * @param string $newvalue - json encoded list of groupids.
+ * @return \core\output\inplace_editable
+ */
 function core_group_inplace_editable($itemtype, $itemid, $newvalue) {
     if ($itemtype === 'user_groups') {
         return \core_group\output\user_groups_editable::update($itemid, $newvalue);

diff --git a/lib/amd/build/form-autocomplete.min.js b/lib/amd/build/form-autocomplete.min.js
diff --git a/lib/amd/build/inplace_editable.min.js b/lib/amd/build/inplace_editable.min.js
diff --git a/lib/amd/src/form-autocomplete.js b/lib/amd/src/form-autocomplete.js
@@ -738,7 +738,7 @@ define(['jquery', 'core/log', 'core/str', 'core/templates', 'core/notification']
             var originalSelect = $(selector);
             if (!originalSelect) {
                 log.debug('Selector not found: ' + selector);
-                return;
+                return false;
             }
 
             // Hide the original select.
@@ -770,7 +770,7 @@ define(['jquery', 'core/log', 'core/str', 'core/templates', 'core/notification']
             var renderDatalist = templates.render('core/form_autocomplete_suggestions', context);
             var renderSelection = templates.render('core/form_autocomplete_selection', context);
 
-            return $.when(renderInput, renderDatalist, renderSelection).done(function(input, suggestions, selection) {
+            return $.when(renderInput, renderDatalist, renderSelection).then(function(input, suggestions, selection) {
                 // Add our new UI elements to the page.
                 originalSelect.after(suggestions);
                 originalSelect.after(input);

diff --git a/lib/amd/src/inplace_editable.js b/lib/amd/src/inplace_editable.js
@@ -262,7 +262,9 @@ define(['jquery',
                 .then(function() {
                 // Focus on the enhanced combobox.
                 el.find('[role=combobox]').focus();
-            });
+                // Stop eslint nagging.
+                return;
+            }).fail(notification.exception);
 
             inputelement.on('keyup', function(e) {
                 if ((e.type === 'keyup' && e.keyCode === 27) || e.type === 'focusout') {

diff --git a/lib/classes/output/inplace_editable.php b/lib/classes/output/inplace_editable.php
@@ -197,7 +197,7 @@ public function set_type_select($options) {
      * Sets the element type to be an autocomplete field
      *
      * @param array $options associative array with dropdown options
-     * @param array $attributes associative array with attributes for autoselect field. @see AMD module core/form-autocomplete
+     * @param array $attributes associative array with attributes for autoselect field. See AMD module core/form-autocomplete.
      * @return self
      */
     public function set_type_autocomplete($options, $attributes) {

diff --git a/lib/templates/inplace_editable.mustache b/lib/templates/inplace_editable.mustache
@@ -56,7 +56,7 @@
         {{^ linkeverything }}{{{displayvalue}}}{{/ linkeverything }}
         <a href="#" class="quickeditlink" data-inplaceeditablelink="1" title="{{edithint}}">
             {{# linkeverything }}{{{displayvalue}}}{{/ linkeverything }}
-            <span class="quickediticon visibleifjs">{{#pix}}t/editstring,core,{{edithint}}{{/pix}}</span>
+            <span class="quickediticon visibleifjs">{{#pix}}t/editstring,core,{{{edithint}}}{{/pix}}</span>
         </a>
 </span>
 {{#js}}

diff --git a/user/classes/participants_table.php b/user/classes/participants_table.php
@@ -76,7 +76,7 @@ class participants_table extends \table_sql {
     protected $countries;
 
     /**
-     * @var stdClass[] The list of groups with membership info for the course.
+     * @var \stdClass[] The list of groups with membership info for the course.
      */
     protected $groups;
 
@@ -86,12 +86,12 @@ class participants_table extends \table_sql {
     protected $extrafields;
 
     /**
-     * @var stdClass The course details.
+     * @var \stdClass The course details.
      */
     protected $course;
 
     /**
-     * @var context The course context.
+     * @var \context The course context.
      */
     protected $context;
 
@@ -115,7 +115,6 @@ public function __construct($courseid, $currentgroup, $accesssince, $roleid, $se
         // Get the context.
         $this->course = get_course($courseid);
         $context = \context_course::instance($courseid, MUST_EXIST);
-        $this->context = $context;
 
         // Define the headers and columns.
         $headers = [];
@@ -136,7 +135,6 @@ public function __construct($courseid, $currentgroup, $accesssince, $roleid, $se
         }
 
         // Load and cache the course groupinfo.
-        $this->groups = groups_get_all_groups($courseid, 0, 0, 'g.*', true);
         // Add column for groups.
         $headers[] = get_string('groups');
         $columns[] = 'groups';
@@ -173,14 +171,15 @@ public function __construct($courseid, $currentgroup, $accesssince, $roleid, $se
         $this->set_attribute('id', 'participants');
 
         // Set the variables we need to use later.
-        $this->courseid = $courseid;
         $this->currentgroup = $currentgroup;
         $this->accesssince = $accesssince;
         $this->roleid = $roleid;
         $this->search = $search;
         $this->selectall = $selectall;
         $this->countries = get_string_manager()->get_list_of_countries();
         $this->extrafields = $extrafields;
+        $this->context = $context;
+        $this->groups = groups_get_all_groups($courseid, 0, 0, 'g.*', true);
     }
 
     /**
@@ -207,25 +206,25 @@ public function col_select($data) {
     public function col_fullname($data) {
         global $OUTPUT;
 
-        return $OUTPUT->user_picture($data, array('size' => 35, 'courseid' => $this->courseid)) . ' ' . fullname($data);
+        return $OUTPUT->user_picture($data, array('size' => 35, 'courseid' => $this->course->id)) . ' ' . fullname($user);
     }
 
     /**
      * Generate the groups column.
      *
-     * @param \stdClass $row
+     * @param \stdClass $data
      * @return string
      */
-    public function col_groups($user) {
+    public function col_groups($data) {
         global $OUTPUT;
 
         $usergroups = [];
         foreach ($this->groups as $coursegroup) {
-            if (isset($coursegroup->members[$user->id])) {
+            if (isset($coursegroup->members[$data->id])) {
                 $usergroups[] = $coursegroup->id;
             }
         }
-        $editable = new \core_group\output\user_groups_editable($this->course, $this->context, $user, $this->groups, $usergroups);
+        $editable = new \core_group\output\user_groups_editable($this->course, $this->context, $data, $this->groups, $usergroups);
         return $OUTPUT->render_from_template('core/inplace_editable', $editable->export_for_template($OUTPUT));
     }
 
@@ -295,7 +294,7 @@ public function other_cols($colname, $data) {
     public function query_db($pagesize, $useinitialsbar = true) {
         list($twhere, $tparams) = $this->get_sql_where();
 
-        $total = user_get_total_participants($this->courseid, $this->currentgroup, $this->accesssince,
+        $total = user_get_total_participants($this->course->id, $this->currentgroup, $this->accesssince,
             $this->roleid, $this->search, $twhere, $tparams);
 
         $this->pagesize($pagesize, $total);
@@ -305,7 +304,7 @@ public function query_db($pagesize, $useinitialsbar = true) {
             $sort = 'ORDER BY ' . $sort;
         }
 
-        $this->rawdata = user_get_participants($this->courseid, $this->currentgroup, $this->accesssince,
+        $this->rawdata = user_get_participants($this->course->id, $this->currentgroup, $this->accesssince,
             $this->roleid, $this->search, $twhere, $tparams, $sort, $this->get_page_start(),
             $this->get_page_size());