From d5917fb9361d88234fcaf1942e840308ced48557 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 30 Aug 2023 12:32:45 +0200 Subject: [PATCH 1/3] Update Shift Swap Request Slack message formatting --- CHANGELOG.md | 6 +++ .../on-call-schedules/shift-swaps/_index.md | 10 ++--- .../slack/scenarios/shift_swap_requests.py | 45 +++++++------------ .../test_shift_swap_requests.py | 26 ++++------- 4 files changed, 36 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4bec82add..a1547b51d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Changed + +- Update Shift Swap Request Slack message formatting by @joeyorlando ([#TBD](https://github.com/grafana/oncall/pull/TBD)) + ## v1.3.29 (2023-08-29) ### Fixed diff --git a/docs/sources/on-call-schedules/shift-swaps/_index.md b/docs/sources/on-call-schedules/shift-swaps/_index.md index 97858cbf87..517c95e9a5 100644 --- a/docs/sources/on-call-schedules/shift-swaps/_index.md +++ b/docs/sources/on-call-schedules/shift-swaps/_index.md @@ -36,14 +36,14 @@ of a schedule, or clicking the button shown when hovering on a particular shift ->**Note**: no recurrence rules support is available when requesting a shift swap. If you need to recurrently change a shift, -consider creating a higher level layer rotation with the desired updates. +> **Note**: no recurrence rules support is available when requesting a shift swap. If you need to recurrently change a shift, +> consider creating a higher level layer rotation with the desired updates. Upon submitting the request, a Slack notification will be sent to the channel associated to the correspondent schedule, if there is one. A [mobile push notification][shift-swap-notifications] will be sent to team members who participate in the schedule and have the notifications enabled. - + Push notifications are sent 4 weeks ahead of the requested shift swap, or shortly after creation in case the shift swap start time is less than 4 weeks away, but always during users' working hours (by default 9am-5pm on @@ -64,8 +64,8 @@ The follow-up notifications will be sent at the following intervals before the s You can delete the swap request at any time. If the swap has been taken, it will automatically be undone upon removal. ->**Note**: if [RBAC][rbac] is enabled, a user is required to have the `SCHEDULES_WRITE` permission to create, -update, take or delete a swap request. `SCHEDULES_READ` will be enough to get details about existing requests. +> **Note**: if [RBAC][rbac] is enabled, a user is required to have the `SCHEDULES_WRITE` permission to create, +> update, take or delete a swap request. `SCHEDULES_READ` will be enough to get details about existing requests. ## Check existing swap requests diff --git a/engine/apps/slack/scenarios/shift_swap_requests.py b/engine/apps/slack/scenarios/shift_swap_requests.py index 525ab7303b..c82faead70 100644 --- a/engine/apps/slack/scenarios/shift_swap_requests.py +++ b/engine/apps/slack/scenarios/shift_swap_requests.py @@ -5,7 +5,6 @@ import humanize from django.utils import timezone -from apps.slack.constants import DIVIDER from apps.slack.models import SlackMessage from apps.slack.scenarios import scenario_step from apps.slack.types import Block, BlockActionType, EventPayload, PayloadType, ScenarioRoute @@ -21,11 +20,19 @@ SHIFT_SWAP_PK_ACTION_KEY = "shift_swap_request_pk" +def _schedule_slack_url(shift_swap_request) -> str: + schedule = shift_swap_request.schedule + return f"<{schedule.web_detail_page_link}|{schedule.name}>" + + class BaseShiftSwapRequestStep(scenario_step.ScenarioStep): def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyBlocks: pk = shift_swap_request.pk - main_message_text = f"Your teammate {shift_swap_request.beneficiary.get_username_with_slack_verbal()} has submitted a shift swap request." + main_message_text = ( + f"*New shift swap request for {_schedule_slack_url(shift_swap_request)}*\n" + f"Your teammate {shift_swap_request.beneficiary.get_username_with_slack_verbal()} has submitted a shift swap request." + ) datetime_format = SlackDateFormat.DATE_LONG_PRETTY time_format = SlackDateFormat.TIME @@ -64,7 +71,7 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB "type": "section", "text": { "type": "mrkdwn", - "text": f"*📅 Shift Details*:\n\n{shift_details}", + "text": f"*Shift details*:\n\n{shift_details}", }, }, ), @@ -78,7 +85,7 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB "type": "section", "text": { "type": "mrkdwn", - "text": f"*📝 Description*: {description}", + "text": f"*Description*: {description}", }, }, ) @@ -92,7 +99,7 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB "type": "section", "text": { "type": "mrkdwn", - "text": "*Update*: this shift swap request has been deleted.", + "text": "❌ this shift swap request has been deleted", }, }, ), @@ -105,7 +112,7 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB "type": "section", "text": { "type": "mrkdwn", - "text": f"*Update*: {shift_swap_request.benefactor.get_username_with_slack_verbal()} has taken the shift swap.", + "text": f"✅ {shift_swap_request.benefactor.get_username_with_slack_verbal()} has accepted the shift swap request", }, }, ), @@ -124,10 +131,9 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB "elements": [ { "type": "button", - "style": "primary", "text": { "type": "plain_text", - "text": "✔️ Accept Shift Swap Request", + "text": "Accept", "emoji": True, }, "value": json.dumps(value), @@ -138,24 +144,6 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB ) ) - blocks.extend( - [ - DIVIDER, - typing.cast( - Block.Context, - { - "type": "context", - "elements": [ - { - "type": "mrkdwn", - "text": f"👀 View the shift swap within Grafana OnCall by clicking <{shift_swap_request.web_link}|here>.", - }, - ], - }, - ), - ] - ) - return blocks def create_message(self, shift_swap_request: "ShiftSwapRequest") -> SlackMessage: @@ -226,8 +214,9 @@ def _generate_blocks(shift_swap_request: "ShiftSwapRequest") -> Block.AnyBlocks: "text": { "type": "mrkdwn", "text": ( - f":exclamation: This shift swap request is still open and will start in {delta}.\n" - "Jump back into the thread and accept it if you're available!" + f"⚠️ This shift swap request for {_schedule_slack_url(shift_swap_request)} is " + f"still open and will start in {delta}. Jump back into the thread and accept it if " + "you're available!" ), }, }, diff --git a/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py b/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py index a2089fbbc9..938a280778 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py @@ -40,26 +40,16 @@ def test_generate_blocks(self, setup) -> None: step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert ( - blocks[0]["text"]["text"] - == f"Your teammate {beneficiary.get_username_with_slack_verbal()} has submitted a shift swap request." + assert blocks[0]["text"]["text"] == ( + f"*New shift swap request for <{ssr.schedule.web_detail_page_link}|{ssr.schedule.name}>*\n" + f"Your teammate {beneficiary.get_username_with_slack_verbal()} has submitted a shift swap request." ) accept_button = blocks[2] - assert accept_button["elements"][0]["text"]["text"] == "✔️ Accept Shift Swap Request" + assert accept_button["elements"][0]["text"]["text"] == "Accept" assert accept_button["type"] == "actions" - assert blocks[3]["type"] == "divider" - - context_section = blocks[4] - - assert context_section["type"] == "context" - assert ( - context_section["elements"][0]["text"] - == f"👀 View the shift swap within Grafana OnCall by clicking <{ssr.web_link}|here>." - ) - @patch("apps.schedules.models.ShiftSwapRequest.shifts") @pytest.mark.parametrize( "shifts,expected_text", @@ -108,7 +98,7 @@ def test_generate_blocks_shift_details(self, mock_shifts, setup, shifts, expecte step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert blocks[1]["text"]["text"] == f"*📅 Shift Details*:\n\n{expected_text}" + assert blocks[1]["text"]["text"] == f"*Shift details*:\n\n{expected_text}" @pytest.mark.django_db def test_generate_blocks_ssr_has_description(self, setup) -> None: @@ -118,7 +108,7 @@ def test_generate_blocks_ssr_has_description(self, setup) -> None: step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert blocks[2]["text"]["text"] == f"*📝 Description*: {description}" + assert blocks[2]["text"]["text"] == f"*Description*: {description}" @pytest.mark.django_db def test_generate_blocks_ssr_is_deleted(self, setup) -> None: @@ -128,7 +118,7 @@ def test_generate_blocks_ssr_is_deleted(self, setup) -> None: step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert blocks[2]["text"]["text"] == "*Update*: this shift swap request has been deleted." + assert blocks[2]["text"]["text"] == "❌ this shift swap request has been deleted" @pytest.mark.django_db def test_generate_blocks_ssr_is_taken(self, setup) -> None: @@ -141,7 +131,7 @@ def test_generate_blocks_ssr_is_taken(self, setup) -> None: assert ( blocks[2]["text"]["text"] - == f"*Update*: {benefactor.get_username_with_slack_verbal()} has taken the shift swap." + == f"✅ {benefactor.get_username_with_slack_verbal()} has accepted the shift swap request" ) @patch("apps.slack.scenarios.shift_swap_requests.BaseShiftSwapRequestStep._generate_blocks") From 08c0edd98dacded077a21a45eb5b8e8aeb74217a Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 30 Aug 2023 12:33:48 +0200 Subject: [PATCH 2/3] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1547b51d7..bfd6c03426 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Update Shift Swap Request Slack message formatting by @joeyorlando ([#TBD](https://github.com/grafana/oncall/pull/TBD)) +- Update Shift Swap Request Slack message formatting by @joeyorlando ([#2918](https://github.com/grafana/oncall/pull/2918)) ## v1.3.29 (2023-08-29) From d0ff704b7e4dddc7e73e001208df92b822c26171 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 31 Aug 2023 12:06:55 +0200 Subject: [PATCH 3/3] design feedback --- .../on-call-schedules/shift-swaps/_index.md | 2 +- .../slack/scenarios/shift_swap_requests.py | 29 +++++++++++-------- .../test_shift_swap_requests.py | 10 +++---- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/docs/sources/on-call-schedules/shift-swaps/_index.md b/docs/sources/on-call-schedules/shift-swaps/_index.md index 517c95e9a5..03340a09bc 100644 --- a/docs/sources/on-call-schedules/shift-swaps/_index.md +++ b/docs/sources/on-call-schedules/shift-swaps/_index.md @@ -43,7 +43,7 @@ Upon submitting the request, a Slack notification will be sent to the channel as schedule, if there is one. A [mobile push notification][shift-swap-notifications] will be sent to team members who participate in the schedule and have the notifications enabled. - + Push notifications are sent 4 weeks ahead of the requested shift swap, or shortly after creation in case the shift swap start time is less than 4 weeks away, but always during users' working hours (by default 9am-5pm on diff --git a/engine/apps/slack/scenarios/shift_swap_requests.py b/engine/apps/slack/scenarios/shift_swap_requests.py index c82faead70..18711b55f9 100644 --- a/engine/apps/slack/scenarios/shift_swap_requests.py +++ b/engine/apps/slack/scenarios/shift_swap_requests.py @@ -38,7 +38,8 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB time_format = SlackDateFormat.TIME shift_details = "" - for shift in shift_swap_request.shifts(): + shifts = shift_swap_request.shifts() + for shift in shifts: shift_start = shift["start"] shift_start_posix = shift_start.timestamp() shift_end = shift["end"] @@ -65,18 +66,22 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB }, }, ), - typing.cast( - Block.Section, - { - "type": "section", - "text": { - "type": "mrkdwn", - "text": f"*Shift details*:\n\n{shift_details}", - }, - }, - ), ] + if shifts: + blocks.append( + typing.cast( + Block.Section, + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": f"*Shift detail{'s' if len(shifts) > 1 else ''}*\n{shift_details}", + }, + }, + ), + ) + if description := shift_swap_request.description: blocks.append( typing.cast( @@ -85,7 +90,7 @@ def _generate_blocks(self, shift_swap_request: "ShiftSwapRequest") -> Block.AnyB "type": "section", "text": { "type": "mrkdwn", - "text": f"*Description*: {description}", + "text": f"*Description*\n{description}", }, }, ) diff --git a/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py b/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py index 938a280778..f480b6b4e9 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_shift_swap_requests.py @@ -45,7 +45,7 @@ def test_generate_blocks(self, setup) -> None: f"Your teammate {beneficiary.get_username_with_slack_verbal()} has submitted a shift swap request." ) - accept_button = blocks[2] + accept_button = blocks[1] assert accept_button["elements"][0]["text"]["text"] == "Accept" assert accept_button["type"] == "actions" @@ -98,7 +98,7 @@ def test_generate_blocks_shift_details(self, mock_shifts, setup, shifts, expecte step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert blocks[1]["text"]["text"] == f"*Shift details*:\n\n{expected_text}" + assert blocks[1]["text"]["text"] == f"*Shift details*\n{expected_text}" @pytest.mark.django_db def test_generate_blocks_ssr_has_description(self, setup) -> None: @@ -108,7 +108,7 @@ def test_generate_blocks_ssr_has_description(self, setup) -> None: step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert blocks[2]["text"]["text"] == f"*Description*: {description}" + assert blocks[1]["text"]["text"] == f"*Description*\n{description}" @pytest.mark.django_db def test_generate_blocks_ssr_is_deleted(self, setup) -> None: @@ -118,7 +118,7 @@ def test_generate_blocks_ssr_is_deleted(self, setup) -> None: step = scenarios.BaseShiftSwapRequestStep(ssr.organization.slack_team_identity, ssr.organization) blocks = step._generate_blocks(ssr) - assert blocks[2]["text"]["text"] == "❌ this shift swap request has been deleted" + assert blocks[1]["text"]["text"] == "❌ this shift swap request has been deleted" @pytest.mark.django_db def test_generate_blocks_ssr_is_taken(self, setup) -> None: @@ -130,7 +130,7 @@ def test_generate_blocks_ssr_is_taken(self, setup) -> None: blocks = step._generate_blocks(ssr) assert ( - blocks[2]["text"]["text"] + blocks[1]["text"]["text"] == f"✅ {benefactor.get_username_with_slack_verbal()} has accepted the shift swap request" )