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

Add datastore infrastructure to get queuedNotifications #8975

Closed
14 tasks done
jimmymadon opened this issue Jul 8, 2024 · 5 comments
Closed
14 tasks done

Add datastore infrastructure to get queuedNotifications #8975

jimmymadon opened this issue Jul 8, 2024 · 5 comments
Labels
P2 Low priority QA: Eng Requires specialized QA by an engineer 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 focuses on computing and fetching the "queue" of notifications from the core/notifications datastore that should be rendered one at a time.


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

Acceptance criteria

  • Datastore infrastructure should be created within the core/notifications datastore which:
    • Fetches all notifications within state that have been registered using the registerNotification action.
    • Filters out these registered notifications for a given viewContext
    • Filters out any dismissed notifications using the isNotificationDismissed(id) selector
    • Filters the list further by running the checkRequirements() callback function that each notification would have defined at the time of registering the notification.
    • Sorts the remaining notifications by priority
  • The above list of notifications should be accessible using a getQueuedNotifications(viewContext) selector as per the design doc.

Implementation Brief

Use the PoC branch within the AC to speed up this issue. Copy-paste and then review the code where relevant.

  • Within assets/js/googlesitekit/notifications/datastore/notifications:
    • Add a getNotifications() selector which simply returns all the notifications currently stored within state.
    • Add a receiveQueuedNotifications(queuedNotifications) action to the list of actions (similar to receiveActiveIDs within the PoC branch).
    • Within the reducer, create a new case for RECEIVE_QUEUED_NOTIFICATIONS (similar to RECEIVE_ACTIVE_IDS in the PoC) and transfer the payload of queuedNotifications to state.
    • Add a getQueuedNotifications(viewContext) resolver (generator function) to the resolvers object. Within it:
      • Similar to getActiveNotifications() resolver in the PoC branch, fetch the list of registered notifications using the getNotifications() selector.
      • Iterate over the array of notification objects using a filter function which:
        • Returns false if the viewContext param does not exist in the list of viewContexts within each notification's data.
        • Returns false if the isDismissble data for a notification is true and the isNotificationDismissed(id) selector returns true (the id is available fron the notification's data).
      • Similar to the PoC branch, run the checkRequirements() callback function within the notification's data for all notifications synchronously and filter out notifications where the callback returns false.
      • Finally, sort the remaining notifications using priority defined in the notification's data. This final result will be an array of queuedNotifications.
      • yield the result using the actions.receiveQueuedNotifications(queuedNotifications) action defined above.
    • Add a getQueuedNotifications(viewContext) selector which returns undefined if the above resolver isn't resolved. Otherwise, it should return the queuedNotifications from state.

Test Coverage

  • Add tests for both the getQueuedNotifications() and getNotifications() selectors, as well as the new receiveQueuedNotifications() action.

QA Brief

  • Like all Banner Notification issues, this one has no user facing changes nor changes to existing code in production. So just a general smoke test of the plugin to check for any new errors or console errors would be sufficient.

QA Eng

  • Within assets/js/googlesitekit/notifications/register-defaults.js, register a bunch of new notifications that test the various points within the AC. A sample test snippet which registers three notifications is given below. Then call the getQueuedNotifications() selector (twice as it won't resolve the first call).
await googlesitekit.data.select('core/notifications').getQueuedNotifications('mainDashboard');
Code Snippet for widget registration
import {
	VIEW_CONTEXT_ENTITY_DASHBOARD,
	VIEW_CONTEXT_MAIN_DASHBOARD,
} from '../constants';
import { CORE_MODULES } from '../modules/datastore/constants';
import { NOTIFICATION_AREAS } from './datastore/constants';

/**
 * Registers notifications not specific to any one particular module.
 *
 * @since n.e.x.t
 *
 * @param {Object} notificationsAPI Notifications API.
 */
export function registerDefaults( notificationsAPI ) {
	// TODO: This file and the below code is pure scaffolding and for test QA purposes.
	// It will be modified in issue #8976 that registers the first refactored notification.
	notificationsAPI.registerNotification( 'gathering-data-notification', {
		Component() {
			return <h1>TODO: Use a valid notification component here.</h1>;
		},
		priority: 100,
		areaSlug: NOTIFICATION_AREAS.BANNERS_ABOVE_NAV,
		viewContexts: [ VIEW_CONTEXT_MAIN_DASHBOARD ],
		checkRequirements: ( { select } ) => {
			return select( CORE_MODULES ).isModuleConnected( 'search-console' );
		},
		isDismissible: true,
	} );

	notificationsAPI.registerNotification( 'zero-data-notification', {
		Component() {
			return <h1>TODO: Use a valid notification component here.</h1>;
		},
		priority: 50,
		areaSlug: NOTIFICATION_AREAS.BANNERS_BELOW_NAV,
		viewContexts: [ VIEW_CONTEXT_ENTITY_DASHBOARD ],
		checkRequirements: ( { select } ) => {
			return select( CORE_MODULES ).isModuleConnected( 'search-console' );
		},
		isDismissible: true,
	} );

	notificationsAPI.registerNotification( 'other-data-notification', {
		Component() {
			return <h1>TODO: Use a valid notification component here.</h1>;
		},
		priority: 10,
		areaSlug: NOTIFICATION_AREAS.BANNERS_ABOVE_NAV,
		viewContexts: [
			VIEW_CONTEXT_ENTITY_DASHBOARD,
			VIEW_CONTEXT_MAIN_DASHBOARD,
		],
		checkRequirements: ( { select } ) => {
			return select( CORE_MODULES ).isModuleConnected( 'search-console' );
		},
		isDismissible: true,
	} );
}

Changelog entry

  • Add datastore infrastructure to get queued notifications.
@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
@binnieshah binnieshah removed the Next Up Issues to prioritize for definition label Jul 11, 2024
@jimmymadon jimmymadon self-assigned this Jul 12, 2024
@jimmymadon jimmymadon added the Next Up Issues to prioritize for definition label Jul 12, 2024
@jimmymadon jimmymadon removed their assignment Jul 12, 2024
@binnieshah binnieshah removed the Next Up Issues to prioritize for definition label Jul 12, 2024
@eugene-manuilov eugene-manuilov self-assigned this Jul 12, 2024
@eugene-manuilov
Copy link
Collaborator

AC and IB ✔️

@eugene-manuilov
Copy link
Collaborator

@jimmymadon could you please add the estimate?

@jimmymadon jimmymadon removed their assignment Jul 15, 2024
@zutigrm zutigrm self-assigned this Jul 15, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Jul 15, 2024

Parking this one, while waiting for #8973 to get merged first

@eugene-manuilov eugene-manuilov removed their assignment Jul 26, 2024
@mohitwp mohitwp self-assigned this Jul 29, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Jul 31, 2024

QA Update ✅

  • Tested on dev environment.
  • Done smoke testing of plugin.
  • Verified site kit setup.
  • Verified all modules setup.
  • Tested banner notifications for all modules and other functionality.
  • Tested main and entity dashboard.

image

image

image

image

image

image

@mohitwp mohitwp removed their assignment Jul 31, 2024
@mohitwp mohitwp added the QA: Eng Requires specialized QA by an engineer label Jul 31, 2024
@wpdarren wpdarren assigned mohitwp and unassigned mohitwp Jul 31, 2024
@tofumatt tofumatt self-assigned this Aug 5, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Aug 5, 2024

QA: Eng ✅

I didn't see any console errors during smoke testing.

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

No branches or pull requests

8 participants