search

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.23k stars 279 forks source link

Remove RRM onboarding state notice when state changes #9262

Open nfmohit opened 2 weeks ago

nfmohit commented 2 weeks ago

Feature Description

Raised in RRM bug bash here.

Currently, when an RRM publication has onboarding states of either ONBOARDING_ACTION_REQUIRED or PENDING_VERIFICATION, we display an onboarding state notice in the RRM settings screens.

The problem in question is more prevalent in the case of ONBOARDING_ACTION_REQUIRED, where we display the following notice:

image

Once the user navigates to the Publisher Center upon clicking on the CTA, actions their remaining steps, and returns to Site Kit, they still see the same notice. This has the potential to confuse the user as in reality, the publication no longer requires any further setup.

This happens because Site Kit stores the publication's onboarding state in the WordPress database and syncs it every 24 hours, i.e. if the user in the above example returns to Site Kit after the next sync, they'd see that the notice no longer exists. However, we should look into improving this so that the UX is further refined.

As part of triage, we've identified two potential approaches here:

  1. Upon clicking the CTA in the notice, we simply remove the notice until at least the next sync.
  2. Upon clicking the CTA, we wait for the user to come back to the Site Kit tab from Publisher Center. Once they do, immediately sync the onboarding state in the background and remove the notice if applicable.

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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

techanvil commented 1 week ago

Hey @nfmohit, it looks like PublicationOnboardingStateNotification is a typo in the AC and should be PublicationOnboardingStateNotice.

That aside, it's worth noting that these component names are more of an implementation detail and are not so useful in the AC when considering them for a non-developer perspective, e.g. QA, PMs etc.

As a quick way to improve it I'd suggest linking to the relevant stories for each component:

nfmohit commented 1 week ago

AC updated, thank you @techanvil!

techanvil commented 1 week ago

That's great, thanks @nfmohit!

AC :white_check_mark:

benbowler commented 1 week ago

Bumped the estimate a little bit for the test requirements.

techanvil commented 1 week ago

Thanks @benbowler.

nfmohit commented 1 week ago

@benbowler I'm not the IB reviewer here but I've made some updates to the relationship between this issue (#9262) and #9149. In summary:

@techanvil Does this sound good to you?

CC: @ankitrox

nfmohit commented 1 week ago

@benbowler An additional note: One of the consumers for the PublicationOnboardingStateNotice is the RRM setup flow, i.e. assets/js/modules/reader-revenue-manager/components/setup/SetupForm.js. In this location, I believe the syncPublicationOnboardingState will not work because it only works when the module is already connected. We might have to work around that somehow.

It's also worth noting that we're in the discussions of removing the PublicationOnboardingStateNotice (or at least its CTA) from the RRM setup flow as this was reported to be confusing for users in the bug bash. However, that has been triaged to be a post-launch initiative. Sigal has kindly proposed to remove the CTA from this notification in the setup flow and update the copy. If that is agreed upon, we can prioritise that issue as well so that we can stick to the approach defined in this issue.

CC: @techanvil

techanvil commented 5 days ago

Thanks @nfmohit, that's a good point. Looking into it a bit more, it looks like we need to remove the check for the publication not having been changed in settings, as on the setup screen we won't have saved it yet anyway (although this is moot if we remove it from setup), and on the settings screen the user could have selected a property from the dropdown, clicked the CTA and completed the steps and returned to SK without saving the setting.

https://github.com/google/site-kit-wp/blob/36b8bcd74d85fd8fa3f560c6b8eaf6ca5031c84a/assets/js/modules/reader-revenue-manager/datastore/publications.js#L89-L97

So, effectively it's looking like we need to lift this block of code to a new action and use that, with a few tweaks (it needs to retrieve the current settings at the top, and at the end, it should only save the settings if the property is the currently saved one).

https://github.com/google/site-kit-wp/blob/36b8bcd74d85fd8fa3f560c6b8eaf6ca5031c84a/assets/js/modules/reader-revenue-manager/datastore/publications.js#L99-L135

techanvil commented 5 days ago

Incidentally if we're extracting the action as per my above comment, that should work in both the setup and settings flow, so we shouldn't need to wait for the decision on whether to remove the CTA from the setup flow.

benbowler commented 5 days ago

Hey @techanvil, I updated the IB with an implementation that doesn't require a new action to be created. Let me know if I've missed something here that requires a new action for this case.

nfmohit commented 5 days ago

Thanks @nfmohit, that's a good point. Looking into it a bit more, it looks like we need to remove the check for the publication not having been changed in settings, as on the setup screen we won't have saved it yet anyway (although this is moot if we remove it from setup), and on the settings screen the user could have selected a property from the dropdown, clicked the CTA and completed the steps and returned to SK without saving the setting.

https://github.com/google/site-kit-wp/blob/36b8bcd74d85fd8fa3f560c6b8eaf6ca5031c84a/assets/js/modules/reader-revenue-manager/datastore/publications.js#L89-L97

So, effectively it's looking like we need to lift this block of code to a new action and use that, with a few tweaks (it needs to retrieve the current settings at the top, and at the end, it should only save the settings if the property is the currently saved one).

https://github.com/google/site-kit-wp/blob/36b8bcd74d85fd8fa3f560c6b8eaf6ca5031c84a/assets/js/modules/reader-revenue-manager/datastore/publications.js#L99-L135

@techanvil what you suggested sounds good to me, thank you!

techanvil commented 5 days ago

Hey @techanvil, I updated the IB with an implementation that doesn't require a new action to be created. Let me know if I've missed something here that requires a new action for this case.

Thanks @benbowler. I think we can take the approach you've suggested and not extract an additional action, however a couple more changes are needed to syncPublicationOnboardingState():

A couple more points

techanvil commented 2 days ago

Thanks for the update @benbowler!

IB :white_check_mark:

nfmohit commented 14 hours ago

@techanvil I'm implementing this and have run into some hiccups and it'd be beneficial to get your advice before I choose a path:

  1. It appears that the syncPublicationOnboardingState action does not re-fetch the GET:publications endpoint if the publications were fetched before (e.g. in SettingsEdit and SetupMain), even though we're using resolveSelect. This is because the resolver for getPublications does not make the fetch request if publications are not undefined in state.
  2. We can get around this by running the resetPublications action before we make the resolveSelect call to getPublications, but in this case, the publication select has no options remaining to display for the user.
  3. Either way, when the fetch to GET:publications happens, the publication select goes on a loading state. This is something we can probably address by introducing a publications loading flag in store which we can set only when we want to show the loader, but the concern 2 above of the blank publication select still stands.

Any insights on these? Thank you!

CC: @benbowler