Enhancement/9363 publication onboarding state sync server side #9673
+350
−245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build files for a6a2ffb are ready:
|
|
Size Change: +581 B (+0.03%) Total Size: 1.88 MB
ℹ️ View Unchanged
|
techanvil
requested changes
Nov 13, 2024
There was a problem hiding this comment.
Hi @ankitrox, this mostly works but I've realised there's a bug we didn't consider while drafting the IB and we'll need to diverge from the IB to avoid it.
Also, please can you add a scenario to the QAB for testing the sync on the settings screen (i.e. when clicking the Complete publication setup CTA and returning to the page after 15 seconds) to ensure the AC are verified.
Comment on lines
125
to
139
| if ( !! response?.publicationOnboardingState ) { | ||
| const newOnboardingState = response.publicationOnboardingState; | ||
|
|
||
| if ( | ||
| !! currentOnboardingState && | ||
| newOnboardingState !== currentOnboardingState && | ||
| newOnboardingState === | ||
| PUBLICATION_ONBOARDING_STATES.ONBOARDING_COMPLETE | ||
| ) { | ||
| setValue( | ||
| UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION, | ||
| true | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be marginally simplified:
Suggested change
| if ( !! response?.publicationOnboardingState ) { | |
| const newOnboardingState = response.publicationOnboardingState; | |
| if ( | |
| !! currentOnboardingState && | |
| newOnboardingState !== currentOnboardingState && | |
| newOnboardingState === | |
| PUBLICATION_ONBOARDING_STATES.ONBOARDING_COMPLETE | |
| ) { | |
| setValue( | |
| UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION, | |
| true | |
| ); | |
| } | |
| } | |
| const newOnboardingState = response?.publicationOnboardingState; | |
| if ( | |
| currentOnboardingState && | |
| newOnboardingState !== currentOnboardingState && | |
| newOnboardingState === | |
| PUBLICATION_ONBOARDING_STATES.ONBOARDING_COMPLETE | |
| ) { | |
| setValue( | |
| UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION, | |
| true | |
| ); | |
| } |
Comment on lines
81
to
108
| reducerCallback: ( | ||
| state, | ||
| { publicationOnboardingState, isSavedSetting } | ||
| ) => { | ||
| if ( ! publicationOnboardingState ) { | ||
| return state; | ||
| } | ||
|
|
||
| return { | ||
| ...state, | ||
| settings: { | ||
| ...state.settings, | ||
| publicationOnboardingState, | ||
| // eslint-disable-next-line sitekit/no-direct-date | ||
| publicationOnboardingStateLastSyncedAtMs: Date.now(), | ||
| }, | ||
| savedSettings: { | ||
| publicationOnboardingState: isSavedSetting | ||
| ? publicationOnboardingState | ||
| : state.savedSettings?.publicationOnboardingState, | ||
| publicationOnboardingStateLastSyncedAtMs: isSavedSetting | ||
| ? // eslint-disable-next-line sitekit/no-direct-date | ||
| Date.now() | ||
| : state.savedSettings | ||
| ?.publicationOnboardingStateLastSyncedAtMs, | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
This is implemented pretty much as the IB specifies, however I've realised that it actually introduces a bug and we need to diverge from the IB to avoid it.
At present, on the setup and settings screens, if the user selects a different publication while the sync request is in flight, the new onboarding state will be incorrectly applied to the newly selected publication. I've recorded a screencast to illustrate this:
9363-wrong-publication-update.mp4
To mitigate this we need to return the publicationID in the sync response and use that to determine which state gets updated (and so there's no need to return isSavedSetting). We'll also need to update the onboarding state for the publication found in state.publications. I'd recommend using createReducer() from the googlesitekit-data module to use Immer for the reducer so we can update the state directly, simplifying the reducer logic.
Suggested change
| reducerCallback: ( | |
| state, | |
| { publicationOnboardingState, isSavedSetting } | |
| ) => { | |
| if ( ! publicationOnboardingState ) { | |
| return state; | |
| } | |
| return { | |
| ...state, | |
| settings: { | |
| ...state.settings, | |
| publicationOnboardingState, | |
| // eslint-disable-next-line sitekit/no-direct-date | |
| publicationOnboardingStateLastSyncedAtMs: Date.now(), | |
| }, | |
| savedSettings: { | |
| publicationOnboardingState: isSavedSetting | |
| ? publicationOnboardingState | |
| : state.savedSettings?.publicationOnboardingState, | |
| publicationOnboardingStateLastSyncedAtMs: isSavedSetting | |
| ? // eslint-disable-next-line sitekit/no-direct-date | |
| Date.now() | |
| : state.savedSettings | |
| ?.publicationOnboardingStateLastSyncedAtMs, | |
| }, | |
| }; | |
| }, | |
| reducerCallback: createReducer( | |
| ( state, { publicationID, publicationOnboardingState } ) => { | |
| if ( ! publicationID ) { | |
| return; | |
| } | |
| // eslint-disable-next-line sitekit/no-direct-date | |
| const publicationOnboardingStateLastSyncedAtMs = Date.now(); | |
| if ( state.settings.publicationID === publicationID ) { | |
| state.settings.publicationOnboardingState = | |
| publicationOnboardingState; | |
| state.settings.publicationOnboardingStateLastSyncedAtMs = | |
| publicationOnboardingStateLastSyncedAtMs; | |
| } | |
| if ( state.savedSettings.publicationID === publicationID ) { | |
| state.savedSettings.publicationOnboardingState = | |
| publicationOnboardingState; | |
| state.savedSettings.publicationOnboardingStateLastSyncedAtMs = | |
| publicationOnboardingStateLastSyncedAtMs; | |
| } | |
| const publication = state.publications.find( | |
| // eslint-disable-next-line sitekit/acronym-case | |
| ( { publicationId: id } ) => id === publicationID | |
| ); | |
| if ( publication ) { | |
| publication.onboardingState = publicationOnboardingState; | |
| } | |
| } | |
| ), |
Cc @nfmohit
Comment on lines
273
to
291
| $saved_settings = false; | ||
| $settings = $this->get_settings(); | ||
|
|
||
| if ( $data['publicationID'] === $settings->get()['publicationID'] ) { | ||
| $settings->merge( | ||
| array( | ||
| 'publicationOnboardingState' => $new_onboarding_state, | ||
| ) | ||
| ); | ||
| $saved_settings = true; | ||
| } | ||
|
|
||
| $return_data = array( | ||
| 'publicationOnboardingState' => $new_onboarding_state, | ||
| ); | ||
|
|
||
| if ( $saved_settings ) { | ||
| $return_data['isSavedSetting'] = true; | ||
| } |
There was a problem hiding this comment.
As per my comment above, we should diverge from the IB and return publicationID here, while not returning isSavedSetting:
Suggested change
| $saved_settings = false; | |
| $settings = $this->get_settings(); | |
| if ( $data['publicationID'] === $settings->get()['publicationID'] ) { | |
| $settings->merge( | |
| array( | |
| 'publicationOnboardingState' => $new_onboarding_state, | |
| ) | |
| ); | |
| $saved_settings = true; | |
| } | |
| $return_data = array( | |
| 'publicationOnboardingState' => $new_onboarding_state, | |
| ); | |
| if ( $saved_settings ) { | |
| $return_data['isSavedSetting'] = true; | |
| } | |
| $settings = $this->get_settings(); | |
| if ( $data['publicationID'] === $settings->get()['publicationID'] ) { | |
| $settings->merge( | |
| array( | |
| 'publicationOnboardingState' => $new_onboarding_state, | |
| ) | |
| ); | |
| } | |
| $return_data = array( | |
| 'publicationID' => $data['publicationID'], | |
| 'publicationOnboardingState' => $new_onboarding_state, | |
| ); |
Cc @nfmohit
techanvil
reviewed
Nov 14, 2024
tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php
Outdated
Show resolved
Hide resolved
techanvil
requested changes
Nov 14, 2024
| @@ -96,7 +94,7 @@ describe( 'modules/reader-revenue-manager publications', () => { | |||
| .dispatch( MODULES_READER_REVENUE_MANAGER ) | |||
| .syncPublicationOnboardingState(); | |||
|
|
|||
| expect( syncStatus ).toBeUndefined(); | |||
| expect( syncStatus ).toEqual( {} ); | |||
| } ); | |||
|
|
|||
| it( 'should update the settings and call the saveSettings endpoint', async () => { | |||
There was a problem hiding this comment.
This test should be renamed as the action is no longer saving the settings. I'd suggest something like this:
it( 'should call the `sync-publication-onboarding-state` endpoint and update the settings in state', async () => {If renaming as suggested, please make the assertion for calling the endpoint more explicit:
| } ); | ||
|
|
||
| it( 'should not update the timestamp, call the saveSettings endpoint, and set UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION to true in CORE_UI store if there is no saved publication', async () => { | ||
| it( 'should not update the timestamp if onboarding state has not changed', async () => { |
There was a problem hiding this comment.
This test can be removed as there's no longer any timestamp specific logic in the syncPublicationOnboardingState() action.
| throw new Missing_Required_Param_Exception( 'publicationOnboardingState' ); | ||
| } | ||
|
|
||
| $return_data = array(); |
There was a problem hiding this comment.
There no real need to define $return_data here.
Suggested change
| $return_data = array(); |
Comment on lines
+278
to
+280
| return function () use ( $return_data ) { | ||
| return (object) $return_data; | ||
| }; |
There was a problem hiding this comment.
As per my comment above we don't need to refer to $return_data here, it's easier to follow if we just return an empty array directly:
Suggested change
| return function () use ( $return_data ) { | |
| return (object) $return_data; | |
| }; | |
| return function () { | |
| return (object) array(); | |
| }; |
| @@ -231,6 +232,92 @@ function ( $domain ) { | |||
| $this->assertEquals( $expected_filter, urldecode( $filter ) ); | |||
| } | |||
|
|
|||
| public function test_sync_publication_onboarding_state_returns_empty_object() { | |||
There was a problem hiding this comment.
This test name is not so useful as it simply describes the return value of the request, rather than the scenario that's being tested. Please can you make it more descriptive of the actual scenario.
This might illustrate the need for some additional tests to cover scenarios which are not currently tested.
| $this->assertEquals( (object) array(), $result ); | ||
| } | ||
|
|
||
| public function test_sync_publication_onboarding_state_returns_new_state() { |
There was a problem hiding this comment.
As per my comment above please can you update this test name to be more descriptive of the scenario that's being tested.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist