From eee060a425aa19796948a2a1c9177e7dcd8cd87a Mon Sep 17 00:00:00 2001 From: greiser Date: Wed, 29 Jul 2026 18:04:45 +0200 Subject: [PATCH] fix: only sync grades for the user actually opening a learning progress page Grade sync used to run unconditionally on every course page view (ilObjMumieTaskListGUI rendered it for every task in a repository listing) and always synced every course/group member's grade, even when only a single student opened their own learning progress page. - Sync now only runs when a learning progress page is actually opened, and scopes to the single viewing user unless the viewer has read_learning_progress (then all course/group members are synced). - Drop the "very hacky" per-listing-item sync in ilObjMumieTaskListGUI. - getValidAndNewXapiGradesForUser() now returns null instead of an undefined array offset when there's no new xAPI grade for that user since the last sync, and updateGradeForUser() skips the update in that case. Without this, a user revisiting their own learning progress page without a new MUMIE submission had their existing grade silently overwritten with a false "failed, 0%" entry. - Fix deleteLPForTask() comparing usr_id against the task id instead of the actual user id when deleting a single user's marks. - getAllMemberIds() now walks the repository tree to the nearest enclosing Course/Group instead of only checking the direct parent ref, and falls back to users with existing LP data instead of every platform user for tasks outside a Course/Group context. --- classes/class.ilMumieTaskGradeSync.php | 9 ++- classes/class.ilMumieTaskLPStatus.php | 31 +++----- classes/class.ilObjMumieTaskGUI.php | 3 +- classes/class.ilObjMumieTaskListGUI.php | 8 --- .../class.ilMumieTaskParticipantService.php | 40 ++++++++--- test/bootstrap.php | 1 + test/ilMumieTaskGradeSyncTest.php | 71 +++++++++++++++++++ test/ilMumieTaskSuite.php | 1 + 8 files changed, 120 insertions(+), 44 deletions(-) create mode 100644 test/ilMumieTaskGradeSyncTest.php diff --git a/classes/class.ilMumieTaskGradeSync.php b/classes/class.ilMumieTaskGradeSync.php index f0b76fd..7dffb5f 100644 --- a/classes/class.ilMumieTaskGradeSync.php +++ b/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; } /** diff --git a/classes/class.ilMumieTaskLPStatus.php b/classes/class.ilMumieTaskLPStatus.php index 278b68f..dfabc2b 100644 --- a/classes/class.ilMumieTaskLPStatus.php +++ b/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); } diff --git a/classes/class.ilObjMumieTaskGUI.php b/classes/class.ilObjMumieTaskGUI.php index 1531cdf..4891a86 100644 --- a/classes/class.ilObjMumieTaskGUI.php +++ b/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']); } diff --git a/classes/class.ilObjMumieTaskListGUI.php b/classes/class.ilObjMumieTaskListGUI.php index 0b96292..1401e25 100644 --- a/classes/class.ilObjMumieTaskListGUI.php +++ b/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', diff --git a/classes/users/class.ilMumieTaskParticipantService.php b/classes/users/class.ilMumieTaskParticipantService.php index 9f6dc9c..739f262 100644 --- a/classes/users/class.ilMumieTaskParticipantService.php +++ b/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; } } diff --git a/test/bootstrap.php b/test/bootstrap.php index d8e1d25..b44b650 100644 --- a/test/bootstrap.php +++ b/test/bootstrap.php @@ -28,3 +28,4 @@ } require_once __DIR__ . '/../classes/class.ilObjMumieTask.php'; +require_once __DIR__ . '/../classes/class.ilMumieTaskGradeSync.php'; diff --git a/test/ilMumieTaskGradeSyncTest.php b/test/ilMumieTaskGradeSyncTest.php new file mode 100644 index 0000000..43bca12 --- /dev/null +++ b/test/ilMumieTaskGradeSyncTest.php @@ -0,0 +1,71 @@ +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); + } +} diff --git a/test/ilMumieTaskSuite.php b/test/ilMumieTaskSuite.php index dd5a83d..405addb 100644 --- a/test/ilMumieTaskSuite.php +++ b/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; }