Allow Slack users to reserve equipment without an ESB account

[Aedalus]

Slack users can now create and manage equipment reservations without first having an Equipment Status Board account. Linked users continue to use their ESB identity, while Slack-only reservations remain available through their Slack identity if an ESB account is linked later.

What changed

• Adds direct Slack ownership for member reservations while preserving the existing ESB-user and admin-hold ownership rules.
• Lets Slack-only users create, list, and cancel their own reservations.
• Preserves Slack ownership during admin edits and sends reservation notifications directly to Slack-only owners.
• Shows Slack owners consistently in administrative views and updates the reservation documentation.

Testing

• 282 focused reservation, Slack, notification, and admin tests passed.
• 1,909 broader tests passed.
• Ruff and git diff --check passed.

Review notes

The migration adds nullable Slack user ID and display-name columns, indexes the Slack user ID, and updates the reservation ownership constraint. One unrelated QR test cannot run locally because the host is missing the native zbar library.

[jantman]

@claude please review this PR

[Copilot]

Pull request overview

This PR extends the reservation system to support Slack-only ownership (Slack users without an ESB account) while preserving existing ESB-user ownership and admin-hold rules. It introduces Slack identity fields on reservations, updates Slack handlers and services to accept either ESB or Slack ownership, and adjusts admin views/notifications to display and message Slack-only owners correctly.

Changes:

• Add Slack-owner columns + updated ownership constraint to the reservations table (with index on Slack user ID).
• Update Slack reservation flows (create/list/cancel) and reservation services/read services to work with either ESB user IDs or Slack user IDs.
• Update admin reservation edit/display paths, notifications, and documentation to handle Slack-only owners consistently.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_views/test_admin_reservation_views.py	Adds coverage ensuring admin edits preserve Slack-only reservation ownership.
tests/test_slack/test_handlers.py	Adds Slack-only create/list/cancel flow tests and adjusts “My Reservations” behavior for unlinked users.
tests/test_services/test_reservation_service.py	Adds service-layer coverage for Slack-owned creation, listing after linking, and Slack-owner cancellation enforcement.
tests/test_services/test_notification_service.py	Adds coverage ensuring Slack-ID-addressed DMs are queued/delivered without email lookup.
tests/test_models/test_reservation.py	Adds model-level coverage for Slack-owned reservations and owner display formatting.
migrations/versions/f6a7b8c9d0e1_add_slack_reservation_owners.py	Adds nullable Slack owner columns + index and updates the DB ownership check constraint (with guarded downgrade).
esb/views/admin_reservations.py	Preserves Slack ownership during admin edits and passes Slack-owner display context into templates/confirmation.
esb/templates/admin/reservation_form.html	Updates admin reservation form UI to show a fixed Slack owner when editing Slack-owned reservations.
esb/slack/reservation_handlers.py	Allows unlinked Slack users to create/list/cancel reservations using Slack identity.
esb/slack/handlers.py	Adds a resolver returning both optional ESB user and stable Slack display name snapshot.
esb/services/slack_dm_service.py	Allows DM delivery via either email lookup or direct Slack user ID (exactly one required).
esb/services/reservation_service.py	Adds Slack-owner parameters/validation and supports Slack-only actors for creation/cancellation flows.
esb/services/reservation_read_service.py	Extends “my reservations” queries to accept either ESB user ID or Slack user ID ownership.
esb/services/notification_service.py	Queues reservation DMs to either member email or Slack user ID and delivers via either path.
esb/models/reservation.py	Adds Slack owner fields, ownership constraint, and owner display helpers.
esb/forms/reservation_forms.py	Allows admin edit submissions to validate when preserving a Slack owner (no member selection).
docs/reservations.md	Updates reservation docs to reflect Slack-only reservation capability and behavior after linking.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jantman]

Review: Slack-only reservations

Reviewed the full main...feat/slack-only-reservations diff (17 files) and ran the reservation / Slack / notification / admin test subset — 317 passed.

The overall shape is good, and a lot of the tricky parts hold up under scrutiny (see below). Three issues are worth addressing before merge, flagged as inline comments: two behavioral (a Slack API failure silently converting a linked member's reservation into an unfixable Slack-owned one, and deactivated members regaining the ability to reserve via Slack), and one cosmetic (a doubled (Slack) suffix on the admin confirmation page).

Verified as correct

• Migration f6a7b8c9d0e1 chains correctly off head e5f6a7b8c9d0; the op order inside the batch (drop constraint → add columns → index → recreate constraint) is valid for MariaDB, and the downgrade guard is sound.
• slack_dm_service.deliver_direct_message's exactly-one-recipient check: no caller can supply both (the DB check constraint prevents it), and user_service.py:294 still passes only recipient_email.
• _owner_filters returns [] when both identities are absent, and both callers short-circuit — no accidental "match every reservation" query.
• cancel_reservation(require_owner=True) correctly rejects admin holds and other users' reservations for both ESB-linked and Slack-only actors; the admin cancel path keeps require_owner=False.
• Template disabled= / hidden-field handling for the locked reservation type and owner is correct (WTForms omits disabled=False; owner_user_id=0 is a valid choice, and allow_slack_owner correctly suppresses the required-member validator).
• owner_display_name is used consistently across all admin serializers; public and ICS views don't display owner names.

— Opus 5

[jantman]

Requesting changes: this blanket except Exception permanently mis-attributes an ESB member's reservation to a Slack-only owner.

_resolve_reservation_owner catches every exception — Slack rate limits (429), network timeouts, transient DB errors — and returns (None, slack_user_id). reservation_handlers.py:110-117 then creates the reservation with owner_user_id=None, owner_slack_user_id=<id>, owner_slack_display_name=<raw Slack ID>.

Concrete scenario: an active, linked member submits a reservation while users_info is rate-limited. The row is written as Slack-owned with display name U012ABC. It then:

• never appears under the admin "Member" filter (reservation_read_service.py:541), and
• can never be corrected — _persist_admin_reservation (reservation_service.py:204-211) forces Slack ownership to be preserved on every admin edit, and the edit form hides the owner select.

The same outage also makes list_user_upcoming_reservations(None, slack_id) omit that member's genuinely ESB-owned reservations, so "My Reservations" silently looks empty and invites a double booking.

Suggest distinguishing "profile lookup failed" (retry, or surface an error modal) from "user has no ESB account" (legitimately Slack-owned), rather than collapsing both into the same fallback.

— Opus 5

[jantman]

Requesting changes: deactivated ESB members can now reserve equipment through Slack.

_resolve_reservation_owner looks up User with filter_by(email=..., is_active=True), so a deactivated member resolves to None, and this handler creates a Slack-owned reservation for them instead of rejecting the request. Before this PR they got "Your Slack account is not linked to an ESB user."

The rest of the system still enforces active status — _validate_admin_reservation_request (reservation_service.py:506-509) rejects an inactive owner, and get_admin_reservation_creation_options only lists active users — so Slack is now the one path around it.

Concrete scenario: a member deactivated for misuse of a tool opens /esb-reserve and books that tool anyway; the reservation shows in admin as Their Name (Slack) with no link to the disabled account.

If allowing any workspace member (including deactivated ones) is genuinely intended, that deserves an explicit decision and a note in the code — as written, the deactivation control is bypassable through Slack.

— Opus 5

[jantman]

Requesting changes: duplicated (Slack) suffix.

Reservation.owner_display_name (esb/models/reservation.py:118) already returns f"{self.slack_display_name} (Slack)", so appending it again renders Member: Slack Owner (Slack) (Slack).

Concrete scenario: staff edits a Slack-owned reservation into a window that produces an overridable policy violation (e.g. inside the advance-notice window) → _render_admin_reservation_confirmation runs and admin/reservation_confirm.html:19 shows the doubled suffix. The existing test only covers the GET edit form (asserting b"Slack Owner (Slack)"), not the confirmation page, so this isn't caught.

Suggested change

	member_label = f"{original.owner_display_name} (Slack)"
	member_label = original.owner_display_name

— Opus 5

[Aedalus]

@jantman Tried to address the above issues if you want to run another review

[jantman]

@Aedalus Apologies for the delay. The changes look good to me, I'll throw another round of claude at it and approve as long as it doesn't find any issues.

[jantman]

@claude please review this PR

[jantman]

@claude please review this PR

[claude]

Claude finished @jantman's task in 7m 30s —— View job

---

Review: Slack-only reservations

Todo list

• Gather context (diff vs origin/main, prior review comments)
• Verify prior review issues (rate-limit mis-attribution, deactivated-member bypass, doubled (Slack) suffix) were addressed in the 2026-08-25 commit
• Review the rest of the diff for new issues (models, services, Slack handlers, migration, forms, templates, tests)
• Run targeted tests, full suite, and ruff

All three issues from the prior review round are fixed correctly:

1. Rate-limit / lookup-failure mis-attribution (esb/slack/handlers.py, esb/slack/reservation_handlers.py) — _resolve_reservation_owner no longer swallows exceptions; _resolve_actor in reservation_handlers.py now distinguishes a lookup failure (LOOKUP_ERROR, surfaced as a modal, reservation never created) from "no ESB account" (legitimately Slack-owned). No more silent mis-attribution of a linked member's reservation to a raw Slack ID.
2. Deactivated members bypassing Slack — _resolve_actor now explicitly checks user is not None and not user.is_active and returns INACTIVE_ERROR, so a deactivated member can no longer reserve equipment through Slack (previously they fell through to Slack-only ownership).
3. Doubled (Slack) suffix — esb/views/admin_reservations.py's _render_admin_reservation_confirmation now uses original.owner_display_name directly (which already appends (Slack)), matching the Copilot/prior suggestion.

Spot-checked the rest of the diff

• Reservation model's new check constraint correctly enforces exactly one of user_id / (slack_user_id + slack_display_name) for member reservations, and none for admin holds — migration's batch op order and downgrade guard (blocks downgrade while Slack-owned rows exist) are sound.
• cancel_reservation's require_owner check correctly lets a since-linked ESB user still cancel a reservation that was originally created as Slack-owned, by matching on actor_slack_user_id (supports the "Slack ownership persists until linked" behavior described in the docs).
• _owner_filters / get_user_reservation / list_user_upcoming_reservations correctly return "no match" (rather than an unbounded query) when both identities are absent.
• _validate_reservation_shape correctly rejects both-owners and neither-owner cases.
• slack_dm_service.deliver_direct_message's exactly-one-recipient XOR check is enforced, and both callers respect it.
• Admin form/template correctly locks reservation type + owner for Slack-owned reservations via disabled fields + hidden inputs (WTForms omits disabled=False, and owner_user_id=0 is treated as "no ESB owner" downstream).

Testing performed

• venv/bin/python -m pytest tests/test_slack/test_handlers.py tests/test_services/test_reservation_service.py tests/test_services/test_notification_service.py tests/test_models/test_reservation.py tests/test_views/test_admin_reservation_views.py -q → 276 passed
• venv/bin/python -m pytest tests/ -q --ignore=tests/e2e --ignore=tests/test_services/test_qr_service.py → 1915 passed
• Full suite including test_qr_service.py: 1 failure (test_post_download_returns_png_with_preset_dimensions), due to the missing native zbar shared library in this environment — matches the PR description's noted unrelated environment limitation, not a regression from this change.
• ruff check esb/ tests/ → all checks passed.

Minor (non-blocking) nit

docs/reservations.md now says "Anyone who can use the ESB Slack app can reserve equipment. An Equipment Status Board account is not required." This is very slightly imprecise since deactivated ESB members are still blocked (INACTIVE_ERROR) — but that's an edge case not worth changing the docs for.

No blocking issues found. This looks ready to merge.