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

Implement RRM `getPublications()` selector

Open nfmohit opened this issue 1 year ago • 4 comments

Feature Description

The getPublications() selector should be implemented for the Reader Revenue Manager module that should interact with the GET:publications REST endpoint and return a list of publications.


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

Acceptance criteria

  • A getPublications() selector should be added to the Reader Revenue Manager module data store.
  • This selector should fetch the GET:publications REST endpoint (being implemented in #8791) to return the list of available publications for the site.

Implementation Brief

  • [x] Create assets/js/module/reader-revenue-manager/datastore/publications.js. This would be a data store to fetch the publications.
    • [x] Add this store in Data.combineStores in assets/js/module/reader-revenue-manager/index.js added in #8786
    • [x] Use createFetchStore to create the publication store (getPublicationStore). This should use GET:publications endpoint (being implemented in #8791) to return the list of available publications for the site.
    • [x] baseName for the store getPublications.
    • [x] Base initial state (baseInitialState) should have the publications property which would be undefined initially and will be set to the list of publications. It should be array of publication objects.
    • [x] In reducerCallback inside createFetchStore, set the publications property in state.
    • [x] Create a baseResolvers object.
      • [x] Add *getPublications generator inside it. It should call getPublications selector. If it is undefined, it will yield the result by calling getPublicationStore.actions.fetchGetPublications
    • [x] Create baseSelectors object.
      • [x] Add getPublications which should return state.publications.
  • [x] Use Data.combineStores and pass the following
    • [x] baseInitialState as initialState
    • [x] baseActions, reducer as empty object.
    • [x] Set resolvers and pass baseResolvers.
    • [x] Set selectors and pass baseSelectors.

Test Coverage

  • [x] Add tests for publications data store. We can use fetchMock to mock the API calls.

QA Brief

Changelog entry

nfmohit avatar Jun 04 '24 05:06 nfmohit

IB ✅

nfmohit avatar Jun 20 '24 18:06 nfmohit

Hi @ankitrox . About the IB, I've removed the requirement of only including the publicationID and displayName properties in the publication object. Ideally, it should include all properties that the API returns. Sorry for not noticing that earlier. I hope that's okay. Thanks!

nfmohit avatar Jun 26 '24 20:06 nfmohit

HI @nfmohit ,

That completely makes sense. In fact I realised this yesterday when I was working on #8837

Thank you for making that change.

ankitrox avatar Jun 27 '24 04:06 ankitrox

I've created the PR: https://github.com/google/site-kit-wp/pull/8950 for this issue, but keeping this issue in Execution because its dependencies #8786 #8791 are yet to be merged.

Once merged, the base branch for this PR needs to be changed to develop and test everything and move this to CR.

ankitrox avatar Jul 02 '24 07:07 ankitrox

QA Update: ⚠️

I've had an initial test and here are the results and queries:

  • I've created the site and triggered the script in browser console and it rightly pulled nothing as there was no Publications. The request was made with 200 status ✅

    Screenshot 2024-07-15 at 19 02 02 Screenshot 2024-07-15 at 19 02 41
  • After the first script execution, I went ahead to create a publication for the website and triggered the script again. However, this round, there was no request made to the publications endpoint. And also, the publication was not retrieved. I would assume we should have triggered and pulled the new publication. Could you review whether any fixes is required here? ⚠️

    Publication is not retrieved: Screenshot 2024-07-15 at 19 24 02

    https://github.com/user-attachments/assets/02aeff5a-3b3b-4a03-8448-b133e9c8ffba

  • One thing to flag is after setting up the RRM module, at the module settings, it shows: "Complete setup for Reader Revenue Management" Question is: is that expected currently due to the implementation is not complete yet or it should have been successful? ⚠️

    Screenshot 2024-07-15 at 18 59 19

kelvinballoo avatar Jul 15 '24 15:07 kelvinballoo

  • After the first script execution, I went ahead to create a publication for the website and triggered the script again. However, this round, there was no request made to the publications endpoint. And also, the publication was not retrieved. I would assume we should have triggered and pulled the new publication. Could you review whether any fixes is required here? ⚠️

getPublications is a selector which queries the API only when the data is not available. When we trigger the script for the first time, data is gathered from the API and is available for subsequent runs, so it will not call the API further.

We have a separate ticket where maybeSyncPublicationOnboardingState selector will periodically sync and update the publications in the database.

One thing to flag is after setting up the RRM module, at the module settings, it shows: "Complete setup for Reader Revenue Management" Question is: is that expected currently due to the implementation is not complete yet or it should have been successful?

Yes, this is expected! The reason for that is we do not have the #8800 ready yet, but you can refer to #8796 IB where step 2 would help to resolve this issue.

  const settings = {
    publicationID: "CAow6J6vDA",
    publicationOnboardingState: "ONBOARDING_STATE_UNSPECIFIED",
    publicationOnboardingStateLastSyncedAtMs: 0
  };

  googlesitekit.data.dispatch('modules/reader-revenue-manager').setSettings( settings );
  googlesitekit.data.dispatch('modules/reader-revenue-manager').saveSettings();

ankitrox avatar Jul 16 '24 17:07 ankitrox

QA Update ✅

  • I've created the site and triggered the script in browser console and it rightly pulled nothing as there was no Publications. The request was made with 200 status ✅

    Screenshot 2024-07-15 at 19 02 02 Screenshot 2024-07-15 at 19 02 41
  • I've also created another side and created a publication for it. After running the script in browser console, it rightly pulled the publication with the proper endpoint with 200 status. ✅

    Screenshot 2024-07-17 at 17 10 57 Screenshot 2024-07-17 at 17 11 16

Moving ticket to Approval.

kelvinballoo avatar Jul 17 '24 13:07 kelvinballoo