Skip to content

fix(google): address a booking's event by the calendar it was created on - #133

Merged
asm0dey merged 18 commits into
mainfrom
feat/per-booking-calendar-address
Aug 17, 2026
Merged

fix(google): address a booking's event by the calendar it was created on#133
asm0dey merged 18 commits into
mainfrom
feat/per-booking-calendar-address

Conversation

@asm0dey

@asm0dey asm0dey commented Aug 17, 2026

Copy link
Copy Markdown
Owner

The bug

A booking row stored only google_event_id — never which calendar the event was created on. Every later write (updateEvent, updateEventDetails, deleteEvent) resolved the calendar from GoogleCalendar.writeTarget(ownerId), i.e. whatever the owner's write target is now.

Rotate the write target — or connect a different Google account — between booking and cancel/reschedule, and calit addresses the event on the wrong calendar. Google answers 404. Since 1.20.1 deleteEvent treats 404 as "already gone", so the cancel succeeds locally while the event lives on forever on the old calendar, invisible to calit. Reschedule 404s loudly instead — noisy, but still unable to reach the event.

Closes the calit-rma2 bean, and unblocks calit-bh5t (per-meeting-type write target), which would otherwise orphan every existing booking the moment a type gets an override.

The fix

Store the address on the booking at creation time, and address every later write by that stored address instead of the owner's current write target.

  • V26 adds two nullable columns to booking: google_calendar_id text and google_credential_id bigint REFERENCES google_credential(id) ON DELETE SET NULL.
  • New record CalendarRef(Long credentialId, String googleCalendarId); Booking.calendarRef() returns one, or null.
  • CalendarPort.updateEvent / updateEventDetails / deleteEvent each take a CalendarRef. GoogleCalendarPort.writeAddress(ownerId, ref) resolves it behind an owner-scope guard — a ref whose credential is missing or belongs to another owner falls back to the owner's current write target.
  • CreatedEvent reports the address it actually wrote to; BookingService persists it behind a null guard.
  • Reschedule captures priorRef before clearing, so the re-approval delete still reaches the old calendar. Both address columns are cleared wherever google_event_id is cleared, and nowhere else.
  • groupEventId + organizerOwnerOf collapsed into one groupEventRow(UUID) lookup, so the event id, owner, and ref are guaranteed to come from the same row.

Deliberately no backfill. NULL means "address unknown, resolve as before". Stamping old rows with the current write target would be confidently wrong for exactly the bookings this bug affects.

The create side still resolves the owner's default write target — the per-meeting-type override is calit-bh5t's job, and it reuses the CalendarRef type introduced here.

Dependency bumps folded in

Supersedes #131 and #132 (both pom.xml-only, both green on their own CI):

  • jackson-core 2.22.1 → 2.22.2
  • spotless-maven-plugin 3.9.0 → 3.10.0 (re-checked: 371 files clean, no reformatting)

Close #131 and #132 when this merges.

Verification

  • Full suite: 830/830 green, 0 failures, 0 errors, 0 skipped — re-run after the dependency bumps and again after the review fixes.
  • spotless:check: clean, under the bumped plugin.
  • quarkus:dev boot: Flyway migrated to v26 - booking calendar address, app started, zero errors — Hibernate's validate strategy accepted the entity against the migration. Caveat: Dev Services hands out a fresh database, so this proves V26 applies in sequence, not that it applies onto a pre-existing populated DB.
  • Each task carried its own spec + quality review, plus a whole-branch review at the end. Every finding raised was a test gap, not a logic defect — no Critical issues, and no production line changed as a result of review.

Findings raised and fixed

Three rounds found the same class of hole: assertions that used any() in the CalendarRef position, so a write site could regress to the pre-fix behaviour without turning a single test red.

  • 1e8c3ef — the reschedule delete had nothing pinning it. Added a test that fails if the priorRef capture moves after the clearing block.
  • bec2dd1 — the updateEvent / updateEventDetails sites had nothing pinning them either, and StoredCalendarAddressTest seeded one credential for both the write target and the ref, so it never actually tested the credential half of the address — the multi-account case, which is the second half of this bug. Three tests added, each proven to fail against the specific regression it guards.

Still matched with any(), tracked on calit-vi8n with the exact tests to write: the group-details site and declineGuest. Both are correct as written — this is regression insurance, not a live risk.

Cleared with no action

The whole-branch review re-derived rather than assumed: the owner-scope guard in writeAddress (a booking's stored credential id cannot reach another owner's calendar — it is guarded and never user-supplied), the NULL-address paths for pre-migration rows, and migration safety. On the last: ADD COLUMN of two nullable columns is metadata-only, and rollback to a pre-V26 app is safe — no ignore-migration-patterns is set, so Flyway's default *:future applies, and Hibernate validate only checks mapped columns.

Notes for review

  • Commit 02c6605 still carries a wip(booking): subject — squash-merge, or reword before merging.
  • No new user-facing strings, so no de/he translation work.
  • The behaviour change is user-observable, so it owes a docs-site changelog entry in whichever release ships it.
  • Deferred deliberately, not dropped: calit-8dqz (what a 404 should mean now the calendar is known — this branch only logs the distinction and keeps 1.20.1's blanket tolerance), calit-64hy (disconnecting an account leaves a half-address that 500s on reschedule), calit-wsdr (index the new FK column).

asm0dey and others added 11 commits August 17, 2026 00:39
Per-(type, host) optional override of the write calendar, stored as a
(credential id, Google calendar id) pair so it survives the delete+reinsert
CalendarSelectionService.save() does to google_calendar rows. Lands after
calit-rma2, which stores the per-booking event address; without it, setting an
override on a type with future bookings orphans those events by design.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Task 4 of docs/superpowers/plans/2026-08-17-per-booking-calendar-address.md,
committed as work in progress: NOT test-run and NOT reviewed.

- update/delete call sites pass booking.calendarRef() instead of null
- reschedule captures priorRef alongside priorEventId and clears the address
  columns whenever googleEventId is cleared
- groupEventId + organizerOwnerOf collapsed into groupEventRow(UUID)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
The Step 4 approval-reschedule path (BookingService.applyRescheduleOutcome)
had zero coverage that could distinguish addressing the stored CalendarRef
from a plain null -- every existing assertion used any() for that argument.
Add a test that books+approves against a requiresApproval type, reschedules
as the invitee (triggering re-approval), and asserts deleteEvent is called
with the booking's stored ref (captured before the clearing block nulls
googleCalendarId/googleCredentialId) plus that the reloaded row is fully
cleared. Verified RED by moving the priorRef capture after the clearing
block; reverted that experiment before committing.

Extracted the repeated 5-line GoogleCredential seeding into a seedCredential
helper (now used by 4 tests) and added a requiresApproval overload of
bookAnySlot.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
@asm0dey asm0dey changed the title [WIP] Feat/per booking calendar address fix(google): address a booking's event by the calendar it was created on Aug 17, 2026
asm0dey and others added 7 commits August 17, 2026 10:29
Ticks the two remaining items, records what shipped, and splits the parts
this branch deliberately did not settle into their own beans:

- calit-8dqz: what a 404 should mean now the calendar is known
- calit-64hy: disconnect leaves a half-address that 500s on reschedule
- calit-wsdr: index booking.google_credential_id for the disconnect path
- calit-vi8n: cosmetic tidy-ups from the review

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
…ites

Two Important findings from the PR #133 final whole-branch review: only
deleteEvent had its CalendarRef pinned in tests, so a reverted
booking.calendarRef() -> null at the reschedule/updateDetails write sites
would still pass the full suite; and StoredCalendarAddressTest never
exercised the multi-account case writeAddress exists for, so it would not
catch writeAddress resolving the write-target's credential instead of the
ref's own.

Adds rescheduleOfANonApprovalTypePatchesTheStoredCalendar and
updateDetailsPatchesTheStoredCalendar to BookingCalendarAddressTest, and
deletesUsingTheStoredRefsCredentialNotTheWriteTargetsCredential to
StoredCalendarAddressTest. Each was proven RED against a temporarily
broken production line and reverted; src/main is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
The final review closed its test-gap finding at two of four write sites.
Records the remaining two (declineGuest, group details) with the exact
tests to write, so the gap is named rather than silent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Brings in 1.20.2 and the per-type working-hours fix (#134).

Only conflict was the calit-rma2 bean file: main added an 'Amended
2026-08-17' note recording that google_calendar_id is text rather than
VARCHAR(255), while this branch closed the bean out. Both kept — the
amendment is the constraint V26 was written against.

pom.xml auto-merged cleanly: version 1.20.2 from main, jackson-core
2.22.2 and spotless 3.10.0 from this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Closes the SonarCloud new_coverage gate on PR #133 (76.6% -> target
80%) with test-only additions -- no production code changed.

- GoogleCalendarPort: updateEvent/updateEventDetails now assert the
  patch is addressed at the stored CalendarRef, not the owner's
  current write target; writeAddress's fallback guard is exercised
  for a ref missing pieces, one pointing at a deleted credential, and
  one owned by another user.
- BookingService group-details path: the shared event's stored ref is
  threaded through and asserted with eq(ref) instead of any(); a group
  that never had a Google event exercises the eventRow == null guard.
- BookingService.declineGuest: a new test proves the owner id (from
  the guest row) and the calendar ref (from the booking row) are both
  wired correctly, since they come from two different objects.

Each new/tightened assertion was proven to fail against the specific
production line it guards, then reverted (git diff e51fb13 -- src/main
is empty). Full suite: 842 tests, 0 failures (835 pre-existing + 7 new).
Both were closed by 2211245 while fixing the SonarCloud coverage gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
Both verifies matched every argument with eq(), so the wrappers were
useless. Raw values are equivalent -- Mockito compares with equals(),
and CalendarRef is a record. The isNull() verify keeps its eq() calls,
since mixing a matcher with raw values is not allowed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017f4ahdPRUMmSQQcXvjkc9Y
@sonarqubecloud

Copy link
Copy Markdown

@asm0dey
asm0dey merged commit 75d1eef into main Aug 17, 2026
11 checks passed
@asm0dey
asm0dey deleted the feat/per-booking-calendar-address branch August 17, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant