site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

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

Open techanvil opened this issue 1 year ago • 1 comments

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

  • [ ] Update includes/Core/Dismissals/REST_Dismissals_Controller.php, create a new rest endpoint:
  • [ ] Update assets/js/googlesitekit/datastore/user/dismissed-items.js:
    • [ ] 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.

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), dispatch the new removeDismissedItem 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

Changelog entry

techanvil avatar Jun 14 '24 15:06 techanvil

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:

https://github.com/google/site-kit-wp/blob/b67e9a1ea124dc8d233def811d9adfb99c995544/assets/js/googlesitekit/modules/datastore/sharing-settings.js#L90-L98

  • [ ] 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 avatar Jul 05 '24 11:07 techanvil

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 avatar Jul 10 '24 10:07 benbowler

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 :white_check_mark:

techanvil avatar Jul 10 '24 14:07 techanvil

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.

https://github.com/user-attachments/assets/174fcedf-838d-4bfe-8650-267d4c97586d

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:

https://github.com/user-attachments/assets/4c9f0bf2-7cd8-4216-bf78-dc20ce0ccf6e


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.

https://github.com/user-attachments/assets/1e1aa34f-57b4-4df5-b5ce-e3e8e736d29a

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

kelvinballoo avatar Aug 07 '24 10:08 kelvinballoo

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 avatar Aug 07 '24 13:08 techanvil

QA Update ✅

Thanks for checking @techanvil . New issue raised under : https://github.com/google/site-kit-wp/issues/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. ✅

https://github.com/user-attachments/assets/1e1aa34f-57b4-4df5-b5ce-e3e8e736d29a

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

kelvinballoo avatar Aug 07 '24 14:08 kelvinballoo