google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 287 forks source link

Implement the Audience Creation Notice happy path #8164

Closed techanvil closed 2 months ago

techanvil commented 8 months ago

Feature Description

Implement the happy path for creating an audience via the Audience Creation Notice. This includes adding the notice to the Selection Panel, creating the audiences and showing the success state. It does not include redirecting to OAuth if necessary, this will be implemented separately via https://github.com/google/site-kit-wp/issues/8165.

See audience creation in the design doc.


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

Acceptance criteria

Main notice

Success notice


Implementation Brief

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/constants.js:

Update the AudienceCreationNotice component

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceCreationNotice.js:

Update the AudienceCreationSuccessNotice component

Rendering the Notice components

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/index.js:

Test Coverage

QA Brief

Changelog entry

techanvil commented 6 months ago

I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, will probably affect the AC for this one

techanvil commented 6 months ago

The audience caching aspect of the design doc has been sufficiently finalised, and I've moved this back to AC.

hussain-t commented 3 months ago

Hi @techanvil, If a user creates one of the Site Kit audiences (either "new visitors" or "returning visitors") from the notice, and the available audiences are synced after the creation, should we continue to show the notice for the other audience that hasn't been created yet? Or should the notice not be displayed when one of the audiences is created? I want to ensure we handle this scenario correctly according to the AC.

Thanks for your guidance!

techanvil commented 3 months ago

Hey @hussain-t, if the available audiences are synced after the user creates one of the audiences, the notice for the other should still remain while the panel is still open so they have the opportunity to create the second audience.

Sending it back to you in IB in case this needs an update, please let me know if anything needs further clarification!

hussain-t commented 3 months ago

Thanks for the clarification, @techanvil. I've updated the IB.

techanvil commented 3 months ago

Thanks @hussain-t. I'm unassigning this for now as I hadn't actually picked this up for review, although I'm happy to do so if nobody else gets it first!

hussain-t commented 3 months ago

Sure, thanks @techanvil 👍

techanvil commented 3 months ago

Thanks @hussain-t. I have ended up picking this one up for IBR :)

Before I get into the detail, a high level consideration I have when reviewing this is that, with this being another 30 point issue, I wonder if we should break this down into smaller issues. This should help us improve the estimation and reduce the risk of another issue running over. I'm thinking we could create the two components presentationally in Storybook with an issue for each of them, with a third issue to integrate them with the defined logic. What do you think?


This aside, I have a few relatively minor points to consider:

  • If the notice is dismissed, return null.
    ...
  • If the Site Kit created audiences are available, return null.
    ...
  • Define the condition under which the Audience Creation Notice should be displayed. The notice should be shown if it has not been dismissed and either:
    • The Site Kit created audiences are not available, or
    • One audience has been created, allowing the user to create the second audience.

Can the first two points be removed? They seem a bit confusing in isolation (particularly the second point), when the condition for showing the notice should include both of these sub-conditions in combination as per the third point.

Also, maybe we should be explicit and say "both of the Site Kit created audiences are not available".

  • Descriptions for the audiences will be available in the retrieved configurable audiences.

This seems incorrect, because the audiences won't exist in the list of configurable audiences yet as we haven't created them. Presumably we should be retrieving the values from SITE_KIT_AUDIENCE_DEFINITIONS.

Lastly, a small formatting consideration - the IB is missing the checkboxed points that we have defined as the format to use for them.

hussain-t commented 3 months ago

Thanks for the thoughtful review, @techanvil. Breaking this into smaller, more manageable issues makes a lot of sense. However, given that we’ve already come this far and the IB has been defined, I’m flexible and open to either approach.

If you think proceeding with the current plan would be more efficient, we can do that. Otherwise, as you mentioned above, we can break it into multiple issues. Let me know how you’d like to proceed!

Apart from that, I have updated the IB.

techanvil commented 3 months ago

Thanks @hussain-t. I think splitting the issue is the way to go here.

I realise the IB is already defined - but, it's worth remembering that the guidance is for us to prefer to split an issue if it's estimated at a 30, which can obviously only happen once the initial IB is drafted anyway. So, reorganising into smaller issues at this stage is the expected thing to do with our process anyway. And of course the main thing is it should allow us to be a bit more precise and accurate with the estimates - if when we evaluate these individually we find they add to more than 30 points then we might have saved ourselves from going over the estimate on this as a single issue.

So, I've gone ahead and split this into three issues. As you've already drafted the IB for this one, I've assigned the other two issues to you in IB, I hope that's OK. These issues being https://github.com/google/site-kit-wp/issues/8986 and https://github.com/google/site-kit-wp/issues/8987.

hussain-t commented 3 months ago

Thanks, @techanvil. That makes sense. I appreciate you splitting the issue and assigning the new ones. I'll take care of the IB for the other two issues.

techanvil commented 3 months ago

Thanks @hussain-t! This, and the IBs for the other issues are LGTM.

IB :white_check_mark:

techanvil commented 2 months ago

Noting that the AC has been tweaked to remove the requirement for neither of the SK audiences to exist in order to show the notice.

As discussed with @hussain-t during execution, this would be problematic in the case where the user created one audience but ran into some technical issue (e.g. browser crashing) before they created the second. If we were to no longer show the notice because one audience exists, the user would be blocked from setting up the second audience.

We will instead show the notice if either of the audiences is still available to create, and the notice hasn't been dismissed.

kelvinballoo commented 2 months ago

QA Update: ⚠️

I have one observation and it might not be directly under the scope of this ticket. Let me know if this needs fixing and thus, a new ticket:

These were tested successfully ✅

https://github.com/user-attachments/assets/74ec8982-fd89-403d-87a6-9bbae430f57c

techanvil commented 2 months ago

I have one observation and it might not be directly under the scope of this ticket. Let me know if this needs fixing and thus, a new ticket:

  • If I dismiss the audience creation notice, the 'All Visitors' audience is there and we will have the text 'Additional groups' below it but it's essentially blank. Should we be hiding that text?

    Screenshot 2024-08-01 at 15 20 36

Thanks for flagging this, @kelvinballoo. It is indeed outside the scope of this issue, but it's something we should address.

The "Additional groups" label should not appear when there are no additional groups to select. Please can you create an issue for this?

kelvinballoo commented 2 months ago

QA Update ✅

Thanks for checking @techanvil . I've raised a new ticket here : https://github.com/google/site-kit-wp/issues/9109

As for this ticket, I'm moving it to Approval as these were tested successfully:

https://github.com/user-attachments/assets/74ec8982-fd89-403d-87a6-9bbae430f57c