From 17ee80ca64ad3e18f3f6f3cc8ea968b5e20437d7 Mon Sep 17 00:00:00 2001 From: Ferran Recio Date: Tue, 16 Feb 2021 17:09:31 +0100 Subject: [PATCH] MDL-70909 mod_h5pactivity: fix reports on freeze context --- .../classes/external/get_user_attempts.php | 35 ++++- mod/h5pactivity/classes/local/manager.php | 90 ++++++++++-- .../classes/local/report/participants.php | 6 +- mod/h5pactivity/tests/behat/locking.feature | 100 +++++++++++++ .../tests/external/get_attempts_test.php | 2 +- .../tests/external/get_user_attempts_test.php | 8 +- mod/h5pactivity/tests/local/manager_test.php | 134 +++++++++++++++++- mod/h5pactivity/upgrade.txt | 7 + 8 files changed, 365 insertions(+), 17 deletions(-) create mode 100644 mod/h5pactivity/tests/behat/locking.feature create mode 100644 mod/h5pactivity/upgrade.txt diff --git a/mod/h5pactivity/classes/external/get_user_attempts.php b/mod/h5pactivity/classes/external/get_user_attempts.php index 1eabb94c6dc..216653e9331 100644 --- a/mod/h5pactivity/classes/external/get_user_attempts.php +++ b/mod/h5pactivity/classes/external/get_user_attempts.php @@ -119,7 +119,7 @@ class get_user_attempts extends external_api { $coursecontext = \context_course::instance($course->id); - $users = get_enrolled_users($coursecontext, '', 0, 'u.id, u.firstname, u.lastname', + $users = self::get_active_users($manager, 'u.id, u.firstname, u.lastname', $sortorder, $page * $perpage, $perpage); $usersattempts = []; @@ -160,6 +160,39 @@ class get_user_attempts extends external_api { return $result; } + /** + * Generate the active users list + * + * @param manager $manager the h5pactivity manager + * @param string $userfields the user fields to get + * @param string $sortorder the SQL sortorder + * @param int $limitfrom SQL limit from + * @param int $limitnum SQL limit num + */ + private static function get_active_users( + manager $manager, + string $userfields = 'u.*', + string $sortorder = null, + int $limitfrom = 0, + int $limitnum = 0 + ): array { + + global $DB; + + $capjoin = $manager->get_active_users_join(true); + + // Final SQL. + $sql = "SELECT DISTINCT {$userfields} + FROM {user} u {$capjoin->joins} + WHERE {$capjoin->wheres}"; + + if (!empty($sortorder)) { + $sql .= " ORDER BY {$sortorder}"; + } + + return $DB->get_records_sql($sql, $capjoin->params, $limitfrom, $limitnum); + } + /** * Export attempts data for a specific user. * diff --git a/mod/h5pactivity/classes/local/manager.php b/mod/h5pactivity/classes/local/manager.php index 54f94cdc26d..12418754267 100644 --- a/mod/h5pactivity/classes/local/manager.php +++ b/mod/h5pactivity/classes/local/manager.php @@ -33,6 +33,7 @@ use cm_info; use moodle_recordset; use core_user; use stdClass; +use core\dml\sql_join; use mod_h5pactivity\event\course_module_viewed; /** @@ -288,21 +289,88 @@ class manager { /** * Count the activity completed attempts. * - * If no user is provided will count all activity attempts. + * If no user is provided the method will count all active users attempts. + * Check get_active_users_join PHPdoc to a more detailed description of "active users". * * @param int|null $userid optional user id (default null) * @return int the total amount of attempts */ public function count_attempts(int $userid = null): int { global $DB; - $params = [ - 'h5pactivityid' => $this->instance->id, - 'completion' => 1 - ]; + + // Counting records is enough for one user. if ($userid) { $params['userid'] = $userid; + $params = [ + 'h5pactivityid' => $this->instance->id, + 'userid' => $userid, + 'completion' => 1, + ]; + return $DB->count_records('h5pactivity_attempts', $params); } - return $DB->count_records('h5pactivity_attempts', $params); + + $usersjoin = $this->get_active_users_join(); + + // Final SQL. + return $DB->count_records_sql( + "SELECT COUNT(*) + FROM {user} u $usersjoin->joins + WHERE $usersjoin->wheres", + array_merge($usersjoin->params) + ); + } + + /** + * Return the join to collect all activity active users. + * + * The concept of active user is relative to the activity permissions. All users with + * "mod/h5pactivity:view" are potential users but those with "mod/h5pactivity:reviewattempts" + * are evaluators and they don't count as valid submitters. + * + * Note that, in general, the active list has the same effect as checking for "mod/h5pactivity:submit" + * but submit capability cannot be used because is a write capability and does not apply to frozen contexts. + * + * @since Moodle 3.11 + * @param bool $allpotentialusers if true, the join will return all active users, not only the ones with attempts. + * @return sql_join the active users attempts join + */ + public function get_active_users_join (bool $allpotentialusers = false): sql_join { + + // Only valid users counts. By default, all users with submit capability are considered potential ones. + $context = $this->get_context(); + + // We want to present all potential users. + $capjoin = get_enrolled_with_capabilities_join($context, '', 'mod/h5pactivity:view'); + + if ($capjoin->cannotmatchanyrows) { + return $capjoin; + } + + // But excluding all reviewattempts users converting a capabilities join into left join. + $reviewersjoin = get_with_capability_join($context, 'mod/h5pactivity:reviewattempts', 'u.id'); + + $capjoin = new sql_join( + $capjoin->joins . "\n LEFT " . str_replace('ra', 'reviewer', $reviewersjoin->joins), + $capjoin->wheres . " AND reviewer.userid IS NULL", + $capjoin->params + ); + + if ($allpotentialusers) { + return $capjoin; + } + + // Add attempts join. + $where = "ha.h5pactivityid = :h5pactivityid AND ha.completion = :completion"; + $params = [ + 'h5pactivityid' => $this->instance->id, + 'completion' => 1, + ]; + + return new sql_join( + $capjoin->joins . "\n JOIN {h5pactivity_attempts} ha ON ha.userid = u.id", + $capjoin->wheres . " AND $where", + array_merge($capjoin->params, $params) + ); } /** @@ -374,6 +442,12 @@ class manager { */ public function get_report(int $userid = null, int $attemptid = null): ?report { global $USER; + + // If tracking is disabled, no reports are available. + if (!$this->instance->enabletracking) { + return null; + } + $attempt = null; if ($attemptid) { $attempt = $this->get_attempt($attemptid); @@ -395,8 +469,8 @@ class manager { return null; } - // Check if that user can be tracked. - if ($user && !$this->is_tracking_enabled($user)) { + // Only enrolled users has reports. + if ($user && !is_enrolled($this->context, $user, 'mod/h5pactivity:view')) { return null; } diff --git a/mod/h5pactivity/classes/local/report/participants.php b/mod/h5pactivity/classes/local/report/participants.php index 5e4765ea067..a9e6f43df36 100644 --- a/mod/h5pactivity/classes/local/report/participants.php +++ b/mod/h5pactivity/classes/local/report/participants.php @@ -28,6 +28,7 @@ namespace mod_h5pactivity\local\report; use mod_h5pactivity\local\report; use mod_h5pactivity\local\manager; use mod_h5pactivity\local\attempt; +use core\dml\sql_join; use table_sql; use moodle_url; use html_writer; @@ -82,8 +83,9 @@ class participants extends table_sql implements report { $this->no_sorting('attempts'); $this->pageable(true); - // Set query SQL. - $capjoin = get_enrolled_with_capabilities_join($this->manager->get_context(), '', 'mod/h5pactivity:submit'); + $capjoin = $this->manager->get_active_users_join(true); + + // Final SQL. $this->set_sql( 'DISTINCT u.id, u.picture, u.firstname, u.lastname, u.firstnamephonetic, u.lastnamephonetic, u.middlename, u.alternatename, u.imagealt, u.email', diff --git a/mod/h5pactivity/tests/behat/locking.feature b/mod/h5pactivity/tests/behat/locking.feature new file mode 100644 index 00000000000..bac16b6f170 --- /dev/null +++ b/mod/h5pactivity/tests/behat/locking.feature @@ -0,0 +1,100 @@ +@mod @mod_h5pactivity @core_h5p +Feature: Add H5P activity context locking + In order to let users access a H5P attempts + As a user + I need to access attempts reports even if no more users can submit attempts + + Background: + Given the following config values are set as admin: + | contextlocking | 1 | + And the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | + | student1 | Student | 1 | student1@example.com | + | student2 | Student | 2 | student2@example.com | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | student1 | C1 | student | + | student2 | C1 | student | + # This test is only about reporting, we don't need to specify any valid H5P file for it. + And the following "activity" exists: + | activity | h5pactivity | + | name | H5P package | + | intro | Test H5P description | + | course | C1 | + | idnumber | h5ppackage | + And the following "mod_h5pactivity > attempt" exists: + | user | student1 | + | h5pactivity | H5P package | + | attempt | 1 | + | interactiontype | compound | + | rawscore | 2 | + | maxscore | 2 | + | duration | 4 | + | completion | 1 | + | success | 1 | + + Scenario: Access participants report on a freeze context + Given the "h5ppackage" "Activity module" is context frozen + And I am on the "C1" "Course" page logged in as "admin" + And I follow "H5P package" + When I follow "View all attempts (1 submitted)" + Then I should see "Student 1" + And I should see "View user attempts (1)" in the "Student 1" "table_row" + And I should see "Student 2" + And I should not see "Teacher 1" + + Scenario: Access own attempts on a freeze context + Given the "h5ppackage" "Activity module" is context frozen + When I am on the "C1" "Course" page logged in as "student1" + And I follow "H5P package" + When I follow "View my attempts" + And I follow "View report" + Then I should see "Attempt #1: Student 1" + And I should see "This attempt is completed" + + Scenario: Access participants report without any user with submit capability + Given the following "permission overrides" exist: + | capability | permission | role | contextlevel | reference | + | mod/h5pactivity:submit | Prohibit | student | System | | + And I am on the "C1" "Course" page logged in as "admin" + And I follow "H5P package" + When I follow "View all attempts (1 submitted)" + Then I should see "Student 1" + And I should see "View user attempts (1)" in the "Student 1" "table_row" + And I should see "Student 2" + And I should not see "Teacher 1" + + Scenario: Access participant report to list students with submit capability but no view one + Given the following "permission overrides" exist: + | capability | permission | role | contextlevel | reference | + | mod/h5pactivity:view | Prohibit | student | System | | + When I am on the "C1" "Course" page logged in as "admin" + And I follow "H5P package" + When I follow "View all attempts (0 submitted)" + Then I should see "No participants to display" + + Scenario: Access participant report but with no users with view or submit capability + Given the following "permission overrides" exist: + | capability | permission | role | contextlevel | reference | + | mod/h5pactivity:submit | Prohibit | student | System | | + | mod/h5pactivity:view | Prohibit | student | System | | + When I am on the "C1" "Course" page logged in as "admin" + And I follow "H5P package" + When I follow "View all attempts (0 submitted)" + Then I should see "No participants to display" + + Scenario: Access participant report in a hidden activity + Given I log in as "admin" + And I am on "Course 1" course homepage with editing mode on + And I click on "Hide" "link" in the "H5P package" activity + When I follow "H5P package" + And I follow "View all attempts (1 submitted)" + Then I should see "Student 1" + And I should see "View user attempts (1)" + And I should see "Student 2" + And I should not see "Teacher 1" diff --git a/mod/h5pactivity/tests/external/get_attempts_test.php b/mod/h5pactivity/tests/external/get_attempts_test.php index bfc6408314f..2ada441780a 100644 --- a/mod/h5pactivity/tests/external/get_attempts_test.php +++ b/mod/h5pactivity/tests/external/get_attempts_test.php @@ -468,7 +468,7 @@ class get_attempts_testcase extends externallib_advanced_testcase { 'editingteacher', ['student1', 'noattempts'], [], ['student1', 'noattempts'] ], 'Teacher checking no students' => [ - 'editingteacher', [], ['editingteacher'], [] + 'editingteacher', [], [], ['editingteacher'] ], 'Teacher checking one student and a no enrolled user' => [ 'editingteacher', ['student1', 'noenrolled'], ['noenrolled'], ['student1'] diff --git a/mod/h5pactivity/tests/external/get_user_attempts_test.php b/mod/h5pactivity/tests/external/get_user_attempts_test.php index 4865256ed28..abc71d65209 100644 --- a/mod/h5pactivity/tests/external/get_user_attempts_test.php +++ b/mod/h5pactivity/tests/external/get_user_attempts_test.php @@ -151,25 +151,25 @@ class get_user_attempts_testcase extends externallib_advanced_testcase { 'Teacher checking students with attempts' => [ 'editingteacher', ['student1', 'student2', 'student3', 'student4', 'student5'], - ['editingteacher'], + [], ['student1', 'student2', 'student3', 'student4', 'student5'], ], 'Teacher checking 2 students with atempts and one not' => [ 'editingteacher', ['student1', 'student2', 'noattempts'], - ['editingteacher'], + [], ['student1', 'student2', 'noattempts'], ], 'Teacher checking no students' => [ 'editingteacher', [], - ['editingteacher'], + [], [], ], 'Teacher checking one student and a no enrolled user' => [ 'editingteacher', ['student1', 'noenrolled'], - ['editingteacher'], + [], ['student1'], ], diff --git a/mod/h5pactivity/tests/local/manager_test.php b/mod/h5pactivity/tests/local/manager_test.php index bbbf9eac79f..c17afc7a5bf 100644 --- a/mod/h5pactivity/tests/local/manager_test.php +++ b/mod/h5pactivity/tests/local/manager_test.php @@ -530,7 +530,7 @@ class manager_testcase extends \advanced_testcase { } /** - * Test static count_attempts. + * Test static count_attempts of one user. */ public function test_count_attempts() { @@ -560,6 +560,138 @@ class manager_testcase extends \advanced_testcase { $this->assertEquals(3, $manager->count_attempts($user3->id)); } + /** + * Test static count_attempts of all active participants. + * + * @dataProvider count_attempts_all_data + * @param bool $canview if the student role has mod_h5pactivity/view capability + * @param bool $cansubmit if the student role has mod_h5pactivity/submit capability + * @param bool $extrarole if an extra role without submit capability is required + * @param int $result the expected result + */ + public function test_count_attempts_all(bool $canview, bool $cansubmit, bool $extrarole, int $result) { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + + $course = $this->getDataGenerator()->create_course(); + $activity = $this->getDataGenerator()->create_module( + 'h5pactivity', + ['course' => $course] + ); + + $manager = manager::create_from_instance($activity); + + $roleid = $DB->get_field('role', 'id', ['shortname' => 'student']); + + $newcap = ($canview) ? CAP_ALLOW : CAP_PROHIBIT; + role_change_permission($roleid, $manager->get_context(), 'mod/h5pactivity:view', $newcap); + + $newcap = ($cansubmit) ? CAP_ALLOW : CAP_PROHIBIT; + role_change_permission($roleid, $manager->get_context(), 'mod/h5pactivity:submit', $newcap); + + // Teacher with review capability and attempts (should not be listed). + if ($extrarole) { + $user1 = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + $this->generate_fake_attempts($activity, $user1, 1); + } + + // Student with attempts. + $user2 = $this->getDataGenerator()->create_and_enrol($course, 'student'); + $this->generate_fake_attempts($activity, $user2, 1); + + // Another student with attempts. + $user3 = $this->getDataGenerator()->create_and_enrol($course, 'student'); + $this->generate_fake_attempts($activity, $user3, 1); + + $this->assertEquals($result, $manager->count_attempts()); + } + + /** + * Data provider for test_count_attempts_all. + * + * @return array + */ + public function count_attempts_all_data(): array { + return [ + 'Students with both view and submit capability' => [true, true, false, 6], + 'Students without view but with submit capability' => [false, true, false, 0], + 'Students with view but without submit capability' => [true, false, false, 6], + 'Students without both view and submit capability' => [false, false, false, 0], + 'Students with both view and submit capability and extra role' => [true, true, true, 6], + 'Students without view but with submit capability and extra role' => [false, true, true, 0], + 'Students with view but without submit capability and extra role' => [true, false, true, 6], + 'Students without both view and submit capability and extra role' => [false, false, true, 0], + ]; + } + + /** + * Test static count_attempts of all active participants. + * + * Most method scenarios are tested in test_count_attempts_all so we only + * need to test the with $allpotentialusers true and false. + * + * @dataProvider get_active_users_join_data + * @param bool $allpotentialusers if the join should return all potential users or only the submitted ones. + * @param int $result the expected result + */ + public function test_get_active_users_join(bool $allpotentialusers, int $result) { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + + $course = $this->getDataGenerator()->create_course(); + $activity = $this->getDataGenerator()->create_module( + 'h5pactivity', + ['course' => $course] + ); + + $manager = manager::create_from_instance($activity); + + $user1 = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + $this->generate_fake_attempts($activity, $user1, 1); + + // Student with attempts. + $user2 = $this->getDataGenerator()->create_and_enrol($course, 'student'); + $this->generate_fake_attempts($activity, $user2, 1); + + // 2 more students without attempts. + $this->getDataGenerator()->create_and_enrol($course, 'student'); + $this->getDataGenerator()->create_and_enrol($course, 'student'); + + $usersjoin = $manager->get_active_users_join($allpotentialusers); + + // Final SQL. + $num = $DB->count_records_sql( + "SELECT COUNT(DISTINCT u.id) + FROM {user} u $usersjoin->joins + WHERE $usersjoin->wheres", + array_merge($usersjoin->params) + ); + + $this->assertEquals($result, $num); + } + + /** + * Data provider for test_get_active_users_join. + * + * @return array + */ + public function get_active_users_join_data(): array { + return [ + 'All potential users' => [ + 'allpotentialusers' => true, + 'result' => 3, + ], + 'Users with attempts' => [ + 'allpotentialusers' => false, + 'result' => 1, + ], + ]; + } + /** * Test static count_attempts. */ diff --git a/mod/h5pactivity/upgrade.txt b/mod/h5pactivity/upgrade.txt new file mode 100644 index 00000000000..70435b5d661 --- /dev/null +++ b/mod/h5pactivity/upgrade.txt @@ -0,0 +1,7 @@ +This files describes API changes in /mod/h5pactivity/*, +information provided here is intended especially for developers. + +=== 3.11 === + +* Added mod_h5pactivity\local\manager::get_active_users_join method to query all active + users from a specific activity, even in a freeze context.