site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Refactor `AdSenseAlerts` to use the new Notifications datastore

Open zutigrm opened this issue 1 year ago • 3 comments

Feature Description

This issue should refactor the AdSenseAlerts so that it uses the new datastore infrastructure to register and queue the notification. It should also incorporate a new "layout".


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

Acceptance criteria

  • The AdSenseAlerts component should be refactored so that it is registered and rendered (queued) using the new core/notifications datastore.
  • This notification component should not be called directly (i.e. in BannerNotifications) but only via the generic getQueuedNotifications selector.
  • The refactored component should not contain any business logic that hides it / prevents it from rendering. This logic should be contained in a callback function defined during registration.
  • The component should not use the bloated BannerNotification component. Instead, it should be rendered using the new Notification component wrapper and a simpler "layout" component that solely encapsulates its structure and design.

Implementation Brief

  • [ ]

Test Coverage

QA Brief

Changelog entry

zutigrm avatar Sep 04 '24 12:09 zutigrm

AC ✔️

eugene-manuilov avatar Sep 05 '24 19:09 eugene-manuilov

I am un-assigning myself since I have been caught up with higher priority epic issues, and will be on the PTO for few days. This notification will take a bit of a thinking it over, as it is potentially a group of notifications that are fetched from the backend, and current notifications API might need some tweaks to support this. The idea I had when initially had time to check this one, is that regular approach would potentially work, as we would register the notification in the API, and then checkRequirements and notification component would handle the fetching and iteration over data. Something to consider was that isDismissable is a fixed property included when notification is registered in the API, if the notifications coming from the backend are different in that regard, it won't render correctly, since isDismissable would be applied to all with the same value either true or false

zutigrm avatar Oct 03 '24 14:10 zutigrm

I'm marking this blocked by #9294 as there is an ongoing discussion there regarding the module level createNotificationsStore. This in an example of notifications that use this existing infrastructure so we need a unified spec/decision on how we want to approach this existing notifications store to bring it into the new store.

benbowler avatar Oct 24 '24 10:10 benbowler

Note around the priority of these banners, AdSense banners are the only banners which should be higher priority than ModuleRecoveryAlert which I have set in #10398, so in this ticket we should set it to higher priority (lower number) and other refactored BannerNotifications should be lower priority (higher number).

benbowler avatar Mar 11 '25 10:03 benbowler

Having discussed this and #9294 with @aaemnnosttv, we decided to hold off on these issues to prevent "nested" queues of notifications. This is because these notification components call a getNotifications selector which fetches one or more notifications and maps through them to render them. We might be better off creating these as "on-demand" notifications and so, we will wait till #9453 is completed first before we amend the AC/IB here.

jimmymadon avatar Mar 12 '25 14:03 jimmymadon

IB ✔

eugene-manuilov avatar Jun 19 '25 19:06 eugene-manuilov

QA Update ⚠️

Tested this and it's looking good overall. Just a question:

ITEM 1: The QAB states: "You should observe a sever notification with buttons that dismiss the notification when clicked"

We actually get 2 buttons:

  • Upgrade now!
  • Dismiss this message

Clicking 'Dismiss this message' will dismiss it accordingly with the relevant GA tracking. When clicking on 'Upgrade now!', this would not dismiss it but will send us to the Google adsense page. I assume that's expected. Can you confirm? That said, it will open it in the same tab. Is that expected or it should be in a new tab?

https://github.com/user-attachments/assets/f4a64d7e-52bb-4e33-b14d-714b270c1249

Clicking on 'Upgrade'

________________________________________________________________________________

Other than that, the gist of the testing is fine:

  • When Adsense is connected, the server notification appears and it's able to be dismissed. GA event being fired accordingly.

https://github.com/user-attachments/assets/aa878063-bae0-46d1-a805-f98883d7128f

  • When Adsense is not connected or if it was not ever installed, no server notification would appear.

    https://github.com/user-attachments/assets/548bf5e1-b4f6-4c7c-987a-78515847e59d

kelvinballoo avatar Jul 17 '25 10:07 kelvinballoo

When clicking on 'Upgrade now!', this would not dismiss it but will send us to the Google AdSense page. I assume that's expected. Can you confirm?

Yes, that's expected behaviour with the test notification 👍🏻

That said, it will open it in the same tab. Is that expected or it should be in a new tab?

That's correct, it looks like these CTA handlers don't currently support target properties, I mistakenly added it as part of the data but it goes unused.

That's fine though, so this can be considered all good 🙂

tofumatt avatar Jul 17 '25 11:07 tofumatt

QA Update: ❌

@tofumatt @jimmymadon I am seeing this regression on my live site, where the AdSense alert content is not appearing in the banner. You can see the issue in the screenshot below.

Image

wpdarren avatar Jul 28 '25 08:07 wpdarren

Hi @wpdarren, the follow-up PR from @tofumatt has been merged. It’s now ready for another round of QA. Thanks!

hussain-t avatar Jul 28 '25 11:07 hussain-t

QA Update: ✅

Verified:

  • When AdSense is enabled the alert notifications appeared on the Site Kit main dashboard.
Image

Note: there is an odd UX experience which is inconsitent with other banners, so I will flag this with Jimmy separately, but for this bug, we have fixed it and the alert now appears.

wpdarren avatar Jul 28 '25 11:07 wpdarren