Conversation
Events moved between two calendars of one account before v2.68.0 kept the old calendar's default assignee, and the runtime fix never sees the move again. The existing admin action "Apply to existing appointments" (#1154) now also collects them: the only assignment is the untouched default assignee of another calendar of the same provider/account, the event was never edited in Yuvomi and not pushed out. The untouched rule is one SQL predicate shared with reassignDefaultOnCalendarMove(); moved candidates join the confirmed set and its token (#1171). Closes #1307 Refs #1270
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const MOVED_DEFAULT_EVENTS = ` | ||
| FROM calendar_events e | ||
| JOIN external_calendars ec ON ec.id = e.calendar_ref_id | ||
| JOIN users u ON u.id = ec.default_assignee_user_id | ||
| JOIN event_assignments cur ON cur.event_id = e.id | ||
| WHERE e.external_source = ec.source | ||
| AND e.user_modified = 0 | ||
| AND cur.user_id <> ec.default_assignee_user_id | ||
| AND ${UNTOUCHED_DEFAULT_ASSIGNMENT('cur.user_id')} | ||
| AND EXISTS ( | ||
| SELECT 1 FROM external_calendars eca | ||
| WHERE eca.source = ec.source | ||
| AND eca.id <> ec.id | ||
| AND eca.default_assignee_user_id = cur.user_id | ||
| AND (ec.source <> 'caldav' OR EXISTS ( | ||
| SELECT 1 FROM caldav_calendar_selection sa | ||
| JOIN caldav_calendar_selection sb ON sb.account_id = sa.account_id | ||
| WHERE sa.calendar_url = eca.external_id AND sb.calendar_url = ec.external_id | ||
| )) | ||
| ) | ||
| `; |
There was a problem hiding this comment.
MOVED_DEFAULT_EVENTS has no record of an event ever actually changing calendar_ref_id - it infers "moved from calendar A" purely from the current shape of the data: an untouched single assignment that no longer matches its own calendar's default, plus some other same-account calendar whose default happens to match that assignee.
That heuristic also fires when nothing ever moved: if an admin changes calendar B's own default_assignee_user_id from X to Y via PATCH /external-calendars (server/routes/calendar/google.js:184-229 - an ordinary, repeatable admin action, not setup-only), every untouched event in B that was correctly auto-assigned to B's old default X now satisfies cur.user_id <> ec.default_assignee_user_id (line 177). If any other same-provider (same-account for CalDAV) calendar still defaults to X, the EXISTS at line 183 matches too, and the backfill silently reassigns these never-moved events from X to Y.
user_modified = 0 doesn't guard against this case (comment at lines 157-163 only anticipates a hand assignment mimicking A's default) - these events were never touched by a human, they were just correctly imported under B's own prior default. "Each member's calendar defaults to that member" is exactly the normal household shape this app is built around, so the other-calendar-with-matching-default precondition is common, not contrived.
There was a problem hiding this comment.
Confirmed by measurement: the schema has no origin, timestamp or history that separates a move from a changed default, so this heuristic cannot be made safe. The PR is back to draft and will be rebuilt so the admin picks each candidate in the preview (details in the PR comment).
The moved-event heal infers the move from the row's shape. A calendar whose default assignee was changed later, while the previous person is another calendar's default, looks identical; the confirmation, OpenAPI text and CHANGELOG now say so.
|
Holding this back from the release: the review finding holds, and it cannot be fixed inside the current shape. Measured against the schema: nothing in the data tells "this event moved from calendar A" apart from "calendar B's default assignee was changed". Next round, after the release: the preview of the #1154 action lists each candidate ("title, date, person X -> Y"), the admin unticks what should stay, and the confirmation binds the chosen subset - the same binding #1171 introduced for the whole set. What carries over from this branch: the shared "untouched" rule used by #1306 and the repair, the single write path, and the series tests. |
Code reviewNo issues found. Checked for bugs and CONTRIBUTING.md compliance (this repo has no CLAUDE.md; CONTRIBUTING.md on Reviewed at commit 6094fe0. |
This branch has not been deployed
1 participant
Problem
Events moved between two calendars of one account before v2.68.0 still carry the default assignee (and so the colour) of the calendar they came from. The runtime fix from #1306 only acts when
calendar_ref_idchanges during a sync, so it never reaches them.Fix
Extends the existing global admin action "Apply to existing appointments" (#1154,
GET/POST /api/v1/calendar/external-calendars/default-assignee-backfill) instead of adding a second mechanism. No migration, no new button.MOVED_DEFAULT_EVENTSinserver/services/sync-assignment.js): an imported event in calendar B whose only assignee is the default assignee of another calendar A of the same provider (for CalDAV the same account, viacaldav_calendar_selection),assigned_toempty or that person, B has a different default assignee. It gets B's default assignee.UNTOUCHED_DEFAULT_ASSIGNMENT(who), used by bothreassignDefaultOnCalendarMove()(fix(calendar): a moved event gets the new calendar's default assignee #1306) and the new query, so the two cannot drift. It includesNOT_PUSHED_OUTBOUND.user_modified = 0for the heal only: the runtime path saw the move, the heal cannot. So "exactly A's person" could also be a hand assignment in B. Every edit of an external event in Yuvomi, including assignment, setsuser_modified, and the inbound never clears it.setEventAssignments, past inherited reminders count as dismissed).Tests
test/test-calendar-routes.js, run through the real routes (count, token, apply):GET /in the window, all three occurrences show the new person)user_modified), two people, a different primary, another CalDAV account's default, target without default, own calendar, another provider, pushed outFailing before the fix:
gezaehlt werden die fuenf umgezogenen Termine - 0 !== 5anddie Zahl allein sieht den Wechsel nicht - 0 !== 1. Passing after. Counterproofs (file copy): droppinguser_modified, the account clause, the token kind, or a clause of the shared predicate each fails (the last one also failstest:sync-calendar-move-assignment).Suites: calendar-routes, sync-calendar-move-assignment, sync-default-assignee, i18n, i18n-plural, i18n-translated, frontend-audit, openapi-structure, openapi-coverage, api, api-offset-conversion, calendar-structure, settings-copy, changelog - all exit 0.
Closes #1307
Refs #1270