fix(tracking): key the change baseline by objectid, not by the page-local slot - #125
Merged
Merged
Conversation
…ocal slot captureItemScores() compared each parsed suspend_data entry against a baseline keyed by N, the page-local DOM index of the iDevice. The legacy suspend_data format reuses N across pages, so N is not an identity: a page-2 iDevice landing on a slot whose page-1 occupant happened to have the same score and weight compared equal to it, was treated as "unchanged", and was dropped. Its gradebook column was simply never written, silently, and only for the learner who hit that combination. The baseline is now keyed by objectid, which is the stable identity the server already routes on (DEC-5-01). Two different iDevices are never "unchanged" relative to each other. Keying by objectid means the baseline can no longer be replaced wholesale by the newly parsed page, since that would forget every other page and re-emit its scores on the way back. It now carries entries forward for pages that are not loaded, so returning to a page emits nothing until a score actually changes — the write-avoidance optimisation survives. Entries that do not resolve against the current page's DOM are skipped before the comparison rather than after it, so a stale cross-page entry can no longer be attributed to whatever iDevice happens to sit at that index on the loaded page. Three tests, all red against the tracker on main: × routes a newly scored entry by stable objectid × emits a page-2 iDevice landing on a slot whose page-1 occupant scored identically × does not re-emit unchanged scores when the learner returns to a page The first two are mirror images and together specify the attribution rule: a collided slot with an identical score must still be graded, and a stale entry must not be attributed to an iDevice the learner never answered. vitest 39/39 with the fix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
============================================
+ Coverage 92.31% 92.32% +0.01%
Complexity 824 824
============================================
Files 51 51
Lines 3734 3740 +6
============================================
+ Hits 3447 3453 +6
Misses 287 287
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
erseco
added a commit
that referenced
this pull request
Aug 21, 2026
…time writes
The vendored runtime this PR replaces is only half the story. The plugin
supplies its copy ONLY to packages that do not carry one —
package_manager.php:254, `if ($present) { continue; }` — and eXeLearning's
SCORM exporter DOES carry one: Scorm12Exporter.spec.ts asserts the
assembled file ships all five layers, client, activities, policy,
lifecycle, adapter. The activities layer is the one that writes
cmi.suspend_data as `exe12/1|{record}|{record}…`.
So the moment core#2209 ships, every SCORM export uploaded here arrives
with a runtime writing a payload neither of this plugin's parsers can
read — `grep -c exe12` was 0 in both classes/local/track.php and
js/scorm_tracker.js — and every per-iDevice grade in those packages is
lost silently. That is caused by this change, not independent of it, so
it belongs in this PR rather than in one of its own.
Both parsers now dispatch on the header and nothing downstream re-sniffs
the payload. One representation serves both formats: every entry carries
title, scorepct and weighted, plus an `objectid` key on — and only on —
an entry that knows its own identity. A versioned record needs no DOM
resolution and cannot collide, so captureItemScores() and the server's
legacy itemnumber fallback both step aside for it; everything else there
exists only because the legacy format cannot name its owner and reuses
the page-local slot N across pages (DEC-5-01).
Version handling is deliberately strict. An unreadable version tag, or
one newer than the parser understands, yields an EMPTY map rather than a
best-effort parse: a future revision may reorder fields, and a silently
misparsed one would publish a WRONG grade — worse than publishing none,
which merely leaves the item visibly ungraded.
Three record shapes are skipped on purpose, each documented where it is
skipped: three-field records (migrated-but-unclaimed legacy entries that
name a page position, not an iDevice), records without the evaluable flag
(the producer excludes them from cmi.core.score.raw, so they must not
reach a gradebook column either), and records whose score field is empty
(no result yet, which is not the same as scoring 0).
Ported from the trace-replay branch rather than copied: that branch
predates #124 and #125, so the exe12 half was lifted onto the current
files and the legacy attribution work it also carries was left there.
filter_registered_scores() is extracted from the inline filter in
ingest(), because the objectid map recovered from suspend_data needs the
same protection against injected objectids.
Verified red without the parser: disabling the dispatch fails all four
new PHP tests and nothing else.
phpunit 344/1287 exit 0 · vitest 33/33 · phpcs 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted from #105, which had bolted it onto the SCORM library replacement. It is a bug in
main, independent of that swap and of the xAPI retirement in #124, so it lands on its own.The defect
captureItemScores()decides which per-iDevice scores to POST by comparing the parsedcmi.suspend_dataagainst a baseline. That baseline was keyed by N — the page-local DOM index of the iDevice.N is not an identity. The legacy
suspend_dataformat restarts it on every page, so slot 2 on page 1 and slot 2 on page 2 are different iDevices wearing the same key. When a page-2 iDevice landed on a slot whose page-1 occupant happened to have the same score and the same weight, it compared equal to the baseline, was treated as unchanged, and was dropped.The learner answered it. The server never heard about it. Its gradebook column was simply never written — silently, with no error, and only for the learners who hit that combination.
The fix
The baseline is keyed by
objectid, the stable identity the server already routes on (DEC-5-01). Two different iDevices are never "unchanged" relative to each other.That change forces a second one. An objectid-keyed baseline can no longer be replaced wholesale by the newly parsed page — that would forget every other page and re-emit its scores as soon as the learner navigated back. It now carries forward the entries for pages that are not loaded, so returning to a page emits nothing until a score actually changes. The write-avoidance optimisation survives.
Entries that do not resolve against the current page's DOM are now skipped before the comparison instead of after it, so a stale cross-page entry can no longer be attributed to whatever iDevice happens to occupy that index on the loaded page.
Tests
Three, all red against the tracker on
main:The first two are mirror images, and together they are the attribution rule: a collided slot with an identical score must still be graded, and a stale entry must not be attributed to an iDevice the learner never answered. A tracker can pass either one alone by being naive in opposite directions; only the pair pins the behaviour.
Scope
js/scorm_tracker.js, its Vitest spec, and the decision recordDEC-125-01. No PHP, no schema, no upgrade step, no user-visible strings.Moodle Playground Preview
The changes in this pull request can be previewed and tested using a Moodle Playground instance.
ℹ️ The eXeLearning editor is fetched from the shared release and unpacked into the plugin when the playground boots, so the first load may take a few extra seconds. ELPX upload, viewer and preview work normally.