Enhancement/7602-parsing-custom-dimensions-report #7697

jimmymadon · 2023-10-13T13:31:24Z

Summary

Addresses issue:

Implement parsing of reports for custom dimensions #7602

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…s-report.

github-actions · 2023-10-13T13:46:53Z

Build files for c7bb05b are ready:

Download development build
Download production build

aaemnnosttv

Thanks @jimmymadon – this is off to a good start. I've left some early feedback for you as requested 👍

includes/Core/Util/WP_Entity_Helpers.php

aaemnnosttv · 2023-10-13T14:00:53Z

includes/Modules/Analytics_4/Report/Response.php

@@ -42,6 +43,8 @@ public function parse_response( Data_Request $data, $response ) {
 			return $response;
 		}

+		$response = $this->parse_custom_dimensions( $response );


I'd suggest a different name for this method since parsing to me is more about extracting one or more values from the input (which is happening here) but more importantly, this is really transforming the response by mapping the IDs to display names. So the response is changing here, but the name doesn't quite reflect that.

aaemnnosttv · 2023-10-13T14:51:39Z

includes/Modules/Analytics_4/Report/Response.php

+		foreach ( $response['rows'] as &$row ) {
+			foreach ( $custom_dimension_map as $dimension_key => $callable ) {
+				$dimension_value = $row['dimensionValues'][ $dimension_key ]->getValue();
+				if ( '(not set)' === $dimension_value ) {


It might be safer to check ! is_numeric here instead.

@aaemnnosttv I actually had that check and then wanted to be more "strict" in preventing any string to filter through. But actually, it makes sense to filter through strings which we can't do anything about.

aaemnnosttv · 2023-10-13T14:58:55Z

includes/Modules/Analytics_4/Report/Response.php

+					continue;
+				}
+
+				$new_dimension_value = call_user_func( $callable, $dimension_value );


This is going to be rather inefficient to call these functions every time for every row that will likely have many duplicates throughout.

Let's update the process to go through the rows and build a list of all of the IDs for each entity without duplicates (i.e. a Set). Then we can map those to their display names, after which we can go through and do the actual replacements in the dimension values all at once rather than doing everything on every iteration.

includes/Modules/Analytics_4/Report/Response.php

aaemnnosttv

Thanks @jimmymadon – this is almost there, just a few points to address 👍

aaemnnosttv · 2023-10-16T19:28:50Z

includes/Modules/Analytics_4/Report/Custom_Dimensions_Response_Parser.php

+		$this->response->setRows( $rows );
+		return $this->response;


This operation mutates the rows of the given response, which is evident in the usage but even the tests don't use the return value.

Let's remove the return to make this more explicit (if nothing is returned, the operation must mutate the given report).

Suggested change

$this->response->setRows( $rows );

return $this->response;

$this->response->setRows( $rows );

(also in @return above)

aaemnnosttv · 2023-10-16T19:29:27Z

includes/Modules/Analytics_4/Report/Custom_Dimensions_Response_Parser.php

+	 */
+	public function swap_custom_dimension_ids_with_names() {
+		if ( $this->response->getRowCount() === 0 ) {
+			return $this->response;


Same here

Suggested change

return $this->response;

return;

aaemnnosttv · 2023-10-16T19:31:50Z

includes/Modules/Analytics_4/Report/Custom_Dimensions_Response_Parser.php

+	public function __construct( Google_Service_AnalyticsData_RunReportResponse $response ) {
+		$this->response = $response;
+	}


This instance is only used in the swap method in which case it could be moved to a dependency of that method rather than the class instance. That way the cache map could be used for multiple responses. It's a minor detail and doesn't need updating right now, just sharing the thought.

I was actually worried about the cache map going stale if multiple responses used it - but this actually makes perfect sense. I also initially thought that I'd need the response in other methods - but ended up keeping them decoupled. I've updated this as it is a quick change.

tests/phpunit/integration/Modules/Analytics_4/Report/Custom_Dimensions_Response_ParserTest.php

includes/Modules/Analytics_4.php

…references.

…s-report.

aaemnnosttv

Thanks @jimmymadon – a few final very minor tweaks I've noted otherwise this is good to go! I'll push these now to avoid another back and forth 👍 Nice work!

aaemnnosttv · 2023-10-16T20:56:26Z

tests/phpunit/integration/Modules/Analytics_4/Report/Custom_Dimensions_Response_ParserTest.php

+		$dimension_header_post_categories->setName( 'customEvent:googlesitekit_post_categories' );
+		$response->setDimensionHeaders( array( $dimension_header_post_categories ) );
+
+		$rows = array();


This variable is unnecessary :)

aaemnnosttv · 2023-10-16T20:57:21Z

includes/Modules/Analytics_4/Report/Custom_Dimensions_Response_Parser.php

+	 * Cache display name results.
+	 *
+	 * @since n.e.x.t
+	 * @var $cache_map


Missing the type :)

aaemnnosttv · 2023-10-16T20:58:32Z

includes/Modules/Analytics_4/Report/Custom_Dimensions_Response_Parser.php

+	 * @param string|int $user_id User ID of the user to get the display name of.
+	 * @return string|int Display name of the user or their original ID if no name is found.
+	 */
+	public function get_post_author_name( $user_id ) {


These don't really need to be public I don't think since they're only called internally. Public methods should have tests though, so let's keep them protected for now.

I actually kept them public so as to add unit tests for them individually. But then ended up creating a sort of larger unit / integration test with the methods. Thanks!

jimmymadon added 6 commits October 13, 2023 14:18

Add custom dimension string constants.

d59ab88

Add helpers to convert user and category IDs to names.

dc956b1

Add unit tests for user and category helpers.

50ba870

Convert custom dimension IDs to names in report response.

aeca2f4

Merge branch 'develop' into enhancement/7602-parsing-custom-dimension…

c72e6f7

…s-report.

Fix potentially existent category in tests.

e5b753f

aaemnnosttv reviewed Oct 13, 2023

View reviewed changes

jimmymadon added 9 commits October 15, 2023 22:24

Generalize entity helpers.

9abf2eb

Fix potential null value in term name.

f2477e0

Extract custom dimensions swapping method into its own class.

07e0e7f

Remove outdated test.

b2966ac

Update references in Response to use new class.

259f120

Add custom dimensions ID to name converter tests.

df7e617

Refactor WP_Entities_Helpers into new parser class.

794f4f2

Remove redundant helper class and methods.

2f0d0e5

Add support for mixed number and string category ID values.

5fe120c

aaemnnosttv requested changes Oct 16, 2023

View reviewed changes

jimmymadon added 4 commits October 16, 2023 21:03

Update return type and dependency of the swap_custom_dimensions method.

3a1b1d9

Update unit tests for dependency changes and remove duplicate object …

5bf0282

…references.

Clarify documentation around the customEvent prefix.

f34d9ca

Merge branch 'develop' into enhancement/7602-parsing-custom-dimension…

12d864c

…s-report.

aaemnnosttv reviewed Oct 16, 2023

View reviewed changes

aaemnnosttv added 4 commits October 16, 2023 17:03

Update doc block.

3380e3f

Fix typo.

93dadc5

Make internal methods protected.

5511c1e

Remove unnecessary variable.

c7bb05b

aaemnnosttv approved these changes Oct 16, 2023

View reviewed changes

aaemnnosttv merged commit 8aff60e into develop Oct 16, 2023
16 of 17 checks passed

aaemnnosttv deleted the enhancement/7602-parsing-custom-dimensions-report branch October 16, 2023 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/7602-parsing-custom-dimensions-report #7697

Enhancement/7602-parsing-custom-dimensions-report #7697

jimmymadon commented Oct 13, 2023

github-actions bot commented Oct 13, 2023 •

edited

Loading

aaemnnosttv left a comment

aaemnnosttv Oct 13, 2023

aaemnnosttv Oct 13, 2023

jimmymadon Oct 13, 2023

aaemnnosttv Oct 13, 2023

aaemnnosttv left a comment

aaemnnosttv Oct 16, 2023

aaemnnosttv Oct 16, 2023

aaemnnosttv Oct 16, 2023

jimmymadon Oct 16, 2023

aaemnnosttv left a comment

aaemnnosttv Oct 16, 2023

aaemnnosttv Oct 16, 2023

aaemnnosttv Oct 16, 2023

jimmymadon Oct 16, 2023

	$this->response->setRows( $rows );
	return $this->response;
	$this->response->setRows( $rows );

Enhancement/7602-parsing-custom-dimensions-report #7697

Enhancement/7602-parsing-custom-dimensions-report #7697

Conversation

jimmymadon commented Oct 13, 2023

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Oct 13, 2023 • edited Loading

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2023 •

edited

Loading