-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RAM] Add Snooze UI and Unsnooze API #128214
Conversation
Not sure how to get the snooze end time to show up on the same line as "Snooze". EUI doesn't seem to like changing the text color in the middle of a context menu item the way it's spec'd out: Also having trouble getting the EUIButton to be any smaller than 112px, I think there might be some hard-coded EUI minimum size going on, so I compensated by shrinking the number field. @mdefazio does this look all right or should I keep hacking at EUI to get this to look exactly like the Figma? |
@Zacqary This looks great. Thanks for putting this together. I'm good with having 2 lines in the context menu for snooze time. The sizing of the number input and button also is no issue. |
@elasticmachine merge upstream |
|
||
describe('snoozeAlertRoute', () => { | ||
beforeAll(() => { | ||
jest.useFakeTimers('modern'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it turns out Jest fake timers doesn't actually work for mocking the output of Date
or moment
several imports deep. We just didn't realize this because the snooze_rule
test isn't actually validating that the snooze date is in the future.
I was able to use jest.spyOn
to mock Date.now()
for the dropdown UI tests, but I haven't been able to find a reliable solution to mock new Date()
or moment()
with no arguments. We can work around this by using new Date(Date.now()
or moment(Date.now())
in any code where we want to test against the current date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to help figure out a way, but I don't know if I understand the problem. Can you elaborate more on what you're trying to do and what's not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's code in the snooze
API that uses Moment to determine if the snoozeEndTime
is in the future:
moment().isBefore(snoozeEndTime);
And also in the new RuleStatusDropdown
to determine how far in the future snoozeEndTime
is:
moment(item.snoozeEndTime).fromNow(true)
We thought that using jest.useFakeTimers
would enable us to change the return result of new Date()
and Date.now()
. For example, in this test we called jest.setSystemTime(new Date(2020, 3, 1))
to set the value of now
to March 2020 (which, now that I think about it, oh god, why would anyone want to make anything permanently March 2020, oh no oh no oh no).
Turns out, this did not actually work as expected. It seems like Jest's fake timers system only applies to Date()
called within the Jest tests themselves, and not to any modules that the tests import.
We didn't realize that this was broken because snooze_rule.test.ts
isn't actually testing the part of the snooze
API that tries to check if the snooze end time is in the future. But we thought that maybe it was.
It appears that moment.fromNow()
uses Date.now()
to determine the value of now
, as opposed to moment([no arguments])
which uses new Date()
. jest.spyOn
allows us to mock the value of global.Date.now
throughout all imports, but mocking global.Date
requires you to mock the entire Date object and there's no easy way to just override the constructor.
So therefore, we can work around that limitation by doing something like:
moment(Date.now()).isBefore(snoozeEndTime);
in any code that we need to test.
Pinging @elastic/response-ops (Team:ResponseOps) |
@mdefazio Wanted to make sure we're expecting that selecting "Enabled" from the context menu will also cancel the snooze? Or do we only want the Cancel Snooze button to do this? |
@@ -695,17 +680,31 @@ export const RulesList: React.FunctionComponent = () => { | |||
{ | |||
field: 'executionStatus.status', | |||
name: i18n.translate( | |||
'xpack.triggersActionsUI.sections.rulesList.rulesListTable.columns.statusTitle', | |||
{ defaultMessage: 'Status' } | |||
'xpack.triggersActionsUI.sections.rulesList.rulesListTable.columns.lastResponseTitle', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zacqary Make sure to update also the current status filter
to be called Last response. I refer to this component https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/application/sections/rules_list/components/rule_status_filter.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new Status
filter needs to be created as well, but I would start with renaming current one to last response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform security privilege changes look fine, though I have a couple of concerns below.
private async unsnoozeWithOCC({ id }: { id: string }) { | ||
const { attributes, version } = await this.unsecuredSavedObjectsClient.get<RawRule>( | ||
'alert', | ||
id | ||
); | ||
|
||
try { | ||
await this.authorization.ensureAuthorized({ | ||
ruleTypeId: attributes.alertTypeId, | ||
consumer: attributes.consumer, | ||
operation: WriteOperations.Unsnooze, | ||
entity: AlertingAuthorizationEntity.Rule, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this, but it appears that the authorization check will never occur if: 1. the user tries to unsnooze an alert, and 2. the alert is not found. In that case it will throw a 404 error before the authZ check occurs (and thus, it will never get audited). At least that's what it appears to be doing.
It looks like the rest of the rules client functions the same way, too. Can you confirm? CC @XavierM
Yes, switching to 'Enabled' should also cancel the snooze. Mentioning again here, the design issue has been updated with mockups for the rule detail view. This replaces the previous Stack rule Enable/Mute switches for this UI. The functionality of this badge should not change. I've moved it to the left in the new layout so we can keep the snooze timing in the same place. Also, since this uses an EUIBadge as does the rule type descriptor, I think it makes sense to group these closer instead of different sides of the header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a first pass and it looks great! Some questions and thoughts
const rulesClient = context.alerting.getRulesClient(); | ||
const params = req.params; | ||
try { | ||
await rulesClient.unsnooze({ ...params }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my own information, we feel comfortable spreading the parameters here because the schema should ensure there isn't anything extra in the object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. This is how all the other routes do it.
updatedBy: await this.getUserName(), | ||
updatedAt: new Date().toISOString(), | ||
}); | ||
const updateOptions = { version }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't seem to pass this in when update the rule from the task runner - why is this different here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea I'm just copypasting from what the snooze
method does, and that was in turn copypasted from the muteAll
method, so I assume it does something for some reason
const INDEFINITELY = i18n.translate( | ||
'xpack.triggersActionsUI.sections.rulesList.remainingSnoozeIndefinite', | ||
{ defaultMessage: 'Indefinitely' } | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make these files smaller in the future? Like move some of these into a lib folder (which also makes it easier to unit test)?
}: ComponentOpts) => { | ||
const [isEnabled, setIsEnabled] = useState<boolean>(item.enabled); | ||
const [isSnoozed, setIsSnoozed] = useState<boolean>(isItemSnoozed(item)); | ||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to listen for this? I think the entire component will re-render when the items change, or what's the use case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because we store isEnabled
and isSnooze
in state to prevent the loading spinner from flickering before switching a status. Consequently we have to manually listen for any outside updates of the item
.
Here's a recording of what it looks like if we don't use these useState
/useEffect
combos and just relied on an item
prop update to re-render the dropdown:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so it sounds like we're doing this to make the UI for responsive - in that we update the UI right after the user interaction while simultaneously sending the request to the server? That's the goal here? And without the useState/useEffect
, we have to wait for the server to respond with the updated SO and if that takes a small amount of time (500ms
to 1s
) we see this weird flickering of the loading icon, then the updated value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the server has already responded with the updated SO, the wait time is just from React. Keeping things in useState/useEffect
gives us control over when things are rendered.
Note that this same code already existed in the previous switch control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay we can defer to discuss this in the future as it's not a new patttern!
[setIntervalUnit] | ||
); | ||
|
||
const onApply1h = useCallback(() => applySnooze(1, 'h'), [applySnooze]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird to hard code it like this - is it possible to define the commonly used time frames in some constant at the top and then build out the UI from that list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an i18n perspective, that could work if we were okay with Moment.js converting 1h
to an hour
instead of 1 hour
but I don't think we are? Unfortunately I'm not sure how to customize Moment's humanize
function to use 1
instead of an
(or equivalent, depending on locale; fr
uses une minute
for example).
If we're okay with changing the language then this would work but otherwise we're constrained by i18n.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
Closes #128011
Adds the Snooze UI to the rules list. Also adds the
_unsnooze
API, necessary for the Cancel Snooze button.Also handles "muting" as "snooze indefinitely":
Checklist
Delete any items that are not applicable to this PR.