-
Notifications
You must be signed in to change notification settings - Fork 291
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
Implement the secondary user variants of the "no audiences" banner. #8577
Comments
Hey @kuasha420, just letting you know I've picked this one up to add the AC. |
Hi @ankitrox, thanks for drafting this IB. However, while reading it I realise that the AC were a bit misleading, this is my mistake and I do apologise for the confusion. The AC used phrases like "if the view-only user has not yet set up the feature", which I was using as a bit of a shorthand for having not fully set up the feature to the point of having an audience selection. I realise though it was a bit confusing because really, we do want the secondary user setup to have completed before determining which variant of the banner to display. It looks like this has manifested in your IB where for example you specify checking for I've modified the AC to be more clear on these points. Please can you take a look at the changes and update the IB accordingly? |
Hi @ankitrox, thanks for updating the IB. However, there are a few issues with it. One of which is the Furthermore, we don't need to bail out of the Essentially the conditions for showing the specced banner variants should boil down to the following:
Obviously, with the "temporarily hide" behaviour to be fleshed out. One more thing, we try to avoid including code blocks in IBs, the IB should be a higher level description describing the approach to implementation rather than including blocks of code. Of course there are always exceptions to be made, but I don't think this case requires the code blocks. Please can you update the IB again taking the above into account? |
Thank you @techanvil . I've updated the IB. I kept the setting name as |
Thanks @ankitrox! That's looking good. Please note, I've added one point to the IB that was previously missed from this feedback point:
IB ✅ |
Hey @ankitrox, I realised there was another AC issue whereby the authenticated user variant was incorrectly specced to have the temporarily hide button, whereas in fact it should show a Settings link as per this version in Figma. I've updated the AC and also restored the criteria for temporarily hidden audiences to remain hidden until an authenticated user has resynced the audiences. This will align this issue with forthcoming changes to remove the ability to sync audiences from view-only users. I've also tweaked the IB accordingly. |
Whooops, I picked this one up for execution without realising it's not in the current sprint. I've hardly spent any time on it, will leave it assigned in the EB to continue but someone else can pick it up if needs be. |
While working on the execution for this issue, I realised that part of the AC as it stood didn't match up with the GA4 Audiences behaviour. The scenarios where the list of available audiences is empty are not expected to be reachable, due to the fact that the "All Users" default audience cannot be archived, so there will always be at least one audience which the user can select. This is something that came up previously, as discussed here: #8155 (comment). At the time we decided to keep a minor edge case to support the unexpected scenario. However, reviewing this again and re-verifying the Analytics behaviour w/re the "All Users" audience, it doesn't seem worth including support for this scenario which we cannot replicate through any expected behaviour. It would add unnecessary complexity to the code and to testing in various forms. I have therefore amended the AC to remove the proposed banner variants for the "no available audience" scenarios, including the ability to temporarily hide the banner until audiences are available which is obviously no longer relevant. There are still possible variants where the user has not previously had a populated audience selection and I've updated the copy for these as follows: You don't have any visitor groups selected. Select groups. This should suffice to move this issue forward, but I'll check in with @sigal-teller to get her thoughts on this copy amendment, if we want to change it we can in a small followup issue. |
Update: I've asked Sigal for her thoughts in a comment on Figma. |
QA Update
|
Hi @kelvinballoo, yes, sorry for not being more explicit but the second admin should indeed be able to see the connected property. I've updated the AC to make that clear. |
QA Update
|
Hi @kelvinballoo, thanks for spotting that. As you've surmised, it's out of scope for this issue, but is not addressed by another issue so please can you raise one for it? |
QA Update ✅Thanks @techanvil . New issue raised under #9366 The following scenarios were tested good:
Moving ticket to approval. |
Thanks @kelvinballoo! |
Feature Description
Show the secondary user variants of the "no audiences" banner.
See secondary user setup > no audiences in the design doc.See Changes during engineering, 6 Sept 2024 in the design doc.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
includes/Core/User/Audience_Settings.php
, add a settingdidSetAudiences
and default it tofalse
.merge
method when theconfiguredAudiences
is set to array with at least one audience in it anddidSetAudiences
is false, setdidSetAudiences
to true. The value ofdidSetAudiences
should never change once it is set totrue
.assets/js/modules/analytics-4/components/audience-segmentation/dashboard/NoAudienceBannerWidget/NoAudienceBanner.js
.isViewOnly
from theuseViewOnly()
hook.hasAudienceSelection
usinggetConfiguredAudiences
selector.true
ifgetConfiguredAudiences().length > 0
elsefalse
.hasAvailableAudiences
usinggetAvailableAudiences
selector.true
ifgetAvailableAudiences().length > 0
elsefalse
.isViewOnly && ! hasAudienceSelection && ! hasAvailableAudiences
, show the following variant:There are no visitor groups available. Learn more about how to group site visitors in Analytics.
You can temporarily hide this feature until groups are available.
isViewOnly && hasAudienceSelection
:hasAvailableAudiences
, show the following variant:It looks like your visitor groups aren’t available anymore. Select other groups.
! hasAvailableAudiences
, show the following variant:There are no visitor groups available. Learn more about how to group site visitors in Analytics.
You can temporarily hide this feature until groups are available.
! isViewOnly && ! hasAvailableAudiences && ! didSetAudiences
, show the following variant:There are no visitor groups available. Learn more about how to group site visitors in Analytics.
You can deactivate this widget in Settings.
NoAudienceBanner
for examples of the Analytics, Select other groups and Settings links.audience-segmentation-no-audiences-banner
. (Refer to thedismissItem
action here).filterActiveWidgets()
callback.Test coverage
NoAudienceBannerWidget
with empty available audiences, available audiences and based on combination of values fordidSetAudiences
setting.QA Brief
View-only user with an empty audience selection
audienceSegmentation
feature flag enabled, and click on Enable groups to set the feature up.View-only user with an populated audience selection
audienceSegmentation
feature flag enabled, and click on Enable groups to set the feature up.Secondary admin who's never had audiences selected
audienceSegmentation
feature flag enabled, and click on Enable groups to set the feature up.Changelog entry
The text was updated successfully, but these errors were encountered: