Refactor Audience Segmentation / FPM Setup Success Subtle Notifications to use a new "on demand" notification

[jimmymadon]

Feature Description

In #9281, to render the Audience Segmentation setup success notification, which needs to be rendered without a new page load or without the changes to query args, we simply invalidate the resolution of the getQueuedNotifications resolver to re-evaluate the full queue. Instead, it this notification would be ideal to add the concept of an 'on-demand' notification which is "added" to the queue without a page reload or needing re-evaluation of the entire queue.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The Audience Segmentation and First Party Mode Setup Success Subtle Notifications should be refactored so that they can be rendered without re-evaluating the entire queue of notifications.
• This should be implemented in a reusable way - so that other notifications that should be rendered without a page reload / URL change can be implemented easily.

Implementation Brief

• In assets/js/googlesitekit/notifications/datastore/notifications.js:
  • Add a new action, insertNotification(notification), where notification is a full notification object as per our new API.
  • resolveSelect the getQueuedNotifications selector for the viewContext and the groupID of the incoming notification to ensure this resolver does not overwrite the queue after the incoming notification has been inserted.
  • Insert the entire notification object, into the queuedNotifications state for the notification's given groupID, similar to the queueNotification(notification) action.
  • Ensure that it isn't possible to add the same notification more than once, by matching the id of the notification to be inserted with existing notifications in the queuedNotifications state for that groupID.
  • When inserting, obey the notification's priority. Do this by looping through the array of queuedNotifications and finding the priority of existing notifications with the incoming notification and inserting the notification before the element with a higher priority number.
• In assets/js/modules/analytics-4/index.js and assets/js/googlesitekit/notifications/register-defaults.js respectively:
  • Remove the registration of the AUDIENCE_SEGMENTATION_SETUP_SUCCESS_NOTIFICATION and setup-success-notification-fpm notifications.
• In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js and assets/js/components/notifications/FirstPartyModeSetupBanner.js:
  • Define a setupSuccessNotification with the same options of the notifications that were removed from registration above. There is no need to define checkRequirements anymore as these notifications will not be "queued" in the normal way on each page reload.
  • Call the new insertNotification(notification) action instead of the invalidateResolution( 'getQueuedNotifications' ) call. For FPM, remove custom logic that uses core/ui setValue / getValue to show the notification.

Test Coverage

• Add JS tests for the new insertNotification action.

QA Brief

Changelog entry

[techanvil]

AC ✅

[tofumatt]

The PoC PR creates a new simple datastore action that takes in a notification ID and puts it at the front of the current state's queuedNotifications. It then uses

It looks like this IB was cut off/is missing text, can you expand on this part? The bullet points below don't really flow from that sentence 🤔

---

Also, instead of the checkRequirements: () => false approach, why not add a new argument to the notifications API that does this. Something like automaticRender: false or something? I think overloading the requirements check (where you'd expect logic for it returning different values based on plugin state) is a bit awkward and misusing the purpose of that function 😄

[jimmymadon]

@tofumatt Your point to not overload checkRequirements was very valid. So I realised that we don't need to register these notifications with an additional setting to just "ignore" queuing this notification! Might as well insert them in the queue only when needed and leave them out of the usual registration and queueing process.

@aaemnnosttv Will be good to have your review here too as we discussed this in our last meeting.

[aaemnnosttv]

The IB mostly sounds good to me here, just a few points to add:

• We should ensure that it isn't possible to add the same notification more than once. This could be done my filtering out any existing incoming notification by matching ID before adding
• We should probably also ensure that queued notifications are resolved before the new notification is inserted otherwise it may be cleared when the aforementioned resolver runs
• The sort should only affect where the notification is inserted – it shouldn't change the order of any other notifications

[jimmymadon]

@aaemnnosttv Thank you for all the helpful suggestions - I have updated the IB.

[aaemnnosttv]

• Add a new action, insertNotification(notification), where notification is a full notification object as per our new API.
• resolveSelect the getQueuedNotifications selector for the viewContext and the groupID of the incoming notification to ensure this resolver does not overwrite the queue after the incoming notification has been inserted.

viewContext could have multiple values and the queue also isn't specific to this value (only the group) so I think we'd need to accept this as a parameter to the action, unless we required that the given notification must specify a single value for its viewContext. This would be odd though since we're triggering it on demand. So either we'd need to:

• have some kind of implicit rule that queued notifications should be resolved first
• require the given notification to have a single viewContext rather than string[]
• require the viewContext is provided as an arg to the action
• store/sync the viewContext in state to be retrieved internally

I'm not a fan of any of these options really, and invoking getQueuedNotifications with a viewContext other than the current could also cause problems in rewriting the queue since the resolver resets it.

Another issue with this approach is that an on-demand notification would be the slowest to appear if we waited for all other notifications to queue before it could be inserted, which undermines the potential benefit of respecting the given notification's priority as we'd want it to be considered at the time the queue is populated.

Let's think about this some more as I don't see an obvious solution to the above.