Skip to content

Overlapping Session Handle - #253

Draft
bdlukaa wants to merge 4 commits into
mainfrom
bruno/tot-1221-implement-overlapping-session-conflict-modal
Draft

Overlapping Session Handle#253
bdlukaa wants to merge 4 commits into
mainfrom
bruno/tot-1221-implement-overlapping-session-conflict-modal

Conversation

@bdlukaa

@bdlukaa bdlukaa commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

/rsvp/{event}/switch gives up spots for any overlapping sessions and attends the given session.

/rsvp/{event} returns an error when there is an overlapping session.

@linear

linear Bot commented Aug 13, 2026

Copy link
Copy Markdown

TOT-1221

@bdlukaa

bdlukaa commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

This comment was marked as outdated.

@bdlukaa

bdlukaa commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author
  1. Web is out of scope for this PR
  2. No custom email needed

@bdlukaa

bdlukaa commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Claude finished @bdlukaa's task in 5m 18s —— View job


Code review — Overlapping session handling

  • Read PR diff
  • Review totem/spaces/models.py conflict logic
  • Review mobile_api.py endpoints (locking, auth, error paths)
  • Check test coverage & existing callers of can_attend
  • Post review

Nice, focused change — the model-level SessionTimeConflict carrying the offending session is the right shape, and the tests cover the tricky edges (back-to-back, cancelled, unpublished space, staff bypass, and the "preserve attendance when the new session is unavailable" rollback). A few things I think need attention before merge.


1. Dead end: an already-started conflicting session can never be switched away from

time_conflicts_for (totem/spaces/models.py:316-331) filters on cancelled, ended_at and the time window, but not on whether the conflicting session has already started. Session.remove_attendee (models.py:451-452) refuses to remove an attendee from a started session.

Concrete failure:

  • now = 10:20
  • Session A: 10:00–11:00, in progress, user is attending
  • Session B: 10:30–11:30, not started, open, seats available

Then:

  • POST /rsvp/Bcan_attend passes the started() check for B, time_conflict_for returns A → 409 with A in the body → client shows the "switch?" modal.
  • POST /rsvp/B/switch {conflicting_session_slug: A}can_attend(excluding=[A]) passes → A.remove_attendee(user) raises SessionException("Session has already started")403, whole transaction rolls back.

The user is told to switch, and switching is impossible — they can never RSVP to B. Note rsvp_cancel has the same started() guard, so there's no client-side workaround either.

Either exclude started sessions from time_conflicts_for (you can't give up that spot, so arguably it shouldn't block a future RSVP), or special-case them in rsvp_switch — but the current combination is unreachable state. Worth a test with start=timezone.now() - timedelta(minutes=30) for the conflicting session. Fix this →

2. The 409 reports one conflict; the switch drops all of them

rsvp_confirm returns session_detail_schema(e.conflicting_session, user) — a single session, the earliest one (mobile_api.py:171). But rsvp_switch removes every overlapping session (mobile_api.py:209-216), which your own test_rsvp_switch_replaces_all_conflicting_sessions asserts.

So a user with two overlapping RSVPs sees a modal naming only the first, taps "switch", and silently loses the second spot too. That's user-visible data loss relative to what they consented to. The 409 body should carry the full conflict list so the modal can enumerate them.

Related: reusing SessionDetailSchema for the 409 makes the response ambiguous for clients — the body describes a different session than the one in the URL, and there's no message or discriminator. A dedicated schema (message + conflicting_sessions: list[SessionDetailSchema]) would be clearer and solves both problems. Fix this →

3. The website RSVP flow now hard-blocks with no remedy

The check lives in can_attend, which the web view also goes through (totem/spaces/views.py:98-117add_attendee). SessionTimeConflict subclasses SessionException, so rsvp() catches it and renders "This session conflicts with another session you are attending" — no switch option, and the message doesn't even name the conflicting session.

I understand web is out of scope for the modal, but this behavior change ships to production with this PR, so it's worth deciding deliberately rather than inheriting it. At minimum the message could name the session. Fix this →

4. Locking is careful in switch but absent in confirm

rsvp_switch goes to real trouble with select_for_update(), while rsvp_confirm (mobile_api.py:161-174) takes no locks at all — two concurrent RSVPs to overlapping sessions can both read "no conflict" and both commit. Since the whole point of the feature is that a user can't hold two overlapping spots, that's the invariant this is meant to protect.

Two smaller notes on the locking that is there:

  • select_for_update() combined with select_related("space") locks Space rows too. select_for_update(of=("self",)) keeps contention on sessions.
  • Locks are taken in two separate statements (mobile_api.py:191-197, then 211-213), each pk-ordered internally but not globally. Two concurrent switches over intersecting session sets can deadlock. Collecting the conflicts and locking once in pk order would avoid it.

Fix this →

5. time_conflicts_for re-implements visibility instead of using visible_to

models.py:52-54 states the invariant explicitly: "Session visibility is decided in exactly one place: SessionQuerySet.visible_to. Every surface that shows sessions … must start from it." time_conflicts_for hand-rolls space__published=True for non-staff and its docstring claims "visible sessions" — but it skips the room__banned_participants exclusion, so a session the user is banned from can still block their RSVP elsewhere. Because it filters attendees=user, the listed OR attending clause is automatically satisfied, so visible_to should be a drop-in.

It also feels like queryset logic sitting on the model — SessionQuerySet is where its siblings live.

One caveat if you do this: visible_to applies .distinct(), and Postgres rejects SELECT … FOR UPDATE with DISTINCT, so the select_for_update() call at mobile_api.py:212 would need restructuring (e.g. collect pks, then lock by pk). Fix this →

Minor consistency point in the same method: ended_at__isnull=True ignores the LiveKit _livekit_ended_backstop that not_ended() applies, so an abandoned LiveKit session with a null ended_at keeps conflicting past its window. Probably fine in practice, but it diverges from the module's other end-time logic.

6. Smaller things

  • Variable shadowingconflicting_session is bound at mobile_api.py:199 and then reused as the loop variable at 215. Harmless today (nothing reads it afterward), but it's exactly the kind of thing that bites on the next edit. Rename the loop variable.
  • conflicting_session_slug is nearly vestigial — since all conflicts are removed regardless, the payload only drives validation. Its side effect is a spurious 404 (mobile_api.py:204) if the client's view is stale (user cancelled that session in another tab), even though the switch could still proceed. Consider making it optional. Fix this →
  • Status code mix — "not attending the named session" and "session not found" are 404, but "sessions do not conflict" is AuthorizationError (403). The latter isn't an authorization failure; a 400/409 would read better.
  • session is rebound to Session | None at mobile_api.py:198. It narrows fine for pyright given the raise, but shadowing the outer non-optional binding weakens the typing the rest of the view relies on. Using a distinct name for the locked instance keeps types tight.
  • Type annotationsoverlaps and time_conflicts_for are annotated; can_attend still has bare silent=False and no return type. Pre-existing, but you're touching the signature anyway.
  • advertise() (models.py:519) calls can_attend(silent=True) with no user, so it will keep emailing subscribers about sessions they'll get a 409 on. Not a regression, but the feature makes it more visible.
  • Staff switch is a no-op switchconflicting_sessions stays [] for staff (mobile_api.py:210), so a staff member calling /switch just RSVPs and keeps both. That's what test_rsvp_switch_staff_keeps_started_conflicting_session locks in; just confirming it's intentional given the endpoint's name.

Test gaps worth closing

  • Conflicting session already in progress (issue 1) — currently an unreachable-state bug.
  • switch when the two sessions don't overlap → 403 "Sessions do not conflict".
  • switch when the user isn't attending the named conflicting session → 404.
  • switch when the target session is full (seats_left() == 0) rather than open=False, to confirm the same rollback path.
  • A 409 case where the earliest conflict is not the one the client would want to show (ties into issue 2).

I didn't run the suite — make test-python needs the Docker stack, which isn't available in this job. Worth confirming make test locally, particularly the new test_models.py cases.
• branch bruno/tot-1221-implement-overlapping-session-conflict-modal

@bdlukaa
bdlukaa marked this pull request as draft August 13, 2026 14:39
@bdlukaa
bdlukaa marked this pull request as ready for review August 13, 2026 18:18
@bdlukaa
bdlukaa marked this pull request as draft August 13, 2026 18:18
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