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

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(

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

public/mod/scorm/tests/courseformat/overview_test.php

@@ -261,7 +261,7 @@ final class overview_test extends \advanced_testcase {
                     'content' => '<strong>2</strong> 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' => '<strong>2</strong> 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' => '<strong>1</strong> 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' => '<strong>2</strong> 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' => '<strong>2</strong> 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);
                 }
             }

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,
         ];
     }
 }