-
Notifications
You must be signed in to change notification settings - Fork 293
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.
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
Deleted tags still used in source code and available in Site Kit settings #6763
Comments
@techanvil this seems about right to me, would be great if @aaemnnosttv could run through the scenarios as well. |
1 similar comment
@techanvil this seems about right to me, would be great if @aaemnnosttv could run through the scenarios as well. |
Thanks @techanvil – a few points to clarify, but otherwise this looks good 👍
It could also be that the property was shared directly before and the user no longer has access to it. We can't know for sure if the property was deleted or not just because it isn't returned from the API or not. I would suggest we use similar ambiguous language in this case as well. With the Admin API, it is possible to include deleted properties in the response using a
"tracking events" might be confused with event tracking (which is a specific thing), but this is really about tracking anything at all with GA. Google seems to refer to this more generally as "collecting data" so I would suggest we use language which is more consistent with what the user gets by placing the tag.
Let's be a bit more specific as to where the error should be shown.
We need a bit more clarity about this part as to what it's referring to and when this should happen. Is this referring to the saved values or just the client side state? I'm assuming you mean the latter but we should make that a bit more explicit especially since one of these happens in a banner notification and the other is in the settings edit view. Should it happen in response to an action or automatically in the background? |
Thanks @aaemnnosttv, you've raised some good points.
Thanks for pointing this out - I'd missed the case where properties are shared individually, making the account shareable, and then removing the permission from a shared property causes it to be removed from the list. I've updated the AC to make the message more ambiguous. Regarding the
Good spot, I've updated the messages to use "collecting data" rather than "tracking events".
Good shout - I think these errors would be most useful shown above the GA4 dropdown controls, and have updated the AC accordingly.
My initial intent was for the saved values to be cleared, as they are no longer valid. However, your comment has made me reconsider and in fact, I think it would probably be better not to do this, and rather simply flag the error state to the user and update the settings in response to their action. In the banner notification case, this would allow for a potential permission to be restored, and in the settings case, it seems preferable to avoid a background save seeing as the error is displayed along with the controls and the CTA, so the usual flow should be enough to update the settings to a valid set. As a result, I've removed the details about clearing the settings from the AC. |
Thanks @techanvil that all sounds good to me, thanks for updating those parts. The only question I have left is whether or not we think it's a good idea to include wording about a potentially deleted property? Site Kit doesn't allow deleting or restoring properties from our end so perhaps this would be a good candidate to link the learn more to our own documentation which could elaborate on the reasons why something might not be showing up in the dropdown choices any more? Otherwise this looks good to go 👍 |
Thanks @aaemnnosttv. Again, your comment has given me pause for thought. The wording about a potentially deleted property made sense when the message was clear about the property being deleted, but as it's not clear that is the case, it doesn't read so well to include this extra detail. I've updated the message, and I'm not sure a "Learn more" link would be so appropriate any more, as it would be a bit inconsistent and might seem a bit odd to include for this one scenario when there are a lot of other edge cases which don't get the same treatment. I've removed the part about a "Learn more" link, but if you think we should still include one, I'll create a separate issue for it as we'd need to create the support content as well and I don't think we need to get that in before launch. I also noticed the data stream error message still contained the phrase "so it has been removed from Site Kit's Analytics configuration", which is no longer accurate, so I've updated that too. I also updated it to read "is no longer available" to be consistent. |
LGTM, thanks @techanvil 👍 |
LGTM, thanks @techanvil IB ✅ |
QA Update
|
@mohitwp the AC only mentioned adding the notices in edit settings. However you make a good point about adding this to view settings for more visibility of the problem. As this covers the ac and the suggested enhancement is a nice to have, I'll suggest you to create a separate issue for that. Cheers |
QA Update ✅Thank you, @kuasha420. Current results are as per AC, and it mentioned showing notice in edit settings mode so I will create another ticket to show notice under analytics settings view mode.
|
Bug Description
This was identified during Bug Bash by @jamesozzie and @techanvil.
If a tag is deleted at Tag Manager level, it's still available within the Web Data Stream lookup.
This may be something to be addressed at Tag Manager level, or it may time some time before it's no longer available.
Note that SK does switch the "Exclude from Analytics" reference from the Google Tag to the G4 tag, no issues there when checking the site's source code.
Another variation:
When trying deleting the tag, the user may be directed to delete the destination (data stream) from GA.
When you delete the destination data stream, you get the following on the Analytics settings page, with the data stream not displayed in the dropdown, which is also empty.
However, when rather than deleting the individual destination data stream, you delete the property instead, you get the following on the Analytics settings page, with the property not present in the dropdown, which is displayed empty, while the datastream is still present and displayed in the dropdown. I would have thought the datastream should no longer be available at this point.
Steps to reproduce
Screenshots
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
The Google tag sync and mismatch notification should be updated as follows:
The Analytics edit settings page should be updated as follows for GA4:
propertyID
) and web data stream (measurementID
):Implementation Brief
Show notification
Within
assets/js/modules/analytics-4/datastore/properties.js
:isWebDataStreamNotAvailable
boolean flag to state, with a correspondingsetIsWebDataStreamNotAvailable()
action & reducer, andisWebDataStreamNotAvailable()
selector.*syncGoogleTagSettings()
, in the case where there's no container found, setisWebDataStreamNotAvailable
totrue
. Allow the flow to continue sogoogleTagLastSyncedAtMs
is updated, however don't allowhasMismatchedGoogleTagID
to be set.Within new file
assets/js/components/notifications/WebDataStreamNotAvailableNotification.js
, create aWebDataStreamNotAvailableNotification
component.BannerNotification
.GoogleTagIDMismatchNotification
: Your Analytics 4 configuration has changeddismissExpires
to an arbitrary low value to ensure it expires before the banner may next be shown in an hour.Update settings UI
Within
assets/js/modules/analytics/components/settings/GA4SettingsControls.js
:PropertySelect
to retrieve the list of properties for the current Analytics account.WebDataStreamSelect
to retrieve the list of web data streams for the current GA4 property.measurementID
.getWebDataStreams()
selector,ErrorText
component above the GA4 dropdowns, with the following message as per the AC:getProperties()
selector.ErrorText
component above the GA4 dropdowns, with the following message as per the AC:Test Coverage
*syncGoogleTagSettings()
.WebDataStreamNotAvailableNotification
component.SettingsForm
to show the new error messages.QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: