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

Move Dimension Filters in Report/Filters Folder

Open zutigrm opened this issue 2 years ago • 6 comments

Feature Description

Metric filter support is added in #7579 , per this comment in CR it is placed in Report/Filters folder. Dimension filters Report/Dimension_Filter should also be moved to the Filters folder together with metric filters, as they can might be used together in the future.


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

Acceptance criteria

  • Move the filter expression classes within the includes/Modules/Analytics_4/Report/Dimension_Filter folder into the includes/Modules/Analytics_4/Report/Filters folder.
  • Ensure naming within each filter expression class is generic, i.e. instead of using metric_name or dimension_name, simply use name as each filter can be used for metrics or dimensions.

Implementation Brief

  • [ ] Move includes/Modules/Analytics_4/Report/Dimension_Filter/Filter.php, to includes/Modules/Analytics_4/Report/Filter/Filter.php:
    • Remove references to "dimension" in the code comments, rename $dimension_name and $dimension_value to $name and $value.
    • Update namespace to Filter.
  • [ ] Move includes/Modules/Analytics_4/Report/Dimension_Filter/In_List_Filter.php to includes/Modules/Analytics_4/Report/Filter/In_List_Filter.php:
    • Remove references to "dimension" in the code comments, rename $dimension_name and $dimension_value to $name and $value.
  • [ ] Move includes/Modules/Analytics_4/Report/Dimension_Filter/String_Filter.php to includes/Modules/Analytics_4/Report/Filter/String_Filter.php:
    • Remove references to "dimension" in the code comments, rename $dimension_name and $dimension_value to $name and $value.
  • [ ] Remove the empty folder includes/Modules/Analytics_4/Report/Dimension_Filter.
  • [ ] Move tests for In_List_Filter and String_Filter into Filter folder and remove the tests/phpunit/integration/Modules/Analytics_4/Report/Dimension_Filter folder.

Test Coverage

  • No new tests required, confirm existing tests for In_List_Filter and String_Filter are working.

QA Brief

Changelog entry

zutigrm avatar Sep 28 '23 13:09 zutigrm

  • Standardise and reorganise the filter expression classes within the includes/Modules/Analytics_4/Report/Dimension_Filter folder into the includes/Modules/Analytics_4/Report/Filters folder.

@zutigrm what do we mean by "standardise"? Can you explain what is an exact outcome of this ticket? Like do we expect all files from includes/Modules/Analytics_4/Report/Dimension_Filter are moved to includes/Modules/Analytics_4/Report/Filters? Then let's say it like that.

eugene-manuilov avatar Sep 26 '24 12:09 eugene-manuilov

@eugene-manuilov Hi, it seems you wanted to tag @jimmymadon since I didn't write the AC. But in regards to the issue purpose, you are correct, that is the idea, moving items from includes/Modules/Analytics_4/Report/Dimension_Filter over to the includes/Modules/Analytics_4/Report/Filters

zutigrm avatar Sep 26 '24 12:09 zutigrm

Ah.. sorry about that @zutigrm. @jimmymadon, let's clearly define what we expect as the result of this task.

eugene-manuilov avatar Sep 26 '24 13:09 eugene-manuilov

@eugene-manuilov Thanks - I've updated the AC. I was just being a bit more generic here mentioning the "aim" of this issue rather than how to achieve this - cause now the AC is equivalent and technical enough to be an IB. But this issue is pretty minor so this is absolutely fine I guess.

jimmymadon avatar Oct 02 '24 20:10 jimmymadon

@jimmymadon the idea in AC to define the outcome state of the plugin that we will reach when this ticket is implemented. In other words, we need to define AC pretending that it is already done making statements, not call to actions.

For example, instead of saying Move the filter expression classes within the X folder into the Y folder., we should better say The filter expression classes are moved from the X folder into the Y folder, and instead of Ensure naming within each filter expression class is generic ... we should say Each filter expression class has a generic name .... In this case, a person who is going to do QA will need to verify all acceptance criteria and confirm that all requirements are met.

I am going to approve AC but in the future it would be nice to use this approach instead of defining our intentions. AC ✔️

eugene-manuilov avatar Oct 03 '24 15:10 eugene-manuilov

IB ✔

eugene-manuilov avatar Oct 15 '24 10:10 eugene-manuilov

QA Update ✅

  • Tested on dev environment.
  • Tested Site kit setup.
  • Done smoke testing.
  • Tested and compared all widgets data on main/dev environment with the latest environment.
  • Verified Audience segmentation widgets data.
  • Verified KMW functionality and metrics data when conversionReporting feature flag was disabled.
  • Verified KMW functionality and metrics data when conversionReporting feature flag was enabled
  • Verified all widgets and metrics data on main environment is matching with latest environment.

mohitwp avatar Feb 19 '25 13:02 mohitwp