Fix condition to display the publication approved overlay notification #9163
Conversation
|
Build files for e7998f3 have been deleted. |
|
Size Change: -260 B (-0.02%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
| const connected = yield commonActions.await( | ||
| registry | ||
| .resolveSelect( CORE_MODULES ) | ||
| .isModuleConnected( READER_REVENUE_MANAGER_MODULE_SLUG ) | ||
| ); |
There was a problem hiding this comment.
I feel this is redundant here as we already do this check as part of maybeSyncPublicationOnboardingState which is the only place that eventually dispatches this action.
| // Do not attempt to sync the onboarding state if the publication ID | ||
| // in state is not the "saved" publication ID. | ||
| if ( hasPublicationIDChanged ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
As the sync is done on the client-side, there is a possibility where we may set an incorrect onboarding state in settings in case the publication ID has been changed in store. To avoid this, I've added this check so that we only attempt the sync when the publication ID has not changed from its saved value.
| const publications = yield commonActions.await( | ||
| registry | ||
| .resolveSelect( MODULES_READER_REVENUE_MANAGER ) | ||
| .getPublications() | ||
| ); |
There was a problem hiding this comment.
The response for this selector should be waited for.
| if ( onboardingState !== currentOnboardingState ) { | ||
| settings.publicationOnboardingState = onboardingState; | ||
| } | ||
|
|
||
| // eslint-disable-next-line sitekit/no-direct-date | ||
| settings.publicationOnboardingStateLastSyncedAtMs = Date.now(); |
There was a problem hiding this comment.
As @aaemnnosttv kindly pointed out internally, we should not set the settings in this way as this apparently mutates the original settings object in registry. Rather, we should clone and create a new object, or just use setSettings directly as I've done in this PR.
kuasha420
force-pushed
the
enhancement/#9089-fix-conditions
branch
from
f307170
to
17d9c73
Compare
There was a problem hiding this comment.
@nfmohit Thanks. This looks good to me and also fixes the issue we're having on #8797. Assigning this to @aaemnnosttv for merging in main since we want to include this in the current release which has been branched off already.
Note that I've rebated this on top of main to prevent including any unrelated changes. It was pretty clean manual rebase with cherry picking.
| const hasPublicationIDChanged = registry | ||
| .select( MODULES_READER_REVENUE_MANAGER ) | ||
| .hasSettingChanged( 'publicationID' ); |
There was a problem hiding this comment.
This will return false if settings haven't been loaded yet, but also, this selector won't invoke the resolver for getSettings so we need to resolve select those first to ensure the state is loaded before calling this.
There was a problem hiding this comment.
Thank you for addressing this, @aaemnnosttv !
Summary
Addresses issue:
Relevant technical choices
This PR includes the following unrelated changes:
syncPublicationOnboardingStateaction to do the following: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