From edfa34bfbf7e6c6bad1e87f21adce5fa5855855a Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:42:19 +0000 Subject: [PATCH] feat(portal): store an optional note on swap requests (IP-132) (#283) * feat(portal): store an optional note on swap requests (IP-132) * fix(portal): address review feedback * fix(portal): address review feedback --- openapi.yaml | 6 +++ src/shared/shared/blocks.py | 20 ++++++--- src/shared/shared/portal_http.py | 1 + src/shared/shared/portal_ops.py | 29 ++++++++++++- src/shared/shared/schedule.py | 35 ++++++++-------- src/shared/shared/side_effects.py | 9 ++++- tests/shared/test_blocks.py | 31 ++++++++++++++ tests/shared/test_portal_ops.py | 67 +++++++++++++++++++++++++++++++ 8 files changed, 173 insertions(+), 25 deletions(-) diff --git a/openapi.yaml b/openapi.yaml index aa4adac..594781a 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -542,6 +542,9 @@ components: targetExtension: type: string minLength: 1 + note: + type: string + description: Optional. Stored after trim, and the trimmed value must be 500 characters or fewer. AdminOverrideBody: type: object @@ -698,6 +701,9 @@ components: type: string incoming: type: boolean + note: + type: string + maxLength: 500 PendingPickup: type: object diff --git a/src/shared/shared/blocks.py b/src/shared/shared/blocks.py index 8e77914..9005ac5 100644 --- a/src/shared/shared/blocks.py +++ b/src/shared/shared/blocks.py @@ -254,8 +254,15 @@ def build_shift_change_message( return [{"type": "section", "text": {"type": "mrkdwn", "text": text}}] +def _mrkdwn_text(value: str) -> str: + return value.replace("&", "&").replace("<", "<").replace(">", ">") + + def build_swap_request_blocks( - requester_slack: str, date_str: str, shift_type: str = "night" + requester_slack: str, + date_str: str, + shift_type: str = "night", + note: str | None = None, ) -> list[dict]: """Build the interactive Accept / Decline message DMed to a swap target.""" dt = datetime.strptime(date_str, "%Y-%m-%d") @@ -266,15 +273,18 @@ def build_swap_request_blocks( else "" ) action_suffix = "_day" if shift_type == "day" else "" + text = ( + f"<@{requester_slack}> wants you to cover the " + f"*{day_label}*{type_label} shift. Accept to take it on." + ) + if note: + text += f"\n\nNote: {_mrkdwn_text(note)}" return [ { "type": "section", "text": { "type": "mrkdwn", - "text": ( - f"<@{requester_slack}> wants you to cover the " - f"*{day_label}*{type_label} shift. Accept to take it on." - ), + "text": text, }, }, { diff --git a/src/shared/shared/portal_http.py b/src/shared/shared/portal_http.py index 1ee7108..c555141 100644 --- a/src/shared/shared/portal_http.py +++ b/src/shared/shared/portal_http.py @@ -100,6 +100,7 @@ def dispatch( body.get("date", ""), body.get("targetExtension", ""), body.get("shiftType"), + note=body.get("note"), ) if ( method == "POST" diff --git a/src/shared/shared/portal_ops.py b/src/shared/shared/portal_ops.py index cb19da5..b9e4882 100644 --- a/src/shared/shared/portal_ops.py +++ b/src/shared/shared/portal_ops.py @@ -160,9 +160,25 @@ def snapshot(schedule: ShiftSchedule, employee: dict, week: str = "this") -> dic return payload +NOTE_MAX = 500 + + +def _clean_swap_note(note: str | None) -> str | None: + if note is None: + return None + if not isinstance(note, str): + raise ActionError(400, "INVALID_NOTE", "Note must be text.") + cleaned = note.strip() + if not cleaned: + return None + if len(cleaned) > NOTE_MAX: + raise ActionError(400, "NOTE_TOO_LONG", "Note must be 500 characters or fewer.") + return cleaned + + def _swap_payload(item: dict, my_ext: str) -> dict: date_str, shift_type = _sk_to_date_shift(item.get("SK", "")) - return { + payload = { "date": date_str, "shiftType": item.get("shift_type") or shift_type, "requesterExt": item.get("requester_ext", ""), @@ -171,6 +187,10 @@ def _swap_payload(item: dict, my_ext: str) -> dict: "targetName": item.get("target_name", ""), "incoming": item.get("target_ext") == my_ext, } + note = item.get("note") + if isinstance(note, str) and note.strip(): + payload["note"] = note.strip() + return payload def _pickup_payload(item: dict) -> dict: @@ -419,6 +439,7 @@ def swap( date_str: str, target_extension: str, shift_type: str | None, + note: str | None = None, ) -> dict: date = _parse_date(date_str) date_str = date.strftime(DATE_FMT) @@ -454,8 +475,11 @@ def swap( raise ActionError(400, "UNKNOWN_TARGET", "That extension is not on the roster.") if target["extension"] == employee["extension"]: raise ActionError(400, "SELF_SWAP", "That shift is already yours.") + cleaned_note = _clean_swap_note(note) expires_at = int(shift_start(date_str, resolved).timestamp()) - schedule.create_pending_swap(date_str, resolved, employee, target, expires_at) + schedule.create_pending_swap( + date_str, resolved, employee, target, expires_at, note=cleaned_note + ) token = effects.slack_token() if target.get("slack_user_id"): effects.dm_swap_request( @@ -465,6 +489,7 @@ def swap( date_str, resolved, employee["name"], + note=cleaned_note, ) return { "ok": True, diff --git a/src/shared/shared/schedule.py b/src/shared/shared/schedule.py index 477b65c..ef08c39 100644 --- a/src/shared/shared/schedule.py +++ b/src/shared/shared/schedule.py @@ -261,29 +261,32 @@ class ShiftSchedule: requester: dict, target: dict, expires_at: int, + note: str | None = None, ) -> None: """Create (or supersede) a pending swap request for a shift. One swap per shift (unique SK), so a new request overwrites any prior pending one. ``expires_at`` is an epoch timestamp used for DynamoDB TTL. + ``note`` is omitted when empty so Slack ``/oncall swap`` stays unchanged. """ sk = f"{date_str}-DAY" if shift_type == "day" else date_str - self.table.put_item( - Item={ - "PK": "SWAP", - "SK": sk, - "shift_type": shift_type, - "status": "pending", - "requester_ext": requester["extension"], - "requester_name": requester["name"], - "requester_slack": requester.get("slack_user_id", ""), - "target_ext": target["extension"], - "target_name": target["name"], - "target_slack": target.get("slack_user_id", ""), - "created_at": datetime.now(EASTERN).isoformat(), - "expires_at": expires_at, - } - ) + item = { + "PK": "SWAP", + "SK": sk, + "shift_type": shift_type, + "status": "pending", + "requester_ext": requester["extension"], + "requester_name": requester["name"], + "requester_slack": requester.get("slack_user_id", ""), + "target_ext": target["extension"], + "target_name": target["name"], + "target_slack": target.get("slack_user_id", ""), + "created_at": datetime.now(EASTERN).isoformat(), + "expires_at": expires_at, + } + if note: + item["note"] = note + self.table.put_item(Item=item) def get_swap(self, date_str: str, shift_type: str = "night") -> dict | None: sk = f"{date_str}-DAY" if shift_type == "day" else date_str diff --git a/src/shared/shared/side_effects.py b/src/shared/shared/side_effects.py index b83f8a0..8f5fd51 100644 --- a/src/shared/shared/side_effects.py +++ b/src/shared/shared/side_effects.py @@ -15,6 +15,7 @@ from shared.blocks import ( build_holiday_added_blocks, build_pickup_request_blocks, build_shift_change_message, + _mrkdwn_text, build_swap_request_blocks, build_week_schedule, ) @@ -272,15 +273,19 @@ def dm_swap_request( date_str: str, shift_type: str, requester_name: str, + note: str | None = None, ) -> bool: if not token or not target_slack: return False + text = f"{requester_name} wants to swap you the {date_str} shift" + if note: + text += f"\nNote: {_mrkdwn_text(note)}" return slack_call( "chat.postMessage", token, channel=target_slack, - blocks=build_swap_request_blocks(requester_slack, date_str, shift_type), - text=f"{requester_name} wants to swap you the {date_str} shift", + blocks=build_swap_request_blocks(requester_slack, date_str, shift_type, note), + text=text, ) diff --git a/tests/shared/test_blocks.py b/tests/shared/test_blocks.py index cd25d9b..70274b2 100644 --- a/tests/shared/test_blocks.py +++ b/tests/shared/test_blocks.py @@ -259,6 +259,37 @@ class TestBuildSwapRequestBlocks: assert elements[0]["style"] == "primary" # Accept assert elements[1]["style"] == "danger" # Decline + def test_note_is_appended_without_changing_actions(self): + blocks = build_swap_request_blocks( + "U_REQ", "2026-06-03", "night", note="Family & more" + ) + assert "Note: Family <commitment> & more" in blocks[0]["text"]["text"] + ids = self._action_ids(blocks) + assert ids == ["swap_accept_2026-06-03", "swap_decline_2026-06-03"] + + +def test_dm_swap_fallback_escapes_the_note(monkeypatch): + from shared import side_effects as effects + + captured = {} + + def fake_slack_call(_method, _token, **kwargs): + captured.update(kwargs) + return True + + monkeypatch.setattr(effects, "slack_call", fake_slack_call) + assert effects.dm_swap_request( + "tok", + "U_REQ", + "U_TGT", + "2026-06-03", + "night", + "Alice", + note="ping <@U_ADMIN>", + ) + assert "Note: ping <@U_ADMIN>" in captured["text"] + assert "<@U_ADMIN>" not in captured["text"] + class TestBuildSwapResolvedBlocks: def test_renders_text_no_buttons(self): diff --git a/tests/shared/test_portal_ops.py b/tests/shared/test_portal_ops.py index 9853318..441e65f 100644 --- a/tests/shared/test_portal_ops.py +++ b/tests/shared/test_portal_ops.py @@ -75,6 +75,73 @@ def test_swap_creates_pending(schedule, seed, quiet_slack): assert snap["pendingSwaps"][0]["incoming"] is True mine = snapshot(schedule, employee) assert mine["pendingSwaps"][0]["incoming"] is False + assert "note" not in pending + assert "note" not in snap["pendingSwaps"][0] + + +def test_swap_stores_returns_and_dms_note(schedule, seed, quiet_slack, monkeypatch): + employee = _alice(schedule, seed) + seed.override("2026-06-12", "114", "Alice") + calls = [] + + def capture(*args, **kwargs): + calls.append((args, kwargs)) + return True + + monkeypatch.setattr(effects, "dm_swap_request", capture) + with freezegun.freeze_time("2026-06-08 12:00:00-04:00"): + swap( + schedule, + employee, + "2026-06-12", + "115", + "night", + note=" Family commitment ", + ) + pending = schedule.get_swap("2026-06-12", "night") + assert pending["note"] == "Family commitment" + snap = snapshot(schedule, employee) + assert snap["pendingSwaps"][0]["note"] == "Family commitment" + assert calls[0][1]["note"] == "Family commitment" + + +def test_swap_accepts_note_that_trims_to_500(schedule, seed, quiet_slack): + employee = _alice(schedule, seed) + seed.override("2026-06-12", "114", "Alice") + with freezegun.freeze_time("2026-06-08 12:00:00-04:00"): + swap(schedule, employee, "2026-06-12", "115", "night", note=f" {'a' * 500} ") + assert schedule.get_swap("2026-06-12", "night")["note"] == "a" * 500 + + +def test_swap_omits_blank_note(schedule, seed, quiet_slack): + employee = _alice(schedule, seed) + seed.override("2026-06-12", "114", "Alice") + with freezegun.freeze_time("2026-06-08 12:00:00-04:00"): + swap(schedule, employee, "2026-06-12", "115", "night", note=" ") + pending = schedule.get_swap("2026-06-12", "night") + assert "note" not in pending + snap = snapshot(schedule, employee) + assert "note" not in snap["pendingSwaps"][0] + + +def test_swap_rejects_long_note(schedule, seed, quiet_slack): + employee = _alice(schedule, seed) + seed.override("2026-06-12", "114", "Alice") + with freezegun.freeze_time("2026-06-08 12:00:00-04:00"): + with pytest.raises(ActionError) as err: + swap(schedule, employee, "2026-06-12", "115", "night", note="a" * 501) + assert err.value.code == "NOTE_TOO_LONG" + assert schedule.get_swap("2026-06-12", "night") is None + + +def test_swap_rejects_non_string_note(schedule, seed, quiet_slack): + employee = _alice(schedule, seed) + seed.override("2026-06-12", "114", "Alice") + with freezegun.freeze_time("2026-06-08 12:00:00-04:00"): + with pytest.raises(ActionError) as err: + swap(schedule, employee, "2026-06-12", "115", "night", note=12) + assert err.value.code == "INVALID_NOTE" + assert schedule.get_swap("2026-06-12", "night") is None def test_late_pickup_creates_request(schedule, seed, quiet_slack):