MDL-70909 mod_h5pactivity: fix reports on freeze context

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,23 +289,90 @@ 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);
         }
 
+        $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.9.7
+     * @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)
+        );
+    }
+
     /**
      * Return an array of all users and it's total attempts.
      *
@@ -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;
         }
 

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

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"

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

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.
      */

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.9.7 ===
+
+* Added mod_h5pactivity\local\manager::get_active_users_join method to query all active
+  users from a specific activity, even in a freeze context.