Add syncPublicationOnboardingState action #9017
Conversation
|
Build files for bef1496 have been deleted. |
|
Size Change: +324 B (+0.02%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
assets/js/modules/reader-revenue-manager/datastore/constants.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.test.js
Outdated
Show resolved
Hide resolved
|
Thank you @techanvil for reviewing the PR and adding your valuable feedback. I would also like to thank you for your guidance on testing I have addressed your feedback and sending your way for another review. Thanks again. |
assets/js/modules/reader-revenue-manager/datastore/publications.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/datastore/publications.js
Outdated
Show resolved
Hide resolved
techanvil
deleted the
enhancement/8796-sync-publication-onboarding-state-action
branch
| import { commonActions, combineStores } from 'googlesitekit-data'; | ||
| import { createFetchStore } from '../../../googlesitekit/data/create-fetch-store'; | ||
| import { | ||
| MODULES_READER_REVENUE_MANAGER, | ||
| MODULE_SLUG, |
There was a problem hiding this comment.
We have an (old) related issue about this. See #3417.
It's worth noting that we included the name in the constant for module stores which previously all used STORE_NAME internally and aliases externally. So this kind of generic constant is one we'll want to avoid here as well.
There was a problem hiding this comment.
Thanks for flagging this @aaemnnosttv .
I will rename this to READER_REVENUE_MANAGER_MODULE_SLUG in subsequent issue #8846
There was a problem hiding this comment.
Let's keep the SLUG part as a prefix rather than a suffix, similar to the store name constant.
There was a problem hiding this comment.
Honestly, I'd be happy to use RRM everywhere instead of the full name which is quite long as can be seen here 😅
There was a problem hiding this comment.
I should mention here that @ankitrox had previously used RRM in another constant name but I advised him against it, referring to our naming conventions whereby we avoid abbreviations in variable names etc.
Tbh, I was wondering if I was being a bit dogmatic, but was simply going on memories of past feedback. I'm entirely happy if we take a more pragmatic approach and use the RRM abbreviation for names in the codebase!
| } from './constants'; | ||
|
|
||
| const fetchGetPublicationsStore = createFetchStore( { | ||
| baseName: 'getPublications', | ||
| controlCallback: () => | ||
| API.get( | ||
| 'modules', | ||
| 'reader-revenue-manager', | ||
| MODULE_SLUG, | ||
| 'publications', | ||
| {}, | ||
| { useCache: true } |
There was a problem hiding this comment.
Unrelated observation – but selectors like this one which are only really used during setup should not use cache so that it pulls fresh data every time, otherwise there is no way to pull a new publication after creating one on the service once cached.
There was a problem hiding this comment.
Good spot @aaemnnosttv.
It would be nice to roll this one-line fix into a forthcoming RRM PR rather than create a separate issue for it.
@ankitrox, could I ask you to please include it in your next PR?
| { useCache: true } | |
| { useCache: false } |
| @@ -94,6 +190,7 @@ const baseResolvers = { | |||
| .getPublications(); | |||
| if ( publications === undefined ) { | |||
| yield fetchGetPublicationsStore.actions.fetchGetPublications(); | |||
| yield baseActions.syncPublicationOnboardingState(); | |||
There was a problem hiding this comment.
@techanvil @ankitrox Why did we add this here? Syncing the onboarding state only makes sense in the context of a selected publication in settings which this selector/resolver is not related to. Also, since this will usually be called before any selection is made I would expect this to not have the intended result. We should only sync the onboarding state for the publication which is saved in settings, not simply selected in state.
There was a problem hiding this comment.
@aaemnnosttv this is defined as a requirement in the design doc, and the AC/IB for this issue. So, I haven't really questioned it. My presumption is that the intention is to resync the onboarding state for the currently selected publication any time we visit the Settings screen after it has been connected. Arguably, though, it could make sense to decouple the sync from getPublications(). Maybe @nfmohit can shed a bit more light on this one?
There was a problem hiding this comment.
Thank you for raising this, @aaemnnosttv. @techanvil is correct with the presumption above. Since we show the PublicationOnboardingStateNotice in the settings screen based on the publicationOnboardingState module setting which is updated once every hour, there is a chance we may show an unintended notice due to the onboarding state not being up to date.
Since the getPublications selector would likely be resolved in SettingsEdit, my intention was to dispatch the syncPublicationOnboardingState action that would update the onboarding state in the module settings. The action only does this if the module is connected and a publication ID is set.
This would ensure that the correct PublicationOnboardingStateNotice is shown (or not shown at all). Would you suggest we do this any differently?
There was a problem hiding this comment.
Now that I think deeply about this, I feel this is not really necessary and does not add much value and has the potential to cause problems as Evan mentioned above. @techanvil What do you think of the idea of removing this dispatch as part of a subsequent issue?
There was a problem hiding this comment.
If the intention is to sync the state on the settings view, then I think it would be more appropriate to move the effect to that component as it's not related to this selector.
We also shouldn't assume this selector will only be called in settings in the future, so this could lead to unexpected behavior.
There was a problem hiding this comment.
Thanks @nfmohit and @aaemnnosttv. Agreed, it would be good to move the sync out to the component.
I think this merits a new issue to flesh out the details. We also need to revisit the PublicationSelect component due to the divergence from the IB in PR #9006:
Note:
publicationOnboardingStateLastSyncedAtMsis not getting updated as part ofonPublicationChangefunction because it will be updated as part of thesyncPublicationOnboardingStateaction, which will run immediately before (getPublicationsresolver)/after (maybeSyncPublicationsOnboardingState) a publication is selected.
@nfmohit, would you be happy to create an issue for this?
There was a problem hiding this comment.
Agreed, it would be good to move the sync out to the component.
I think this merits a new issue to flesh out the details.
Thank you @techanvil. About this one, now that I think about it, I feel updating it in SettingsEdit might actually have unexpected results. The action is responsible for showing the publication approved overlay notification by changing a core/ui data store key, which would only happen when the action is dispatched in the main dashboard. Also, the state would be updated once every hour anyway, so just updating it only for SettingsEdit doesn't seem to be worth it while the publication onboarding state notice also appears in SettingsView. To be fair, dispatching this as a part of the resolver was planned for redundancy.
I think this merits a new issue to flesh out the details. We also need to revisit the
PublicationSelectcomponent due to the divergence from the IB in PR #9006:Note:
publicationOnboardingStateLastSyncedAtMsis not getting updated as part ofonPublicationChangefunction because it will be updated as part of thesyncPublicationOnboardingStateaction, which will run immediately before (getPublicationsresolver)/after (maybeSyncPublicationsOnboardingState) a publication is selected.
About this one, I also feel updating publicationOnboardingStateLastSyncedAtMs as part of syncPublicationOnboardingState is sufficient.
Please let me know what you think, thanks so much!
There was a problem hiding this comment.
Thank you @techanvil. About this one, now that I think about it, I feel updating it in
SettingsEditmight actually have unexpected results. The action is responsible for showing the publication approved overlay notification by changing acore/uidata store key, which would only happen when the action is dispatched in the main dashboard. Also, the state would be updated once every hour anyway, so just updating it only forSettingsEditdoesn't seem to be worth it while the publication onboarding state notice also appears inSettingsView. To be fair, dispatching this as a part of the resolver was planned for redundancy.
Thanks @nfmohit. As you've pointed out, the publication onboarding state notice appears both in SettingsEdit and SettingsView, so it's a bit of an oddity to only sync the state in SettingsEdit. I think we could either dispatch the sync in SettingsView instead, ensuring the notice is up-to-date in both places, or not at all, and allow for the fact it could be stale. Obviously from UX perspective it would be better if it was up-to-date, so I'd lean in that direction; that said we might then want to consider a loading state which could add a bit of complexity. Maybe we can simply omit it for now and add it as an improvement post launch.
If we do still sync on the Settings screen, setting the core/ui value won't have any effect so I don't see it being too much of a concern here; that said we could still amend the action so as to avoid that if not on the dashboard.
Note:
publicationOnboardingStateLastSyncedAtMsis not getting updated as part ofonPublicationChangefunction because it will be updated as part of thesyncPublicationOnboardingStateaction, which will run immediately before (getPublicationsresolver)/after (maybeSyncPublicationsOnboardingState) a publication is selected.About this one, I also feel updating
publicationOnboardingStateLastSyncedAtMsas part ofsyncPublicationOnboardingStateis sufficient.
Hmm. If we are removing the call to syncPublicationOnboardingState() from the Settings screen entirely, then the logic about it being run immediately before a publication is selected no longer holds true.
So, we could find ourselves making another call to the API in order to sync the onboarding state when we land on the dashboard, even though we've just updated the value.
I would say it would be preferable to avoid this to help reduce unneeded API requests and the relevant API quota usage. So my preference would be to update the timestamp if we are not syncing on the Settings screen. However, it's a pretty marginal point and again something we could address post launch.
There was a problem hiding this comment.
Looks good, nice one Nahid!
Summary
Addresses issue:
syncPublicationOnboardingState()action #8796Relevant 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