From dbaf99df497e693ec09a2f67be9cde1338f348b7 Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Tue, 12 May 2026 16:32:46 -0400 Subject: [PATCH] Fix review findings: IAM, routing guards, past-date check, roster safety - Ring scheduler: use DynamoDBCrudPolicy (resolve_shift needs Query) - Button pickup: update 3CX for active shift type, not just night - Pick/drop/swap commands: only update 3CX when shift type is active - Swap command: add missing past-date guard - add_roster_entry: reject if extension already exists - Apply ruff formatting --- src/shared/python/shared/blocks.py | 4 ++- src/shared/python/shared/ring_scheduler.py | 4 +-- src/shared/python/shared/schedule.py | 30 +++++++++------- src/slack-bot/app.py | 41 ++++++++++++++++++---- src/weekly-post/app.py | 4 ++- template.yaml | 7 ++-- 6 files changed, 60 insertions(+), 30 deletions(-) diff --git a/src/shared/python/shared/blocks.py b/src/shared/python/shared/blocks.py index b362057..1c3d947 100644 --- a/src/shared/python/shared/blocks.py +++ b/src/shared/python/shared/blocks.py @@ -130,7 +130,9 @@ def build_shift_change_message( """Build a channel notification for a shift change.""" dt = datetime.strptime(date_str, "%Y-%m-%d") day_label = dt.strftime("%A, %b %-d") - type_label = f" ({SHIFT_LABELS.get(shift_type, shift_type)})" if shift_type == "day" else "" + type_label = ( + f" ({SHIFT_LABELS.get(shift_type, shift_type)})" if shift_type == "day" else "" + ) if action == "picked_up": text = f":white_check_mark: <@{user_id}> picked up the *{day_label}*{type_label} shift (Ext {ext})" diff --git a/src/shared/python/shared/ring_scheduler.py b/src/shared/python/shared/ring_scheduler.py index 4c786e9..a3d4529 100644 --- a/src/shared/python/shared/ring_scheduler.py +++ b/src/shared/python/shared/ring_scheduler.py @@ -27,7 +27,5 @@ def update_queue_routing( closed_destination=extension, holiday_destination=extension, ) - logger.info( - "Updated queue %s to forward to Ext %s", queue_number, extension - ) + logger.info("Updated queue %s to forward to Ext %s", queue_number, extension) return {"extension": extension, "queue": queue_number} diff --git a/src/shared/python/shared/schedule.py b/src/shared/python/shared/schedule.py index 2644d50..40c6b83 100644 --- a/src/shared/python/shared/schedule.py +++ b/src/shared/python/shared/schedule.py @@ -145,9 +145,7 @@ class ShiftSchedule: # ── Schedule post tracking ─────────────────────────────────────────── def get_schedule_post(self, channel_id: str) -> dict | None: - resp = self.table.get_item( - Key={"PK": "SCHEDULE_POST", "SK": channel_id} - ) + resp = self.table.get_item(Key={"PK": "SCHEDULE_POST", "SK": channel_id}) return resp.get("Item") def save_schedule_post( @@ -183,16 +181,22 @@ class ShiftSchedule: config = self.get_config() return config.get("admin_users", []) - def add_roster_entry(self, extension: str, name: str) -> None: - self.table.put_item( - Item={ - "PK": "ROSTER", - "SK": extension, - "name": name, - "extension": extension, - "slack_user_id": "", - } - ) + def add_roster_entry(self, extension: str, name: str) -> bool: + """Add a new roster entry. Returns False if extension already exists.""" + try: + self.table.put_item( + Item={ + "PK": "ROSTER", + "SK": extension, + "name": name, + "extension": extension, + "slack_user_id": "", + }, + ConditionExpression="attribute_not_exists(PK)", + ) + return True + except self.table.meta.client.exceptions.ConditionalCheckFailedException: + return False def remove_roster_entry(self, extension: str) -> None: self.table.delete_item(Key={"PK": "ROSTER", "SK": extension}) diff --git a/src/slack-bot/app.py b/src/slack-bot/app.py index 583b920..9532a88 100644 --- a/src/slack-bot/app.py +++ b/src/slack-bot/app.py @@ -88,6 +88,18 @@ def is_today(date_str: str) -> bool: return date_str == datetime.now(EASTERN).strftime("%Y-%m-%d") +WEEKEND_DAYS = {"Saturday", "Sunday"} + + +def _is_active_shift_type(shift_type: str) -> bool: + """Check if the given shift type is the currently active one.""" + now = datetime.now(EASTERN) + day_name = now.strftime("%A") + if day_name in WEEKEND_DAYS and now.hour < 17: + return shift_type == "day" + return shift_type == "night" + + def create_app( bot_token: str, signing_secret: str, schedule_channel: str | None = None ) -> App: @@ -186,11 +198,15 @@ def create_app( ) return - if is_today(date_str) and shift_type == "night": + if is_today(date_str) and _is_active_shift_type(shift_type): _update_3cx_routing(employee["extension"]) blocks = build_shift_change_message( - user_id, date_str, "picked_up", employee["extension"], employee["name"], + user_id, + date_str, + "picked_up", + employee["extension"], + employee["name"], shift_type=shift_type, ) respond( @@ -375,7 +391,7 @@ def create_app( schedule.set_override(date_str, employee["extension"], employee["name"]) - if is_today(date_str): + if is_today(date_str) and _is_active_shift_type("night"): _update_3cx_routing(employee["extension"]) respond(text=f"You picked up the shift for *{date.strftime('%A, %b %-d')}*.") @@ -427,7 +443,7 @@ def create_app( schedule.mark_open(date_str) - if is_today(date_str): + if is_today(date_str) and _is_active_shift_type("night"): _update_3cx_routing(FALLBACK_EXTENSION) respond( @@ -465,6 +481,10 @@ def create_app( return date_str = date.strftime("%Y-%m-%d") + if date_str < datetime.now(EASTERN).strftime("%Y-%m-%d"): + respond(text="You can't swap a shift in the past.") + return + day_name = date.strftime("%A") ext, name, source = schedule.resolve_shift(date_str, day_name) @@ -493,7 +513,7 @@ def create_app( schedule.set_override(date_str, target["extension"], target["name"]) - if is_today(date_str): + if is_today(date_str) and _is_active_shift_type("night"): _update_3cx_routing(target["extension"]) blocks = build_shift_change_message( @@ -597,7 +617,12 @@ def create_app( return ext = parts[3] name = " ".join(parts[4:]) - schedule.add_roster_entry(ext, name) + added = schedule.add_roster_entry(ext, name) + if not added: + respond( + text=f"Extension `{ext}` already exists. Use `roster rename` to change the name." + ) + return respond(text=f"Added *{name}* (Ext {ext}) to the roster.") elif roster_cmd == "remove": @@ -630,7 +655,9 @@ def create_app( ) else: - respond(text="Unknown roster command. Use `add`, `remove`, or `rename`.") + respond( + text="Unknown roster command. Use `add`, `remove`, or `rename`." + ) else: respond(text=f"Unknown admin command: `{subcmd}`. Try `/oncall help`.") diff --git a/src/weekly-post/app.py b/src/weekly-post/app.py index f15b7cb..6f36901 100644 --- a/src/weekly-post/app.py +++ b/src/weekly-post/app.py @@ -212,7 +212,9 @@ def handler(event, context): schedule.save_schedule_post( channel_id, result["ts"], this_monday.strftime("%Y-%m-%d") ) - logger.info("Posted weekly schedule to channel %s (ts=%s)", channel_id, result["ts"]) + logger.info( + "Posted weekly schedule to channel %s (ts=%s)", channel_id, result["ts"] + ) return { "posted": True, "channel": channel_id, diff --git a/template.yaml b/template.yaml index 617dc0f..3077ac2 100644 --- a/template.yaml +++ b/template.yaml @@ -195,12 +195,9 @@ Resources: QUEUE_NUMBER: !Ref QueueNumber TZ: !Ref Timezone Policies: + - DynamoDBCrudPolicy: + TableName: !Ref ShiftTable - Statement: - - Effect: Allow - Action: - - dynamodb:GetItem - Resource: - - !GetAtt ShiftTable.Arn - Effect: Allow Action: - secretsmanager:GetSecretValue