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 maybeSyncPublicationOnboardingState() action #8797

Closed
1 of 10 tasks
nfmohit opened this issue Jun 4, 2024 · 12 comments
Closed
1 of 10 tasks

Implement RRM maybeSyncPublicationOnboardingState() action #8797

nfmohit opened this issue Jun 4, 2024 · 12 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 maybeSyncPublicationOnboardingState() action should be implemented for the Reader Revenue Manager module that periodically dispatches the syncPublicationOnboardingState() action (once every hour) to sync the value of the publicationOnboardingState module setting.

  • The maybeSyncPublicationOnboardingState() action should be dispatched in the Site Kit dashboard on load, similar to how it is done for syncGoogleTagSettings().
  • See relevant section in the design doc.

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

Acceptance criteria

  • A maybeSyncPublicationOnboardingState() action should be implemented for the modules/reader-revenue-manager data store.
  • This action should check the value of the publicationOnboardingStateLastSyncedAtMs module setting and determine if it has been more than an hour since the last sync. If so, the syncPublicationOnboardingState() action should be dispatched.
  • The maybeSyncPublicationOnboardingState() action should be dispatched in the Site Kit dashboard on load, similar to how it is done for syncGoogleTagSettings().

Implementation Brief

  • In assets/js/components/DashboardEntryPoint.js, collect actions object of RRM module using useDispatch
        const rrmActions = useDispatch( READER_REVENUE_MANAGER );
    • Call rrmActions?.maybeSyncPublicationOnboardingState() inside useMount hook.
  • Create a new generator action maybeSyncPublicationOnboardingState inside assets/js/module/reader-revenue-manager/datastore/publications.js
    • Wait for getModules to resolved.
    • Return null if module is not connected.
    • Call the getSettings selector on module and wait for it to be resolved so that settings are available before we make another check. Refer this for similar approach.
    • Call the getPublicationOnboardingStateLastSyncedAtMs selector on module. If the value is present and an hour has not passed since last sync, return from the action and do nothing. Refer here.
      • Otherwise, dispatch an action syncPublicationOnboardingState which would update the settings on server as well as in the local state for the module based on following.
        • If more than an hour has passed since last sync.
        • If the value of getPublicationOnboardingStateLastSyncedAtMs is zero.

Test Coverage

Add tests for maybeSyncPublicationOnboardingState action with different values for publicationOnboardingStateLastSyncedAtMs setting.

QA Brief

  • Follow the QAB of 8796 to setup RRM module and and verify that the onboarding state is ONBOARDING_STATE_UNSPECIFIED and publicationOnboardingStateLastSyncedAtMs is zero. This can be verified by using googlesitekit.data.select('modules/reader-revenue-manager').getSettings().
  • Visit Site Kit Dashboard.
  • In the network tab, there should be a request to GET:modules/reader-revenue-manager/data/publications and the publicationOnboardingStateLastSyncedAtMs should be updated. Use the above snippet to verify the timestamp.
  • For subsequent refreshes in the next 1 hour, there should be no request to the above endpoint and timestamp should remain unchanged.
  • Once 1 hour has passed, revisiting the dashboard should do the sync again using the endpoint and update the timestamp accordingly.

Changelog entry

  • Add a mechanism to synchronize the onboarding state of a publication once every hour in the Reader Revenue Manager module.
@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
@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 and removed Module: RRM Reader Revenue Manager module related issues labels Jun 11, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 14, 2024
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jun 28, 2024
@nfmohit nfmohit self-assigned this Jun 30, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 6, 2024

Thank you for the IB, @ankitrox ! Please take a look at my comments below:

  • Return if module is not connected.

Let's also wait for getModules to resolve before we check for the module's connection status.

  • If more than an hour has passed since last sync, dispatch an action syncPublicationOnboardingState which would update the settings on server as well as in the local state for the module.

It would also be beneficial to mention that this should also run if getPublicationOnboardingStateLastSyncedAtMs is not set, i.e. 0 as well.


Please let me know if you have any questions, thank you!

@nfmohit nfmohit assigned ankitrox and unassigned nfmohit Jul 6, 2024
@ankitrox
Copy link
Collaborator

ankitrox commented Jul 8, 2024

Thank you @nfmohit .

I have updated the couple of points mentioned in the feedback.

@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Jul 8, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jul 9, 2024

Thank you @ankitrox ! I have made some small improvements to the IB, namely:

  • We do not need to return undefined if getModules has not resolved, simply using resolveSelect should be sufficient.

The IB LGTM 👍 ✅

@aaemnnosttv
Copy link
Collaborator

@kuasha420 has addressed this correctly in his PR but I just wanted to note how we should not be introducing any module-specific effects/coupling into core components like DashboardEntryPoint or DashboardMainApp. See #8211

@nfmohit nfmohit removed their assignment Jul 31, 2024
@wpdarren wpdarren assigned wpdarren and kelvinballoo and unassigned wpdarren Aug 1, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Not sure if we need to update the QAB but I tried to set it up but I can't get the 'GET:modules/reader-revenue-manager/data/publications' request.

Video is attached for reference:

request.not.there.1080p.mov

@kuasha420
Copy link
Contributor

kuasha420 commented Aug 3, 2024

Hi @kelvinballoo, I just tested develop and the endpoint is correct and is getting hit. Can you try again? I'm not sure why it didn't work for you, maybe the zip on develop didn't update correctly.

Screenshot_20240803_085143

@kuasha420 kuasha420 removed their assignment Aug 3, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Hi @kuasha420 , thanks.
I've been able to get the request now:
Screenshot 2024-08-05 at 15 57 57

From where do I actually find the time stamp though? ⚠️
I ran the snippet googlesitekit.data.select('modules/reader-revenue-manager').getSettings() but I don't see anything related to timestamp from there:
Screenshot 2024-08-05 at 15 58 33

@kuasha420
Copy link
Contributor

Hi @kelvinballoo Sorry for the confusion. The timestamp is stored as publicationOnboardingStateLastSyncedAtMs which is 0 in your screenshot. Hope that clears it up.

@kuasha420 kuasha420 removed their assignment Aug 6, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Hi @kuasha420
Noted that publicationOnboardingStateLastSyncedAtMs being 0 is the timestamped.
One follow up question from there:

Once 1 hour has passed, revisiting the dashboard should do the sync again using the endpoint and update the timestamp accordingly.

What am I expecting for the update to the timestamp? 1 for 1 hour ? Or any other value?

@kelvinballoo
Copy link
Collaborator

QA Update ❌

Hi @kuasha420 , as per our sync, I'm always getting the publicationOnboardingStateLastSyncedAtMs as 0, even the first round or after 1 hour+.

Assigning to you for investigation.

Screenshot 2024-08-07 at 11 34 11

@aaemnnosttv
Copy link
Collaborator

@kelvinballoo Moving this back to QA now that #9163 has been merged, as @kuasha420 mentioned that it should address the issue here #9163 (review)

@aaemnnosttv aaemnnosttv assigned kelvinballoo and unassigned kuasha420 Aug 7, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Tested this and it's looking good now.
Moving ticket to Approval.

  • In the network tab, there is a request to GET:modules/reader-revenue-manager/data/publications and the publicationOnboardingStateLastSyncedAtMs was updated to the time the request was triggered. ✅

    Screenshot 2024-08-08 at 11 07 49 Screenshot 2024-08-08 at 11 09 23

    Verified that the epoch time corresponded to the trigger time.
    Screenshot 2024-08-08 at 11 10 30

  • For subsequent refreshes in the next 1 hour, there was no request to the above endpoint and timestamp remain unchanged. ✅

    There was no publication request again after refreshing within the next hour Screenshot 2024-08-08 at 11 14 38

    publicationOnboardingStateLastSyncedAtMs remained unchanged (Note: This one was for another timeframe, the requests were made 5 mins after the other and the values were the same)

    Screenshot 2024-08-08 at 13 16 50
  • Once 1 hour has passed, revisiting the dashboard does the sync again using the endpoint and updates the timestamp accordingly. ✅

    After 1hour plus, publications request was made when refreshing SK dashboard. Screenshot 2024-08-08 at 12 55 05

    New timestamp was made:

    Screenshot 2024-08-08 at 12 55 21 Screenshot 2024-08-08 at 12 55 37

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

7 participants