Display change metrics feature tour.

[zutigrm]

Summary

Addresses issue:

• Display "Change Metrics" Feature Tour tooltip when widget is setup by another Admin #7346

Relevant technical choices

Had to add /* eslint-disable jsdoc/check-line-alignment */ in 3 utils functions for e2e, as eslint was complaining about alignment which is already aligned. Included is also e2e for shared key metrics tour, with 3 e2e plugins for it.

PR Author Checklist

• My code is tested and passes existing unit tests.
• My code has an appropriate set of unit tests which all pass.
• My code is backward-compatible with WordPress 5.2 and PHP 5.6.
• My code follows the WordPress coding standards.
• My code has proper inline documentation.
• I have added a QA Brief on the issue linked above.
• I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

---

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

Code Reviewer Checklist

• Run the code.
• Ensure the acceptance criteria are satisfied.
• Reassess the implementation with the IB.
• Ensure no unrelated changes are included.
• Ensure CI checks pass.
• Check Storybook where applicable.
• Ensure there is a QA Brief.

Merge Reviewer Checklist

• Ensure the PR has the correct target branch.
• Double-check that the PR is okay to be merged.
• Ensure the corresponding issue has a ZenHub release assigned.
• Add a changelog message to the issue.

[github-actions]

Build files for 7e4e26b are ready:

• Download development build
• Download production build

[aaemnnosttv]

Thanks @zutigrm ! This looks good for the most part, just a few things to clean up and possibly simplify around the tour itself.

Regarding the E2E additions, I appreciate the above-and-beyond effort here (I know most of this was a learning exercise for you), please save this in a separate branch for us to review and see how best to include in a new issue. This issue was defined calling for no tests for the tour itself and the amount of code added here is certainly non-trivial so I'd like to be able to spend a bit more time to review this thoroughly. There were also a few things added that might not be needed as mentioned below.

Overall, great job in tackling E2E. Hopefully we can keep most of what you did, but we need to approach it separately.

[aaemnnosttv]

The version isn't considered for on-demand tours, so if this stays as such, we can simply remove this line.

For normal tours, this should be the version that the feature it's showing was introduced. This won't be for a while yet so in this case we'd use a future version to make sure it continues to be shown, and then we'd update it to be the version it is released to all users in.

[aaemnnosttv]

@techanvil @tofumatt Why did we decide to go with an on-demand tour? It seems like a good candidate for a normal "automatic" tour. All of this state for conditionally invoking the tour would be part of the tour's checkRequirements function, and it would automatically be triggered when the element is present. 🤔

An on-demand trigger should really be triggered in response to an explicit action, rather than as an effect. Let me know if I'm missing something relevant here.

[tofumatt]

@aaemnnosttv It was suggested by @techanvil here: #7346 (comment). The code/logic to handle checking otherwise was actually a fair bit more complicated (see the issue history for my previous IBs without it), so we went with this option because it was much simpler to understand/implement.

[aaemnnosttv]

That makes sense, it would require additional logic to account for KMW showing if we were to remove it from here. In other tours such as the All Traffic widget, we'd used a flag in UI state to indicate when the widget was loaded as a similar case where we would otherwise need to leak a lot of detail into the tour's conditional check. If we went this route, the only change needed in this component would be to set that flag as a way around needing to duplicate conditions. The downside to this approach however is that it would be blocking in resolving the next tour and may never resolve if KMW isn't going to be shown, so it does seem better in this case to keep it as-is 👍

[aaemnnosttv]

As mentioned above, we can do the same thing via a selector from the ID value – I don't see a reason to pass this along from the server as a separate bit of state.

[zutigrm]

@aaemnnosttv This is also according to the IB, to include keyMetricsSetupCompletedByUserID for getting user ID and keep the current keyMetricsSetupCompleted as boolean
Should I change the approach per suggestion?
@tofumatt @techanvil

[aaemnnosttv]

See above #7661 (comment)

[zutigrm]

@aaemnnosttv Thanks for the feedback.
Had to keep keyMetricsSetupCompletedByUserID as separate selector, due to issue mentioned here - #7661 (comment)

The e2e is moved to a new branch shared-key-metrics-e2e

[aaemnnosttv]

Thanks @zutigrm – back to you with a few replies/clarifications and follow-up comments. Note: some comments are above rather than linked to this review.

Also, I observed an experience when attempting to test this locally where the tooltip disappeared almost immediately after appearing (before it was even fully loaded). See below

Kapture.2023-10-04.at.14.44.05.mp4

[aaemnnosttv]

That makes sense, it would require additional logic to account for KMW showing if we were to remove it from here. In other tours such as the All Traffic widget, we'd used a flag in UI state to indicate when the widget was loaded as a similar case where we would otherwise need to leak a lot of detail into the tour's conditional check. If we went this route, the only change needed in this component would be to set that flag as a way around needing to duplicate conditions. The downside to this approach however is that it would be blocking in resolving the next tour and may never resolve if KMW isn't going to be shown, so it does seem better in this case to keep it as-is 👍

[aaemnnosttv]

There's a fair amount of code here which is only needed for this effect rather than the component itself. How about we extract it to a custom hook, e.g. useChangeMetricsFeatureTourEffect? Not strictly necessary of course, but would be a nice refactoring if time allows.

[aaemnnosttv]

We're in the process of removing the ga4Reporting flag which will also for sure be removed before userInput is released so we can safely omit it here.

[zutigrm]

@aaemnnosttv Thank you for the feedback. I re-worked the state selector as discussed, and applied suggestions.

Also, I observed an experience when attempting to test this locally where the tooltip disappeared almost immediately after appearing (before it was even fully loaded). See below

Hm, this is odd, I can't replicate it on my end, with either Admin or shared role. Did site kit reset few times as well. Are you still seeing this?

[aaemnnosttv]

Thanks @zutigrm – this is very close, I'll push a few tweaks now to address the last remaining details 👍

[aaemnnosttv]

This isn't necessary since we can do the same in the one selector as this isn't a common need.

[aaemnnosttv]

I realize this is how it was before but we shouldn't be using selectors to make assertions about actions. These should test the effect of the action (state change, request, etc).

[aaemnnosttv]

Also, I observed an experience when attempting to test this locally where the tooltip disappeared almost immediately after appearing (before it was even fully loaded). See below

Hm, this is odd, I can't replicate it on my end, with either Admin or shared role. Did site kit reset few times as well. Are you still seeing this?

@zutigrm I found it was due to code in triggerTourForView which would receive a null tour if none were triggered. If one was triggered, it wouldn't override an existing tour, so this was the only way to encounter this problem. I've gone ahead and fixed it.

You'll notice there are a few more changes here than expected as I wanted to update the naming of the new value throughout for consistency. It wasn't actually very time consuming since the IDE is able to update references automatically.

Everything else should be along the lines of what we discussed 👍

[aaemnnosttv]

Thanks for your patience here @zutigrm!

[eugene-manuilov]

Thanks, @zutigrm. This is almost looks good to me. Left two comments with minor nitpicks. Could you please take a look at them?

[eugene-manuilov]

What is the point of adding this new hook? I don't see it is required in IB. I understand that you try to encapsulate the logic but extracting it into a separate hook makes sense only when there are multiple components that use it. In this situation we have just one place where we use this hook and there is no potential to have another component that needs this in the future. Let's merge this hook into the ChangeMetricsLink component to avoid bloating up our code base with extra files.

[zutigrm]

@eugene-manuilov It wasn't required in IB, it was a suggestion on previous CR, it was originally in ChangeMetricsLink, but since it was adding certain number of lines for this logic, it was moved to separate hook - comment for reference - #7661 (comment)

Let me know what you think

[eugene-manuilov]

Ah... okay, then let's keep it as is although that is less appropriate IMO because:

1. It makes us have more files in the codebase than we really need.
2. It makes the logic in the component harder to read and understand because we need to find and open an additional file to understand what happens in that hook.
3. Webpack will bundle component and hook as separate modules thus the size of the final bundle will be slightly bigger than it could be if we not extract that logic into a custom hook.

cc @aaemnnosttv

[aaemnnosttv]

Thanks for your review @eugene-manuilov ! A few thoughts in response to your comments above.

• It makes us have more files in the codebase than we really need.

We shouldn't have more files for no reason of course, but we often use more files to keep each of them smaller and easier to understand and extract complexity. I don't think that it only makes sense to extract components/hooks if there is more than one usage of them, there are plenty of instances where we extract parts to keep the overall size and complexity down, e.g. BannerNotification components.

• It makes the logic in the component harder to read and understand because we need to find and open an additional file to understand what happens in that hook.

I find this point a bit hard to see, as I feel that it's harder to understand the component with a large portion of its code only needed for the tour effect which is itself easier to understand in isolation IMO. In the context of the component, we see that it has a named effect about its tour, so you could argue that it's easier to understand in this way.

• Webpack will bundle component and hook as separate modules thus the size of the final bundle will be slightly bigger than it could be if we not extract that logic into a custom hook.

This is true, but the increase in size is probably only a handful of bytes which isn't a concern. There are likely many other things we could optimize to reduce the overall size of our assets in more impactful ways, so I don't think bundle size is an issue here. We have the compressed size action to help highlight changes to our bundle size which reported didn't show any substantial change here.

[eugene-manuilov]

This calculation should be defined outside of the useEffect hook to avoid unnecessary execution of this hook when the key metric settings are saved and the completed by value is changed from its original value to current user ID.

const isUserEligibleForTour =
	renderChangeMetricLink && 
	Number.isInteger( keyMetricsSetupCompletedBy ) &&
	Number.isInteger( currentUserID ) &&
	keyMetricsSetupCompletedBy > 0 &&
	currentUserID !== keyMetricsSetupCompletedBy;

useEffect( () => {
	if ( isUserEligibleForTour  ) {
		triggerOnDemandTour( sharedKeyMetrics );
	}
}, [ isUserEligibleForTour, triggerOnDemandTour ] );

[eugene-manuilov]

One more thing to update, everything else looks good.

[eugene-manuilov]

Looks good to me now. Thanks, @zutigrm.