diff --git a/docs/reservations.md b/docs/reservations.md index 41d38a9..9a7adf7 100644 --- a/docs/reservations.md +++ b/docs/reservations.md @@ -17,7 +17,7 @@ Use **Previous**, the date field, or **Next** to view another day. If the page s {% if slack_enabled %} ## Reserve Equipment in Slack -Before you start, make sure that your Slack account is linked to an active ESB user. Ask a staff member for help if Slack says that your account is not linked. +Anyone who can use the ESB Slack app can reserve equipment. An Equipment Status Board account is not required. ### Create a Reservation @@ -44,7 +44,7 @@ If your requested time is unavailable or does not meet the equipment reservation ![Slack My Reservations window showing an upcoming reservation and its cancel action](images/reservations-cancel-form.png){ .docs-screenshot-compact } -You can cancel only an active reservation that belongs to you. Slack confirms when the reservation is canceled. +You can cancel only an active reservation that belongs to your Slack identity or linked ESB account. If staff later creates an ESB account for you, reservations you made previously through Slack still appear here. Slack confirms when the reservation is canceled. {% endif %} ## Manage Reservations diff --git a/esb/forms/reservation_forms.py b/esb/forms/reservation_forms.py index 4ab87d2..9d547f7 100644 --- a/esb/forms/reservation_forms.py +++ b/esb/forms/reservation_forms.py @@ -36,7 +36,11 @@ class AdminReservationCreateForm(FlaskForm): submit = SubmitField("Review reservation") def validate_owner_user_id(self, field): - if self.reservation_type.data == RESERVATION_TYPE_MEMBER and not field.data: + if ( + self.reservation_type.data == RESERVATION_TYPE_MEMBER + and not field.data + and not getattr(self, "allow_slack_owner", False) + ): raise ValidationError("Select an active member for this reservation.") diff --git a/esb/models/reservation.py b/esb/models/reservation.py index 3da6b72..a170b4e 100644 --- a/esb/models/reservation.py +++ b/esb/models/reservation.py @@ -24,8 +24,11 @@ class Reservation(db.Model): name="ck_reservations_type", ), db.CheckConstraint( - "(reservation_type = 'member' AND user_id IS NOT NULL) " - "OR (reservation_type = 'admin_hold' AND user_id IS NULL)", + "(reservation_type = 'member' AND " + "((user_id IS NOT NULL AND slack_user_id IS NULL AND slack_display_name IS NULL) " + "OR (user_id IS NULL AND slack_user_id IS NOT NULL AND slack_display_name IS NOT NULL))) " + "OR (reservation_type = 'admin_hold' AND user_id IS NULL " + "AND slack_user_id IS NULL AND slack_display_name IS NULL)", name="ck_reservations_type_owner", ), db.CheckConstraint("ends_at > starts_at", name="ck_reservations_valid_interval"), @@ -34,6 +37,8 @@ class Reservation(db.Model): id = db.Column(db.Integer, primary_key=True) equipment_id = db.Column(db.Integer, db.ForeignKey("equipment.id"), nullable=False, index=True) user_id = db.Column(db.Integer, db.ForeignKey("users.id"), nullable=True, index=True) + slack_user_id = db.Column(db.String(32), nullable=True, index=True) + slack_display_name = db.Column(db.String(80), nullable=True) starts_at = db.Column(db.DateTime, nullable=False, index=True) ends_at = db.Column(db.DateTime, nullable=False, index=True) status = db.Column(db.String(20), default='active', nullable=False, index=True) @@ -99,5 +104,19 @@ def is_admin_hold(self) -> bool: """Return whether this row blocks equipment without a member owner.""" return self.reservation_type == RESERVATION_TYPE_ADMIN_HOLD + @property + def is_slack_owned(self) -> bool: + """Return whether this member reservation belongs directly to Slack.""" + return self.slack_user_id is not None + + @property + def owner_display_name(self) -> str: + """Return the stable owner label used by administrative views.""" + if self.user is not None: + return self.user.display_name + if self.slack_display_name: + return f"{self.slack_display_name} (Slack)" + return "Admin Hold" + def __repr__(self): return f'' diff --git a/esb/services/notification_service.py b/esb/services/notification_service.py index 1f99b2c..59ced54 100644 --- a/esb/services/notification_service.py +++ b/esb/services/notification_service.py @@ -170,18 +170,20 @@ def queue_notification( def queue_member_reservation_notification(reservation: Reservation, event_type: str) -> str | None: """Queue a member reservation DM; return a non-fatal warning on failure.""" - if reservation.reservation_type == RESERVATION_TYPE_ADMIN_HOLD or reservation.user is None: + if reservation.reservation_type == RESERVATION_TYPE_ADMIN_HOLD: return None if not current_app.config.get('SLACK_BOT_TOKEN', ''): return 'Reservation was saved, but Slack notifications are not configured.' equipment = reservation.equipment base_url = current_app.config.get('ESB_BASE_URL', '').rstrip('/') + recipient = reservation.user.email if reservation.user else reservation.slack_user_id payload = { 'event_type': event_type, 'reservation_id': reservation.id, - 'recipient_email': reservation.user.email, - 'recipient_username': reservation.user.username, + 'recipient_email': reservation.user.email if reservation.user else None, + 'recipient_slack_user_id': reservation.slack_user_id, + 'recipient_username': reservation.user.username if reservation.user else reservation.slack_display_name, 'equipment_name': equipment.name if equipment else f'Equipment {reservation.equipment_id}', 'area_name': equipment.area.name if equipment and equipment.area else 'Unknown Area', 'starts_at_label': utc_naive_to_local(reservation.starts_at).strftime('%Y-%m-%d %I:%M %p %Z'), @@ -192,7 +194,7 @@ def queue_member_reservation_notification(reservation: Reservation, event_type: try: queue_notification( notification_type='slack_dm', - target=reservation.user.email, + target=recipient, payload=payload, ) except Exception: @@ -402,16 +404,18 @@ def _deliver_slack_message(notification: PendingNotification) -> None: def _deliver_slack_dm(notification: PendingNotification) -> None: - """Resolve a member by email and deliver a reservation DM through Slack.""" + """Deliver a reservation DM by stored Slack ID or member email.""" payload = dict(notification.payload or {}) recipient_email = payload.get('recipient_email') - if not recipient_email: - raise RuntimeError('Reservation notification has no recipient email') + recipient_slack_user_id = payload.get('recipient_slack_user_id') + if not recipient_email and not recipient_slack_user_id: + raise RuntimeError('Reservation notification has no recipient') text, _blocks = _format_slack_message(payload) from esb.services import slack_dm_service slack_dm_service.deliver_direct_message( recipient_email=recipient_email, + recipient_slack_user_id=recipient_slack_user_id, text=text, timeout=15, ) diff --git a/esb/services/reservation_read_service.py b/esb/services/reservation_read_service.py index 69173e7..1f530be 100644 --- a/esb/services/reservation_read_service.py +++ b/esb/services/reservation_read_service.py @@ -4,6 +4,7 @@ from datetime import UTC, date, datetime, timedelta from typing import TypedDict +from sqlalchemy import or_ from sqlalchemy.orm import joinedload from esb.extensions import db @@ -138,20 +139,33 @@ def get_admin_reservation_creation_options() -> dict[str, list]: } -def get_user_reservation(reservation_id: int, user_id: int) -> Reservation | None: - """Return a reservation only when it belongs to the given user.""" +def get_user_reservation( + reservation_id: int, + user_id: int | None, + slack_user_id: str | None = None, +) -> Reservation | None: + """Return a reservation only when either supplied identity owns it.""" + owner_filters = _owner_filters(user_id, slack_user_id) + if not owner_filters: + return None return db.session.execute( - db.select(Reservation).filter_by(id=reservation_id, user_id=user_id) + db.select(Reservation).filter(Reservation.id == reservation_id, or_(*owner_filters)) ).scalar_one_or_none() -def list_user_upcoming_reservations(user_id: int) -> list[Reservation]: - """Return a user's active reservations that have not ended yet.""" +def list_user_upcoming_reservations( + user_id: int | None, + slack_user_id: str | None = None, +) -> list[Reservation]: + """Return active future reservations owned by either supplied identity.""" + owner_filters = _owner_filters(user_id, slack_user_id) + if not owner_filters: + return [] return list( db.session.execute( db.select(Reservation) .filter( - Reservation.user_id == user_id, + or_(*owner_filters), Reservation.status == ACTIVE_STATUS, Reservation.ends_at > _utc_now(), ) @@ -162,6 +176,15 @@ def list_user_upcoming_reservations(user_id: int) -> list[Reservation]: ) +def _owner_filters(user_id: int | None, slack_user_id: str | None) -> list: + filters = [] + if user_id is not None: + filters.append(Reservation.user_id == user_id) + if slack_user_id: + filters.append(Reservation.slack_user_id == slack_user_id) + return filters + + def get_admin_reservation(reservation_id: int) -> Reservation: """Return a reservation and its edit/display relationships for admin actions.""" reservation = db.session.execute( @@ -557,7 +580,7 @@ def _serialize_admin_reservation(reservation: Reservation) -> AdminReservationRo ends_local = utc_naive_to_local(reservation.ends_at) equipment = reservation.equipment settings = equipment.reservation_settings if equipment else None - owner = reservation.user.display_name if reservation.user else "Admin Hold" + owner = reservation.owner_display_name equipment_name = equipment.name if equipment else f"Equipment {reservation.equipment_id}" note = reservation.notes or "" calendar_label = f"{equipment_name}: {owner}" @@ -578,8 +601,16 @@ def _serialize_admin_reservation(reservation: Reservation) -> AdminReservationRo "ends_at_label": _format_local_datetime_label(ends_local), "status": reservation.status, "created_via": reservation.created_via, - "created_by": reservation.created_by_user.display_name if reservation.created_by_user else "", - "canceled_by": reservation.canceled_by_user.display_name if reservation.canceled_by_user else "", + "created_by": ( + reservation.created_by_user.display_name + if reservation.created_by_user + else owner if reservation.created_via == "slack" else "" + ), + "canceled_by": ( + reservation.canceled_by_user.display_name + if reservation.canceled_by_user + else owner if reservation.status == CANCELED_STATUS and reservation.is_slack_owned else "" + ), "replaces_reservation_id": reservation.replaces_reservation_id, "replaces_label": ( f"Reservation #{reservation.replaces_reservation_id}" if reservation.replaces_reservation_id else "" diff --git a/esb/services/reservation_service.py b/esb/services/reservation_service.py index 699820b..1f41130 100644 --- a/esb/services/reservation_service.py +++ b/esb/services/reservation_service.py @@ -34,30 +34,36 @@ def create_reservation( created_via: str, *, actor_user_id: int | None = None, + owner_slack_user_id: str | None = None, + owner_slack_display_name: str | None = None, reservation_type: str = RESERVATION_TYPE_MEMBER, overridden_policy_codes: list[str] | tuple[str, ...] | None = None, commit: bool = True, ) -> Reservation: """Validate and persist a reservation. - ``actor_user_id`` controls policy privileges; ``owner_user_id`` is the - member the reservation belongs to. Callers creating a larger transaction + ``actor_user_id`` controls policy privileges. A member reservation belongs + to either an ESB user or a Slack user. Callers creating a larger transaction can pass ``commit=False`` and commit or roll back the session themselves. """ if created_via not in RESERVATION_CREATED_VIA: raise ValidationError(f"Invalid reservation source: {created_via!r}") + owner_slack_user_id = (owner_slack_user_id or "").strip() or None + owner_slack_display_name = (owner_slack_display_name or "").strip() or None _validate_reservation_shape( reservation_type=reservation_type, owner_user_id=owner_user_id, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, starts_at=None, ends_at=None, ) owner = _get_user(owner_user_id, label="owner") if owner_user_id is not None else None if actor_user_id is None: - if owner is None: + if owner is None and owner_slack_user_id is None: raise ValidationError("Reservation actor is required for an admin hold") - actor_user_id = owner.id + actor_user_id = owner.id if owner is not None else None validated = validate_reservation_request( equipment_id=equipment_id, starts_at_utc=starts_at_utc, @@ -67,6 +73,8 @@ def create_reservation( reservation = persist_reservation( validated=validated, owner_user_id=owner.id if owner is not None else None, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, notes=notes, created_via=created_via, reservation_type=reservation_type, @@ -75,7 +83,8 @@ def create_reservation( commit=commit, ) if commit: - _log_reservation_created(reservation, _get_user(actor_user_id, label="actor")) + actor = _get_user(actor_user_id, label="actor") if actor_user_id is not None else None + _log_reservation_created(reservation, actor) return reservation @@ -91,8 +100,20 @@ def preview_admin_reservation( exclude_reservation_id: int | None = None, ) -> reservation_policy.ReservationValidationResult: """Validate an admin form without taking a lock or writing a reservation.""" + owner_slack_user_id = None + owner_slack_display_name = None + if exclude_reservation_id is not None: + original = _get_reservation(exclude_reservation_id, for_update=False) + if original.is_slack_owned: + owner_user_id = None + owner_slack_user_id = original.slack_user_id + owner_slack_display_name = original.slack_display_name + reservation_type = RESERVATION_TYPE_MEMBER + _validate_admin_reservation_request( owner_user_id=owner_user_id, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, notes=notes, reservation_type=reservation_type, ) @@ -180,8 +201,18 @@ def _persist_admin_reservation( if original.status == CANCELED_STATUS: raise ValidationError("Reservation is already canceled") + owner_slack_user_id = None + owner_slack_display_name = None + if original and original.is_slack_owned: + owner_slack_user_id = original.slack_user_id + owner_slack_display_name = original.slack_display_name + owner_user_id = None + reservation_type = RESERVATION_TYPE_MEMBER + _validate_admin_reservation_request( owner_user_id=owner_user_id, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, notes=notes, reservation_type=reservation_type, ) @@ -205,6 +236,8 @@ def _persist_admin_reservation( reservation = persist_reservation( validated=result.validated, owner_user_id=owner_user_id, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, notes=notes, created_via="admin", reservation_type=reservation_type, @@ -244,7 +277,7 @@ def validate_reservation_request( equipment_id: int, starts_at_utc: datetime, duration_minutes: int, - actor_user_id: int, + actor_user_id: int | None, ) -> reservation_policy.ValidatedReservation: """Validate a proposed reservation without persisting one. @@ -271,7 +304,7 @@ def evaluate_reservation_request( equipment_id: int, starts_at_utc: datetime, duration_minutes: int, - actor_user_id: int, + actor_user_id: int | None, lock_equipment: bool = False, exclude_reservation_id: int | None = None, ) -> reservation_policy.ReservationValidationResult: @@ -282,8 +315,8 @@ def evaluate_reservation_request( should use the default unlocked query; persistence callers should request a lock and revalidate in the transaction that writes the reservation. """ - actor = _get_user(actor_user_id, label="actor") - can_override_policy = actor.role in reservation_policy.RESERVATION_POLICY_OVERRIDE_ROLES + actor = _get_user(actor_user_id, label="actor") if actor_user_id is not None else None + can_override_policy = bool(actor and actor.role in reservation_policy.RESERVATION_POLICY_OVERRIDE_ROLES) equipment = _get_equipment(equipment_id, for_update=lock_equipment) starts_at = to_utc_naive(starts_at_utc) ends_at = starts_at + timedelta(minutes=duration_minutes) @@ -312,6 +345,8 @@ def persist_reservation( *, validated: reservation_policy.ValidatedReservation, owner_user_id: int | None, + owner_slack_user_id: str | None = None, + owner_slack_display_name: str | None = None, notes: str | None, created_via: str, reservation_type: str = RESERVATION_TYPE_MEMBER, @@ -328,9 +363,13 @@ def persist_reservation( if created_via not in RESERVATION_CREATED_VIA: raise ValidationError(f"Invalid reservation source: {created_via!r}") + owner_slack_user_id = (owner_slack_user_id or "").strip() or None + owner_slack_display_name = (owner_slack_display_name or "").strip() or None _validate_reservation_shape( reservation_type=reservation_type, owner_user_id=owner_user_id, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, starts_at=validated.starts_at, ends_at=validated.ends_at, ) @@ -344,6 +383,8 @@ def persist_reservation( reservation = Reservation( equipment_id=validated.equipment_id, user_id=owner.id if owner is not None else None, + slack_user_id=owner_slack_user_id, + slack_display_name=owner_slack_display_name, starts_at=validated.starts_at, ends_at=validated.ends_at, notes=(notes or "").strip() or None, @@ -362,8 +403,10 @@ def persist_reservation( def cancel_reservation( reservation_id: int, - actor_user_id: int, + actor_user_id: int | None, *, + actor_slack_user_id: str | None = None, + require_owner: bool = False, commit: bool = True, ) -> Reservation: """Cancel an active reservation while preserving history. @@ -373,19 +416,26 @@ def cancel_reservation( """ reservation = _get_reservation(reservation_id, for_update=True) - actor = _get_user(actor_user_id, label="actor") + actor = _get_user(actor_user_id, label="actor") if actor_user_id is not None else None + if actor is None and not actor_slack_user_id: + raise ValidationError("Reservation cancellation actor is required") + if require_owner and not ( + (actor is not None and reservation.user_id == actor.id) + or (actor_slack_user_id and reservation.slack_user_id == actor_slack_user_id) + ): + raise ValidationError("That reservation is no longer available to cancel") if reservation.status == CANCELED_STATUS: raise ValidationError("Reservation is already canceled") reservation.status = CANCELED_STATUS reservation.canceled_at = _utc_now() - reservation.canceled_by_user_id = actor.id + reservation.canceled_by_user_id = actor.id if actor is not None else None if commit: db.session.commit() log_mutation( "reservation.status_changed", - actor.username, + actor.username if actor is not None else actor_slack_user_id, { "reservation_id": reservation.id, "equipment_id": reservation.equipment_id, @@ -437,6 +487,8 @@ def _get_user(user_id: int, *, label: str) -> User: def _validate_admin_reservation_request( *, owner_user_id: int | None, + owner_slack_user_id: str | None = None, + owner_slack_display_name: str | None = None, notes: str, reservation_type: str, ) -> None: @@ -444,6 +496,8 @@ def _validate_admin_reservation_request( _validate_reservation_shape( reservation_type=reservation_type, owner_user_id=owner_user_id, + owner_slack_user_id=owner_slack_user_id, + owner_slack_display_name=owner_slack_display_name, starts_at=None, ends_at=None, ) @@ -461,15 +515,16 @@ def _require_admin_reservation_actor(result: reservation_policy.ReservationValid raise ValidationError("Admin reservations require a staff or technician actor") -def _log_reservation_created(reservation: Reservation, actor: User) -> None: +def _log_reservation_created(reservation: Reservation, actor: User | None) -> None: """Write a privacy-conscious structured log after a committed creation.""" log_mutation( "reservation.created", - actor.username, + actor.username if actor is not None else reservation.slack_user_id, { "reservation_id": reservation.id, "equipment_id": reservation.equipment_id, "owner_user_id": reservation.user_id, + "owner_slack_user_id": reservation.slack_user_id, "reservation_type": reservation.reservation_type, "created_via": reservation.created_via, "overridden_policy_codes": reservation.overridden_policy_codes or [], @@ -481,14 +536,24 @@ def _validate_reservation_shape( *, reservation_type: str, owner_user_id: int | None, + owner_slack_user_id: str | None = None, + owner_slack_display_name: str | None = None, starts_at: datetime | None, ends_at: datetime | None, ) -> None: if reservation_type not in RESERVATION_TYPES: raise ValidationError(f"Invalid reservation type: {reservation_type!r}") - if reservation_type == RESERVATION_TYPE_MEMBER and owner_user_id is None: + has_user_owner = owner_user_id is not None + has_slack_owner = owner_slack_user_id is not None or owner_slack_display_name is not None + if has_slack_owner and not ( + (owner_slack_user_id or "").strip() and (owner_slack_display_name or "").strip() + ): + raise ValidationError("Slack reservations require a user ID and display name") + if reservation_type == RESERVATION_TYPE_MEMBER and not (has_user_owner or has_slack_owner): raise ValidationError("Member reservations require an owner") - if reservation_type == RESERVATION_TYPE_ADMIN_HOLD and owner_user_id is not None: + if reservation_type == RESERVATION_TYPE_MEMBER and has_user_owner and has_slack_owner: + raise ValidationError("Member reservations require exactly one owner") + if reservation_type == RESERVATION_TYPE_ADMIN_HOLD and (has_user_owner or has_slack_owner): raise ValidationError("Admin holds cannot have an owner") if starts_at is not None and ends_at is not None and ends_at <= starts_at: raise ValidationError("Reservation end must be after start") diff --git a/esb/services/slack_dm_service.py b/esb/services/slack_dm_service.py index ee6ded5..3d52afe 100644 --- a/esb/services/slack_dm_service.py +++ b/esb/services/slack_dm_service.py @@ -5,12 +5,13 @@ def deliver_direct_message( *, - recipient_email: str, text: str, timeout: int, + recipient_email: str | None = None, + recipient_slack_user_id: str | None = None, client_factory=None, ) -> None: - """Resolve a Slack user by email, open a DM, and post ``text``. + """Resolve an email when needed, open a DM, and post ``text``. Callers choose whether an error should be surfaced synchronously or left to the notification worker's retry policy. @@ -24,8 +25,12 @@ def deliver_direct_message( client_factory = WebClient client = client_factory(token=token, timeout=timeout) - lookup = client.users_lookupByEmail(email=recipient_email) - slack_user_id = lookup["user"]["id"] + if bool(recipient_email) == bool(recipient_slack_user_id): + raise ValueError("Provide exactly one Slack DM recipient") + slack_user_id = recipient_slack_user_id + if recipient_email: + lookup = client.users_lookupByEmail(email=recipient_email) + slack_user_id = lookup["user"]["id"] opened = client.conversations_open(users=[slack_user_id]) dm_channel_id = opened["channel"]["id"] client.chat_postMessage(channel=dm_channel_id, text=text) diff --git a/esb/slack/handlers.py b/esb/slack/handlers.py index b996b15..73bb396 100644 --- a/esb/slack/handlers.py +++ b/esb/slack/handlers.py @@ -44,20 +44,34 @@ def _resolve_esb_user(client, slack_user_id): Note: Must be called from within a Flask app context (provided by the calling handler's _ensure_app_context wrapper). """ + try: + user, _display_name = _resolve_reservation_owner(client, slack_user_id) + return user if user is not None and user.is_active else None + except Exception: + logger.warning('Failed to resolve Slack profile for user %s', slack_user_id, exc_info=True) + return None + + +def _resolve_reservation_owner(client, slack_user_id): + """Return an optional ESB user plus a stable Slack display-name snapshot.""" from esb.extensions import db from esb.models.user import User - try: - result = client.users_info(user=slack_user_id) - email = result['user']['profile'].get('email') - if not email: - return None - return db.session.execute( - db.select(User).filter_by(email=email, is_active=True) + slack_user = client.users_info(user=slack_user_id)['user'] + profile = slack_user.get('profile', {}) + email = profile.get('email') + user = None + if email: + user = db.session.execute( + db.select(User).filter_by(email=email) ).scalars().first() - except Exception: - logger.warning('Failed to resolve ESB user for Slack user %s', slack_user_id, exc_info=True) - return None + display_name = ( + profile.get('display_name') + or profile.get('real_name') + or slack_user.get('name') + or slack_user_id + ) + return user, display_name[:80] # IMPORTANT: All handlers that access DB/services must wrap their body in @@ -71,7 +85,7 @@ def register_handlers(bolt_app, app): bolt_app, app, ensure_app_context=_ensure_app_context, - resolve_esb_user=_resolve_esb_user, + resolve_reservation_owner=_resolve_reservation_owner, ) @bolt_app.command('/esb-report') diff --git a/esb/slack/reservation_handlers.py b/esb/slack/reservation_handlers.py index a36e871..0cbdff9 100644 --- a/esb/slack/reservation_handlers.py +++ b/esb/slack/reservation_handlers.py @@ -4,6 +4,9 @@ logger = logging.getLogger(__name__) +LOOKUP_ERROR = 'Unable to verify your Slack account. Please try again.' +INACTIVE_ERROR = 'Your Equipment Status Board account is inactive. Contact staff for help.' + def _update_error_modal(client, body, message): from esb.slack.reservation_forms import build_reservation_error_modal @@ -14,7 +17,18 @@ def _update_error_modal(client, body, message): ) -def register_reservation_handlers(bolt_app, app, *, ensure_app_context, resolve_esb_user): +def _resolve_actor(client, slack_user_id, resolve_reservation_owner): + try: + user, display_name = resolve_reservation_owner(client, slack_user_id) + except Exception: + logger.exception('Failed to resolve reservation owner for Slack user %s', slack_user_id) + return None, None, LOOKUP_ERROR + if user is not None and not user.is_active: + return None, None, INACTIVE_ERROR + return user, display_name, None + + +def register_reservation_handlers(bolt_app, app, *, ensure_app_context, resolve_reservation_owner): """Register only the reservation Slack commands, actions, and submissions.""" @bolt_app.command('/esb-reserve') @@ -24,7 +38,22 @@ def handle_esb_reserve(ack, body, client): from datetime import UTC, datetime from esb.services import reservation_read_service - from esb.slack.reservation_forms import build_reservation_landing_modal + from esb.slack.reservation_forms import ( + build_reservation_error_modal, + build_reservation_landing_modal, + ) + + _user, _display_name, error = _resolve_actor( + client, + body['user_id'], + resolve_reservation_owner, + ) + if error: + client.views_open( + trigger_id=body['trigger_id'], + view=build_reservation_error_modal(error), + ) + return now = datetime.now(UTC) availability = reservation_read_service.get_public_availability(now=now) @@ -77,7 +106,6 @@ def handle_reservation_availability_submission(ack, body, client, view): from esb.services import equipment_service, reservation_service from esb.slack.reservation_forms import ( build_reservation_confirmation_modal, - build_reservation_error_modal, build_reservation_processing_modal, build_reservation_unavailable_modal, ) @@ -100,14 +128,14 @@ def handle_reservation_availability_submission(ack, body, client, view): ack(response_action='update', view=build_reservation_processing_modal()) view_id = body.get('view', view).get('id') - esb_user = resolve_esb_user(client, body['user']['id']) - if esb_user is None: - client.views_update( - view_id=view_id, - view=build_reservation_error_modal( - 'Your Slack account is not linked to an ESB user.' - ), - ) + slack_user_id = body['user']['id'] + esb_user, slack_display_name, error = _resolve_actor( + client, + slack_user_id, + resolve_reservation_owner, + ) + if error: + _update_error_modal(client, body, error) return equipment_name = equipment_service.get_equipment_display_name(equipment_id) @@ -115,12 +143,14 @@ def handle_reservation_availability_submission(ack, body, client, view): try: 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, starts_at_utc=starts_at, duration_minutes=duration_minutes, notes=notes, created_via='slack', - actor_user_id=esb_user.id, + actor_user_id=esb_user.id if esb_user else None, + owner_slack_user_id=None if esb_user else slack_user_id, + owner_slack_display_name=None if esb_user else slack_display_name, ) except ValidationError as e: client.views_update( @@ -198,16 +228,19 @@ def handle_reservation_view_mine(ack, body, client): from esb.services import reservation_read_service from esb.slack.reservation_forms import build_my_reservations_modal - esb_user = resolve_esb_user(client, body['user']['id']) - if esb_user is None: - _update_error_modal( - client, - body, - 'Your Slack account is not linked to an ESB user.', - ) + slack_user_id = body['user']['id'] + esb_user, _display_name, error = _resolve_actor( + client, + slack_user_id, + resolve_reservation_owner, + ) + if error: + _update_error_modal(client, body, error) return - - reservations = reservation_read_service.list_user_upcoming_reservations(esb_user.id) + reservations = reservation_read_service.list_user_upcoming_reservations( + esb_user.id if esb_user else None, + slack_user_id, + ) client.views_update( view_id=body['view']['id'], view=build_my_reservations_modal(reservations), @@ -240,17 +273,21 @@ def handle_reservation_cancel_start(ack, body, client): from esb.services import reservation_read_service from esb.slack.reservation_forms import build_cancel_reservation_modal - esb_user = resolve_esb_user(client, body['user']['id']) - if esb_user is None: - _update_error_modal( - client, - body, - 'Your Slack account is not linked to an ESB user.', - ) + slack_user_id = body['user']['id'] + esb_user, _display_name, error = _resolve_actor( + client, + slack_user_id, + resolve_reservation_owner, + ) + if error: + _update_error_modal(client, body, error) return - reservation_id = int(body['actions'][0]['value']) - reservation = reservation_read_service.get_user_reservation(reservation_id, esb_user.id) + reservation = reservation_read_service.get_user_reservation( + reservation_id, + esb_user.id if esb_user else None, + slack_user_id, + ) if reservation is None or reservation.status != 'active': _update_error_modal( client, @@ -271,16 +308,19 @@ def handle_reservation_cancel_keep(ack, body, client): from esb.services import reservation_read_service from esb.slack.reservation_forms import build_my_reservations_modal - esb_user = resolve_esb_user(client, body['user']['id']) - if esb_user is None: - _update_error_modal( - client, - body, - 'Your Slack account is not linked to an ESB user.', - ) + slack_user_id = body['user']['id'] + esb_user, _display_name, error = _resolve_actor( + client, + slack_user_id, + resolve_reservation_owner, + ) + if error: + _update_error_modal(client, body, error) return - - reservations = reservation_read_service.list_user_upcoming_reservations(esb_user.id) + reservations = reservation_read_service.list_user_upcoming_reservations( + esb_user.id if esb_user else None, + slack_user_id, + ) client.views_update( view_id=body['view']['id'], view=build_my_reservations_modal(reservations), @@ -294,17 +334,21 @@ def handle_reservation_cancel_confirm(ack, body, client): from esb.slack.reservation_forms import build_reservation_canceled_modal from esb.utils.exceptions import ValidationError - esb_user = resolve_esb_user(client, body['user']['id']) - if esb_user is None: - _update_error_modal( - client, - body, - 'Your Slack account is not linked to an ESB user.', - ) + slack_user_id = body['user']['id'] + esb_user, _display_name, error = _resolve_actor( + client, + slack_user_id, + resolve_reservation_owner, + ) + if error: + _update_error_modal(client, body, error) return - reservation_id = int(body['actions'][0]['value']) - reservation = reservation_read_service.get_user_reservation(reservation_id, esb_user.id) + reservation = reservation_read_service.get_user_reservation( + reservation_id, + esb_user.id if esb_user else None, + slack_user_id, + ) if reservation is None: _update_error_modal( client, @@ -316,7 +360,9 @@ def handle_reservation_cancel_confirm(ack, body, client): try: canceled = reservation_service.cancel_reservation( reservation.id, - esb_user.id, + esb_user.id if esb_user else None, + actor_slack_user_id=slack_user_id, + require_owner=True, ) except ValidationError as e: _update_error_modal(client, body, str(e)) diff --git a/esb/templates/admin/reservation_form.html b/esb/templates/admin/reservation_form.html index ce4bbe6..d3f1c41 100644 --- a/esb/templates/admin/reservation_form.html +++ b/esb/templates/admin/reservation_form.html @@ -15,7 +15,8 @@

{{ title|default('New Reservation') }}

- {{ form.reservation_type(class="form-select" ~ (" is-invalid" if form.reservation_type.errors else "")) }} + {{ form.reservation_type(class="form-select" ~ (" is-invalid" if form.reservation_type.errors else ""), disabled=slack_owner is not none) }} + {% if slack_owner %}{% endif %} {% for error in form.reservation_type.errors %}
{{ error }}
{% endfor %}
@@ -24,8 +25,14 @@

{{ title|default('New Reservation') }}

{% for error in form.equipment_id.errors %}
{{ error }}
{% endfor %}
- + {% if slack_owner %} + +
{{ slack_owner }}
+ + {% else %} + {{ form.owner_user_id(class="form-select" ~ (" is-invalid" if form.owner_user_id.errors else "")) }} + {% endif %} {% for error in form.owner_user_id.errors %}
{{ error }}
{% endfor %}
diff --git a/esb/views/admin_reservations.py b/esb/views/admin_reservations.py index 56fc797..42c9b57 100644 --- a/esb/views/admin_reservations.py +++ b/esb/views/admin_reservations.py @@ -137,11 +137,15 @@ def _set_admin_reservation_choices(form): ] -def _admin_reservation_command_from_form(form, starts_at_utc): +def _admin_reservation_command_from_form(form, starts_at_utc, original=None): reservation_type = form.reservation_type.data return { "equipment_id": form.equipment_id.data, - "owner_user_id": form.owner_user_id.data if reservation_type == RESERVATION_TYPE_MEMBER else None, + "owner_user_id": ( + None + if original and original.is_slack_owned + else form.owner_user_id.data if reservation_type == RESERVATION_TYPE_MEMBER else None + ), "starts_at_utc": starts_at_utc, "duration_minutes": form.duration_minutes.data, "notes": form.notes.data.strip(), @@ -167,6 +171,7 @@ def _admin_reservation_command_payload(command): def _admin_reservation_form_response(original=None): form = AdminReservationCreateForm() + form.allow_slack_owner = bool(original and original.is_slack_owned) _set_admin_reservation_choices(form) if request.method == "GET": _populate_admin_reservation_form(form, original) @@ -177,13 +182,18 @@ def _admin_reservation_form_response(original=None): except ValidationError as error: form.start_time.errors.append(str(error)) else: - command = _admin_reservation_command_from_form(form, starts_at_utc) + command = _admin_reservation_command_from_form(form, starts_at_utc, original) response = _review_or_persist_admin_reservation(command, form, original) if response is not None: return response title = "Edit Reservation" if original is not None else "New Reservation" - return render_template("admin/reservation_form.html", form=form, title=title) + return render_template( + "admin/reservation_form.html", + form=form, + title=title, + slack_owner=original.owner_display_name if original and original.is_slack_owned else None, + ) def _populate_admin_reservation_form(form, original=None): @@ -241,7 +251,14 @@ def _reservation_confirmation_serializer(): def _render_admin_reservation_confirmation(*, command, violation_codes, violations, replacement_reservation_id=None): equipment = equipment_service.get_equipment(command["equipment_id"]) member_label = "No member (admin hold)" - if command["owner_user_id"] is not None: + 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 = original.owner_display_name + elif command["owner_user_id"] is not None: member_label = user_service.get_user(command["owner_user_id"]).display_name payload = _admin_reservation_command_payload(command) | { "actor_user_id": current_user.id, diff --git a/migrations/versions/f6a7b8c9d0e1_add_slack_reservation_owners.py b/migrations/versions/f6a7b8c9d0e1_add_slack_reservation_owners.py new file mode 100644 index 0000000..ffbfe71 --- /dev/null +++ b/migrations/versions/f6a7b8c9d0e1_add_slack_reservation_owners.py @@ -0,0 +1,50 @@ +"""add Slack reservation owners + +Revision ID: f6a7b8c9d0e1 +Revises: e5f6a7b8c9d0 +""" + +from alembic import op +import sqlalchemy as sa + + +revision = 'f6a7b8c9d0e1' +down_revision = 'e5f6a7b8c9d0' +branch_labels = None +depends_on = None + + +def upgrade(): + with op.batch_alter_table('reservations', schema=None) as batch_op: + batch_op.drop_constraint('ck_reservations_type_owner', type_='check') + batch_op.add_column(sa.Column('slack_user_id', sa.String(length=32), nullable=True)) + batch_op.add_column(sa.Column('slack_display_name', sa.String(length=80), nullable=True)) + batch_op.create_index('ix_reservations_slack_user_id', ['slack_user_id'], unique=False) + batch_op.create_check_constraint( + 'ck_reservations_type_owner', + "(reservation_type = 'member' AND " + "((user_id IS NOT NULL AND slack_user_id IS NULL AND slack_display_name IS NULL) " + "OR (user_id IS NULL AND slack_user_id IS NOT NULL AND slack_display_name IS NOT NULL))) " + "OR (reservation_type = 'admin_hold' AND user_id IS NULL " + "AND slack_user_id IS NULL AND slack_display_name IS NULL)", + ) + + +def downgrade(): + bind = op.get_bind() + slack_reservations = bind.execute( + sa.text("SELECT COUNT(*) FROM reservations WHERE slack_user_id IS NOT NULL") + ).scalar_one() + if slack_reservations: + raise RuntimeError('Cannot downgrade while Slack-owned reservations exist.') + + with op.batch_alter_table('reservations', schema=None) as batch_op: + batch_op.drop_constraint('ck_reservations_type_owner', type_='check') + batch_op.drop_index('ix_reservations_slack_user_id') + batch_op.drop_column('slack_display_name') + batch_op.drop_column('slack_user_id') + batch_op.create_check_constraint( + 'ck_reservations_type_owner', + "(reservation_type = 'member' AND user_id IS NOT NULL) " + "OR (reservation_type = 'admin_hold' AND user_id IS NULL)", + ) diff --git a/tests/test_models/test_reservation.py b/tests/test_models/test_reservation.py index cfc3684..8315586 100644 --- a/tests/test_models/test_reservation.py +++ b/tests/test_models/test_reservation.py @@ -123,6 +123,24 @@ def test_create_active_reservation(self, app, make_equipment, staff_user): assert reservation.reservation_type == RESERVATION_TYPE_MEMBER assert reservation.overridden_policy_codes == [] + def test_slack_owned_member_reservation(self, app, make_equipment): + equipment = make_equipment(name='Slack-owned Tool') + reservation = Reservation( + equipment_id=equipment.id, + slack_user_id='U123', + slack_display_name='Alex Maker', + starts_at=datetime(2026, 6, 15, 14, 0, tzinfo=UTC), + ends_at=datetime(2026, 6, 15, 15, 0, tzinfo=UTC), + created_via='slack', + ) + + _db.session.add(reservation) + _db.session.commit() + + assert reservation.user is None + assert reservation.is_slack_owned is True + assert reservation.owner_display_name == 'Alex Maker (Slack)' + def test_admin_hold_has_no_member_and_tracks_creator( self, app, make_equipment, staff_user, ): diff --git a/tests/test_services/test_notification_service.py b/tests/test_services/test_notification_service.py index 0d184c4..8f49cd5 100644 --- a/tests/test_services/test_notification_service.py +++ b/tests/test_services/test_notification_service.py @@ -1533,6 +1533,56 @@ def fake_poll(batch_size=100): class TestReservationNotifications: + def test_queues_slack_id_addressed_dm_for_slack_owner( + self, app, make_equipment, + ): + app.config['SLACK_BOT_TOKEN'] = 'xoxb-test' + equipment = make_equipment(name='Slack Notification Tool') + reservation = Reservation( + equipment_id=equipment.id, + slack_user_id='U-NOTIFY', + slack_display_name='Slack Maker', + starts_at=datetime(2026, 6, 15, 13, 0), + ends_at=datetime(2026, 6, 15, 14, 0), + created_via='slack', + ) + _db.session.add(reservation) + _db.session.commit() + + warning = notification_service.queue_member_reservation_notification( + reservation, + 'reservation_updated', + ) + + notification = _db.session.execute( + _db.select(PendingNotification).filter_by(notification_type='slack_dm') + ).scalar_one() + assert warning is None + assert notification.target == 'U-NOTIFY' + assert notification.payload['recipient_slack_user_id'] == 'U-NOTIFY' + assert notification.payload['recipient_email'] is None + + def test_delivers_slack_owner_dm_without_email_lookup(self, app): + app.config['SLACK_BOT_TOKEN'] = 'xoxb-test' + notification = _create_notification( + notification_type='slack_dm', + target='U-DIRECT', + payload={ + 'event_type': 'reservation_updated', + 'recipient_slack_user_id': 'U-DIRECT', + 'equipment_name': 'Laser', + }, + ) + with patch('slack_sdk.WebClient') as web_client: + client = web_client.return_value + client.conversations_open.return_value = {'channel': {'id': 'D-DIRECT'}} + + notification_service._deliver_slack_dm(notification) + + client.users_lookupByEmail.assert_not_called() + client.conversations_open.assert_called_once_with(users=['U-DIRECT']) + client.chat_postMessage.assert_called_once() + def test_queues_email_addressed_dm_after_member_mutation( self, app, make_equipment, staff_user, ): diff --git a/tests/test_services/test_reservation_service.py b/tests/test_services/test_reservation_service.py index 404bace..491ad7b 100644 --- a/tests/test_services/test_reservation_service.py +++ b/tests/test_services/test_reservation_service.py @@ -21,6 +21,29 @@ class TestCreateReservation: + def test_creates_slack_owned_reservation_without_esb_user( + self, app, make_equipment, monkeypatch, + ): + _freeze_now(monkeypatch) + equipment = make_equipment(name='Slack-only Laser') + _settings(equipment) + + reservation = reservation_service.create_reservation( + equipment_id=equipment.id, + owner_user_id=None, + owner_slack_user_id=' U-SLACK ', + owner_slack_display_name=' Slack Maker ', + starts_at_utc=datetime(2026, 6, 15, 13, 0, tzinfo=UTC), + duration_minutes=60, + notes=None, + created_via='slack', + ) + + assert reservation.user_id is None + assert reservation.slack_user_id == 'U-SLACK' + assert reservation.slack_display_name == 'Slack Maker' + assert reservation.created_by_user_id is None + def test_creates_reservation_and_stores_utc_naive_time( self, app, make_equipment, staff_user, monkeypatch, ): @@ -1019,6 +1042,27 @@ def test_returns_only_active_future_reservations( assert reservations == [future] + def test_returns_slack_reservations_after_esb_account_is_linked( + self, app, make_equipment, staff_user, monkeypatch, + ): + _freeze_now(monkeypatch) + equipment = make_equipment(name='Linked Later Tool') + reservation = Reservation( + equipment_id=equipment.id, + slack_user_id='U-LINKED-LATER', + slack_display_name='Future Member', + starts_at=datetime(2026, 6, 15, 13, 0), + ends_at=datetime(2026, 6, 15, 14, 0), + created_via='slack', + ) + _db.session.add(reservation) + _db.session.commit() + + assert reservation_read_service.list_user_upcoming_reservations( + staff_user.id, + 'U-LINKED-LATER', + ) == [reservation] + class TestGetUserReservation: def test_returns_reservation_owned_by_user(self, app, make_equipment, staff_user): @@ -1047,3 +1091,35 @@ def test_returns_none_for_other_user_reservation( result = reservation_read_service.get_user_reservation(reservation.id, staff_user.id) assert result is None + + def test_slack_owner_can_cancel_but_another_slack_user_cannot( + self, app, make_equipment, + ): + equipment = make_equipment(name='Slack Cancel Tool') + reservation = Reservation( + equipment_id=equipment.id, + slack_user_id='U-OWNER', + slack_display_name='Owner', + starts_at=datetime(2026, 6, 15, 13, 0), + ends_at=datetime(2026, 6, 15, 14, 0), + created_via='slack', + ) + _db.session.add(reservation) + _db.session.commit() + + with pytest.raises(ValidationError, match='no longer available'): + reservation_service.cancel_reservation( + reservation.id, + None, + actor_slack_user_id='U-OTHER', + require_owner=True, + ) + + canceled = reservation_service.cancel_reservation( + reservation.id, + None, + actor_slack_user_id='U-OWNER', + require_owner=True, + ) + assert canceled.status == 'canceled' + assert canceled.canceled_by_user_id is None diff --git a/tests/test_slack/test_handlers.py b/tests/test_slack/test_handlers.py index 8a9caa0..bfd4d75 100644 --- a/tests/test_slack/test_handlers.py +++ b/tests/test_slack/test_handlers.py @@ -141,6 +141,9 @@ def test_reserve_command_calls_ack_and_opens_landing_modal(self): """/esb-reserve opens Flow 1 populated from reservation database data.""" ack = MagicMock() client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': 'missing@example.test'}}, + } body = { 'trigger_id': 'T123', 'user_id': 'U123', @@ -203,6 +206,9 @@ def test_reserve_landing_hides_availability_buttons_without_public_url(self): self.app.config['STATIC_PAGE_PUBLIC_URL'] = '' ack = MagicMock() client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': 'missing@example.test'}}, + } body = { 'trigger_id': 'T123', 'user_id': 'U123', @@ -222,6 +228,26 @@ def test_reserve_landing_hides_availability_buttons_without_public_url(self): for block in action_blocks: assert [element['text']['text'] for element in block['elements']] == ['Reserve'] + def test_reserve_command_blocks_inactive_esb_user(self): + self.user.is_active = False + self.db.session.commit() + ack = MagicMock() + client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': self.user.email}}, + } + body = { + 'trigger_id': 'T123', + 'user_id': 'U123', + 'channel_id': 'C123', + } + + self.handlers['command:/esb-reserve'](ack=ack, body=body, client=client) + + modal = client.views_open.call_args.kwargs['view'] + assert modal['callback_id'] == 'reservation_error' + assert 'account is inactive' in modal['blocks'][0]['text']['text'] + def test_reserve_button_updates_to_one_tool_availability_modal(self): """Flow 2: clicking Reserve updates to the selected tool availability modal.""" ack = MagicMock() @@ -469,6 +495,78 @@ def users_info_side_effect(user): assert reservations[0].created_via == 'slack' assert actions[0]['value'] == str(reservations[0].id) + def test_unlinked_slack_user_can_create_reservation(self): + start_timestamp, end_timestamp = self._future_aligned_window() + ack = MagicMock() + client = MagicMock() + client.users_info.return_value = { + 'user': { + 'name': 'slackmaker', + 'profile': {'email': 'missing@example.test', 'display_name': 'Slack Maker'}, + }, + } + body = {'user': {'id': 'U-SLACK'}, 'view': {'id': 'V123'}} + view = self._reservation_submission_view(self.laser.id, start_timestamp, end_timestamp) + + self.handlers['view:reservation_availability'](ack=ack, body=body, client=client, view=view) + + modal = client.views_update.call_args.kwargs['view'] + reservation = Reservation.query.filter_by(equipment_id=self.laser.id).one() + assert modal['callback_id'] == 'reservation_confirmation' + assert reservation.user_id is None + assert reservation.slack_user_id == 'U-SLACK' + assert reservation.slack_display_name == 'Slack Maker' + + def test_profile_lookup_failure_shows_error_without_creating_reservation(self): + start_timestamp, end_timestamp = self._future_aligned_window() + ack = MagicMock() + client = MagicMock() + client.users_info.side_effect = RuntimeError('Slack unavailable') + body = {'user': {'id': 'U-FALLBACK'}, 'view': {'id': 'V123'}} + view = self._reservation_submission_view(self.laser.id, start_timestamp, end_timestamp) + + self.handlers['view:reservation_availability'](ack=ack, body=body, client=client, view=view) + + modal = client.views_update.call_args.kwargs['view'] + assert modal['callback_id'] == 'reservation_error' + assert 'Unable to verify your Slack account' in modal['blocks'][0]['text']['text'] + assert Reservation.query.filter_by(equipment_id=self.laser.id).count() == 0 + + def test_profile_lookup_failure_does_not_show_empty_reservation_list(self): + ack = MagicMock() + client = MagicMock() + client.users_info.side_effect = RuntimeError('Slack unavailable') + body = { + 'user': {'id': 'U123'}, + 'view': {'id': 'V123'}, + 'actions': [{'value': 'stub-my-reservations'}], + } + + self.handlers['action:reservation_view_mine'](ack=ack, body=body, client=client) + + modal = client.views_update.call_args.kwargs['view'] + assert modal['callback_id'] == 'reservation_error' + assert 'Unable to verify your Slack account' in modal['blocks'][0]['text']['text'] + + def test_inactive_esb_user_cannot_create_reservation(self): + self.user.is_active = False + self.db.session.commit() + start_timestamp, end_timestamp = self._future_aligned_window() + ack = MagicMock() + client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': self.user.email}}, + } + body = {'user': {'id': 'U-INACTIVE'}, 'view': {'id': 'V123'}} + view = self._reservation_submission_view(self.laser.id, start_timestamp, end_timestamp) + + self.handlers['view:reservation_availability'](ack=ack, body=body, client=client, view=view) + + modal = client.views_update.call_args.kwargs['view'] + assert modal['callback_id'] == 'reservation_error' + assert 'account is inactive' in modal['blocks'][0]['text']['text'] + assert Reservation.query.filter_by(equipment_id=self.laser.id).count() == 0 + def test_reservation_submission_updates_modal_to_unavailable_on_conflict(self): """Flow 3: conflicting reservation submit shows retry modal.""" start_timestamp, end_timestamp = self._future_aligned_window(hours_from_now=4) @@ -589,8 +687,7 @@ def test_view_my_reservations_pushes_upcoming_reservations_modal(self): assert footer_actions['elements'][0]['text']['text'] == 'Reserve another tool' assert footer_actions['elements'][0]['action_id'] == 'reservation_reserve_another' - def test_view_my_reservations_unlinked_user_updates_to_error_modal(self): - """Flow 4: modal action errors do not require a Slack channel.""" + def test_view_my_reservations_unlinked_user_shows_empty_modal(self): ack = MagicMock() client = MagicMock() client.users_info.return_value = { @@ -608,9 +705,29 @@ def test_view_my_reservations_unlinked_user_updates_to_error_modal(self): client.chat_postEphemeral.assert_not_called() client.views_update.assert_called_once() assert client.views_update.call_args.kwargs['view_id'] == 'V123' + modal = client.views_update.call_args.kwargs['view'] + assert modal['callback_id'] == 'reservation_mine' + assert 'do not have any upcoming reservations' in modal['blocks'][0]['text']['text'] + + def test_inactive_esb_user_cannot_view_reservations(self): + self.user.is_active = False + self.db.session.commit() + ack = MagicMock() + client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': self.user.email}}, + } + body = { + 'user': {'id': 'U123'}, + 'view': {'id': 'V123'}, + 'actions': [{'value': 'stub-my-reservations'}], + } + + self.handlers['action:reservation_view_mine'](ack=ack, body=body, client=client) + modal = client.views_update.call_args.kwargs['view'] assert modal['callback_id'] == 'reservation_error' - assert 'Your Slack account is not linked to an ESB user.' in modal['blocks'][0]['text']['text'] + assert 'account is inactive' in modal['blocks'][0]['text']['text'] def test_reserve_another_tool_updates_to_landing_modal(self): """Flow 4 footer action returns to Flow 1 tool selection.""" @@ -787,6 +904,66 @@ def test_cancel_confirmation_cancels_reservation_and_shows_result(self): assert actions[1]['text']['text'] == 'View availability' assert actions[1]['url'] == 'http://example.test/status' + def test_unlinked_slack_owner_can_cancel(self): + start_timestamp, end_timestamp = self._future_aligned_window(hours_from_now=5) + reservation = Reservation( + equipment_id=self.laser.id, + slack_user_id='U-SLACK-CANCEL', + slack_display_name='Slack Canceler', + starts_at=datetime.fromtimestamp(start_timestamp, UTC).replace(tzinfo=None), + ends_at=datetime.fromtimestamp(end_timestamp, UTC).replace(tzinfo=None), + created_via='slack', + ) + self.db.session.add(reservation) + self.db.session.commit() + ack = MagicMock() + client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': 'missing@example.test', 'display_name': 'Slack Canceler'}}, + } + body = { + 'user': {'id': 'U-SLACK-CANCEL'}, + 'view': {'id': 'V123'}, + 'actions': [{'value': str(reservation.id)}], + } + + self.handlers['action:reservation_cancel_confirm'](ack=ack, body=body, client=client) + + self.db.session.refresh(reservation) + assert reservation.status == 'canceled' + assert reservation.canceled_by_user_id is None + assert client.views_update.call_args.kwargs['view']['callback_id'] == 'reservation_canceled' + + def test_inactive_esb_user_cannot_cancel_reservation(self): + self.user.is_active = False + reservation = Reservation( + equipment_id=self.laser.id, + user_id=self.user.id, + starts_at=datetime.now(UTC).replace(tzinfo=None) + timedelta(hours=5), + ends_at=datetime.now(UTC).replace(tzinfo=None) + timedelta(hours=6), + created_via='slack', + ) + self.db.session.add(reservation) + self.db.session.commit() + ack = MagicMock() + client = MagicMock() + client.users_info.return_value = { + 'user': {'profile': {'email': self.user.email}}, + } + body = { + 'user': {'id': 'U123'}, + 'view': {'id': 'V123'}, + 'actions': [{'value': str(reservation.id)}], + } + + self.handlers['action:reservation_cancel_confirm'](ack=ack, body=body, client=client) + + self.db.session.refresh(reservation) + assert reservation.status == 'active' + modal = client.views_update.call_args.kwargs['view'] + assert modal['callback_id'] == 'reservation_error' + assert 'account is inactive' in modal['blocks'][0]['text']['text'] + class TestProblemReportSubmission: """Tests for problem_report_submission view handler.""" diff --git a/tests/test_views/test_admin_reservation_views.py b/tests/test_views/test_admin_reservation_views.py index 0f87365..a5ae3d7 100644 --- a/tests/test_views/test_admin_reservation_views.py +++ b/tests/test_views/test_admin_reservation_views.py @@ -480,6 +480,78 @@ def test_edit_replaces_original_and_preserves_lineage( assert replacement.notes == "Replacement admin note" assert replacement.created_by_user_id == staff_user.id + def test_edit_preserves_slack_only_owner( + self, + staff_client, + staff_user, + make_equipment, + ): + equipment = make_equipment(name="Slack Edit Tool") + self._settings(equipment) + data = self._data(equipment, 0) + local_start = datetime.strptime( + f"{data['start_date']} {data['start_time']}", + "%Y-%m-%d %H:%M", + ).replace(tzinfo=MAKERSPACE_TIMEZONE) + original = Reservation( + equipment_id=equipment.id, + slack_user_id="U-ADMIN-EDIT", + slack_display_name="Slack Owner", + starts_at=local_start.astimezone(UTC).replace(tzinfo=None), + ends_at=(local_start + timedelta(minutes=60)).astimezone(UTC).replace(tzinfo=None), + created_via="slack", + ) + _db.session.add(original) + _db.session.commit() + + get_response = staff_client.get(f"/admin/reservations/{original.id}/edit") + response = staff_client.post( + f"/admin/reservations/{original.id}/edit", + data=self._data(equipment, 0, notes="Adjusted by staff"), + ) + + replacement = _db.session.execute( + _db.select(Reservation).filter_by(replaces_reservation_id=original.id) + ).scalar_one() + assert get_response.status_code == 200 + assert b"Slack Owner (Slack)" in get_response.data + assert response.status_code == 302 + assert replacement.user_id is None + assert replacement.slack_user_id == "U-ADMIN-EDIT" + assert replacement.slack_display_name == "Slack Owner" + + def test_slack_owner_confirmation_has_one_slack_suffix( + self, + staff_client, + make_equipment, + ): + equipment = make_equipment(name="Slack Confirmation Tool") + self._settings(equipment, min_duration=90) + data = self._data(equipment, 0) + local_start = datetime.strptime( + f"{data['start_date']} {data['start_time']}", + "%Y-%m-%d %H:%M", + ).replace(tzinfo=MAKERSPACE_TIMEZONE) + original = Reservation( + equipment_id=equipment.id, + slack_user_id="U-CONFIRM", + slack_display_name="Slack Owner", + starts_at=local_start.astimezone(UTC).replace(tzinfo=None), + ends_at=(local_start + timedelta(minutes=60)).astimezone(UTC).replace(tzinfo=None), + created_via="slack", + ) + _db.session.add(original) + _db.session.commit() + + response = staff_client.post( + f"/admin/reservations/{original.id}/edit", + data=self._data(equipment, 0, notes="Confirm Slack owner"), + ) + + assert response.status_code == 200 + assert b"Slack Owner (Slack)" in response.data + assert b"Slack Owner (Slack) (Slack)" not in response.data + def test_cancel_requires_confirmation_and_is_idempotent( self, tech_client,