fix: clean up and some optimisations

- add missing phpdocs, - optimise coursecontext get by caching the call to the class, - convert moodle_url to abs path - remove double blank lines - remove dead code - fix file upload handling
catalyst · May 3, 2024 · 25c1397 · 25c1397
1 parent e1bb6c1
commit 25c1397
Show file tree

Hide file tree

Showing 13 changed files with 132 additions and 46 deletions.
diff --git a/attendees.php b/attendees.php
@@ -59,7 +59,6 @@
 // Load cancellations.
 $cancellations = facetoface_get_cancellations($session->id);
 
-
 /*
  * Capability checks to see if the current user can view this page
  *
@@ -341,7 +340,6 @@
 }
 echo html_writer::link($url, get_string('goback', 'facetoface')) . html_writer::end_tag('p');
 
-
 /*
  * Print unapproved requests (if user able to view)
  */
@@ -358,7 +356,6 @@
             echo html_writer::tag('p', get_string('cannotapproveatcapacity', 'facetoface'));
         }
 
-
         $action = new moodle_url('attendees.php', ['s' => $s]);
         echo html_writer::start_tag('form', ['action' => $action->out(), 'method' => 'post']);
         echo html_writer::empty_tag('input', ['type' => 'hidden', 'name' => 'sesskey', 'value' => $USER->sesskey]);

diff --git a/classes/booking_manager.php b/classes/booking_manager.php
@@ -17,8 +17,10 @@
 namespace mod_facetoface;
 
 use context_course;
-use moodle_exception;
+use context_user;
+use file_storage;
 use lang_string;
+use moodle_exception;
 
 class booking_manager {
 
@@ -31,6 +33,9 @@ class booking_manager {
     /** @var int The course id. */
     private $course;
 
+    /** @var context_course The course context. */
+    private $coursecontext;
+
     /** @var int The course id. */
     private $facetoface;
 
@@ -58,13 +63,13 @@ public function __construct($f, $records = []) {
         $this->f = $f;
         $this->facetoface = $facetoface;
         $this->course = $course;
+        $this->coursecontext = context_course::instance($course->id);
         $this->records = $records;
     }
 
     /**
      * Returns file from file system. File must exist.
      * @param int $fileitemid Item id of file stored in the current $USER's draft file area
-     * @return stored_file
      */
     public function load_from_file(int $fileitemid) {
         global $USER;
@@ -77,7 +82,7 @@ public function load_from_file(int $fileitemid) {
             throw new moodle_exception('error:cannotloadfile', 'mod_facetoface');
         }
 
-        return current($files);
+        $this->file = current($files);
     }
 
     /**
@@ -91,6 +96,10 @@ public function load_from_array(array $records) {
         return $this;
     }
 
+    /**
+     * Get the headers for the records.
+     * @return array
+     */
     public static function get_headers(): array {
         return [
             'email',
@@ -101,7 +110,11 @@ public static function get_headers(): array {
         ];
     }
 
-    private function get_iterator() {
+    /**
+     * Get an iterator for the records.
+     * @return Generator
+     */
+    private function get_iterator(): \Generator {
         if (!$this->usefile) {
             foreach ($this->records as $record) {
                 yield $record;
@@ -131,12 +144,20 @@ private function get_iterator() {
         }
     }
 
-    public function validate() {
+    /**
+     * Validate the records provided to ensure they can be processed without errors.
+     *
+     * As there are multiple dependant data points (users, sessions, capacity)
+     * that are checked. They are all in this method.
+     *
+     * @return array An array of errors.
+     */
+    public function validate(): array {
         global $DB;
         $errors = [];
         $sessioncapacitycache = [];
 
-        // Break into rows.
+        // Break into rows and validate the multiple interdependant fields together.
         foreach ($this->get_iterator() as $index => $entry) {
             $row = $index + 1;
 
@@ -200,8 +221,7 @@ public function validate() {
             }
 
             // Check user enrolment into the course.
-            $coursecontext = context_course::instance($this->course->id);
-            if (isset($userid) && !is_enrolled($coursecontext, $userid)) {
+            if (isset($userid) && !is_enrolled($this->coursecontext, $userid)) {
                 $errors[] = [$row, new lang_string('error:userisnotenrolledintocourse', 'mod_facetoface', $entry->email)];
             }
 

diff --git a/classes/event/course_viewed.php b/classes/event/course_viewed.php
@@ -75,5 +75,4 @@ public static function get_name() {
     public function get_url() {
         return new \moodle_url('/mod/facetoface/index.php', ['id' => $this->courseid]);
     }
-
 }
diff --git a/classes/event/csv_processed.php b/classes/event/csv_processed.php
@@ -0,0 +1,81 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace mod_facetoface\event;
+
+/**
+ * The mod_facetoface CSV processed event.
+ *
+ * @package    mod_facetoface
+ * @author     Kevin Pham <kevinpham@catalyst-au.net>
+ * @copyright  Catalyst IT, 2024
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class csv_processed extends \core\event\base {
+
+    /**
+     * Init method.
+     *
+     * @return void
+     */
+    protected function init() {
+        $this->data['crud'] = 'r';
+        $this->data['edulevel'] = self::LEVEL_PARTICIPATING;
+        $this->data['objecttable'] = 'facetoface';
+    }
+
+    /**
+     * Returns description of what happened.
+     *
+     * @return string
+     */
+    public function get_description() {
+        return "The user with id '$this->userid' has processed an uploaded CSV file for session bookings " .
+            "in the facetoface instance with the course module id '$this->contextinstanceid'.";
+    }
+
+    /**
+     * Return localised event name.
+     *
+     * @return string
+     */
+    public static function get_name() {
+        return get_string('eventcsvprocessed', 'mod_facetoface');
+    }
+
+    /**
+     * Get URL related to the action
+     *
+     * @return \moodle_url
+     */
+    public function get_url() {
+        return new \moodle_url('/mod/facetoface/upload.php', ['f' => $this->objectid]);
+    }
+
+    /**
+     * Custom validation.
+     *
+     * @throws \coding_exception
+     * @return void
+     */
+    protected function validate_data() {
+        parent::validate_data();
+
+        if ($this->contextlevel != CONTEXT_MODULE) {
+            throw new \coding_exception('Context level must be CONTEXT_MODULE.');
+        }
+    }
+}
diff --git a/classes/form/confirm_bookings_form.php b/classes/form/confirm_bookings_form.php
@@ -54,7 +54,7 @@ public function definition() {
 
         $backurl = new moodle_url('/mod/facetoface/upload.php', ['f' => $f]);
         $htmlbuttons = $OUTPUT->render((new single_button(
-            new moodle_url('upload.php', ['f' => $f, 'fileid' => $fileid, 'process' => 1]),
+            new moodle_url('/mod/facetoface/upload.php', ['f' => $f, 'fileid' => $fileid, 'process' => 1]),
             get_string('facetoface:confirmandprocess', 'mod_facetoface'),
             'post',
             true

diff --git a/classes/form/upload_bookings_form.php b/classes/form/upload_bookings_form.php
@@ -30,7 +30,7 @@
 class upload_bookings_form extends \moodleform {
 
     /**
-     * Build form for importing woekflows.
+     * Build form for importing bookings.
      *
      * {@inheritDoc}
      * @see \moodleform::definition()
@@ -42,7 +42,7 @@ public function definition() {
 
         $mform->addElement('header', 'settingsheader', get_string('upload'));
 
-        $url = new moodle_url('example.csv');
+        $url = new moodle_url('/mod/facetoface/example.csv');
         $link = html_writer::link($url, 'example.csv');
         $mform->addElement('static', 'examplecsv', get_string('facetoface:examplecsv', 'mod_facetoface'), $link);
 

diff --git a/classes/privacy/provider.php b/classes/privacy/provider.php
@@ -55,7 +55,7 @@ class provider implements
      * @param collection $items a reference to the collection to use to store the metadata.
      * @return collection the updated collection of metadata items.
      */
-    public static function get_metadata(collection $collection) : collection {
+    public static function get_metadata(collection $collection): collection {
         $collection->add_database_table(
             'facetoface_signups',
             [
@@ -98,7 +98,7 @@ public static function get_metadata(collection $collection) : collection {
      * @param int $userid the userid.
      * @return contextlist the list of contexts containing user info for the user.
      */
-    public static function get_contexts_for_userid(int $userid) : contextlist {
+    public static function get_contexts_for_userid(int $userid): contextlist {
         // Fetch all facetoface contexts with userdata.
         $sql = "SELECT c.id
                   FROM {context} c

diff --git a/db/upgrade.php b/db/upgrade.php
@@ -29,7 +29,6 @@
  */
 
 /**
- *
  * Sends message to administrator listing all updated
  * duplicate custom fields
  * @param array $data

diff --git a/lang/en/facetoface.php b/lang/en/facetoface.php
@@ -810,6 +810,7 @@
 
 /* Face-to-face events and logging */
 $string['eventaddsession'] = 'Session added';
+$string['eventcsvprocessed'] = 'CSV processed';
 $string['eventaddsessionfailed'] = 'Session add (FAILED)';
 $string['eventapproverequests'] = 'Session approve requests';
 $string['eventattendeesviewed'] = 'Session attendees viewed';

diff --git a/lib.php b/lib.php
@@ -1162,7 +1162,7 @@ function facetoface_get_attendees($sessionid) {
     global $CFG, $DB;
 
     $usernamefields = facetoface_get_all_user_name_fields(true, 'u');
-    $records = $DB->get_records_sql("
+    return $DB->get_records_sql("
         SELECT u.id, {$usernamefields},
             u.email,
             su.id AS submissionid,
@@ -1213,8 +1213,6 @@ function facetoface_get_attendees($sessionid) {
             sign.timecreated ASC,
             ss.timecreated ASC
     ", [$sessionid, MDL_F2F_STATUS_BOOKED, MDL_F2F_STATUS_WAITLISTED, $sessionid, MDL_F2F_STATUS_APPROVED]);
-
-    return $records;
 }
 
 /**
@@ -1954,7 +1952,6 @@ function facetoface_user_signup($session, $facetoface, $course, $discountcode,
 
     if (!$success) {
         throw new moodle_exception('error:couldnotupdatef2frecord', 'facetoface');
-        return false;
     }
 
     // Work out which status to use.
@@ -1985,7 +1982,6 @@ function facetoface_user_signup($session, $facetoface, $course, $discountcode,
     // Update status.
     if (!facetoface_update_signup_status($usersignup->id, $newstatus, $userid)) {
         throw new moodle_exception('error:f2ffailedupdatestatus', 'facetoface');
-        return false;
     }
 
     // Add to user calendar -- if facetoface usercalentry is set to true.
@@ -2124,7 +2120,6 @@ function facetoface_send_request_notice($facetoface, $session, $userid) {
  * @param bool $usetransaction Set to true if database transactions are to be used
  *
  * @returns integer ID of newly created signup status, or false
- *
  */
 function facetoface_update_signup_status($signupid, $statuscode, $createdby, $note='', $grade=null) {
     global $DB;
@@ -2904,7 +2899,7 @@ function facetoface_cm_info_view(cm_info $coursemodule) {
                     $moreinfolink = html_writer::link(
                         $signupurl,
                         get_string($signupstr, 'facetoface'),
-                        ['class' => 'f2fsessionlinks f2fsessioninfolink'],
+                        ['class' => 'f2fsessionlinks f2fsessioninfolink']
                     );
                     $span = html_writer::tag('span', get_string('options', 'facetoface').':', ['class' => 'f2fsessionnotice']);
                 }
@@ -2963,13 +2958,13 @@ function facetoface_cm_info_view(cm_info $coursemodule) {
                     $output .= html_writer::tag(
                         'span',
                         get_string('signupforsession', 'facetoface'),
-                        ['class' => 'f2fsessionnotice'],
+                        ['class' => 'f2fsessionnotice']
                     );
                 } else {
                     $output .= html_writer::tag(
                         'span',
                         get_string('upcomingsessions', 'facetoface'),
-                        ['class' => 'f2fsessionnotice'],
+                        ['class' => 'f2fsessionnotice']
                     );
 
                     if (!empty($futuresessions)) {
@@ -3860,9 +3855,7 @@ function facetoface_get_customfielddata($sessionid) {
               JOIN {facetoface_session_data} d ON f.id = d.fieldid
               WHERE d.sessionid = ?";
 
-    $records = $DB->get_records_sql($sql, [$sessionid]);
-
-    return $records;
+    return $DB->get_records_sql($sql, [$sessionid]);
 }
 
 /**
@@ -3937,9 +3930,6 @@ function facetoface_update_trainers($sessionid, $form) {
 
                 if (!$DB->insert_record('facetoface_session_roles', $newtrainer)) {
                     throw new moodle_exception('error:couldnotaddtrainer', 'facetoface');
-                    $transaction->force_transaction_rollback();
-
-                    return false;
                 }
             } else {
                 unset($oldtrainers[$roleid][$trainer]);
@@ -3963,8 +3953,6 @@ function facetoface_update_trainers($sessionid, $form) {
                     'userid' => $trainer->id,
                 ])) {
                     throw new moodle_exception('error:couldnotdeletetrainer', 'facetoface');
-                    $transaction->force_transaction_rollback();
-                    return false;
                 }
             }
         }

diff --git a/sessions.php b/sessions.php
@@ -95,7 +95,6 @@
     'context'  => $modulecontext,
 ];
 
-
 // Handle deletions.
 if ($d && $confirm) {
     if (!confirm_sesskey()) {
@@ -361,7 +360,6 @@
 
 $pagetitle = format_string($facetoface->name);
 
-
 $PAGE->set_title($pagetitle);
 $PAGE->set_heading($course->fullname);
 

diff --git a/sitenotice.php b/sitenotice.php
@@ -97,8 +97,6 @@
         throw new moodle_exception('error:unknownbuttonclicked', 'facetoface', $returnurl);
     }
 
-
-
     $todb = new stdClass();
     $todb->name = trim($fromform->name);
     $todb->text = trim($fromform->text['text']);
-Original file line number
+Diff line change
@@ Expand Up / @@ -75,5 +75,4 @@ public static function get_name() { @@
         public function get_url() {
             return new \moodle_url('/mod/facetoface/index.php', ['id' => $this->courseid]);
         }
     }