bugfix/#5632 Grade sync fires on every course-page view instead of only on gradebook open

classes/class.ilMumieTaskGradeSync.php

@@ -18,12 +18,12 @@ class ilMumieTaskGradeSync
     private $admin_settings;
     private $force_update;
 
-    public function __construct($task, $force_update)
+    public function __construct($task, $force_update, ?array $user_ids = null)
     {
         $this->admin_settings = ilMumieTaskAdminSettings::getInstance();
         $this->task = $task;
         $this->force_update = $force_update;
-        $this->user_ids = ilMumieTaskParticipantService::getAllMemberIds($task);
+        $this->user_ids = $user_ids ?? ilMumieTaskParticipantService::getAllMemberIds($task);
     }
 
     public function getSyncIdForUser($user_id)
@@ -165,11 +165,14 @@ public function getValidAndNewXapiGradesByUser()
         return $this->getValidGradeByUser($this->getNewXapiGrades());
     }
 
+    /**
+     * @return null if there is no new xAPI grade for this user since the last sync
+     */
     public function getValidAndNewXapiGradesForUser($user_id)
     {
         $grades_by_user = $this->getValidAndNewXapiGradesByUser();
 
-        return $grades_by_user[$user_id];
+        return $grades_by_user[$user_id] ?? null;
     }
 
     /**

classes/class.ilMumieTaskLPStatus.php

@@ -86,13 +86,16 @@ public static function updateGradeForUser($task, $user_id, $force_update = false
         if (!self::isGradable($task)) {
             return;
         }
-        $grade_sync = new ilMumieTaskGradeSync($task, $force_update);
+        $grade_sync = new ilMumieTaskGradeSync($task, $force_update, [$user_id]);
 
         if ($force_update) {
             self::deleteLPForTask($task, $user_id);
         }
 
         $xapi_grade = $grade_sync->getValidAndNewXapiGradesForUser($user_id);
+        if (null === $xapi_grade) {
+            return;
+        }
         self::upsertXapiGrade($xapi_grade, $task, $user_id);
     }
 
@@ -162,24 +165,6 @@ public static function updateMark($user_id, $task_id, $percentage, $timestamp)
         );
     }
 
-    /**
-     * Update grade for all MumieTasks that are found in a given ilContainer (e.g. Course).
-     *
-     * @param int $refId RefId of the ilContainer
-     */
-    public static function updateGradesForIlContainer($refId)
-    {
-        $mumieTasks = ilMumieTaskLPStatus::getMumieTasksInRepository($refId);
-        foreach ($mumieTasks as $mumieTask) {
-            try {
-                self::updateGrades($mumieTask);
-            } catch (Exception $e) {
-                ilLoggerFactory::getLogger('xmum')->info('Error when updating grades for MUMIE Task: ' . $mumieTask->getId());
-                ilLoggerFactory::getLogger('xmum')->info($e);
-            }
-        }
-    }
-
     /**
      * @return ilObjMumieTask[]
      */
@@ -262,12 +247,16 @@ private static function getLpMark($user_id, ilObjMumieTask $mumie_task): ?array
 
     private static function deleteLPForTask($task, $user_id = 0)
     {
-        ilChangeEvent::_deleteReadEvents($task->getId());
+        if ($user_id > 0) {
+            ilChangeEvent::_deleteReadEventsForUsers($task->getId(), [$user_id]);
+        } else {
+            ilChangeEvent::_deleteReadEvents($task->getId());
+        }
         global $DIC;
         $db = $DIC->database();
         $query = 'DELETE FROM ut_lp_marks WHERE obj_id = ' . $db->quote($task->getId(), 'integer');
         if ($user_id > 0) {
-            $query .= ' AND usr_id = ' . $db->quote($task->getId(), 'integer');
+            $query .= ' AND usr_id = ' . $db->quote($user_id, 'integer');
         }
         $db->manipulate($query);
     }

classes/class.ilObjMumieTaskGUI.php

@@ -351,10 +351,11 @@ public function displayLearningProgress()
         global $DIC;
         $ctrl = $DIC->ctrl();
 
-        ilMumieTaskLPStatus::updateGrades($this->object);
         if ($this->checkPermissionBool('read_learning_progress')) {
+            ilMumieTaskLPStatus::updateGrades($this->object);
             $ctrl->redirectByClass(['ilObjMumieTaskGUI', 'ilLearningProgressGUI', 'ilLPListOfObjectsGUI'], 'showObjectSummary');
         } else {
+            ilMumieTaskLPStatus::updateGradeForUser($this->object, $DIC->user()->getId());
             $this->setProgressInfo();
             $ctrl->redirectByClass(['ilObjMumieTaskGUI', 'ilLearningProgressGUI']);
         }

classes/class.ilObjMumieTaskListGUI.php

@@ -29,14 +29,6 @@ public function getGuiClass(): string
 
     public function initCommands(): array
     {
-        // Very hacky solution to update all grades for MumieTasks that are direct children of an ilContainer (e.g. Course)
-        try {
-            ilMumieTaskLPStatus::updateGradesForIlContainer($_GET['ref_id']);
-        } catch (Exception $e) {
-            ilLoggerFactory::getLogger('xmum')->info('Error when updating MUMIE grades:');
-            ilLoggerFactory::getLogger('xmum')->info($e);
-        }
-
         return [
             [
                 'permission' => 'read',

classes/users/class.ilMumieTaskParticipantService.php

@@ -46,30 +46,48 @@ private static function matchesCaseInsensitive($haystack, $needle)
 
     public static function getAllMemberIds(ilObjMumieTask $mumie_task): array
     {
-        if (self::isInBaseRepository($mumie_task)) {
-            return self::getAllUserIds();
+        $container_ref_id = self::findEnclosingCourseOrGroupRefId($mumie_task->getRefId());
+        if (null === $container_ref_id) {
+            return self::getMemberIdsWithoutCourseContext($mumie_task);
         }
 
-        return ilParticipants::getInstance($mumie_task->getParentRef())->getMembers();
+        return ilParticipants::getInstance($container_ref_id)->getMembers();
     }
 
-    private static function isInBaseRepository(ilObjMumieTask $mumie_task): bool
+    /**
+     * @return null if the task isn't inside a Course or Group (e.g. base repository)
+     */
+    private static function findEnclosingCourseOrGroupRefId(int $ref_id): ?int
     {
-        return 1 == $mumie_task->getParentRef();
+        global $DIC;
+        $tree = $DIC->repositoryTree();
+
+        foreach (array_reverse($tree->getPathFull($ref_id)) as $node) {
+            if (in_array($node['type'], ['crs', 'grp'], true)) {
+                return (int) $node['child'];
+            }
+        }
+
+        return null;
     }
 
-    private static function getAllUserIds(): array
+    /**
+     * Without a Course/Group roster to scope by, fall back to users who already have
+     * LP data for this task (i.e. have opened or submitted it at least once), instead
+     * of syncing every user on the platform.
+     */
+    private static function getMemberIdsWithoutCourseContext(ilObjMumieTask $mumie_task): array
     {
         global $DIC;
         $db = $DIC->database();
         $result = $db->query(
-            'SELECT usr_id FROM usr_data;',
+            'SELECT DISTINCT usr_id FROM ut_lp_marks WHERE obj_id = ' . $db->quote($mumie_task->getId(), 'integer'),
         );
-        $allIds = [];
-        while ($user_id = $db->fetchAssoc($result)) {
-            array_push($allIds, $user_id['usr_id']);
+        $ids = [];
+        while ($row = $db->fetchAssoc($result)) {
+            $ids[] = (int) $row['usr_id'];
         }
 
-        return $allIds;
+        return $ids;
     }
 }

test/bootstrap.php

@@ -28,3 +28,4 @@
 }
 
 require_once __DIR__ . '/../classes/class.ilObjMumieTask.php';
+require_once __DIR__ . '/../classes/class.ilMumieTaskGradeSync.php';

test/ilMumieTaskGradeSyncTest.php

@@ -0,0 +1,71 @@
+<?php
+
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Skips the real constructor (it hits ilMumieTaskAdminSettings::getInstance(), which needs
+ * $DIC/DB) and cans getValidAndNewXapiGradesByUser(), so getValidAndNewXapiGradesForUser()'s
+ * own lookup logic can be exercised in isolation.
+ */
+class ilMumieTaskGradeSyncTestDouble extends ilMumieTaskGradeSync
+{
+    private array $canned_grades_by_user;
+
+    public function __construct(array $canned_grades_by_user)
+    {
+        $this->canned_grades_by_user = $canned_grades_by_user;
+    }
+
+    public function getValidAndNewXapiGradesByUser()
+    {
+        return $this->canned_grades_by_user;
+    }
+}
+
+class ilMumieTaskGradeSyncTest extends TestCase
+{
+    public function testGetValidAndNewXapiGradesForUserReturnsGradeWhenPresent()
+    {
+        $grade = (object) ['result' => (object) ['score' => (object) ['scaled' => 0.8]]];
+        $sync = new ilMumieTaskGradeSyncTestDouble([42 => $grade]);
+
+        $this->assertSame($grade, $sync->getValidAndNewXapiGradesForUser(42));
+    }
+
+    public function testGetValidAndNewXapiGradesForUserReturnsNullWhenUserHasNoNewGrade()
+    {
+        $sync = new ilMumieTaskGradeSyncTestDouble([]);
+
+        $this->assertNullWithoutPhpWarning($sync, 42);
+    }
+
+    public function testGetValidAndNewXapiGradesForUserReturnsNullWhenOnlyOtherUsersHaveNewGrades()
+    {
+        $othersGrade = (object) ['result' => (object) ['score' => (object) ['scaled' => 0.5]]];
+        $sync = new ilMumieTaskGradeSyncTestDouble([7 => $othersGrade]);
+
+        $this->assertNullWithoutPhpWarning($sync, 42);
+    }
+
+    /**
+     * Plain assertNull() alone wouldn't catch a regression to the old `$grades_by_user[$user_id]`
+     * (no `?? null`): PHP resolves a missing array key to null either way, it just also raises a
+     * warning along the way. That warning is the actual bug (it fed into upsertXapiGrade() as if
+     * it were a real, empty grade), so assert on its absence directly instead of relying on a
+     * fail-on-warning PHPUnit setting the plugin's own test run may not enable.
+     */
+    private function assertNullWithoutPhpWarning(ilMumieTaskGradeSync $sync, $user_id): void
+    {
+        $triggered = [];
+        set_error_handler(function (int $errno, string $errstr) use (&$triggered) {
+            $triggered[] = $errstr;
+
+            return true;
+        });
+        $result = $sync->getValidAndNewXapiGradesForUser($user_id);
+        restore_error_handler();
+
+        $this->assertSame([], $triggered, 'Expected no PHP warning/notice, got: ' . implode(', ', $triggered));
+        $this->assertNull($result);
+    }
+}

test/ilMumieTaskSuite.php

@@ -10,6 +10,7 @@ public static function suite()
         $suite->addTestSuite('ilMumieTaskServerTest');
         $suite->addTestSuite('ilObjMumieTaskTest');
         $suite->addTestSuite('ilMumieTaskCryptographyServiceTest');
+        $suite->addTestSuite('ilMumieTaskGradeSyncTest');
 
         return $suite;
     }