google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Refactor the UnsatisfiedScopesAlert to use the new Notifications datastore and layout components #8978

Closed jimmymadon closed 2 months ago

jimmymadon commented 4 months ago

Feature Description

This issue is the first issue to refactor an "Error" notification in Site Kit. It should refactor the UnsatisfiedScopesAlert so that it uses the new datastore infrastructure to register and queue the notification. It should also use newer lighter "layout" and "common" components that replace the bloated BannerNotification component as per the pattern introduced in #9071.


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

tofumatt commented 3 months ago

Moving this to the backlog until #8976, which is marked as blocking this issue, is completed. That way we can mirror the approach used in #8976 for other migrations/refactoring.

eugene-manuilov commented 2 months ago

AC ✔️

eugene-manuilov commented 2 months ago

IB ✔️

jimmymadon commented 2 months ago

@zutigrm

For priority, I would suggest using 400 as base number.

Since Errors are higher priority than the other notifications, I would suggest we use 150. GatheringData and ZeroData have priorities 300 and 310. The other banners (about 9 other Banners will come above them taking us to priority 210 approx (each banner increments by 10 - so 290, 280, 270, etc...). There aren't many Errors or notifications above the UnsatisfiedScopesAlert - so we will have many empty slots above and below this notification once all of them are registered.

As mentioned in the stand up, I will probably pick this up this week itself so I can ensure the order is correct. I will also add these priority values into our spreadsheet when completing it.

c.c. @eugene-manuilov

zutigrm commented 2 months ago

@jimmymadon Thanks, makes sense

jimmymadon commented 2 months ago

Since these issues combine the registration and design, I will bump up the estimate to 15 here in line with our points experiment as this would unlikely be a 7 in "normal" scenarios. As seen in other issues, these issues do require proper CR and QA + any time for fixes in case the issue fails QA.

mohitwp commented 2 months ago

QA Update ❌

@jimmymadon Please find my observations below

![image](https://github.com/user-attachments/assets/80ecc760-b11c-4d21-a7b4-fecf9f5f414b) ![image](https://github.com/user-attachments/assets/bb4c996e-7138-4d8f-bf3e-8a74db2181dc) ![image](https://github.com/user-attachments/assets/69aa1cd6-b2d5-446e-ac9d-381af2ff8725) ![image](https://github.com/user-attachments/assets/f7c5fbac-f0b3-431f-8450-4f224bd986e4)
https://github.com/user-attachments/assets/868d0c92-6f2e-4703-a35b-b2c1f066ddc1

Issue 3 : Alignment of unsatisfied scope alert is not correct on dev environment.

![image](https://github.com/user-attachments/assets/69e5fb72-b281-43f5-888a-b7ac256b4247) ![image](https://github.com/user-attachments/assets/a26f336d-6c3e-442d-a3ec-c033ea478392) ![image](https://github.com/user-attachments/assets/d9d53439-6802-4d1a-aee2-9080b202f6a6) ![image](https://github.com/user-attachments/assets/ac6bd7f3-f55b-44f8-96d1-49a7b498a204)
jimmymadon commented 2 months ago

QA Update ❌

  • Tested on dev environment.

@jimmymadon Please find my observations below

  • Issue 1: Zero and data-gathering notifications are appearing below the success notification and other notifications, such as the 'Enable Enhanced Measurement' banner notice.

This should now be fixed.

  • Issue 2: Zero data notifications appear only once and do not reappear after reloading the page or opening the dashboard in a new tab.

For me, the ZeroDataNotification does appear on page reload. However, it does not appear when we open the dashboard in a new tab. I will add this to the ACs of #9227 as the cause of the inconsistent behaviour is the same here.

Issue 3 : Alignment of unsatisfied scope alert is not correct on dev environment.

This should now be fixed as well.

mohitwp commented 2 months ago

QA Update ✅

@jimmymadon Regarding issue number 2, I’ve observed that the "zero data" notification appears upon reload if both Analytics and SC are connected. However, if only SC is connected and is in a zero data state, the notification appears only once. It does not reappear upon reload or if we log in from another browser or open the dashboard in a new tab. I will add this scenario under #9227. Same issue exist on latest environment also.

cc @aaemnnosttv @wpdarren

https://github.com/user-attachments/assets/64222246-a799-44e6-8d98-943f5003f4e8

PASS CASES

**Main** ![image](https://github.com/user-attachments/assets/0e27438b-51a9-4ba4-8a34-c5162f08e601) **Latest** ![image](https://github.com/user-attachments/assets/5be50082-e85d-4f70-b8cf-ecab4b852f95) https://github.com/user-attachments/assets/f9689fa3-53eb-4d28-bfe7-c542ecec37cf https://github.com/user-attachments/assets/7866c4c7-4b16-4461-985a-8bf42b7940bb