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

Improve RRM publication synchronization efficiency

Open nfmohit opened this issue 9 months ago • 4 comments

Feature Description

Information related to the connected publication in Reader Revenue Manager, such as the onboarding state, available product IDs, and payment option, is stored as module settings. This stored information is synchronized only once every hour using a scheduled event. However, there is an evident way of improving this efficiency, as all it takes for the synchronization is a fetch of the publications list (i.e. GET:publications endpoint).

A fetch to the GET:publications endpoint should also update this stored information so that we don't need to wait for the next scheduled event unnecessarily.


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

Acceptance criteria

  • Whenever Site Kit fetches the list of available Reader Revenue Manager publications for the connected Google account and the current site:
    • Information related to the connected publication, such as the onboarding state, available product IDs, and payment option, should be updated to reflect the latest values.
    • The immediate next scheduled event that is supposed to synchronize the publication information should be cleared, and a new event should be scheduled one hour later so that the synchronization isn't initiated again unnecessarily.

Implementation Brief

This has mostly been implemented in this PoC, which can be refined, perfected, have tests added/updated, and then merged. The following is a high-level description of the changes proposed as part of the PoC:

Synchronize publication information as part of GET:publications fetch

  1. Create a copy of Modules\Reader_Revenue_Manager\Synchronize_Publication::synchronize_publication_data in Modules\Reader_Revenue_Manager, removing anything that has to do with user permissions and switching.
  2. In Modules\Reader_Revenue_Manager, make synchronize_publication_data accept the publications response as a parameter and call this method as part of the parse_data_response method for the GET:publications case, before returning the response.
  3. In the original Modules\Reader_Revenue_Manager\Synchronize_Publication::synchronize_publication_data, the entire synchronization logic is no longer needed and should be removed as simply fetching GET:publications should synchronize the publication information.

Update the datastore state

In assets/js/modules/reader-revenue-manager/datastore/publications.js, update the reducer callback for fetchGetPublicationsStore so that settings and savedSettings in state is updated to store the latest values for productIDs and paymentOption.

Test Coverage

  • Add test coverage for changes in GET:publications.
  • Add test coverage for changes in fetchGetPublicationsStore.

QA Brief

Changelog entry

nfmohit avatar Mar 24 '25 23:03 nfmohit

Noting that the concerns raised in https://github.com/google/site-kit-wp/issues/10482#issuecomment-2748088675 are applicable here, and should be taken into consideration when reviewing the issue.

techanvil avatar Mar 25 '25 10:03 techanvil

Hi @nfmohit, with regard to the AC, can you think of any scenarios for the RRM settings edit screen (or indeed anywhere else, but I think this is the only place where the sync would actually take place) where the delayed update to the settings from the get-and-sync publications would introduce any issue for the UI? Taking a look myself, I think it would be OK, and you've probably already had a look but I just want to make sure we've considered the angles raised in the comment linked above, https://github.com/google/site-kit-wp/issues/10482#issuecomment-2748088675

When it comes to the IB, I think we should rename the getPublications() selector to something like getAndMaybeSyncPublications() to make it clear what the code is doing. This would help developers understand at a glance what's going on, and serve as a warning to treat its use appropriately. We don't want to run into bugs later where the selector is used without realising it overrides the settings. WDYT?

techanvil avatar Mar 26 '25 10:03 techanvil

Hi @nfmohit, with regard to the AC, can you think of any scenarios for the RRM settings edit screen (or indeed anywhere else, but I think this is the only place where the sync would actually take place) where the delayed update to the settings from the get-and-sync publications would introduce any issue for the UI? Taking a look myself, I think it would be OK, and you've probably already had a look but I just want to make sure we've considered the angles raised in the comment linked above, #10482 (comment)

Hi, @techanvil! Thank you for raising this. I did think about this when drafting this issue, and I believe there are no other places where this would have an effect.

I'm assigning this to you for an AC review. In the meantime, I'll continue working on the IB. If the AC looks good to you, please assign the issue to me in IB. Thank you!

nfmohit avatar Apr 25 '25 08:04 nfmohit

Thanks @nfmohit! In that case, the AC LGTM. Back to you to wrap up the IB.

AC ✅

techanvil avatar Apr 28 '25 09:04 techanvil