fix: sweep the eight decided, ready bugs - #148
Merged
Merged
Conversation
Plan covers the eight decided, ready bugs: calit-4whp, ek26, 75vf, jk8y, w7gq, h8mb, a4yj, mhgs. Excludes o69e (its own plan), mjof (needs a product answer) and 64hy (already answered).
A crafted POST could store a zone id that ZoneId.of rejects, 500ing the owner's public booking page and every booking on it. Guard on save, mirroring the locale guard one line below.
…ncelled cancelSingle deleted the remote event but left googleEventId, meetLink and the calendar address on the row, so a cancelled booking kept pointing at an event that no longer exists. Mirror deleteGroupGoogleEvent, including its rule that the refs are cleared even when the remote call is skipped.
The notice said '1 upcoming bookings stay on the calendar they were created
on'. There is no pluralization mechanism in the codebase, so reword the
message in all three locales to put the count last, where no noun agrees
with it.
Grepped src/main/java/site/asm0dey/calit/i18n/ for '{count}|(int count)|(long
count)' and found two other counted messages beyond the expected
adm_hosts_error_cap: adm_shared_revokeConfirm_count and
adm_hosts_removeConfirm_count, both phrased "{count} upcoming booking(s)".
The parenthetical (s) already makes those read correctly at any count, so no
further reword was needed. No message was found where a noun disagrees with
the count the way "1 upcoming bookings" did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
…m strings Commit 72df9e3 judged adm_shared_revokeConfirm_count and adm_hosts_removeConfirm_count as "already count-agnostic" on the strength of the English "{count} ... booking(s)" suffix, without checking the German and Hebrew .properties values. That claim was wrong: de read "1 bevorstehende Termine" (wrong noun number and adjective ending) and he read "1 פגישות עתידיות" (plural noun against the numeral 1) -- the identical bug class calit-75vf exists to fix. Reword both keys in all three locales (English included, so the whole message bundle uses one count-last convention instead of splitting it between the "(s)" trick and the count-last shape). adm_hosts_error_cap is untouched -- its {max} is a limit, not a subject noun, so the original judgment there stands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
Both write-calendar save paths counted bookings on a different calendar and showed the notice whenever that count was non-zero -- including on a save that re-submitted the calendar the type was already on. Compare the pre-save ref against the post-save one and stay quiet when they match.
Saving a type as Google Meet while its write calendar can't mint Meet links threw BadRequestException, which no handler caught and no mapper covered -- the Host got a blank, unlocalized 400. Throw the IllegalStateException both save paths already catch, with a message in en, de and he. Corrected the brief's German string: "Termintyp" instead of "Terminart" -- the codebase's established term (22 existing occurrences) and the correct grammatical gender for "dieser Termintyp".
resolveOwner resolved /{username} with no enabled check, so an account an
admin had switched off kept a live, bookable page -- and a disabled owner
who had working hours before being switched off was bookable right now,
mailing someone who has left. Guard the one method both the landing and the
booking routes go through.
/signup persisted the AppUser and stopped, while the other three creation paths each seeded the NOT NULL settings row the public booking path reads unguarded (issue #99). Extract OwnerSettings.seed and route all four through it, so the reason lives once instead of in three near-identical copies.
Not a bug fix — OidcSignInService.provision already seeded owner_settings correctly. It was a fifth account-creation path missed by the task-7 brief (5250cb1), which named only four and consolidated onto OwnerSettings.seed. Leaving this one hand-built defeated the point: the NOT NULL reason and its null-email handling would still live in a fourth copy instead of once on the helper's javadoc. Route it through OwnerSettings.seed too, dropping the now- redundant null-email ternary.
The manage page pre-selected the browser-detected zone in its picker while the dashboard and /me/pending used the stored one, so a travelling host read the same booking as 15:00 and 22:00 one click apart. The picker now defaults to the zone the page was authored in -- stored on /me, detected on the invitee pages, which emit no data-tz -- and both /me list pages name the zone on screen.
Commits the working-tree state of the jbcontext 2-week trial (calit-7a6t) so the trial is reproducible rather than living only on one machine: - .mcp.json: register the jbcontext stdio MCP server. - .claude/settings.json: reindex on SessionStart and SessionEnd, both async so neither blocks the session. - .claude/agents/context-explorer.md: the read-only research subagent the CLAUDE.md instructions point at, budgeted to 3 searches and 3 reads. - CLAUDE.md: the generated instruction block, inside the tool's own <!-- jbcontext-instructions-start/end --> markers so a later regeneration replaces it cleanly. No secrets or tokens in any of these. `.claude/agents/` is not gitignored (only `.claude/skills/` is, per the skills-symlink note in CLAUDE.md). Unrelated to the eight bug fixes on this branch; committed separately so the sweep's history stays reviewable on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
calit-h8mb closed the disabled-owner hole at the bug's reported call site, PublicResource.resolveOwner, rather than at the invariant. POST /api/bookings is a second, public and unauthenticated way in: BookingResource.create resolves the username itself and never touches resolveOwner, so the guard never applied to it. Nothing downstream caught it either. The bean's "the co-host path is already covered" note holds only for multi-host types -- MeetingHosts.bookable returns true unconditionally for a single-host type, which is exactly what a departed owner has. A plain curl still booked the disabled owner, wrote to their calendar and mailed them: the new test returned 201 Created before this change. Apply the same owner == null || !owner.enabled guard at that call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
calit-4whp closed the invalid-timezone hole at the bug's reported call site, AdminResource.updateSettings, and left the guard as a private zoneIds() helper local to that resource. MeSetupResource:103 -- the first-login wizard, which every user passes through -- still did a bare s.timezone = timezone. Same column, same eleven unguarded ZoneId.of(settings.timezone) readers, same blast radius: the owner's PUBLIC booking page and the booking transaction both 500 on a value the JDK cannot parse. The new wizard test stored "Not/AZone" before this change. Move the invariant onto OwnerSettings, mirroring the OwnerSettings.seed extraction this branch already made: zoneIds() as the single source for both pickers, and coerceZone(String) whose javadoc says every writer of the column must call it. Route both writers through it, delete the two duplicate private zoneIds() helpers and repoint all four picker call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
Seven smaller findings from the last-gate review, none of which change behaviour except one German string: - Nothing posted locationType and writeCalendar together, so the two tasks that share editMeetingType's transaction were never exercised in conflict. Add a test that submits both and pins that the calendar move rolls back with the refused save. - The two count keys reworded in d8b9646 had no coverage: the interstitial tests asserted only containsString("1"), which survived the rewording. Assert the wording in full. - TZ_SCRIPT's render() repeated the `initial` fallback expression verbatim. Collapse it to `initial`. The inverted-ternary protection moves to thePickerDefaultsToTheZoneThePageWasAuthoredIn, which pins `initial`'s definition character-for-character; verified by mutation that inverting it still fails a test. - That test named picker behaviour but ran against /me, which has no picker. Point it at /me/bookings/{id}/manage, which renders one, and assert so. - Layout.tzBar's javadoc still claimed the detected zone is selected. Reword to the actual body[data-tz]-first behaviour. - German adm_times_shown_in was a headline fragment; "Alle Zeiten in {zone}" reads as native UI German. English and Hebrew unchanged. - PublicDisabledOwnerTest's form POST sent name/email; submitBooking declares inviteeName/inviteeEmail. Use the real contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
- Token-keyed reschedule still reaches a disabled owner's calendar. Found by the whole-branch review and deliberately scoped out of fix/bug-sweep: calit-h8mb closed the stranger-facing routes, but a manage token bypasses both guards. Needs a rendered page rather than a 404 — the invitee holds a legitimate token. - ReminderScheduler NPEs and silently drops mail when an owner has no settings row. Pre-existing (that scheduler is untouched by this branch); surfaced while verifying it. calit-a4yj closed the creation side, so this is the unguarded READ that remains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
…stale SonarCloud flagged two issues on PR #148, both the same root cause: java:S1854 (useless assignment) and java:S1481 (unused local) on the `MeetingType t` at CohostManageTest:283. The seeding call is needed for its side effect but its return value never was — drop the assignment, keep the call. Also refreshes AdminTimeRenderingTest's class javadoc, which still claimed "/me pages have no #tz-picker". True of the dashboard and /me/pending, but no longer of the whole class: calit-mhgs pointed the picker-default test at /me/bookings/{id}/manage, which does have one. The javadoc now describes both shapes and why each matters. No production change; 921 tests green, spotless clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
EmailService.read did ZoneId.of(owner.timezone) straight off a possibly-null owner, so a booking whose owner has no owner_settings row threw NPE. read() backs EVERY mail path, not just reminders — but the reminder path is where it bit hardest: ReminderScheduler's deliberate per-booking catch (which exists so one poison booking can't roll back the other 49) marked the reminder sent and dropped the mail, so the invitee silently never got it and only an ERROR line recorded that anything happened. That catch is correct and is left alone; the defect was the unguarded read upstream of it (calit-sv6a). Now returns null for a missing settings row — the same "nothing to send" path every caller already takes for a missing booking — and warns, because after OwnerSettings.seed covers all five creation paths this should be unreachable. Also swaps the bare ZoneId.of for OwnerSettings.coerceZone: a row written before calit-4whp's save-time guard existed can still hold an unparseable zone, and a DateTimeException here would take out every mail for that owner rather than just one. The reminder now goes out coerced to UTC instead of throwing — pinned by test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
The invitee token routes are keyed by an unguessable manage token, not by username, so calit-h8mb's resolveOwner guard never sees them. An invitee holding a link from before their host was disabled could still POST a new time onto that departed host's calendar and trigger a fresh notification to someone who has left (calit-jyck). Reschedule and edit-details now refuse. Cancel deliberately does NOT: an invitee must always be able to get out of a meeting, and cancelling a departed host's booking is exactly what you want to happen. The refusal is not a 404 — the token is legitimate and the invitee knows the booking exists, so 404 would be a lie. The manage hub instead swaps both write forms for a localized notice (en/de/he) and keeps the cancel button live. The two POST handlers carry the same guard server-side, so a stale tab or crafted POST re-renders that hub rather than writing. Scoped to booking.ownerId, the row the invitee is managing, rather than the type's creator: on a group booking that is whose calendar carries the event. Also swaps renderManage's bare ZoneId.of for OwnerSettings.coerceZone — the same pre-guard-era unparseable-zone exposure calit-4whp was about, on a page that a 500 would make unusable. Guard mutation-tested: neutering hostInactive() fails exactly the three tests that assert refusal, while cancel and the enabled-host control stay green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
Both were filed as follow-ups from the whole-branch review and are now done here instead — this is a bug sweep, so they belong in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
|
asm0dey
added a commit
that referenced
this pull request
Aug 22, 2026
Six user-facing entries from PR #148, plus the upgrade note. The one caveat worth surfacing: bookings taken on a disabled account's page before the upgrade are not cancelled retroactively. Written at merge, per the project's rule that changelog entries land when a change reaches main rather than at release time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug
Closed
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
1 participant
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.
Closes eight open bugs that were already triaged and decided, one commit (or one commit plus a follow-up) each. No schema change, no new dependency, no migration.
mvn testis green on the branch tip: 921 tests, 0 failures, 0 errors, BUILD SUCCESS (up from an 898-test baseline).mvn spotless:checkpasses. i18n parity verified mechanically —AdminMessageshas 237 keys and bothadm_de.propertiesandadm_he.propertieshave exactly those 237, zero missing, zero extra.What's fixed
calit-4whpZoneId.of(...)sites. Coerced toUTCon save — on the entity, so both write paths are covered.calit-ek26cancelSingledeleted the remote Google event but left all four event-reference fields on the row. Now clears them, matching the group path, including when the account is disconnected.calit-75vf(s)hid the identical bug in German and Hebrew were reworded too.calit-jk8ycalit-w7gqBadRequestException→ blank, unlocalized 400, form lost. Now the localizedIllegalStateExceptionboth handlers already catch.calit-h8mbcalit-a4yj/signupcreated a user with noowner_settingsrow while every other path seeded one. ExtractedOwnerSettings.seed(...); all five creation paths route through it.calit-mhgs/mepages disagreed on timezone — two used the owner's stored zone, one the browser-detected one. All now use the stored zone; the picker became an explicit override. Invitee pages still auto-detect.Two findings from the whole-branch review
Both were cases where a fix had landed at the bug's reported call site rather than at the invariant, leaving a second reachable path open. Both are fixed here.
POST /api/bookingsstill booked a disabled owner. It resolves the username itself and never goes throughPublicResource.resolveOwner, and it is a public unauthenticated endpoint.MeetingHosts.bookablereturnstrueunconditionally for single-host types, so nothing downstream caught it. Reproduced as a real201 Createdbefore fixing.MeSetupResourceis a second writer of the same column with the same blast radius. The guard now lives onOwnerSettingsand both writers call it; two duplicate privatezoneIds()helpers were collapsed into one.Review notes
adm_he.properties:143/146were deliberately left alone rather than changed without a native reader.docs/drop-flyway-version-rangeis folded into this PR, deliberately. That branch was this one's fork point, it is fully contained here (git log fix/bug-sweep..docs/drop-flyway-version-rangeis empty), and its two commits —83de3dc(drop the Flyway version range from CLAUDE.md) and4f3e052(file the jbcontext trial and the create-form workplan) — land with this PR. It needs no separate PR; the branch can be deleted once this merges.0f6f3d0is unrelated tooling — the jbcontext trial wiring (MCP server, index hooks,context-exploreragent, generated CLAUDE.md block). Committed separately so the sweep's history stays reviewable on its own. No secrets.Two more, folded in after review
Both were first filed as follow-ups and then done here instead — this is a bug sweep, so they belong in it.
calit-jyck— token links could still write to a departed host's calendar. The invitee routes are keyed by an unguessable manage token, not by username, socalit-h8mb's guard never saw them.rescheduleandedit-detailsnow refuse;canceldeliberately does not — an invitee must always be able to get out of a meeting. The refusal is not a 404 (the token is legitimate and the booking exists, so 404 would be a lie): the hub swaps both write forms for a localized notice and keeps cancel live, with the same guard server-side for stale tabs. Mutation-tested — neutering the guard fails exactly the three refusal assertions and leaves cancel and the enabled-host control green.calit-sv6a— a settings-less owner silently ate every mail.EmailService.readdidZoneId.of(owner.timezone)off a possibly-null owner. It backs every mail path, but bit hardest on reminders:ReminderScheduler's deliberate per-booking catch (which exists so one poison booking can't roll back the other 49) marked the reminder sent and dropped it, so only an ERROR line recorded that anything happened. That catch is correct and untouched; the unguarded read upstream was the defect. Also swaps two bareZoneId.ofreads forOwnerSettings.coerceZone, so a row written beforecalit-4whp's guard existed degrades instead of throwing.Follow-ups still open
None from this branch.
Upgrade note
Nothing to do — no configuration and no database changes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HnLuwUFvipixJum9fR8xug