diff --git a/services/hackbot-api/app/slack/listeners/actions/start_agent_run.py b/services/hackbot-api/app/slack/listeners/actions/start_agent_run.py index b405f089e70..e3060753dc0 100644 --- a/services/hackbot-api/app/slack/listeners/actions/start_agent_run.py +++ b/services/hackbot-api/app/slack/listeners/actions/start_agent_run.py @@ -73,6 +73,39 @@ def _generate_replacement_block( return {"type": "context", "elements": [{"type": "mrkdwn", "text": note}]} +def _replace_in_actions_block( + block: dict, action: ButtonAction, note_block: dict +) -> list[dict] | None: + """The blocks replacing an actions block once its clicked button is gone.""" + elements = block["elements"] + filtered_elements = [e for e in elements if e["action_id"] != action.action_id] + if len(filtered_elements) == len(elements): + log.error( + "Clicked button '%s' is not in its block's elements, which is unexpected", + action.action_id, + ) + return None + if not filtered_elements: + return [note_block] + return [{**block, "elements": filtered_elements}, note_block] + + +def _replace_in_section_block( + block: dict, action: ButtonAction, note_block: dict +) -> list[dict] | None: + """The blocks replacing a section once its clicked accessory button is gone.""" + accessory = block.get("accessory") + if accessory is None or accessory.get("action_id") != action.action_id: + log.error( + "Clicked button '%s' is not its section's accessory, which is unexpected", + action.action_id, + ) + return None + section = block.copy() + del section["accessory"] + return [section, note_block] + + def _message_with_note( message: dict, action: ButtonAction, note_block: dict ) -> dict | None: @@ -85,26 +118,19 @@ def _message_with_note( for i, block in enumerate(blocks): if block["block_id"] != action.block_id: continue - if block["type"] != "actions": + if block["type"] == "actions": + replacement = _replace_in_actions_block(block, action, note_block) + elif block["type"] == "section": + replacement = _replace_in_section_block(block, action, note_block) + else: log.error( - "Clicked button '%s' is in a '%s' block, which is not supported yet", + "Clicked button '%s' is in a '%s' block, which is not supported", action.action_id, block["type"], ) return None - elements = block["elements"] - filtered_elements = [e for e in elements if e["action_id"] != action.action_id] - if len(filtered_elements) == len(elements): - log.error( - "Clicked button '%s' is not in its block's elements, which is unexpected", - action.action_id, - ) + if replacement is None: return None - replacement = ( - [{**block, "elements": filtered_elements}, note_block] - if filtered_elements - else [note_block] - ) return { "text": f"{message['text']}\n{note_block['elements'][0]['text']}".strip(), "blocks": blocks[:i] + replacement + blocks[i + 1 :], diff --git a/services/hackbot-api/tests/test_slack_listeners.py b/services/hackbot-api/tests/test_slack_listeners.py index 5e224a475b7..23a1ef63596 100644 --- a/services/hackbot-api/tests/test_slack_listeners.py +++ b/services/hackbot-api/tests/test_slack_listeners.py @@ -436,14 +436,31 @@ async def test_an_update_slack_refuses_is_logged_and_the_click_acknowledged( self.fake_ack.assert_awaited_once() assert "Slack said no" in caplog.text - async def test_a_button_outside_an_actions_block_is_logged_and_left(self, caplog): - # A button can also be a section's accessory, which is not supported yet. + async def test_a_section_accessory_button_is_replaced_with_the_note(self): + section = { + "type": "section", + "block_id": BLOCK_ID, + "text": {"type": "mrkdwn", "text": "Bug 1234"}, + "accessory": _action(), + } + self.body["message"]["blocks"] = [section] + + await self._call() + + blocks = self._updated_blocks() + assert blocks[0] == {k: v for k, v in section.items() if k != "accessory"} + assert blocks[1]["type"] == "context" + assert "<@U0CLICKER>" in blocks[1]["elements"][0]["text"] + assert RUN_ID in blocks[1]["elements"][0]["text"] + + async def test_a_section_whose_accessory_is_another_button_is_left(self, caplog): + other = {"type": "button", "action_id": "something_else", "value": "x"} self.body["message"]["blocks"] = [ { "type": "section", "block_id": BLOCK_ID, "text": {"type": "mrkdwn", "text": "Bug 1234"}, - "accessory": _action(), + "accessory": other, } ] @@ -451,7 +468,19 @@ async def test_a_button_outside_an_actions_block_is_logged_and_left(self, caplog await self._call() assert self._edits() == [] - assert "'section' block" in caplog.text + assert "not its section's accessory" in caplog.text + self.fake_ack.assert_awaited_once() + + async def test_a_button_in_an_unsupported_block_is_logged_and_left(self, caplog): + self.body["message"]["blocks"] = [ + {"type": "input", "block_id": BLOCK_ID, "element": _action()} + ] + + with caplog.at_level(logging.ERROR): + await self._call() + + assert self._edits() == [] + assert "'input' block" in caplog.text self.fake_ack.assert_awaited_once() async def test_a_message_without_the_button_is_left_alone(self):