From 150280bafa72bad2e1c06a31195fb77cdf7f29e8 Mon Sep 17 00:00:00 2001 From: Ferran Recio Date: Tue, 16 Feb 2021 17:08:51 +0100 Subject: [PATCH 1/3] MDL-70909 core: add cannotmatchanyrows to capabilities join --- enrol/tests/enrollib_test.php | 75 ++++++++++++++++++++++++++++++++++- lib/enrollib.php | 12 +++++- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/enrol/tests/enrollib_test.php b/enrol/tests/enrollib_test.php index acd3e6086f1..891cc8e8c69 100644 --- a/enrol/tests/enrollib_test.php +++ b/enrol/tests/enrollib_test.php @@ -34,7 +34,7 @@ defined('MOODLE_INTERNAL') || die(); * @copyright 2012 Petr Skoda {@link http://skodak.org} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class core_enrollib_testcase extends advanced_testcase { +class enrollib_test extends advanced_testcase { public function test_enrol_get_all_users_courses() { global $DB, $CFG; @@ -1362,4 +1362,77 @@ class core_enrollib_testcase extends advanced_testcase { $durationinday = $duration / DAYSECS; $this->assertEquals(9, $durationinday); } + + /** + * Test get_enrolled_with_capabilities_join cannotmatchanyrows attribute. + * + * @dataProvider get_enrolled_with_capabilities_join_cannotmatchanyrows_data() + * @param string $capability the tested capability + * @param bool $useprohibit if the capability must be assigned to prohibit + * @param int $expectedmatch expected cannotmatchanyrows value + * @param int $expectedcount expceted count value + */ + public function test_get_enrolled_with_capabilities_join_cannotmatchanyrows( + string $capability, + bool $useprohibit, + int $expectedmatch, + int $expectedcount + ) { + global $DB, $CFG; + + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + $roleid = $CFG->defaultuserroleid; + + // Override capability if necessary. + if ($useprohibit && $capability) { + assign_capability($capability, CAP_PROHIBIT, $roleid, $context); + } + + // Check if we must enrol or not. + $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + + $join = get_enrolled_with_capabilities_join($context, '', $capability); + + // Execute query. + $sql = "SELECT COUNT(DISTINCT u.id) + FROM {user} u {$join->joins} + WHERE {$join->wheres}"; + $countrecords = $DB->count_records_sql($sql, $join->params); + + // Validate cannotmatchanyrows. + $this->assertEquals($expectedmatch, $join->cannotmatchanyrows); + $this->assertEquals($expectedcount, $countrecords); + } + + /** + * Data provider for test_get_enrolled_with_capabilities_join_cannotmatchanyrows + * + * @return @array of testing scenarios + */ + public function get_enrolled_with_capabilities_join_cannotmatchanyrows_data() { + return [ + 'no prohibits, no capability' => [ + 'capability' => '', + 'useprohibit' => false, + 'expectedmatch' => 0, + 'expectedcount' => 1, + ], + 'no prohibits with capability' => [ + 'capability' => 'moodle/course:manageactivities', + 'useprohibit' => false, + 'expectedmatch' => 0, + 'expectedcount' => 1, + ], + 'prohibits with capability' => [ + 'capability' => 'moodle/course:manageactivities', + 'useprohibit' => true, + 'expectedmatch' => 1, + 'expectedcount' => 0, + ], + ]; + } } diff --git a/lib/enrollib.php b/lib/enrollib.php index 7b40ca068ce..22611c6af44 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -1384,6 +1384,10 @@ function is_enrolled(context $context, $user = null, $withcapability = '', $only * several times (e.g. as manual enrolment, and as self enrolment). You may * need to use a SELECT DISTINCT in your query (see get_enrolled_sql for example). * + * In case is guaranteed some of the joins never match any rows, the resulting + * join_sql->cannotmatchanyrows will be true. This happens when the capability + * is prohibited. + * * @param context $context * @param string $prefix optional, a prefix to the user id column * @param string|array $capability optional, may include a capability name, or array of names. @@ -1393,24 +1397,27 @@ function is_enrolled(context $context, $user = null, $withcapability = '', $only * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions * @param bool $onlysuspended inverse of onlyactive, consider only suspended enrolments * @param int $enrolid The enrolment ID. If not 0, only users enrolled using this enrolment method will be returned. - * @return \core\dml\sql_join Contains joins, wheres, params + * @return \core\dml\sql_join Contains joins, wheres, params and cannotmatchanyrows */ function get_enrolled_with_capabilities_join(context $context, $prefix = '', $capability = '', $group = 0, $onlyactive = false, $onlysuspended = false, $enrolid = 0) { $uid = $prefix . 'u.id'; $joins = array(); $wheres = array(); + $cannotmatchanyrows = false; $enrolledjoin = get_enrolled_join($context, $uid, $onlyactive, $onlysuspended, $enrolid); $joins[] = $enrolledjoin->joins; $wheres[] = $enrolledjoin->wheres; $params = $enrolledjoin->params; + $cannotmatchanyrows = $cannotmatchanyrows || $enrolledjoin->cannotmatchanyrows; if (!empty($capability)) { $capjoin = get_with_capability_join($context, $capability, $uid); $joins[] = $capjoin->joins; $wheres[] = $capjoin->wheres; $params = array_merge($params, $capjoin->params); + $cannotmatchanyrows = $cannotmatchanyrows || $capjoin->cannotmatchanyrows; } if ($group) { @@ -1420,13 +1427,14 @@ function get_enrolled_with_capabilities_join(context $context, $prefix = '', $ca if (!empty($groupjoin->wheres)) { $wheres[] = $groupjoin->wheres; } + $cannotmatchanyrows = $cannotmatchanyrows || $groupjoin->cannotmatchanyrows; } $joins = implode("\n", $joins); $wheres[] = "{$prefix}u.deleted = 0"; $wheres = implode(" AND ", $wheres); - return new \core\dml\sql_join($joins, $wheres, $params); + return new \core\dml\sql_join($joins, $wheres, $params, $cannotmatchanyrows); } /** From cd97f217b61ca8b21273d8baee1ecfbf7a7e166f Mon Sep 17 00:00:00 2001 From: Ferran Recio Date: Mon, 19 Apr 2021 12:01:36 +0200 Subject: [PATCH 2/3] MDL-70909 behat: context freeze and not freeze steps --- lib/behat/classes/behat_generator_base.php | 51 ++---------------- lib/behat/classes/behat_session_trait.php | 63 ++++++++++++++++++++++ lib/tests/behat/behat_permissions.php | 40 ++++++++++++++ 3 files changed, 106 insertions(+), 48 deletions(-) diff --git a/lib/behat/classes/behat_generator_base.php b/lib/behat/classes/behat_generator_base.php index 44ea79d81c7..04942c3c965 100644 --- a/lib/behat/classes/behat_generator_base.php +++ b/lib/behat/classes/behat_generator_base.php @@ -25,10 +25,11 @@ defined('MOODLE_INTERNAL') || die(); +require_once(__DIR__ . '/../../behat/behat_base.php'); + use Behat\Gherkin\Node\TableNode as TableNode; use Behat\Behat\Tester\Exception\PendingException as PendingException; - /** * Class to quickly create Behat test data using component data generators. * @@ -480,53 +481,7 @@ abstract class behat_generator_base { * @return context */ protected function get_context($levelname, $contextref) { - global $DB; - - // Getting context levels and names (we will be using the English ones as it is the test site language). - $contextlevels = context_helper::get_all_levels(); - $contextnames = array(); - foreach ($contextlevels as $level => $classname) { - $contextnames[context_helper::get_level_name($level)] = $level; - } - - if (empty($contextnames[$levelname])) { - throw new Exception('The specified "' . $levelname . '" context level does not exist'); - } - $contextlevel = $contextnames[$levelname]; - - // Return it, we don't need to look for other internal ids. - if ($contextlevel == CONTEXT_SYSTEM) { - return context_system::instance(); - } - - switch ($contextlevel) { - - case CONTEXT_USER: - $instanceid = $DB->get_field('user', 'id', array('username' => $contextref)); - break; - - case CONTEXT_COURSECAT: - $instanceid = $DB->get_field('course_categories', 'id', array('idnumber' => $contextref)); - break; - - case CONTEXT_COURSE: - $instanceid = $DB->get_field('course', 'id', array('shortname' => $contextref)); - break; - - case CONTEXT_MODULE: - $instanceid = $DB->get_field('course_modules', 'id', array('idnumber' => $contextref)); - break; - - default: - break; - } - - $contextclass = $contextlevels[$contextlevel]; - if (!$context = $contextclass::instance($instanceid, IGNORE_MISSING)) { - throw new Exception('The specified "' . $contextref . '" context reference does not exist'); - } - - return $context; + return behat_base::get_context($levelname, $contextref); } /** diff --git a/lib/behat/classes/behat_session_trait.php b/lib/behat/classes/behat_session_trait.php index ebded0b9d61..8d2b1ad4d50 100644 --- a/lib/behat/classes/behat_session_trait.php +++ b/lib/behat/classes/behat_session_trait.php @@ -1068,6 +1068,69 @@ EOF; \core\session\manager::set_user($user); } + + /** + * Gets the internal moodle context id from the context reference. + * + * The context reference changes depending on the context + * level, it can be the system, a user, a category, a course or + * a module. + * + * @throws Exception + * @param string $levelname The context level string introduced by the test writer + * @param string $contextref The context reference introduced by the test writer + * @return context + */ + public static function get_context(string $levelname, string $contextref): context { + global $DB; + + // Getting context levels and names (we will be using the English ones as it is the test site language). + $contextlevels = context_helper::get_all_levels(); + $contextnames = array(); + foreach ($contextlevels as $level => $classname) { + $contextnames[context_helper::get_level_name($level)] = $level; + } + + if (empty($contextnames[$levelname])) { + throw new Exception('The specified "' . $levelname . '" context level does not exist'); + } + $contextlevel = $contextnames[$levelname]; + + // Return it, we don't need to look for other internal ids. + if ($contextlevel == CONTEXT_SYSTEM) { + return context_system::instance(); + } + + switch ($contextlevel) { + + case CONTEXT_USER: + $instanceid = $DB->get_field('user', 'id', array('username' => $contextref)); + break; + + case CONTEXT_COURSECAT: + $instanceid = $DB->get_field('course_categories', 'id', array('idnumber' => $contextref)); + break; + + case CONTEXT_COURSE: + $instanceid = $DB->get_field('course', 'id', array('shortname' => $contextref)); + break; + + case CONTEXT_MODULE: + $instanceid = $DB->get_field('course_modules', 'id', array('idnumber' => $contextref)); + break; + + default: + break; + } + + $contextclass = $contextlevels[$contextlevel]; + if (!$context = $contextclass::instance($instanceid, IGNORE_MISSING)) { + throw new Exception('The specified "' . $contextref . '" context reference does not exist'); + } + + return $context; + } + /** * Trigger click on node via javascript instead of actually clicking on it via pointer. * diff --git a/lib/tests/behat/behat_permissions.php b/lib/tests/behat/behat_permissions.php index b0bca4842f6..155ad85d001 100644 --- a/lib/tests/behat/behat_permissions.php +++ b/lib/tests/behat/behat_permissions.php @@ -249,4 +249,44 @@ class behat_permissions extends behat_base { } } } + + /** + * Mark context as frozen. + * + * @Then /^the "(?P(?:[^"]|\\")*)" "(?P[^"]*)" is context frozen$/ + * @throws ExpectationException if the context cannot be frozen or found + * @param string $element Element we look on + * @param string $selector The type of where we look (activity, course) + */ + public function the_context_is_context_frozen(string $element, string $selector) { + + // Enable context freeze if it is not done yet. + set_config('contextlocking', 1); + + // Find context. + $context = self::get_context($selector, $element); + + // Freeze context. + $context->set_locked(true); + } + + /** + * Unmark context as frozen. + * + * @Then /^the "(?P(?:[^"]|\\")*)" "(?P[^"]*)" is not context frozen$/ + * @throws ExpectationException if the context cannot be frozen or found + * @param string $element Element we look on + * @param string $selector The type of where we look (activity, course) + */ + public function the_context_is_not_context_frozen(string $element, string $selector) { + + // Enable context freeze if it is not done yet. + set_config('contextlocking', 1); + + // Find context. + $context = self::get_context($selector, $element); + + // Freeze context. + $context->set_locked(false); + } } From 17ee80ca64ad3e18f3f6f3cc8ea968b5e20437d7 Mon Sep 17 00:00:00 2001 From: Ferran Recio Date: Tue, 16 Feb 2021 17:09:31 +0100 Subject: [PATCH 3/3] 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.