Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor GatheringDataNotification to use the new Notifications datastore #8976

Closed
13 tasks done
jimmymadon opened this issue Jul 8, 2024 · 5 comments
Closed
13 tasks done
Labels
P2 Low priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@jimmymadon
Copy link
Collaborator

jimmymadon commented Jul 8, 2024

Feature Description

This issue is the first of many issues that will "use" the new datastore infrastructure to queue notifications while also creating a new Notification component that will be reused by other notifications.


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

Acceptance criteria

  • The GatheringDataNotification should be refactored and rendered (queued) using the new core/notifications datastore. This means that the component should not be called directly but via the generic getQueuedNotifications selector.
  • The logic to render the GatheringDataNotification should be extracted from the ZeroDataStateNotifications component and moved to the checkRequirements callback when registering this notification.
  • The refactored component should be its own standalone component without any business logic that prevents it from rendering. This component should be passed through when registering the new notification.

Implementation Brief

  • From the assets/js/components/notifications/ZeroDataStateNotifications.js:
    • Move all logic that computes both, searchConsoleGatheringData and analyticsGatheringData to the checkRequirements(registry) callback function when registering the gathering-data-notification notification within assets/js/googlesitekit/notifications/register-defaults.js. The callback should return true only if either searchConsoleGatheringData or analyticsGatheringData are true.
    • Move the logic to compute the gatheringDataTitle and the gatheringDataWaitTimeInHours to the GatheringDataNotification component. This will include the code to compute searchConsoleGatheringData or analyticsGatheringData. This means that the GatheringDataNotification will not have any props anymore.
    • Move all logic that computes both, searchConsoleZeroData and analyticsZeroData to the assets/js/components/notifications/ZeroDataStateNotifications/ZeroDataNotification.js component.
    • Remove the assets/js/components/notifications/ZeroDataStateNotifications.js eventually.
  • Create assets/js/components/notifications/Notifications.js similar to the PoC.
    • This component should have two props: viewContext and areaSlug.
    • Use the getQueuedNotifications(viewContext) selector to fetch the queue of notifications. Check if the first notification in the queue has the same areaSlug as the prop passed, if it does, render this notification's Component, otherwise do not render anything. Do not
  • In assets/js/googlesitekit/notifications/register-defaults.js:
    • Modify the registration of the dummy notification by passing the actual GatheringDataNotification component to the Component setting of the gathering-data-notification notification. Currently, set isDismissible to false as all the dismissal logic is still going to be handled by the react component itself.
  • In assets/js/components/notifications/BannerNotifications.js:
    • Replace the rendering of ZeroDataStateNotifications with ZeroDataStateNotification.
    • Add the new Notifications component passing the areaSlug as NOTIFICATION_AREAS.BANNERS_ABOVE_NAV and the viewContext to be VIEW_CONTEXT_MAIN_DASHBOARD if the dashboard is normal or VIEW_CONTEXT_MAIN_DASHBOARD_VIEW_ONLY if the dashboard is view only.

Test Coverage

  • No new tests required. Existing storybook stories should be updated.

QA Brief

Ideally - WAIT for #8977 to be merged and then QA both the issues together as they are exactly the same pattern in code.

  • Thoroughly smoke test the GatheringData and ZeroData notifications:
    • Test when only Search Console is gathering data (and analytics is disconnected).
    • Test when both, Search Console and Analytics is Gathering Data.
    • Test when Search Console has finished gathering data but Analytics is still Gathering Data.
    • Test when either and both, SC and Analytics are in Zero data state.
    • Test all of the above for the Entity Dashboard as well.
  • Test when any other notification, say any other SetupSuccessBannerNotification is active - they should take precedence over the Gathering and Zero Data notifications.
  • Smoke test the remaining notifications to ensure they still work normally.
  • Test the new Storybook stories in Components -> Notifications.

Changelog entry

  • Update notifications to use new notifications infrastructure.
@jimmymadon jimmymadon added Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 labels Jul 8, 2024
@binnieshah binnieshah added the Next Up Issues to prioritize for definition label Jul 8, 2024
@eclarke1 eclarke1 added the P2 Low priority label Jul 8, 2024
@jimmymadon jimmymadon self-assigned this Jul 15, 2024
@binnieshah binnieshah removed the Next Up Issues to prioritize for definition label Jul 18, 2024
@tofumatt tofumatt changed the title Refactor the GatheringDataNotification to use the new Notifications datastore Refactor GatheringDataNotification to use the new Notifications datastore Jul 23, 2024
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@eugene-manuilov
Copy link
Collaborator

IB ✔️

@mohitwp
Copy link
Collaborator

mohitwp commented Aug 16, 2024

QA Update ❌

  • Tested on dev environment.
  • Verified zero data notifications on main, entity and view only dashboard.
  • Smoke tested plugin notification and verified that other notifiations - take precedence over the Gathering and Zero Data notifications.
  • Tested storybook component >notifications stories.

@jimmymadon
Issue > On View only dashboard zero and gathering data notifications are not showing. On latest environment zero and gathering data state notifications are showing.

Recording.1304.mp4

@jimmymadon
Copy link
Collaborator Author

@eugene-manuilov Assigning you here since you did the original CR/MR for this issue. So it will make sense to do the follow up PR as well. Thanks!

@mohitwp
Copy link
Collaborator

mohitwp commented Aug 21, 2024

QA Update ✅

image

image

image

image

Recording.1324.mp4
Recording.1325.mp4

@mohitwp mohitwp removed their assignment Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants