Improve publication onboarding state synchronization

[nfmohit]

Feature Description

Currently, the RRM publication onboarding state is synchronized in the client-side. However, with similar synchronization efforts in other modules, such as the PHP classes Synchronize_Property and Synchronize_AdsLinked, it has been proven that syncing data in the server-side is more reliable. The primary concern in the case of publication onboarding state synchronization is that there is a chance that an incorrect publication is referenced as the publication ID in the data store is not always the "saved" setting value.

Ideally, a scheduled event should be used to synchronize the publication onboarding state, similar to Synchronize_Property and Synchronize_AdsLinked.

As part of this improvement, the mechanism to display the publication approved overlay notification (added as part of #8843) will also need to be updated. The overlay notification currently appears as part of the syncPublicationOnboardingState action.

---

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

Acceptance criteria

• The periodic synchronization of the connected RRM publication's onboarding state should be moved from the client to the server side.
• It should be scheduled as a task that runs at most once per hour.
• The task should only be scheduled when the user is on the Site Kit dashboard or Settings page.
• From the user's perspective, there should be no apparent change in behaviour; the "Your Reader Revenue Manager publication is approved" notification should continue to be shown when the connected publication's onboarding state is determined to have changed to ONBOARDING_COMPLETE from some other non-empty state (see #8843 and #8796).

Implementation Brief

Note: PR #9304 can be used as a starter for this task.

• [x] In includes/Modules/Reader_Revenue_Manager/Settings.php, add a new setting publicationOnboardingStateChanged.
  • Default value of the setting should be false.
  • In get_default method, remove publicationOnboardingStateLastSyncedAtMs field.
• [x] In includes/Modules/Reader_Revenue_Manager/, create a class Synchronize_OnboardingState.
  • Class constructor should accept Reader_Revenue_Manager and User_Options instances. Store both instances in class properties.
  • Create a class constant CRON_SYNCHRONIZE_ONBOARDING_STATE to store the cron action name. Value for it can be googlesitekit_cron_synchronize_onboarding_state.
  • Add a method register which should add an action to the cron hook. The callback can be a closure which should call a protected method synchronize_publication_data.
  • Create a method synchronize_publication_data in class. Check if the user has permission to view authenticated dashboard Permissions::VIEW_AUTHENTICATED_DASHBOARD. If user has permission, call synchronize_onboarding_state method.
  • Create a method synchronize_onboarding_state. Check if the module is connected, if not connected return early.
    • If module is connected and publication ID present, call the publications API using $this->reader_revenue_manager->get_data( 'publications' ).
    • Check if the current publication onboarding state is different from what has been set in the API, if that is the case, set the new onboarding state using $this->reader_revenue_manager->get_settings()->set( $new_settings ); and set publicationOnboardingStateChanged to true.
  • Create a method maybe_schedule_synchronize_onboarding_state, check if module is connected and if cron is not scheduled already, then schedule a cron with action self::CRON_SYNCHRONIZE_ONBOARDING_STATE.
    • Timestamp should be time() + ( HOUR_IN_SECONDS ).
• [x] In includes/Modules/Reader_Revenue_Manager.php
  • Create instance of Synchronize_OnboardingState class and store it in $synchronize_onboarding_state.
  • Call the register method of Synchronize_OnboardingState.
  • In register method, use load-toplevel_page_googlesitekit-dashboard and load-toplevel_page_googlesitekit-settings actions to hook to Synchronize_OnboardingState::maybe_schedule_synchronize_onboarding_state method. This will ensure that task is only scheduled when user is on dashboard or settings page.
  • In get_debug_fields method, remove reader_revenue_manager_publication_onboarding_state_last_synced_at field completely as it is no longer needed.
• [x] In assets/js/modules/reader-revenue-manager/components/dashboard/PublicationApprovedOverlayNotification.js.
  • Check if onboarding state has changed using getSettings selector.
  • Assign this value to initialPublicationOnboardingStateChanged using useRef, this is necessary as we will need to change publicationOnboardingStateChanged to false in useEffect as soon as component renders, but want to keep component visible.
  • If initialPublicationOnboardingStateChanged is true and the onboarding state is ONBOARDING_COMPLETE, display the notice.
  • In useEffect, set publicationOnboardingStateChanged to false using setPublicationOnboardingStateChanged method and save the setting using saveSettings action. This effect should be run only once when component is mounted.
• [x] Cleaup the client side code.
  • Remove DashboardMainEffectComponent component from the code base which calls.
  • Remove maybeSyncPublicationOnboardingState action from assets/js/modules/reader-revenue-manager/datastore/publications.js.
• [x] In assets/js/modules/reader-revenue-manager/datastore/base.js and assets/js/modules/reader-revenue-manager/datastore/publications.js
  • Add publicationOnboardingStateChanged to settingSlugs in assets/js/modules/reader-revenue-manager/datastore/base.js.
  • Remove references for publicationOnboardingStateLastSyncedAtMs field.

Test coverage

1. Clean up js tests for the removed actions.
2. Add unit tests in php for Synchronize_OnboardingState class.
3. Fix test for PublicationApprovedOverlayNotification.test.js
4. Fix any other failing tests due to removal of publicationOnboardingStateLastSyncedAtMs field.

QA Brief

Note for the QA team by @nfmohit: Please verify that this issue was fixed as well.

1. Install Advanced Cron Manager plugin and activate.

2. Make sure you have at least one publication setup in RRM publication center with the status Complete.

3. In tester plugin, RRM > Force Reader Revenue Manager publication onboarding state, set it to ONBOARDING_ACTION_REQUIRED.

4. Enable RRM feature and module. Make sure publications are setup for the site and they are available in settings screen.

5. Select the publication with completestate in publication center and complete the setup. This will setup the publication state as ONBOARDING_ACTION_REQUIRED in database.

6. Visit the Site Kit dashboard.

7. Go to Tools > Cron Manager and make sure that googlesitekit_cron_synchronize_onboarding_state event is scheduled in an hour (approx). Click Execute now link for the event.

8. Once the job is executed, go to Site Kit dashboard, you should see the publication approved overlay notification.

Changelog entry

[nfmohit]

AC ✅

[benbowler]

Note for IB reviewer:

#6992 adds code to remove scheduled events on plugin reset/deactivation/uninstall. This IB should be updated to make sure that this new cron is also removed in these cases.

CC: @ankitrox @nfmohit

[nfmohit]

Hi @ankitrox . Thank you for drafting the IB and the PoC here, excellent work! We're taking a step back here to potentially add an additional AC.

• Any other on-demand synchronization of the connected RRM publication's onboarding state should be moved from the client to the server-side approach, e.g. #9262 is in progress to introduce an on-demand synchronization of the onboarding state.

@techanvil In response to the relationship conflict between this issue (#9149) and #9262, I've added the above point to the AC. Does this sound OK to you? Thanks!

[techanvil]

Thanks @nfmohit, however, I don't think we should actually make that a requirement here.

The favoured approach for the server side sync is to use the WP Cron scheduling system, as we do for similar cases, and it's hard to imagine a nice way to expose this for use in the on-demand, client-side sync that we are going to be doing for the notice in #9262.

We'd almost certainly need to add a new endpoint, while the discussion on #9262 has identified that we'll likely be extracting a simple action from syncPublicationOnboardingState() which just makes use of the existing RRM settings endpoint.

I think the best approach is to stick with that for the client-side approach, we'd probably just need to remove publicationOnboardingStateLastSyncedAtMs from the settings update if that's no longer needed for the sync.

https://github.com/google/site-kit-wp/blob/36b8bcd74d85fd8fa3f560c6b8eaf6ca5031c84a/assets/js/modules/reader-revenue-manager/datastore/publications.js#L132

[nfmohit]

Thanks @nfmohit, however, I don't think we should actually make that a requirement here.

The favoured approach for the server side sync is to use the WP Cron scheduling system, as we do for similar cases, and it's hard to imagine a nice way to expose this for use in the on-demand, client-side sync that we are going to be doing for the notice in #9262.

We'd almost certainly need to add a new endpoint, while the discussion on #9262 has identified that we'll likely be extracting a simple action from syncPublicationOnboardingState() which just makes use of the existing RRM settings endpoint.

I think the best approach is to stick with that for the client-side approach, we'd probably just need to remove publicationOnboardingStateLastSyncedAtMs from the settings updated if that's no longer needed for the sync.

https://github.com/google/site-kit-wp/blob/36b8bcd74d85fd8fa3f560c6b8eaf6ca5031c84a/assets/js/modules/reader-revenue-manager/datastore/publications.js#L132

That sounds good to me, I've removed the new AC, thank you!

I've also removed #9262 as a blocker for this issue. This should be able to proceed independently while not removing the syncPublicationOnboardingState() action. Back to you @ankitrox.

[ankitrox]

Thank you @nfmohit and @techanvil .

I have slightly updated the IB so as not to remove syncPublicationOnboardingState action and to remove publicationOnboardingStateLastSyncedAtMs field.

I think rest of it doesn't get affected here, so sending it for review.

Thanks

[nfmohit]

The IB looks solid to me. IMO, we should bump up the estimate by one notch considering the wide surface area here and its test coverage.

I'll leave the final go-ahead call to @techanvil.

[techanvil]

Thanks @nfmohit. Bumping the estimate up sounds reasonable to me given the scope of the implementation.

@ankitrox, a small detail, to meet the AC where the task should only be scheduled when the user is on the Site Kit dashboard or Settings page, it looks like we should register with the load-toplevel_page_googlesitekit-dashboard and load-toplevel_page_googlesitekit-settings hooks rather than admin_init.

[nfmohit]

Note for the QA team: @kelvinballoo discovered an issue in the module settings edit screen and reported internally here.

• At the 'Connect service' page, normally if we edit the settings for one module, e.g. we change the GA property, the 'Save' button will change to 'Confirm changes' and if I revert back to the original property, the 'Confirm changes' button changes to 'Save'.
• The same is not applied for the RRM module. Once we change the Publication, it changes to 'Confirm changes', which is fine but if we change to the original publication, the button remains as 'Confirm changes'. It doesn't change to 'Save'.
• Video for reference here: https://drive.google.com/file/d/1CSbYlqulpohkmbZhd4RLNg8Qjn3qp90B/view?usp=sharing

According to my investigation, this bug should be fixed as part of this issue. This is happening because when the user changes their publication selection, we reset the publicationOnboardingStateLastSyncedAtMs module setting to the current time, which is why the button copy is never reverted to "Save". This issue is supposed to remove this module setting entirely, so this should be fixed as well.

CC: @ankitrox @wpdarren @mohitwp @techanvil

[ankitrox]

Thank you @techanvil . I've updated the IB to schedule the task only on load-toplevel_page_googlesitekit-dashboard and load-toplevel_page_googlesitekit-settings actions.

[techanvil]

Thanks @ankitrox. I've had another read of the IB and a couple of things occur to me. As it stands:

• There will be a small change to the current behaviour. Currently, the PublicationApprovedOverlayNotification notification is only shown on the initial page load where the state is determined to have changed. Having followed the IB, the notification will be shown on every page load after the state has changed until a user dismisses it (or another resync occurs). This is not necessarily a show-stopper, but it's worth noting.
• However, it would also be possible for the notification never to be shown to a user, although it should be, in the case where they land on the dashboard or Settings page to schedule a resync, then visit the site an hour later causing the resync to occur, setting publicationOnboardingStateChanged to true. If they don't visit the dashboard, but land on the Settings page an hour later, causing another resync to be scheduled, this will result in publicationOnboardingStateChanged reverting to false. On their next visit to the dashboard, they will effectively have missed seeing the notification.

It seems to me we can fix both of these with the following approach:

• In the scheduled event handler, only ever set publicationOnboardingStateChanged to true.
• When visiting the dashboard, if publicationOnboardingStateChanged is true , ensure we show the notification when the onboarding state is ONBOARDING_COMPLETE, and save publicationOnboardingStateChanged as false.

How does that sound to you? Cc @nfmohit

[ankitrox]

Thank you @techanvil .

I've updated the IB so that we never set publicationOnboardingStateChanged to false in cron. We will set publicationOnboardingStateChanged to false in useEffect hook of the component.

Also, we can make use of useRef to ensure that component keeps displaying even if we changed publicationOnboardingStateChanged to false.

[techanvil]

Thanks @ankitrox, that's looking good. My only minor quibble is regarding the initialVisibility name, as this doesn't in isolation reflect the component's visibility.

I'd suggest we either rename it to initialPublicationOnboardingStateChanged, or, keep the name initialVisibility but assign it the initial value of publicationOnboardingStateChanged && onboardingState === ONBOARDING_COMPLETE.

Alternatively, we don't have to be quite so prescriptive in the IB and can simply say "use a React ref to track the initial state of the publicationOnboardingStateChanged setting and refer to this when determining whether the notification should be displayed" or something along those lines...

[ankitrox]

Thanks @techanvil . I simply changed initialVisibility to initialPublicationOnboardingStateChanged as it was simpler change to do in IB.

We can of course change it to something else more appropriate in implementation. :)

Over to you for another review.

[techanvil]

Thanks @ankitrox, that's great. The IB LGTM! :white_check_mark:

[mohitwp]

QA Update ⚠

@ankitrox

• Tested on dev environment.
• I followed the QAB but publication approved overlay notification. is not appearing on dashboard after executing googlesitekit_cron_synchronize_onboarding_state event. Note : Your Reader Revenue Manager publication is approved" notification successfully appeared.

https://github.com/user-attachments/assets/4223c9e7-3459-4bf9-be6b-eb4eb92e3afe

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified that on Site Kit dashboard, publication approved overlay notification. appears successfully.
• Verified that "Your Reader Revenue Manager publication is approved" notification continue to be shown when the connected publication's onboarding state is determined to have changed to ONBOARDING_COMPLETE.

https://github.com/user-attachments/assets/15824210-d663-4837-9829-9c902d9cde1e