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

Implement RRM getPublications() selector #8794

Closed
16 tasks done
nfmohit opened this issue Jun 4, 2024 · 7 comments
Closed
16 tasks done

Implement RRM getPublications() selector #8794

nfmohit opened this issue Jun 4, 2024 · 7 comments
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Jun 4, 2024

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

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

Test Coverage

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

QA Brief

  • Activating the rrmModule feature flag and the Reader Revenue Manager module.
  • Run the following command in browser console.
googlesitekit.data.select( 'modules/reader-revenue-manager' ).getPublications();
  • This will trigger a request to google-site-kit/v1/modules/reader-revenue-manager/data/publications endpoint.
  • It should return an empty array or array of publication objects depending on whether the publications are setup in the account associated with the site URL, with 200 response code.

Changelog entry

  • Add Reader Revenue Manager data store functionality to list available publications.
@nfmohit nfmohit self-assigned this Jun 4, 2024
@nfmohit nfmohit added Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 Module: RRM Reader Revenue Manager module related issues labels Jun 4, 2024
@nfmohit nfmohit changed the title Add getPublications selector for RRM Implement RRM getPublications() selector Jun 4, 2024
@ivonac4 ivonac4 added the P0 High priority label Jun 5, 2024
@nfmohit nfmohit removed their assignment Jun 8, 2024
@ivonac4 ivonac4 added Module: RRM Reader Revenue Manager module related issues Next Up Issues to prioritize for definition and removed Module: RRM Reader Revenue Manager module related issues labels Jun 11, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 18, 2024
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jun 20, 2024
@nfmohit nfmohit self-assigned this Jun 20, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 20, 2024

IB ✅

@nfmohit nfmohit removed their assignment Jun 20, 2024
@ivonac4 ivonac4 added Sp Wk 2 Issues to be completed in the second week of the assigned sprint and removed Next Up Issues to prioritize for definition labels Jun 21, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 26, 2024

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!

@ankitrox
Copy link
Collaborator

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
Copy link
Collaborator

ankitrox commented Jul 2, 2024

I've created the PR: #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 ankitrox assigned ankitrox and unassigned ankitrox Jul 4, 2024
@nfmohit nfmohit assigned nfmohit and ankitrox and unassigned nfmohit Jul 8, 2024
@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Jul 10, 2024
@nfmohit nfmohit removed their assignment Jul 11, 2024
@eclarke1 eclarke1 removed the Sp Wk 2 Issues to be completed in the second week of the assigned sprint label Jul 11, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Jul 15, 2024

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
    2nd.try.mov
  • 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

@ankitrox
Copy link
Collaborator

  • 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();

@kelvinballoo
Copy link
Collaborator

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 kelvinballoo removed their assignment Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants