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.

Sign up for GitHub

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

Jump to bottom

Add syncPublicationOnboardingState action #9017

Merged
merged 13 commits into from
Jul 19, 2024
Merged

Add syncPublicationOnboardingState action #9017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ff7f465
Add syncPublicationOnboardingState action.
ankitrox Jul 15, 2024
bacfb31
Add onboarding constants and UI store key.
ankitrox Jul 15, 2024
1749b7e
Set UI store key if onboarding state transitions to complete.
ankitrox Jul 15, 2024
e06cc64
Call action from getPublications resolver.
ankitrox Jul 15, 2024
3936c68
Merge branch 'develop' into enhancement/8796-sync-publication-onboard…
ankitrox Jul 16, 2024
79a1161
Fix tests.
ankitrox Jul 16, 2024
297c496
Use await actions and resolveSelect.
ankitrox Jul 16, 2024
878c8df
Resolve conflict: merge develop.
ankitrox Jul 16, 2024
e89e18b
Update doc blocks.
ankitrox Jul 16, 2024
9586f22
Merge branch 'develop' into enhancement/8796-sync-publication-onboard…
ankitrox Jul 18, 2024
909ea55
Optimize tests.
ankitrox Jul 19, 2024
24fb10d
Fix typo in comment.
techanvil Jul 19, 2024
bef1496
Fix typo in comment.
techanvil Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion assets/js/modules/reader-revenue-manager/datastore/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,18 @@
* limitations under the License.
*/

export const ERROR_CODE_NON_HTTPS_SITE = 'non_https_site';

export const MODULES_READER_REVENUE_MANAGER = 'modules/reader-revenue-manager';

export const ERROR_CODE_NON_HTTPS_SITE = 'non_https_site';
export const MODULE_SLUG = 'reader-revenue-manager';

export const PUBLICATION_ONBOARDING_STATES = {
ONBOARDING_COMPLETE: 'ONBOARDING_COMPLETE',
ONBOARDING_ACTION_REQUIRED: 'ONBOARDING_ACTION_REQUIRED',
PENDING_VERIFICATION: 'PENDING_VERIFICATION',
UNSPECIFIED: 'ONBOARDING_STATE_UNSPECIFIED',
};

export const UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION =
'READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION';
101 changes: 99 additions & 2 deletions assets/js/modules/reader-revenue-manager/datastore/publications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,26 @@
*/

/**
* Internal dependencies
* Internal dependencies.
*/
import API from 'googlesitekit-api';
import { CORE_MODULES } from '../../../googlesitekit/modules/datastore/constants';
import { CORE_UI } from '../../../googlesitekit/datastore/ui/constants';
import { commonActions, combineStores } from 'googlesitekit-data';
import { createFetchStore } from '../../../googlesitekit/data/create-fetch-store';
import {
MODULES_READER_REVENUE_MANAGER,
MODULE_SLUG,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nfmohit @techanvil

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging this @aaemnnosttv .

I will rename this to READER_REVENUE_MANAGER_MODULE_SLUG in subsequent issue #8846

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the SLUG part as a prefix rather than a suffix, similar to the store name constant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'd be happy to use RRM everywhere instead of the full name which is quite long as can be seen here 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

PUBLICATION_ONBOARDING_STATES,
UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION,
} from './constants';

const fetchGetPublicationsStore = createFetchStore( {
baseName: 'getPublications',
controlCallback: () =>
API.get(
'modules',
'reader-revenue-manager',
MODULE_SLUG,
'publications',
{},
{ useCache: true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Suggested change
{ useCache: true }
{ useCache: false }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you folks! Just to make sure we don't miss this out, I've included this in my PR for #8800.

Expand All @@ -45,6 +49,98 @@ const baseInitialState = {
};

const baseActions = {
/**
* Syncronizes the onboarding state of the publication with the API.
* Updates the settings on the server.
*
* @since n.e.x.t
*
* @return {void}
*/
*syncPublicationOnboardingState() {
const registry = yield commonActions.getRegistry();
const connected = yield commonActions.await(
registry
.resolveSelect( CORE_MODULES )
.isModuleConnected( MODULE_SLUG )
);

// If the module is not connected, do not attempt to sync the onboarding state.
if ( ! connected ) {
return;
}

yield commonActions.await(
registry
.resolveSelect( MODULES_READER_REVENUE_MANAGER )
.getSettings()
);

const publicationID = registry
.select( MODULES_READER_REVENUE_MANAGER )
.getPublicationID();

// If there is no publication ID, do not attempt to sync the onboarding state.
if ( publicationID === undefined ) {
return;
}

techanvil marked this conversation as resolved.
Show resolved Hide resolved
const publications = registry
.select( MODULES_READER_REVENUE_MANAGER )
.getPublications();

// If there are no publications, do not attempt to sync the onboarding state.
if ( ! publications ) {
return;
}

const publication = publications.find(
// eslint-disable-next-line sitekit/acronym-case
( { publicationId } ) => publicationId === publicationID
);

techanvil marked this conversation as resolved.
Show resolved Hide resolved
// If the publication is not found, do not attempt to sync the onboarding state.
if ( ! publication ) {
return;
}

const { onboardingState } = publication;
const currentOnboardingState = registry
.select( MODULES_READER_REVENUE_MANAGER )
.getPublicationOnboardingState();

const settings = registry
.select( MODULES_READER_REVENUE_MANAGER )
.getSettings();

if ( onboardingState !== currentOnboardingState ) {
settings.publicationOnboardingState = onboardingState;
}

// eslint-disable-next-line sitekit/no-direct-date
settings.publicationOnboardingStateLastSyncedAtMs = Date.now();

registry
.dispatch( MODULES_READER_REVENUE_MANAGER )
.setSettings( settings );

// Save the settings to the API.
registry.dispatch( MODULES_READER_REVENUE_MANAGER ).saveSettings();

// If the onboarding state is complete, set the key in CORE_UI to trigger the notification.
if (
onboardingState ===
PUBLICATION_ONBOARDING_STATES.ONBOARDING_COMPLETE
) {
registry
.dispatch( CORE_UI )
.setValue(
UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION,
true
);
}
nfmohit marked this conversation as resolved.
Show resolved Hide resolved
},

/**
* Finds a matched publication.
*
Expand Down Expand Up @@ -94,6 +190,7 @@ const baseResolvers = {
.getPublications();
if ( publications === undefined ) {
yield fetchGetPublicationsStore.actions.fetchGetPublications();
yield baseActions.syncPublicationOnboardingState();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@aaemnnosttv aaemnnosttv Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: publicationOnboardingStateLastSyncedAtMs is not getting updated as part of onPublicationChange function because it will be updated as part of the syncPublicationOnboardingState action, which will run immediately before (getPublications resolver)/after (maybeSyncPublicationsOnboardingState) a publication is selected.

@nfmohit, would you be happy to create an issue for this?

Copy link
Collaborator

@nfmohit nfmohit Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PublicationSelect component due to the divergence from the IB in PR #9006:

Note: publicationOnboardingStateLastSyncedAtMs is not getting updated as part of onPublicationChange function because it will be updated as part of the syncPublicationOnboardingState action, which will run immediately before (getPublications resolver)/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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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: publicationOnboardingStateLastSyncedAtMs is not getting updated as part of onPublicationChange function because it will be updated as part of the syncPublicationOnboardingState action, which will run immediately before (getPublications resolver)/after (maybeSyncPublicationsOnboardingState) a publication is selected.

About this one, I also feel updating publicationOnboardingStateLastSyncedAtMs as part of syncPublicationOnboardingState is 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your input, Tom! I've included updating this module setting as part of my selectPublication action proposed in my PR for #8800.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice one Nahid!

}
},
};
Expand Down
Loading
Loading