Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Direct paging improvements #2537

Merged
merged 29 commits into from
Jul 17, 2023
Merged

Conversation

vstpme
Copy link
Member

@vstpme vstpme commented Jul 14, 2023

What this PR does

  • Deprecates /oncall Slack command in favour of /esalate (direct paging) + fixes a regression bug in both commands
  • Unifies direct paging UX across Slack & Web UI (or at least makes an attempt to make things more similar). Kudos to @iskhakov for all the great work on this recently!
  • A bunch of minor changes that hopefully make direct paging more usable
  • TODO: documentation updates will be added in a separate PR

Screenshots

No issues scenario

Slack:

Screenshot 2023-07-14 at 23 53 11

Web UI:

Screenshot 2023-07-14 at 23 52 25

Not configured scenario

Slack:

Screenshot 2023-07-14 at 23 45 22

Web UI:

Screenshot 2023-07-14 at 23 47 31

/oncall deprecation warning

Screenshot 2023-07-17 at 10 31 56

Which issue(s) this PR fixes

#2442

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@vstpme vstpme added the pr:no public docs Added to a PR that does not require public documentation updates label Jul 17, 2023
@vstpme vstpme marked this pull request as ready for review July 17, 2023 09:17
@vstpme vstpme requested a review from a team July 17, 2023 09:17
@vstpme vstpme requested review from a team and iskhakov July 17, 2023 09:17
<ul className={cx('responders-list')}>
<li>
<HorizontalGroup justify="space-between">
<HorizontalGroup>
{escalationChainsExist && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed?

Copy link
Member Author

@vstpme vstpme Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to #2537 (comment), the PR udpates which integrations are considered "bad" or non-configured, and now this tooltip doesn't convey any additional information, so I removed it.

@@ -42,7 +42,7 @@ def _trigger_alert(
deleted_at=None,
defaults={
"author": from_user,
"verbal_name": "Direct paging",
"verbal_name": f"Direct paging ({team.name if team else 'No'} team)",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to return team name to integration name, this way it's easier to filter by integration on the alert groups page.
It also makes things more consistent with other platforms where it's not possible to provide a team widget alongside integration name (e.g. Slack).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it is a good practice to hardcode it lake that. We should use the value from teams table in the places you mentioned

@@ -90,7 +90,7 @@ def _trigger_alert(
return alert.group


def check_user_availability(user: User, team: Team) -> list[dict[str, Any]]:
def check_user_availability(user: User) -> list[dict[str, Any]]:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made so that user availability doesn't depend on the selected team. My reasoning is the following:

  • Imagine this scenario: I want to page some team + some other person
  • This other person is probably from some other team, because why would I want to page two people from the same team? My assumption here is that most of the times when I want to notify additional responders, I want these responders to be from a different team (e.g. for alert groups relevant for multiple teams).

So I'd think it makes sense to check availability across all teams, not only the selected one.

@@ -49,7 +49,7 @@ class DirectPagingSerializer(serializers.Serializer):
alert_group = serializers.HiddenField(default=None) # set in DirectPagingSerializer.validate

title = serializers.CharField(required=False, default=None)
message = serializers.CharField(required=False, default=None)
message = serializers.CharField(required=False, default=None, allow_null=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frontend treats an empty message as null, updating the backend to support this.

@@ -20,6 +20,7 @@
class StartCreateIncidentFromSlashCommand(scenario_step.ScenarioStep):
"""
StartCreateIncidentFromSlashCommand triggers creation of a manual incident from the slack message via slash command
THIS FEATURE IS DEPRECATED AND WILL BE REMOVED IN A FUTURE RELEASE
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecating /oncall Slack command, so the only way to create alerts is /escalate

@@ -300,7 +300,8 @@ def post(self, request):
# Open pop-up to inform user why OnCall bot doesn't work if any action was triggered
self._open_warning_window_if_needed(payload, slack_team_identity, warning_text)
return Response(status=200)
elif organization is None and payload_type_is_block_actions:
# direct paging / manual incident dialogs don't require organization to be set
elif organization is None and payload_type_is_block_actions and not payload.get("view"):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing modal views to bypass organization check, since there's an organization select input in the direct paging view itself.

@@ -52,10 +52,11 @@ const GrafanaTeamSelect = observer(({ onSelect, onHide, withoutModal, defaultVal

const select = (
<GSelect
showSearch
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing searching by name in team select, this was disabled for some reason

@@ -200,22 +203,11 @@ const ManualAlertGroup: FC<ManualAlertGroupProps> = (props) => {
);
};

const submitButtonDisabled = !(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to disabled={!selectedTeamId} so it's easier for a team to create a direct paging integration (so the team can create an alert group without choosing additional responders to get their direct paging integration created automatically)

@@ -169,29 +160,41 @@ const ManualAlertGroup: FC<ManualAlertGroupProps> = (props) => {
</HorizontalGroup>
</li>
</ul>

{(escalationChainsExist || !chatOpsAvailableChannels) && (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed check for chatOpsAvailableChannels, so an integration is considered "bad" only when it doesn't have linked escalation chains. Imagine an integration that has an escalation chain but doesn't have a Slack channel, I think no warnings should be displayed for such integration, but lmk if there's a strong reason for checking ChatOps too.

@vstpme vstpme requested a review from a team July 17, 2023 10:05
@vstpme vstpme merged commit 69bafb6 into dev Jul 17, 2023
@vstpme vstpme deleted the vadimkerr/deprecate-oncall-slash-commands branch July 17, 2023 13:21
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

- Deprecates `/oncall` Slack command in favour of `/esalate` (direct
paging) + fixes a regression bug in both commands
- Unifies direct paging UX across Slack & Web UI (or at least makes an
attempt to make things more similar). Kudos to @iskhakov for all the
great work on this recently!
- A bunch of minor changes that hopefully make direct paging more usable
- TODO: documentation updates will be added in a separate PR

## Screenshots

### No issues scenario

Slack:

<img width="522" alt="Screenshot 2023-07-14 at 23 53 11"
src="https://github.com/grafana/oncall/assets/20116910/ec15a18f-d817-4177-b1f2-6b89d79bb361">


Web UI: 

<img width="1172" alt="Screenshot 2023-07-14 at 23 52 25"
src="https://github.com/grafana/oncall/assets/20116910/813f967c-2fdd-4868-9287-487dbfa7cea6">


### Not configured scenario

Slack:

<img width="519" alt="Screenshot 2023-07-14 at 23 45 22"
src="https://github.com/grafana/oncall/assets/20116910/932fa05c-81ea-42ca-be80-41b05f767d3e">

Web UI:

<img width="1172" alt="Screenshot 2023-07-14 at 23 47 31"
src="https://github.com/grafana/oncall/assets/20116910/6bcb07e4-2e50-4120-9fac-be8b0277e181">

### `/oncall` deprecation warning

<img width="521" alt="Screenshot 2023-07-17 at 10 31 56"
src="https://github.com/grafana/oncall/assets/20116910/4ff28337-1693-4af0-81d9-9eda90099c1b">


## Which issue(s) this PR fixes

#2442

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants