Allow users to configure custom alerts based on data-lake variables#2305
Allow users to configure custom alerts based on data-lake variables#2305rafaellehmkuhl wants to merge 4 commits intobluerobotics:masterfrom
Conversation
ES-Alexander
left a comment
ES-Alexander
left a comment
There was a problem hiding this comment.
Nice! :D
Haven't tried it yet. Code mostly seems reasonable, but I feel like it could be cleaned up to reduce some redundancy (although I may be misunderstanding ts/JS limitations), and I've suggested some ideas for improvement (some for now, some maybe for later).
Key new ideas are:
- turning alert triggers into data lake variables (so they can also trigger other things), and
- allowing alerts to compare data lake variables against each other.
| <!-- Conditions Section --> | ||
| <div class="flex flex-col gap-3"> | ||
| <div class="flex items-center justify-between"> | ||
| <label class="text-sm font-medium">Conditions (all must match)</label> |
There was a problem hiding this comment.
| <label class="text-sm font-medium">Conditions (all must match)</label> | |
| <label class="text-sm font-medium">Trigger when ALL conditions are met:</label> |
| :key="index" | ||
| class="flex flex-col gap-2 p-3 rounded bg-[#FFFFFF11]" | ||
| > | ||
| <!-- Variable Selection for this condition --> |
There was a problem hiding this comment.
Would maybe be good to have an "AND" here, above or before each condition that's not the first one. I didn't see the "all must match" initially, and that could make it clearer.
Even better would be if we could support changing between AND and OR combinations, but then things get complicated from having to determine/specify groupings of them, which is probably overly complex for an initial implementation.
| class="flex-1" | ||
| @update:model-value="onConditionVariableChanged(condition)" | ||
| /> | ||
| <v-tooltip text="Copy variable name for use in custom message" location="top"> |
There was a problem hiding this comment.
Uhh, aren't data lake references supposed to be made using the ID, not the name?
Variable names aren't guaranteed to be unique...
| variant="outlined" | ||
| density="compact" | ||
| hide-details | ||
| hint="Minimum time between alerts" |
There was a problem hiding this comment.
| hint="Minimum time between alerts" | |
| hint="Minimum time before this alert can be triggered again" |
| <div class="flex flex-col gap-2"> | ||
| <v-checkbox | ||
| v-model="isDurationEnabled" | ||
| label="Require minimum time meeting conditions" |
There was a problem hiding this comment.
| label="Require minimum time meeting conditions" | |
| label="Require sustained conditions (ignore brief spikes)" |
| const alert = globalAlerts.find((a) => a.id === id) | ||
| if (!alert) return | ||
|
|
||
| alert.enabled = enabled |
There was a problem hiding this comment.
As above, I think this should trigger a cleanup or setup of listeners depending on whether it is becoming disabled or enabled.
| // Set default compare value based on type | ||
| switch (type) { | ||
| case 'boolean': | ||
| condition.compareValue = true | ||
| break | ||
| case 'number': | ||
| condition.compareValue = 0 | ||
| break | ||
| default: | ||
| condition.compareValue = '' | ||
| } |
There was a problem hiding this comment.
Should this be using the getDefaultCompareValue function from the typescript file?
| /** | ||
| * Push a success alert | ||
| * @param {string} message - The alert message | ||
| * @param {Date} timeCreated - Optional time created | ||
| */ | ||
| export const pushSuccessAlert = (message: string, timeCreated: Date = new Date()): void => { | ||
| pushAlert(new Alert(AlertLevel.Success, message, timeCreated)) | ||
| } | ||
|
|
||
| /** | ||
| * Push an error alert | ||
| * @param {string} message - The alert message | ||
| * @param {Date} timeCreated - Optional time created | ||
| */ | ||
| export const pushErrorAlert = (message: string, timeCreated: Date = new Date()): void => { | ||
| pushAlert(new Alert(AlertLevel.Error, message, timeCreated)) | ||
| } | ||
|
|
||
| /** | ||
| * Push an info alert | ||
| * @param {string} message - The alert message | ||
| * @param {Date} timeCreated - Optional time created | ||
| */ | ||
| export const pushInfoAlert = (message: string, timeCreated: Date = new Date()): void => { | ||
| pushAlert(new Alert(AlertLevel.Info, message, timeCreated)) | ||
| } | ||
|
|
||
| /** | ||
| * Push a warning alert | ||
| * @param {string} message - The alert message | ||
| * @param {Date} timeCreated - Optional time created | ||
| */ | ||
| export const pushWarningAlert = (message: string, timeCreated: Date = new Date()): void => { | ||
| pushAlert(new Alert(AlertLevel.Warning, message, timeCreated)) | ||
| } | ||
|
|
||
| /** | ||
| * Push a critical alert | ||
| * @param {string} message - The alert message | ||
| * @param {Date} timeCreated - Optional time created | ||
| */ | ||
| export const pushCriticalAlert = (message: string, timeCreated: Date = new Date()): void => { | ||
| pushAlert(new Alert(AlertLevel.Critical, message, timeCreated)) | ||
| } |
There was a problem hiding this comment.
Is there a way to define these dynamically? This seems like excessive repetition that then needs to be maintained.
| export const getSortedAlerts = (): Alert[] => { | ||
| return [...alerts].sort((a, b) => a.time_created.getTime() - b.time_created.getTime()) | ||
| } |
There was a problem hiding this comment.
There's some nearly identical code in alert.ts.
| return [...alerts].sort((a, b) => a.time_created.getTime() - b.time_created.getTime()) | ||
| }) | ||
|
|
||
| // Wrapper functions that delegate to the manager |
There was a problem hiding this comment.
Why do we need these wrappers, if they have the same signatures as the manager's functions? Could we not just directly use the manager's functions?
|
Relevant to (and possibly closes) #2115 |
2 participants
Cockpit.0.0.0.2025-12-12.10.29.56.mp4
Helps #1616
Helps #2210
Fix #2304