Enhance/#7573 - Add `create-custom-dimension` datapoint #7622

hussain-t · 2023-09-24T18:17:42Z

Summary

Addresses issue:

Add datapoints for Analytics custom dimensions #7573

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.

github-actions · 2023-09-24T18:32:45Z

Build files for ecdb3a9 have been deleted.

techanvil

Hi @hussain-t, nice work here so far. I have left a few comments, please take a look.

In addition, with regard to the QAB - which does go into a nice amount of detail in general - it could use a bit of clarification with regard to how to retrieve the Nonce value. Also, the point about replacing the [REPLACE_WITH_COOKIE] placeholder is a little bit confusing, as it mentions including the wordpress_logged_in cookie value, whereas in fact we'd want to include the full wordpress_logged_in key/value pair here. That said, if running the snippet from the devtools of a browser which is logged in to Site Kit, this cookie property can be omitted entirely as the existing cookies will be sent by the browser...

techanvil · 2023-09-25T10:22:34Z

includes/Modules/Analytics_4.php

+					);
+				}
+
+				$custom_dimention_data = $data['customDimension'];


Typo (here and where the variable is used):

Suggested change

$custom_dimention_data = $data['customDimension'];

$custom_dimension_data = $data['customDimension'];

Updated ✅

techanvil · 2023-09-25T10:31:42Z

includes/Modules/Analytics_4.php

+				// If the scope is not set or is not `EVENT`, set it to `EVENT`.
+				if ( empty( $custom_dimention_data['scope'] ) || 'EVENT' !== $custom_dimention_data['scope'] ) {
+					$custom_dimention_data['scope'] = 'EVENT';
+				}


Hmm, I must say I'm not convinced this is the best way to approach the validation of this required property. Silently coercing the invalid value here could lead to unexpected behaviour from the front end perspective when using the endpoint.

Tbh I'd be more inclined to simply check that scope exists and its value is valid for the DimensionScope enum, making this a fairly transparent pass-through to the GA4 endpoint.

Then, we can enforce sending EVENT in the front end, while still getting warned by the back end if we accidentally make a request for something else...

Alternatively, to save the front end having to send EVENT every time, we could default to EVENT in the case where the property is not supplied, but I'd still prefer to avoid coercing a non-EVENT value, for the reasons above.

That's a good point. However, we will be using only the EVENT scope, at least for this phase. Anyways, I will validate against the available enum.

Updated ✅

techanvil · 2023-09-25T10:36:33Z

includes/Modules/Analytics_4.php

+				$custom_dimension->setScope( $custom_dimention_data['scope'] );
+				$custom_dimension->setDisallowAdsPersonalization( $custom_dimention_data['disallowAdsPersonalization'] );
+
+				/* @var Google_Service_GoogleAnalyticsAdmin $analyticsadmin phpcs:ignore Squiz.PHP.CommentedOutCode.Found */


Hmm, I think this has been copy/pasted from similar lines elsewhere in the file, but it doesn't look to me like phpcs:ignore Squiz.PHP.CommentedOutCode.Found is needed here. Could you please confirm this, and if so could you also remove it elsewhere in the file where it's redundantly used in a similar context?

Removed ✅

techanvil · 2023-09-25T10:46:23Z

tests/phpunit/integration/Modules/Analytics_4Test.php

+		// Verify the custom dimension is returned by checking all field values.
+		foreach ( $response_array as $key => $value ) {
+			$this->assertEquals( $value, $response_array[ $key ] );
+		}


This assertion doesn't make sense - you're essentially just comparing $response to itself. What you need to do is add support for the /v1beta/properties/$property_id/customDimensions path to the fake HTTP handler returned by create_fake_http_handler(), returning a mock CustomDimension object from there, and then verify one or more of the expected values from the mock object here.

That's a valid point 👍

Updated ✅

hussain-t · 2023-09-25T13:28:34Z

Hi @hussain-t, nice work here so far. I have left a few comments, please take a look.

In addition, with regard to the QAB - which does go into a nice amount of detail in general - it could use a bit of clarification with regard to how to retrieve the Nonce value. Also, the point about replacing the [REPLACE_WITH_COOKIE] placeholder is a little bit confusing, as it mentions including the wordpress_logged_in cookie value, whereas in fact we'd want to include the full wordpress_logged_in key/value pair here. That said, if running the snippet from the devtools of a browser which is logged in to Site Kit, this cookie property can be omitted entirely as the existing cookies will be sent by the browser...

Thanks, @techanvil. I'm unable to create a request without the cookie. The best way to call the API is to copy a POST fetch call from the network and change the endpoint and the body, as I have mentioned in the QAB.

Update: I have updated the QAB.

techanvil

Hey @hussain-t, thanks for the update to the code and QAB. There's just one last point to address - and, sorry if I was a bit unclear in my previous review, we could have probably saved a round trip. Nearly there, though!

includes/Modules/Analytics_4.php

techanvil

LGTM, nice one @hussain-t!

hussain-t added 3 commits September 24, 2023 23:44

Add create-custom-dimension datapoint and data request.

c8bccf1

Add PHPunit tests for create-custom-dimension datapoint.

bb262b5

Fix PHP lint error.

36aeb0a

hussain-t added 2 commits September 25, 2023 14:53

Update code comment.

6f8631c

Update test comment.

63e50c4

techanvil requested changes Sep 25, 2023

View reviewed changes

hussain-t added 4 commits September 25, 2023 19:11

Fix typo. Validate against the enum.

5e20e7e

Remove unnecesary phpcs:ignore.

9962f60

Update test to mock proper response.

5df8222

Test invalid scope value.

929d712

techanvil requested changes Sep 25, 2023

View reviewed changes

includes/Modules/Analytics_4.php Show resolved Hide resolved

If the scope field is not set, default to .

ecdb3a9

techanvil approved these changes Sep 25, 2023

View reviewed changes

techanvil merged commit 3ecad74 into develop Sep 25, 2023
14 of 17 checks passed

techanvil deleted the enhance/#7573-add-create-custom-dimensions-datapoint branch September 25, 2023 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance/#7573 - Add `create-custom-dimension` datapoint #7622

Enhance/#7573 - Add `create-custom-dimension` datapoint #7622

hussain-t commented Sep 24, 2023 •

edited by techanvil

Loading

github-actions bot commented Sep 24, 2023 •

edited

Loading

techanvil left a comment

techanvil Sep 25, 2023

hussain-t Sep 25, 2023

techanvil Sep 25, 2023

hussain-t Sep 25, 2023 •

edited

Loading

hussain-t Sep 25, 2023

techanvil Sep 25, 2023

hussain-t Sep 25, 2023

techanvil Sep 25, 2023

hussain-t Sep 25, 2023

hussain-t Sep 25, 2023

hussain-t commented Sep 25, 2023 •

edited

Loading

techanvil left a comment

techanvil left a comment

	$custom_dimention_data = $data['customDimension'];
	$custom_dimension_data = $data['customDimension'];

Enhance/#7573 - Add create-custom-dimension datapoint #7622

Enhance/#7573 - Add create-custom-dimension datapoint #7622

Conversation

hussain-t commented Sep 24, 2023 • edited by techanvil Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Sep 24, 2023 • edited Loading

techanvil 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

hussain-t Sep 25, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hussain-t commented Sep 25, 2023 • edited Loading

techanvil left a comment

Choose a reason for hiding this comment

techanvil left a comment

Choose a reason for hiding this comment

Enhance/#7573 - Add `create-custom-dimension` datapoint #7622

Enhance/#7573 - Add `create-custom-dimension` datapoint #7622

hussain-t commented Sep 24, 2023 •

edited by techanvil

Loading

github-actions bot commented Sep 24, 2023 •

edited

Loading

hussain-t Sep 25, 2023 •

edited

Loading

hussain-t commented Sep 25, 2023 •

edited

Loading