From f23ae999982398b26090173de83a674b6b74738f Mon Sep 17 00:00:00 2001 From: Rares Mardare Date: Wed, 18 Oct 2023 14:41:14 +0300 Subject: [PATCH 1/7] Add truncation for content beyond 2 lines and show tooltip with original value if content was truncated (#3123) # What this PR does - Removed `MAX_LINE_LENGTH` which was used to truncate Integrations Name - Added new component `TextEllipsisTooltip` that determines if its content was truncated, and if so, it will show a tooltip with the original value. Integrations Name and Alert Group Name fields inside tables have been changed so that after filling in 2 lines, the content gets truncated and a tooltip is shown on hover. Before ![image](https://github.com/grafana/oncall/assets/40542072/1bdd29f3-5804-45f1-85b1-69657d26c73f) After ![image](https://github.com/grafana/oncall/assets/40542072/1a68c3ed-5884-431f-8a6f-f8ea1185987c) As you can see this results in more space being taken for the integrations name (but no more than what it should take), and the end result is always truncated after 2 lines. Same applies to the Integrations Before ![image](https://github.com/grafana/oncall/assets/40542072/802e9a4f-89b5-461d-9c04-15d7117cdb8b) After ![image](https://github.com/grafana/oncall/assets/40542072/a0674469-bdcc-4051-9783-9ffc81b8ba31) The `Don't delete` integration wasn't even showing 100% of its full width in the before screen, but it shows in the after because now there's no more limit on the characters. --- CHANGELOG.md | 4 ++ grafana-plugin/src/assets/style/global.css | 3 + .../src/assets/style/responsive.css | 23 ------- grafana-plugin/src/assets/style/utils.css | 31 +++++++-- .../MatchMediaTooltip/MatchMediaTooltip.tsx | 53 -------------- .../TextEllipsisTooltip.tsx | 60 ++++++++++++++++ .../components/TooltipBadge/TooltipBadge.tsx | 18 ++++- .../src/containers/TeamName/TeamName.tsx | 5 +- .../src/pages/incident/Incident.helpers.tsx | 69 +++++++++---------- .../src/pages/incident/Incident.module.scss | 4 ++ .../src/pages/incidents/Incidents.tsx | 69 +++++++++++-------- .../integrations/Integrations.module.scss | 14 ++-- .../src/pages/integrations/Integrations.tsx | 29 ++++---- .../outgoing_webhooks/OutgoingWebhooks.tsx | 19 +++-- .../src/pages/schedules/Schedules.module.css | 6 -- .../src/pages/schedules/Schedules.tsx | 20 +++--- .../src/plugin/GrafanaPluginRootPage.tsx | 1 - grafana-plugin/src/utils/consts.ts | 5 +- 18 files changed, 239 insertions(+), 194 deletions(-) delete mode 100644 grafana-plugin/src/assets/style/responsive.css delete mode 100644 grafana-plugin/src/components/MatchMediaTooltip/MatchMediaTooltip.tsx create mode 100644 grafana-plugin/src/components/TextEllipsisTooltip/TextEllipsisTooltip.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b188aa6..03508f5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Improve alert group deletion API by @vadimkerr ([#3124](https://github.com/grafana/oncall/pull/3124)) +- Removed Integrations Name max characters limit + ([#3123](https://github.com/grafana/oncall/pull/3123)) +- Truncate long table rows (Integration Name/Alert Group) and show tooltip for the truncated content + ([#3123](https://github.com/grafana/oncall/pull/3123)) ## v1.3.42 (2023-10-04) diff --git a/grafana-plugin/src/assets/style/global.css b/grafana-plugin/src/assets/style/global.css index 5d47bdc4..ef7ae849 100644 --- a/grafana-plugin/src/assets/style/global.css +++ b/grafana-plugin/src/assets/style/global.css @@ -47,6 +47,9 @@ .rc-table-cell { padding-left: 4px; padding-right: 4px; + + /* works better than break-all, especially for table headers */ + word-break: break-word; } .grecaptcha-badge { diff --git a/grafana-plugin/src/assets/style/responsive.css b/grafana-plugin/src/assets/style/responsive.css deleted file mode 100644 index e57034a5..00000000 --- a/grafana-plugin/src/assets/style/responsive.css +++ /dev/null @@ -1,23 +0,0 @@ -/* - Make sure if you chage max-width here - You also change it in consts.ts -*/ -@media screen and (max-width: 1500px) { - .table__email-column { - max-width: 175px; - } - - .table__email-content { - text-overflow: ellipsis; - overflow: hidden; - } - - .incident__title-column { - overflow-wrap: anywhere; - white-space: pre-wrap; - } -} - -.table__wrap-column { - word-break: break-word; -} diff --git a/grafana-plugin/src/assets/style/utils.css b/grafana-plugin/src/assets/style/utils.css index b48abdd0..bbcc0df9 100644 --- a/grafana-plugin/src/assets/style/utils.css +++ b/grafana-plugin/src/assets/style/utils.css @@ -3,7 +3,7 @@ */ .u-flex { - display: flex; + display: flex !important; flex-direction: row; } @@ -84,10 +84,6 @@ position: relative; } -.u-overflow-x-auto { - overflow-x: auto; -} - .u-break-word { word-break: break-word; } @@ -129,3 +125,28 @@ margin-bottom: 0; margin-right: 4px; } + +/* ----- + * Overflow + */ + +.u-overflow-x-auto { + overflow-x: auto; +} + +.overflow-child { + overflow: hidden; + text-overflow: ellipsis; + display: -webkit-box; + white-space: initial; + -webkit-line-clamp: 2; + -webkit-box-orient: vertical; +} + +.break-word { + word-break: break-all; +} + +.line-clamp-3 { + -webkit-line-clamp: 3; +} diff --git a/grafana-plugin/src/components/MatchMediaTooltip/MatchMediaTooltip.tsx b/grafana-plugin/src/components/MatchMediaTooltip/MatchMediaTooltip.tsx deleted file mode 100644 index 6199712c..00000000 --- a/grafana-plugin/src/components/MatchMediaTooltip/MatchMediaTooltip.tsx +++ /dev/null @@ -1,53 +0,0 @@ -import React, { FC, useEffect, useState } from 'react'; - -import { Tooltip } from '@grafana/ui'; -import { debounce } from 'throttle-debounce'; - -interface MatchMediaTooltipProps { - placement: 'top' | 'bottom' | 'right' | 'left'; - content: string; - children: JSX.Element; - - maxWidth?: number; - minWidth?: number; -} - -const DEBOUNCE_MS = 200; - -export const MatchMediaTooltip: FC = ({ minWidth, maxWidth, placement, content, children }) => { - const [match, setMatch] = useState(getMatch()); - - useEffect(() => { - const debouncedResize = debounce(DEBOUNCE_MS, onWindowResize); - window.addEventListener('resize', debouncedResize); - return () => { - window.removeEventListener('resize', debouncedResize); - }; - }, []); - - if (match?.matches) { - return ( - - {children} - - ); - } - - return <>{children}; - - function onWindowResize() { - setMatch(getMatch()); - } - - function getMatch() { - if (minWidth && maxWidth) { - return window.matchMedia(`(min-width: ${minWidth}px) and (max-width: ${maxWidth}px)`); - } else if (minWidth) { - return window.matchMedia(`(min-width: ${minWidth}px)`); - } else if (maxWidth) { - return window.matchMedia(`(max-width: ${maxWidth}px)`); - } - - return undefined; - } -}; diff --git a/grafana-plugin/src/components/TextEllipsisTooltip/TextEllipsisTooltip.tsx b/grafana-plugin/src/components/TextEllipsisTooltip/TextEllipsisTooltip.tsx new file mode 100644 index 00000000..80164980 --- /dev/null +++ b/grafana-plugin/src/components/TextEllipsisTooltip/TextEllipsisTooltip.tsx @@ -0,0 +1,60 @@ +import React, { useEffect, useRef, useState } from 'react'; + +import { Tooltip } from '@grafana/ui'; +import cn from 'classnames/bind'; + +import styles from 'assets/style/utils.css'; +import { TEXT_ELLIPSIS_CLASS } from 'utils/consts'; + +const cx = cn.bind(styles); + +interface TextEllipsisTooltipProps { + content: string; + queryClassName?: string; + placement?: string; + className?: string; + children: JSX.Element | JSX.Element[]; +} + +const TextEllipsisTooltip: React.FC = ({ + queryClassName = TEXT_ELLIPSIS_CLASS, + className, + content: textContent, + placement, + children, +}) => { + const [isEllipsis, setIsEllipsis] = useState(true); + const elContentRef = useRef(null); + + useEffect(() => { + setEllipsis(); + }, []); + + const elContent = ( +
+ {children} +
+ ); + + if (isEllipsis) { + return ( + + {/* The wrapping div is needed, otherwise the attached ref will be lost when mounts */} +
{elContent}
+
+ ); + } + + return elContent; + + function setEllipsis() { + const el = elContentRef?.current?.querySelector(`.${queryClassName}`); + if (!el) { + return; + } + + setIsEllipsis(el.offsetHeight < el.scrollHeight); + } +}; + +export default TextEllipsisTooltip; diff --git a/grafana-plugin/src/components/TooltipBadge/TooltipBadge.tsx b/grafana-plugin/src/components/TooltipBadge/TooltipBadge.tsx index b3eb8e72..84e89a94 100644 --- a/grafana-plugin/src/components/TooltipBadge/TooltipBadge.tsx +++ b/grafana-plugin/src/components/TooltipBadge/TooltipBadge.tsx @@ -17,6 +17,7 @@ interface TooltipBadgeProps { icon?: IconName; customIcon?: React.ReactNode; addPadding?: boolean; + placement?; onHover?: () => void; } @@ -24,14 +25,25 @@ interface TooltipBadgeProps { const cx = cn.bind(styles); const TooltipBadge: FC = (props) => { - const { borderType, text, tooltipTitle, tooltipContent, onHover, addPadding, icon, customIcon, className, ...rest } = - props; + const { + borderType, + text, + tooltipTitle, + tooltipContent, + placement, + onHover, + addPadding, + icon, + customIcon, + className, + ...rest + } = props; const testId = rest['data-testid']; return ( diff --git a/grafana-plugin/src/containers/TeamName/TeamName.tsx b/grafana-plugin/src/containers/TeamName/TeamName.tsx index 8727dc88..956fd846 100644 --- a/grafana-plugin/src/containers/TeamName/TeamName.tsx +++ b/grafana-plugin/src/containers/TeamName/TeamName.tsx @@ -14,11 +14,12 @@ const cx = cn.bind(styles); interface TeamNameProps { team: GrafanaTeam; + className?: string; size?: 'small' | 'medium' | 'large'; } const TeamName = observer((props: TeamNameProps) => { - const { team, size = 'medium' } = props; + const { team, size = 'medium', className } = props; if (!team) { return null; } @@ -26,7 +27,7 @@ const TeamName = observer((props: TeamNameProps) => { return ; } return ( - + {team.name} diff --git a/grafana-plugin/src/pages/incident/Incident.helpers.tsx b/grafana-plugin/src/pages/incident/Incident.helpers.tsx index 125e86cc..b97f9e30 100644 --- a/grafana-plugin/src/pages/incident/Incident.helpers.tsx +++ b/grafana-plugin/src/pages/incident/Incident.helpers.tsx @@ -4,10 +4,10 @@ import { Button, HorizontalGroup, IconButton, Tooltip, VerticalGroup } from '@gr import cn from 'classnames/bind'; import Avatar from 'components/Avatar/Avatar'; -import { MatchMediaTooltip } from 'components/MatchMediaTooltip/MatchMediaTooltip'; import PluginLink from 'components/PluginLink/PluginLink'; import Tag from 'components/Tag/Tag'; import Text from 'components/Text/Text'; +import TextEllipsisTooltip from 'components/TextEllipsisTooltip/TextEllipsisTooltip'; import { WithPermissionControlTooltip } from 'containers/WithPermissionControl/WithPermissionControlTooltip'; import { Alert as AlertType, Alert, IncidentStatus } from 'models/alertgroup/alertgroup.types'; import { User } from 'models/user/user.types'; @@ -15,7 +15,7 @@ import { SilenceButtonCascader } from 'pages/incidents/parts/SilenceButtonCascad import { move } from 'state/helpers'; import { getVar } from 'utils/DOM'; import { UserActions } from 'utils/authorization'; -import { TABLE_COLUMN_MAX_WIDTH } from 'utils/consts'; +import { TEXT_ELLIPSIS_CLASS } from 'utils/consts'; import styles from './Incident.module.scss'; @@ -78,14 +78,13 @@ export function renderRelatedUsers(incident: Alert, isFull = false) { } return ( - - - {' '} - - {user.username} - {' '} - {badge} - + + + + {user.username} + {badge} + + ); } @@ -117,32 +116,30 @@ export function renderRelatedUsers(incident: Alert, isFull = false) { } return ( -
- - {visibleUsers.map(renderUser)} - {Boolean(otherUsers.length) && ( - - {otherUsers.map((user, index) => ( - <> - {index ? ', ' : ''} - {renderUser(user)} - - ))} - - } - > - - - +{otherUsers.length} user{otherUsers.length > 1 ? 's' : ''} - - - - )} - -
+ + {visibleUsers.map(renderUser)} + {Boolean(otherUsers.length) && ( + + {otherUsers.map((user, index) => ( + <> + {index ? ', ' : ''} + {renderUser(user)} + + ))} + + } + > + + + +{otherUsers.length} user{otherUsers.length > 1 ? 's' : ''} + + + + )} + ); } diff --git a/grafana-plugin/src/pages/incident/Incident.module.scss b/grafana-plugin/src/pages/incident/Incident.module.scss index 56fc1eb9..e40de8a9 100644 --- a/grafana-plugin/src/pages/incident/Incident.module.scss +++ b/grafana-plugin/src/pages/incident/Incident.module.scss @@ -196,3 +196,7 @@ } } } + +.user-badge { + vertical-align: middle; +} \ No newline at end of file diff --git a/grafana-plugin/src/pages/incidents/Incidents.tsx b/grafana-plugin/src/pages/incidents/Incidents.tsx index 9627b708..878bbf78 100644 --- a/grafana-plugin/src/pages/incidents/Incidents.tsx +++ b/grafana-plugin/src/pages/incidents/Incidents.tsx @@ -1,6 +1,6 @@ import React, { ReactElement, SyntheticEvent } from 'react'; -import { Button, HorizontalGroup, Icon, LoadingPlaceholder, Tooltip, VerticalGroup } from '@grafana/ui'; +import { Button, HorizontalGroup, Icon, LoadingPlaceholder, VerticalGroup } from '@grafana/ui'; import cn from 'classnames/bind'; import { get } from 'lodash-es'; import { observer } from 'mobx-react'; @@ -15,6 +15,7 @@ import IntegrationLogo from 'components/IntegrationLogo/IntegrationLogo'; import ManualAlertGroup from 'components/ManualAlertGroup/ManualAlertGroup'; import PluginLink from 'components/PluginLink/PluginLink'; import Text from 'components/Text/Text'; +import TextEllipsisTooltip from 'components/TextEllipsisTooltip/TextEllipsisTooltip'; import Tutorial from 'components/Tutorial/Tutorial'; import { TutorialStep } from 'components/Tutorial/Tutorial.types'; import { IncidentsFiltersType } from 'containers/IncidentsFilters/IncidentFilters.types'; @@ -27,7 +28,7 @@ import { PageProps, WithStoreProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; import LocationHelper from 'utils/LocationHelper'; import { UserActions } from 'utils/authorization'; -import { PAGE, PLUGIN_ROOT } from 'utils/consts'; +import { PAGE, PLUGIN_ROOT, TEXT_ELLIPSIS_CLASS } from 'utils/consts'; import styles from './Incidents.module.scss'; import { IncidentDropdown } from './parts/IncidentDropdown'; @@ -463,7 +464,7 @@ class Incidents extends React.Component const columns = [ { - width: '5%', + width: '140px', title: 'Status', key: 'time', render: withSkeleton(this.renderStatus), @@ -553,7 +554,13 @@ class Incidents extends React.Component }; renderId(record: AlertType) { - return #{record.inside_organization_number}; + return ( + + + #{record.inside_organization_number} + + + ); } renderTitle = (record: AlertType) => { @@ -565,25 +572,25 @@ class Incidents extends React.Component const { incidentsItemsPerPage, incidentsCursor } = store.alertGroupStore; return ( - -
- - - {record.render_for_web.title} - - - {Boolean(record.dependent_alert_groups.length) && ` + ${record.dependent_alert_groups.length} attached`} -
-
+
+ + + + {record.render_for_web.title} + + + + {Boolean(record.dependent_alert_groups.length) && ` + ${record.dependent_alert_groups.length} attached`} +
); }; @@ -598,10 +605,14 @@ class Incidents extends React.Component const integration = alertReceiveChannelStore.getIntegration(record.alert_receive_channel); return ( - + - - + + ); }; @@ -631,7 +642,11 @@ class Incidents extends React.Component } renderTeam(record: AlertType, teams: any) { - return ; + return ( + + + + ); } getOnActionButtonClick = (incidentId: string, action: AlertAction): ((e: SyntheticEvent) => Promise) => { diff --git a/grafana-plugin/src/pages/integrations/Integrations.module.scss b/grafana-plugin/src/pages/integrations/Integrations.module.scss index 42146649..a2d942f2 100644 --- a/grafana-plugin/src/pages/integrations/Integrations.module.scss +++ b/grafana-plugin/src/pages/integrations/Integrations.module.scss @@ -2,7 +2,12 @@ width: 180px; } -.title { +.heartbeat-badge { + padding: 4px 10px; + width: 40px; +} + +.integrations-header { margin-bottom: 24px; right: 0; } @@ -15,11 +20,6 @@ margin-top: 16px; } -.heartbeat-badge { - padding: 4px 10px; - width: 40px; -} - .integrations-actionsList { display: flex; flex-direction: column; @@ -44,4 +44,4 @@ &:hover { background: var(--cards-background); } -} \ No newline at end of file +} diff --git a/grafana-plugin/src/pages/integrations/Integrations.tsx b/grafana-plugin/src/pages/integrations/Integrations.tsx index 5b5cfcfd..d9a8a1e5 100644 --- a/grafana-plugin/src/pages/integrations/Integrations.tsx +++ b/grafana-plugin/src/pages/integrations/Integrations.tsx @@ -19,6 +19,7 @@ import { } from 'components/PageErrorHandlingWrapper/PageErrorHandlingWrapper.helpers'; import PluginLink from 'components/PluginLink/PluginLink'; import Text from 'components/Text/Text'; +import TextEllipsisTooltip from 'components/TextEllipsisTooltip/TextEllipsisTooltip'; import TooltipBadge from 'components/TooltipBadge/TooltipBadge'; import { WithContextMenu } from 'components/WithContextMenu/WithContextMenu'; import IntegrationForm from 'containers/IntegrationForm/IntegrationForm'; @@ -34,14 +35,13 @@ import { withMobXProviderContext } from 'state/withStore'; import { openNotification } from 'utils'; import LocationHelper from 'utils/LocationHelper'; import { UserActions } from 'utils/authorization'; -import { PAGE } from 'utils/consts'; +import { PAGE, TEXT_ELLIPSIS_CLASS } from 'utils/consts'; import styles from './Integrations.module.scss'; const cx = cn.bind(styles); const FILTERS_DEBOUNCE_MS = 500; const ITEMS_PER_PAGE = 15; -const MAX_LINE_LENGTH = 40; interface IntegrationsState extends PageBaseState { integrationsFilters: Filters; @@ -227,16 +227,11 @@ class Integrations extends React.Component ...query, }} > - - MAX_LINE_LENGTH - ? item.verbal_name?.substring(0, MAX_LINE_LENGTH) + '...' - : item.verbal_name - } - /> - + + + + +
); }; @@ -278,6 +273,7 @@ class Integrations extends React.Component icon="link" text={`${connectedEscalationsChainsCount}/${routesCounter}`} tooltipContent={undefined} + placement="top" tooltipTitle={ connectedEscalationsChainsCount + ' connected escalation chain' + @@ -328,6 +325,7 @@ class Integrations extends React.Component : } tooltipTitle={`Last heartbeat: ${heartbeat?.last_heartbeat_time_verbal}`} @@ -347,6 +345,7 @@ class Integrations extends React.Component } renderTeam(item: AlertReceiveChannel, teams: any) { - return ; + return ( + + + + ); } renderButtons = (item: AlertReceiveChannel) => { diff --git a/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx b/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx index d61294ee..99f2c1d3 100644 --- a/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx +++ b/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx @@ -26,6 +26,7 @@ import { } from 'components/PageErrorHandlingWrapper/PageErrorHandlingWrapper.helpers'; import PluginLink from 'components/PluginLink/PluginLink'; import Text from 'components/Text/Text'; +import TextEllipsisTooltip from 'components/TextEllipsisTooltip/TextEllipsisTooltip'; import OutgoingWebhookForm from 'containers/OutgoingWebhookForm/OutgoingWebhookForm'; import RemoteFilters from 'containers/RemoteFilters/RemoteFilters'; import TeamName from 'containers/TeamName/TeamName'; @@ -36,7 +37,7 @@ import { PageProps, WithStoreProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; import { openErrorNotification, openNotification } from 'utils'; import { isUserActionAllowed, UserActions } from 'utils/authorization'; -import { PAGE, PLUGIN_ROOT } from 'utils/consts'; +import { PAGE, PLUGIN_ROOT, TEXT_ELLIPSIS_CLASS } from 'utils/consts'; import styles from './OutgoingWebhooks.module.scss'; import { WebhookFormActionType } from './OutgoingWebhooks.types'; @@ -246,7 +247,11 @@ class OutgoingWebhooks extends React.Component; + return ( + + + + ); } renderActionButtons = (record: OutgoingWebhook) => { @@ -342,9 +347,13 @@ class OutgoingWebhooks extends React.Component - {url} - + + openNotification('URL has been copied')}> + + {url} + + + ); } diff --git a/grafana-plugin/src/pages/schedules/Schedules.module.css b/grafana-plugin/src/pages/schedules/Schedules.module.css index 2511f367..c77a5af7 100644 --- a/grafana-plugin/src/pages/schedules/Schedules.module.css +++ b/grafana-plugin/src/pages/schedules/Schedules.module.css @@ -35,9 +35,3 @@ flex-grow: 1; gap: 8px; } - -.schedules__user-on-call { - display: flex; - flex-wrap: nowrap; - gap: 4px; -} diff --git a/grafana-plugin/src/pages/schedules/Schedules.tsx b/grafana-plugin/src/pages/schedules/Schedules.tsx index d2fa86be..041cda71 100644 --- a/grafana-plugin/src/pages/schedules/Schedules.tsx +++ b/grafana-plugin/src/pages/schedules/Schedules.tsx @@ -9,11 +9,11 @@ import qs from 'query-string'; import { RouteComponentProps, withRouter } from 'react-router-dom'; import Avatar from 'components/Avatar/Avatar'; -import { MatchMediaTooltip } from 'components/MatchMediaTooltip/MatchMediaTooltip'; import NewScheduleSelector from 'components/NewScheduleSelector/NewScheduleSelector'; import PluginLink from 'components/PluginLink/PluginLink'; import Table from 'components/Table/Table'; import Text from 'components/Text/Text'; +import TextEllipsisTooltip from 'components/TextEllipsisTooltip/TextEllipsisTooltip'; import TimelineMarks from 'components/TimelineMarks/TimelineMarks'; import TooltipBadge from 'components/TooltipBadge/TooltipBadge'; import UserTimezoneSelect from 'components/UserTimezoneSelect/UserTimezoneSelect'; @@ -33,7 +33,7 @@ import { WithStoreProps, PageProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; import LocationHelper from 'utils/LocationHelper'; import { UserActions } from 'utils/authorization'; -import { PAGE, PLUGIN_ROOT, TABLE_COLUMN_MAX_WIDTH } from 'utils/consts'; +import { PAGE, PLUGIN_ROOT, TEXT_ELLIPSIS_CLASS } from 'utils/consts'; import styles from './Schedules.module.css'; @@ -369,14 +369,14 @@ class SchedulesPage extends React.Component { return ( -
-
- -
- - {user.username} - -
+ + + + {' '} + {user.username} + + +
); })} diff --git a/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx b/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx index 86494341..2a156f34 100644 --- a/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx +++ b/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx @@ -53,7 +53,6 @@ dayjs.extend(customParseFormat); import 'assets/style/vars.css'; import 'assets/style/global.css'; import 'assets/style/utils.css'; -import 'assets/style/responsive.css'; import { getQueryParams, isTopNavbar } from './GrafanaPluginRootPage.helpers'; import PluginSetup from './PluginSetup'; diff --git a/grafana-plugin/src/utils/consts.ts b/grafana-plugin/src/utils/consts.ts index 0182abe7..b045079c 100644 --- a/grafana-plugin/src/utils/consts.ts +++ b/grafana-plugin/src/utils/consts.ts @@ -41,9 +41,6 @@ export const FARO_ENDPOINT_PROD = export const DOCS_SLACK_SETUP = 'https://grafana.com/docs/oncall/latest/open-source/#slack-setup'; export const DOCS_TELEGRAM_SETUP = 'https://grafana.com/docs/oncall/latest/notify/telegram/'; -// Make sure if you chage max-width here you also change it in responsive.css -export const TABLE_COLUMN_MAX_WIDTH = 1500; - export const generateAssignToTeamInputDescription = (objectName: string): string => `Assigning to a team allows you to filter ${objectName} and configure their visibility. Go to OnCall -> Settings -> Team and Access Settings for more details.`; @@ -54,3 +51,5 @@ export enum PAGE { Webhooks = 'webhooks', Schedules = 'schedules', } + +export const TEXT_ELLIPSIS_CLASS = 'overflow-child'; From 5c85ced4a96694b58a4ce87f6e79eef3ce8c0e8e Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Wed, 18 Oct 2023 13:41:25 -0300 Subject: [PATCH 2/7] Update ical schedule creation/update to trigger final schedule refresh (#3156) Otherwise iCal schedules only refresh their cached final representation once a day (via periodic task). Related to https://github.com/grafana/oncall/issues/3154 --- CHANGELOG.md | 6 ++ engine/apps/api/serializers/schedule_ical.py | 14 +++- engine/apps/api/tests/test_schedules.py | 70 +++++++++++++------ .../public_api/serializers/schedules_ical.py | 8 +++ .../apps/public_api/tests/test_schedules.py | 41 ++++++----- 5 files changed, 99 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03508f5f..7e6e81ee 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 + +### Fixed + +- Update ical schedule creation/update to trigger final schedule refresh ([#3156](https://github.com/grafana/oncall/pull/3156)) + ## v1.3.44 (2023-10-16) ### Added diff --git a/engine/apps/api/serializers/schedule_ical.py b/engine/apps/api/serializers/schedule_ical.py index 3852f662..a668b140 100644 --- a/engine/apps/api/serializers/schedule_ical.py +++ b/engine/apps/api/serializers/schedule_ical.py @@ -1,6 +1,10 @@ from apps.api.serializers.schedule_base import ScheduleBaseSerializer from apps.schedules.models import OnCallScheduleICal -from apps.schedules.tasks import schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule +from apps.schedules.tasks import ( + refresh_ical_final_schedule, + schedule_notify_about_empty_shifts_in_schedule, + schedule_notify_about_gaps_in_schedule, +) from apps.slack.models import SlackChannel, SlackUserGroup from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField from common.api_helpers.utils import validate_ical_url @@ -37,6 +41,12 @@ class ScheduleICalCreateSerializer(ScheduleICalSerializer): allow_null=True, ) + def create(self, validated_data): + created_schedule = super().create(validated_data) + # for iCal-based schedules we need to refresh final schedule information + refresh_ical_final_schedule.apply_async((created_schedule.pk,)) + return created_schedule + class Meta: model = OnCallScheduleICal fields = [ @@ -80,4 +90,6 @@ class ScheduleICalUpdateSerializer(ScheduleICalCreateSerializer): updated_schedule.check_gaps_for_next_week() schedule_notify_about_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_gaps_in_schedule.apply_async((instance.pk,)) + # for iCal-based schedules we need to refresh final schedule information + refresh_ical_final_schedule.apply_async((instance.pk,)) return updated_schedule diff --git a/engine/apps/api/tests/test_schedules.py b/engine/apps/api/tests/test_schedules.py index 24983191..756d8285 100644 --- a/engine/apps/api/tests/test_schedules.py +++ b/engine/apps/api/tests/test_schedules.py @@ -601,25 +601,25 @@ def test_create_ical_schedule(schedule_internal_api_setup, make_user_auth_header user, token, _, _, _, _ = schedule_internal_api_setup client = APIClient() url = reverse("api-internal:schedule-list") + data = { + "ical_url_primary": ICAL_URL, + "ical_url_overrides": None, + "name": "created_ical_schedule", + "type": 1, + "slack_channel_id": None, + "user_group": None, + "team": None, + "warnings": [], + "on_call_now": [], + "has_gaps": False, + "mention_oncall_next": False, + "mention_oncall_start": True, + "notify_empty_oncall": 0, + "notify_oncall_shift_freq": 1, + } with patch( "apps.api.serializers.schedule_ical.ScheduleICalSerializer.validate_ical_url_primary", return_value=ICAL_URL - ): - data = { - "ical_url_primary": ICAL_URL, - "ical_url_overrides": None, - "name": "created_ical_schedule", - "type": 1, - "slack_channel_id": None, - "user_group": None, - "team": None, - "warnings": [], - "on_call_now": [], - "has_gaps": False, - "mention_oncall_next": False, - "mention_oncall_start": True, - "notify_empty_oncall": 0, - "notify_oncall_shift_freq": 1, - } + ), patch("apps.schedules.tasks.refresh_ical_final_schedule.apply_async") as mock_refresh_final: response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) # modify initial data by adding id and None for optional fields schedule = OnCallSchedule.objects.get(public_primary_key=response.data["id"]) @@ -628,6 +628,8 @@ def test_create_ical_schedule(schedule_internal_api_setup, make_user_auth_header data["enable_web_overrides"] = False assert response.status_code == status.HTTP_201_CREATED assert response.data == data + # check final schedule refresh triggered + mock_refresh_final.assert_called_once_with((schedule.pk,)) @pytest.mark.django_db @@ -736,12 +738,40 @@ def test_update_ical_schedule(schedule_internal_api_setup, make_user_auth_header "type": 1, "team": None, } - response = client.put( - url, data=json.dumps(data), content_type="application/json", **make_user_auth_headers(user, token) - ) + with patch("apps.schedules.tasks.refresh_ical_final_schedule.apply_async") as mock_refresh_final: + response = client.put( + url, data=json.dumps(data), content_type="application/json", **make_user_auth_headers(user, token) + ) updated_instance = OnCallSchedule.objects.get(public_primary_key=ical_schedule.public_primary_key) assert response.status_code == status.HTTP_200_OK assert updated_instance.name == "updated_ical_schedule" + # check refresh final is not triggered (url unchanged) + assert not mock_refresh_final.called + + +@pytest.mark.django_db +def test_update_ical_schedule_url(schedule_internal_api_setup, make_user_auth_headers): + user, token, _, ical_schedule, _, _ = schedule_internal_api_setup + client = APIClient() + + url = reverse("api-internal:schedule-detail", kwargs={"pk": ical_schedule.public_primary_key}) + + updated_url = "another-url" + data = { + "name": ical_schedule.name, + "type": 1, + "ical_url_primary": updated_url, + } + with patch( + "apps.api.serializers.schedule_ical.ScheduleICalSerializer.validate_ical_url_primary", return_value=updated_url + ), patch("apps.schedules.tasks.refresh_ical_final_schedule.apply_async") as mock_refresh_final: + response = client.put( + url, data=json.dumps(data), content_type="application/json", **make_user_auth_headers(user, token) + ) + updated_instance = OnCallSchedule.objects.get(public_primary_key=ical_schedule.public_primary_key) + assert response.status_code == status.HTTP_200_OK + # check refresh final triggered (changing url) + mock_refresh_final.assert_called_once_with((updated_instance.pk,)) @pytest.mark.django_db diff --git a/engine/apps/public_api/serializers/schedules_ical.py b/engine/apps/public_api/serializers/schedules_ical.py index 458265c4..51a66dca 100644 --- a/engine/apps/public_api/serializers/schedules_ical.py +++ b/engine/apps/public_api/serializers/schedules_ical.py @@ -2,6 +2,7 @@ from apps.public_api.serializers.schedules_base import ScheduleBaseSerializer from apps.schedules.models import OnCallScheduleICal from apps.schedules.tasks import ( drop_cached_ical_task, + refresh_ical_final_schedule, schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule, ) @@ -32,6 +33,12 @@ class ScheduleICalSerializer(ScheduleBaseSerializer): def validate_ical_url_overrides(self, url): return validate_ical_url(url) + def create(self, validated_data): + created_schedule = super().create(validated_data) + # for iCal-based schedules we need to refresh final schedule information + refresh_ical_final_schedule.apply_async((created_schedule.pk,)) + return created_schedule + class ScheduleICalUpdateSerializer(ScheduleICalSerializer): team_id = TeamPrimaryKeyRelatedField(required=False, allow_null=True, source="team") @@ -70,4 +77,5 @@ class ScheduleICalUpdateSerializer(ScheduleICalSerializer): ) schedule_notify_about_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_gaps_in_schedule.apply_async((instance.pk,)) + refresh_ical_final_schedule.apply_async((instance.pk,)) return super().update(instance, validated_data) diff --git a/engine/apps/public_api/tests/test_schedules.py b/engine/apps/public_api/tests/test_schedules.py index 2e1ed13e..45a78dd1 100644 --- a/engine/apps/public_api/tests/test_schedules.py +++ b/engine/apps/public_api/tests/test_schedules.py @@ -393,24 +393,24 @@ def test_update_ical_url_overrides_calendar_schedule( with patch("common.api_helpers.utils.validate_ical_url", return_value=ICAL_URL): response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") - result = { - "id": schedule.public_primary_key, - "team_id": None, - "name": schedule.name, - "type": "calendar", - "time_zone": schedule.time_zone, - "on_call_now": [], - "shifts": [], - "slack": { - "channel_id": "SLACKCHANNELID", - "user_group_id": None, - }, - "ical_url_overrides": ICAL_URL, - "enable_web_overrides": False, - } + result = { + "id": schedule.public_primary_key, + "team_id": None, + "name": schedule.name, + "type": "calendar", + "time_zone": schedule.time_zone, + "on_call_now": [], + "shifts": [], + "slack": { + "channel_id": "SLACKCHANNELID", + "user_group_id": None, + }, + "ical_url_overrides": ICAL_URL, + "enable_web_overrides": False, + } - assert response.status_code == status.HTTP_200_OK - assert response.json() == result + assert response.status_code == status.HTTP_200_OK + assert response.json() == result @pytest.mark.django_db @@ -633,7 +633,7 @@ def test_create_ical_schedule(make_organization_and_user_with_token): with patch( "apps.public_api.serializers.schedules_ical.ScheduleICalSerializer.validate_ical_url_primary", return_value=ICAL_URL, - ): + ), patch("apps.schedules.tasks.refresh_ical_final_schedule.apply_async") as mock_refresh_final: response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") schedule = OnCallSchedule.objects.get(public_primary_key=response.data["id"]) @@ -653,6 +653,7 @@ def test_create_ical_schedule(make_organization_and_user_with_token): assert response.status_code == status.HTTP_201_CREATED assert response.json() == result + mock_refresh_final.assert_called_once_with((schedule.pk,)) @pytest.mark.django_db @@ -680,7 +681,8 @@ def test_update_ical_schedule( assert schedule.name != data["name"] - response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + with patch("apps.schedules.tasks.refresh_ical_final_schedule.apply_async") as mock_refresh_final: + response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") result = { "id": schedule.public_primary_key, @@ -700,6 +702,7 @@ def test_update_ical_schedule( schedule.refresh_from_db() assert schedule.name == data["name"] assert response.json() == result + assert not mock_refresh_final.called @pytest.mark.django_db From 35620028ccbb88aa908f2f52627a226197662cda Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Wed, 18 Oct 2023 14:14:56 -0300 Subject: [PATCH 3/7] Add user TZ information to next shifts per user endpoint (#3157) --- CHANGELOG.md | 4 ++++ engine/apps/api/tests/test_schedules.py | 32 ++++++++++++++++++++----- engine/apps/api/views/schedule.py | 9 ++++--- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e6e81ee..8a81dca4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Update ical schedule creation/update to trigger final schedule refresh ([#3156](https://github.com/grafana/oncall/pull/3156)) +### Changed + +- Add user TZ information to next shifts per user endpoint ([#3157](https://github.com/grafana/oncall/pull/3157)) + ## v1.3.44 (2023-10-16) ### Added diff --git a/engine/apps/api/tests/test_schedules.py b/engine/apps/api/tests/test_schedules.py index 756d8285..828aa80c 100644 --- a/engine/apps/api/tests/test_schedules.py +++ b/engine/apps/api/tests/test_schedules.py @@ -1398,7 +1398,15 @@ def test_next_shifts_per_user( ) tomorrow = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + timezone.timedelta(days=1) - user_a, user_b, user_c, user_d = (make_user_for_organization(organization, username=i) for i in "ABCD") + users = ( + ("A", "Europe/London"), + ("B", "UTC"), + ("C", None), + ("D", "America/Montevideo"), + ) + user_a, user_b, user_c, user_d = ( + make_user_for_organization(organization, username=i, _timezone=tz) for i, tz in users + ) # clear users pks <-> organization cache (persisting between tests) memoized_users_in_ical.cache_clear() @@ -1456,13 +1464,25 @@ def test_next_shifts_per_user( assert response.status_code == status.HTTP_200_OK expected = { - user_a.public_primary_key: (tomorrow + timezone.timedelta(hours=15), tomorrow + timezone.timedelta(hours=16)), - user_b.public_primary_key: (tomorrow + timezone.timedelta(hours=7), tomorrow + timezone.timedelta(hours=12)), - user_c.public_primary_key: (tomorrow + timezone.timedelta(hours=17), tomorrow + timezone.timedelta(hours=18)), - user_d.public_primary_key: None, + user_a.public_primary_key: ( + tomorrow + timezone.timedelta(hours=15), + tomorrow + timezone.timedelta(hours=16), + user_a.timezone, + ), + user_b.public_primary_key: ( + tomorrow + timezone.timedelta(hours=7), + tomorrow + timezone.timedelta(hours=12), + user_b.timezone, + ), + user_c.public_primary_key: ( + tomorrow + timezone.timedelta(hours=17), + tomorrow + timezone.timedelta(hours=18), + user_c.timezone, + ), + user_d.public_primary_key: (None, None, user_d.timezone), } returned_data = { - u: (ev["start"], ev["end"]) if ev is not None else None for u, ev in response.data["users"].items() + u: (ev.get("start"), ev.get("end"), ev.get("user_timezone")) for u, ev in response.data["users"].items() } assert returned_data == expected diff --git a/engine/apps/api/views/schedule.py b/engine/apps/api/views/schedule.py index eb300ee2..83121b1e 100644 --- a/engine/apps/api/views/schedule.py +++ b/engine/apps/api/views/schedule.py @@ -375,11 +375,14 @@ class ScheduleView( events = schedule.final_events(now, datetime_end) - users = {u.public_primary_key: None for u in schedule.related_users()} + # include user TZ information for every user + users = {u.public_primary_key: {"user_timezone": u.timezone} for u in schedule.related_users()} + added_users = set() for e in events: user = e["users"][0]["pk"] if e["users"] else None - if user is not None and users.get(user) is None and e["end"] > now: - users[user] = e + if user is not None and user not in added_users and e["end"] > now: + users[user].update(e) + added_users.add(user) result = {"users": users} return Response(result, status=status.HTTP_200_OK) From 2d656c50db6858f7fef12519d8cbb463323842b2 Mon Sep 17 00:00:00 2001 From: Maxim Mordasov Date: Thu, 19 Oct 2023 12:53:12 +0300 Subject: [PATCH 4/7] =?UTF-8?q?Build=20=E2=80=98When=20I=20am=20on-call?= =?UTF-8?q?=E2=80=99=20for=20web=20UI=20#2915=20(#3100)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # What this PR does Polish ‘When I am on-call’ feature ## Which issue(s) this PR fixes [Build ‘When I am on-call’ for web UI #2915](https://github.com/grafana/oncall/issues/2915) ## 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) --- CHANGELOG.md | 7 ++ .../src/containers/Rotation/Rotation.tsx | 6 +- .../containers/Rotations/ScheduleFinal.tsx | 4 +- .../Rotations/ScheduleOverrides.tsx | 4 +- .../containers/Rotations/SchedulePersonal.tsx | 118 +++++++++++++----- .../containers/ScheduleSlot/ScheduleSlot.tsx | 17 +-- .../src/models/schedule/schedule.helpers.ts | 63 ++++++++-- .../src/models/schedule/schedule.ts | 34 +++-- .../src/models/schedule/schedule.types.ts | 3 +- .../src/pages/schedule/Schedule.tsx | 7 ++ .../src/pages/schedules/Schedules.tsx | 21 ++-- 11 files changed, 208 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a81dca4..1f9c1cc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Added + +- Use shift data from event object + ### Fixed - Update ical schedule creation/update to trigger final schedule refresh ([#3156](https://github.com/grafana/oncall/pull/3156)) +- Polish "Build 'When I am on-call' for web UI" [#2915](https://github.com/grafana/oncall/issues/2915) +- Fix iCal schedule incorrect view [#2001](https://github.com/grafana/oncall-private/issues/2001) +- Fix rotation name rendering issue [#2324](https://github.com/grafana/oncall/issues/2324) ### Changed diff --git a/grafana-plugin/src/containers/Rotation/Rotation.tsx b/grafana-plugin/src/containers/Rotation/Rotation.tsx index 97156cc1..29c798a3 100644 --- a/grafana-plugin/src/containers/Rotation/Rotation.tsx +++ b/grafana-plugin/src/containers/Rotation/Rotation.tsx @@ -8,7 +8,7 @@ import hash from 'object-hash'; import { ScheduleFiltersType } from 'components/ScheduleFilters/ScheduleFilters.types'; import Text from 'components/Text/Text'; import ScheduleSlot from 'containers/ScheduleSlot/ScheduleSlot'; -import { Event, RotationFormLiveParams, Shift, ShiftSwap } from 'models/schedule/schedule.types'; +import { Event, RotationFormLiveParams, ShiftSwap } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; import RotationTutorial from './RotationTutorial'; @@ -34,7 +34,7 @@ interface RotationProps { tutorialParams?: RotationFormLiveParams; simplified?: boolean; filters?: ScheduleFiltersType; - getColor?: (shiftId: Shift['id']) => string; + getColor?: (event: Event) => string; onSlotClick?: (event: Event) => void; emptyText?: string; showScheduleNameAsSlotTitle?: boolean; @@ -156,7 +156,7 @@ const Rotation: FC = (props) => { event={event} startMoment={startMoment} currentTimezone={currentTimezone} - color={propsColor || getColor(event.shift?.pk)} + color={propsColor || getColor(event)} handleAddOverride={getAddOverrideClickHandler(event)} handleAddShiftSwap={getAddShiftSwapClickHandler(event)} handleOpenSchedule={getOpenScheduleClickHandler(event)} diff --git a/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx b/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx index d75e6cab..89166869 100644 --- a/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx +++ b/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx @@ -16,7 +16,7 @@ import { getOverridesFromStore, getShiftsFromStore, } from 'models/schedule/schedule.helpers'; -import { Schedule, Shift, ShiftSwap, Event } from 'models/schedule/schedule.types'; +import { Schedule, ShiftSwap, Event } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; import { WithStoreProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; @@ -59,7 +59,7 @@ class ScheduleFinal extends Component { const currentTimeHidden = currentTimeX < 0 || currentTimeX > 1; - const getColor = (shiftId: Shift['id']) => findColor(shiftId, layers, overrides); + const getColor = (event: Event) => findColor(event.shift?.pk, layers, overrides); return ( <> diff --git a/grafana-plugin/src/containers/Rotations/ScheduleOverrides.tsx b/grafana-plugin/src/containers/Rotations/ScheduleOverrides.tsx index 0c22747d..accbdebe 100644 --- a/grafana-plugin/src/containers/Rotations/ScheduleOverrides.tsx +++ b/grafana-plugin/src/containers/Rotations/ScheduleOverrides.tsx @@ -46,6 +46,7 @@ interface ScheduleOverridesProps extends WithStoreProps { onUpdate: () => void; onDelete: () => void; disabled: boolean; + disableShiftSwaps: boolean; filters: ScheduleFiltersType; } @@ -72,6 +73,7 @@ class ScheduleOverrides extends Component + + + + + + diff --git a/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx b/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx index 821ed787..32ad21b0 100644 --- a/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx +++ b/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx @@ -57,7 +57,7 @@ const ScheduleSlot: FC = observer((props) => { const base = 60 * 60 * 24 * 7; - const width = duration / base; + const width = Math.max(duration / base, 0); const currentMoment = useMemo(() => dayjs(), []); @@ -172,6 +172,7 @@ const ShiftSwapEvent = (props: ShiftSwapEventProps) => { content={ { {users.map(({ display_name, pk: userPk, swap_request }) => { const storeUser = store.userStore.items[userPk]; + const { schedule, shift } = event; + const isCurrentUserSlot = userPk === store.userStore.currentUserPk; const inactive = filters && filters.users.length && !filters.users.includes(userPk); - const userTitle = storeUser ? getTitle(storeUser) : display_name; + const userTitle = showScheduleNameAsSlotTitle ? schedule?.name : storeUser ? getTitle(storeUser) : display_name; const isShiftSwap = Boolean(swap_request); + const title = isShiftSwap ? 'Shift swap' : showScheduleNameAsSlotTitle ? schedule?.name : getShiftName(shift); + let backgroundColor = color; if (isShiftSwap) { backgroundColor = SHIFT_SWAP_COLOR; @@ -282,7 +287,7 @@ const RegularEvent = (props: RegularEventProps) => { key={userPk} content={ { @@ -344,7 +349,7 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { beneficiaryName, benefactorName, currentMoment, - showScheduleNameAsSlotTitle, + title, } = props; const { scheduleStore } = useStore(); @@ -368,8 +373,6 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { } }, [shift]); - const title = isShiftSwap ? 'Shift swap' : showScheduleNameAsSlotTitle ? schedule?.name : getShiftName(shift); - // const onCallNow = schedule?.on_call_now; // const isOncall = Boolean(storeUser && onCallNow && onCallNow.some((onCallUser) => storeUser.pk === onCallUser.pk)); diff --git a/grafana-plugin/src/models/schedule/schedule.helpers.ts b/grafana-plugin/src/models/schedule/schedule.helpers.ts index 6464db0b..81e4ca71 100644 --- a/grafana-plugin/src/models/schedule/schedule.helpers.ts +++ b/grafana-plugin/src/models/schedule/schedule.helpers.ts @@ -63,7 +63,7 @@ export const fillGaps = (events: Event[]) => { return newEvents; }; -export const splitToShiftsAndFillGaps = (events: Event[]) => { +export const splitToShifts = (events: Event[]) => { const shifts: Array<{ shiftId: Shift['id']; priority: Shift['priority_level']; events: Event[] }> = []; for (const [_i, event] of events.entries()) { @@ -77,13 +77,20 @@ export const splitToShiftsAndFillGaps = (events: Event[]) => { } } - shifts.forEach((shift) => { - shift.events = fillGaps(shift.events); - }); - return shifts; }; +export const fillGapsInShifts = (shifts: ShiftEvents[]) => { + return shifts.map((shift) => ({ + ...shift, + events: fillGaps(shift.events), + })); +}; + +export const enrichEventsWithScheduleData = (events: Event[], schedule: Partial) => { + return events.map((event) => ({ ...event, schedule })); +}; + export const getPersonalShiftsFromStore = ( store: RootStore, userPk: User['pk'], @@ -102,6 +109,44 @@ export const getShiftsFromStore = ( : (store.scheduleStore.events[scheduleId]?.['final']?.[getFromString(startMoment)] as any); }; +export const unFlattenShiftEvents = (shifts: ShiftEvents[]) => { + for (let i = 0; i < shifts.length; i++) { + const shift = shifts[i]; + + for (let j = 0; j < shift.events.length - 1; j++) { + for (let k = j + 1; k < shift.events.length; k++) { + const event1 = shift.events[j]; + const event2 = shift.events[k]; + + const event1Start = dayjs(event1.start); + const event1End = dayjs(event1.end); + + const event2Start = dayjs(event2.start); + const event2End = dayjs(event2.end); + + if ( + (event1Start.isBefore(event2Start) && event1End.isAfter(event2Start)) || + (event1End.isAfter(event2End) && event1Start.isBefore(event2End)) + ) { + const firstEvent = event1Start.isBefore(event2Start) ? event1 : event2; + const secondEvent = firstEvent === event1 ? event2 : event1; + + const oldShift = { ...shift, events: shift.events.filter((event) => event !== secondEvent) }; + + const newShift = { ...shift, events: [secondEvent] }; + + shifts[i] = oldShift; + shifts.push(newShift); + + return unFlattenShiftEvents(shifts); + } + } + } + } + + return shifts; +}; + export const flattenShiftEvents = (shifts: ShiftEvents[]) => { if (!shifts) { return undefined; @@ -241,9 +286,7 @@ export const getOverridesFromStore = ( : (store.scheduleStore.events[scheduleId]?.['override']?.[getFromString(startMoment)] as ShiftEvents[]); }; -export const splitToLayers = ( - shifts: Array<{ shiftId: Shift['id']; priority: Shift['priority_level']; events: Event[] }> -) => { +export const splitToLayers = (shifts: ShiftEvents[]) => { return shifts .reduce((memo, shift) => { let layer = memo.find((level) => level.priority === shift.priority); @@ -395,7 +438,7 @@ export const getOverrideColor = (rotationIndex: number) => { return OVERRIDE_COLORS[normalizedRotationIndex]; }; -export const getShiftName = (shift: Shift) => { +export const getShiftName = (shift: Partial) => { if (!shift) { return ''; } @@ -408,5 +451,5 @@ export const getShiftName = (shift: Shift) => { return 'Override'; } - return `[L${shift.priority_level}] Rotation`; + return 'Rotation'; }; diff --git a/grafana-plugin/src/models/schedule/schedule.ts b/grafana-plugin/src/models/schedule/schedule.ts index 08749df3..4ea418a1 100644 --- a/grafana-plugin/src/models/schedule/schedule.ts +++ b/grafana-plugin/src/models/schedule/schedule.ts @@ -11,12 +11,15 @@ import { SelectOption } from 'state/types'; import { createShiftSwapEventFromShiftSwap, + enrichEventsWithScheduleData, enrichLayers, enrichOverrides, + fillGapsInShifts, flattenShiftEvents, getFromString, splitToLayers, - splitToShiftsAndFillGaps, + splitToShifts, + unFlattenShiftEvents, } from './schedule.helpers'; import { Rotation, @@ -34,7 +37,7 @@ import { export class ScheduleStore extends BaseStore { @observable - searchResult: { count?: number; results?: Array } = {}; + searchResult: { page_size?: number; count?: number; results?: Array } = {}; @observable.shallow items: { [id: string]: Schedule } = {}; @@ -137,7 +140,7 @@ export class ScheduleStore extends BaseStore { shouldUpdateFn: () => boolean = undefined ) { const filters = typeof f === 'string' ? { search: f } : f; - const { count, results } = await makeRequest(this.path, { + const { page_size, count, results } = await makeRequest(this.path, { method: 'GET', params: { ...filters, page }, }); @@ -157,6 +160,7 @@ export class ScheduleStore extends BaseStore { ), }; this.searchResult = { + page_size, count, results: results.map((item: Schedule) => item.id), }; @@ -193,6 +197,7 @@ export class ScheduleStore extends BaseStore { return undefined; } return { + page_size: this.searchResult.page_size, count: this.searchResult.count, results: this.searchResult.results?.map((scheduleId: Schedule['id']) => this.items[scheduleId]), }; @@ -287,7 +292,7 @@ export class ScheduleStore extends BaseStore { this.rotationPreview = { ...this.rotationPreview, [fromString]: layers }; } - this.finalPreview = { ...this.finalPreview, [fromString]: splitToShiftsAndFillGaps(response.final) }; + this.finalPreview = { ...this.finalPreview, [fromString]: fillGapsInShifts(splitToShifts(response.final)) }; } @action @@ -450,7 +455,9 @@ export class ScheduleStore extends BaseStore { }); const fromString = getFromString(startMoment); - const shifts = splitToShiftsAndFillGaps(response.events); + const shiftsRaw = splitToShifts(response.events); + const shiftsUnflattened = unFlattenShiftEvents(shiftsRaw); + const shifts = fillGapsInShifts(shiftsUnflattened); const layers = type === 'rotation' ? splitToLayers(shifts) : undefined; this.events = { @@ -535,7 +542,7 @@ export class ScheduleStore extends BaseStore { }; } - async updatePersonalEvents(userPk: User['pk'], startMoment: dayjs.Dayjs, days = 9) { + async updatePersonalEvents(userPk: User['pk'], startMoment: dayjs.Dayjs, days = 9, isUpdateOnCallNow = false) { const fromString = getFromString(startMoment); const dayBefore = startMoment.subtract(1, 'day'); @@ -548,8 +555,8 @@ export class ScheduleStore extends BaseStore { }, }); - const shiftEventsList = schedules.reduce((acc, schedule) => { - return [...acc, ...splitToShiftsAndFillGaps(schedule.events)]; + const shiftEventsList = schedules.reduce((acc, { events, id, name }) => { + return [...acc, ...fillGapsInShifts(splitToShifts(enrichEventsWithScheduleData(events, { id, name })))]; }, []); const shiftEventsListFlattened = flattenShiftEvents(shiftEventsList); @@ -562,9 +569,12 @@ export class ScheduleStore extends BaseStore { }, }; - this.onCallNow = { - ...this.onCallNow, - [userPk]: is_oncall, - }; + if (isUpdateOnCallNow) { + // since current endpoint works incorrectly we are waiting for https://github.com/grafana/oncall/issues/3164 + this.onCallNow = { + ...this.onCallNow, + [userPk]: is_oncall, + }; + } } } diff --git a/grafana-plugin/src/models/schedule/schedule.types.ts b/grafana-plugin/src/models/schedule/schedule.types.ts index 2ce3272e..2acd94be 100644 --- a/grafana-plugin/src/models/schedule/schedule.types.ts +++ b/grafana-plugin/src/models/schedule/schedule.types.ts @@ -94,7 +94,7 @@ export interface Event { is_gap: boolean; missing_users: Array<{ display_name: User['username']; pk: User['pk'] }>; priority_level: number; - shift: { pk: Shift['id'] | null }; + shift: Pick & { pk: string }; source: string; start: string; users: Array<{ @@ -104,6 +104,7 @@ export interface Event { }>; is_override: boolean; + schedule?: Partial; // populated by frontend for personal schedule to display schedule name instead of user name shiftSwapId?: ShiftSwap['id']; // if event is acually shift swap request (filled out by frontend) } diff --git a/grafana-plugin/src/pages/schedule/Schedule.tsx b/grafana-plugin/src/pages/schedule/Schedule.tsx index 3b95bcd8..a386332d 100644 --- a/grafana-plugin/src/pages/schedule/Schedule.tsx +++ b/grafana-plugin/src/pages/schedule/Schedule.tsx @@ -156,6 +156,12 @@ class SchedulePage extends React.Component shiftIdToShowRotationForm || shiftSwapIdToShowForm; + const disabledShiftSwaps = + !isUserActionAllowed(UserActions.SchedulesWrite) || + !!shiftIdToShowOverridesForm || + shiftIdToShowRotationForm || + shiftSwapIdToShowForm; + return ( {() => ( @@ -314,6 +320,7 @@ class SchedulePage extends React.Component shiftIdToShowRotationForm={shiftIdToShowOverridesForm} onShowRotationForm={this.handleShowOverridesForm} disabled={disabledOverrideForm} + disableShiftSwaps={disabledShiftSwaps} shiftStartToShowOverrideForm={shiftStartToShowOverrideForm} shiftEndToShowOverrideForm={shiftEndToShowOverrideForm} onShowShiftSwapForm={!shiftSwapIdToShowForm ? this.handleShowShiftSwapForm : undefined} diff --git a/grafana-plugin/src/pages/schedules/Schedules.tsx b/grafana-plugin/src/pages/schedules/Schedules.tsx index 041cda71..c366c877 100644 --- a/grafana-plugin/src/pages/schedules/Schedules.tsx +++ b/grafana-plugin/src/pages/schedules/Schedules.tsx @@ -25,7 +25,7 @@ import SchedulePersonal from 'containers/Rotations/SchedulePersonal'; import ScheduleForm from 'containers/ScheduleForm/ScheduleForm'; import TeamName from 'containers/TeamName/TeamName'; import { WithPermissionControlTooltip } from 'containers/WithPermissionControl/WithPermissionControlTooltip'; -import { Schedule, ScheduleType } from 'models/schedule/schedule.types'; +import { Schedule } from 'models/schedule/schedule.types'; import { getSlackChannelName } from 'models/slack_channel/slack_channel.helpers'; import { Timezone } from 'models/timezone/timezone.types'; import { getStartOfWeek } from 'pages/schedule/Schedule.helpers'; @@ -39,7 +39,7 @@ import styles from './Schedules.module.css'; const cx = cn.bind(styles); const FILTERS_DEBOUNCE_MS = 500; -const ITEMS_PER_PAGE = 10; +const PAGE_SIZE_DEFAULT = 15; interface SchedulesPageProps extends WithStoreProps, RouteComponentProps, PageProps {} @@ -83,7 +83,7 @@ class SchedulesPage extends React.Component { - console.log(rest); - }} />
@@ -179,7 +176,11 @@ class SchedulesPage extends React.Component { const { history, query } = this.props; - if (data.type === ScheduleType.API) { - history.push(`${PLUGIN_ROOT}/schedules/${data.id}?${qs.stringify(query)}`); - } + history.push(`${PLUGIN_ROOT}/schedules/${data.id}?${qs.stringify(query)}`); }; handleExpandRow = (expanded: boolean, data: Schedule) => { @@ -455,7 +454,7 @@ class SchedulesPage extends React.Component Date: Thu, 19 Oct 2023 14:30:12 -0300 Subject: [PATCH 5/7] Update shifts public API to improve web shifts support (#3165) --- CHANGELOG.md | 1 + .../public_api/serializers/on_call_shifts.py | 12 +- .../public_api/tests/test_on_call_shifts.py | 112 +++++++++++++++++- .../apps/public_api/views/on_call_shifts.py | 5 +- 4 files changed, 127 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f9c1cc8..e20b8e5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Use shift data from event object +- Update shifts public API to improve web shifts support ([#3165](https://github.com/grafana/oncall/pull/3165)) ### Fixed diff --git a/engine/apps/public_api/serializers/on_call_shifts.py b/engine/apps/public_api/serializers/on_call_shifts.py index ffdf2cb1..7b9dfb83 100644 --- a/engine/apps/public_api/serializers/on_call_shifts.py +++ b/engine/apps/public_api/serializers/on_call_shifts.py @@ -5,6 +5,7 @@ from rest_framework import fields, serializers from apps.schedules.models import CustomOnCallShift from apps.user_management.models import User from common.api_helpers.custom_fields import ( + OrganizationFilteredPrimaryKeyRelatedField, RollingUsersField, TeamPrimaryKeyRelatedField, TimeZoneField, @@ -70,6 +71,7 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer id = serializers.CharField(read_only=True, source="public_primary_key") organization = serializers.HiddenField(default=CurrentOrganizationDefault()) team_id = TeamPrimaryKeyRelatedField(required=False, allow_null=True, source="team") + schedule = OrganizationFilteredPrimaryKeyRelatedField(read_only=True) type = CustomOnCallShiftTypeField() time_zone = TimeZoneField(required=False, allow_null=True) users = UsersFilteredByOrganizationField(queryset=User.objects, required=False) @@ -92,6 +94,7 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer "id", "organization", "team_id", + "schedule", "name", "type", "time_zone", @@ -116,7 +119,8 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer "source": {"required": False, "write_only": True}, } - PREFETCH_RELATED = ["users"] + SELECT_RELATED = ["schedule"] + PREFETCH_RELATED = ["schedules", "users"] def create(self, validated_data): self._validate_frequency_and_week_start( @@ -244,6 +248,9 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer def to_representation(self, instance): result = super().to_representation(instance) + if result["schedule"] is None: + related_schedules = instance.schedules.all() + result["schedule"] = related_schedules[0].public_primary_key if related_schedules else None result["duration"] = int(instance.duration.total_seconds()) result["start"] = instance.start.strftime("%Y-%m-%dT%H:%M:%S") result["rotation_start"] = instance.rotation_start.strftime("%Y-%m-%dT%H:%M:%S") @@ -377,4 +384,7 @@ class CustomOnCallShiftUpdateSerializer(CustomOnCallShiftSerializer): result = super().update(instance, validated_data) for schedule in instance.schedules.all(): instance.start_drop_ical_and_check_schedule_tasks(schedule) + if instance.schedule: + # web-schedule shifts use FK instead + instance.start_drop_ical_and_check_schedule_tasks(instance.schedule) return result diff --git a/engine/apps/public_api/tests/test_on_call_shifts.py b/engine/apps/public_api/tests/test_on_call_shifts.py index 1a66fa09..ce531e06 100644 --- a/engine/apps/public_api/tests/test_on_call_shifts.py +++ b/engine/apps/public_api/tests/test_on_call_shifts.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + import pytest from django.urls import reverse from django.utils import timezone @@ -48,6 +50,43 @@ invalid_field_data_10 = { } +@pytest.mark.django_db +def test_filter_on_call_shift_schedule(make_organization_and_user_with_token, make_on_call_shift, make_schedule): + organization, user, token = make_organization_and_user_with_token() + client = APIClient() + + schedule_1 = make_schedule(organization, schedule_class=OnCallScheduleWeb) + schedule_2 = make_schedule(organization, schedule_class=OnCallScheduleWeb) + start_date = timezone.now().replace(microsecond=0) + shifts = [] + for schedule in (schedule_1, schedule_2): + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(seconds=7200), + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_SINGLE_EVENT, **data + ) + on_call_shift.users.add(user) + shifts.append(on_call_shift) + + url = reverse("api-public:on_call_shifts-list") + response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") + assert response.status_code == status.HTTP_200_OK + expected = sorted([s.public_primary_key for s in shifts]) + returned = sorted([s["id"] for s in response.json()["results"]]) + assert returned == expected + + url += f"?schedule_id={schedule_1.public_primary_key}" + response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") + assert response.status_code == status.HTTP_200_OK + expected = [shifts[0].public_primary_key] + returned = [s["id"] for s in response.json()["results"]] + assert returned == expected + + @pytest.mark.django_db def test_get_on_call_shift(make_organization_and_user_with_token, make_on_call_shift, make_schedule): organization, user, token = make_organization_and_user_with_token() @@ -73,6 +112,7 @@ def test_get_on_call_shift(make_organization_and_user_with_token, make_on_call_s result = { "id": on_call_shift.public_primary_key, "team_id": None, + "schedule": schedule.public_primary_key, "name": on_call_shift.name, "type": "single_event", "time_zone": None, @@ -111,6 +151,7 @@ def test_get_override_on_call_shift(make_organization_and_user_with_token, make_ result = { "id": on_call_shift.public_primary_key, "team_id": None, + "schedule": schedule.public_primary_key, "name": on_call_shift.name, "type": "override", "time_zone": None, @@ -155,6 +196,7 @@ def test_create_on_call_shift(make_organization_and_user_with_token): result = { "id": on_call_shift.public_primary_key, "team_id": None, + "schedule": None, "name": data["name"], "type": "recurrent_event", "time_zone": None, @@ -206,6 +248,7 @@ def test_create_on_call_shift_using_default_interval(make_organization_and_user_ expected = { "id": on_call_shift.public_primary_key, "team_id": None, + "schedule": None, "name": data["name"], "type": "recurrent_event", "time_zone": None, @@ -282,6 +325,7 @@ def test_create_override_on_call_shift(make_organization_and_user_with_token): result = { "id": on_call_shift.public_primary_key, "team_id": None, + "schedule": None, "name": data["name"], "type": "override", "time_zone": None, @@ -360,11 +404,13 @@ def test_update_on_call_shift(make_organization_and_user_with_token, make_on_cal assert on_call_shift.by_day != data_to_update["by_day"] assert len(on_call_shift.users.filter(public_primary_key=user.public_primary_key)) == 0 - response = client.put(url, data=data_to_update, format="json", HTTP_AUTHORIZATION=f"{token}") + with patch("apps.schedules.models.CustomOnCallShift.start_drop_ical_and_check_schedule_tasks") as mock_drop_ical: + response = client.put(url, data=data_to_update, format="json", HTTP_AUTHORIZATION=f"{token}") result = { "id": on_call_shift.public_primary_key, "team_id": None, + "schedule": schedule.public_primary_key, "name": on_call_shift.name, "type": "recurrent_event", "time_zone": None, @@ -389,6 +435,69 @@ def test_update_on_call_shift(make_organization_and_user_with_token, make_on_cal assert on_call_shift.by_day == data_to_update["by_day"] assert len(on_call_shift.users.filter(public_primary_key=user.public_primary_key)) == 1 assert response.data == result + mock_drop_ical.assert_called_once_with(schedule) + + +@pytest.mark.django_db +def test_update_on_call_shift_web_schedule(make_organization_and_user_with_token, make_on_call_shift, make_schedule): + organization, user, token = make_organization_and_user_with_token() + client = APIClient() + + start_date = timezone.now().replace(microsecond=0) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(seconds=7200), + "frequency": CustomOnCallShift.FREQUENCY_WEEKLY, + "interval": 2, + "by_day": ["MO", "FR"], + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user]]) + + url = reverse("api-public:on_call_shifts-detail", kwargs={"pk": on_call_shift.public_primary_key}) + data_to_update = { + "duration": 14400, + "by_day": ["MO", "WE", "FR"], + } + assert int(on_call_shift.duration.total_seconds()) != data_to_update["duration"] + assert on_call_shift.by_day != data_to_update["by_day"] + + with patch("apps.schedules.models.CustomOnCallShift.start_drop_ical_and_check_schedule_tasks") as mock_drop_ical: + response = client.put(url, data=data_to_update, format="json", HTTP_AUTHORIZATION=f"{token}") + + result = { + "id": on_call_shift.public_primary_key, + "team_id": None, + "schedule": schedule.public_primary_key, + "name": on_call_shift.name, + "type": "rolling_users", + "time_zone": None, + "level": 0, + "start": on_call_shift.start.strftime("%Y-%m-%dT%H:%M:%S"), + "rotation_start": on_call_shift.rotation_start.strftime("%Y-%m-%dT%H:%M:%S"), + "duration": data_to_update["duration"], + "frequency": "weekly", + "interval": on_call_shift.interval, + "until": None, + "week_start": "SU", + "by_day": data_to_update["by_day"], + "rolling_users": [[user.public_primary_key]], + "start_rotation_from_user_index": None, + "by_month": None, + "by_monthday": None, + } + + assert response.status_code == status.HTTP_200_OK + on_call_shift.refresh_from_db() + assert int(on_call_shift.duration.total_seconds()) == data_to_update["duration"] + assert on_call_shift.by_day == data_to_update["by_day"] + assert response.data == result + mock_drop_ical.assert_called_once_with(schedule) @pytest.mark.django_db @@ -482,6 +591,7 @@ def test_create_web_override(make_organization_and_user_with_token, make_on_call expected_response = { "id": shift.public_primary_key, "team_id": None, + "schedule": None, "name": "test web override", "type": "override", "start": start_str, diff --git a/engine/apps/public_api/views/on_call_shifts.py b/engine/apps/public_api/views/on_call_shifts.py index 7b904b6a..af944b00 100644 --- a/engine/apps/public_api/views/on_call_shifts.py +++ b/engine/apps/public_api/views/on_call_shifts.py @@ -1,3 +1,4 @@ +from django.db.models import Q from django_filters.rest_framework import DjangoFilterBackend from rest_framework.exceptions import NotFound from rest_framework.permissions import IsAuthenticated @@ -35,7 +36,9 @@ class CustomOnCallShiftView(RateLimitHeadersMixin, UpdateSerializerMixin, ModelV queryset = CustomOnCallShift.objects.filter(organization=self.request.auth.organization) if schedule_id: - queryset = queryset.filter(schedules__public_primary_key=schedule_id) + queryset = queryset.filter( + Q(schedules__public_primary_key=schedule_id) | Q(schedule__public_primary_key=schedule_id) + ) if name: queryset = queryset.filter(name=name) return queryset.order_by("schedules") From 848bd1277f12062e61e06b71c6606aea3b25a057 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 19 Oct 2023 14:39:08 -0300 Subject: [PATCH 6/7] Handle None role when syncing users from Grafana (#3147) Fixes https://github.com/grafana/oncall-private/issues/2201 --- CHANGELOG.md | 1 + engine/apps/api/permissions.py | 5 ++- engine/apps/api/tests/test_alert_group.py | 14 +++++++ .../api/tests/test_alert_receive_channel.py | 16 ++++++++ .../test_alert_receive_channel_template.py | 2 + engine/apps/api/tests/test_channel_filter.py | 10 +++++ engine/apps/api/tests/test_custom_button.py | 5 +++ .../apps/api/tests/test_escalation_policy.py | 8 ++++ .../api/tests/test_integration_heartbeat.py | 5 +++ engine/apps/api/tests/test_oncall_shift.py | 8 ++++ engine/apps/api/tests/test_organization.py | 16 +++++--- .../api/tests/test_postmortem_messages.py | 5 +++ .../apps/api/tests/test_public_api_tokens.py | 4 ++ engine/apps/api/tests/test_schedule_export.py | 4 ++ engine/apps/api/tests/test_schedules.py | 12 ++++++ .../api/tests/test_set_general_log_channel.py | 1 + engine/apps/api/tests/test_shift_swaps.py | 7 ++++ engine/apps/api/tests/test_slack_channels.py | 2 + .../api/tests/test_slack_team_settings.py | 4 ++ engine/apps/api/tests/test_team.py | 1 + .../apps/api/tests/test_telegram_channel.py | 4 ++ engine/apps/api/tests/test_user.py | 11 ++++++ engine/apps/api/tests/test_user_groups.py | 1 + .../api/tests/test_user_schedule_export.py | 7 ++++ engine/apps/api/tests/test_webhooks.py | 5 +++ engine/apps/api/views/organization.py | 2 +- engine/apps/api/views/slack_channel.py | 8 +++- engine/apps/api/views/slack_team_settings.py | 12 +++++- engine/apps/api/views/user_group.py | 8 +++- engine/apps/slack/tests/test_reset_slack.py | 1 + .../test_alert_group_actions.py | 5 ++- .../migrations/0016_alter_user_role.py | 18 +++++++++ engine/apps/user_management/models/user.py | 4 +- .../apps/user_management/tests/test_sync.py | 38 +++++++++++++++++++ engine/conftest.py | 1 + 35 files changed, 238 insertions(+), 17 deletions(-) create mode 100644 engine/apps/user_management/migrations/0016_alter_user_role.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e20b8e5f..8977e56f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Update ical schedule creation/update to trigger final schedule refresh ([#3156](https://github.com/grafana/oncall/pull/3156)) +- Handle None role when syncing users from Grafana ([#3147](https://github.com/grafana/oncall/pull/3147)) - Polish "Build 'When I am on-call' for web UI" [#2915](https://github.com/grafana/oncall/issues/2915) - Fix iCal schedule incorrect view [#2001](https://github.com/grafana/oncall-private/issues/2001) - Fix rotation name rendering issue [#2324](https://github.com/grafana/oncall/issues/2324) diff --git a/engine/apps/api/permissions.py b/engine/apps/api/permissions.py index 20aecdc0..ce58e375 100644 --- a/engine/apps/api/permissions.py +++ b/engine/apps/api/permissions.py @@ -80,6 +80,7 @@ class LegacyAccessControlRole(enum.IntEnum): ADMIN = 0 EDITOR = 1 VIEWER = 2 + NONE = 3 @classmethod def choices(cls): @@ -99,9 +100,9 @@ RBACObjectPermissionsAttribute = typing.Dict[permissions.BasePermission, typing. def get_most_authorized_role(permissions: LegacyAccessControlCompatiblePermissions) -> LegacyAccessControlRole: if not permissions: - return LegacyAccessControlRole.VIEWER + return LegacyAccessControlRole.NONE - # ex. Admin is 0, Viewer is 2, thereby min makes sense here + # ex. Admin is 0, None is 3, thereby min makes sense here return min({p.fallback_role for p in permissions}, key=lambda r: r.value) diff --git a/engine/apps/api/tests/test_alert_group.py b/engine/apps/api/tests/test_alert_group.py index 03ac9ae3..ee52d202 100644 --- a/engine/apps/api/tests/test_alert_group.py +++ b/engine/apps/api/tests/test_alert_group.py @@ -848,6 +848,7 @@ def test_get_filter_escalation_chain( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_acknowledge_permissions( @@ -883,6 +884,7 @@ def test_alert_group_acknowledge_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_unacknowledge_permissions( @@ -917,6 +919,7 @@ def test_alert_group_unacknowledge_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_resolve_permissions( @@ -951,6 +954,7 @@ def test_alert_group_resolve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_unresolve_permissions( @@ -985,6 +989,7 @@ def test_alert_group_unresolve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_silence_permissions( @@ -1019,6 +1024,7 @@ def test_alert_group_silence_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_unsilence_permissions( @@ -1053,6 +1059,7 @@ def test_alert_group_unsilence_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_attach_permissions( @@ -1087,6 +1094,7 @@ def test_alert_group_attach_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_unattach_permissions( @@ -1121,6 +1129,7 @@ def test_alert_group_unattach_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_list_permissions( @@ -1155,6 +1164,7 @@ def test_alert_group_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_stats_permissions( @@ -1189,6 +1199,7 @@ def test_alert_group_stats_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_bulk_action_permissions( @@ -1221,6 +1232,7 @@ def test_alert_group_bulk_action_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_filters_permissions( @@ -1255,6 +1267,7 @@ def test_alert_group_filters_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_detail_permissions( @@ -1678,6 +1691,7 @@ def test_alert_group_status_field( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_group_preview_template_permissions( diff --git a/engine/apps/api/tests/test_alert_receive_channel.py b/engine/apps/api/tests/test_alert_receive_channel.py index 16579802..2539a1a0 100644 --- a/engine/apps/api/tests/test_alert_receive_channel.py +++ b/engine/apps/api/tests/test_alert_receive_channel.py @@ -264,6 +264,7 @@ def test_integration_search( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_create_permissions( @@ -294,6 +295,7 @@ def test_alert_receive_channel_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_update_permissions( @@ -331,6 +333,7 @@ def test_alert_receive_channel_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_delete_permissions( @@ -363,6 +366,7 @@ def test_alert_receive_channel_delete_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_list_permissions( @@ -394,6 +398,7 @@ def test_alert_receive_channel_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_detail_permissions( @@ -427,6 +432,7 @@ def test_alert_receive_channel_detail_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_send_demo_alert_permissions( @@ -462,6 +468,7 @@ def test_alert_receive_channel_send_demo_alert_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_integration_options_permissions( @@ -493,6 +500,7 @@ def test_alert_receive_channel_integration_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_preview_template_permissions( @@ -606,6 +614,7 @@ def test_alert_receive_channel_preview_template_dynamic_payload( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_change_team_permissions( @@ -669,6 +678,7 @@ def test_alert_receive_channel_change_team( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_counters_permissions( @@ -702,6 +712,7 @@ def test_alert_receive_channel_counters_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_counters_per_integration_permissions( @@ -928,6 +939,7 @@ def test_alert_receive_channel_send_demo_alert_not_enabled( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_get_connected_contact_points_permissions( @@ -965,6 +977,7 @@ def test_alert_receive_channel_get_connected_contact_points_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_get_contact_points_permissions( @@ -998,6 +1011,7 @@ def test_alert_receive_channel_get_contact_points_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_connect_contact_point_permissions( @@ -1035,6 +1049,7 @@ def test_alert_receive_channel_connect_contact_point_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_201_CREATED), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_create_contact_point_permissions( @@ -1072,6 +1087,7 @@ def test_alert_receive_channel_create_contact_point_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_disconnect_contact_point_permissions( diff --git a/engine/apps/api/tests/test_alert_receive_channel_template.py b/engine/apps/api/tests/test_alert_receive_channel_template.py index 728add84..111696cd 100644 --- a/engine/apps/api/tests/test_alert_receive_channel_template.py +++ b/engine/apps/api/tests/test_alert_receive_channel_template.py @@ -19,6 +19,7 @@ from apps.base.tests.messaging_backend import TestOnlyBackend (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_template_update_permissions( @@ -53,6 +54,7 @@ def test_alert_receive_channel_template_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_alert_receive_channel_template_detail_permissions( diff --git a/engine/apps/api/tests/test_channel_filter.py b/engine/apps/api/tests/test_channel_filter.py index 5c8167f4..31ab0a27 100644 --- a/engine/apps/api/tests/test_channel_filter.py +++ b/engine/apps/api/tests/test_channel_filter.py @@ -17,6 +17,7 @@ from apps.api.permissions import LegacyAccessControlRole (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_create_permissions( @@ -48,6 +49,7 @@ def test_channel_filter_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_update_permissions( @@ -87,6 +89,7 @@ def test_channel_filter_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_list_permissions( @@ -122,6 +125,7 @@ def test_channel_filter_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_retrieve_permissions( @@ -157,6 +161,7 @@ def test_channel_filter_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_delete_permissions( @@ -192,6 +197,7 @@ def test_channel_filter_delete_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_move_to_position_permissions( @@ -487,6 +493,7 @@ def test_channel_filter_update_invalid_notification_backends( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_channel_filter_convert_from_regex_to_jinja2( @@ -521,6 +528,9 @@ def test_channel_filter_convert_from_regex_to_jinja2( url = reverse("api-internal:channel_filter-detail", kwargs={"pk": regex_channel_filter.public_primary_key}) response = client.get(url, format="json", **make_user_auth_headers(user, token)) + if role == LegacyAccessControlRole.NONE: + assert response.status_code == status.HTTP_403_FORBIDDEN + return assert response.status_code == status.HTTP_200_OK # Check if preview of the filtering term migration is correct diff --git a/engine/apps/api/tests/test_custom_button.py b/engine/apps/api/tests/test_custom_button.py index 0c3660eb..240b6745 100644 --- a/engine/apps/api/tests/test_custom_button.py +++ b/engine/apps/api/tests/test_custom_button.py @@ -280,6 +280,7 @@ def test_delete_custom_button(custom_button_internal_api_setup, make_user_auth_h (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_custom_button_create_permissions( @@ -311,6 +312,7 @@ def test_custom_button_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_custom_button_update_permissions( @@ -348,6 +350,7 @@ def test_custom_button_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_custom_button_list_permissions( @@ -381,6 +384,7 @@ def test_custom_button_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_custom_button_retrieve_permissions( @@ -414,6 +418,7 @@ def test_custom_button_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_custom_button_delete_permissions( diff --git a/engine/apps/api/tests/test_escalation_policy.py b/engine/apps/api/tests/test_escalation_policy.py index f14d4daa..30609c41 100644 --- a/engine/apps/api/tests/test_escalation_policy.py +++ b/engine/apps/api/tests/test_escalation_policy.py @@ -141,6 +141,7 @@ def test_move_to_position_invalid_index(escalation_policy_internal_api_setup, ma (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_create_permissions( @@ -178,6 +179,7 @@ def test_escalation_policy_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_update_permissions( @@ -219,6 +221,7 @@ def test_escalation_policy_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_list_permissions( @@ -256,6 +259,7 @@ def test_escalation_policy_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_retrieve_permissions( @@ -293,6 +297,7 @@ def test_escalation_policy_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_delete_permissions( @@ -330,6 +335,7 @@ def test_escalation_policy_delete_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_escalation_options_permissions( @@ -367,6 +373,7 @@ def test_escalation_policy_escalation_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_delay_options_permissions( @@ -405,6 +412,7 @@ def test_escalation_policy_delay_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_escalation_policy_move_to_position_permissions( diff --git a/engine/apps/api/tests/test_integration_heartbeat.py b/engine/apps/api/tests/test_integration_heartbeat.py index 8954b74a..9b0cd9c7 100644 --- a/engine/apps/api/tests/test_integration_heartbeat.py +++ b/engine/apps/api/tests/test_integration_heartbeat.py @@ -188,6 +188,7 @@ def test_update_integration_heartbeat( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_integration_heartbeat_create_permissions( @@ -218,6 +219,7 @@ def test_integration_heartbeat_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_integration_heartbeat_update_permissions( @@ -257,6 +259,7 @@ def test_integration_heartbeat_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_integration_heartbeat_list_permissions( @@ -292,6 +295,7 @@ def test_integration_heartbeat_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_integration_heartbeat_timeout_options_permissions( @@ -323,6 +327,7 @@ def test_integration_heartbeat_timeout_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_integration_heartbeat_retrieve_permissions( diff --git a/engine/apps/api/tests/test_oncall_shift.py b/engine/apps/api/tests/test_oncall_shift.py index ab962707..af8191fe 100644 --- a/engine/apps/api/tests/test_oncall_shift.py +++ b/engine/apps/api/tests/test_oncall_shift.py @@ -1213,6 +1213,7 @@ def test_create_on_call_shift_override_in_past(on_call_shift_internal_api_setup, (LegacyAccessControlRole.ADMIN, status.HTTP_201_CREATED), (LegacyAccessControlRole.EDITOR, status.HTTP_201_CREATED), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_create_permissions( @@ -1245,6 +1246,7 @@ def test_on_call_shift_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_update_permissions( @@ -1292,6 +1294,7 @@ def test_on_call_shift_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_list_permissions( @@ -1323,6 +1326,7 @@ def test_on_call_shift_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_retrieve_permissions( @@ -1366,6 +1370,7 @@ def test_on_call_shift_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_delete_permissions( @@ -1409,6 +1414,7 @@ def test_on_call_shift_delete_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_frequency_options_permissions( @@ -1440,6 +1446,7 @@ def test_on_call_shift_frequency_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_days_options_permissions( @@ -1471,6 +1478,7 @@ def test_on_call_shift_days_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_on_call_shift_preview_permissions( diff --git a/engine/apps/api/tests/test_organization.py b/engine/apps/api/tests/test_organization.py index 04470364..058c20a6 100644 --- a/engine/apps/api/tests/test_organization.py +++ b/engine/apps/api/tests/test_organization.py @@ -1,3 +1,4 @@ +import os from unittest.mock import patch import pytest @@ -10,12 +11,11 @@ from apps.api.permissions import LegacyAccessControlRole @pytest.mark.django_db -@pytest.mark.parametrize("rbac_enabled", [True, False]) -def test_get_organization_rbac_enabled( - make_organization_and_user_with_plugin_token, make_user_auth_headers, rbac_enabled -): +def test_get_organization_rbac_enabled(make_organization_and_user_with_plugin_token, make_user_auth_headers): + is_rbac_enabled = os.getenv("ONCALL_TESTING_RBAC_ENABLED", "True") == "True" organization, user, token = make_organization_and_user_with_plugin_token() - organization.is_rbac_permissions_enabled = rbac_enabled + # set rbac enabled based on env variable (factories use this value) + organization.is_rbac_permissions_enabled = is_rbac_enabled organization.save() client = APIClient() @@ -23,7 +23,7 @@ def test_get_organization_rbac_enabled( response = client.get(url, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK - assert response.json()["rbac_enabled"] == rbac_enabled + assert response.json()["rbac_enabled"] == organization.is_rbac_permissions_enabled @pytest.mark.django_db @@ -49,6 +49,7 @@ def test_update_organization_settings(make_organization_and_user_with_plugin_tok (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_organization_retrieve_permissions( @@ -79,6 +80,7 @@ def test_organization_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_organization_update_permissions( @@ -110,6 +112,7 @@ def test_organization_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_organization_get_telegram_verification_code_permissions( @@ -134,6 +137,7 @@ def test_organization_get_telegram_verification_code_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_organization_get_channel_verification_code_permissions( diff --git a/engine/apps/api/tests/test_postmortem_messages.py b/engine/apps/api/tests/test_postmortem_messages.py index e2877ce0..a467a6a9 100644 --- a/engine/apps/api/tests/test_postmortem_messages.py +++ b/engine/apps/api/tests/test_postmortem_messages.py @@ -215,6 +215,7 @@ def test_delete_resolution_note( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_resolution_note_create_permissions( @@ -248,6 +249,7 @@ def test_resolution_note_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_resolution_note_update_permissions( @@ -292,6 +294,7 @@ def test_resolution_note_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_resolution_note_delete_permissions( @@ -334,6 +337,7 @@ def test_resolution_note_delete_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_resolution_note_list_permissions( @@ -366,6 +370,7 @@ def test_resolution_note_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_resolution_note_detail_permissions( diff --git a/engine/apps/api/tests/test_public_api_tokens.py b/engine/apps/api/tests/test_public_api_tokens.py index 5984a61e..2abc956a 100644 --- a/engine/apps/api/tests/test_public_api_tokens.py +++ b/engine/apps/api/tests/test_public_api_tokens.py @@ -13,6 +13,7 @@ from apps.api.permissions import LegacyAccessControlRole (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_public_api_tokens_retrieve_permissions( @@ -39,6 +40,7 @@ def test_public_api_tokens_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_public_api_tokens_list_permissions( @@ -65,6 +67,7 @@ def test_public_api_tokens_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_201_CREATED), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_public_api_tokens_create_permissions( @@ -96,6 +99,7 @@ def test_public_api_tokens_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_public_api_tokens_delete_permissions( diff --git a/engine/apps/api/tests/test_schedule_export.py b/engine/apps/api/tests/test_schedule_export.py index 02dc6a5b..f747067e 100644 --- a/engine/apps/api/tests/test_schedule_export.py +++ b/engine/apps/api/tests/test_schedule_export.py @@ -17,6 +17,7 @@ ICAL_URL = "https://calendar.google.com/calendar/ical/amixr.io_37gttuakhrtr75ano (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_get_schedule_export_token( @@ -52,6 +53,7 @@ def test_get_schedule_export_token( (LegacyAccessControlRole.ADMIN, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.EDITOR, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_export_token_not_found( @@ -85,6 +87,7 @@ def test_schedule_export_token_not_found( (LegacyAccessControlRole.ADMIN, status.HTTP_201_CREATED), (LegacyAccessControlRole.EDITOR, status.HTTP_201_CREATED), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_create_export_token( @@ -118,6 +121,7 @@ def test_schedule_create_export_token( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_delete_export_token( diff --git a/engine/apps/api/tests/test_schedules.py b/engine/apps/api/tests/test_schedules.py index 828aa80c..db37c36d 100644 --- a/engine/apps/api/tests/test_schedules.py +++ b/engine/apps/api/tests/test_schedules.py @@ -1693,6 +1693,7 @@ def test_filter_events_invalid_type( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_create_permissions( @@ -1731,6 +1732,7 @@ def test_schedule_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_update_permissions( @@ -1773,6 +1775,7 @@ def test_schedule_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_list_permissions( @@ -1811,6 +1814,7 @@ def test_schedule_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_retrieve_permissions( @@ -1849,6 +1853,7 @@ def test_schedule_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_delete_permissions( @@ -1887,6 +1892,7 @@ def test_schedule_delete_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_events_permissions( @@ -1925,6 +1931,7 @@ def test_events_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_filter_shift_swaps_permissions( @@ -1963,6 +1970,7 @@ def test_filter_shift_swaps_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_reload_ical_permissions( @@ -2001,6 +2009,7 @@ def test_reload_ical_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_notify_oncall_shift_freq_options_permissions( @@ -2025,6 +2034,7 @@ def test_schedule_notify_oncall_shift_freq_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_notify_empty_oncall_options_permissions( @@ -2049,6 +2059,7 @@ def test_schedule_notify_empty_oncall_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_schedule_mention_options_permissions( @@ -2073,6 +2084,7 @@ def test_schedule_mention_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_current_user_events_permissions( diff --git a/engine/apps/api/tests/test_set_general_log_channel.py b/engine/apps/api/tests/test_set_general_log_channel.py index cdcce180..ad6a708e 100644 --- a/engine/apps/api/tests/test_set_general_log_channel.py +++ b/engine/apps/api/tests/test_set_general_log_channel.py @@ -17,6 +17,7 @@ from apps.api.permissions import LegacyAccessControlRole (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_set_general_log_channel_permissions( diff --git a/engine/apps/api/tests/test_shift_swaps.py b/engine/apps/api/tests/test_shift_swaps.py index 741bacdc..e45ccb75 100644 --- a/engine/apps/api/tests/test_shift_swaps.py +++ b/engine/apps/api/tests/test_shift_swaps.py @@ -116,6 +116,7 @@ def test_list(ssr_setup, make_user_auth_headers, expand_users): (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_list_permissions( @@ -157,6 +158,7 @@ def test_retrieve(ssr_setup, make_user_auth_headers, expand_users): (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_retrieve_permissions( @@ -277,6 +279,7 @@ def test_create_swap_start_and_swap_end_must_include_time_zone( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_create_permissions( @@ -398,6 +401,7 @@ def test_update_swap_start_and_swap_end_must_include_time_zone( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_update_own_ssr_permissions(ssr_setup, make_user_auth_headers, role, expected_status): @@ -551,6 +555,7 @@ def test_related_shifts(ssr_setup, make_on_call_shift, make_user_auth_headers): (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_partial_update_own_ssr_permissions(ssr_setup, make_user_auth_headers, role, expected_status): @@ -670,6 +675,7 @@ def test_delete( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_delete_own_ssr_permissions(ssr_setup, make_user_auth_headers, role, expected_status): @@ -778,6 +784,7 @@ def test_take_deleted_ssr(ssr_setup, make_user_auth_headers): (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_take_permissions( diff --git a/engine/apps/api/tests/test_slack_channels.py b/engine/apps/api/tests/test_slack_channels.py index 70a083b1..21224e4f 100644 --- a/engine/apps/api/tests/test_slack_channels.py +++ b/engine/apps/api/tests/test_slack_channels.py @@ -16,6 +16,7 @@ from apps.api.permissions import LegacyAccessControlRole (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_slack_channels_list_permissions( @@ -46,6 +47,7 @@ def test_slack_channels_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_slack_channels_detail_permissions( diff --git a/engine/apps/api/tests/test_slack_team_settings.py b/engine/apps/api/tests/test_slack_team_settings.py index 9de0e2ba..b492617f 100644 --- a/engine/apps/api/tests/test_slack_team_settings.py +++ b/engine/apps/api/tests/test_slack_team_settings.py @@ -16,6 +16,7 @@ from apps.api.permissions import LegacyAccessControlRole (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_get_slack_settings_permissions( @@ -46,6 +47,7 @@ def test_get_slack_settings_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_update_slack_settings_permissions( @@ -76,6 +78,7 @@ def test_update_slack_settings_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_get_acknowledge_remind_options_permissions( @@ -106,6 +109,7 @@ def test_get_acknowledge_remind_options_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_get_unacknowledge_timeout_options_permissions( diff --git a/engine/apps/api/tests/test_team.py b/engine/apps/api/tests/test_team.py index 91c7d5fe..7b611e5c 100644 --- a/engine/apps/api/tests/test_team.py +++ b/engine/apps/api/tests/test_team.py @@ -116,6 +116,7 @@ def test_list_teams_for_non_member( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_list_teams_permissions( diff --git a/engine/apps/api/tests/test_telegram_channel.py b/engine/apps/api/tests/test_telegram_channel.py index fa340ec4..6eccd9c2 100644 --- a/engine/apps/api/tests/test_telegram_channel.py +++ b/engine/apps/api/tests/test_telegram_channel.py @@ -37,6 +37,7 @@ def test_not_authorized(make_organization_and_user_with_plugin_token, make_teleg (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_list_telegram_channels_permissions( @@ -61,6 +62,7 @@ def test_list_telegram_channels_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_get_telegram_channels_permissions( @@ -87,6 +89,7 @@ def test_get_telegram_channels_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_delete_telegram_channels_permissions( @@ -114,6 +117,7 @@ def test_delete_telegram_channels_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_set_default_telegram_channels_permissions( diff --git a/engine/apps/api/tests/test_user.py b/engine/apps/api/tests/test_user.py index 47a6ece9..924cbcee 100644 --- a/engine/apps/api/tests/test_user.py +++ b/engine/apps/api/tests/test_user.py @@ -327,6 +327,7 @@ def test_notification_chain_verbal( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_update_self_permissions( @@ -356,6 +357,7 @@ def test_user_update_self_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_update_other_permissions( @@ -384,6 +386,7 @@ def test_user_update_other_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_list_permissions( @@ -414,6 +417,7 @@ def test_user_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_detail_self_permissions( @@ -444,6 +448,7 @@ def test_user_detail_self_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_detail_other_permissions( @@ -470,6 +475,7 @@ def test_user_detail_other_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_get_own_verification_code( @@ -500,6 +506,7 @@ def test_user_get_own_verification_code( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_get_other_verification_code( @@ -572,6 +579,7 @@ def test_verification_code_provider_exception( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_verify_own_phone( @@ -607,6 +615,7 @@ Tests below are outdated (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_verify_another_phone( @@ -635,6 +644,7 @@ def test_user_verify_another_phone( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_get_own_telegram_verification_code( @@ -659,6 +669,7 @@ def test_user_get_own_telegram_verification_code( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_get_another_telegram_verification_code( diff --git a/engine/apps/api/tests/test_user_groups.py b/engine/apps/api/tests/test_user_groups.py index 2e45e727..28176b56 100644 --- a/engine/apps/api/tests/test_user_groups.py +++ b/engine/apps/api/tests/test_user_groups.py @@ -55,6 +55,7 @@ def test_usergroup_list_without_slack_installed( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_usergroup_permissions( diff --git a/engine/apps/api/tests/test_user_schedule_export.py b/engine/apps/api/tests/test_user_schedule_export.py index fd467477..eec82fa0 100644 --- a/engine/apps/api/tests/test_user_schedule_export.py +++ b/engine/apps/api/tests/test_user_schedule_export.py @@ -16,6 +16,7 @@ ICAL_URL = "https://calendar.google.com/calendar/ical/amixr.io_37gttuakhrtr75ano (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_get_user_schedule_export_token( @@ -47,6 +48,7 @@ def test_get_user_schedule_export_token( (LegacyAccessControlRole.ADMIN, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.EDITOR, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_schedule_export_token_not_found( @@ -73,6 +75,7 @@ def test_user_schedule_export_token_not_found( (LegacyAccessControlRole.ADMIN, status.HTTP_201_CREATED), (LegacyAccessControlRole.EDITOR, status.HTTP_201_CREATED), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_schedule_create_export_token( @@ -99,6 +102,7 @@ def test_user_schedule_create_export_token( (LegacyAccessControlRole.ADMIN, status.HTTP_409_CONFLICT), (LegacyAccessControlRole.EDITOR, status.HTTP_409_CONFLICT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_schedule_create_multiple_export_tokens_fails( @@ -130,6 +134,7 @@ def test_user_schedule_create_multiple_export_tokens_fails( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_schedule_delete_export_token( @@ -166,6 +171,7 @@ def test_user_schedule_delete_export_token( (LegacyAccessControlRole.ADMIN, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.EDITOR, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_cannot_get_another_users_schedule_token( @@ -198,6 +204,7 @@ def test_user_cannot_get_another_users_schedule_token( (LegacyAccessControlRole.ADMIN, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.EDITOR, status.HTTP_404_NOT_FOUND), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_user_cannot_delete_another_users_schedule_token( diff --git a/engine/apps/api/tests/test_webhooks.py b/engine/apps/api/tests/test_webhooks.py index 867e366f..f3162515 100644 --- a/engine/apps/api/tests/test_webhooks.py +++ b/engine/apps/api/tests/test_webhooks.py @@ -291,6 +291,7 @@ def test_delete_webhook(webhook_internal_api_setup, make_user_auth_headers): (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_webhook_create_permissions( @@ -322,6 +323,7 @@ def test_webhook_create_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_webhook_update_permissions( @@ -359,6 +361,7 @@ def test_webhook_update_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_webhook_list_permissions( @@ -392,6 +395,7 @@ def test_webhook_list_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_200_OK), (LegacyAccessControlRole.VIEWER, status.HTTP_200_OK), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_webhook_retrieve_permissions( @@ -425,6 +429,7 @@ def test_webhook_retrieve_permissions( (LegacyAccessControlRole.ADMIN, status.HTTP_204_NO_CONTENT), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_webhook_delete_permissions( diff --git a/engine/apps/api/views/organization.py b/engine/apps/api/views/organization.py index 63b3f1e9..2ec62976 100644 --- a/engine/apps/api/views/organization.py +++ b/engine/apps/api/views/organization.py @@ -22,7 +22,7 @@ class CurrentOrganizationView(APIView): permission_classes = (IsAuthenticated, RBACPermission) rbac_permissions = { - "get": [], + "get": [RBACPermission.Permissions.OTHER_SETTINGS_READ], "put": [RBACPermission.Permissions.OTHER_SETTINGS_WRITE], } diff --git a/engine/apps/api/views/slack_channel.py b/engine/apps/api/views/slack_channel.py index 59ba10f3..1ec5cbec 100644 --- a/engine/apps/api/views/slack_channel.py +++ b/engine/apps/api/views/slack_channel.py @@ -3,6 +3,7 @@ from rest_framework.filters import SearchFilter from rest_framework.permissions import IsAuthenticated from rest_framework.viewsets import GenericViewSet +from apps.api.permissions import RBACPermission from apps.api.serializers.slack_channel import SlackChannelSerializer from apps.auth_token.auth import PluginAuthentication from apps.slack.models import SlackChannel @@ -12,7 +13,7 @@ from common.api_helpers.paginators import HundredPageSizePaginator class SlackChannelView(PublicPrimaryKeyMixin, mixins.ListModelMixin, mixins.RetrieveModelMixin, GenericViewSet): authentication_classes = (PluginAuthentication,) - permission_classes = (IsAuthenticated,) + permission_classes = (IsAuthenticated, RBACPermission) pagination_class = HundredPageSizePaginator @@ -21,6 +22,11 @@ class SlackChannelView(PublicPrimaryKeyMixin, mixins.ListModelMixin, mixins.Retr serializer_class = SlackChannelSerializer search_fields = ["name"] + rbac_permissions = { + "list": [RBACPermission.Permissions.CHATOPS_READ], + "retrieve": [RBACPermission.Permissions.CHATOPS_READ], + } + def get_queryset(self): organization = self.request.auth.organization slack_team_identity = organization.slack_team_identity diff --git a/engine/apps/api/views/slack_team_settings.py b/engine/apps/api/views/slack_team_settings.py index e52f250b..e91aa19d 100644 --- a/engine/apps/api/views/slack_team_settings.py +++ b/engine/apps/api/views/slack_team_settings.py @@ -44,7 +44,11 @@ class SlackTeamSettingsAPIView(views.APIView): class AcknowledgeReminderOptionsAPIView(views.APIView): authentication_classes = (PluginAuthentication,) - permission_classes = (IsAuthenticated,) + permission_classes = (IsAuthenticated, RBACPermission) + + rbac_permissions = { + "get": [RBACPermission.Permissions.CHATOPS_READ], + } def get(self, request): choices = [] @@ -57,7 +61,11 @@ class AcknowledgeReminderOptionsAPIView(views.APIView): class UnAcknowledgeTimeoutOptionsAPIView(views.APIView): authentication_classes = (PluginAuthentication,) - permission_classes = (IsAuthenticated,) + permission_classes = (IsAuthenticated, RBACPermission) + + rbac_permissions = { + "get": [RBACPermission.Permissions.CHATOPS_READ], + } def get(self, request): choices = [] diff --git a/engine/apps/api/views/user_group.py b/engine/apps/api/views/user_group.py index 4f230bae..296fcf1e 100644 --- a/engine/apps/api/views/user_group.py +++ b/engine/apps/api/views/user_group.py @@ -2,6 +2,7 @@ from rest_framework import mixins, viewsets from rest_framework.filters import SearchFilter from rest_framework.permissions import IsAuthenticated +from apps.api.permissions import RBACPermission from apps.api.serializers.user_group import UserGroupSerializer from apps.auth_token.auth import PluginAuthentication from apps.slack.models import SlackUserGroup @@ -9,9 +10,14 @@ from apps.slack.models import SlackUserGroup class UserGroupViewSet(mixins.ListModelMixin, viewsets.GenericViewSet): authentication_classes = (PluginAuthentication,) - permission_classes = (IsAuthenticated,) + permission_classes = (IsAuthenticated, RBACPermission) serializer_class = UserGroupSerializer + rbac_permissions = { + "list": [RBACPermission.Permissions.CHATOPS_READ], + "retrieve": [RBACPermission.Permissions.CHATOPS_READ], + } + filter_backends = (SearchFilter,) search_fields = ("name", "handle") diff --git a/engine/apps/slack/tests/test_reset_slack.py b/engine/apps/slack/tests/test_reset_slack.py index df572177..d54d7f0f 100644 --- a/engine/apps/slack/tests/test_reset_slack.py +++ b/engine/apps/slack/tests/test_reset_slack.py @@ -20,6 +20,7 @@ from apps.user_management.models import User (LegacyAccessControlRole.ADMIN, status.HTTP_200_OK), (LegacyAccessControlRole.EDITOR, status.HTTP_403_FORBIDDEN), (LegacyAccessControlRole.VIEWER, status.HTTP_403_FORBIDDEN), + (LegacyAccessControlRole.NONE, status.HTTP_403_FORBIDDEN), ], ) def test_reset_slack_integration_permissions( diff --git a/engine/apps/slack/tests/test_scenario_steps/test_alert_group_actions.py b/engine/apps/slack/tests/test_scenario_steps/test_alert_group_actions.py index 75517f30..0732f6ff 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_alert_group_actions.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_alert_group_actions.py @@ -83,12 +83,13 @@ def _get_payload(action_type="button", **kwargs): @pytest.mark.parametrize("step_class", ALERT_GROUP_ACTIONS_STEPS) +@pytest.mark.parametrize("role", (LegacyAccessControlRole.VIEWER, LegacyAccessControlRole.NONE)) @pytest.mark.django_db def test_alert_group_actions_unauthorized( - step_class, make_organization_and_user_with_slack_identities, make_alert_receive_channel, make_alert_group + step_class, make_organization_and_user_with_slack_identities, make_alert_receive_channel, make_alert_group, role ): organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities( - role=LegacyAccessControlRole.VIEWER + role=role ) alert_receive_channel = make_alert_receive_channel(organization) diff --git a/engine/apps/user_management/migrations/0016_alter_user_role.py b/engine/apps/user_management/migrations/0016_alter_user_role.py new file mode 100644 index 00000000..e6b8f1d8 --- /dev/null +++ b/engine/apps/user_management/migrations/0016_alter_user_role.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.20 on 2023-10-18 18:10 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('user_management', '0015_auto_20230926_2203'), + ] + + operations = [ + migrations.AlterField( + model_name='user', + name='role', + field=models.PositiveSmallIntegerField(choices=[(0, 'ADMIN'), (1, 'EDITOR'), (2, 'VIEWER'), (3, 'NONE')]), + ), + ] diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 996b8427..e387f9d4 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -88,7 +88,7 @@ class UserManager(models.Manager["User"]): email=user["email"], name=user["name"], username=user["login"], - role=LegacyAccessControlRole[user["role"].upper()], + role=getattr(LegacyAccessControlRole, user["role"].upper(), LegacyAccessControlRole.NONE), avatar_url=user["avatarUrl"], permissions=user["permissions"], ) @@ -120,7 +120,7 @@ class UserManager(models.Manager["User"]): users_to_update = [] for user in organization.users.filter(user_id__in=existing_user_ids): grafana_user = grafana_users[user.user_id] - g_user_role = LegacyAccessControlRole[grafana_user["role"].upper()] + g_user_role = getattr(LegacyAccessControlRole, grafana_user["role"].upper(), LegacyAccessControlRole.NONE) if ( user.email != grafana_user["email"] diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index c63d8639..ac7eb99d 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -5,6 +5,7 @@ from django.conf import settings from django.test import override_settings from apps.alerts.models import AlertReceiveChannel +from apps.api.permissions import LegacyAccessControlRole from apps.grafana_plugin.helpers.client import GcomAPIClient, GrafanaAPIClient from apps.user_management.models import Team, User from apps.user_management.sync import check_grafana_incident_is_enabled, cleanup_organization, sync_organization @@ -62,6 +63,43 @@ def test_sync_users_for_organization(make_organization, make_user_for_organizati ) +@pytest.mark.django_db +def test_sync_users_for_organization_role_none(make_organization, make_user_for_organization): + organization = make_organization(grafana_url="https://test.test") + users = tuple(make_user_for_organization(organization, user_id=user_id) for user_id in (1, 2)) + + api_users = tuple( + { + "userId": user_id, + "email": "test@test.test", + "name": "Test", + "login": "test", + "role": "None", + "avatarUrl": "/test/1234", + "permissions": [], + } + for user_id in (2, 3) + ) + + User.objects.sync_for_organization(organization, api_users=api_users) + + assert organization.users.count() == 2 + + # check that excess users are deleted + assert not organization.users.filter(pk=users[0].pk).exists() + + # check that existing users are updated + updated_user = organization.users.filter(pk=users[1].pk).first() + assert updated_user is not None + assert updated_user.role == LegacyAccessControlRole.NONE + + # check that missing users are created + created_user = organization.users.filter(user_id=api_users[1]["userId"]).first() + assert created_user is not None + assert created_user.user_id == api_users[1]["userId"] + assert created_user.role == LegacyAccessControlRole.NONE + + @pytest.mark.django_db def test_sync_teams_for_organization(make_organization, make_team): organization = make_organization() diff --git a/engine/conftest.py b/engine/conftest.py index 270b1ec2..ae810e51 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -279,6 +279,7 @@ def get_user_permission_role_mapping_from_frontend_plugin_json() -> RoleMapping: plugin_json: PluginJSON = json.load(fp) role_mapping: RoleMapping = { + LegacyAccessControlRole.NONE: [], LegacyAccessControlRole.VIEWER: [], LegacyAccessControlRole.EDITOR: [], LegacyAccessControlRole.ADMIN: [], From bb0bee421ebd10fce3ef93693899c2870d0bfa58 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 19 Oct 2023 14:51:17 -0300 Subject: [PATCH 7/7] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8977e56f..f8ee2920 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ 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.45 (2023-10-19) ### Added