Fix externallib and improve tests. (#431)

* Fix update_user_status external method (#430).

* Define return types for remove_* external methods.

* Improve externallib tests.

* Refactor test generator.

* Address CI copy-paste issues.

externallib.php

@@ -148,16 +148,18 @@ class mod_attendance_external extends external_api {
         require_capability('mod/attendance:manageattendances', $context);
 
         // Delete attendance instance.
-        attendance_delete_instance($params['attendanceid']);
+        $result = attendance_delete_instance($params['attendanceid']);
         rebuild_course_cache($cm->course, true);
+        return $result;
     }
 
     /**
      * Describes remove_attendance return values.
      *
-     * @return void
+     * @return external_value
      */
     public static function remove_attendance_returns() {
+        return new external_value(PARAM_BOOL, 'attendance deletion result');
     }
 
     /**
@@ -286,7 +288,7 @@ class mod_attendance_external extends external_api {
      * Delete session from attendance instance.
      *
      * @param int $sessionid
-     * @return int $sessionid
+     * @return bool
      */
     public static function remove_session(int $sessionid) {
         global $DB;
@@ -310,14 +312,17 @@ class mod_attendance_external extends external_api {
         // Delete session.
         $attendance->delete_sessions(array($sessionid));
         attendance_update_users_grade($attendance);
+
+        return true;
     }
 
     /**
      * Describes remove_session return values.
      *
-     * @return void
+     * @return external_value
      */
     public static function remove_session_returns() {
+        return new external_value(PARAM_BOOL, 'attendance session deletion result');
     }
 
     /**
@@ -494,6 +499,8 @@ class mod_attendance_external extends external_api {
      * @param int $statusset
      */
     public static function update_user_status($sessionid, $studentid, $takenbyid, $statusid, $statusset) {
+        global $DB;
+
         $params = self::validate_parameters(self::update_user_status_parameters(), array(
             'sessionid' => $sessionid,
             'studentid' => $studentid,
@@ -502,12 +509,21 @@ class mod_attendance_external extends external_api {
             'statusset' => $statusset,
         ));
 
-        // Make sure session is open for marking.
         $session = $DB->get_record('attendance_sessions', array('id' => $params['sessionid']), '*', MUST_EXIST);
-        list($canmark, $reason) = attendance_can_student_mark($attforsession);
+        $cm = get_coursemodule_from_instance('attendance', $session->attendanceid, 0, false, MUST_EXIST);
+
+        // Check permissions.
+        $context = context_module::instance($cm->id);
+        self::validate_context($context);
+        require_capability('mod/attendance:view', $context);
+
+        // If not a teacher, make sure session is open for self-marking.
+        if (!has_capability('mod/attendance:takeattendances', $context)) {
+            list($canmark, $reason) = attendance_can_student_mark($session);
             if (!$canmark) {
                 throw new invalid_parameter_exception($reason);
             }
+        }
 
         // Check user id is valid.
         $student = $DB->get_record('user', array('id' => $params['studentid']), '*', MUST_EXIST);

tests/externallib_test.php

@@ -18,7 +18,7 @@
  * External functions test for attendance plugin.
  *
  * @package    mod_attendance
- * @category   external
+ * @category   test
  * @copyright  2015 Caio Bressan Doneda
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
@@ -36,7 +36,7 @@ require_once($CFG->dirroot . '/mod/attendance/externallib.php');
  * This class contains the test cases for webservices.
  *
  * @package    mod_attendance
- * @category external
+ * @category   test
  * @copyright  2015 Caio Bressan Doneda
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
@@ -60,8 +60,7 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
     public function setUp() {
         $this->category = $this->getDataGenerator()->create_category();
         $this->course = $this->getDataGenerator()->create_course(array('category' => $this->category->id));
-
+        $this->attendance = $this->getDataGenerator()->create_module('attendance', array('course' => $this->course->id));
-        $this->attendance = $this->create_attendance();
 
         $this->create_and_enrol_users();
 
@@ -85,28 +84,14 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->attendance->add_sessions($this->sessions);
     }
 
-    /**
-     * Create new attendance activity.
-     */
-    private function create_attendance() {
-        global $DB;
-        $att = $this->getDataGenerator()->create_module('attendance', array('course' => $this->course->id));
-        $cm = $DB->get_record('course_modules', array('id' => $att->cmid));
-        unset($att->cmid);
-        return new mod_attendance_structure($att, $cm, $this->course);
-    }
-
     /** Creating 10 students and 1 teacher. */
     protected function create_and_enrol_users() {
         $this->students = array();
         for ($i = 0; $i < 10; $i++) {
-            $student = $this->getDataGenerator()->create_user();
+            $this->students[] = $this->getDataGenerator()->create_and_enrol($this->course, 'student');
-            $this->getDataGenerator()->enrol_user($student->id, $this->course->id, 5); // Enrol as student.
-            $this->students[] = $student;
         }
 
-        $this->teacher = $this->getDataGenerator()->create_user();
+        $this->teacher = $this->getDataGenerator()->create_and_enrol($this->course, 'editingteacher');
-        $this->getDataGenerator()->enrol_user($this->teacher->id, $this->course->id, 3); // Enrol as teacher.
     }
 
     public function test_get_courses_with_today_sessions() {
@@ -116,12 +101,14 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->attendance->add_sessions($this->sessions);
 
         $courseswithsessions = attendance_handler::get_courses_with_today_sessions($this->teacher->id);
+        $courseswithsessions = external_api::clean_returnvalue(mod_attendance_external::get_courses_with_today_sessions_returns(),
+            $courseswithsessions);
 
         $this->assertTrue(is_array($courseswithsessions));
         $this->assertEquals(count($courseswithsessions), 1);
         $course = array_pop($courseswithsessions);
-        $this->assertEquals($course->fullname, $this->course->fullname);
+        $this->assertEquals($course['fullname'], $this->course->fullname);
-        $attendanceinstance = array_pop($course->attendance_instances);
+        $attendanceinstance = array_pop($course['attendance_instances']);
         $this->assertEquals(count($attendanceinstance['today_sessions']), 2);
     }
 
@@ -129,7 +116,7 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->resetAfterTest(true);
 
         // Make another attendance.
-        $second = $this->create_attendance();
+        $second = $this->getDataGenerator()->create_module('attendance', array('course' => $this->course->id));
 
         // Just add the same session.
         $secondsession = clone $this->sessions[0];
@@ -138,26 +125,33 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $second->add_sessions([$secondsession]);
 
         $courseswithsessions = attendance_handler::get_courses_with_today_sessions($this->teacher->id);
+        $courseswithsessions = external_api::clean_returnvalue(mod_attendance_external::get_courses_with_today_sessions_returns(),
+            $courseswithsessions);
+
         $this->assertTrue(is_array($courseswithsessions));
         $this->assertEquals(count($courseswithsessions), 1);
         $course = array_pop($courseswithsessions);
-        $this->assertEquals(count($course->attendance_instances), 2);
+        $this->assertEquals(count($course['attendance_instances']), 2);
     }
 
     public function test_get_session() {
         $this->resetAfterTest(true);
 
         $courseswithsessions = attendance_handler::get_courses_with_today_sessions($this->teacher->id);
+        $courseswithsessions = external_api::clean_returnvalue(mod_attendance_external::get_courses_with_today_sessions_returns(),
+            $courseswithsessions);
 
         $course = array_pop($courseswithsessions);
-        $attendanceinstance = array_pop($course->attendance_instances);
+        $attendanceinstance = array_pop($course['attendance_instances']);
         $session = array_pop($attendanceinstance['today_sessions']);
 
-        $sessioninfo = attendance_handler::get_session($session->id);
+        $sessioninfo = attendance_handler::get_session($session['id']);
+        $sessioninfo = external_api::clean_returnvalue(mod_attendance_external::get_session_returns(),
+            $sessioninfo);
 
-        $this->assertEquals($this->attendance->id, $sessioninfo->attendanceid);
+        $this->assertEquals($this->attendance->id, $sessioninfo['attendanceid']);
-        $this->assertEquals($session->id, $sessioninfo->id);
+        $this->assertEquals($session['id'], $sessioninfo['id']);
-        $this->assertEquals(count($sessioninfo->users), 10);
+        $this->assertEquals(count($sessioninfo['users']), 10);
     }
 
     public function test_get_session_with_group() {
@@ -190,39 +184,51 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
             $this->attendance->add_sessions([$session]);
             $courseswithsessions = attendance_handler::get_courses_with_today_sessions($this->teacher->id);
         }
+        $courseswithsessions = external_api::clean_returnvalue(mod_attendance_external::get_courses_with_today_sessions_returns(),
+            $courseswithsessions);
 
         $course = array_pop($courseswithsessions);
-        $attendanceinstance = array_pop($course->attendance_instances);
+        $attendanceinstance = array_pop($course['attendance_instances']);
         $session = array_pop($attendanceinstance['today_sessions']);
 
-        $sessioninfo = attendance_handler::get_session($session->id);
+        $sessioninfo = attendance_handler::get_session($session['id']);
+        $sessioninfo = external_api::clean_returnvalue(mod_attendance_external::get_session_returns(),
+            $sessioninfo);
 
-        $this->assertEquals($session->id, $sessioninfo->id);
+        $this->assertEquals($session['id'], $sessioninfo['id']);
-        $this->assertEquals($group->id, $sessioninfo->groupid);
+        $this->assertEquals($group->id, $sessioninfo['groupid']);
-        $this->assertEquals(count($sessioninfo->users), 5);
+        $this->assertEquals(count($sessioninfo['users']), 5);
     }
 
     public function test_update_user_status() {
         $this->resetAfterTest(true);
 
         $courseswithsessions = attendance_handler::get_courses_with_today_sessions($this->teacher->id);
+        $courseswithsessions = external_api::clean_returnvalue(mod_attendance_external::get_courses_with_today_sessions_returns(),
+            $courseswithsessions);
 
         $course = array_pop($courseswithsessions);
-        $attendanceinstance = array_pop($course->attendance_instances);
+        $attendanceinstance = array_pop($course['attendance_instances']);
         $session = array_pop($attendanceinstance['today_sessions']);
 
-        $sessioninfo = attendance_handler::get_session($session->id);
+        $sessioninfo = attendance_handler::get_session($session['id']);
+        $sessioninfo = external_api::clean_returnvalue(mod_attendance_external::get_session_returns(),
+            $sessioninfo);
+
+        $student = array_pop($sessioninfo['users']);
+        $status = array_pop($sessioninfo['statuses']);
+        $statusset = $sessioninfo['statusset'];
 
-        $student = array_pop($sessioninfo->users);
+        $result = mod_attendance_external::update_user_status($session['id'], $student['id'], $this->teacher->id, $status['id'], $statusset);
-        $status = array_pop($sessioninfo->statuses);
+        $result = external_api::clean_returnvalue(mod_attendance_external::update_user_status_returns(), $result);
-        $statusset = $sessioninfo->statusset;
-        attendance_handler::update_user_status($session->id, $student->id, $this->teacher->id, $status->id, $statusset);
 
-        $sessioninfo = attendance_handler::get_session($session->id);
+        $sessioninfo = attendance_handler::get_session($session['id']);
-        $log = $sessioninfo->attendance_log;
+        $sessioninfo = external_api::clean_returnvalue(mod_attendance_external::get_session_returns(),
-        $studentlog = $log[$student->id];
+            $sessioninfo);
 
-        $this->assertEquals($status->id, $studentlog->statusid);
+        $log = array_pop($sessioninfo['attendance_log']);
+        $this->assertEquals($student['id'], $log['studentid']);
+        $this->assertEquals($status['id'], $log['statusid']);
     }
 
     public function test_add_attendance() {
@@ -242,6 +248,7 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
 
         // Create attendance.
         $result = mod_attendance_external::add_attendance($course->id, 'test', 'test', NOGROUPS);
+        $result = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $result);
 
         // Check attendance exist.
         $this->assertCount(1, $DB->get_records('attendance', ['course' => $course->id]));
@@ -255,6 +262,7 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
 
         // Create attendance with "separate groups" group mode.
         $result = mod_attendance_external::add_attendance($course->id, 'testsepgrp', 'testsepgrp', SEPARATEGROUPS);
+        $result = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $result);
 
         // Check attendance exist.
         $this->assertCount(2, $DB->get_records('attendance', ['course' => $course->id]));
@@ -286,7 +294,8 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->assertCount(1, $DB->get_records('attendance_sessions', ['attendanceid' => $this->attendance->id]));
 
         // Remove attendance.
-        mod_attendance_external::remove_attendance($this->attendance->id);
+        $result = mod_attendance_external::remove_attendance($this->attendance->id);
+        $result = external_api::clean_returnvalue(mod_attendance_external::remove_attendance_returns(), $result);
 
         // Check attendance removed.
         $this->assertCount(0, $DB->get_records('attendance', ['course' => $this->course->id]));
@@ -306,47 +315,41 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $teacherrole->id);
         $this->setUser($teacher);
 
-        // Create attendances.
+        // Create attendance with separate groups mode.
-        $attendancenogroups = mod_attendance_external::add_attendance($course->id, 'nogroups', 'test', NOGROUPS);
         $attendancesepgroups = mod_attendance_external::add_attendance($course->id, 'sepgroups', 'test', SEPARATEGROUPS);
-        $attendancevisgroups = mod_attendance_external::add_attendance($course->id, 'visgroups', 'test', VISIBLEGROUPS);
+        $attendancesepgroups = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $attendancesepgroups);
 
-        // Check attendances exist.
+        // Check attendance exist.
-        $this->assertCount(3, $DB->get_records('attendance', ['course' => $course->id]));
+        $this->assertCount(1, $DB->get_records('attendance', ['course' => $course->id]));
-
-        // Create session with group in "no groups" attendance.
-        $this->expectException('invalid_parameter_exception');
-        mod_attendance_external::add_session($attendancenogroups['attendanceid'], 'test', time(), 3600, $group->id, false);
-
-        // Create session with no group in "separate groups" attendance.
-        $this->expectException('invalid_parameter_exception');
-        mod_attendance_external::add_session($attendancesepgroups['attendanceid'], 'test', time(), 3600, 0, false);
-
-        // Create session with invalid group in "visible groups" attendance.
-        $this->expectException('invalid_parameter_exception');
-        mod_attendance_external::add_session($attendancevisgroups['attendanceid'], 'test', time(), 3600, $group->id + 100, false);
 
         // Create session and validate record.
         $time = time();
         $duration = 3600;
         $result = mod_attendance_external::add_session($attendancesepgroups['attendanceid'],
             'testsession', $time, $duration, $group->id, true);
+        $result = external_api::clean_returnvalue(mod_attendance_external::add_session_returns(), $result);
 
         $this->assertCount(1, $DB->get_records('attendance_sessions', ['id' => $result['sessionid']]));
-        $record = $DB->get_records('attendance_sessions', ['id' => $result['sessionid']]);
+        $record = $DB->get_record('attendance_sessions', ['id' => $result['sessionid']]);
         $this->assertEquals($record->description, 'testsession');
         $this->assertEquals($record->attendanceid, $attendancesepgroups['attendanceid']);
         $this->assertEquals($record->groupid, $group->id);
         $this->assertEquals($record->sessdate, $time);
         $this->assertEquals($record->duration, $duration);
         $this->assertEquals($record->calendarevent, 1);
+
+        // Create session with no group in "separate groups" attendance.
+        $this->expectException('invalid_parameter_exception');
+        mod_attendance_external::add_session($attendancesepgroups['attendanceid'], 'test', time(), 3600, 0, false);
     }
 
-    public function test_remove_session() {
+
+    public function test_add_session_group_in_no_group_exception() {
         global $DB;
         $this->resetAfterTest(true);
 
         $course = $this->getDataGenerator()->create_course();
+        $group = $this->getDataGenerator()->create_group(array('courseid' => $course->id));
 
         // Become a teacher.
         $teacher = self::getDataGenerator()->create_user();
@@ -354,32 +357,24 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $teacherrole->id);
         $this->setUser($teacher);
 
-        // Create attendance.
+        // Create attendance with no groups mode.
-        $attendance = mod_attendance_external::add_attendance($course->id, 'test', 'test', NOGROUPS);
+        $attendancenogroups = mod_attendance_external::add_attendance($course->id, 'nogroups', 'test', NOGROUPS);
+        $attendancenogroups = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $attendancenogroups);
 
         // Check attendance exist.
         $this->assertCount(1, $DB->get_records('attendance', ['course' => $course->id]));
 
-        // Create session.
+        // Create session with group in "no groups" attendance.
-        $result0 = mod_attendance_external::add_session($attendance['attendanceid'], 'test0', time(), 3600, 0, false);
+        $this->expectException('invalid_parameter_exception');
-        $result1 = mod_attendance_external::add_session($attendance['attendanceid'], 'test1', time(), 3600, 0, false);
+        mod_attendance_external::add_session($attendancenogroups['attendanceid'], 'test', time(), 3600, $group->id, false);
-
-        $this->assertCount(2, $DB->get_records('attendance_sessions', ['attendanceid' => $attendance['attendanceid']]));
-
-        // Delete session 0.
-        mod_attendance_external::remove_session($result0['sessionid']);
-        $this->assertCount(1, $DB->get_records('attendance_sessions', ['attendanceid' => $attendance['attendanceid']]));
-
-        // Delete session 1.
-        mod_attendance_external::remove_session($result1['sessionid']);
-        $this->assertCount(0, $DB->get_records('attendance_sessions', ['attendanceid' => $attendance['attendanceid']]));
     }
 
-    public function test_add_session_creates_calendar_event() {
+    public function test_add_session_invalid_group_exception() {
         global $DB;
         $this->resetAfterTest(true);
 
         $course = $this->getDataGenerator()->create_course();
+        $group = $this->getDataGenerator()->create_group(array('courseid' => $course->id));
 
         // Become a teacher.
         $teacher = self::getDataGenerator()->create_user();
@@ -387,17 +382,59 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $teacherrole->id);
         $this->setUser($teacher);
 
-        // Create attendance.
+        // Create attendance with visible groups mode.
-        $attendance = mod_attendance_external::add_attendance($course->id, 'test', 'test', NOGROUPS);
+        $attendancevisgroups = mod_attendance_external::add_attendance($course->id, 'visgroups', 'test', VISIBLEGROUPS);
+        $attendancevisgroups = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $attendancevisgroups);
 
         // Check attendance exist.
         $this->assertCount(1, $DB->get_records('attendance', ['course' => $course->id]));
 
+        // Create session with invalid group in "visible groups" attendance.
+        $this->expectException('invalid_parameter_exception');
+        mod_attendance_external::add_session($attendancevisgroups['attendanceid'], 'test', time(), 3600, $group->id + 100, false);
+    }
+
+    public function test_remove_session() {
+        global $DB;
+        $this->resetAfterTest(true);
+
+        // Create attendance with no groups mode.
+        $attendance = mod_attendance_external::add_attendance($this->course->id, 'test', 'test', NOGROUPS);
+        $attendance = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $attendance);
+
+        // Create sessions.
+        $result0 = mod_attendance_external::add_session($attendance['attendanceid'], 'test0', time(), 3600, 0, false);
+        $result0 = external_api::clean_returnvalue(mod_attendance_external::add_session_returns(), $result0);
+        $result1 = mod_attendance_external::add_session($attendance['attendanceid'], 'test1', time(), 3600, 0, false);
+        $result1 = external_api::clean_returnvalue(mod_attendance_external::add_session_returns(), $result1);
+
+        $this->assertCount(2, $DB->get_records('attendance_sessions', ['attendanceid' => $attendance['attendanceid']]));
+
+        // Delete session 0.
+        $result = mod_attendance_external::remove_session($result0['sessionid']);
+        $result = external_api::clean_returnvalue(mod_attendance_external::remove_session_returns(), $result);
+        $this->assertCount(1, $DB->get_records('attendance_sessions', ['attendanceid' => $attendance['attendanceid']]));
+
+        // Delete session 1.
+        $result = mod_attendance_external::remove_session($result1['sessionid']);
+        $result = external_api::clean_returnvalue(mod_attendance_external::remove_session_returns(), $result);
+        $this->assertCount(0, $DB->get_records('attendance_sessions', ['attendanceid' => $attendance['attendanceid']]));
+    }
+
+    public function test_add_session_creates_calendar_event() {
+        global $DB;
+        $this->resetAfterTest(true);
+
+        // Create attendance with no groups mode.
+        $attendance = mod_attendance_external::add_attendance($this->course->id, 'test', 'test', NOGROUPS);
+        $attendance = external_api::clean_returnvalue(mod_attendance_external::add_attendance_returns(), $attendance);
+
         // Prepare events tracing.
         $sink = $this->redirectEvents();
 
         // Create session with no calendar event.
-        mod_attendance_external::add_session($attendance['attendanceid'], 'test0', time(), 3600, 0, false);
+        $result = mod_attendance_external::add_session($attendance['attendanceid'], 'test0', time(), 3600, 0, false);
+        $result = external_api::clean_returnvalue(mod_attendance_external::add_session_returns(), $result);
 
         // Capture the event.
         $events = $sink->get_events();
@@ -408,7 +445,8 @@ class mod_attendance_external_testcase extends externallib_advanced_testcase {
         $this->assertInstanceOf('\mod_attendance\event\session_added', $events[0]);
 
         // Create session with calendar event.
-        mod_attendance_external::add_session($attendance['attendanceid'], 'test0', time(), 3600, 0, true);
+        $result = mod_attendance_external::add_session($attendance['attendanceid'], 'test0', time(), 3600, 0, true);
+        $result = external_api::clean_returnvalue(mod_attendance_external::add_session_returns(), $result);
 
         // Capture the event.
         $events = $sink->get_events();

tests/generator/lib.php

@@ -40,10 +40,10 @@ class mod_attendance_generator extends testing_module_generator {
      *
      * @param array|stdClass $record
      * @param array $options
-     * @return stdClass activity record with extra cmid field
+     * @return stdClass mod_attendance_structure
      */
     public function create_instance($record = null, array $options = null) {
-        global $CFG;
+        global $CFG, $DB;
         require_once($CFG->dirroot.'/mod/attendance/lib.php');
 
         $this->instancecount++;
@@ -62,6 +62,10 @@ class mod_attendance_generator extends testing_module_generator {
             $record->grade = 100;
         }
 
-        return parent::create_instance($record, (array)$options);
+        $att = parent::create_instance($record, (array)$options);
+        $cm = $DB->get_record('course_modules', array('id' => $att->cmid), '*', MUST_EXIST);
+        $course = $DB->get_record('course', array('id' => $cm->course), '*', MUST_EXIST);
+        unset($att->cmid);
+        return new mod_attendance_structure($att, $cm, $course);
     }
 }