Fix duplicate external (#79)

* ensure external id is unique globally * ensure time between rotations is positive * use delimiter to distinguish view id and objectid * fix tests * save positive rotation time in db * remove unnecessary abs assertion * fix mypy * extend readme * refactoring * try different permissions, better tests * hmm * black --------- Co-authored-by: Kerem Dokuz <[email protected]>
Germandrummer92 · Sep 7, 2023 · 2d7dbb0 · 2d7dbb0
1 parent eadaa3c
commit 2d7dbb0
Show file tree

Hide file tree

Showing 7 changed files with 68 additions and 25 deletions.
diff --git a/README.md b/README.md
@@ -37,6 +37,7 @@ Your One-Stop-Shop for setting up rotating :calendar: schedules.
 * Copy & paste app_manifest.yml
 ![Image of creating a slack app](https://github.com/Germandrummer92/SchedSlackBot/raw/main/assets/slack_bot_creation.png "Creating a Slack bot")
 * Adjust the request url to something valid (either ngrok or already final k8s deployment url) (you can change it later)
+* Make sure the url is verified in the App Manifest section. (it should have the prefix /slack/events)
 * Install to your workspace
 ![Image of installing the slack bot](https://github.com/Germandrummer92/SchedSlackBot/raw/main/assets/install.png "Installing the Slack bot")
 * Copy the Slack_Bot_Token and Slack_Signing_Secret for local development or deployment

diff --git a/sched_slack_bot/model/schedule.py b/sched_slack_bot/model/schedule.py
@@ -10,13 +10,14 @@
 from sched_slack_bot.utils.slack_typing_stubs import SlackState, SlackBody
 from sched_slack_bot.views.schedule_dialog_block_ids import (
     DISPLAY_NAME_BLOCK_ID,
+    SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER,
     USERS_INPUT_BLOCK_ID,
     CHANNEL_INPUT_BLOCK_ID,
     DatetimeSelectorType,
     get_datetime_block_ids,
     FIRST_ROTATION_LABEL,
     SECOND_ROTATION_LABEL,
-    CREATE_NEW_SCHEDULE_VIEW_ID,
+    CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX,
 )
 
 logger = logging.getLogger(__name__)
@@ -144,12 +145,19 @@ def from_modal_submission(cls, submission_body: SlackBody) -> Schedule:
             state=state, date_input_block_ids=get_datetime_block_ids(label=SECOND_ROTATION_LABEL)
         )
 
-        time_between_rotations = second_rotation - next_rotation
+        time_between_rotations = abs(second_rotation - next_rotation)
 
         schedule_id = submission_body["view"]["external_id"]
 
-        if schedule_id == CREATE_NEW_SCHEDULE_VIEW_ID:
+        if schedule_id.startswith(CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX):
             schedule_id = str(uuid.uuid4())
+        elif SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER in schedule_id:
+            schedule_id = schedule_id.split(SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER)[0]
+        else:
+            raise ValueError(
+                f"external id of schedule doesn't contain delimiter '{SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER}'"
+                f"nor prefix '{CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX}', received instead: '{schedule_id}'"
+            )
 
         return Schedule(
             id=schedule_id,

diff --git a/sched_slack_bot/views/schedule_dialog.py b/sched_slack_bot/views/schedule_dialog.py
@@ -2,6 +2,7 @@
 
 from enum import StrEnum
 from typing import Optional, Dict
+import uuid
 
 from slack_sdk.models.blocks import (
     HeaderBlock,
@@ -19,11 +20,12 @@
 from sched_slack_bot.views.schedule_dialog_block_ids import (
     DISPLAY_NAME_BLOCK_ID,
     CHANNEL_INPUT_BLOCK_ID,
+    SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER,
     USERS_INPUT_BLOCK_ID,
     FIRST_ROTATION_LABEL,
     SECOND_ROTATION_LABEL,
     DatetimeSelectorType,
-    CREATE_NEW_SCHEDULE_VIEW_ID,
+    CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX,
 )
 
 CREATION_CHANNEL_PLACEHOLDER = "#channel"
@@ -84,7 +86,9 @@ def get_edit_schedule_block(
     schedule: Optional[Schedule] = None, callback: ScheduleDialogCallback = ScheduleDialogCallback.CREATE_DIALOG
 ) -> View:
     modal_type = CREATE_MODAL_TYPE if schedule is None else EDIT_MODAL_TYPE
-    external_id = schedule.id if schedule is not None else CREATE_NEW_SCHEDULE_VIEW_ID
+    # make sure external id is unique globally: https://github.com/slackapi/node-slack-sdk/issues/1012#issuecomment-684818059
+    external_id = schedule.id if schedule is not None else CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX
+    external_id = f"{external_id}{SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER}{str(uuid.uuid4())}"
     return View(
         type="modal",
         external_id=external_id,

diff --git a/sched_slack_bot/views/schedule_dialog_block_ids.py b/sched_slack_bot/views/schedule_dialog_block_ids.py
@@ -9,7 +9,9 @@
 FIRST_ROTATION_LABEL = "First Rotation Reminder/Rotation"
 SECOND_ROTATION_LABEL = "Second Rotation Reminder/Rotation"
 
-CREATE_NEW_SCHEDULE_VIEW_ID = "NEW_DIALOG"
+CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX = "NEW_DIALOG"
+
+SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER = "::"
 
 
 class DatetimeSelectorType(Enum):

diff --git a/test_sched_slack_bot/model/test_schedule.py b/test_sched_slack_bot/model/test_schedule.py
@@ -17,6 +17,8 @@
     get_datetime_block_ids,
     FIRST_ROTATION_LABEL,
     SECOND_ROTATION_LABEL,
+    SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER,
+    CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX,
 )
 
 
@@ -81,7 +83,7 @@ def _add_valid_datetime_values(
 def valid_slack_body(minimum_slack_body: SlackBody, schedule: Schedule) -> SlackBody:
     valid_slack_body = copy.deepcopy(minimum_slack_body)
 
-    valid_slack_body["view"]["external_id"] = schedule.id
+    valid_slack_body["view"]["external_id"] = f"{schedule.id}{SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER}{str(uuid.uuid4())}"
 
     # mypy cant deal with dynamic typed dicts
     valid_slack_body["view"]["state"]["values"][DISPLAY_NAME_BLOCK_ID] = {
@@ -231,7 +233,7 @@ def test_schedule_from_modal_submission_raises_with_missing_values(
         Schedule.from_modal_submission(submission_body=valid_slack_body)
 
 
-def test_schedule_from_modal_submission_works(schedule: Schedule, valid_slack_body: SlackBody) -> None:
+def test_schedule_from_modal_submission_works_for_edit(schedule: Schedule, valid_slack_body: SlackBody) -> None:
     from_modal = Schedule.from_modal_submission(submission_body=valid_slack_body)
 
     assert from_modal.display_name == schedule.display_name
@@ -241,3 +243,25 @@ def test_schedule_from_modal_submission_works(schedule: Schedule, valid_slack_bo
     assert from_modal.current_index == schedule.current_index
     assert from_modal.time_between_rotations == schedule.time_between_rotations
     assert from_modal.id == schedule.id
+
+
+def test_schedule_from_modal_submission_works_for_create(schedule: Schedule, valid_slack_body: SlackBody) -> None:
+    valid_slack_body["view"][
+        "external_id"
+    ] = f"{CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX}{SCHEDULE_VIEW_ID_SCHEDULE_ID_DELIMITER}{str(uuid.uuid4())}"
+
+    from_modal = Schedule.from_modal_submission(submission_body=valid_slack_body)
+
+    assert from_modal.display_name == schedule.display_name
+    assert from_modal.members == schedule.members
+    assert from_modal.channel_id_to_notify_in == schedule.channel_id_to_notify_in
+    assert from_modal.next_rotation == schedule.next_rotation
+    assert from_modal.current_index == schedule.current_index
+    assert from_modal.time_between_rotations == schedule.time_between_rotations
+    assert from_modal.id != schedule.id
+
+
+def test_schedule_from_modal_submission_raises_with_wrong_external_id(schedule: Schedule, valid_slack_body: SlackBody) -> None:
+    valid_slack_body["view"]["external_id"] = "broken"
+    with pytest.raises(ValueError):
+        Schedule.from_modal_submission(submission_body=valid_slack_body)
diff --git a/test_sched_slack_bot/test_controller.py b/test_sched_slack_bot/test_controller.py
@@ -151,12 +151,14 @@ def test_handle_clicked_create_opens_create_schedule_dialog(
     schedule: Schedule,
 ) -> None:
     mocked_schedule_access.get_available_schedules.return_value = [schedule]
+    with mock.patch(target="sched_slack_bot.views.schedule_dialog.uuid", return_value="uuid"):
+        ack = mock.MagicMock()
+        controller_with_mocks.handle_clicked_create_schedule(ack=ack, body=slack_body)
 
-    ack = mock.MagicMock()
-    controller_with_mocks.handle_clicked_create_schedule(ack=ack, body=slack_body)
-
-    ack.assert_called_once()
-    mocked_slack_client.views_open.assert_called_once_with(trigger_id=slack_body["trigger_id"], view=get_edit_schedule_block())
+        ack.assert_called_once()
+        mocked_slack_client.views_open.assert_called_once_with(
+            trigger_id=slack_body["trigger_id"], view=get_edit_schedule_block()
+        )
 
 
 @pytest.mark.parametrize(
@@ -194,15 +196,15 @@ def test_handle_clicked_edit_schedule_opens_edit_dialog(
 ) -> None:
     mocked_schedule_access.get_schedule.return_value = schedule
     slack_body["actions"] = [SlackAction(action_id=EDIT_SCHEDULE_ACTION_ID, block_id=schedule.id)]
-
-    ack = mock.MagicMock()
-    controller_with_mocks.handle_clicked_edit_schedule(ack=ack, body=slack_body)
-
-    ack.assert_called_once()
-    mocked_slack_client.views_open.assert_called_once_with(
-        trigger_id=slack_body["trigger_id"],
-        view=get_edit_schedule_block(schedule=schedule, callback=ScheduleDialogCallback.EDIT_DIALOG),
-    )
+    with mock.patch(target="sched_slack_bot.views.schedule_dialog.uuid", return_value="uuid"):
+        ack = mock.MagicMock()
+        controller_with_mocks.handle_clicked_edit_schedule(ack=ack, body=slack_body)
+
+        ack.assert_called_once()
+        mocked_slack_client.views_open.assert_called_once_with(
+            trigger_id=slack_body["trigger_id"],
+            view=get_edit_schedule_block(schedule=schedule, callback=ScheduleDialogCallback.EDIT_DIALOG),
+        )
 
 
 def test_handle_submitted_create_schedule_creates_new_schedule(

diff --git a/test_sched_slack_bot/views/test_schedule_dialog.py b/test_sched_slack_bot/views/test_schedule_dialog.py
@@ -19,7 +19,7 @@
     get_second_rotation_block,
 )
 from sched_slack_bot.views.schedule_dialog_block_ids import (
-    CREATE_NEW_SCHEDULE_VIEW_ID,
+    CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX,
     FIRST_ROTATION_LABEL,
     SECOND_ROTATION_LABEL,
 )
@@ -47,7 +47,8 @@ def test_get_create_schedule_dialog() -> None:
     assert create_block.blocks[0].text is not None
     assert CREATE_MODAL_TYPE in create_block.blocks[0].text.text
 
-    assert create_block.external_id == CREATE_NEW_SCHEDULE_VIEW_ID
+    assert create_block.external_id is not None
+    assert create_block.external_id.startswith(CREATE_NEW_SCHEDULE_VIEW_ID_PREFIX)
 
 
 def test_get_edit_schedule_dialog(schedule: Schedule) -> None:
@@ -59,7 +60,8 @@ def test_get_edit_schedule_dialog(schedule: Schedule) -> None:
     assert create_block.blocks[0].text is not None
     assert EDIT_MODAL_TYPE in create_block.blocks[0].text.text
 
-    assert create_block.external_id == schedule.id
+    assert create_block.external_id is not None
+    assert create_block.external_id.startswith(schedule.id)
 
 
 def test_get_display_name_input_creation() -> None: