diff --git a/alert/queries.sql b/alert/queries.sql index fad07aea04..0a3e0f89c3 100644 --- a/alert/queries.sql +++ b/alert/queries.sql @@ -38,7 +38,7 @@ SELECT FROM alert_feedback WHERE - alert_id = ANY($1::int[]); + alert_id = ANY ($1::int[]); -- name: SetAlertFeedback :exec INSERT INTO alert_feedback(alert_id, noise_reason) @@ -48,3 +48,15 @@ ON CONFLICT (alert_id) noise_reason = $2 WHERE alert_feedback.alert_id = $1; + +-- name: SetManyAlertFeedback :many +INSERT INTO alert_feedback(alert_id, noise_reason) + VALUES (unnest(@alert_ids::bigint[]), @noise_reason) +ON CONFLICT (alert_id) + DO UPDATE SET + noise_reason = excluded.noise_reason + WHERE + alert_feedback.alert_id = excluded.alert_id + RETURNING + alert_id; + diff --git a/alert/store.go b/alert/store.go index d357103c81..c84944b627 100644 --- a/alert/store.go +++ b/alert/store.go @@ -822,6 +822,44 @@ func (s *Store) Feedback(ctx context.Context, alertIDs []int) ([]Feedback, error return result, nil } +func (s Store) UpdateManyAlertFeedback(ctx context.Context, noiseReason string, alertIDs []int) ([]int, error) { + err := permission.LimitCheckAny(ctx, permission.User) + if err != nil { + return nil, err + } + + err = validate.Many( + validate.Range("AlertIDs", len(alertIDs), 1, maxBatch), + validate.Text("NoiseReason", noiseReason, 1, 255), + ) + if err != nil { + return nil, err + } + + // GraphQL generates type of int[], while sqlc + // expects an int64[] as a result of the unnest function + ids := make([]int64, len(alertIDs)) + for i, v := range alertIDs { + ids[i] = int64(v) + } + + res, err := gadb.New(s.db).SetManyAlertFeedback(ctx, gadb.SetManyAlertFeedbackParams{ + AlertIds: ids, + NoiseReason: noiseReason, + }) + if err != nil { + return nil, err + } + + // cast back to []int + updatedIDs := make([]int, len(res)) + for i, v := range res { + updatedIDs[i] = int(v) + } + + return updatedIDs, nil +} + func (s Store) UpdateFeedback(ctx context.Context, feedback *Feedback) error { err := permission.LimitCheckAny(ctx, permission.System, permission.User) if err != nil { diff --git a/gadb/queries.sql.go b/gadb/queries.sql.go index c0d5e5ca45..b1be937ae8 100644 --- a/gadb/queries.sql.go +++ b/gadb/queries.sql.go @@ -23,7 +23,7 @@ SELECT FROM alert_feedback WHERE - alert_id = ANY($1::int[]) + alert_id = ANY ($1::int[]) ` type AlertFeedbackRow struct { @@ -733,6 +733,46 @@ func (q *Queries) SetAlertFeedback(ctx context.Context, arg SetAlertFeedbackPara return err } +const setManyAlertFeedback = `-- name: SetManyAlertFeedback :many +INSERT INTO alert_feedback(alert_id, noise_reason) + VALUES (unnest($1::bigint[]), $2) +ON CONFLICT (alert_id) + DO UPDATE SET + noise_reason = excluded.noise_reason + WHERE + alert_feedback.alert_id = excluded.alert_id + RETURNING + alert_id +` + +type SetManyAlertFeedbackParams struct { + AlertIds []int64 + NoiseReason string +} + +func (q *Queries) SetManyAlertFeedback(ctx context.Context, arg SetManyAlertFeedbackParams) ([]int64, error) { + rows, err := q.db.QueryContext(ctx, setManyAlertFeedback, pq.Array(arg.AlertIds), arg.NoiseReason) + if err != nil { + return nil, err + } + defer rows.Close() + var items []int64 + for rows.Next() { + var alert_id int64 + if err := rows.Scan(&alert_id); err != nil { + return nil, err + } + items = append(items, alert_id) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const statusMgrCMInfo = `-- name: StatusMgrCMInfo :one SELECT user_id, diff --git a/graphql2/generated.go b/graphql2/generated.go index 8c3260353e..d393e5f684 100644 --- a/graphql2/generated.go +++ b/graphql2/generated.go @@ -29371,7 +29371,7 @@ func (ec *executionContext) unmarshalInputUpdateAlertsInput(ctx context.Context, asMap[k] = v } - fieldsInOrder := [...]string{"alertIDs", "newStatus"} + fieldsInOrder := [...]string{"alertIDs", "newStatus", "noiseReason"} for _, k := range fieldsInOrder { v, ok := asMap[k] if !ok { @@ -29391,11 +29391,20 @@ func (ec *executionContext) unmarshalInputUpdateAlertsInput(ctx context.Context, var err error ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("newStatus")) - data, err := ec.unmarshalNAlertStatus2githubᚗcomᚋtargetᚋgoalertᚋgraphql2ᚐAlertStatus(ctx, v) + data, err := ec.unmarshalOAlertStatus2ᚖgithubᚗcomᚋtargetᚋgoalertᚋgraphql2ᚐAlertStatus(ctx, v) if err != nil { return it, err } it.NewStatus = data + case "noiseReason": + var err error + + ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("noiseReason")) + data, err := ec.unmarshalOString2ᚖstring(ctx, v) + if err != nil { + return it, err + } + it.NoiseReason = data } } diff --git a/graphql2/graphqlapp/alert.go b/graphql2/graphqlapp/alert.go index f63dd3479e..08be6e4ae4 100644 --- a/graphql2/graphqlapp/alert.go +++ b/graphql2/graphqlapp/alert.go @@ -22,6 +22,7 @@ import ( "github.com/target/goalert/service" "github.com/target/goalert/util/log" "github.com/target/goalert/util/timeutil" + "github.com/target/goalert/validation" "github.com/target/goalert/validation/validate" ) @@ -500,24 +501,37 @@ func (m *Mutation) EscalateAlerts(ctx context.Context, ids []int) ([]alert.Alert } func (m *Mutation) UpdateAlerts(ctx context.Context, args graphql2.UpdateAlertsInput) ([]alert.Alert, error) { - var status alert.Status - - err := validate.OneOf("Status", args.NewStatus, graphql2.AlertStatusStatusAcknowledged, graphql2.AlertStatusStatusClosed) - if err != nil { - return nil, err + if args.NewStatus != nil && args.NoiseReason != nil { + return nil, validation.NewGenericError("cannot set both 'newStatus' and 'noiseReason'") } - switch args.NewStatus { - case graphql2.AlertStatusStatusAcknowledged: - status = alert.StatusActive - case graphql2.AlertStatusStatusClosed: - status = alert.StatusClosed + var updatedIDs []int + if args.NewStatus != nil { + err := validate.OneOf("Status", *args.NewStatus, graphql2.AlertStatusStatusAcknowledged, graphql2.AlertStatusStatusClosed) + if err != nil { + return nil, err + } + + var status alert.Status + switch *args.NewStatus { + case graphql2.AlertStatusStatusAcknowledged: + status = alert.StatusActive + case graphql2.AlertStatusStatusClosed: + status = alert.StatusClosed + } + + updatedIDs, err = m.AlertStore.UpdateManyAlertStatus(ctx, status, args.AlertIDs, nil) + if err != nil { + return nil, err + } } - var updatedIDs []int - updatedIDs, err = m.AlertStore.UpdateManyAlertStatus(ctx, status, args.AlertIDs, nil) - if err != nil { - return nil, err + if args.NoiseReason != nil { + var err error + updatedIDs, err = m.AlertStore.UpdateManyAlertFeedback(ctx, *args.NoiseReason, args.AlertIDs) + if err != nil { + return nil, err + } } return m.AlertStore.FindMany(ctx, updatedIDs) diff --git a/graphql2/models_gen.go b/graphql2/models_gen.go index 7af777028d..0846521b6f 100644 --- a/graphql2/models_gen.go +++ b/graphql2/models_gen.go @@ -566,8 +566,9 @@ type UpdateAlertsByServiceInput struct { } type UpdateAlertsInput struct { - AlertIDs []int `json:"alertIDs"` - NewStatus AlertStatus `json:"newStatus"` + AlertIDs []int `json:"alertIDs"` + NewStatus *AlertStatus `json:"newStatus,omitempty"` + NoiseReason *string `json:"noiseReason,omitempty"` } type UpdateBasicAuthInput struct { diff --git a/graphql2/schema.graphql b/graphql2/schema.graphql index fd198f7a4b..d8776ef8e9 100644 --- a/graphql2/schema.graphql +++ b/graphql2/schema.graphql @@ -530,6 +530,7 @@ type Mutation { createAlert(input: CreateAlertInput!): Alert setAlertNoiseReason(input: SetAlertNoiseReasonInput!): Boolean! + @deprecated(reason: "Use updateAlerts instead with the noiseReason field.") createService(input: CreateServiceInput!): Service createEscalationPolicy(input: CreateEscalationPolicyInput!): EscalationPolicy @@ -935,7 +936,8 @@ input UpdateAlertsInput { # List of alertIDs. alertIDs: [Int!]! - newStatus: AlertStatus! + newStatus: AlertStatus + noiseReason: String } input UpdateRotationInput { diff --git a/web/src/app/alerts/AlertsList.tsx b/web/src/app/alerts/AlertsList.tsx index 90b31f10d0..7704333b54 100644 --- a/web/src/app/alerts/AlertsList.tsx +++ b/web/src/app/alerts/AlertsList.tsx @@ -7,6 +7,7 @@ import { ArrowUpward as EscalateIcon, Check as AcknowledgeIcon, Close as CloseIcon, + ThumbDownOffAlt, } from '@mui/icons-material' import AlertsListFilter from './components/AlertsListFilter' @@ -20,6 +21,7 @@ import { Time } from '../util/Time' import { NotificationContext } from '../main/SnackbarNotification' import ReactGA from 'react-ga4' import { useConfigValue } from '../util/RequireConfig' +import AlertFeedbackDialog from './components/AlertFeedbackDialog' interface AlertsListProps { serviceID: string @@ -35,8 +37,9 @@ interface StatusUnacknowledgedVariables { } interface MutationVariablesInput { - newStatus: string alertIDs: (string | number)[] + newStatus: string + noiseReason?: string } export const alertsListQuery = gql` @@ -106,8 +109,14 @@ function getStatusFilter(s: string): string[] { export default function AlertsList(props: AlertsListProps): JSX.Element { const classes = useStyles() + // event sent to Google Analytics const [event, setEvent] = useState('') const [analyticsID] = useConfigValue('General.GoogleAnalyticsID') as [string] + + // stores alertIDs, if length present, feedback dialog is shown + const [showFeedbackDialog, setShowFeedbackDialog] = useState>( + [], + ) const [selectedCount, setSelectedCount] = useState(0) const [checkedCount, setCheckedCount] = useState(0) @@ -148,6 +157,7 @@ export default function AlertsList(props: AlertsListProps): JSX.Element { const { setNotification } = useContext(NotificationContext) + // mutation to update alert status to either acknowledge, close, or escalate const [mutate] = useMutation(updateMutation, { onCompleted: (data) => { const numUpdated = @@ -170,6 +180,7 @@ export default function AlertsList(props: AlertsListProps): JSX.Element { }, }) + // alertIDs passed onClick from ControlledPaginatedList "checkedItems" const makeUpdateAlerts = (newStatus: string) => (alertIDs: (string | number)[]) => { setCheckedCount(alertIDs.length) @@ -191,9 +202,16 @@ export default function AlertsList(props: AlertsListProps): JSX.Element { case 'StatusClosed': setEvent('alertlist_closed') break + case 'noise': + setEvent('alertlist_noise') + break } - mutate({ mutation, variables }) + if (newStatus === 'noise') { + setShowFeedbackDialog(alertIDs.map((id) => id.toString())) + } else { + mutate({ mutation, variables }) + } } /* @@ -268,6 +286,12 @@ export default function AlertsList(props: AlertsListProps): JSX.Element { } } + actions.push({ + icon: , + label: 'Mark as Noise', + onClick: makeUpdateAlerts('noise'), + }) + return actions } @@ -325,6 +349,13 @@ export default function AlertsList(props: AlertsListProps): JSX.Element { /> + 0} + onClose={() => { + setShowFeedbackDialog([]) + }} + alertIDs={showFeedbackDialog} + /> ) } diff --git a/web/src/app/alerts/components/AlertFeedback.tsx b/web/src/app/alerts/components/AlertFeedback.tsx index 0fb1fc8014..df732c2adb 100644 --- a/web/src/app/alerts/components/AlertFeedback.tsx +++ b/web/src/app/alerts/components/AlertFeedback.tsx @@ -25,8 +25,11 @@ export const mutation = gql` interface AlertFeedbackProps { alertID: number + alertIDs?: Array } +export const options = ['False positive', 'Not actionable', 'Poor details'] + export default function AlertFeedback(props: AlertFeedbackProps): JSX.Element { const { alertID } = props @@ -37,8 +40,6 @@ export default function AlertFeedback(props: AlertFeedbackProps): JSX.Element { }, }) - const options = ['False positive', 'Not actionable', 'Poor details'] - const dataNoiseReason = data?.alert?.noiseReason ?? '' const getDefaults = (): [Array, string] => { diff --git a/web/src/app/alerts/components/AlertFeedbackDialog.tsx b/web/src/app/alerts/components/AlertFeedbackDialog.tsx new file mode 100644 index 0000000000..78e930b111 --- /dev/null +++ b/web/src/app/alerts/components/AlertFeedbackDialog.tsx @@ -0,0 +1,147 @@ +import React, { useState, useContext } from 'react' +import { useMutation, gql } from 'urql' +import Button from '@mui/material/Button' +import Checkbox from '@mui/material/Checkbox' +import Dialog from '@mui/material/Dialog' +import DialogContent from '@mui/material/DialogContent' +import DialogTitle from '@mui/material/DialogTitle' +import FormGroup from '@mui/material/FormGroup' +import FormControlLabel from '@mui/material/FormControlLabel' +import TextField from '@mui/material/TextField' +import { options } from './AlertFeedback' +import { DialogActions, Typography } from '@mui/material' +import { NotificationContext } from '../../main/SnackbarNotification' + +const updateMutation = gql` + mutation UpdateAlertsMutation($input: UpdateAlertsInput!) { + updateAlerts(input: $input) { + status + id + } + } +` + +interface AlertFeedbackDialogProps { + open: boolean + onClose: () => void + alertIDs: Array +} + +export default function AlertFeedbackDialog( + props: AlertFeedbackDialogProps, +): JSX.Element { + const { alertIDs, open, onClose } = props + + const [noiseReasons, setNoiseReasons] = useState>([]) + const [other, setOther] = useState('') + const [otherChecked, setOtherChecked] = useState(false) + const [mutationStatus, commit] = useMutation(updateMutation) + const { error } = mutationStatus + + const { setNotification } = useContext(NotificationContext) + + function handleCheck( + e: React.ChangeEvent, + noiseReason: string, + ): void { + if (e.target.checked) { + setNoiseReasons([...noiseReasons, noiseReason]) + } else { + setNoiseReasons(noiseReasons.filter((n) => n !== noiseReason)) + } + } + + function handleSubmit(): void { + let n = noiseReasons.slice() + if (other !== '' && otherChecked) n = [...n, other] + commit({ + input: { + alertIDs, + noiseReason: n.join('|'), + }, + }).then((result) => { + const numUpdated = result.data.updateAlerts.length + const count = alertIDs.length + + const msg = `${numUpdated} of ${count} alert${ + count === 1 ? '' : 's' + } updated` + + setNotification({ + message: msg, + severity: 'info', + }) + + onClose() + }) + } + + return ( + + + Mark the Following Alerts as Noise + + + Alerts: {alertIDs.join(', ')} + + {options.map((option) => ( + handleCheck(e, option)} + /> + } + /> + ))} + + setOtherChecked(true)} + onChange={(e) => { + setOther(e.target.value) + }} + /> + } + control={ + { + setOtherChecked(e.target.checked) + if (!e.target.checked) { + setOther('') + } + }} + /> + } + disableTypography + /> + + {error?.message && ( + + {error?.message} + + )} + + + + + + ) +} diff --git a/web/src/cypress/e2e/alerts.cy.ts b/web/src/cypress/e2e/alerts.cy.ts index 2591e02cfd..e9e3345106 100644 --- a/web/src/cypress/e2e/alerts.cy.ts +++ b/web/src/cypress/e2e/alerts.cy.ts @@ -269,6 +269,25 @@ function testAlerts(screen: ScreenFormat): void { // first two already acked, third now acked cy.get('[role="alert"]').should('contain', '1 of 3 alerts updated') }) + + it('should set a noise reason on multiple alerts', () => { + cy.get(`span[data-cy=item-${alert1.id}] input`).check() + cy.get(`span[data-cy=item-${alert2.id}] input`).check() + + cy.get('button[aria-label="Mark as Noise"]').click() + cy.dialogTitle('Mark the Following Alerts as Noise') + + cy.get('[placeholder="Other (please specify)"]').type('Test') + cy.dialogFinish('Submit') + + cy.get('[role="alert"]').should('contain', '2 of 2 alerts updated') + + cy.visit('/alerts/' + alert1.id) + cy.get('body').should('contain.text', 'Reason: Test') + + cy.visit('/alerts/' + alert2.id) + cy.get('body').should('contain.text', 'Reason: Test') + }) }) describe('Alert Creation', () => { diff --git a/web/src/schema.d.ts b/web/src/schema.d.ts index aa212234cb..00b9679d82 100644 --- a/web/src/schema.d.ts +++ b/web/src/schema.d.ts @@ -730,7 +730,8 @@ export type RotationType = 'weekly' | 'daily' | 'hourly' export interface UpdateAlertsInput { alertIDs: number[] - newStatus: AlertStatus + newStatus?: null | AlertStatus + noiseReason?: null | string } export interface UpdateRotationInput {