diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..c1ecb11 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,25 @@ +# Enforce LF line endings for all text files. +* text=auto eol=lf + +# Binary files — no conversion. +*.png binary +*.jpg binary +*.gif binary +*.ico binary +*.woff binary +*.woff2 binary +*.ttf binary +*.eot binary +*.min.js binary +*.min.css binary + +# Exclude developer artefacts from release archives. +.github/ export-ignore +docs/ export-ignore +tools/ export-ignore +makefile export-ignore +CHANGELOG.md export-ignore +.gitattributes export-ignore +.gitignore export-ignore +.phpcsignore export-ignore +.phpcs.xml export-ignore diff --git a/.gitignore b/.gitignore index f2054e9..d5f059b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,53 @@ -/vue3/node_modules/ -/vue3/coverage +# OS artefacts .DS_Store +Thumbs.db +desktop.ini +*Zone.Identifier ._* + +# IDE +.idea/ +.vscode/ +*.iml +*.iws + +# Node +node_modules/ +npm-debug.log* +yarn-error.log* +package-lock.json +yarn.lock + +# vue +/vue3/node_modules/ +/vue3/coverage + +# PHP / Composer +vendor/ +composer.lock + +# AMD build artefacts (keep src, ignore build output) +amd/build/ + +# moodle-plugin-ci working directories +ci/ +moodle/ + +# PHPUnit / coverage +.phpunit.result.cache +.phpunit.cache/ +coverage/ +*.clover + +# Behat screenshots +behatfaildumps/ + +# Temporary / backup files +*.bak +*.tmp +*.swp +*~ + +# Local developer overrides +.env +.env.local diff --git a/classes/completion.php b/classes/completion.php index 5873119..3a73d0c 100644 --- a/classes/completion.php +++ b/classes/completion.php @@ -26,9 +26,10 @@ declare(strict_types=1); namespace local_adele; + +use core\event\course_completed; use local_adele\helper\user_path_relation; -use context_system; -use local_adele\event\user_path_updated; +use local_adele\learning_path_update; /** * External Service for local adele. @@ -40,35 +41,77 @@ */ class completion { /** - * Observer for course completed + * Observer for the core course_completed event. + * + * When a Moodle course is completed, every active learning path of the + * affected student that references this course in one of its nodes has to be + * re-evaluated so that the corresponding node-completion criterion can turn + * the node into "completed". * - * @param object $event + * Important: on \core\event\course_completed the affected student is carried + * in $event->relateduserid. $event->userid is the ACTOR that caused the + * completion (a grading teacher, an administrator or - most commonly - the + * cron/system process aggregating the completion criteria) and must NOT be + * used to look up the learning path owner. + * + * @param course_completed $event The Moodle course_completed event. + * @return void */ - public static function completed($event) { - // The course_completed event carries the actor in userid and the affected student - // in relateduserid. Route by the student, otherwise a teacher- or cron-triggered - // completion recomputes the wrong user's paths (or none) and the node never - // completes (#495). + public static function completed(course_completed $event): void { + // The student whose course was completed is represented by relateduser, not userid. + $userid = (int) $event->relateduserid; + $courseid = (int) $event->courseid; + + // Nothing sensible to do without both a student and a course. + if ($userid <= 0 || $courseid <= 0) { + return; + } + $userpathrelation = new user_path_relation(); - $learningpaths = $userpathrelation->get_learning_paths($event->relateduserid); - if (!$learningpaths) { + $learningpaths = $userpathrelation->get_learning_paths($userid); + + if (empty($learningpaths)) { return; } + foreach ($learningpaths as $learningpath) { - $learningpath->json = json_decode($learningpath->json, true); - foreach ($learningpath->json['tree']['nodes'] as $node) { - if (is_array($node['data']['course_node_id']) && in_array($event->courseid, $node['data']['course_node_id'])) { - // Recompute the path once per event, even if the course maps to several nodes. - $eventsingle = user_path_updated::create([ - 'objectid' => $learningpath->id, - 'context' => context_system::instance(), - 'other' => [ - 'userpath' => $learningpath, - ], - ]); - $eventsingle->trigger(); - break; + $pathjson = json_decode($learningpath->json, true); + + // A single malformed or incomplete snapshot must not abort the + // processing of the remaining (valid) learning paths. + if ( + !is_array($pathjson) || + empty($pathjson['tree']['nodes']) || + !is_array($pathjson['tree']['nodes']) + ) { + continue; + } + + foreach ($pathjson['tree']['nodes'] as $node) { + $courseids = $node['data']['course_node_id'] ?? []; + + if (!is_array($courseids)) { + $courseids = [$courseids]; } + + // The stored course_node_id values may be strings or integers + // depending on the JSON origin - normalise both sides for a + // type-safe match. + $courseids = array_map('intval', $courseids); + + if (!in_array($courseid, $courseids, true)) { + continue; + } + + // Hand the decoded snapshot to the central update service, which + // re-evaluates every node of the path against the current state. + $learningpath->json = $pathjson; + learning_path_update::trigger_user_path_update($learningpath); + + // The recompute already covers all nodes of this path, so a + // single trigger per learning path is sufficient even if the + // course is referenced by more than one node. + break; } } } diff --git a/classes/course_completion/conditions/course_completed.php b/classes/course_completion/conditions/course_completed.php index b0c1f0a..2ac5381 100644 --- a/classes/course_completion/conditions/course_completed.php +++ b/classes/course_completion/conditions/course_completed.php @@ -163,16 +163,27 @@ public function get_completion_status($node, $userid) { if ($course->enablecompletion) { // Get the course completion instance. $completion = new completion_info($course); - $progress = progress::get_course_progress_percentage($course, $userid) ?? 0; - if ($progress !== null) { + $progress = progress::get_course_progress_percentage($course, $userid); + // Ticket #502: "in progress" starts at the first course access + // (erster Kursaufruf), not at mere enrolment. A learner who is + // only enrolled but has never opened the course is NOT started. + // The timed processing-duration restriction is unaffected; it + // still keys off the node's first unlock. + if ($this->has_accessed_course((int) $course->id, (int) $userid)) { $isinbetween = true; } + $progress ??= 0; // Check if the user has completed the course. $coursecompleted = $completion->is_course_complete($userid); if ($coursecompleted) { $progress = 100; $completed = true; $finished++; + // A completed course cannot have stayed untouched, so it also + // counts as started. This keeps partially completed + // multi-course nodes (some done, not enough yet) in the + // inbetween state. + $isinbetween = true; } $progresses[] = $progress; // Ticket #464 H4: this list is rendered via v-html in the node feedback, so the @@ -248,6 +259,23 @@ public function get_completion_status($node, $userid) { return $coursecompletion; } + /** + * Whether the user has ever accessed the given course (first course view). + * + * Ticket #502: this is the "started" (Bearbeitungsbeginn) criterion for the + * progress display. Moodle records a row in user_lastaccess the first time a + * user accesses a course, so the presence of that row means the course has + * been opened at least once. + * + * @param int $courseid + * @param int $userid + * @return bool + */ + private function has_accessed_course(int $courseid, int $userid) { + global $DB; + return $DB->record_exists('user_lastaccess', ['courseid' => $courseid, 'userid' => $userid]); + } + /** * Get the average of the furthest nodes. * @param array $progresses diff --git a/classes/enrollment.php b/classes/enrollment.php index c0ce561..7d38ef5 100644 --- a/classes/enrollment.php +++ b/classes/enrollment.php @@ -70,7 +70,7 @@ public static function subscribe_user_to_learning_path($learningpath, $params, $ } $userpath = self::buildsqlqueryuserpath($learningpath->id, $params->relateduserid, $courseid); if (!$userpath) { - $id = $DB->insert_record('local_adele_path_user', [ + $newrecord = [ 'user_id' => $params->relateduserid, 'course_id' => $courseid, 'learning_path_id' => $learningpath->id, @@ -86,8 +86,22 @@ public static function subscribe_user_to_learning_path($learningpath, $params, $ 'tree' => $learningpath->json['tree'] ?? ['nodes' => [], 'edges' => []], 'modules' => $learningpath->json['modules'] ?? null, ]), - ]); - $userpath = $DB->get_record('local_adele_path_user', ['id' => $id]); + ]; + try { + $id = $DB->insert_record('local_adele_path_user', $newrecord); + $userpath = $DB->get_record('local_adele_path_user', ['id' => $id]); + } catch (\dml_exception $e) { + // Ticket #501: a concurrent request won the race and inserted + // the row first; the unique index (user_id, course_id, + // learning_path_id) rejected this insert. Reuse the row that + // now exists instead of failing or creating a duplicate. If no + // such row is found the exception was not a duplicate conflict + // and must propagate. + $userpath = self::buildsqlqueryuserpath($learningpath->id, $params->relateduserid, $courseid); + if (!$userpath) { + throw $e; + } + } } $userpath->json = json_decode($userpath->json, true); $eventsingle = user_path_updated::create([ diff --git a/classes/learning_path_update.php b/classes/learning_path_update.php index 9f8e209..8a8d759 100644 --- a/classes/learning_path_update.php +++ b/classes/learning_path_update.php @@ -118,46 +118,96 @@ public static function trigger_user_path_update($userpath) { } /** - * Finished quiz. + * Observer entry point for the mod_quiz attempt_submitted event. * - * @param object $event + * The affected student is carried in $event->relateduserid. $event->userid + * is the acting user (e.g. a teacher submitting on behalf of a student) and + * must NOT be used to look up the learning-path owner. + * + * @param \mod_quiz\event\attempt_submitted $event */ - public static function quiz_finished($event) { - // Get the user path relations. - $userpathrelation = new user_path_relation(); - $records = $userpathrelation->get_learning_paths( - $event->userid, - null, - '"quizid":"' . $event->other['quizid'] . '"' + public static function quiz_finished(\mod_quiz\event\attempt_submitted $event) { + self::recompute_quiz_paths( + (int) $event->relateduserid, + (int) $event->other['quizid'] ); - foreach ($records as $userpath) { - $userpath->json = json_decode($userpath->json, true); - $eventsingle = user_path_updated::create([ - 'objectid' => $userpath->id, - 'context' => context_system::instance(), - 'other' => [ - 'userpath' => $userpath, - ], - ]); - $eventsingle->trigger(); - } } /** - * Finished quiz. + * Recompute every active learning path of a user that references a quiz. + * + * Event-independent so that all quiz change events (submission, regrading, + * manual grading, deletion, reopening) can share the same recompute path. + * + * @param int $userid The affected student. + * @param int $quizid The quiz instance id. + */ + public static function recompute_quiz_paths(int $userid, int $quizid) { + self::recompute_activity_paths($userid, 'modquiz', 'quizid', $quizid); + } + + /** + * Recompute every active learning path of a user that references a CAT quiz. + * + * @param int $userid The affected student. + * @param int $componentid The adaptivequiz instance id. + */ + public static function recompute_catquiz_paths(int $userid, int $componentid) { + self::recompute_activity_paths($userid, 'catquiz', 'componentid', $componentid); + } + + /** + * Finished CAT quiz. * * @param object $event */ public static function catquiz_finished($event) { - // Get the user path relations. $cm = get_coursemodule_from_id(null, $event->contextinstanceid); + if (!$cm) { + return; + } + self::recompute_catquiz_paths((int) $event->userid, (int) $cm->instance); + } + + /** + * Recompute the active learning paths of a user that reference a given + * activity instance in a completion condition. + * + * Ticket #497: the affected paths are found by structurally decoding the + * stored snapshot and comparing ids type-safely, instead of a fragile LIKE + * on the serialized JSON (which only matched the quoted-string form and + * missed integer-serialised ids). Each matching path is recomputed at most + * once; a single malformed snapshot never aborts the remaining paths. + * + * @param int $userid The affected student. + * @param string $conditionlabel The completion condition label ('modquiz'|'catquiz'). + * @param string $valuekey The id key inside data.value ('quizid'|'componentid'). + * @param int $instanceid The activity instance id to match. + */ + private static function recompute_activity_paths( + int $userid, + string $conditionlabel, + string $valuekey, + int $instanceid + ) { + if ($userid <= 0 || $instanceid <= 0) { + return; + } $userpathrelation = new user_path_relation(); - $records = $userpathrelation->get_learning_paths( - $event->userid, - '"componentid":"' . $cm->instance . '"' - ); + $records = $userpathrelation->get_learning_paths($userid); foreach ($records as $userpath) { - $userpath->json = json_decode($userpath->json, true); + $decoded = json_decode($userpath->json, true); + if ( + !is_array($decoded) || + empty($decoded['tree']['nodes']) || + !is_array($decoded['tree']['nodes']) + ) { + continue; + } + if (!self::path_references_activity($decoded['tree']['nodes'], $conditionlabel, $valuekey, $instanceid)) { + continue; + } + $userpath->json = $decoded; $eventsingle = user_path_updated::create([ 'objectid' => $userpath->id, 'context' => context_system::instance(), @@ -169,6 +219,39 @@ public static function catquiz_finished($event) { } } + /** + * Whether any node of a path references the given activity instance in a + * completion condition, comparing ids type-safely (#497). + * + * @param array $nodes The tree nodes of the snapshot. + * @param string $conditionlabel The completion condition label to match. + * @param string $valuekey The id key inside data.value. + * @param int $instanceid The activity instance id to match. + * @return bool + */ + private static function path_references_activity( + array $nodes, + string $conditionlabel, + string $valuekey, + int $instanceid + ) { + foreach ($nodes as $node) { + if (empty($node['completion']['nodes']) || !is_array($node['completion']['nodes'])) { + continue; + } + foreach ($node['completion']['nodes'] as $condition) { + if (($condition['data']['label'] ?? null) !== $conditionlabel) { + continue; + } + $value = $condition['data']['value'][$valuekey] ?? null; + if ($value !== null && (int) $value === $instanceid) { + return true; + } + } + } + return false; + } + /** * Observer for course completed * diff --git a/classes/observer.php b/classes/observer.php index 71f1fc1..e538f26 100755 --- a/classes/observer.php +++ b/classes/observer.php @@ -34,12 +34,12 @@ */ class local_adele_observer { /** - * Observer for the update_catscale event + * Observer for the core course_completed event. * - * @param base $event + * @param \core\event\course_completed $event */ - public static function course_completed(base $event) { - $observer = completion::completed($event); + public static function course_completed(\core\event\course_completed $event) { + completion::completed($event); } /** @@ -98,12 +98,36 @@ public static function node_finished(base $event) { } /** - * Observer for the update_catscale event + * Observer for the mod_quiz attempt_submitted event. * - * @param base $event + * @param \mod_quiz\event\attempt_submitted $event + */ + public static function quiz_attempt_finished(\mod_quiz\event\attempt_submitted $event) { + learning_path_update::quiz_finished($event); + } + + /** + * Observer for quiz attempt change events that can alter a completion result + * after submission: manual grading, regrading, deletion and reopening (#498). + * + * The affected student is carried in relateduserid (the acting user is a + * teacher/cron and must not be used). The quiz id is read from other['quizid'] + * and, if the event does not carry it, derived from the module context. The + * recompute re-evaluates the full current state, so a completion can also be + * revoked (e.g. after a deletion or a regrade below the threshold). + * + * @param \core\event\base $event */ - public static function quiz_attempt_finished(base $event) { - $observer = learning_path_update::quiz_finished($event); + public static function quiz_attempt_changed(base $event) { + $other = $event->other ?? []; + $quizid = isset($other['quizid']) ? (int) $other['quizid'] : 0; + if ($quizid <= 0 && (int) $event->contextlevel === CONTEXT_MODULE) { + $cm = get_coursemodule_from_id('quiz', (int) $event->contextinstanceid, 0, false, IGNORE_MISSING); + if ($cm) { + $quizid = (int) $cm->instance; + } + } + learning_path_update::recompute_quiz_paths((int) $event->relateduserid, $quizid); } /** diff --git a/classes/relation_update.php b/classes/relation_update.php index 641764b..eba7445 100644 --- a/classes/relation_update.php +++ b/classes/relation_update.php @@ -66,6 +66,7 @@ public static function updated_single($event) { $completioncriteria = $completionclass->get_condition_status($node, $userpath->user_id); $restrictioncriteria = $restrictionclass->get_restriction_status($node, $userpath); $restrictionnodepaths = []; + $restrictionnodepathsall = []; $singlerestrictionnode = []; if (isset($node['data']['completion']['master'])) { $userpath->json['user_path_relation'][$node['id']]['master'] = diff --git a/classes/task/reconcile_user_paths.php b/classes/task/reconcile_user_paths.php new file mode 100644 index 0000000..04bd300 --- /dev/null +++ b/classes/task/reconcile_user_paths.php @@ -0,0 +1,134 @@ +. + +/** + * Ad-hoc task that reconciles all active user learning paths. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele\task; + +use local_adele\learning_path_update; + +/** + * Ad-hoc task that re-evaluates every active user learning path exactly once. + * + * This is required after the course_completed observer fix + * (completion::completed used $event->userid - the acting user - instead of + * $event->relateduserid - the affected student), because course_completed does + * not fire again for courses that were already completed + * (course_completions.timecompleted is already set). Existing learning paths can + * therefore still carry a stale node status until they are recomputed once. + * + * The task deliberately reuses the central update service + * (learning_path_update::trigger_user_path_update) instead of duplicating the + * evaluation logic. It is idempotent (a recompute is deterministic), processes + * the records in bounded batches, and logs progress and per-record errors. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class reconcile_user_paths extends \core\task\adhoc_task { + /** + * Number of records processed per batch. + */ + const BATCHSIZE = 200; + + /** + * Human readable name of the task. + * + * @return string + */ + public function get_name() { + return get_string('task_reconcile_user_paths', 'local_adele'); + } + + /** + * Re-evaluate every active user learning path in bounded batches. + * + * The custom data may carry a "lastid" cursor so that the task can be + * re-queued to continue where it left off; this keeps the run repeatable and + * resilient on large installations. + * + * @return void + */ + public function execute() { + global $DB; + + $taskdata = $this->get_custom_data(); + $lastid = (int) ($taskdata->lastid ?? 0); + + $processed = 0; + $failed = 0; + + // Fetch one bounded batch of active snapshots ordered by id, so the + // cursor ("lastid") makes the run resumable and idempotent. + $records = $DB->get_records_select( + 'local_adele_path_user', + 'status = :status AND id > :lastid', + ['status' => 'active', 'lastid' => $lastid], + 'id ASC', + '*', + 0, + self::BATCHSIZE + ); + + if (empty($records)) { + mtrace('local_adele: reconcile_user_paths - nothing left to reconcile.'); + return; + } + + foreach ($records as $record) { + $lastid = (int) $record->id; + try { + $record->json = json_decode($record->json, true); + + // Skip structurally broken snapshots instead of aborting the run. + if (!is_array($record->json) || empty($record->json['tree']['nodes'])) { + mtrace("local_adele: reconcile_user_paths - skipping path_user #{$record->id} (invalid JSON)."); + continue; + } + + learning_path_update::trigger_user_path_update($record); + $processed++; + } catch (\Throwable $e) { + $failed++; + mtrace( + "local_adele: reconcile_user_paths - error on path_user #{$record->id}: " . $e->getMessage() + ); + } + } + + mtrace( + "local_adele: reconcile_user_paths - batch done (processed: {$processed}, failed: {$failed}, lastid: {$lastid})." + ); + + // If the batch was full there may be more records: queue a follow-up run + // that continues past the current cursor. + if (count($records) >= self::BATCHSIZE) { + $next = new self(); + $next->set_custom_data(['lastid' => $lastid]); + \core\task\manager::queue_adhoc_task($next, true); + mtrace('local_adele: reconcile_user_paths - queued follow-up batch from id ' . $lastid . '.'); + } else { + mtrace('local_adele: reconcile_user_paths - reconciliation complete.'); + } + } +} diff --git a/db/events.php b/db/events.php index b738e15..796d362 100644 --- a/db/events.php +++ b/db/events.php @@ -47,12 +47,24 @@ 'callback' => 'local_adele_observer::node_finished', ], [ - 'eventname' => '\mod_quiz\event\attempt_finished', + 'eventname' => '\mod_quiz\event\attempt_submitted', 'callback' => 'local_adele_observer::quiz_attempt_finished', ], [ - 'eventname' => '\mod_quiz\event\attempt_reviewed', - 'callback' => 'local_adele_observer::quiz_attempt_finished', + 'eventname' => '\mod_quiz\event\attempt_manual_grading_completed', + 'callback' => 'local_adele_observer::quiz_attempt_changed', + ], + [ + 'eventname' => '\mod_quiz\event\attempt_regraded', + 'callback' => 'local_adele_observer::quiz_attempt_changed', + ], + [ + 'eventname' => '\mod_quiz\event\attempt_deleted', + 'callback' => 'local_adele_observer::quiz_attempt_changed', + ], + [ + 'eventname' => '\mod_quiz\event\attempt_reopened', + 'callback' => 'local_adele_observer::quiz_attempt_changed', ], [ 'eventname' => '\mod_adaptivequiz\event\attempt_completed', diff --git a/db/install.xml b/db/install.xml index 42be610..c3ad8cb 100644 --- a/db/install.xml +++ b/db/install.xml @@ -1,5 +1,5 @@ - @@ -40,6 +40,9 @@ + + + diff --git a/db/upgrade.php b/db/upgrade.php index e5d3db2..4ab90f7 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -209,6 +209,7 @@ function xmldb_local_adele_upgrade($oldversion) { // Adele savepoint reached. upgrade_plugin_savepoint(true, 2026061800, 'local', 'adele'); } + if ($oldversion < 2026071500) { // Ticket #482: the local/adele:teacheredit capability gained the manager // archetype so a course Manager operates a learning path like an editing @@ -223,5 +224,50 @@ function xmldb_local_adele_upgrade($oldversion) { // Adele savepoint reached. upgrade_plugin_savepoint(true, 2026071500, 'local', 'adele'); } + if ($oldversion < 2026072200) { + // The course_completed observer now resolves the affected student via + // relateduserid instead of the acting user (userid). Courses that were + // already completed before this fix do not re-fire course_completed, so + // their learning paths can still carry a stale node status. Queue a + // one-off reconciliation that recomputes every active learning path. + $reconcile = new \local_adele\task\reconcile_user_paths(); + \core\task\manager::queue_adhoc_task($reconcile, true); + + upgrade_plugin_savepoint(true, 2026072200, 'local', 'adele'); + + // Ticket #501: guarantee at most one active user-path relation per + // (user_id, course_id, learning_path_id). First remove pre-existing + // duplicates created by the historical check-then-insert race, keeping + // the most recently created row (highest id). buildsqlqueryuserpath() + // reads with ORDER BY id DESC, so the highest-id row is the one every + // read/update path already targets and therefore carries the up-to-date + // progress; the orphaned lower-id copies were never updated after + // creation, so nothing is lost. The nested derived table keeps the + // statement portable across PostgreSQL and MariaDB/MySQL. + $DB->execute(" + DELETE FROM {local_adele_path_user} + WHERE id NOT IN ( + SELECT keepid FROM ( + SELECT MAX(id) AS keepid + FROM {local_adele_path_user} + GROUP BY user_id, course_id, learning_path_id + ) keptrows + ) + "); + + // With the duplicates removed, the unique index can be created. + $table = new xmldb_table('local_adele_path_user'); + $index = new xmldb_index( + 'useridcourseidlpid', + XMLDB_INDEX_UNIQUE, + ['user_id', 'course_id', 'learning_path_id'] + ); + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + upgrade_plugin_savepoint(true, 2026072203, 'local', 'adele'); + } + return true; } diff --git a/lang/de/local_adele.php b/lang/de/local_adele.php index f3a6a11..ed4ef0d 100755 --- a/lang/de/local_adele.php +++ b/lang/de/local_adele.php @@ -486,6 +486,7 @@ $string['tagsinclude'] = 'Eingeschlossene Tags definieren'; $string['tagsinclude_desc'] = 'Definieren Sie, welche Kurse basierend auf ihren Tags gefiltert werden sollen. Kurse mit einem dieser Tags sind auffindbar.'; $string['task_check_timed_restrictions'] = 'Zeitgesteuerte Lernpfad-Voraussetzungen neu auswerten'; +$string['task_reconcile_user_paths'] = 'Aktive Lernpfade abgleichen (Node-Abschluss neu berechnen)'; $string['timed_duration_incomplete_modal'] = 'Bitte definieren Sie eine gültige Dauer und Zeiteinheit für das Zugangskriterium „Bearbeitungszeitraum“, bevor Sie speichern.'; $string['timed_incomplete_modal'] = 'Bitte geben Sie mindestens einen Start- oder Endzeitpunkt für das Zugangskriterium „Start- und Endzeitpunkt“ an, bevor Sie speichern.'; $string['title_change_visibility'] = 'Sichtbarkeit geändert'; diff --git a/lang/en/local_adele.php b/lang/en/local_adele.php index 3770536..b97fe5c 100644 --- a/lang/en/local_adele.php +++ b/lang/en/local_adele.php @@ -483,6 +483,7 @@ $string['tagsinclude'] = 'Define included tags'; $string['tagsinclude_desc'] = 'Define which courses should be filtered based on their tags. Courses with one of these tags will be found.'; $string['task_check_timed_restrictions'] = 'Re-evaluate timed learning-path restrictions'; +$string['task_reconcile_user_paths'] = 'Reconcile active learning paths (recompute node completion)'; $string['timed_duration_incomplete_modal'] = 'Please define a valid duration and time unit for the "Editing period" access criterion before saving.'; $string['timed_incomplete_modal'] = 'Please enter at least a start or end time for the "Start and end time" access criterion before saving.'; $string['title_change_visibility'] = 'Visibility changed'; diff --git a/tests/adele_learningpath_testcase.php b/tests/adele_learningpath_testcase.php index 98e0785..8e12c2c 100644 --- a/tests/adele_learningpath_testcase.php +++ b/tests/adele_learningpath_testcase.php @@ -221,4 +221,23 @@ protected function mark_course_complete_in_db(int $courseid, int $userid): void $cache = \cache::make('core', 'coursecompletion'); $cache->delete($userid . '_' . $courseid); } + + /** + * Record a user_lastaccess row so that the course counts as accessed at + * least once (erster Kursaufruf), which is the "started"/inbetween criterion + * of the course_completed condition (#502). + * + * @param int $courseid + * @param int $userid + */ + protected function mark_course_accessed_in_db(int $courseid, int $userid): void { + global $DB; + if (!$DB->record_exists('user_lastaccess', ['courseid' => $courseid, 'userid' => $userid])) { + $DB->insert_record('user_lastaccess', (object)[ + 'userid' => $userid, + 'courseid' => $courseid, + 'timeaccess' => time(), + ]); + } + } } diff --git a/tests/behat/adele_crossstack_flows.feature b/tests/behat/adele_crossstack_flows.feature index a0dc285..df0d0b7 100644 --- a/tests/behat/adele_crossstack_flows.feature +++ b/tests/behat/adele_crossstack_flows.feature @@ -79,6 +79,35 @@ Feature: High-value cross-stack Adele flows that unit tests cannot reach Then "[data-id='dndnode_2'] .icon-link i.fa-play" "css_element" should exist And "[data-id='dndnode_2'] .icon-link i.fa-lock" "css_element" should not exist + @javascript + Scenario: A course_completed event aggregated by the system unlocks the gated node + # Regression guard: when the course completion is aggregated by the system + # (cron/admin) rather than the learner, \core\event\course_completed carries + # userid = admin and relateduserid = learner. The observer must resolve the + # learner via relateduserid; otherwise the gated node would never unlock. + Given the following "local_adele > learningpaths" exist: + | name | description | filepath | courses | image | + | Completion gated LP | Completion desc | local/adele/tests/fixtures/completion_gated_lp.json | C1,C2 | | + And a learning path activity "Adele Completion" for "Completion gated LP" exists in course "C1" + And I log in as "student" + And I am on "Course 1" course homepage + And I follow "Adele Completion" + And I wait until the page is ready + And I wait until "[data-id='dndnode_2']" "css_element" exists + And "[data-id='dndnode_1']" "css_element" should exist + # The gated node is LOCKED (parent not yet complete). + And "[data-id='dndnode_2'] .icon-link i.fa-lock" "css_element" should exist + And "[data-id='dndnode_2'] .icon-link i.fa-play" "css_element" should not exist + # The system aggregates the course completion; the REAL course_completed + # observer chain (userid = admin, relateduserid = learner) recomputes the path. + When the course "C1" is completed and aggregated by the system + And I reload the page + And I wait until the page is ready + And I wait until "[data-id='dndnode_2']" "css_element" exists + # The node is now ACCESSIBLE: the lock is gone and the play button is shown. + Then "[data-id='dndnode_2'] .icon-link i.fa-play" "css_element" should exist + And "[data-id='dndnode_2'] .icon-link i.fa-lock" "css_element" should not exist + @javascript Scenario: Teacher opens the activity and sees the participant progress list # The teacher needs Adele manager access at system context to see the progress diff --git a/tests/behat/behat_local_adele.php b/tests/behat/behat_local_adele.php index 1d5fc55..8b7d4fc 100644 --- a/tests/behat/behat_local_adele.php +++ b/tests/behat/behat_local_adele.php @@ -575,6 +575,92 @@ public function the_course_is_completed_for_every_subscribed_learner(string $sho } } + /** + * Complete a course for every subscribed learner and let the REAL + * \core\event\course_completed observer chain recompute the paths. + * + * Unlike "is completed for every subscribed learner" (which fires + * user_path_updated directly), this step exercises the genuine event chain: + * it records the course completion and then triggers + * \core\event\course_completed with the SYSTEM/admin as the acting user + * (event->userid) while the affected learner is carried in + * event->relateduserid - exactly as Moodle's completion aggregation cron + * does. It therefore guards the fix where the observer must resolve the + * learner via relateduserid rather than the acting user. + * + * @Given /^the course "(?P[^"]+)" is completed and aggregated by the system$/ + * + * @param string $shortname The course shortname whose completion should be recorded. + */ + public function the_course_is_completed_and_aggregated_by_the_system(string $shortname): void { + global $DB, $USER; + + $course = $DB->get_record('course', ['shortname' => $shortname], '*', MUST_EXIST); + $courseid = (int) $course->id; + + $records = $DB->get_records('local_adele_path_user', ['status' => 'active']); + if (empty($records)) { + throw new \RuntimeException('No active learner snapshots found to complete a course for.'); + } + + // Act as the system/admin user, so the triggered course_completed event + // carries userid = admin (the acting process) and NOT the learner. + $olduser = $USER; + \core\session\manager::set_user(get_admin()); + + try { + $cache = \cache::make('core', 'coursecompletion'); + $coursecontext = \context_course::instance($courseid); + + foreach ($records as $record) { + $userid = (int) $record->user_id; + + if (!$DB->record_exists('course_completions', ['course' => $courseid, 'userid' => $userid])) { + $ccid = $DB->insert_record('course_completions', (object) [ + 'course' => $courseid, + 'userid' => $userid, + 'timeenrolled' => time(), + 'timestarted' => time(), + 'timecompleted' => time(), + 'reaggregate' => 0, + ]); + } else { + $DB->set_field( + 'course_completions', + 'timecompleted', + time(), + ['course' => $courseid, 'userid' => $userid] + ); + $ccid = (int) $DB->get_field( + 'course_completions', + 'id', + ['course' => $courseid, 'userid' => $userid], + MUST_EXIST + ); + } + $cache->delete($userid . '_' . $courseid); + + // Fire the genuine core event: the affected learner is the + // relateduserid; the acting user is the admin set above. + $event = \core\event\course_completed::create([ + 'objectid' => $ccid, + 'relateduserid' => $userid, + 'context' => $coursecontext, + 'courseid' => $courseid, + 'other' => ['relateduserid' => $userid], + ]); + ob_start(); + try { + $event->trigger(); + } finally { + ob_end_clean(); + } + } + } finally { + \core\session\manager::set_user($olduser); + } + } + /** * Complete a course for a single learner and fire the REAL \core\event\course_completed * as a DIFFERENT actor (e.g. a teacher who graded them). The event then carries the diff --git a/tests/completion_test.php b/tests/completion_test.php index 04f6c24..3829d92 100644 --- a/tests/completion_test.php +++ b/tests/completion_test.php @@ -25,12 +25,8 @@ namespace local_adele; -use local_adele\helper\user_path_relation; -use local_adele\event\user_path_updated; use local_adele\completion; -use local_adele\relation_update; use advanced_testcase; -use context_system; use PHPUnit\Framework\Attributes\CoversMethod; use PHPUnit\Framework\Attributes\RunInSeparateProcess; @@ -45,7 +41,7 @@ * * @runTestsInSeparateProcesses */ -#[CoversMethod(relation_update::class, 'searchnestedarray')] +#[CoversMethod(completion::class, 'searchnestedarray')] #[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] final class completion_test extends advanced_testcase { /** diff --git a/tests/course_completed_relateduserid_test.php b/tests/course_completed_relateduserid_test.php new file mode 100644 index 0000000..49d35c9 --- /dev/null +++ b/tests/course_completed_relateduserid_test.php @@ -0,0 +1,415 @@ +. + +/** + * Regression tests for local_adele\completion::completed(). + * + * Covers the fix where the course_completed observer resolved the affected + * student via $event->userid (the acting user) instead of + * $event->relateduserid (the student whose course was completed). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; +use context_course; +use context_system; +use core\event\course_completed; +use local_adele\completion; +use local_adele\event\user_path_updated; +use local_adele\relation_update; + +/** + * Regression tests for the course_completed -> relateduserid fix. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \local_adele\completion::completed + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class course_completed_relateduserid_test extends advanced_testcase { + /** + * A minimal, editor-shaped completion structure with a single + * course_completed criterion (min_courses = 1). + * + * @return array + */ + private function course_completed_completion(): array { + return [ + 'edges' => [], + 'nodes' => [ + [ + 'id' => 'condition_1', + 'type' => 'custom', + 'parentCondition' => ['starting_condition'], + 'childCondition' => ['condition_1_feedback'], + 'data' => [ + 'label' => 'course_completed', + 'name' => 'Course completed', + 'value' => ['min_courses' => 1], + ], + ], + [ + 'id' => 'condition_1_feedback', + 'type' => 'feedback', + 'parentCondition' => ['condition_1'], + 'childCondition' => [], + 'data' => [ + 'visibility' => true, + 'feedback_before' => 'do {item}', + 'feedback_after' => 'done {item}', + 'feedback_inbetween' => 'nearly {item}', + 'feedback_inbetween_checkmark' => false, + ], + ], + ], + ]; + } + + /** + * Build a single-node learning-path tree whose starting node references the + * given course and completes on course_completed. + * + * @param mixed $coursenodeid Value stored in data.course_node_id[0] (string or int). + * @return array The decoded json (with a 'tree' key). + */ + private function build_tree($coursenodeid): array { + return [ + 'name' => 'Completion LP', + 'tree' => [ + 'nodes' => [ + [ + 'id' => 'dndnode_1', + 'type' => 'circle', + 'parentCourse' => ['starting_node'], + 'childCourse' => [], + 'data' => [ + 'course_node_id' => [$coursenodeid], + 'fullname' => 'Start node', + 'label' => 'Start', + ], + 'completion' => $this->course_completed_completion(), + 'restriction' => ['nodes' => [], 'edges' => []], + ], + ], + 'edges' => [], + ], + 'modules' => null, + ]; + } + + /** + * Persist a learning path and an active path_user snapshot for a user. + * + * @param int $userid + * @param int $courseid + * @param int $creatorid + * @param array|string $tree Decoded tree array, or a raw (possibly invalid) json string. + * @return int The path_user id. + */ + private function persist_path(int $userid, int $courseid, int $creatorid, $tree): int { + global $DB; + + $treejson = is_string($tree) ? $tree : json_encode(['tree' => $tree['tree'], 'modules' => null]); + + $lpid = (int) $DB->insert_record('local_adele_learning_paths', (object) [ + 'name' => 'Completion LP', + 'json' => is_string($tree) ? $tree : json_encode($tree), + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + ]); + + return (int) $DB->insert_record('local_adele_path_user', [ + 'user_id' => $userid, + 'course_id' => $courseid, + 'learning_path_id' => $lpid, + 'status' => 'active', + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + 'json' => $treejson, + ]); + } + + /** + * Insert a course_completions row and return the real course_completed event + * with a distinct acting user (teacher) and the student as relateduserid. + * + * @param int $courseid + * @param int $studentid + * @param int $actorid The acting user (e.g. grading teacher). + * @return course_completed + */ + private function make_course_completed_event(int $courseid, int $studentid, int $actorid): course_completed { + global $DB; + + $ccid = (int) $DB->insert_record('course_completions', (object) [ + 'course' => $courseid, + 'userid' => $studentid, + 'timeenrolled' => time(), + 'timestarted' => time(), + 'timecompleted' => time(), + 'reaggregate' => 0, + ]); + \cache::make('core', 'coursecompletion')->delete($studentid . '_' . $courseid); + + // The acting user is set as the current user, so $event->userid becomes + // the teacher/system - exactly the mismatch this fix addresses. + $this->setUser($actorid); + + return course_completed::create([ + 'objectid' => $ccid, + 'relateduserid' => $studentid, + 'context' => context_course::instance($courseid), + 'courseid' => $courseid, + 'other' => ['relateduserid' => $studentid], + ]); + } + + /** + * Collect the user_path_updated events (objectids) captured by a sink. + * + * @param \phpunit_event_sink $sink + * @return int[] The objectids (path_user ids) of the fired events. + */ + private function captured_userpath_ids($sink): array { + $ids = []; + foreach ($sink->get_events() as $e) { + if ($e->eventname === '\\local_adele\\event\\user_path_updated') { + $ids[] = (int) $e->objectid; + } + } + return $ids; + } + + /** + * The affected student is resolved via relateduserid (not the acting user), + * the student's path is updated, and the node ends up 'completed'. + * + * @covers \local_adele\completion::completed + * @return void + */ + public function test_completed_uses_relateduserid_not_actor(): void { + global $DB; + $this->resetAfterTest(true); + + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + // Store the course id as a STRING in the node to also cover normalisation. + $tree = $this->build_tree((string) $course->id); + $pathid = $this->persist_path((int) $student->id, (int) $course->id, (int) $teacher->id, $tree); + + $event = $this->make_course_completed_event((int) $course->id, (int) $student->id, (int) $teacher->id); + + // The acting user must be the teacher; the affected student must be relateduserid. + $this->assertNotEquals($student->id, $event->userid); + $this->assertEquals($student->id, $event->relateduserid); + + // Capture the recompute trigger without dispatching it to observers. + $sink = $this->redirectEvents(); + completion::completed($event); + $fired = $this->captured_userpath_ids($sink); + $sink->close(); + + $this->assertSame([$pathid], $fired, 'Exactly the student path must be scheduled for recompute.'); + + // Now drive the real recompute and assert the node becomes completed. + $record = $DB->get_record('local_adele_path_user', ['id' => $pathid]); + $record->json = json_decode($record->json, true); + $recompute = user_path_updated::create([ + 'objectid' => $record->id, + 'context' => context_system::instance(), + 'other' => ['userpath' => $record], + ]); + relation_update::updated_single($recompute); + + $stored = json_decode($DB->get_record('local_adele_path_user', ['id' => $pathid])->json, true); + $this->assertSame( + 'completed', + $stored['user_path_relation']['dndnode_1']['feedback']['status'], + 'The node must be stored as completed once the course is completed.' + ); + } + + /** + * Only the learning path of the relateduserid student is updated; another + * user holding a path for the same course is left untouched. + * + * @covers \local_adele\completion::completed + * @return void + */ + public function test_completed_ignores_other_users_paths(): void { + $this->resetAfterTest(true); + + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $studenta = $gen->create_user(); + $studentb = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($studenta->id, $course->id); + $gen->enrol_user($studentb->id, $course->id); + + $tree = $this->build_tree((int) $course->id); + $patha = $this->persist_path((int) $studenta->id, (int) $course->id, (int) $teacher->id, $tree); + $pathb = $this->persist_path((int) $studentb->id, (int) $course->id, (int) $teacher->id, $tree); + + $event = $this->make_course_completed_event((int) $course->id, (int) $studenta->id, (int) $teacher->id); + + $sink = $this->redirectEvents(); + completion::completed($event); + $fired = $this->captured_userpath_ids($sink); + $sink->close(); + + $this->assertContains($patha, $fired, "Student A's path must be updated."); + $this->assertNotContains($pathb, $fired, "Student B's path must NOT be updated."); + } + + /** + * A course id is matched whether it is stored as a string or as an integer. + * + * @covers \local_adele\completion::completed + * @return void + */ + public function test_completed_matches_string_and_int_course_ids(): void { + $this->resetAfterTest(true); + + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + // Two active paths for the same student: one storing the course id as a + // string, one as an integer. + $pathstring = $this->persist_path( + (int) $student->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((string) $course->id) + ); + $pathint = $this->persist_path( + (int) $student->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((int) $course->id) + ); + + $event = $this->make_course_completed_event((int) $course->id, (int) $student->id, (int) $teacher->id); + + $sink = $this->redirectEvents(); + completion::completed($event); + $fired = $this->captured_userpath_ids($sink); + $sink->close(); + + $this->assertContains($pathstring, $fired, 'A string-typed course_node_id must be matched.'); + $this->assertContains($pathint, $fired, 'An int-typed course_node_id must be matched.'); + } + + /** + * When the same course is referenced by several nodes of one path, only a + * single recompute is triggered for that path. + * + * @covers \local_adele\completion::completed + * @return void + */ + public function test_completed_triggers_single_recompute_per_path(): void { + global $DB; + $this->resetAfterTest(true); + + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + // Build a tree where the SAME course appears in two nodes. + $tree = $this->build_tree((int) $course->id); + $secondnode = $tree['tree']['nodes'][0]; + $secondnode['id'] = 'dndnode_2'; + $tree['tree']['nodes'][] = $secondnode; + + $pathid = $this->persist_path((int) $student->id, (int) $course->id, (int) $teacher->id, $tree); + + $event = $this->make_course_completed_event((int) $course->id, (int) $student->id, (int) $teacher->id); + + $sink = $this->redirectEvents(); + completion::completed($event); + $fired = $this->captured_userpath_ids($sink); + $sink->close(); + + $this->assertSame( + [$pathid], + $fired, + 'A course referenced by multiple nodes must still trigger only one recompute per path.' + ); + } + + /** + * A structurally broken snapshot must not abort processing of other valid + * learning paths of the same student. + * + * @covers \local_adele\completion::completed + * @return void + */ + public function test_completed_survives_invalid_json(): void { + $this->resetAfterTest(true); + + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + // A broken snapshot (not valid learning-path json) ... + $brokenpath = $this->persist_path( + (int) $student->id, + (int) $course->id, + (int) $teacher->id, + 'this-is-not-json' + ); + // ... and a valid one referencing the completed course. + $validpath = $this->persist_path( + (int) $student->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((int) $course->id) + ); + + $event = $this->make_course_completed_event((int) $course->id, (int) $student->id, (int) $teacher->id); + + $sink = $this->redirectEvents(); + completion::completed($event); + $fired = $this->captured_userpath_ids($sink); + $sink->close(); + + $this->assertDebuggingNotCalled(); + $this->assertContains($validpath, $fired, 'The valid path must still be processed.'); + $this->assertNotContains($brokenpath, $fired, 'The broken path must be skipped, not fatal.'); + } +} diff --git a/tests/issue496_quiz_submitted_relateduserid_test.php b/tests/issue496_quiz_submitted_relateduserid_test.php new file mode 100644 index 0000000..8491edb --- /dev/null +++ b/tests/issue496_quiz_submitted_relateduserid_test.php @@ -0,0 +1,294 @@ +. + +/** + * Regression tests for issue #496. + * + * A quiz submission must recompute the learning path of the attempt owner + * (relateduserid), never the acting user (userid), and the observer must be + * wired to the real mod_quiz submission event. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; + +/** + * Quiz submission observer/user regression tests (#496). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class issue496_quiz_submitted_relateduserid_test extends advanced_testcase { + /** + * Build a learning path whose single node completes on a given quiz. + * + * The quiz id is stored as a string so that the current JSON search matches + * it; the string/int robustness of that search is addressed in #497. + * + * @param int $quizid + * @param int $courseid + * @param array $extranodequizids Additional nodes referencing quiz ids. + * @return array + */ + private function build_tree(int $quizid, int $courseid, array $extranodequizids = []): array { + $nodes = []; + $nodes[] = $this->quiz_node('dndnode_1', $quizid, $courseid); + $i = 2; + foreach ($extranodequizids as $extraquizid) { + $nodes[] = $this->quiz_node('dndnode_' . $i, $extraquizid, $courseid); + $i++; + } + return ['name' => 'Quiz LP', 'tree' => ['nodes' => $nodes, 'edges' => []], 'modules' => null]; + } + + /** + * Build a single modquiz completion node. + * + * @param string $id + * @param int $quizid + * @param int $courseid + * @return array + */ + private function quiz_node(string $id, int $quizid, int $courseid): array { + return [ + 'id' => $id, + 'type' => 'circle', + 'parentCourse' => ['starting_node'], + 'childCourse' => [], + 'data' => [ + 'course_node_id' => [(string) $courseid], + 'fullname' => 'Quiz node', + 'label' => $id, + ], + 'completion' => [ + 'nodes' => [ + [ + 'id' => 'condition_1', + 'type' => 'custom', + 'parentCondition' => ['starting_condition'], + 'childCondition' => [], + 'data' => [ + 'label' => 'modquiz', + 'value' => ['quizid' => (string) $quizid, 'grade' => 1], + ], + ], + ], + ], + 'restriction' => ['nodes' => [], 'edges' => []], + ]; + } + + /** + * Persist a learning path plus an active user path, returning the user-path id. + * + * @param int $userid + * @param int $courseid + * @param int $creatorid + * @param array $tree + * @return int + */ + private function persist_path(int $userid, int $courseid, int $creatorid, array $tree): int { + global $DB; + $lpid = (int) $DB->insert_record('local_adele_learning_paths', (object) [ + 'name' => 'Quiz LP', + 'json' => json_encode($tree), + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + ]); + return (int) $DB->insert_record('local_adele_path_user', [ + 'user_id' => $userid, + 'course_id' => $courseid, + 'learning_path_id' => $lpid, + 'status' => 'active', + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + 'json' => json_encode(['tree' => $tree['tree'], 'modules' => null]), + ]); + } + + /** + * Create a mod_quiz attempt_submitted event whose acting user differs from + * the attempt owner. + * + * @param \stdClass $quiz + * @param int $studentid The attempt owner (relateduserid). + * @param int $actorid The acting user (userid), e.g. a teacher. + * @return \mod_quiz\event\attempt_submitted + */ + private function make_submitted_event(\stdClass $quiz, int $studentid, int $actorid): \mod_quiz\event\attempt_submitted { + $this->setUser($actorid); + return \mod_quiz\event\attempt_submitted::create([ + 'objectid' => 1, + 'relateduserid' => $studentid, + 'courseid' => $quiz->course, + 'context' => \context_module::instance($quiz->cmid), + 'other' => ['quizid' => (int) $quiz->id, 'submitterid' => $actorid], + ]); + } + + /** + * Collect the objectids of all user_path_updated events captured by a sink. + * + * @param \core\event\manager|object $sink + * @return array + */ + private function fired_userpath_ids($sink): array { + $ids = []; + foreach ($sink->get_events() as $e) { + if ($e->eventname === '\\local_adele\\event\\user_path_updated') { + $ids[] = (int) $e->objectid; + } + } + return $ids; + } + + /** + * A submission recomputes the attempt owner's path, not the acting user's. + * + * @covers \local_adele\learning_path_update::quiz_finished + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_submission_recomputes_students_path_not_actor(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $quiz = $gen->create_module('quiz', ['course' => $course->id]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + $pathid = $this->persist_path( + (int) $student->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((int) $quiz->id, (int) $course->id) + ); + + $event = $this->make_submitted_event($quiz, (int) $student->id, (int) $teacher->id); + $this->assertNotEquals($student->id, $event->userid, 'The acting user must be the teacher.'); + $this->assertEquals($student->id, $event->relateduserid, 'The attempt owner must be the student.'); + + $sink = $this->redirectEvents(); + learning_path_update::quiz_finished($event); + $fired = $this->fired_userpath_ids($sink); + $sink->close(); + + $this->assertSame([$pathid], $fired, 'Exactly the student path must be recomputed.'); + } + + /** + * Another learner's path is not recomputed by a foreign submission. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_other_users_path_not_recomputed(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $quiz = $gen->create_module('quiz', ['course' => $course->id]); + $studenta = $gen->create_user(); + $studentb = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($studenta->id, $course->id); + $gen->enrol_user($studentb->id, $course->id); + + $patha = $this->persist_path( + (int) $studenta->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((int) $quiz->id, (int) $course->id) + ); + $this->persist_path( + (int) $studentb->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((int) $quiz->id, (int) $course->id) + ); + + $event = $this->make_submitted_event($quiz, (int) $studenta->id, (int) $teacher->id); + + $sink = $this->redirectEvents(); + learning_path_update::quiz_finished($event); + $fired = $this->fired_userpath_ids($sink); + $sink->close(); + + $this->assertSame([$patha], $fired, 'Only the submitting student path must be recomputed.'); + } + + /** + * A quiz used in several nodes triggers at most one recompute per path. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_single_recompute_when_quiz_in_multiple_nodes(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $quiz = $gen->create_module('quiz', ['course' => $course->id]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + // The same quiz appears in two nodes of the same path. + $pathid = $this->persist_path( + (int) $student->id, + (int) $course->id, + (int) $teacher->id, + $this->build_tree((int) $quiz->id, (int) $course->id, [(int) $quiz->id]) + ); + + $event = $this->make_submitted_event($quiz, (int) $student->id, (int) $teacher->id); + + $sink = $this->redirectEvents(); + learning_path_update::quiz_finished($event); + $fired = $this->fired_userpath_ids($sink); + $sink->close(); + + $this->assertSame([$pathid], $fired, 'A path must be recomputed once even if the quiz is used twice.'); + } + + /** + * The observer registration targets attempt_submitted and no longer uses + * the non-existent attempt_finished or the read-only attempt_reviewed. + * + * @coversNothing + * @return void + */ + public function test_quiz_observer_registration(): void { + $observers = []; + include(__DIR__ . '/../db/events.php'); + $eventnames = array_column($observers, 'eventname'); + + $this->assertContains('\\mod_quiz\\event\\attempt_submitted', $eventnames); + $this->assertNotContains('\\mod_quiz\\event\\attempt_reviewed', $eventnames); + $this->assertNotContains('\\mod_quiz\\event\\attempt_finished', $eventnames); + } +} diff --git a/tests/issue497_structured_activity_match_test.php b/tests/issue497_structured_activity_match_test.php new file mode 100644 index 0000000..e7cac93 --- /dev/null +++ b/tests/issue497_structured_activity_match_test.php @@ -0,0 +1,224 @@ +. + +/** + * Regression tests for issue #497. + * + * The event-driven search for learning paths referencing a quiz or CAT instance + * must be structural and type-safe (integer OR string ids), not a fragile LIKE + * on the serialized JSON. A malformed snapshot must not abort the rest. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; + +/** + * Structured activity-matching regression tests (#497). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class issue497_structured_activity_match_test extends advanced_testcase { + /** @var int The learner id used across the test. */ + private int $userid = 0; + + /** + * Create a user to own the paths. + */ + protected function setUp(): void { + parent::setUp(); + $this->resetAfterTest(); + $this->userid = (int) self::getDataGenerator()->create_user()->id; + } + + /** + * Persist a learning path plus an active user path from the given tree nodes. + * + * @param array $treenodes + * @return int The user-path id. + */ + private function persist(array $treenodes): int { + return $this->persist_raw(json_encode(['tree' => ['nodes' => $treenodes], 'modules' => null])); + } + + /** + * Persist an active user path with a raw json body (used to inject malformed json). + * + * @param string $rawjson + * @return int The user-path id. + */ + private function persist_raw(string $rawjson): int { + global $DB; + $lpid = (int) $DB->insert_record('local_adele_learning_paths', (object) [ + 'name' => 'LP', + 'json' => json_encode(['tree' => ['nodes' => []]]), + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $this->userid, + ]); + return (int) $DB->insert_record('local_adele_path_user', [ + 'user_id' => $this->userid, + 'course_id' => 1, + 'learning_path_id' => $lpid, + 'status' => 'active', + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $this->userid, + 'json' => $rawjson, + ]); + } + + /** + * Build a node whose completion condition references an activity instance. + * + * @param string $id + * @param string $label + * @param string $key + * @param mixed $value + * @return array + */ + private function node(string $id, string $label, string $key, $value): array { + return [ + 'id' => $id, + 'data' => ['course_node_id' => ['1'], 'fullname' => 'N'], + 'completion' => [ + 'nodes' => [ + ['id' => 'condition_1', 'data' => ['label' => $label, 'value' => [$key => $value, 'grade' => 1]]], + ], + ], + ]; + } + + /** + * Collect the objectids of captured user_path_updated events. + * + * @param object $sink + * @return array + */ + private function fired($sink): array { + $ids = []; + foreach ($sink->get_events() as $e) { + if ($e->eventname === '\\local_adele\\event\\user_path_updated') { + $ids[] = (int) $e->objectid; + } + } + return $ids; + } + + /** + * An integer-serialised quizid must be matched (the case the old LIKE missed). + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_matches_integer_quizid(): void { + $pathid = $this->persist([$this->node('dndnode_1', 'modquiz', 'quizid', 4242)]); + $sink = $this->redirectEvents(); + learning_path_update::recompute_quiz_paths($this->userid, 4242); + $fired = $this->fired($sink); + $sink->close(); + $this->assertSame([$pathid], $fired, 'Integer-serialised quizid must be matched (#497).'); + } + + /** + * A string-serialised quizid must also be matched. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_matches_string_quizid(): void { + $pathid = $this->persist([$this->node('dndnode_1', 'modquiz', 'quizid', '4242')]); + $sink = $this->redirectEvents(); + learning_path_update::recompute_quiz_paths($this->userid, 4242); + $fired = $this->fired($sink); + $sink->close(); + $this->assertSame([$pathid], $fired, 'String-serialised quizid must be matched (#497).'); + } + + /** + * A different quizid must not match. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_no_match_for_different_quizid(): void { + $this->persist([$this->node('dndnode_1', 'modquiz', 'quizid', 4242)]); + $sink = $this->redirectEvents(); + learning_path_update::recompute_quiz_paths($this->userid, 9999); + $fired = $this->fired($sink); + $sink->close(); + $this->assertSame([], $fired, 'An unrelated quizid must not trigger a recompute.'); + } + + /** + * A malformed snapshot must not abort processing of the remaining paths. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_invalid_json_does_not_abort(): void { + $this->persist_raw('{ this is : not valid json '); + $good = $this->persist([$this->node('dndnode_1', 'modquiz', 'quizid', 4242)]); + $sink = $this->redirectEvents(); + learning_path_update::recompute_quiz_paths($this->userid, 4242); + $fired = $this->fired($sink); + $sink->close(); + $this->assertSame([$good], $fired, 'A malformed snapshot must be skipped, the valid path still recomputed.'); + } + + /** + * A quiz referenced by several nodes triggers one recompute per path. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_single_recompute_when_quiz_in_multiple_nodes(): void { + $pathid = $this->persist([ + $this->node('dndnode_1', 'modquiz', 'quizid', 4242), + $this->node('dndnode_2', 'modquiz', 'quizid', 4242), + ]); + $sink = $this->redirectEvents(); + learning_path_update::recompute_quiz_paths($this->userid, 4242); + $fired = $this->fired($sink); + $sink->close(); + $this->assertSame([$pathid], $fired, 'A path referencing the quiz twice must recompute exactly once.'); + } + + /** + * The CAT quiz componentid is matched type-safely as well. + * + * @covers \local_adele\learning_path_update::recompute_catquiz_paths + * @return void + */ + public function test_catquiz_matches_integer_componentid(): void { + $pathid = $this->persist([$this->node('dndnode_1', 'catquiz', 'componentid', 777)]); + $sink = $this->redirectEvents(); + learning_path_update::recompute_catquiz_paths($this->userid, 777); + $fired = $this->fired($sink); + $sink->close(); + $this->assertSame([$pathid], $fired, 'Integer componentid must be matched for CAT quiz (#497).'); + } +} diff --git a/tests/issue498_quiz_change_events_test.php b/tests/issue498_quiz_change_events_test.php new file mode 100644 index 0000000..4bbcaf2 --- /dev/null +++ b/tests/issue498_quiz_change_events_test.php @@ -0,0 +1,168 @@ +. + +/** + * Regression tests for issue #498. + * + * Post-submission quiz changes (manual grading, regrading, deletion, reopening) + * must trigger a full recompute of the attempt owner's learning paths, resolved + * via relateduserid rather than the acting user. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; + +/** + * Quiz change-event synchronisation regression tests (#498). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class issue498_quiz_change_events_test extends advanced_testcase { + /** + * Persist a learning path plus an active user path with a modquiz node. + * + * @param int $userid + * @param int $quizid + * @param int $courseid + * @param int $creatorid + * @return int The user-path id. + */ + private function persist_path(int $userid, int $quizid, int $courseid, int $creatorid): int { + global $DB; + $tree = ['tree' => ['nodes' => [[ + 'id' => 'dndnode_1', + 'type' => 'circle', + 'parentCourse' => ['starting_node'], + 'childCourse' => [], + 'data' => ['course_node_id' => [(string) $courseid], 'fullname' => 'Quiz node', 'label' => 'q'], + 'completion' => [ + 'nodes' => [ + ['id' => 'condition_1', 'data' => ['label' => 'modquiz', 'value' => ['quizid' => $quizid, 'grade' => 1]]], + ], + ], + 'restriction' => ['nodes' => [], 'edges' => []], + ]]], 'modules' => null]; + $lpid = (int) $DB->insert_record('local_adele_learning_paths', (object) [ + 'name' => 'LP', + 'json' => json_encode($tree), + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + ]); + return (int) $DB->insert_record('local_adele_path_user', [ + 'user_id' => $userid, + 'course_id' => $courseid, + 'learning_path_id' => $lpid, + 'status' => 'active', + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + 'json' => json_encode(['tree' => $tree['tree'], 'modules' => null]), + ]); + } + + /** + * Collect the objectids of captured user_path_updated events. + * + * @param object $sink + * @return array + */ + private function fired($sink): array { + $ids = []; + foreach ($sink->get_events() as $e) { + if ($e->eventname === '\\local_adele\\event\\user_path_updated') { + $ids[] = (int) $e->objectid; + } + } + return $ids; + } + + /** + * The generic quiz-change observer recomputes the attempt owner's path + * (relateduserid), not the acting user's. + * + * @covers \local_adele\learning_path_update::recompute_quiz_paths + * @return void + */ + public function test_quiz_change_recomputes_relateduserid_not_actor(): void { + $this->resetAfterTest(); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $quiz = $gen->create_module('quiz', ['course' => $course->id]); + $student = $gen->create_user(); + $teacher = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + $pathid = $this->persist_path((int) $student->id, (int) $quiz->id, (int) $course->id, (int) $teacher->id); + + // A quiz attempt change carries the affected learner in relateduserid; the + // acting user (teacher) is in userid. attempt_submitted is used here as a + // constructible \core\event\base carrying both fields. + $this->setUser($teacher); + $event = \mod_quiz\event\attempt_submitted::create([ + 'objectid' => 1, + 'relateduserid' => $student->id, + 'courseid' => $quiz->course, + 'context' => \context_module::instance($quiz->cmid), + 'other' => ['quizid' => (int) $quiz->id, 'submitterid' => (int) $teacher->id], + ]); + $this->assertNotEquals($student->id, $event->userid, 'The acting user must be the teacher.'); + + $sink = $this->redirectEvents(); + \local_adele_observer::quiz_attempt_changed($event); + $fired = $this->fired($sink); + $sink->close(); + + $this->assertSame([$pathid], $fired, 'A quiz change must recompute the attempt owner path (relateduserid).'); + } + + /** + * The manual-grading, regrading, deletion and reopening events are all wired + * to the generic quiz-change observer. + * + * @coversNothing + * @return void + */ + public function test_change_events_registration(): void { + $observers = []; + include(__DIR__ . '/../db/events.php'); + $map = []; + foreach ($observers as $observer) { + $map[$observer['eventname']] = $observer['callback']; + } + $expected = [ + '\\mod_quiz\\event\\attempt_manual_grading_completed', + '\\mod_quiz\\event\\attempt_regraded', + '\\mod_quiz\\event\\attempt_deleted', + '\\mod_quiz\\event\\attempt_reopened', + ]; + foreach ($expected as $eventname) { + $this->assertArrayHasKey($eventname, $map, "Event {$eventname} must be registered."); + $this->assertSame('local_adele_observer::quiz_attempt_changed', $map[$eventname]); + } + } +} diff --git a/tests/issue500_restriction_scope_leak_test.php b/tests/issue500_restriction_scope_leak_test.php new file mode 100644 index 0000000..92d260d --- /dev/null +++ b/tests/issue500_restriction_scope_leak_test.php @@ -0,0 +1,232 @@ +. + +/** + * Regression tests for issue #500. + * + * The restriction paths of one node must not leak into the following nodes + * during a full learning-path recompute. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; +use context_system; +use local_adele\event\user_path_updated; + +/** + * Restriction scope-leak regression tests (#500). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class issue500_restriction_scope_leak_test extends advanced_testcase { + /** + * A minimal but valid parent_courses restriction (condition + feedback node). + * + * Mirrors the structure proven by the parent_courses use-case tests. The + * referenced parent id is deliberately dangling; parent_courses treats a + * dangling reference as satisfied (#445), so no debugging is emitted. + * + * @return array + */ + private function parent_courses_restriction(): array { + return [ + 'nodes' => [ + [ + 'id' => 'condition_1', + 'type' => 'custom', + 'parentCondition' => ['starting_condition'], + 'childCondition' => ['condition_1_feedback'], + 'data' => [ + 'label' => 'parent_courses', + 'name' => 'parent', + 'node_id' => 'condition_1', + 'description_before' => 'before {node_name}', + 'visibility' => true, + 'value' => ['min_courses' => 1, 'courses_id' => ['ghost_node']], + ], + ], + [ + 'id' => 'condition_1_feedback', + 'type' => 'feedback', + 'parentCondition' => ['condition_1'], + 'childCondition' => [], + 'data' => [ + 'childCondition' => 'condition_1', + 'visibility' => true, + 'feedback_before' => 'before {node_name}', + 'feedback_before_checkmark' => true, + ], + ], + ], + 'edges' => [], + ]; + } + + /** + * Build a linear tree from a list of [id, courseid, hasrestriction] tuples. + * + * @param array $nodedefs + * @return array + */ + private function build_tree(array $nodedefs): array { + $nodes = []; + $prev = 'starting_node'; + foreach ($nodedefs as $def) { + [$id, $courseid, $hasrestriction] = $def; + $nodes[] = [ + 'id' => $id, + 'type' => 'circle', + 'parentCourse' => [$prev], + 'childCourse' => [], + 'data' => [ + 'course_node_id' => [(string) $courseid], + 'fullname' => 'Node ' . $id, + 'label' => $id, + ], + 'restriction' => $hasrestriction + ? $this->parent_courses_restriction() + : ['nodes' => [], 'edges' => []], + ]; + $prev = $id; + } + $count = count($nodes); + for ($i = 0; $i < $count - 1; $i++) { + $nodes[$i]['childCourse'] = [$nodes[$i + 1]['id']]; + } + return ['name' => 'Leak LP', 'tree' => ['nodes' => $nodes, 'edges' => []], 'modules' => null]; + } + + /** + * Persist a learning path plus an active user path and run updated_single. + * + * @param int $userid + * @param int $courseid + * @param int $creatorid + * @param array $tree + * @return array The decoded user-path json after the recompute. + */ + private function run_recompute(int $userid, int $courseid, int $creatorid, array $tree): array { + global $DB; + $lpid = (int) $DB->insert_record('local_adele_learning_paths', (object) [ + 'name' => 'Leak LP', + 'json' => json_encode($tree), + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + ]); + $userpathid = (int) $DB->insert_record('local_adele_path_user', [ + 'user_id' => $userid, + 'course_id' => $courseid, + 'learning_path_id' => $lpid, + 'status' => 'active', + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $creatorid, + 'json' => json_encode(['tree' => $tree['tree'], 'modules' => null]), + ]); + $userpath = $DB->get_record('local_adele_path_user', ['id' => $userpathid]); + $userpath->json = json_decode($userpath->json, true); + $event = user_path_updated::create([ + 'objectid' => $userpath->id, + 'context' => context_system::instance(), + 'other' => ['userpath' => $userpath], + ]); + relation_update::updated_single($event); + return json_decode($DB->get_record('local_adele_path_user', ['id' => $userpathid])->json, true); + } + + /** + * A restriction on the first node must not leak into a later unrestricted node. + * + * @covers \local_adele\relation_update::updated_single + * @return void + */ + public function test_restriction_paths_do_not_leak_to_following_node(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $creator = $gen->create_user(); + $user = $gen->create_user(); + $ca = $gen->create_course(['enablecompletion' => 1]); + $cb = $gen->create_course(['enablecompletion' => 1]); + $gen->enrol_user($user->id, $ca->id); + $this->setUser($creator); + + $tree = $this->build_tree([ + ['restrnode', (int) $ca->id, true], + ['plainnode', (int) $cb->id, false], + ]); + + $json = $this->run_recompute((int) $user->id, (int) $ca->id, (int) $creator->id, $tree); + + $this->assertNotEmpty( + $json['user_path_relation']['restrnode']['restrictionnode'], + 'The restricted node must carry its own restriction paths.' + ); + $this->assertSame( + [], + $json['user_path_relation']['plainnode']['restrictionnode'], + 'A node without a restriction must not inherit the previous node restriction paths (#500).' + ); + } + + /** + * The leaky variable must not accumulate across three or more nodes. + * + * @covers \local_adele\relation_update::updated_single + * @return void + */ + public function test_no_accumulation_across_three_nodes(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $creator = $gen->create_user(); + $user = $gen->create_user(); + $ca = $gen->create_course(['enablecompletion' => 1]); + $cb = $gen->create_course(['enablecompletion' => 1]); + $cc = $gen->create_course(['enablecompletion' => 1]); + $gen->enrol_user($user->id, $ca->id); + $this->setUser($creator); + + $tree = $this->build_tree([ + ['restrnode', (int) $ca->id, true], + ['plain1', (int) $cb->id, false], + ['plain2', (int) $cc->id, false], + ]); + + $json = $this->run_recompute((int) $user->id, (int) $ca->id, (int) $creator->id, $tree); + + $this->assertSame( + [], + $json['user_path_relation']['plain1']['restrictionnode'], + 'The second node must not inherit restriction paths.' + ); + $this->assertSame( + [], + $json['user_path_relation']['plain2']['restrictionnode'], + 'The third node must not accumulate restriction paths of earlier nodes.' + ); + } +} diff --git a/tests/issue501_unique_active_path_test.php b/tests/issue501_unique_active_path_test.php new file mode 100644 index 0000000..b0b7d39 --- /dev/null +++ b/tests/issue501_unique_active_path_test.php @@ -0,0 +1,123 @@ +. + +/** + * Regression tests for issue #501. + * + * At most one active user-path relation may exist per (user_id, course_id, + * learning_path_id). The database enforces this via a unique index, and the + * subscribe flow reuses an existing snapshot instead of creating a duplicate. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; + +/** + * Unique active user-path relation regression tests (#501). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class issue501_unique_active_path_test extends advanced_testcase { + /** + * Insert an active user-path row for the given triple. + * + * @param int $userid + * @param int $courseid + * @param int $lpid + * @return int + */ + private function insert_path(int $userid, int $courseid, int $lpid): int { + global $DB; + return (int) $DB->insert_record('local_adele_path_user', [ + 'user_id' => $userid, + 'course_id' => $courseid, + 'learning_path_id' => $lpid, + 'status' => 'active', + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $userid, + 'json' => '{"tree":{"nodes":[]}}', + ]); + } + + /** + * The database rejects a second row with the same (user, course, lp) triple. + * + * @coversNothing + * @return void + */ + public function test_unique_index_prevents_duplicate_active_paths(): void { + $this->resetAfterTest(); + $this->insert_path(101, 202, 303); + $this->expectException(\dml_exception::class); + $this->insert_path(101, 202, 303); + } + + /** + * A repeat subscribe reuses the existing active snapshot instead of creating + * a duplicate row. + * + * @covers \local_adele\enrollment::subscribe_user_to_learning_path + * @return void + */ + public function test_subscribe_reuses_existing_active_path(): void { + global $DB; + $this->resetAfterTest(); + $gen = self::getDataGenerator(); + $user = $gen->create_user(); + $course = $gen->create_course(); + + $lpid = (int) $DB->insert_record('local_adele_learning_paths', (object) [ + 'name' => 'LP', + 'json' => json_encode(['tree' => ['nodes' => [], 'edges' => []], 'modules' => null]), + 'timecreated' => time(), + 'timemodified' => time(), + 'createdby' => $user->id, + ]); + + // A snapshot already exists for this triple. + $this->insert_path((int) $user->id, (int) $course->id, $lpid); + + $learningpath = (object) [ + 'id' => $lpid, + 'json' => ['tree' => ['nodes' => [], 'edges' => []], 'modules' => null], + ]; + $params = (object) ['relateduserid' => (int) $user->id, 'userid' => (int) $user->id]; + + $sink = $this->redirectEvents(); + enrollment::subscribe_user_to_learning_path($learningpath, $params, (int) $course->id); + $sink->close(); + + $count = $DB->count_records('local_adele_path_user', [ + 'user_id' => $user->id, + 'course_id' => $course->id, + 'learning_path_id' => $lpid, + 'status' => 'active', + ]); + $this->assertSame(1, $count, 'subscribe must reuse the existing active snapshot, not duplicate it.'); + } +} diff --git a/tests/issue502_progress_inbetween_test.php b/tests/issue502_progress_inbetween_test.php new file mode 100644 index 0000000..564653a --- /dev/null +++ b/tests/issue502_progress_inbetween_test.php @@ -0,0 +1,173 @@ +. + +/** + * Regression tests for issue #502. + * + * The course_completed condition must treat a course as "in progress" + * (inbetween) from the first course access (erster Kursaufruf) onwards, not + * merely because the learner is enrolled. The timed processing-duration + * restriction is a separate mechanism and is not exercised here. + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace local_adele; + +use advanced_testcase; +use local_adele\course_completion\conditions\course_completed; + +/** + * Regression tests for the first-access "in progress" criterion (#502). + * + * @package local_adele + * @copyright 2026 Wunderbyte GmbH + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @runTestsInSeparateProcesses + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class issue502_progress_inbetween_test extends advanced_testcase { + /** + * Build a single course_completed node referencing one course. + * + * @param int $courseid + * @return array + */ + private function build_node(int $courseid): array { + return [ + 'id' => 'dndnode_1', + 'data' => [ + 'course_node_id' => [$courseid], + 'fullname' => 'Node', + ], + 'completion' => [ + 'nodes' => [ + [ + 'id' => 'condition_1', + 'data' => [ + 'label' => 'course_completed', + 'value' => ['min_courses' => 1], + ], + ], + ], + ], + ]; + } + + /** + * Simulate the first course access by recording a user_lastaccess row, as + * Moodle does the first time a user opens a course. + * + * @param int $courseid + * @param int $userid + */ + private function mark_course_accessed(int $courseid, int $userid): void { + global $DB; + $DB->insert_record('user_lastaccess', (object) [ + 'userid' => $userid, + 'courseid' => $courseid, + 'timeaccess' => time(), + ]); + } + + /** + * A learner who is only enrolled but never opened the course is not started. + * + * @covers \local_adele\course_completion\conditions\course_completed::get_completion_status + * @return void + */ + public function test_enrolled_but_never_accessed_is_not_inbetween(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $gen->create_module('page', [ + 'course' => $course->id, + 'completion' => COMPLETION_TRACKING_MANUAL, + ]); + $student = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + + $status = (new course_completed())->get_completion_status( + $this->build_node((int) $course->id), + (int) $student->id + ); + + $this->assertFalse( + $status['inbetween']['condition_1'], + 'Enrolled-but-never-accessed must not be treated as in progress (#502).' + ); + } + + /** + * The first course access marks the node as in progress, even at 0 % progress. + * + * @covers \local_adele\course_completion\conditions\course_completed::get_completion_status + * @return void + */ + public function test_first_course_access_is_inbetween(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + // An activity with completion keeps progress computable, but nothing is done (0 %). + $gen->create_module('page', [ + 'course' => $course->id, + 'completion' => COMPLETION_TRACKING_MANUAL, + ]); + $student = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + $this->mark_course_accessed((int) $course->id, (int) $student->id); + + $status = (new course_completed())->get_completion_status( + $this->build_node((int) $course->id), + (int) $student->id + ); + + $this->assertTrue( + $status['inbetween']['condition_1'], + 'The first course access must mark the node as in progress (#502).' + ); + } + + /** + * The access-based criterion is independent of completion activities: a + * course without any completion activities still counts as started once opened. + * + * @covers \local_adele\course_completion\conditions\course_completed::get_completion_status + * @return void + */ + public function test_access_without_completion_activities_is_inbetween(): void { + $this->resetAfterTest(true); + $gen = self::getDataGenerator(); + $course = $gen->create_course(['enablecompletion' => 1]); + $student = $gen->create_user(); + $gen->enrol_user($student->id, $course->id); + $this->mark_course_accessed((int) $course->id, (int) $student->id); + + $status = (new course_completed())->get_completion_status( + $this->build_node((int) $course->id), + (int) $student->id + ); + + $this->assertTrue( + $status['inbetween']['condition_1'], + 'A course opened at least once counts as in progress regardless of criteria (#502).' + ); + } +} diff --git a/tests/usecases/uc07_course_completed_completion_test.php b/tests/usecases/uc07_course_completed_completion_test.php index b8770e2..43bc653 100644 --- a/tests/usecases/uc07_course_completed_completion_test.php +++ b/tests/usecases/uc07_course_completed_completion_test.php @@ -128,6 +128,11 @@ public function test_completion_is_inbetween_when_enrolled_not_completed(): void global $DB; $this->subscribe_users_to_lp(); + // Ticket #502: "in progress" now requires a first course access, not mere + // enrolment. Simulate the learner having opened dndnode_1's course. + foreach ($DB->get_records('local_adele_path_user') as $accessrecord) { + $this->mark_course_accessed_in_db((int)$this->courseids[0], (int)$accessrecord->user_id); + } $updateevents = $this->get_update_events(); relation_update::updated_single($updateevents[0]); relation_update::updated_single($updateevents[1]); diff --git a/tests/usecases/uc14_course_completed_and_modquiz_completion_test.php b/tests/usecases/uc14_course_completed_and_modquiz_completion_test.php index a9644e9..468f778 100644 --- a/tests/usecases/uc14_course_completed_and_modquiz_completion_test.php +++ b/tests/usecases/uc14_course_completed_and_modquiz_completion_test.php @@ -223,6 +223,10 @@ public function test_and_chain_inbetween_with_no_attempt(): void { global $DB; $this->subscribe_users_to_lp(); + // Ticket #502: inbetween now requires a first course access on dndnode_1's course. + foreach ($DB->get_records('local_adele_path_user') as $accessrecord) { + $this->mark_course_accessed_in_db((int)$this->courseids[0], (int)$accessrecord->user_id); + } $updateevents = $this->get_update_events(); relation_update::updated_single($updateevents[0]); relation_update::updated_single($updateevents[1]); @@ -238,7 +242,7 @@ public function test_and_chain_inbetween_with_no_attempt(): void { $this->assertEquals( 'inbetween', $fb['status_completion'], - "User {$record->user_id}: expected 'inbetween' — enrollment inbetween fires." + "User {$record->user_id}: expected 'inbetween' — course accessed, not yet completed." ); $this->assertEquals( 'accessible', diff --git a/tests/usecases/uc16_or_completion_test.php b/tests/usecases/uc16_or_completion_test.php index 4399f71..1216ca3 100644 --- a/tests/usecases/uc16_or_completion_test.php +++ b/tests/usecases/uc16_or_completion_test.php @@ -183,6 +183,14 @@ private function enrol_complete_and_eval( } } + // Ticket #502: inbetween now requires a first course access. dndnode_2's + // course becomes accessible once dndnode_1 is complete; simulate the + // learner opening it so the pending-completion state is inbetween, not + // before. (For the completed/manual cases the node resolves to 'after'.) + foreach ($DB->get_records('local_adele_path_user') as $accessrecord) { + $this->mark_course_accessed_in_db((int)$this->courseids[2], (int)$accessrecord->user_id); + } + // Pass 2: fire fresh evaluation events from the (possibly updated) records. $freshrecords = $DB->get_records('local_adele_path_user'); foreach ($freshrecords as $freshrecord) { @@ -209,8 +217,8 @@ private function enrol_complete_and_eval( * $completionnodepaths empty → getnodestatusforcompletion does not return 'after'. * * However, course_completed::get_completion_status() sets inbetween=true for the - * condition_1 node because progress::get_course_progress_percentage() receives the - * null-coalescing default (0%), and 0 !== null → $isinbetween=true. Therefore + * condition_1 node because dndnode_2's course has been accessed (user_lastaccess), + * which is the #502 "started" criterion. Therefore * $completioncriteria['course_completed']['inbetween']['condition_1'] = true → * getnodestatusforcompletion returns 'inbetween' (not 'before'). * diff --git a/version.php b/version.php index 7dccff1..a8d870a 100755 --- a/version.php +++ b/version.php @@ -26,7 +26,7 @@ $plugin->component = 'local_adele'; $plugin->release = '0.4.3'; -$plugin->version = 2026072200; +$plugin->version = 2026072204; $plugin->requires = 2022112800; $plugin->maturity = MATURITY_ALPHA; $plugin->supported = [401, 405];