Add customer-facing appointment cancellation via UUID slug - #5
Add customer-facing appointment cancellation via UUID slug#5jamesmacwilliam wants to merge 1 commit into
Conversation
Customers receive a cancellation link in their confirmation email and can view their appointment and cancel from a public-facing page, identified by a UUID slug rather than the guessable integer ID. Refund policy: full refund if cancelled more than 48 hours before the appointment; deposit forfeited if within 48 hours. Changes: - Migration adds slug (UUID, unique) and cancelled_at to appointments - Appointment model: cancelled status, cancel!, cancellable?, within_48h?, corrected refund_amount, generate_slug callback, previously_new_record? guard to prevent double-send on create - CustomerAppointmentsController: show + cancel actions (slug lookup) - UUID-constrained routes alongside integer-constrained admin routes - Separate practitioner cancellation notification mailer (own email, not CC on the customer email) - Confirmation and status_changed emails include the appointment link - All user-facing strings extracted to en.yml (model, controller, mailer subjects, views) - Customer show page: appointment card with hero date/time, cancel demoted to a quiet ghost-button section below - RSpec + shoulda-matchers + factory_bot_rails with 45 model specs
jamesmacwilliam
left a comment
There was a problem hiding this comment.
Overview
Solid, well-scoped feature. The customer gets a public UUID-based URL to view and cancel their appointment, with a refund policy enforced at the model level. The split between cancellation_confirmation (customer only) and practitioner_cancellation_notification (practitioner only) is a cleaner design than a CC. i18n coverage is complete, routing constraints are correct, and the 45 model specs are comprehensive.
Bugs / Correctness
1. within_48h? uses .abs — semantically surprising (app/models/appointment.rb)
```ruby
def within_48h?
(starts_at - Time.current).abs <= 48.hours
end
```
The .abs makes this return true for past appointments within 48 hours too (e.g. an appointment that ended 24h ago). There's even a spec that explicitly tests this (spec line 147–150), which tests a confusing contract rather than useful behavior. Functionally it's harmless today because every call site goes through cancellable? first, but it's a landmine. The correct expression:
```ruby
def within_48h?
starts_at <= 48.hours.from_now
end
```
This is unambiguous — true only when the appointment is within 48 hours from now — and the "past appointments within 48h" spec should be removed.
2. cancel! has no idempotency guard (app/models/appointment.rb)
Calling cancel! on an already-cancelled appointment silently overwrites cancelled_at. The controller guards against it via cancellable?, but the model method itself has no protection. A simple guard prevents accidental double-firing:
```ruby
def cancel!
return if cancelled?
update!(status: :cancelled, cancelled_at: Time.current)
end
```
Design
3. Two synchronous deliver_now calls inside an after_save callback
If the second email raises (SMTP timeout, etc.), the exception bubbles up after the first email has already been sent — the customer gets a cancellation email but the practitioner doesn't (or vice versa). This is a pre-existing pattern in the codebase, but worth noting since this PR adds two emails in a single callback. deliver_later would isolate failures.
Code Quality
4. deposit_forfeit is a dead duplicate method (app/models/appointment.rb)
```ruby
def deposit_forfeit
return 0.0 if payment.nil?
payment.deposit_cents / 100.0
end
```
Identical to deposit_paid. The PR moved to deposit_paid in refund_amount but never removed deposit_forfeit. Should be deleted.
5. ActionMailer deliveries not cleared between specs (spec/rails_helper.rb)
The email tests (spec lines 67–81) address deliveries by index ([-2], .last) without a before { ActionMailer::Base.deliveries.clear } in rails_helper. use_transactional_fixtures doesn't clear the deliveries array. The change { ... }.by(2) assertion provides some protection, but if an earlier spec sends an email the indices are wrong. Standard fix:
```ruby
config.before { ActionMailer::Base.deliveries.clear }
```
6. Inline onclick confirm (app/views/customer_appointments/show.html.erb)
```ruby
onclick: "return confirm('#{j t(".cancel_confirm")}')"
```
Works, but Rails 7/Turbo idiom is data: { turbo_confirm: t(".cancel_confirm") } — no inline JS, integrates with Turbo's confirmation hook.
What's Good
previously_new_record?guard preventing confirmation + status_changed from double-firing on create is exactly right.generate_sluguses||=— safe to call without overwriting a pre-seeded slug.- Migration correctly backfills slugs before adding the NOT NULL constraint.
- UUID constraint on customer routes + integer constraint on
resources :appointmentsis a clean, explicit defense. - No raw strings anywhere — i18n is thorough.
cancellable?=confirmed? && upcoming?— simple and correct.
Summary
Two real issues to fix before merging: the .abs in within_48h? (semantic bug, test documents it wrong) and the dead deposit_forfeit method. The missing delivery clear in rails_helper is a low-risk test fragility worth adding. The idempotency guard on cancel! is a nice-to-have. Everything else is clean.
Summary
en.yml— no raw text in models, controllers, or mailersWhat changed
Data
slug(UUID, unique index, backfilled) andcancelled_at(datetime) toappointmentsModel (
Appointment)cancelled: 3added to status enumbefore_create :generate_slugsets UUID slugcancel!overrides the enum default to also stampcancelled_atcancellable?— confirmed + upcoming onlywithin_48h?— drives refund policyrefund_amount— full refund beyond 48h, deposit forfeited within 48hpreviously_new_record?guard prevents double-send on create (confirmation + status_changed both firing)Routing
resources :appointments, which gets an integer constraint as a backstopMailers
AppointmentMailer#cancellation_confirmation— customer only (CC removed)AppointmentMailer#practitioner_cancellation_notification— practitioner only, includes refund detailCustomer view
Test plan
bin/rspec spec/models/appointment_spec.rb— 45 examples, 0 failures