google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.22k stars 278 forks source link

Implement RRM `maybeSyncPublicationOnboardingState()` action #8797

Closed nfmohit closed 1 month ago

nfmohit commented 3 months ago

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.


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

Acceptance criteria

Implementation Brief

Test Coverage

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

QA Brief

Changelog entry

nfmohit commented 2 months ago

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!

ankitrox commented 2 months ago

Thank you @nfmohit .

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

nfmohit commented 2 months ago

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

The IB LGTM 👍 ✅

aaemnnosttv commented 1 month ago

@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

kelvinballoo commented 1 month ago

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:

https://github.com/user-attachments/assets/22c91d45-2bbf-4c1f-ad67-6216106095b7
kuasha420 commented 1 month ago

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

kelvinballoo commented 1 month ago

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 commented 1 month ago

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

kelvinballoo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@kelvinballoo Moving this back to QA now that #9163 has been merged, as @kuasha420 mentioned that it should address the issue here https://github.com/google/site-kit-wp/pull/9163#pullrequestreview-2225486186

kelvinballoo commented 1 month ago

QA Update ✅

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