-
Notifications
You must be signed in to change notification settings - Fork 330
Enhancement/11278 worker email sending #11874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Build files for 857fa3e have been deleted. |
…worker-email-sending.
benbowler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @zutigrm, alongside @tofumatt's review, I've reviewed the code and have a few comments inline below to look at.
Also, I noticed that the template preview was broken because of all of the data key/structure changes. Rather than have you get up to speed on the preview setup, I updated this for you and pushed a commit here: 1222b54
I have now tested this with the tester plugin updates as well and we're looking good.
includes/Core/Email_Reporting/templates/email-report/parts/section-conversions-metric-part.php
Outdated
Show resolved
Hide resolved
includes/Core/Email_Reporting/templates/email-report/parts/section-metrics.php
Outdated
Show resolved
Hide resolved
| // If no conversion data is present in payload it means user do not have conversion tracking set up | ||
| // or no data is received yet and we can skip this section. | ||
| if ( empty( $this->payload['total_conversion_events'] ) || ! isset( $this->payload['total_conversion_events'] ) ) { | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should return an empty array otherwise the array_merge above fails with an error if this data is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned by Ben: this needs to return an empty array, not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a check for this section being empty in get_sections, so no error would be raised, but thinking about it not sure why I complicated it with that, since empty array would not require additional guard and is more straightforward
| */ | ||
| protected function get_growth_drivers_section() { | ||
| if ( empty( $this->payload['keywords_ctr_increase'] ) && empty( $this->payload['pages_clicks_increase'] ) ) { | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should return an empty array otherwise the array_merge above fails with an error if this data is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please address this one too.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, no error would be raised, but simplified it with empty array
includes/Core/Email_Reporting/Email_Reporting_Data_Requests.php
Outdated
Show resolved
Hide resolved
tofumatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just a few return values and small things I noticed, though admittedly it's a big PR to take in 😅
| // If no conversion data is present in payload it means user do not have conversion tracking set up | ||
| // or no data is received yet and we can skip this section. | ||
| if ( empty( $this->payload['total_conversion_events'] ) || ! isset( $this->payload['total_conversion_events'] ) ) { | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned by Ben: this needs to return an empty array, not null.
| */ | ||
| protected function get_growth_drivers_section() { | ||
| if ( empty( $this->payload['keywords_ctr_increase'] ) && empty( $this->payload['pages_clicks_increase'] ) ) { | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please address this one too.)
| </td> | ||
| </tr> | ||
| </table> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep newlines at the end of files. 🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is weirder, my VS code stripping last line at random on some files, or linter not detecting it 🤔
| * @param Email_Report_Payload_Processor|null $report_processor Optional. Report processor instance. | ||
| * @param Report_Data_Processor|null $data_processor Optional. Analytics data processor. | ||
| * @param array $audience_display_map Optional. Audience resource => display name map. | ||
| * @param Context|null $context Optional. Plugin context for audience lookup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should line up; not sure why the linter doesn't catch it 😢
| * @param Email_Report_Payload_Processor|null $report_processor Optional. Report processor instance. | |
| * @param Report_Data_Processor|null $data_processor Optional. Analytics data processor. | |
| * @param array $audience_display_map Optional. Audience resource => display name map. | |
| * @param Context|null $context Optional. Plugin context for audience lookup. | |
| * @param Email_Report_Payload_Processor|null $report_processor Optional. Report processor instance. | |
| * @param Report_Data_Processor|null $data_processor Optional. Analytics data processor. | |
| * @param array $audience_display_map Optional. Audience resource => display name map. | |
| * @param Context|null $context Optional. Plugin context for audience lookup. |
| protected function format_value( $value ) { | ||
| if ( ! is_numeric( $value ) ) { | ||
| return $value; | ||
| } | ||
|
|
||
| $number = (float) $value; | ||
| $abs = abs( $number ); | ||
|
|
||
| if ( $abs >= 1000000 ) { | ||
| return round( $number / 1000000, 1 ) . 'M'; | ||
| } | ||
|
|
||
| if ( $abs >= 1000 ) { | ||
| return round( $number / 1000, 1 ) . 'K'; | ||
| } | ||
|
|
||
| return (string) round( $number ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the reports are translated/localisation; isn't this a bit English-specific and not properly localised? 🤔
Additionally: what happens if there are billions in the number? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I updated this to translatable, and added billions abbreviation
…worker-email-sending.
…oogle/site-kit-wp into enhancement/11278-worker-email-sending.
|
Thanks @benbowler and @tofumatt PR updated |
…errors being thrown and us correct context for admin request mocks.
Summary
Addresses issue:
Relevant technical choices
There were more adaptation and adjustment of the pipeline data than anticipated to render all section parts correctly. PR grew quite big, I also refactored several classes for improvement purposes and to simplify them into more leaner classes where possible. All Search Console metrics have been implemented as part of this PR as well.
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist