Update alert subscription UX

[kiahna-tucker]

Issues

The issues directly below are advanced by this PR:
#1960

Changes

1960

• changes...

Tests

Manually tested

• scenarios you manually tested

Automated tests

• unit testing covered

Playwright tests ran locally

• Admin
• Captures
• Collections
• HomePage
• Login
• Materialization

Screenshots

NOTE: Code that altered button label styling in the application was merged into this branch after screenshots were taken for this PR. The button labels in the Organization Notifications section of the Settings tab on the Admin page are in title case.

Table

Organization Notifications table

Dialogs

Configure Notification Methods dialog | Initialized

Configure Notification Methods dialog | Subscription summary expanded

Update Notification Methods dialog | Initialized

Update Notification Methods dialog | Subscription summary expanded

Recipients section | No recipients found

Alert Types List Item Interactions

Standard alert type | Deselected

Standard alert type | Deselected | Hovered

Standard alert type | Selected

Standard alert type | Selected | Hovered

System alert type

A list item corresponding to a system alert type does not have a hover state since it cannot be deselected.

Errors

Duplicate subscribed email detected | Subscriber summary expanded

Duplicate subscribed email detected | Subscriber summary collapsed

Multiple email input errors detected | Subscriber summary expanded

Multiple email input errors detected | Subscriber summary collapsed

[kiahna-tucker]

Not sure what was discussed, but I might expect to see some of the other alert configuration options here - at least shardFailed.

The data movement setting was the only global prefix setting requested at this time. Given the stage this feature is in and its UX breadth, I would advocate that support of additional, global prefix settings be considered fast follows.

Related to this, the UI doesn't appear to show any config inherited from parent prefixes. For example, if I configure a 24 hour interval for dataMovementStalled on the acmeCo/ prefix, when i open the notification dialog to acmeCo/nested/ , the dataMovementStalled dropdown says "None", even though the parent prefix is in effect there.

The EffectiveAlertConfig is currently used to populate the Interval field in the Data Movement Stalled section. This object is reported to correspond to the fully-resolved effective config at this scope, merging all ancestor prefix layers and controller defaults. I will take another look at the network calls to ensure that the client is making requests as expected. Please let me know if different object should be used.

UPDATE: This behavior is expected as no row exists for acmeCo/nested/ in the alert_configs database table at this time, leaving the server unaware of the new prefix until the user has clicked the Save button in the Configure Notification Methods dialog. The only sensible way to get around this in the absence of cleanup routines on/deletion APIs exposed by the backend would be to have a button (preferably inline) with the Prefix field that would create a row in the alert_configs database table with an empty object in the config database table cell when clicked.

To my knowledge, a GraphQL endpoint that allows the client to delete rows from the alert_configs database table does not exist; therefore, there is a question of which entity (i.e., client or server) should be responsible for cleaning up "stale" rows in this table (i.e., a row where the config database table cell contains an empty object) and what the permitted scope of deletions should be. Regarding the latter open question, if a deletion API is the proper direction, it would be beneficial if the request could accept a prefix and, perhaps, a flag indicating the intent to clean up "stale" rows corresponding to as well as under that prefix. But I minimally digress.

Tl;dr: The GraphQL API will not return the "inherited", global settings for a prefix that does not have a corresponding row in the alert_configs table, resulting in this UX quirk that I raised around a month ago. At the time, I was directed to implement the interactive proposal as closely as possible, therefore I assume the broader team is comfortable addressing this post-release as a fast-follow if we desire to take action.

(and i'm also seeing now that if i save that acmeCo/nested/ dialog without making any changes, both parent configs and subscriptions are saved as new rows with the acmeCo/nested/ prefix)

This was ultimately an open UX question that is tightly coupled to that highlighted above. Allowing rows to be inserted into the alert_configs and alert_subscriptions tables for a new prefix when global prefix settings nor subscriptions are defined, allows for the new prefix to be visible in the Organization Notifications table and the "inherited", global prefix settings (i.e., the effective alert config) to be viewed in Update Notification Methods dialog.

[kiahna-tucker]

Along the same lines as my comment about redundant alert configs being created, if i have a dataMovementStalled: 24h config defined at acmeCo/hello/world/ and I open the dialog to acmeCo/hello/ (the parent) i see the dataMovementStalled value set to 24h even though i only ever set that value on the child. And then if i click the save button it does create the config with that value at acmeCo/hello/, effectively promoting that config to the parent

I'm also seeing that if i make a change to the data movement stalled interval, and then click the "delete prefix alerts" button, the dialog closes and saves that config option - i could imagine this being surprising to a user. maybe we could remove the delete prefix alerts button entirely?

The quoted UI/UX callouts above are consequences of the proposed and approved design I was instructed to follow closely. I made similar callouts in the past, however the most likely scenario will be that UI/UX adjustments will be made as fast-follows, either as other teammates encounter the friction in production or as customers do.

There's some dead space in the alert type boxes between the text and the edge of the box that responds to hover but isn't clickable at all.

There are 16 pixels of padding on the right and left of MUI's ListItem component that are not clickable. This is not a blocking and can be addressed as a fast-follow.

Action Items:

• when i open the dialog and add a new recipient, the alert types respond to mouse over (with hover states) but not to mouse clicks. I have to interact with the email field first, and then the alert types are clickable.
• If i change the global settings at acmeCo/ without having added a recipient, clicking safe hangs on the loading indicator (i don't see any network requests being made)
• probably unlikely but the dialog crashes if the API sends down {"dataMovementStalled":{"enabled":true}} (no "condition" key defined)
• Implement the workaround for the alertConfigs GraphQL endpoint discussed in Slack.

[GregorShear]

a dataMovementStalled config with "stalledFor: "30days" will show up in the UI as "None".

Probably better to just fall back to the value sent by the server if it doesn't match one of the existing choices

[GregorShear]

hey by the way - did you do a claude review on this? i'm thinking a lot of this stuff would be caught by automated PR review...

[kiahna-tucker]

Along the same lines as my comment about redundant alert configs being created, if i have a dataMovementStalled: 24h config defined at acmeCo/hello/world/ and I open the dialog to acmeCo/hello/ (the parent) i see the dataMovementStalled value set to 24h even though i only ever set that value on the child. And then if i click the save button it does create the config with that value at acmeCo/hello/, effectively promoting that config to the parent.

As aforementioned, the Configure Notification Methods dialog only affects a single prefix. Changing the prefix will not reset the settings or subscriptions the user configured unless there is existing information in the database tables to override it. This UX is peculiar, was brought to the attention of necessary parties by me, and it was decided that the UX would be released as-is and re-evaluated later on.

email validation is pretty aggressive - could we wait until the user clicks away

Email validation is pretty aggressive as requested and agreed to in a UI/UX meeting when alert subscriptions was first introduced to the application. If you would like to see it relaxed now, that is fine but such a nit is not blocking.

also, the save button remains disabled until i blur the email input field

This is intentional and necessary for reasons explained in dailies in May.

i acknowledge this isn't represented in the mock, but we'll want a way to suppress alerts at a given prefix - e.g. acmeCo/ set some dataMovementStalled condition, but we want to set dataMovementStalled: { enabled: false }} at acmeCo/broken_prototype/

This is a fair point, however it is out-of-scope for this feature. Please review the interactive proposal to get a better understanding of the scope of this feature.

[kiahna-tucker]

No. I do not know what commit you are working off of, but it appears to be older than that of the current branch. Nevertheless, no, that is not what I am saying here. You and I are misinterpreting each other's words -- which is to be expected as we learn the other's communication styles and patterns. I believed you described behavior strictly within the dialog, therefore I described how changes to the prefix input affects the data in other dialog sections. This UX is something that does feel odd to me, so I wanted to make sure you were aware that it is, presently, expected.

As stated in Slack, it does appear that we have an ethos to PRs that differ. I have not done a code review of the entire PR after the changes made yesterday. I will explicitly re-request a review in GitHub (and message you directly, if that would be beneficial) when it is time for you to take a second look at this PR.