Add GET:report datapoint for GA4. #6379

techanvil · 2023-01-06T12:53:10Z

Summary

Addresses issue:

Add GET:report datapoint for GA4 #6172

Relevant technical choices

Rather than adding test cases for the new protected methods create_analytics_site_data_request and parse_reporting_orderby, which are protected and therefore not part of the public interface and should ideally not be tested directly in a unit test, I have added tests which get the report and check the report parameters have been correctly generated. These are more complete integration-style tests that provides better coverage than isolating these protected methods.

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.

…ing.

The metric `name` is required. If `expression` is provided, `name` works as an alias. Otherwise `name` must be a valid metric name.

Instead these will be enabled on a per-item basis as they are used in the plugin.

tests/phpunit/integration/Modules/Analytics_4Test.php

aaemnnosttv

Thanks @techanvil ! I really appreciate your thorough approach on this one, especially around the tests 👍 I left a few comments, mostly around the tests as well. Overall, everything is quite solid, there are a few points that are important to address and several comments are related to the same thing just given in-place for context.

Great work!

includes/Modules/Analytics_4.php

tests/phpunit/integration/Modules/Analytics_4Test.php

aaemnnosttv · 2023-01-09T23:44:16Z

tests/phpunit/integration/Modules/Analytics_4Test.php

+	public function test_report__metric_validation() {
+		// Enable some metrics.
+		add_filter(
+			'googlesitekit_shareable_analytics_4_metrics',
+			function() {
+				return array(
+					'sessions',
+					'totalUsers',
+				);
+			}
+		);
+
+		$property_id = '123456789';
+
+		$request_handler_calls = array();
+
+		$analytics = $this->setup_report( $property_id, $request_handler_calls, true );


We should also test the relevant behavior for authenticated admins around validation –that non-shareable metrics+dimensions can be requested.

Following our discussion I've taken an approach using a @dataProvider to provide an access token (or not) for each test, with a corresponding call to authenticate the user; I am not entirely sure about it, please take a look and see what you think...

tests/phpunit/integration/Modules/Analytics_4Test.php

includes/Modules/Analytics_4.php

aaemnnosttv

Thanks @techanvil – there were a few more things I noticed to address here but this is looking great. Almost there I think 👍

includes/Modules/Analytics_4.php

aaemnnosttv · 2023-01-11T20:11:33Z

includes/Modules/Analytics_4.php

+		}
+
+		$option      = $this->get_settings()->get();
+		$property_id = self::normalize_property_id( $option['propertyID'] );


If propertyID is empty, we should probably return an error here; as it would be guaranteed to fail

The "normalized version" of the property ID is API specific, so we should probably apply that right before the API call, or even inline it otherwise it might be used somewhere else in the method assuming it is the setting value (which would be a reasonable assumption to make)

Sounds good, I've updated to return an error when propertyID is empty and moved the normalization to the point of use.

I added a test for the empty propertyID scenario. I've also refactored the validation tests slightly as I realised they don't actually need the fake HTTP client as they don't get as far as making a request. As a result I noticed another test scenario to cover and added a test for insufficient permissions.

aaemnnosttv · 2023-01-11T20:21:19Z

includes/Modules/Analytics_4.php

+		} else {
+			$date_range    = $data['dateRange'] ?: 'last-28-days';
+			$date_ranges[] = $this->parse_date_range( $date_range, $data['compareDateRanges'] ? 2 : 1 );
+
+			// When using multiple date ranges, it changes the structure of the response:
+			// Aggregate properties (minimum, maximum, totals) will have an entry per date range.
+			// The rows property will have additional row entries for each date range.
+			if ( ! empty( $data['multiDateRange'] ) ) {
+				$date_ranges[] = $this->parse_date_range( $date_range, 1, 1, true );
+			}
+		}


AFAIK we don't use dateRange, compareDateRanges, or multiDateRange report args anymore – these predate explicit start/end and comparison date range dates which we should be using exclusively for quite a while now. I see these still exist in other modules but given that they're legacy/deprecated, we shouldn't introduce them here. There's probably no harm in leaving these in the main Analytics module for now since the whole thing is going way eventually anyways, but we should open an issue to remove these from the other modules if we don't already have one.

Sounds good, I've removed dateRange, compareDateRanges, and multiDateRange.

I have, though, kept the default case where no date params are provided at all, in which case it defaults to the last 28 days. I've also added a test case as this scenario was missed in the coverage. If you'd prefer to remove this default entirely, please let me know.

Have also created a followup issue to remove these legacy params from the codebase: #6395

includes/Modules/Analytics_4.php

tests/phpunit/integration/Modules/Analytics_4Test.php

aaemnnosttv · 2023-01-11T21:07:40Z

tests/phpunit/integration/Modules/Analytics_4Test.php

+	protected function days_ago_date_string( $days_ago ) {
+		return gmdate( 'Y-m-d', strtotime( $days_ago . ' days ago' ) );
+	}


This probably isn't needed as we shouldn't need to do any date range ID -> dates conversion (as we have in some other modules) as mentioned before. This also makes the test slightly fragile to dates computing differently if the test were to run in a way that the date changed during the run that could cause a failure. Unlikely, but we have seen this kind of thing before in other tests.

I've ended up leaving this in, due to the new default date range test case mentioned above.

I started looking for solutions for mocking the date in PHPUnit but didn't want to go too far down a rabbit hole.

Given the low probability of hitting the edge case you mentioned on a given test run, I'd suggest leaving it in for now and following up with a separate issue to provide a date mocking solution and rewrite the test, as this would also provide useful functionality for other tests. What do you think?

Sure, that sounds good to me. It's very unlikely that the test would fail for this reason, but I agree we should preserve the default date range 👍

Ok, cool. I've created the issue #6407 to follow up on mocking the date in PHPUnit tests.

…perty_id.

… setting.

aaemnnosttv

Thanks for your patience on this @techanvil – this is very close, just a few final details to touch up.

aaemnnosttv · 2023-01-13T17:23:58Z

includes/Modules/Analytics_4.php

+		$option = $this->get_settings()->get();
+
+		if ( empty( $option['propertyID'] ) ) {
+			return new WP_Error( 'missing_required_setting', __( 'No connected Google Analytics 4 property ID.', 'google-site-kit' ), array( 'status' => 400 ) );


A bit of a nitpick here but a 400 response would indicate that the request was malformed in some way that it should not be retried. Since this condition is based on internal state, I think a 500 error would be more appropriate, and this probably makes sense to go after the 4xx errors (i.e. once we determine the request was made correctly).

Good spot! I've done this, and also updated the metric/dimension validation errors to return a 400 rather than the current default of 500. Also updated the tests to check for the status code.

Thanks! As mentioned in #6379 (comment) I don't think we should change the status of the errors for the DS-related validation because that is also dependent on internal state rather than an improperly made request. I've gone ahead and reverted this part.

aaemnnosttv · 2023-01-13T17:37:18Z

tests/phpunit/integration/Modules/Analytics_4Test.php

+	protected function days_ago_date_string( $days_ago ) {
+		return gmdate( 'Y-m-d', strtotime( $days_ago . ' days ago' ) );
+	}


Sure, that sounds good to me. It's very unlikely that the test would fail for this reason, but I agree we should preserve the default date range 👍

tests/phpunit/integration/Modules/Analytics_4Test.php

Verify status codes in tests.

aaemnnosttv

Thanks @techanvil ! Just one thing related to one of the extra changes you made. I'll update this myself to avoid another round here as it should be a trivial change to make, but great job here 👏

aaemnnosttv · 2023-01-13T20:31:07Z

includes/Modules/Analytics_4.php

@@ -926,7 +922,8 @@ function ( $dimension_def ) {
 				} catch ( Invalid_Report_Dimensions_Exception $exception ) {
 					return new WP_Error(
 						'invalid_analytics_4_report_dimensions',
-						$exception->getMessage()
+						$exception->getMessage(),
+						array( 'status' => 400 )


I can see why you used the status here as well but let's not do that because it would imply we should make the same changes in the other modules that also implement this check. Aside from that, I think the default status is okay here because the client is not making an invalid request – the validation is applied due to internal state/conditions related to dashboard sharing.

No worries and thanks for changing it back 👍

aaemnnosttv · 2023-01-13T21:07:18Z

includes/Modules/Analytics_4.php

+		);
+
+		$request = new Google_Service_AnalyticsData_RunReportRequest();
+		$request->setProperty( self::normalize_property_id( $property_id ) );


This seems to be unnecessary but we can look at it in a separate issue as I don't think it's hurting anything here. The property ID is provided as a path parameter in the API request, but the object we're building here is essentially the body of that request so it's being duplicated. It's also not mentioned in the schema for the request body, but is specifiable in the API explorer 😄 🤷‍♂️

Perhaps this is a leftover detail from an earlier version of the API or maybe there for consistency with the batch method (although I think a batch is still limited to the same property). See https://github.com/googleapis/google-api-php-client-services/blob/ff7e67045c666d292e07e0898ce63be458592745/src/AnalyticsData/Resource/Properties.php#L199-L205

Yes, possibly unnecessary, worth taking another look at 👍

aaemnnosttv

Great! Thanks @techanvil 💪

techanvil added 13 commits January 6, 2023 12:37

Add initial Analytics-4 GET:report implementation.

54b16e1

Add metric aggregations to match UA reports.

45e9dde

Update return type.

384d626

Fix typo.

065681e

Add a reminder to remove metrics/dimensions for subsequent allow-list…

ad4f5df

…ing.

Update error codes for GA4.

423a820

Update metric validation.

a0974d6

Update metric parsing to reflect requirement for name.

f56e24d

The metric `name` is required. If `expression` is provided, `name` works as an alias. Otherwise `name` must be a valid metric name.

Update comment.

88a47bc

Fix test.

b9f041d

Add PHPUnit test for GET:report.

695a5ec

Remove metrics and dimensions from their allow-lists.

4c0b1a9

Instead these will be enabled on a per-item basis as they are used in the plugin.

Flesh out GET:report test coverage.

717bf16

techanvil commented Jan 6, 2023

View reviewed changes

tests/phpunit/integration/Modules/Analytics_4Test.php Outdated Show resolved Hide resolved

techanvil added 12 commits January 6, 2023 13:12

Re-order report parameter validation.

c1a6229

Fix metric validation.

857f745

Add tests for report parameter validation.

05db15a

Remove comment.

6e2da29

Update parameter description.

43df5f9

Simplify test slightly.

7ed6d1f

Fix comment.

7056c14

Add missing @param doc line.

2b0bfee

Fix comment alignment.

b714579

Trim unused imports.

98243e0

Extend test coverage to all GET:report input parameters.

b586ad2

Add notes about required/optional parameter usage.

13334bf

aaemnnosttv requested changes Jan 10, 2023

View reviewed changes

techanvil added 3 commits January 10, 2023 13:33

Retrieve property ID from module settings within GET:report handler.

74e7b4d

Use the single-report runReport rather than batchRunReports.

cb54367

Remove unnecessary fake HTTP client.

ef6d6c7

techanvil added 2 commits January 10, 2023 18:06

Add authenticated user variants for all relevant report tests.

15be316

Remove @SInCE from test function.

1be6cb4

aaemnnosttv reviewed Jan 11, 2023

View reviewed changes

techanvil added 11 commits January 12, 2023 11:16

Remove unused import.

e6a969d

Return error if no connected property and move calls to normalize_pro…

eca7c7e

…perty_id.

Don't fake HTTP client for tests which don't need it.

6407fd5

Add test for insufficient permissions.

4093e6f

Add test for default date range.

a61350a

Remove legacy date params.

203fbe0

Use permute_site_hosts rather than mapping result of permute_site_url.

4037108

Reuse setup_user_authentication.

82e3567

Set new admin as owner of Analytics 4 module for test.

732f9be

Set shared roled from current user, and use default management: owner…

70e4d78

… setting.

Extract setting user access token to a new trait.

93a6b27

techanvil force-pushed the enhancement/6172-analytics-4-get-report branch from 1140866 to 93a6b27 Compare January 12, 2023 15:40

aaemnnosttv reviewed Jan 13, 2023

View reviewed changes

aaemnnosttv and others added 4 commits January 13, 2023 12:54

Merge branch 'develop' into enhancement/6172-analytics-4-get-report.

e9c0a41

Change status code to 400 for metrics and dimensions validation errors.

38f8695

Verify status codes in tests.

Change the "no property ID setting" error to be a 500.

80476e8

Move call to set granted scopes into the test cases.

f85fc3e

aaemnnosttv reviewed Jan 13, 2023

View reviewed changes

aaemnnosttv added 4 commits January 13, 2023 15:43

Add missing import.

06d6faf

Remove HTTP status from DS validation.

615f0ad

Require property ID after required params.

a3183f3

Update formatting of returned errors.

01c24e5

aaemnnosttv reviewed Jan 13, 2023

View reviewed changes

aaemnnosttv approved these changes Jan 13, 2023

View reviewed changes

aaemnnosttv merged commit f17c6b0 into develop Jan 13, 2023

aaemnnosttv deleted the enhancement/6172-analytics-4-get-report branch January 13, 2023 21:14

techanvil mentioned this pull request Mar 31, 2023

Enhancement/6623 ga4 widgets real data #6790

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GET:report datapoint for GA4. #6379

Add GET:report datapoint for GA4. #6379

techanvil commented Jan 6, 2023 •

edited

Loading

aaemnnosttv left a comment

aaemnnosttv Jan 9, 2023

techanvil Jan 10, 2023 •

edited

Loading

aaemnnosttv left a comment

aaemnnosttv Jan 11, 2023

techanvil Jan 12, 2023

aaemnnosttv Jan 11, 2023

techanvil Jan 12, 2023

aaemnnosttv Jan 11, 2023

techanvil Jan 12, 2023

aaemnnosttv Jan 13, 2023

techanvil Jan 13, 2023

aaemnnosttv left a comment

aaemnnosttv Jan 13, 2023

techanvil Jan 13, 2023

aaemnnosttv Jan 13, 2023

aaemnnosttv Jan 13, 2023

aaemnnosttv left a comment

aaemnnosttv Jan 13, 2023

techanvil Jan 16, 2023

aaemnnosttv Jan 13, 2023

techanvil Jan 16, 2023

aaemnnosttv left a comment

Add GET:report datapoint for GA4. #6379

Add GET:report datapoint for GA4. #6379

Conversation

techanvil commented Jan 6, 2023 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil Jan 10, 2023 • edited Loading

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

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

aaemnnosttv left a comment

Choose a reason for hiding this comment

techanvil commented Jan 6, 2023 •

edited

Loading

techanvil Jan 10, 2023 •

edited

Loading