Email data section part and builder classes #11732

benbowler · 2025-11-05T13:10:42Z

Summary

Addresses issue:

Create email reporting data and section classes #11680

Relevant technical choices

I built this via TDD using example report output for an example report section that includes both Search Console and GA data. As the sections are built out, we will most likely need to add a few more label mappings and confirm all report structures work with the builder.

Once, a slight variation from the IB is that I took a CPT post into the constructor, I did this because the worker should have the post for each email it is sending and pass this through everywhere to ensure each class it runs has the same data regarding the email it is sending.

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 7.4.
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.
Ensure there are no unexpected significant changes to file sizes.

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 · 2025-11-05T13:15:00Z

Build files for 87dfa65 are ready:

Download development build
Download production build

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

includes/Core/Email_Reporting/Email_Log.php

zutigrm · 2025-11-05T13:30:34Z

includes/Core/Email_Reporting/Email_Log.php

+		if ( '' === $value || null === $value ) {
+			return null;
+		}
+
+		if ( is_numeric( $value ) ) {
+			$timestamp = (int) $value;
+			if ( $timestamp <= 0 ) {
+				return null;
+			}
+		} else {
+			$timestamp = strtotime( (string) $value );
+		}
+
+		if ( false === $timestamp ) {
+			return null;
+		}


Also this one, like validate_reference_date to make the method leaner

Lets extract this one into a separate method

zutigrm · 2025-11-05T13:32:08Z

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

+			'totalUsers'     => __( 'Total Visitors', 'google-site-kit' ),
+			'newUsers'       => __( 'New Visitors', 'google-site-kit' ),
+			'returningUsers' => __( 'Returning Visitors', 'google-site-kit' ),
+			'subscribers'    => __( 'Subscribers', 'google-site-kit' ),


Not sure we will have a dimension header with this label (subscribers) 🤔 But I might be wrong

Yep it's here: https://www.figma.com/design/IsjMhBZHT5aMgBENleQoSQ/Proactive-user-engagement?node-id=1080-29600&t=KGOGvuYfrW7rVUxr-0

Yes, but we are localising the dimensionHeaders, there is no dimension subscribers - this is probably going to be a count of contact, submit_lead_form, and generate_lead events, so we will need to make some kind of aliasing for some cases like this, which will no have a dimension, but we need to attach this label to the actual metric value count. Also other labels here:

For example newUsers and returningUsers are also not available dimensions, they are extracted from newVsReturning dimension instead (you can check NewVIsitorsWidget and NewVsReturningWidget). Here is the example response for some dimensions:

These translations/localizations should map 1:1 with dimensionHeaders which will be received in response. So we can probably add some that are obvious from several KMW report options, and then fill up the rest in last issue that connects data with template - it can go over the untranslated headers and map them.

Unless I am getting this wrong (the label we are targeting to translate - if we are making these labels like a static sections instead, but I don;t think that's the case)

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

zutigrm · 2025-11-05T13:36:29Z

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

+				$section = new Email_Report_Data_Section_Part(
+					isset( $section_payload['section_key'] ) ? (string) $section_payload['section_key'] : 'section',
+					isset( $section_payload['title'] ) ? (string) $section_payload['title'] : '',
+					$labels,
+					$values,
+					$trends,
+					$date_range,
+					$this->format_dashboard_link( $module_slug )
+				);


Just a thought, when put like this, this is quite long list of parameters, can be easy to misplace something in the future, perhaps we can accept single config array here, with values keyed - so they can be properly extracted regardless of the order

Do we do this elsewhere in PHP. I know we do in JS but not sure we do in PHP.

I don't recall seeing it, but I don't recall seeing other data type class like this. In this case we are not passing instances as DI, we are passing arguments, 7 of them. It would be safer and more practical to receive them as single config array or an object. This is common approach we can see in WP core as well, for example WP_Query, WP_Widget https://developer.wordpress.org/reference/classes/wp_widget/__construct/, etc

Or tho align more with WP core, we could do something like new Email_Report_Data_Section_Part( $section_key, $section_data )

…rted version.

…email-data-section-classes.

zutigrm

Thanks @benbowler , left you some comments to tidy up the implementation further.

Also, please add QAB it is missing currently

zutigrm · 2025-11-06T20:57:59Z

includes/Core/Email_Reporting/Email_Log.php

+		$normalized = array();
+		$start      = isset( $decoded['startDate'] ) ? self::format_reference_date( $decoded['startDate'] ) : null;
+		if ( $start ) {
+			$normalized['startDate'] = $start;
+		}
+
+		$send = isset( $decoded['sendDate'] ) ? self::format_reference_date( $decoded['sendDate'] ) : null;
+		if ( $send ) {
+			$normalized['endDate'] = $send;
+		}
+
+		$compare_start = isset( $decoded['compareStartDate'] ) ? self::format_reference_date( $decoded['compareStartDate'] ) : null;
+		if ( $compare_start ) {
+			$normalized['compareStartDate'] = $compare_start;
+		}
+
+		$compare_end = isset( $decoded['compareEndDate'] ) ? self::format_reference_date( $decoded['compareEndDate'] ) : null;
+		if ( $compare_end ) {
+			$normalized['compareEndDate'] = $compare_end;
+		}


We can further simplify this repetitive parts, for example:

$keys = array( 'startDate', 'sendDate' => 'endDate', 'compareStartDate', 'compareEndDate', ); $normalized = array(); foreach ( $keys as $key => $alias ) { if ( is_int( $key ) ) { $key = $alias; $alias = $key; } if ( isset( $decoded[ $key ] ) ) { $formatted = self::format_reference_date( $decoded[ $key ] ); if ( null !== $formatted ) { $normalized[ $alias ] = $formatted; } } }

Or skip including sendDate to the array, only handle that one manually after the loop, to make foreach more straightforward, etc. WDYT?

zutigrm · 2025-11-06T20:58:05Z

includes/Core/Email_Reporting/Email_Log.php

+		if ( '' === $value || null === $value ) {
+			return null;
+		}
+
+		if ( is_numeric( $value ) ) {
+			$timestamp = (int) $value;
+			if ( $timestamp <= 0 ) {
+				return null;
+			}
+		} else {
+			$timestamp = strtotime( (string) $value );
+		}
+
+		if ( false === $timestamp ) {
+			return null;
+		}


Lets extract this one into a separate method

zutigrm · 2025-11-06T20:58:11Z

includes/Core/Email_Reporting/Email_Report_Data_Section_Part.php

+		array $labels,
+		array $values,


We usually don't provide type declarations for arguments, for older php compatibility, although we raised the minimal PHP version to 7.4, rest of the codebase doesn't adapt this yet, and it makes them inconsistent with rest of the arguments that do not have type declarations. For consistency we should either add them for each argument or remove them - but considering we do not do this in any other method we should remove it. Checks against these arguments being an arrays is already handled, and argument types are documented in the doc header so we should be covered without the type declarations here

zutigrm · 2025-11-06T20:58:16Z

includes/Core/Email_Reporting/Email_Report_Data_Section_Part.php

+		$section_key = is_string( $section_key ) ? $section_key : '';
+		$title       = is_string( $title ) ? $title : '';
+
+		if ( '' === $section_key ) {
+			throw new InvalidArgumentException( 'section_key must be a non-empty string' );
+		}
+		if ( '' === $title ) {
+			throw new InvalidArgumentException( 'title must be a non-empty string' );
+		}


Perhaps we can merge this into single check, and skip re-declaring the arguments:

Suggested change

$section_key = is_string( $section_key ) ? $section_key : '';

$title = is_string( $title ) ? $title : '';

if ( '' === $section_key ) {

throw new InvalidArgumentException( 'section_key must be a non-empty string' );

}

if ( '' === $title ) {

throw new InvalidArgumentException( 'title must be a non-empty string' );

}

if ( ! is_string( $section_key ) || empty( $section_key ) ) {

throw new InvalidArgumentException( 'section_key must be a non-empty string' );

}

if ( ! is_string( $title ) || empty( $title ) ) {

throw new InvalidArgumentException( 'title must be a non-empty string' );

}

zutigrm · 2025-11-06T20:58:41Z

includes/Core/Email_Reporting/Email_Report_Data_Section_Part.php

+		$this->labels = array_map( 'strval', $labels );
+		$this->values = array_map( 'strval', $values );
+
+		if ( ! is_array( $labels ) ) {
+			throw new InvalidArgumentException( 'labels must be an array' );
+		}
+		if ( ! is_array( $values ) ) {
+			throw new InvalidArgumentException( 'values must be an array' );
+		}


This seems to be logically inverted, mapping is done before knowing the input is valid. array_map() will issue a warning or fatal error if $labels or $values are not arrays, so the validation after the map will never be reached safely.

Suggested change

$this->labels = array_map( 'strval', $labels );

$this->values = array_map( 'strval', $values );

if ( ! is_array( $labels ) ) {

throw new InvalidArgumentException( 'labels must be an array' );

}

if ( ! is_array( $values ) ) {

throw new InvalidArgumentException( 'values must be an array' );

}

if ( ! is_array( $labels ) ) {

throw new InvalidArgumentException( 'labels must be an array' );

}

if ( ! is_array( $values ) ) {

throw new InvalidArgumentException( 'values must be an array' );

}

$this->labels = array_map( 'strval', $labels );

$this->values = array_map( 'strval', $values );

zutigrm · 2025-11-06T21:54:21Z

includes/Core/Email_Reporting/Email_Report_Report_Processor.php

+		if ( ! empty( $current_values ) && ! empty( $comparison_values ) ) {
+			$trends = array();
+			foreach ( $metric_names as $index => $metric_name ) {
+				$current    = isset( $current_values[ $index ] ) ? $current_values[ $index ] : null;
+				$comparison = isset( $comparison_values[ $index ] ) ? $comparison_values[ $index ] : null;
+
+				if ( is_numeric( $current ) && is_numeric( $comparison ) && 0.0 !== (float) $comparison ) {
+					$trends[] = ( (float) $current - (float) $comparison ) / (float) $comparison * 100;
+				} else {
+					$trends[] = null;
+				}
+			}
+		} elseif ( count( $totals ) > 1 ) {
+			$comparison_totals = $totals[1];
+			$trends            = array();
+			foreach ( $metric_names as $metric_name ) {
+				if ( isset( $totals[0][ $metric_name ], $comparison_totals[ $metric_name ] ) && is_numeric( $totals[0][ $metric_name ] ) && is_numeric( $comparison_totals[ $metric_name ] ) && 0.0 !== (float) $comparison_totals[ $metric_name ] ) {
+					$trends[] = ( (float) $totals[0][ $metric_name ] - (float) $comparison_totals[ $metric_name ] ) / (float) $comparison_totals[ $metric_name ] * 100;
+				} else {
+					$trends[] = null;
+				}
+			}
+		}


There is a lot going on here, lets simplify it so logic is easier to follow (and reduce repetition) by extracting the same part to a method, like:

function $compute_trend( $current, $comparison ) { if ( is_numeric( $current ) && is_numeric( $comparison ) && (float) $comparison !== 0.0 ) { return ( (float) $current - (float) $comparison ) / (float) $comparison * 100; } return null; };

Thenreduce the complexity of the rest of the logic, something like:

if ( ! empty( $current_values ) && ! empty( $comparison_values ) ) { foreach ( $metric_names as $i => $name ) { $trends[] = $compute_trend( $current_values[ $i ] ?? null, $comparison_values[ $i ] ?? null ); } } elseif ( count( $totals ) > 1 ) { foreach ( $metric_names as $name ) { $trends[] = $compute_trend( $totals[0][ $name ] ?? null, $totals[1][ $name ] ?? null ); } }

zutigrm · 2025-11-06T22:07:21Z

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

+			'totalUsers'     => __( 'Total Visitors', 'google-site-kit' ),
+			'newUsers'       => __( 'New Visitors', 'google-site-kit' ),
+			'returningUsers' => __( 'Returning Visitors', 'google-site-kit' ),
+			'subscribers'    => __( 'Subscribers', 'google-site-kit' ),


Yes, but we are localising the dimensionHeaders, there is no dimension subscribers - this is probably going to be a count of contact, submit_lead_form, and generate_lead events, so we will need to make some kind of aliasing for some cases like this, which will no have a dimension, but we need to attach this label to the actual metric value count. Also other labels here:

For example newUsers and returningUsers are also not available dimensions, they are extracted from newVsReturning dimension instead (you can check NewVIsitorsWidget and NewVsReturningWidget). Here is the example response for some dimensions:

These translations/localizations should map 1:1 with dimensionHeaders which will be received in response. So we can probably add some that are obvious from several KMW report options, and then fill up the rest in last issue that connects data with template - it can go over the untranslated headers and map them.

Unless I am getting this wrong (the label we are targeting to translate - if we are making these labels like a static sections instead, but I don;t think that's the case)

zutigrm · 2025-11-06T22:15:41Z

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

+				$section = new Email_Report_Data_Section_Part(
+					isset( $section_payload['section_key'] ) ? (string) $section_payload['section_key'] : 'section',
+					isset( $section_payload['title'] ) ? (string) $section_payload['title'] : '',
+					$labels,
+					$values,
+					$trends,
+					$date_range,
+					$this->format_dashboard_link( $module_slug )
+				);


I don't recall seeing it, but I don't recall seeing other data type class like this. In this case we are not passing instances as DI, we are passing arguments, 7 of them. It would be safer and more practical to receive them as single config array or an object. This is common approach we can see in WP core as well, for example WP_Query, WP_Widget https://developer.wordpress.org/reference/classes/wp_widget/__construct/, etc

Or tho align more with WP core, we could do something like new Email_Report_Data_Section_Part( $section_key, $section_data )

zutigrm · 2025-11-06T22:19:57Z

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

+		$output = array();
+		foreach ( $labels as $label ) {
+			$output[] = isset( $this->label_translations[ $label ] ) ? $this->label_translations[ $label ] : (string) $label;
+		}
+		return $output;


Perhaps we can make it leaner with something like:

return array_map( fn( $label ) => $this->label_translations[ $label ] ?? (string) $label, $labels );

zutigrm · 2025-11-06T22:39:12Z

includes/Core/Email_Reporting/Email_Report_Section_Builder.php

+	protected function build_module_section_payloads( $module_key, array $module_payload ) {
+		switch ( $module_key ) {
+			case 'analytics-4':
+				return $this->build_sections_from_analytics_module( $module_payload );
+			case 'search-console':
+				return $this->build_sections_from_search_console_module( $module_payload );
+			default:
+				return array();
+		}
+	}


We are also going to have one section from AdSense module, although the report is not going to be built from making the request to the AdSense module, but GA4 API, as it will be built on count of totalAdRevenue metric. But we will be passing the AdSense as $module_key considering it is listed in expected module sections and will be created for report options config in #11552.

Considering we have only one section for AdSense (at least for now) and is made from GA4 report, maybe we can include same section for it like:

case 'analytics-4': case 'adsense': return $this->build_sections_from_analytics_module( $module_payload ); WDYT?

Create email report section part and builder classes with TDD.

b076bec

benbowler changed the base branch from develop to enhancement/11544-add-email-log-cpt November 5, 2025 13:10

zutigrm reviewed Nov 5, 2025

View reviewed changes

includes/Core/Email_Reporting/Email_Report_Section_Builder.php Outdated Show resolved Hide resolved

zutigrm reviewed Nov 5, 2025

View reviewed changes

includes/Core/Email_Reporting/Email_Report_Section_Builder.php Show resolved Hide resolved

zutigrm reviewed Nov 5, 2025

View reviewed changes

includes/Core/Email_Reporting/Email_Log.php Outdated Show resolved Hide resolved

zutigrm reviewed Nov 5, 2025

View reviewed changes

includes/Core/Email_Reporting/Email_Report_Section_Builder.php Outdated Show resolved Hide resolved

zutigrm reviewed Nov 5, 2025

View reviewed changes

Base automatically changed from enhancement/11544-add-email-log-cpt to develop November 5, 2025 14:09

benbowler added 3 commits November 6, 2025 11:12

Remove function_exists checks for WP functions added before min suppo…

747077f

…rted version.

Refactor report processor to new class and other small improvements.

22e9f26

Merge remote-tracking branch 'origin/develop' into enhancement/11680-…

87dfa65

…email-data-section-classes.

zutigrm requested changes Nov 6, 2025

View reviewed changes

Email data section part and builder classes #11732

Are you sure you want to change the base?

Email data section part and builder classes #11732

Uh oh!

Conversation

benbowler commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zutigrm Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zutigrm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benbowler commented Nov 5, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading

zutigrm Nov 5, 2025 •

edited

Loading

zutigrm left a comment •

edited

Loading