Skip to content
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 setup #8130

Open
14 tasks
techanvil opened this issue Jan 24, 2024 · 12 comments
Open
14 tasks

Implement the secondary user setup #8130

techanvil opened this issue Jan 24, 2024 · 12 comments
Labels
P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2024

Feature Description

Bootstrap the audience selection for secondary users (secondary admins and view-only users).

This involves reusing the relevant subset of logic defined for the initial setup, showing a loading state and handling errors.

This issue does not include the secondary user variants of the "no audiences" banner, these will be implemented separately via #8577.

See secondary user setup in the design doc.


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

Acceptance criteria

  • Given that the Audience Segmentation feature has already been set up by a primary admin:
    • When a secondary user (i.e. a secondary admin, or a view-only user) visits the dashboard with the Traffic section in view, and they don't yet have an audience selection configured, the Audiences Widget Area should be displayed with two Audience Tiles present in a loading state (see Implement the loading state for the Audience Tile #8145).
    • If the cached list of audiences is stale (i.e. an hour or more has passed since the last sync), it should be resynced (see Add REST and datastore APIs for audience caching #8486).
    • The user's audience selection should be populated according to the rules defined in audience prioritization for selection in the design doc:
      • The selection should be populated to a maximum of two audiences.
      • Any existing user-created audiences should be added first, ordered by user count over the past 90 days. Only user-created audiences with a non-zero user count should be considered for inclusion.
      • Following this, the Site Kit-created “new visitors” and “returning visitors” audiences should be added, if they exist, in that order.
      • Lastly, the “Purchasers” default audience should be added if it’s “active” i.e. it’s ever had any users and it hasn’t been archived.
    • If an error occurs while resyncing the cached list of audiences, or retrieving the audience user counts, the Full Width Error Banner should be displayed instead of the set of audience tiles (see Add the Full Width Error Banner (Storybook) #8230), in the appropriate state (either the "insufficient permissions" state or the generic error state).
      • The error banner's Retry button should reattempt the failed request(s) and, upon success, proceed to populate the selection as defined above.
    • Once the user's audience selection has been successfully populated the Audiences Widget Area should display Audience Tiles in the loaded state for the selection.

Implementation Brief

Determining if the feature has already been setup

  1. When the primary admin sets up the feature i.e. configures the audiences, save a flag audienceSegmentationSetupComplete in the Analytics settings googlesitekit_analytics-4_settings. Default it to false in get_default function in Analytics module.

  2. Add the setting audienceSegmentationSetupComplete in get_view_only_keys in Analytics module here.

Resyncing the cached audiences

  1. Check when was the last time the audiences were synced. Note that the timestamp saved there is Unix timestamp in PHP, so it needs to be multiplied by 1000.

  2. Timestamp is saved in the Google Analytics settings under the key availableAudiencesLastSyncedAt, so we can check if the current timestamp greater than the saved one by an hour or more, if it has passed, then we should re-sync the audiences using syncAvailableAudiences selector.

    googlesitekit.data.dispatch( 'modules/analytics-4' ).syncAvailableAudiences();
  1. POST:sync-audiences endpoint needs to be made shareable in order to be able to call it from the view-only dashboard.

Secondary user journey flow

  • In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js

  • Refer to the enableAudienceGroup action for an existing block of code that handles the initial part of the setup, to configure the audiences for the secondary user according to total users.

  • Refactor this section of enableAudienceGroup action, which gets and sorts the audiences according to the total users in them. This can be extracted into a separate action, let's say retrieveInitialAudienceSelection.

    • Create/extract a new action enableSecondaryUserAudienceGroup which dispatches the extracted action and runs the other logic for the setup (retrieveInitialAudienceSelection action).
      • enableSecondaryUserAudienceGroup should also perform the syncing audiences inside it. This will help us to collect the error at the single point and return { response, error } object from a single place which will help us in handling the errors as mentioned in below section.
      • Check if we have two audiences in response received from retrieveInitialAudienceSelection action, if there is still a place, check if Purchasers audience is available and has had any users. This can be checked with getResourceDataAvailabilityDate selector from partial data module.
      • Note that we should not add "All Users" default audience to configuredAudiences.
    • After completing the above operation of collecting audiences according to the priority, set these audiences as configured audiences for the user using setConfiguredAudiences action from "audience-settings" data store and save the setting using saveAudienceSettings action.
  • In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js

    • Check if the feature has already been setup and if the configured audiences are not available for the user, dispatch an action enableSecondaryUserAudienceGroup.
    • AudienceTilesWidget component will be updated significantly after implementation of Implement the “no audiences” banner happy path view #8155, please refer IB for the same.

Note: As the audiences will be synced from within enableSecondaryUserAudienceGroup action, those won't be re-synced when we display Audience tiles the loading states. Please refer #8145 acceptance criteria.

Handling errors

  • enableSecondaryUserAudienceGroup action will return an object of the form, so that we can determine if there is any error. If error occurs, AudienceSegmentationErrorWidget should be rendered.
  { response, error }
  • AudienceSegmentationErrorWidget will accept a new prop onRetry which needs to be passed to ReportErrorActions, so that it should be able to re-run the setup logic after clicking "Retry" button.

Test coverage

  1. AudienceTilesWidget - Add the stories for the state where secondary admin is visiting for the first time.
    a. Loading state for tiles.
    b. Error states for the tiles.
    e. Single tile.

  2. Add a new test for AudienceSegmentationErrorWidget to test the new prop onRetry.

  3. Add tests for retrieveInitialAudienceSelection action.

  4. Add tests for enableSecondaryUserAudienceGroup action.

QA Brief

Changelog entry

@techanvil techanvil added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jan 24, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Feb 5, 2024
@techanvil techanvil changed the title Add two tier settings infrastructure Implement the secondary user setup Feb 6, 2024
@techanvil
Copy link
Collaborator Author

Note that we have some UX considerations to resolve before this can move forward to AC, as per this thread in Figma.

@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Feb 16, 2024
@techanvil
Copy link
Collaborator Author

The UX considerations have been resolved, and I've moved this forward to AC.

@techanvil
Copy link
Collaborator Author

Hi @ankitrox, thanks for the IB. Here are a few points:

  1. When the primary admin sets up the feature i.e. configures the audiences, save a flag audienceSegmentationSetupComplete in the analytics settings googlesitekit_analytics-4_settings. Default it to 0 in get_default function in analytics module.
  • This should be a boolean, let's default it to false.

  1. Timestamp is saved in the google analytics settings under the key availableAudiencesLastSyncedAt, so we can check if the current timestamp greater than the saved one ( availableAudiencesLastSyncedAt * 1000 ), then we should re-sync the audiences using syncAvailableAudiences selector.
  • I realise the AC wasn't so clear as to what it means for the cached list to be stale, so I've amended it. Basically we only want to resync the cached audiences if an hour or more has passed since the last sync.
  • It's worth noting we'll need to make the POST:sync-audiences endpoint shareable in order to be able to call it from the view-only dashboard.

  • In assets/js/modules/analytics-4/index.js, for analyticsAudienceTiles in isActive callback, instead of checking for the configured audiences, check if the feature is already setup as mentioned above. We can create a selector isFeatureSetup in audience-settings module which should return a boolean.
  • We don't need to create a new isFeatureSetup(), selector, we'll have the auto-generated getAudienceSegmentationSetupCompleted() selector as a result of adding the audienceSegmentationSetupCompleted Analytics setting.

  • For the rest of the logic under Secondary user journey flow - please note that most of it is already implemented in the enableAudienceGroup() action (this section of it). We should be able to refactor and extract the linked section to a reusable action. This will cover the user and Site Kit-created audiences, with only the Purchasers audience needing to be handled with some additional code.
    • Please also note that the error handling isn't yet implemented in enableAudienceGroup() at the time of writing; this is coming via Implement the unhappy paths for the Setup CTA Banner #8134. As can be seen in the IB for that issue we'll be returning an object with an error property from the action in each of the identified error cases.
  • We also need to reconsider the Handling errors section. It's not going to be sufficient to render AudienceSegmentationErrorWidget and let it handle the retry logic as it stands. The current implementation of AudienceSegmentationErrorWidget is a bit limited in its error handling and will only retry report errors. We actually want to re-run the setup logic when pressing Retry, so we'll need to extend AudienceSegmentationErrorWidget to support a retry handler that is passed as a prop.
  • Last but not least, please can you ensure correct capitalisation for names and acronyms, e.g. it should be "Google Analytics", "Unix" or "UNIX", "PHP", etc.

@techanvil techanvil assigned ankitrox and unassigned techanvil Jun 3, 2024
@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jun 4, 2024
@techanvil
Copy link
Collaborator Author

Thanks for the update to the IB @ankitrox. A few more comments:

  • I didn't spot this the first time round, but the IB doesn't cover the AC point about showing two audience tiles in the loading state while setting up the secondary user's audiences.
  • It might also be worth thinking a bit more about how we manage the existing data retrieval within the AudienceTilesWidget component. Presumably that's the entry point for the setup logic here; we'll want to prevent most of the existing calls to getReport() (and getPivotReport() when the pivot refactor is done) from firing while we perform the setup, so we aren't making unwanted API requests. A prosaic approach could simply be to return early from their useSelect() calls while configuredAudiences is not an array; this might be sufficient, or we could consider extracting all of the get*Report() calls into a new selector, this would help to simplify the component.
  • A minor point but I'd suggest "Invalidating the cached audience" should be worded as "Resyncing the cached audiences".

Add the setting audienceSegmentationSetupComplete in get_view_only_keys in analytics module, so that it should not get overwritten.

  • Please note, get_view_only_keys specifies the settings which are made visible on the view-only dashboard. It shouldn't be interpreted as the setting being read-only, so please remove the point about it not getting overwritten.

  • Make use of enableAudienceGroup action to configure the audiences for the secondary user according to total users.
  • Not sure if the wording here is a little unclear, or if this is simply a mistake - "make use of" sounds like we should be dispatching the action. I'd suggest "refer to the enableAudienceGroup action for an existing block of code that handles the initial part of the setup" or something like that.

  • Create/extract a new action which uses this section to sort and get the audience according to total users in them. Note that we may have to take care of the date range as currently it is taking 90 days in consideration from the reference date.
  • Sounds like a bit of confusion here - we don't need to take the current date range into consideration, as we always check the user count over the last 90 days regardless of what the current range is. This is as per the spec (both in the AC and in the design doc)
  • We might as well be a bit more explicit and specify that we create a new action called, say, enableSecondaryUserAudienceGroup which dispatches the extracted action and runs the other logic for the setup. We can include syncing the cached audiences in this action as well. This will mean, when it comes to error handling we simply need to check for an error being returned from enableSecondaryUserAudienceGroup which should simplify things a bit.

  • Regarding the Handling errors section:
    • If we are wrapping the setup up in the enableSecondaryUserAudienceGroup action we only need to check if an error is returned from the action rather than needing to check for the various errors described here.
    • I'd suggest naming the new prop onRetry rather than retryHandler for consistency with elsewhere in the codebase.
    • On a more general note - I had imagined we'd register AudienceSegmentationErrorWidget as a separate widget. The IB seems to suggest we'll simply render it inline - presumably in AudienceTilesWidget, although it's not specific about this. I would like to see it as its own widget, but we'd need to think about how to manage the state to ensure the widget is active and has access to the relevant errors, and how to trigger additional retry logic. We could potentially look at using the error store to manage this, calling receiveError when an error is returned by enableSecondaryUserAudienceGroup and using the presence of this error in the isActive callback for the error widget to determine whether to show the widget. Then we could clear the error in onRetry and use this to trigger the retry logic back in the tile. Anyhow - that all said, I don't want to overcomplicate things here so I think it's OK to render inline for now but we might want to look at a refactor to extract it to its own widget later. Just wanted to capture these thoughts and get you thinking along these lines too.
  • Another minor one, you've missed the capitalisation of Analytics in a few places.
  • Lastly, once you're ready for it, please add an estimate :)

@techanvil techanvil assigned ankitrox and unassigned techanvil Jun 6, 2024
@ankitrox
Copy link
Collaborator

@techanvil Thank you for reviewing the IB.

It might also be worth thinking a bit more about how we manage the existing data retrieval within the AudienceTilesWidget component. Presumably that's the entry point for the setup logic here; we'll want to prevent most of the existing calls to getReport() (and getPivotReport() when the pivot refactor is done) from firing while we perform the setup, so we aren't making unwanted API requests. A prosaic approach could simply be to return early from their useSelect() calls while configuredAudiences is not an array; this might be sufficient, or we could consider extracting all of the get*Report() calls into a new selector, this would help to simplify the component.

I think this is exactly what we will be doing in #8155 . We will split the AudienceTilesWidget into two components.

  1. AudienceTilesWidget - Responsible to check whether audiences are present and return the appropriate component from within it.
  2. AudienceTiles - Responsible to query the report for audiences, build metrics and render AudienceTile component. This will be mostly like the current implementation of AudienceTilesWidget.

On a more general note - I had imagined we'd register AudienceSegmentationErrorWidget as a separate widget. The IB seems to suggest we'll simply render it inline - presumably in AudienceTilesWidget, although it's not specific about this. I would like to see it as its own widget, but we'd need to think about how to manage the state to ensure the widget is active and has access to the relevant errors, and how to trigger additional retry logic. We could potentially look at using the error store to manage this, calling receiveError when an error is returned by enableSecondaryUserAudienceGroup and using the presence of this error in the isActive callback for the error widget to determine whether to show the widget. Then we could clear the error in onRetry and use this to trigger the retry logic back in the tile. Anyhow - that all said, I don't want to overcomplicate things here so I think it's OK to render inline for now but we might want to look at a refactor to extract it to its own widget later. Just wanted to capture these thoughts and get you thinking along these lines too.

IMO, this is a great idea and I think this is the correct way to do it, but the only question is we will need some way of passing the onRetry prop to the AudienceSegmentationErrorWidget component. As of now, I have kept the error component inline, we can register a separate widget for it once we figure out a way to pass/call onRetry. For errors, we can use the error store as you suggested.

I am moving this to IBR for your review by keeping the error handling component inline as of now.

Thanks again!

@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jun 11, 2024
@techanvil
Copy link
Collaborator Author

Hi @ankitrox, thanks for the IB update. A few more points:

  • I'd suggest renaming the suggested sortAvailableAudiences to setInitialAudienceSelection or setAudienceSelectionFromNonDefaultAudiences, or something along those lines. It's doing more than just a sort.
  • Did you mean configuredAudiences rather than collectedAudiences?

  • This point doesn't cover the AC - the requirement is to show two tiles in a loading state while the audience selection is being determined. This is described a bit more explicitly in the design doc:

The audiences widget area will display two tiles in the loading state while the audience selection is being determined in anticipation of what should be the most common happy path journey.



c. User created audience in tiles.
d. One user created and other one from site kit created.

  • I am not sure there's much value in adding both of stories - the VRTs are primarily designed for capturing the visual side of things rather than for testing logic, and the presentational aspect of these tiles is already captured for the most part. There's some marginal value in displaying one of the stories, for the sake of rendering a user created audience (which has a distinct tooltip), but I don't think we need both.

  • Last but not least, I would say the estimate looks a bit on the low side, particularly taking testing into account. This could easily be a 19 if not a 30 once it's fully fleshed out.

@techanvil techanvil assigned ankitrox and unassigned techanvil Jun 12, 2024
@ankitrox
Copy link
Collaborator

Hi @techanvil ,

Thanks for the review.

I'd suggest renaming the suggested sortAvailableAudiences to setInitialAudienceSelection or setAudienceSelectionFromNonDefaultAudiences, or something along those lines. It's doing more than just a sort.

I have updated the action to setAudienceSelectionFromNonDefaultAudiences as suggested.

Did you mean configuredAudiences rather than collectedAudiences?

Thanks, corrected it to configuredAudiences which refers to saved selection.

This point doesn't cover the AC - the requirement is to show two tiles in a loading state while the audience selection is being determined. This is described a bit more explicitly in the design doc:

Removed this one and amended the IB (first point under "Secondary user journey flow").

It's a bit unclear what this is referring to - what aspect of the update is relevant to this issue? Also, does this imply that #8155 should be made a dependency of this issue?

Yes that's correct, we will need to add #8155 as a dependency for this issue. Added it. This is because AudienceTilesWidget component will be refactored into several small components to save on querying reports API when it is not required.

I am not sure there's much value in adding both of stories - the VRTs are primarily designed for capturing the visual side of things rather than for testing logic, and the presentational aspect of these tiles is already captured for the most part. There's some marginal value in displaying one of the stories, for the sake of rendering a user created audience (which has a distinct tooltip), but I don't think we need both.

Removed both of these points.

Last but not least, I would say the estimate looks a bit on the low side, particularly taking testing into account. This could easily be a 19 if not a 30 once it's fully fleshed out.

I agree with this. I have raised the estimate to 19, but I still feel that this should be more than 19 if not upto 30, but we do not have any pointer for this, so keeping this as 19.

As the IB for #8145 is still pending, I have mentioned in the "loading state" point that we may have to pass appropriate props to AudienceTile to display two tiles in loading state. This can further be expanded when we have IB for #8145 ready.

Assigning this to you for review.

Thanks again!

@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jun 13, 2024
@techanvil
Copy link
Collaborator Author

Thanks @ankitrox - however, the IB hasn't been updated, maybe there was an issue when you tried to save it?

@techanvil techanvil assigned ankitrox and unassigned techanvil Jun 13, 2024
@ankitrox
Copy link
Collaborator

Sorry @techanvil , I think it was issue from Zenhub that it did not saved changes.

I have updated it again, please let me know if you can see the updated IB.

Thanks

@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jun 13, 2024
@techanvil
Copy link
Collaborator Author

Thanks @ankitrox! Just one last thing from me and I think this is GTG.

  • Check if we have two audiences in response received from setAudienceSelectionFromNonDefaultAudiences action, if there is still a place, check if Purchasers audience is available and has had any users. This can be checked with getResourceDataAvailabilityDate selector from partial data module.

Maybe I am being overly pedantic, but it feels a little wrong to be dispatching a set action but returning the items we're setting in a response from the action. I would prefer for the action to actually set configuredAudiences (but of course not save it).

An alternative would be to rename the action getAudienceSelectionFromNonDefaultAudiences, but this would be confusing as it looks like a selector name.

What do you think?

@techanvil techanvil assigned ankitrox and unassigned techanvil Jun 13, 2024
@ankitrox
Copy link
Collaborator

@techanvil I completely understood your concern and also feel like its a valid one. I have renamed the action to retrieveInitialAudienceSelection as it does not include get/set. Another alternative would be to rename it to fetchInitialAudienceSelection

Let me know your thoughts.

Thanks

@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jun 13, 2024
@techanvil
Copy link
Collaborator Author

Thanks @ankitrox. Let's go with retrieveInitialAudienceSelection() and move this to the EB!

IB ✅

@techanvil techanvil removed their assignment Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants