Skip to content

fix(bookings): attribute cancellation reason server-side (#61) - #97

Closed
MinosChatzidakis wants to merge 1 commit into
Calnode:mainfrom
MinosChatzidakis:fix/cancel-attribution
Closed

MinosChatzidakis wants to merge 1 commit into
Calnode:mainfrom
MinosChatzidakis:fix/cancel-attribution

Conversation

@MinosChatzidakis

Copy link
Copy Markdown

Fixed the generic message which showed up when a booking was cancelled. No matter who cancelled it and what the reason was, "cancelled by admin" was shown to users.
After the following changes, this has been resolved, and the emails received both by the host and by the attendee clearly state the reason behind the cancellation (if the attendee cancelled it) or the name of the host (if the host cancelled it)

Files affected:
1.frontend/src/routes/bookings/+page.svelte

  • Removed the hardcoded "cancelled by admin" payload from the client call in +page.svelte.

2.internal/handler/booking_handler.go

  • Attributed the cancellation reason server-side in CancelBooking using the authenticated user's name/email and added a fallback in case neither was available.

I have tested the above changes manually with success.

This PR concerns Issue #90 which was created following discussion #61

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA. ✅
Posted by the CLA Assistant Lite bot.

@MinosChatzidakis

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Sep 23, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes

  • Server-side cancellation attribution — CancelBooking now builds Cancelled by <name|email|host> from the authenticated AuthUser and drops the legacy "cancelled by admin" literal before persisting (internal/handler/booking_handler.go:1629-1643).
  • Admin UI stops sending the hardcoded reason — the bookings page posts an empty body so the server owns attribution (frontend/src/routes/bookings/+page.svelte:210).

This matches issue #90's prescribed fix, and the manage-link/MCP paths are correctly left alone. go test ./internal/handler/ -run 'TestCancelBooking|TestRescheduleBooking_alreadyCancelled' passes.

ℹ️ Nitpicks

  • The two blank lines added at new lines 1628 and 1645 carry a literal tab; gofmt strips it. (main already trips the CI gofmt check on this file for unrelated drift, so this is not a new break — but it does add two more reasons to the list.)
  • No test pins the new attribution. TestCancelBooking (internal/handler/api_test.go:723) only asserts status == cancelled, yet the regression being fixed was exactly a wrong cancellation_reason. A short assertion that the reason no longer equals the legacy literal, or contains the caller, would lock the behavior in.
  • fmt.Sprintf("Cancelled by %s", actor) is untranslated English, and it lands in the translated attendee cancellation email (cancelOrgTmpl, internal/mailer/booking.go:465) as the Reason: value. The free-text reason is untranslated too, so impact is low; composing the prefix from a locale key would keep it consistent with the rest of the email.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using opencode-go/deepseek-v4.1-flash | 𝕏

// Ignore decode errors — reason is optional.
_ = json.NewDecoder(r.Body).Decode(&req)


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This blank line (and the one added at new line 1645) contains a literal tab. gofmt rewrites it and the repo's CI gofmt check step lists the file; the file is already flagged on main for other drift, so this is not blocking — but dropping the tab keeps these new lines clean.

shockalotti added a commit that referenced this pull request Sep 23, 2026
Edge caught reviewing PR #97 (MinosChatzidakis): the server-side
attribution assumed at least one of Name/Email is set. Belt and braces
for service/system callers acting without a full profile.
@shockalotti

Copy link
Copy Markdown
Contributor

Hi Minos — thank you for this PR, and apologies for the slow response on it.

Heads-up on what happened behind the scenes: while your PR was open, a very similar fix for #90 landed on main first (commit b918f4b — server-side attribution in CancelBooking plus the same frontend cleanup you made). That was on me: a bunch of PRs came through at once and I lost track of the fact that you were already working this issue, so your work got superseded without anyone telling you. Sorry about that — it's a process failure on my side, not a reflection on the contribution. The fix itself was correct and well-scoped.

Your review still caught one real edge the merged version missed: if both name and email are empty, main produced "Cancelled by " with a trailing space. I've just pushed that fallback ("Cancelled by host") to main in 95d4f17 and credited this PR in the message — so part of your work is in the tree.

One deliberate divergence I kept from main: the merged version drops the client-supplied reason entirely rather than appending it, treating the client string as untrusted. Happy to discuss if you feel strongly otherwise.

Please keep contributing — outside contributors catching things like this is exactly how the project grows, and I'd rather fix my triage process than lose that. If you want a next issue, say the word and I'll point you at one with a clear scope.

@github-actions github-actions Bot locked and limited conversation to collaborators Sep 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants