Add module-specific settings CTAs to error report emails. by mehidi258 · Pull Request #13253 · google/site-kit-wp

mehidi258 · 2026-08-04T11:29:54Z

Summary

Related issue(s):

Resolves Combined email template follow ups #12427

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 · 2026-08-04T11:35:29Z

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

📦 Build files for 20fff8a:

Download development build
Download production build

🎭 Playwright reports for 20fff8a:

tofumatt · 2026-08-06T13:15:09Z

+	/**
+	 * Gets template subject mappings that differ from their title.
+	 *
+	 * Only templates whose email subject must diverge from their
+	 * in-email title/heading need an entry here; every other template
+	 * falls back to `get_title()` in `get_subject()`.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @return array Mapping of template names to subject strings.
+	 */
+	protected static function get_all_subjects() {
+		return array(
+			'error-email' => __( 'Action needed: your Site Kit report couldn’t be generated', 'google-site-kit' ),
+		);
+	}


This is oddly named and described, was it done by an LLM by any chance?

"get_all_subjects" implies that this will return all subjects available, but that's not the case. I'd expect this function to be named get_templates_with_custom_subjects.

And the comment/description seems like it would be better written as:

Suggested change

/**

* Gets template subject mappings that differ from their title.

*

* Only templates whose email subject must diverge from their

* in-email title/heading need an entry here; every other template

* falls back to `get_title()` in `get_subject()`.

*

* @since n.e.x.t

*

* @return array Mapping of template names to subject strings.

*/

protected static function get_all_subjects() {

return array(

'error-email' => __( 'Action needed: your Site Kit report couldn’t be generated', 'google-site-kit' ),

);

}

/**

* Get a mapping of all templates with custom subjects (eg. ones

* that differ from their title).

*

* Templates not in this mapping use `get_title()` as their subject.

*

* @since n.e.x.t

*

* @return array Mapping of template names to subject strings.

*/

protected static function get_all_subjects() {

return array(

'error-email' => __( 'Action needed: your Site Kit report couldn’t be generated', 'google-site-kit' ),

);

}

tofumatt · 2026-08-06T13:21:02Z

+	 * Only the four module-specific error keys define a CTA, each
+	 * linking to that module's settings screen; every other key
+	 * (including the generic `error-email` key) defines no CTA.
+	 *


The documentation here will need to be updated every time we change the content here, which seems needless; it's evident from the simple code here what's happening. I think we can omit this. 🙂

Suggested change

* Only the four module-specific error keys define a CTA, each

* linking to that module's settings screen; every other key

* (including the generic `error-email` key) defines no CTA.

*

tofumatt · 2026-08-06T13:21:56Z

+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param string  $content_key Content key (e.g. 'error-email-report-analytics-4').


Isn't this the name of the template? Why use a generic name like "content key"?

tofumatt · 2026-08-06T13:23:47Z

+		$subject = Content_Map::get_subject( 'error-email' );
+		$title   = Content_Map::get_title( 'error-email' );
+
+		$this->assertSame( 'Action needed: your Site Kit report couldn’t be generated', $subject, 'Generic error email subject should match the corrected copy.' );


What do you mean by "corrected copy"? I'm not sure what that means. 🤔

tofumatt · 2026-08-06T13:24:06Z

+		$title   = Content_Map::get_title( 'error-email' );
+
+		$this->assertSame( 'Action needed: your Site Kit report couldn’t be generated', $subject, 'Generic error email subject should match the corrected copy.' );
+		$this->assertSame( 'Email reports are failing to send', $title, 'Generic error email title/heading should remain unchanged.' );


Remain unchanged from what?

tofumatt · 2026-08-06T13:25:06Z

+
+		$this->assertSame( 'Action needed: your Site Kit report couldn’t be generated', $subject, 'Generic error email subject should match the corrected copy.' );
+		$this->assertSame( 'Email reports are failing to send', $title, 'Generic error email title/heading should remain unchanged.' );
+		$this->assertNotSame( $subject, $title, 'Generic error email subject should differ from its title.' );


Arguably this isn't needed given the tests above cover this scenario…

Suggested change

$this->assertNotSame( $subject, $title, 'Generic error email subject should differ from its title.' );

$this->assertNotSame( $subject, $title, 'Error emails should use a custom subject, not the title of the email.' );

tofumatt · 2026-08-06T13:28:21Z

+		$subject = Content_Map::get_subject( $content_key );
+		$title   = Content_Map::get_title( $content_key );
+
+		$this->assertSame( $title, $subject, "Subject for '$content_key' should equal its title." );


Isn't this the default behaviour? What's the value in asserting it for each template?

tofumatt · 2026-08-06T13:29:16Z

+		$cta = Content_Map::get_cta( $content_key, $this->build_golinks() );
+
+		$this->assertSame( 'Request access', $cta['label'], "CTA label for '$content_key' should be 'Request access'." );
+		$this->assertStringContainsString( 'module=' . $module_slug, $cta['url'], "CTA url for '$content_key' should target the $module_slug module settings." );


This line uses a mix of string interpolation and concatenation, let's just use one (interpolation) for consistency/ease-of-reading 😅

tofumatt · 2026-08-06T13:32:40Z

+	/**
+	 * @dataProvider data_report_ctas
+	 */
+	public function test_get_cta_returns_go_to_settings_for_report_keys( $content_key, $module_slug ) {
+		$cta = Content_Map::get_cta( $content_key, $this->build_golinks() );
+
+		$this->assertSame( 'Go to settings', $cta['label'], "CTA label for '$content_key' should be 'Go to settings'." );
+		$this->assertStringContainsString( 'module=' . $module_slug, $cta['url'], "CTA url for '$content_key' should target the $module_slug module settings." );
+	}
+
+	public function data_report_ctas() {
+		return array(
+			'search-console' => array( 'error-email-report-search-console', 'search-console' ),
+			'analytics-4'    => array( 'error-email-report-analytics-4', 'analytics-4' ),
+		);
+	}


These data provider tests are pretty roundabout and harder to read than just placing the content right into the tests. I know we use them elsewhere, but for two lines… I think it'd be just as many lines of code to manually write each test's code and assertions… let's do that instead because it'll be MUCH easier to read and make sense of, which is very valuable in tests.

Tests aren't about "Don't Repeat Yourself", they're about making clear the intention of the code, so I think it's fine (arguably better) if they're a bit verbose in exchange for being more immediately understandable.

Add module-specific settings CTAs to error report emails.

20fff8a

tofumatt requested changes Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

Add module-specific settings CTAs to error report emails. - #13253

Add module-specific settings CTAs to error report emails.#13253
mehidi258 wants to merge 1 commit into
developfrom
enhancement/12427-combined-email-template-follow-ups

mehidi258 commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	$this->assertNotSame( $subject, $title, 'Generic error email subject should differ from its title.' );
	$this->assertNotSame( $subject, $title, 'Error emails should use a custom subject, not the title of the email.' );

Uh oh!

Conversation

mehidi258 commented Aug 4, 2026

Summary

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

2 participants

github-actions Bot commented Aug 4, 2026 •

edited

Loading