diff --git a/CHANGELOG.md b/CHANGELOG.md index 826bcc0f..61de9a7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,15 @@ 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 +## v1.3.78 (2023-12-12) + +### Changed + +- Add error code for mobile push notification logs when device is not set up @Ferril ([#3554](https://github.com/grafana/oncall/pull/3554)) + +### Fixed + +- Fix issue when mobile push notification message is too big @Ferril ([#3556](https://github.com/grafana/oncall/pull/3556) ## v1.3.77 (2023-12-11) diff --git a/Tiltfile b/Tiltfile index ec6e80ba..2b8a79f9 100644 --- a/Tiltfile +++ b/Tiltfile @@ -35,7 +35,7 @@ allow_k8s_contexts(["kind-kind"]) docker_build_sub( "localhost:63628/oncall/engine:dev", context="./engine", - cache_from=["grafana/oncall:latest"], + cache_from=["grafana/oncall:latest", "grafana/oncall:dev"], ignore=["./grafana-plugin/test-results/", "./grafana-plugin/dist/", "./grafana-plugin/e2e-tests/"], child_context=".", target="dev", diff --git a/engine/apps/alerts/tasks/send_update_log_report_signal.py b/engine/apps/alerts/tasks/send_update_log_report_signal.py index 8d40efab..a59ad75c 100644 --- a/engine/apps/alerts/tasks/send_update_log_report_signal.py +++ b/engine/apps/alerts/tasks/send_update_log_report_signal.py @@ -7,7 +7,7 @@ from .task_logger import task_logger @shared_dedicated_queue_retry_task( - autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else None + autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else 10 ) def send_update_log_report_signal(log_record_pk=None, alert_group_pk=None): from apps.alerts.models import AlertGroup, AlertReceiveChannel diff --git a/engine/apps/base/models/user_notification_policy_log_record.py b/engine/apps/base/models/user_notification_policy_log_record.py index 1c28a092..d8c83e31 100644 --- a/engine/apps/base/models/user_notification_policy_log_record.py +++ b/engine/apps/base/models/user_notification_policy_log_record.py @@ -69,7 +69,8 @@ class UserNotificationPolicyLogRecord(models.Model): ERROR_NOTIFICATION_MESSAGING_BACKEND_ERROR, ERROR_NOTIFICATION_FORBIDDEN, ERROR_NOTIFICATION_TELEGRAM_USER_IS_DEACTIVATED, - ) = range(27) + ERROR_NOTIFICATION_MOBILE_USER_HAS_NO_ACTIVE_DEVICE, + ) = range(28) # for this errors we want to send message to general log channel ERRORS_TO_SEND_IN_SLACK_CHANNEL = [ @@ -264,6 +265,11 @@ class UserNotificationPolicyLogRecord(models.Model): == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_TELEGRAM_USER_IS_DEACTIVATED ): result += f"failed to send telegram message to {user_verbal} because user has been deactivated" + elif ( + self.notification_error_code + == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_MOBILE_USER_HAS_NO_ACTIVE_DEVICE + ): + result += f"failed to send push notification to {user_verbal} because user has no device set up" else: # TODO: handle specific backend errors try: diff --git a/engine/apps/mobile_app/alert_rendering.py b/engine/apps/mobile_app/alert_rendering.py index ef5082db..37453c15 100644 --- a/engine/apps/mobile_app/alert_rendering.py +++ b/engine/apps/mobile_app/alert_rendering.py @@ -10,9 +10,14 @@ class AlertMobileAppTemplater(AlertTemplater): def get_push_notification_subtitle(alert_group): + MAX_ALERT_TITLE_LENGTH = 200 alert = alert_group.alerts.first() templated_alert = AlertMobileAppTemplater(alert).render() alert_title = str_or_backup(templated_alert.title, "Alert Group") + # limit alert title length to prevent FCM `message is too big` exception + # https://firebase.google.com/docs/cloud-messaging/concept-options#notifications_and_data_messages + if len(alert_title) > MAX_ALERT_TITLE_LENGTH: + alert_title = f"{alert_title[:MAX_ALERT_TITLE_LENGTH]}..." status_verbose = "Firing" # TODO: we should probably de-duplicate this text if alert_group.resolved: diff --git a/engine/apps/mobile_app/tasks/new_alert_group.py b/engine/apps/mobile_app/tasks/new_alert_group.py index 715d6300..e359328b 100644 --- a/engine/apps/mobile_app/tasks/new_alert_group.py +++ b/engine/apps/mobile_app/tasks/new_alert_group.py @@ -113,7 +113,7 @@ def notify_user_about_new_alert_group(user_pk, alert_group_pk, notification_poli logger.warning(f"User notification policy {notification_policy_pk} does not exist") return - def _create_error_log_record(): + def _create_error_log_record(notification_error_code=None): """ Utility method to create a UserNotificationPolicyLogRecord with error """ @@ -125,13 +125,14 @@ def notify_user_about_new_alert_group(user_pk, alert_group_pk, notification_poli reason="Mobile push notification error", notification_step=notification_policy.step, notification_channel=notification_policy.notify_by, + notification_error_code=notification_error_code, ) device_to_notify = FCMDevice.get_active_device_for_user(user) # create an error log in case user has no devices set up if not device_to_notify: - _create_error_log_record() + _create_error_log_record(UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_MOBILE_USER_HAS_NO_ACTIVE_DEVICE) logger.error(f"Error while sending a mobile push notification: user {user_pk} has no device set up") return diff --git a/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py b/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py index 5c784497..fec871e1 100644 --- a/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py +++ b/engine/apps/mobile_app/tests/tasks/test_new_alert_group.py @@ -2,7 +2,9 @@ from unittest.mock import patch import pytest +from apps.alerts.incident_appearance.templaters.alert_templater import TemplatedAlert from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord +from apps.mobile_app.alert_rendering import AlertMobileAppTemplater, get_push_notification_subtitle from apps.mobile_app.models import FCMDevice, MobileAppUserSettings from apps.mobile_app.tasks.new_alert_group import _get_fcm_message, notify_user_about_new_alert_group @@ -82,6 +84,10 @@ def test_notify_user_about_new_alert_group_no_device_connected( log_record = alert_group.personal_log_records.last() assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED + assert ( + log_record.notification_error_code + == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_MOBILE_USER_HAS_NO_ACTIVE_DEVICE + ) @pytest.mark.django_db @@ -171,3 +177,37 @@ def test_fcm_message_user_settings_critical_override_dnd_disabled( apns_sound = message.apns.payload.aps.sound assert apns_sound.critical is False assert message.apns.payload.aps.custom_data["interruption-level"] == "time-sensitive" + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "alert_title", + [ + "Some short title", + "Some long title" * 100, + ], +) +def test_get_push_notification_subtitle( + alert_title, + make_organization_and_user, + make_alert_receive_channel, + make_alert_group, + make_alert, +): + MAX_ALERT_TITLE_LENGTH = 200 + organization, user = make_organization_and_user() + alert_receive_channel = make_alert_receive_channel(organization=organization) + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group=alert_group, title=alert_title, raw_request_data={"title": alert_title}) + expected_alert_title = ( + f"{alert_title[:MAX_ALERT_TITLE_LENGTH]}..." if len(alert_title) > MAX_ALERT_TITLE_LENGTH else alert_title + ) + expected_result = ( + f"#1 {expected_alert_title}\n" + f"via {alert_group.channel.short_name}" + "\nStatus: Firing, alerts: 1" + ) + templated_alert = TemplatedAlert() + templated_alert.title = alert_title + with patch.object(AlertMobileAppTemplater, "render", return_value=templated_alert): + result = get_push_notification_subtitle(alert_group) + assert len(expected_alert_title) <= MAX_ALERT_TITLE_LENGTH + 3 + assert result == expected_result diff --git a/engine/apps/telegram/tasks.py b/engine/apps/telegram/tasks.py index f2fbabb0..d6fa1bec 100644 --- a/engine/apps/telegram/tasks.py +++ b/engine/apps/telegram/tasks.py @@ -184,8 +184,14 @@ def on_create_alert_telegram_representative_async(self, alert_pk): """ It's async in order to prevent Telegram downtime or formatting issues causing delay with SMS and other destinations. """ - - alert = Alert.objects.get(pk=alert_pk) + try: + alert = Alert.objects.get(pk=alert_pk) + except Alert.DoesNotExist as e: + if on_create_alert_telegram_representative_async.request.retries >= 10: + logger.error(f"Alert {alert_pk} was not found. Probably it was deleted. Stop retrying") + return + else: + raise e alert_group = alert.group alert_group_messages = alert_group.telegram_messages.filter( diff --git a/grafana-plugin/e2e-tests/integrations/heartbeat.test.ts b/grafana-plugin/e2e-tests/integrations/heartbeat.test.ts index 374ca6ae..2ba7e16e 100644 --- a/grafana-plugin/e2e-tests/integrations/heartbeat.test.ts +++ b/grafana-plugin/e2e-tests/integrations/heartbeat.test.ts @@ -7,6 +7,7 @@ const HEARTBEAT_SETTINGS_FORM_TEST_ID = 'heartbeat-settings-form'; test.describe("updating an integration's heartbeat interval works", async () => { const _openHeartbeatSettingsForm = async (page: Page) => { await page.getByTestId('integration-settings-context-menu-wrapper').getByRole('img').click(); + await page.waitForTimeout(1000); await page.getByTestId('integration-heartbeat-settings').click(); }; @@ -29,6 +30,8 @@ test.describe("updating an integration's heartbeat interval works", async () => await heartbeatSettingsForm.getByTestId('update-heartbeat').click(); + await page.waitForTimeout(1000); + await _openHeartbeatSettingsForm(page); const heartbeatIntervalValue = await heartbeatSettingsForm diff --git a/grafana-plugin/e2e-tests/utils/schedule.ts b/grafana-plugin/e2e-tests/utils/schedule.ts index 3627109a..9c5257f6 100644 --- a/grafana-plugin/e2e-tests/utils/schedule.ts +++ b/grafana-plugin/e2e-tests/utils/schedule.ts @@ -20,13 +20,6 @@ export const createOnCallSchedule = async (page: Page, scheduleName: string, use await clickButton({ page, buttonText: 'Add rotation' }); - /** - * Drag the modal such that the "Create" button will always be visible within the viewport. We cannot scroll - * on the modal itself - * https://playwright.dev/docs/input#dragging-manually - */ - await page.locator('.ReactModal__Content .drag-handler').dragTo(page.locator('.page-header__logo')); - await selectDropdownValue({ page, selectType: 'grafanaSelect', diff --git a/grafana-plugin/package.json b/grafana-plugin/package.json index 60ac20c9..719b6c57 100644 --- a/grafana-plugin/package.json +++ b/grafana-plugin/package.json @@ -122,7 +122,7 @@ "@grafana/data": "^9.2.4", "@grafana/faro-web-sdk": "^1.0.0-beta4", "@grafana/faro-web-tracing": "^1.0.0-beta4", - "@grafana/labels": "~1.4.2", + "@grafana/labels": "~1.4.4", "@grafana/runtime": "9.3.0-beta1", "@grafana/ui": "^10.2.0", "@lifeomic/attempt": "^3.0.3", diff --git a/grafana-plugin/src/PluginPage.tsx b/grafana-plugin/src/PluginPage.tsx index 4497af92..5b81bdc2 100644 --- a/grafana-plugin/src/PluginPage.tsx +++ b/grafana-plugin/src/PluginPage.tsx @@ -3,8 +3,10 @@ import React from 'react'; import { PluginPageProps, PluginPage as RealPluginPage } from '@grafana/runtime'; import Header from 'navbar/Header/Header'; +import RenderConditionally from 'components/RenderConditionally/RenderConditionally'; import { pages } from 'pages'; import { isTopNavbar } from 'plugin/GrafanaPluginRootPage.helpers'; +import { DEFAULT_PAGE } from 'utils/consts'; interface AppPluginPageProps extends PluginPageProps { page?: string; @@ -14,10 +16,14 @@ export const PluginPage = (isTopNavbar() ? RealPlugin : PluginPageFallback) as R function RealPlugin(props: AppPluginPageProps): React.ReactNode { const { page } = props; + const isDefaultPage = page === DEFAULT_PAGE; return ( -
+ +
+ + {pages[page]?.text && !pages[page]?.hideTitle && (

{pages[page].text} diff --git a/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx b/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx index b4a85352..5150caac 100644 --- a/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx +++ b/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx @@ -12,8 +12,10 @@ import { VerticalGroup, useStyles2, } from '@grafana/ui'; +import cn from 'classnames/bind'; import { observer } from 'mobx-react'; +import styles from 'assets/style/utils.css'; import Block from 'components/GBlock/Block'; import Text from 'components/Text/Text'; import { WithPermissionControlTooltip } from 'containers/WithPermissionControl/WithPermissionControlTooltip'; @@ -28,6 +30,8 @@ import { useDebouncedCallback } from 'utils/hooks'; import { getColumnsSelectorWrapperStyles } from './ColumnsSelectorWrapper.styles'; +const cx = cn.bind(styles); + interface ColumnsModalProps { isModalOpen: boolean; labelKeys: Array; @@ -108,7 +112,7 @@ export const ColumnsModal: React.FC = observer( {!result.isCollapsed && ( {result.values === undefined ? ( - + ) : ( renderLabelValues(result.name, result.values) )} @@ -135,7 +139,7 @@ export const ColumnsModal: React.FC = observer( variant="primary" onClick={onAddNewColumns} > - {isLoading ? : 'Add'} + {isLoading ? : 'Add'} diff --git a/grafana-plugin/src/containers/IntegrationForm/IntegrationForm.tsx b/grafana-plugin/src/containers/IntegrationForm/IntegrationForm.tsx index cd3c62bf..dd9f9d8a 100644 --- a/grafana-plugin/src/containers/IntegrationForm/IntegrationForm.tsx +++ b/grafana-plugin/src/containers/IntegrationForm/IntegrationForm.tsx @@ -153,7 +153,7 @@ const IntegrationForm = observer((props: IntegrationFormProps) => { {id === 'new' ? ( 'Alert group labeling' ) : ( - navigateToAlertGroupLabels(id)}>Alert group labels + navigateToAlertGroupLabels(id)}>Alert group labeling )}{' '} drawer. diff --git a/grafana-plugin/src/containers/IntegrationLabelsForm/IntegrationLabelsForm.tsx b/grafana-plugin/src/containers/IntegrationLabelsForm/IntegrationLabelsForm.tsx index a83c912b..ec9024cd 100644 --- a/grafana-plugin/src/containers/IntegrationLabelsForm/IntegrationLabelsForm.tsx +++ b/grafana-plugin/src/containers/IntegrationLabelsForm/IntegrationLabelsForm.tsx @@ -156,7 +156,7 @@ const IntegrationLabelsForm = observer((props: IntegrationLabelsFormProps) => { - + Allows for the extraction and modification of multiple labels from the alert payload using a single template. Supports not only dynamic values but also dynamic keys. The Jinja template must result in diff --git a/grafana-plugin/src/navbar/Header/Header.module.scss b/grafana-plugin/src/navbar/Header/Header.module.scss index 78318cca..efa4bcd3 100644 --- a/grafana-plugin/src/navbar/Header/Header.module.scss +++ b/grafana-plugin/src/navbar/Header/Header.module.scss @@ -4,7 +4,8 @@ .header-topnavbar { padding-top: 0; - padding-bottom: 36px; + padding-bottom: 0; + margin-bottom: 32px; } .navbar-heading { @@ -25,6 +26,7 @@ .navbar-left { display: flex; flex-basis: 100%; + align-items: flex-start; } .navbar-heading-container { @@ -34,6 +36,7 @@ flex-direction: row; column-gap: 8px; row-gap: 8px; + margin-left: -50px; } .irm-icon { @@ -44,7 +47,6 @@ } .banners { - padding-top: 12px; margin-bottom: 24px; &:empty { @@ -52,3 +54,16 @@ margin-bottom: 0; } } + +.logo-container, +.page-header__img { + height: 32px; +} + +.logo-container { + margin-top: 2px; +} + +.page-header__title { + margin-bottom: 8px; +} diff --git a/grafana-plugin/src/navbar/Header/Header.tsx b/grafana-plugin/src/navbar/Header/Header.tsx index 00cae379..c7878ae5 100644 --- a/grafana-plugin/src/navbar/Header/Header.tsx +++ b/grafana-plugin/src/navbar/Header/Header.tsx @@ -23,8 +23,8 @@ const Header = observer(() => {
- - Grafana OnCall + + Grafana OnCall
{renderHeading()}
@@ -41,6 +41,7 @@ const Header = observer(() => {

Grafana OnCall

{APP_SUBTITLE}
+