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

Clear the "temporarily hidden" state for an audience when it's removed from the audience selection #8877

Closed
10 tasks
techanvil opened this issue Jun 14, 2024 · 6 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jun 14, 2024

Feature Description

If an audience has zero data and has been temporarily hidden, and the audience is subsequently removed from the audience selection, the "temporarily hidden" state should be cleared.


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

Acceptance criteria

  • Given an audience has zero data and has been temporarily hidden:
    • When the audience is removed from the audience selection via the Selection Panel and the selection is saved via clicking on the Save selection CTA, the "temporarily hidden" state of the audience should be cleared.
    • This in turn means next time the audience is added to the selection its tile will be shown as usual.

Implementation Brief

Updates to Dismissed Item Infra

Audience Selection Updates

  • Update assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/Footer.js, within the saveSettings callback
    • If there in not an error:
    • Get all dismissed items with getDismissedItems selector.
    • Use .filter to find any dismissed items where the slug starts with audience-tile-
    • For each of these (if any are found), collect the slugs in an array, and dispatch the new removeDismissedItems action.

Test Coverage

  • Add new test coverage for the new remove functionality in both tests/phpunit/integration/Core/Dismissals/REST_Dismissals_ControllerTest.php and assets/js/googlesitekit/datastore/user/dismissed-items.test.js

QA Brief

Happy path

  1. Setup the Audience Segmentation feature with at least two audiences selected, one or more of which should be in the "partial data" state with zero data, thus showing the zero data tile variant with the Temporarily hide CTA. Refer to the QAB for Implement the zero data state of an Audience Tile #8143 for more details on this setup.
  2. Click on Temporarily hide to hide the audiences.
  3. Open the Selection Panel, and remove one or more of the hidden audiences from the selection.
  4. Save the selection, then re-add the audiences to the selection and save it again.
  5. The previously hidden audiences which were removed and re-added should reappear in the list of Audience Tiles.

Unhappy path

  • Follow steps 1-3 above.
  • Using the Tweak Chrome extension, create a rule as follows (feel free to test with different error types):
    • URL: .*/wp-json/google-site-kit/v1/core/user/data/dismissed-items.*
    • Enable the use of regular expressions (.*)
    • HTTP Method: POST
    • Status code: 500
    • Response payload:
      {
        "code": "internal_server_error",
        "message": "Internal server error",
        "data": {
          "status": 500
        }
      }
  • Save the selection. The error message should appear in the footer of the Selection Panel:
    image
  • Disable the Tweak rule and re-save the selection. The error message should disappear.

Changelog entry

  • Ensure the “temporarily hidden” state of an audience with zero data is cleared when the audience is removed from the selection.
@techanvil techanvil added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jun 14, 2024
@ivonac4 ivonac4 added the Team M Issues for Squad 2 label Jun 17, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler Jul 2, 2024
@techanvil techanvil self-assigned this Jul 5, 2024
@techanvil
Copy link
Collaborator Author

Hi @benbowler, thanks for drafting this IB. A couple of points:

  • Create a new action called removeDismissedItem, which takes a slug and resolves using a new fetch store, which uses API.delete within it's controllerCallback to delete the requested dismissed item key.

We don't actually have an API.delete() function at present. We could add one, but the only current use of the HTTP DELETE method is implemented using API.set() as follows:

controlCallback: ( {} ) => {
return API.set(
'core',
'modules',
'sharing-settings',
{},
{ method: 'DELETE' }
);
},

  • For each of these (if any are found), dispatch the new removeDismissedItem action.

This point makes me think that, seeing as we're adding a new REST endpoint for the deletion, we might as well allow that endpoint to handle multiple items so we can batch these into a single request. WDYT?

@techanvil techanvil assigned benbowler and unassigned techanvil Jul 5, 2024
@benbowler
Copy link
Collaborator

Thanks @techanvil, I've updated this to use API.set and to accept and array of slugs and delete them all to reduce the number of REST requests.

@benbowler benbowler assigned techanvil and unassigned benbowler Jul 10, 2024
@techanvil
Copy link
Collaborator Author

techanvil commented Jul 10, 2024

Thanks @benbowler! IB LGTM. I've bumped the estimate up a level to allow for extra time modelling the scenarios in testing, and because we've generally been underestimating Audience Segmentation issues.

IB ✅

@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Aug 7, 2024

QA Update ⚠️

The tests on desktop are fine. I did one round of testing on mobile and there are 2 issues to flag:

ITEM 1:
After I hide the temporarily hide for one tile, all the tiles vanish or rather are collapsed I would say.
When I click on the group that is there, it will appear.
Question would then be, once I hide one of the tiles, should we auto-reselect the existing audience rather than having to click on it? ⚠️

Refer around 5th second.
RPReplay_Final1723027160.MP4

ITEM 2:

I had 3 tiles setup and somehow hit an error on mobile when I clicked on one of the tabs. ❌

Refer around the 17th second thereon:
RPReplay_Final1723027160.MP4

Other than that, I tested the happy and unhappy scenarios on desktop and it's working as expected. ✅

Happy path:
The previously hidden audiences which were removed and re-added reappeared in the list of Audience Tiles.

temporarily.hide.working.as.expected.mov

Unhappy path:
The error will be showing up when the tweak extension is activated and once disabled, it works as expected too.

Screenshot 2024-08-07 at 14 28 39

@techanvil
Copy link
Collaborator Author

Hey @kelvinballoo! Thanks for flagging these defects. They are both valid concerns, but are outside the scope of this issue. Please can you raise issues for them?

@techanvil techanvil removed their assignment Aug 7, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks for checking @techanvil .
New issue raised under : #9168

As for this issue, it's good to go to Approval.
I tested the happy and unhappy scenarios on desktop and it's working as expected.

Happy path:
The previously hidden audiences which were removed and re-added reappeared in the list of Audience Tiles. ✅

temporarily.hide.working.as.expected.mov

Unhappy path:
The error will be showing up when the tweak extension is activated and once disabled, it works as expected too. ✅

Screenshot 2024-08-07 at 14 28 39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues 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

6 participants