Add crash notifications #4409
Add crash notifications #4409
Conversation
…fications to all members/owners
…eric notifications
This is 7 notifications, but the total in the top if 5 because the "2" on thr right include read messages, but reads like it doesn't... |
|
I wouldn't be showing the number on the right at all - if they view "read" messages, they can see them all, we're notifying them correctly, think it's fine |
|
Partly due to MVP aspect, I did not add a Is doable and will do that if insisted, however, I left like this for your eyes to see whether you think this is something to worry about now or later? PS: The way I chose to read this is: |
|
I know, but you're filtering on "unread only", but showing the count to include "read" messages, I think just remove the counters on the right for now please |
That was not the approach Joe. It is a possible approach, but not the chosen approach. This is a single updated notification using the
In other words, there is only ONE notification with a |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4409 +/- ##
==========================================
+ Coverage 78.34% 78.43% +0.09%
==========================================
Files 294 294
Lines 13588 13673 +85
Branches 3056 3078 +22
==========================================
+ Hits 10646 10725 +79
- Misses 2942 2948 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Discussed notification count conundrum with @joepavitt Decision was made to incorporate any "grouped" notifications into the overall counter pill. Implemented in include grouped notifications in the total count Demo: |
|
@joepavitt grouped notification count included in header pill & and tests added - ready for review. |
| substitutions (str) { | ||
| let result = str | ||
| const regex = /{{([^}]+)}}/g // find all {{key}} occurrences | ||
| let match = regex.exec(result) | ||
| while (match) { | ||
| const key = match[1] | ||
| const value = this.getObjectProperty(this.notification.data || {}, key) || key.split('.')[0].replace(/\b\w/g, l => l.toUpperCase()) | ||
| result = result.replace(match[0], value) | ||
| match = regex.exec(result) | ||
| } | ||
| return result | ||
| }, |
closes #4408
Description
Firstly, my apologies for what looks and smells like feature creep. That was not the intention. Initial implementation was done and as was soon discovered, the crash notifications were very quickly filling the notification area. This was discussed in slack and suggestions for change were made. Rather than throwing away good (and relevant) dev work, I kept it in.
notification.sendfor instances devices and devicesnotification.sendAPI to support upserting and supersedingoptions.supersedewas added first as discussed in slack. Essentially, this permits a repeated notification to to be auto-marked as read and its new (superseding) notification to be addedoptions.upsertwas implemented to "update" an existing notification (or add is not existing) while adding a counter to the data. This permits multiple successive notifications to be shown as one item (with a pill/badge) and not fill up the notification panel with read notificationsoptions.supersedeand discussed in slack as a viable option.options.upsert. It is left is as useful development and worthy of keeping as more notifications from other instances and occurrences could easily fill (and push unread items) down the list.Genericnotification that simplifies and reduces boilerplateMVP parts of this.
Ideally, the notification API and DB tables would natively support
severityandcounterbut as MVP, I added them into thedataobject of the notification under ametaobject. This may end up being a retro thing but until we have more notifications to add it is not fully clear - something to be aware of.Demo
Default view - [x] Hide Read
View with [ ] Hide Read unchecked
Tests
test/unit/forge/routes/logging/index_spec.jstest/unit/forge/routes/api/userNotifications_spec.js(was previously nameduserNotifications.jsand thus NEVER ran!Related Issue(s)
Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
area:migrationlabel