Allow Slack users to reserve equipment without an ESB account - #81
Allow Slack users to reserve equipment without an ESB account#81Aedalus wants to merge 1 commit into
Conversation
|
@claude please review this PR |
There was a problem hiding this comment.
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
reservationstable (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.
| original = ( | ||
| reservation_read_service.get_admin_reservation(replacement_reservation_id) | ||
| if replacement_reservation_id is not None | ||
| else None | ||
| ) | ||
| if original and original.is_slack_owned: | ||
| member_label = f"{original.owner_display_name} (Slack)" | ||
| elif command["owner_user_id"] is not None: | ||
| member_label = user_service.get_user(command["owner_user_id"]).display_name |
jantman
left a comment
jantman
left a comment
There was a problem hiding this comment.
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
f6a7b8c9d0e1chains correctly off heade5f6a7b8c9d0; 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), anduser_service.py:294still passes onlyrecipient_email._owner_filtersreturns[]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 keepsrequire_owner=False.- Template
disabled=/ hidden-field handling for the locked reservation type and owner is correct (WTForms omitsdisabled=False;owner_user_id=0is a valid choice, andallow_slack_ownercorrectly suppresses the required-member validator). owner_display_nameis used consistently across all admin serializers; public and ICS views don't display owner names.
— Opus 5
| or slack_user_id | ||
| ) | ||
| return user, display_name[:80] | ||
| except Exception: |
There was a problem hiding this comment.
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
| reservation = reservation_service.create_reservation( | ||
| equipment_id=equipment_id, | ||
| owner_user_id=esb_user.id, | ||
| owner_user_id=esb_user.id if esb_user else None, |
There was a problem hiding this comment.
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
| else None | ||
| ) | ||
| if original and original.is_slack_owned: | ||
| member_label = f"{original.owner_display_name} (Slack)" |
There was a problem hiding this comment.
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.
| member_label = f"{original.owner_display_name} (Slack)" | |
| member_label = original.owner_display_name |
— Opus 5
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
Testing
git diff --checkpassed.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
zbarlibrary.