From e86dfcd285f14031a0a142f82ea05cf606623aae Mon Sep 17 00:00:00 2001 From: Laurent David Date: Tue, 2 Sep 2025 14:36:40 +0200 Subject: [PATCH] MDL-86520 mod_scorm: Change the average attempts computation * Make the average calculation uniform with other modules (it is the students who attempted who are counted, not all participants) * Add a decimal point to the rounded value --- .../scorm/classes/courseformat/overview.php | 9 +-- public/mod/scorm/classes/manager.php | 20 +++++ .../tests/courseformat/overview_test.php | 30 +++---- public/mod/scorm/tests/manager_test.php | 80 ++++++++++++++++++- 4 files changed, 115 insertions(+), 24 deletions(-) diff --git a/public/mod/scorm/classes/courseformat/overview.php b/public/mod/scorm/classes/courseformat/overview.php index ea60e0a4d39..c87c65269f9 100644 --- a/public/mod/scorm/classes/courseformat/overview.php +++ b/public/mod/scorm/classes/courseformat/overview.php @@ -150,14 +150,15 @@ class overview extends \core_courseformat\activityoverviewbase { return null; } $groups = array_map(fn($group) => $group->id, $this->get_groups_for_filtering()); - $totalattempts = $this->manager->count_users_who_attempted($groups); - $userswhocanattempt = $this->manager->count_participants($groups); $maxattempts = $this->manager->get_max_attempts(); if ($maxattempts === 0) { $maxattemptstext = get_string('unlimited'); } else { $maxattemptstext = (string) $maxattempts; } + $totalattempts = $this->manager->count_all_attempts($groups); + $attemptedusers = $this->manager->count_users_who_attempted($groups); + $averageattempts = $totalattempts ? round($totalattempts / $attemptedusers, 1) : 0; $content = new overviewdialog( buttoncontent: $totalattempts, @@ -171,10 +172,6 @@ class overview extends \core_courseformat\activityoverviewbase { ); $content->add_item(get_string('allowedattemptsstudent', 'mod_scorm'), $maxattemptstext); - $averageattempts = 0; - if ($userswhocanattempt > 0) { - $averageattempts = (int) round($totalattempts / $userswhocanattempt); - } $content->add_item(get_string('averageattemptperstudent', 'mod_scorm'), $averageattempts); return new overviewitem( diff --git a/public/mod/scorm/classes/manager.php b/public/mod/scorm/classes/manager.php index 132ec1ae849..da9f2cb99d3 100644 --- a/public/mod/scorm/classes/manager.php +++ b/public/mod/scorm/classes/manager.php @@ -160,6 +160,26 @@ class manager { return $this->db->count_records_sql($query, $params); } + /** + * Count the total number of attempts for the SCORM activity. + * + * @param array $groupids optional group id array, empty array means no group filtering. + * @return int the total number of attempts for the SCORM activity + */ + public function count_all_attempts(array $groupids = []): int { + $params = ['scormid' => $this->instance->id]; + $joins = ''; + $where = "WHERE a.scormid = :scormid"; + if ($groupids) { + $sqljoin = groups_get_members_join($groupids, 'a.userid', $this->context); + $joins = $sqljoin->joins; + $where .= " AND $sqljoin->wheres"; + $params += $sqljoin->params; + } + $sql = "SELECT COUNT(DISTINCT a.id) FROM {scorm_attempt} a $joins $where"; + return $this->db->count_records_sql($sql, $params); + } + /** * Get the max attempt setting. * diff --git a/public/mod/scorm/tests/courseformat/overview_test.php b/public/mod/scorm/tests/courseformat/overview_test.php index f3057c92c90..1fc1da77b0e 100644 --- a/public/mod/scorm/tests/courseformat/overview_test.php +++ b/public/mod/scorm/tests/courseformat/overview_test.php @@ -261,7 +261,7 @@ final class overview_test extends \advanced_testcase { 'content' => '2 of 4', ], 'totalattempts' => [ - 'value' => 2, + 'value' => 3, 'items' => [ [ 'label' => 'Grading method', @@ -273,12 +273,10 @@ final class overview_test extends \advanced_testcase { ], [ 'label' => 'Average attempts per student', - 'value' => '1', + 'value' => '1.5', ], ], - ], - ], ]; yield 'teacher 1 - no groups without attempts' => [ @@ -320,7 +318,7 @@ final class overview_test extends \advanced_testcase { 'content' => '2 of 4', ], 'totalattempts' => [ - 'value' => 2, + 'value' => 3, 'items' => [ [ 'label' => 'Grading method', @@ -332,7 +330,7 @@ final class overview_test extends \advanced_testcase { ], [ 'label' => 'Average attempts per student', - 'value' => '1', + 'value' => '1.5', ], ], ], @@ -348,7 +346,7 @@ final class overview_test extends \advanced_testcase { 'content' => '1 of 2', // Teacher can also attempt, so s1 and t1 are counted. ], 'totalattempts' => [ - 'value' => 1, // Attempt from s1 only. + 'value' => 2, // Attempt from s1 only. 'items' => [ [ 'label' => 'Grading method', @@ -360,7 +358,7 @@ final class overview_test extends \advanced_testcase { ], [ 'label' => 'Average attempts per student', - 'value' => '1', + 'value' => '2', // Only student 1 in this group attempted twice. ], ], ], @@ -378,7 +376,7 @@ final class overview_test extends \advanced_testcase { 'content' => '2 of 4', ], 'totalattempts' => [ - 'value' => 2, + 'value' => 3, 'items' => [ [ 'label' => 'Grading method', @@ -390,7 +388,7 @@ final class overview_test extends \advanced_testcase { ], [ 'label' => 'Average attempts per student', - 'value' => '1', + 'value' => '1.5', ], ], ], @@ -406,7 +404,7 @@ final class overview_test extends \advanced_testcase { 'content' => '2 of 4', ], 'totalattempts' => [ - 'value' => 2, + 'value' => 3, 'items' => [ [ 'label' => 'Grading method', @@ -418,7 +416,7 @@ final class overview_test extends \advanced_testcase { ], [ 'label' => 'Average attempts per student', - 'value' => '1', + 'value' => '1.5', ], ], ], @@ -489,8 +487,7 @@ final class overview_test extends \advanced_testcase { // attempt for testing purposes. if ($createattempt && $createusers) { $scormgenerator = $this->getDataGenerator()->get_plugin_generator('mod_scorm'); - // Create attempts for the students. - // Two attempts for the first student, one for the second. + // Create an attempt for each student, and two attempts for the student s1. foreach ($data as $username => $userinfo) { $record = [ 'userid' => $users[$username]->id, @@ -500,8 +497,11 @@ final class overview_test extends \advanced_testcase { $record['element'] = 'cmi.core.score.raw'; $record['value'] = $grades[$username]; } - // Create an attempt for each student. + // Create two attempts for the first student. if ($userinfo['role'] === 'student') { + if ($username === 's1') { + $scormgenerator->create_attempt($record); + } $scormgenerator->create_attempt($record); } } diff --git a/public/mod/scorm/tests/manager_test.php b/public/mod/scorm/tests/manager_test.php index da33a2151c8..c6f01fdb782 100644 --- a/public/mod/scorm/tests/manager_test.php +++ b/public/mod/scorm/tests/manager_test.php @@ -98,7 +98,7 @@ final class manager_test extends \advanced_testcase { */ public function test_can_view_reports(): void { $this->resetAfterTest(); - ['users' => $users, 'course' => $course, 'instances' => $instances] = $this->setup_users_and_activity(); + ['users' => $users, 'instances' => $instances] = $this->setup_users_and_activity(); $manager = \mod_scorm\manager::create_from_instance($instances['withattempts']); // Create an attempt for the current user. $this->assertTrue($manager->can_view_reports($users['t1'])); @@ -132,8 +132,7 @@ final class manager_test extends \advanced_testcase { */ public function test_count_users_who_attempted(int $groupmode, string $activity, array $expected): void { $this->resetAfterTest(); - ['users' => $users, 'course' => $course, 'instances' => $instances] = - $this->setup_users_and_activity(groupmode: $groupmode); + ['users' => $users, 'instances' => $instances] = $this->setup_users_and_activity(groupmode: $groupmode); $manager = \mod_scorm\manager::create_from_instance($instances[$activity]); // Check the count of users who attempted. @@ -193,6 +192,80 @@ final class manager_test extends \advanced_testcase { ]; } + /** + * Count the number of attempts for the SCORM activity. + * + * @param int $groupmode the group mode to use for the course. + * @param string $activity the activity name to test. + * @param array $currentgroups + * @param int $expectedcount + * @throws \moodle_exception + */ + #[\PHPUnit\Framework\Attributes\DataProvider('get_count_all_attempts_data')] + public function test_count_all_attempts(int $groupmode, string $activity, array $currentgroups, int $expectedcount): void { + $this->resetAfterTest(); + ['instances' => $instances, 'groups' => $groups] = $this->setup_users_and_activity(groupmode: $groupmode); + $manager = \mod_scorm\manager::create_from_instance($instances[$activity]); + $groupids = array_map( + fn($gname) => $groups[$gname]->id, + $currentgroups + ); + $this->assertEquals( + $expectedcount, + $manager->count_all_attempts($groupids), + ); + } + + /** + * Data provider for participant count tests. + * + * @return \Generator + */ + public static function get_count_all_attempts_data(): \Generator { + yield 'No groups' => [ + 'groupmode' => NOGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => [], + 'expectedcount' => 3, + ]; + yield 'Separate groups, g1 selected' => [ + 'groupmode' => SEPARATEGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => ['g1'], + 'expectedcount' => 2, + ]; + yield 'Separate groups, g2 selected' => [ + 'groupmode' => SEPARATEGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => ['g2'], + 'expectedcount' => 1, + ]; + yield 'Separate groups, g1 and g2 selected' => [ + 'groupmode' => SEPARATEGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => ['g1', 'g2'], + 'expectedcount' => 3, + ]; + yield 'Separate groups, no group selected' => [ + 'groupmode' => SEPARATEGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => [], + 'expectedcount' => 3, // Seems counter-intuitive but with no group selected we count all attempts. + ]; + yield 'Visible groups' => [ + 'groupmode' => VISIBLEGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => ['g1', 'g2'], + 'expectedcount' => 3, + ]; + yield 'Visible groups g1 selected' => [ + 'groupmode' => VISIBLEGROUPS, + 'activity' => 'withattempts', + 'currentgroups' => ['g1'], + 'expectedcount' => 2, + ]; + } + /** * Test if the manager can view reports for a user. * @@ -392,6 +465,7 @@ final class manager_test extends \advanced_testcase { 'users' => $users, 'course' => $course, 'instances' => $instances, + 'groups' => $groups, ]; } }