Repository navigation
Conversation
AttendanceSession.date_modified was declared with a creation-time default and no auto-update, and nothing ever assigned it, so it reported the creation time no matter how often the session's attendance was edited. Every other modified timestamp in the tree is kept current, either with auto_now or by explicit assignment. Set it in the serializer's update rather than with auto_now on the model: deserialization saves through the same path, so auto_now would overwrite a value authored on another device with the receiving device's clock. Fixes learningequality#15670
github-actions
Bot
added
DEV: backend
|
👋 Hi @namanjain24-sudo, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
Confirming both checklist items:
|
Build Artifacts
Smoke test screenshot |
Setting date_modified in the session serializer's update did not cover bulk_update, which edits a session's records through its own endpoint and left the session's timestamp at its creation value.
Both attendance fixes add a test immediately before test_bulk_update_creates_and_updates_records, which made them conflict textually despite touching unrelated code. Move this one after it.
|
Pushed two follow-up commits. The first is a gap in my own fix that I'd rather surface than leave for review: setting The second is housekeeping. Both attendance branches were inserting a new test immediately before
|
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesOutcome: reviewed Last updated: 2026-10-28 21:41 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15671: correct fix. Both write paths move date_modified. Morango's save() adds the dirty bit to update_fields, so the narrow save still syncs.
CI passing.
- suggestion: the new
assertGreaterchecks can tie on Windows CI (inline). - nitpick: one comment restates its code (inline).
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Confirm that also moving the session's
date_modified(a synced field) from the bulk record endpoint, which the issue didn't name, is wanted.
Written by rtibblesbot, an LLM-based coding agent.
Both tests compared two local_now() values one request apart. The Windows clock ticks every ~15.6ms, so they can tie and the assertGreater fail intermittently. Backdate the session a minute and compare against that.
|
Both addressed, thanks — the tie risk was real and I had waved it away on the grounds that Windows CI happened to pass, which is not the same thing as it being safe. suggestion — nitpick — the comment at
|
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15671: all 3 prior findings resolved. No new findings. CI pending.
Prior-finding status
RESOLVED — kolibri/core/attendance/test/test_api.py:326 — assertGreater tie risk on Windows py3.8
RESOLVED — kolibri/core/attendance/test/test_api.py:615 — same tie risk in bulk-update test
RESOLVED — kolibri/core/attendance/viewsets/attendance_record.py — comment restates the next two lines
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
Written by rtibblesbot, an LLM-based coding agent.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
The fix works; the timestamp should be maintained by the model rather than by each write path.
- blocking: move the bump into
AttendanceSession.pre_save(inline). - blocking: add a model test that a session saved with
update_dirty_bit_to=Falsekeeps a backdateddate_modified. That is the property ruling outauto_now, and nothing tests it. - Keep both API tests as they are.
CI passing.
Every new writer had to remember to bump it, and the bulk endpoint already needed a second copy. AttendanceSession.pre_save does it instead, skipping the bump when update_dirty_bit_to is False, which is how deserialization saves -- so a synced-in value is kept rather than overwritten with the receiving device's clock, the property that ruled out auto_now. Both write paths drop their own assignment, and a model test covers the deserialization case, which nothing exercised before.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15671: CI lint fails on the new model test; fix it per the inline comment. All 3 prior findings are resolved.
Prior-finding status
RESOLVED — kolibri/core/attendance/test/test_api.py:326 — assertGreater tie risk on Windows py3.8
RESOLVED — kolibri/core/attendance/test/test_api.py:615 — same tie risk; backdate before the request
RESOLVED — kolibri/core/attendance/viewsets/attendance_record.py — comment restates the next two lines
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Confirm bumping
date_modifiedinAttendanceSession.pre_save(skipped only whenupdate_dirty_bit_to is False) is the right rule for every local save path, including ones that don't change attendance content.
Written by rtibblesbot, an LLM-based coding agent.
|
Both done, thank you — the model is the right home for it and I had not considered that every future writer inherits the problem otherwise. blocking — bump in blocking — model test for the deserialization case. Added Both API tests kept as they are.
|
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15671: all 4 prior findings are resolved. CI is still running.
Prior-finding status
RESOLVED — kolibri/core/attendance/test/test_api.py:326 — assertGreater tie risk on Windows py3.8
RESOLVED — kolibri/core/attendance/test/test_api.py:615 — same tie risk; backdate date_modified
RESOLVED — kolibri/core/attendance/viewsets/attendance_record.py — comment restates the code
RESOLVED — kolibri/core/attendance/test/test_models.py — ruff-format failure
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Confirm bumping
AttendanceSession.date_modifiedinpre_saveon every dirty save (skipped only whenupdate_dirty_bit_to=False) suits sync and any consumers ordering by it.
Written by rtibblesbot, an LLM-based coding agent.
Superseded by my review of b83a578: no blocking findings remain.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Excellent, well contained now! Thanks @namanjain24-sudo!
|
Quick check on rtibblesbot's review of this PR: react 👍 helpful · 😕 mixed · 👎 not helpful. Reply with anything specific. |
Summary
AttendanceSession.date_modifiedis published through the API but never moves. It is declared with a creation-time default and no auto-update, and nothing assigns it anywhere else:The serializer also lists it in
read_only_fields, so a client cannot set it andupdatenever receives it invalidated_data. The result is that editing a session — including flipping a learner'spresentflag through the nestedattendance_recordswrite path — leavesdate_modifiedreporting the creation time.Other modified/updated timestamps in the tree are kept current, either with
auto_now(device/models.py:517and:629,discovery/models.py:102) or by explicit assignment (channel.last_updated = local_now()inchannel_import.py:996). This one is neither.Correction: an earlier version of this description said it was the only such field that is neither. That was wrong —
Job.time_updated(kolibri/core/tasks/models.py:32) is never written either, as @rtibblesbot pointed out on #15670. I've verified that and corrected the claim here.References
Fixes #15670
Reviewer guidance
pytest kolibri/core/attendance/test/test_api.py -k date_modifiedis the regression test. Againstdevelopit fails with the two timestamps byte-for-byte equal; it passes here.pytest kolibri/core/attendance/— 60 passed.prek run --files …clean.auto_nowon the model. Morango deserialization saves through the same path (app_model.save(update_dirty_bit_to=False),morango/sync/operations.py:454), so anauto_nowfield would be rewritten with the receiving device's clock every time a session synced in, discarding the value the authoring device sent. Setting it inupdateconfines the change to genuine API edits. I verified this rather than assuming it: a session given adate_modifiedthree days in the past and then saved the way deserialization saves keeps that value unchanged on this branch.channel.last_updated— the otherDateTimeTzFieldof this kind — is handled.AI usage
Claude Code (on my instruction to keep scanning the newer code) spotted that
date_modifiedhad no auto-update while reading the attendance models for an unrelated issue, and I had it confirm the behaviour before anything was written. It drove the real API as a coach to show the value unchanged after aPATCHthat genuinely flipped the attendance, surveyed how every other modified timestamp in the tree is maintained — which is what makes this inconsistent rather than deliberate — and traced morango's deserialization save path to establish thatauto_nowwould be the wrong fix here. It then worked test-first, with the regression test confirmed red before the change, and afterwards verified empirically that a synced-in value survives a deserialization-style save. I reviewed the survey of the other timestamp fields, the decision to put the fix in the serializer, and every test run above before opening this PR.