Bug/6763 web data stream not available #6816
Conversation
|
Size Change: +670 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
| @@ -222,6 +281,10 @@ WithGA4Tag.decorators = [ | |||
| ); | |||
| }, | |||
| ]; | |||
| WithGA4Tag.scenario = { | |||
There was a problem hiding this comment.
This story had degenerated into a broken state, and was fixed as a byproduct of this PR. This scenario was therefore added to provide some additional test coverage.
| <GoogleTagIDMismatchNotification /> | ||
| <WebDataStreamNotAvailableNotification /> |
There was a problem hiding this comment.
This order may need some thinking as it was not defined in AC/IB, but this made sense to me while implementing.
There was a problem hiding this comment.
This makes sense to me too, I think that's fine 👍🏻
There was a problem hiding this comment.
There was a problem hiding this comment.
After further review, it seems okay since the code that triggers it only happens when syncing Google tag settings so while this seems off at first glance, it should be fine.
| { isDisabled && ( | ||
| { isDisabled ? ( | ||
| <GA4ActivateSwitch | ||
| disabled={ ! hasAnalyticsAccess } | ||
| onActivate={ onActivate } | ||
| /> | ||
| ) } | ||
|
|
||
| { ! isDisabled && ( | ||
| ) : ( |
There was a problem hiding this comment.
Unrelated refactor that was needed to satisfy the cyclometic complexity eslint rules. 😅
There was a problem hiding this comment.
Wow, that's a real shame. I always find ternaries in JSX to be a nightmare to read and have frequently discouraged their use in the codebase up until now.
I'd much rather eslint-disable this rule here than satisfy it. It increases the complexity by changing two independent sections into one single section.
Ternaries are great for one-line, easily readable variable assignments, but for multiline, multi-component JSX like this they're brutal to follow.
Can this rule be disabled here with a note explaining that the ternary makes it harder to read?
There was a problem hiding this comment.
Just chipping in here to say, I think disabling the complexity rule here would be the wrong approach - it would be necessary to disable it for the whole function, meaning this function could continue to grow more complex with no limit to it. It seems a bit of a slippery slope to start disabling the rule like this.
If this refactor is objectionable, I'd suggest taking a different approach to refactoring the function rather than disabling the rule entirely...
There was a problem hiding this comment.
@techanvil @tofumatt yep, disabling the rule is per function and not per line so disabling the rule will really bite us in the future. I can try to find other way to claim back another execution path without this specific refactor, but as far as I remember this was the most straightforward and reasonable one 😅
There was a problem hiding this comment.
I actually don't mind the ternary here, but @tofumatt does seem quite strongly against it and I don't have a strong enough opinion on the matter to argue the point :)
There was a problem hiding this comment.
Well, I had to extract out the error related logic to a separate component anyway, as there were a few more changes that came from other merged PRs that put the complexity to the roof. Hence, I have reverted this change here but made @tofumatt's life harder by introducing another component and corresponding tests 😅
| <GoogleTagIDMismatchNotification /> | ||
| <WebDataStreamNotAvailableNotification /> |
There was a problem hiding this comment.
This makes sense to me too, I think that's fine 👍🏻
| description={ sprintf( | ||
| /* translators: 1: Google Analytics 4 Measurement ID. */ | ||
| __( | ||
| 'The previously selected web data stream with measurement ID %1$s is no longer available. Please select a new web data stream to continue collecting data with Google Analytics 4', |
There was a problem hiding this comment.
| 'The previously selected web data stream with measurement ID %1$s is no longer available. Please select a new web data stream to continue collecting data with Google Analytics 4', | |
| 'The previously selected web data stream with measurement ID %1$s is no longer available. Please select a new web data stream to continue collecting data with Google Analytics 4.', |
| @@ -889,6 +911,92 @@ describe( 'modules/analytics-4 properties', () => { | |||
| ).toEqual( googleTagID ); | |||
| } ); | |||
|
|
|||
| it( 'should set `isWebDataStreamNotAvailable` to `true` when there is no google tag conainer available', async () => { | |||
There was a problem hiding this comment.
| it( 'should set `isWebDataStreamNotAvailable` to `true` when there is no google tag conainer available', async () => { | |
| it( 'should set `isWebDataStreamNotAvailable` to `true` when there is no Google Tag Container available', async () => { |
| * @param {Object} state Data store's state. | ||
| * @return {boolean} If the Web Data Stream is not available. | ||
| */ | ||
| isWebDataStreamNotAvailable( state ) { |
There was a problem hiding this comment.
I know this was all spelled out in the IB, but the selector/data used here is a double-negative, which is a bit harder to reason about. The data stream is available when "is not available is false".
It might be a bit easier to reason about these values if the selector/data instead was isWebDataStreamAvailable and it was set to true by default.
We only use the selector in one place (https://github.com/google/site-kit-wp/pull/6816/files#diff-2b5924118d9c2ea36627f1a4869ae7400537225ece2b18d3256354cf1046ad80R50), but I think it'd be easier to read:
// If the data stream is available, we don't have to show a warning to the user.
if ( isWebDataStreamAvailable ) {
return null;
}instead of:
if ( ! isWebDataStreamNotAvailable ) {
return null;
}| { isDisabled && ( | ||
| { isDisabled ? ( | ||
| <GA4ActivateSwitch | ||
| disabled={ ! hasAnalyticsAccess } | ||
| onActivate={ onActivate } | ||
| /> | ||
| ) } | ||
|
|
||
| { ! isDisabled && ( | ||
| ) : ( |
There was a problem hiding this comment.
Wow, that's a real shame. I always find ternaries in JSX to be a nightmare to read and have frequently discouraged their use in the codebase up until now.
I'd much rather eslint-disable this rule here than satisfy it. It increases the complexity by changing two independent sections into one single section.
Ternaries are great for one-line, easily readable variable assignments, but for multiline, multi-component JSX like this they're brutal to follow.
Can this rule be disabled here with a note explaining that the ternary makes it harder to read?
There was a problem hiding this comment.
I didn't notice this before—so I'm not sure if it's due to some changes since the PR or if this was always happening, but I see a brief flash of the message when changing GA4 accounts in the Settings Edit component:
CleanShot.2023-04-14.at.19.04.17.mp4
It's worth noting some of these accounts are older but I think I actually do have access to them so the data stream should be available? It seems to happen while the GA4 data for that account is loading, as it goes away and doesn't appear instantly. So there's probably a condition for displaying the error that isn't quite right 🤔
| ); | ||
| } | ||
|
|
||
| if ( ! propertyAvailable && ! getPropertiesError ) { |
There was a problem hiding this comment.
| if ( ! propertyAvailable && ! getPropertiesError ) { | |
| if ( ! propertyAvailable && getPropertiesError === false ) { |
There was a problem hiding this comment.
Ah, this is what was causing the bug I encountered: getPropertiesError can be undefined (for instance, while loading) and it causes a flag of this message when no property is available and there's no error.
Changing this to === false fixes the issue.
There was a problem hiding this comment.
I was wrong about that, the issue is actually that the previously-selected property for an old account isn't reset until the new properties are loaded, meaning there's a time when the property ID is valid while things are still loading.
The trick should be "resetting" the property ID to be undefined when the account changes.
|
I needed to make this change: 5203129 to get everything working and prevent this error: #6816 (review). I've otherwise code-reviewed this PR and it looks good, but I'd appreciate a quick MR to make sure this won't cause any problems I haven't noticed. In my testing with GA4 Reporting feature flag on and off, and settings for all combinations of both/either modules, it worked fine. But it'd be good to get another set of eyes on just this commit: 5203129 |
|
Actually I think this is safe to merge, we can always revert my small change if needed. |
|
@tofumatt Thank you for spotting the edge case, figuring out a fix for it and getting this merged. 🎉 |
Summary
Addresses issue:
Relevant 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