Premium Analytics: Leave Average CPM empty when no ads were served - #52690
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 3 files.
|
5f00bb1 to
473163b
Compare
473163b to
599bc9d
Compare
dognose24
left a comment
There was a problem hiding this comment.
Tested the branch: jp test js packages/premium-analytics (3771 tests) and tsgo --noEmit both pass, and I read the normalizer, the two shared helpers and the hook. The change is right: a CPM of 0 with no impressions is a guard value, not a reading, and nulling it at the data layer is the one place every consumer benefits from.
Two things I checked because the helpers are shared:
toPointsnow keepsnullfor every metric tab, not just CPM. Today only the WordAds normalizer emitsnullinto a data point; the time-series normalizer behind the Traffic chart never does, so this changes nothing there. Worth remembering when a future normalizer starts returningnull.appendTooltipExtrasnow lists a point whose value isnull, and #52523's tooltip reads that as "No data". Good.
One question, not a blocker: when the whole range served no ads, the CPM tab is unavailable, and MetricTabsChart leaves unavailable metrics out of the extras, so the Ads Served and Revenue tooltips drop the CPM row entirely rather than reading "No data for Average CPM" as they do on a single empty day. Intentional? It reads fine either way; just noting the two cases differ.
Nit: the description still says "Retarget to trunk once it merges", but #52523 is in and the base is already trunk.
|
Good question. That's the existing |
|
Good catch, updated the description to drop that line since #52523 already merged. |
|
Also ran it on the local Docker site with a mocked |
Fixes UNI-805
Proposed changes
The Average CPM tab in the Ads chart read a day with no ads served as a measured $0.00 CPM. Today's bucket stays empty until the nightly WordAds run, so every range ending today dropped the line to $0.00, and a range with no ads at all showed a $0.00 headline.
WordPress.com writes a CPM of 0 when nothing was served, as a divide-by-zero guard, so the data layer now marks it missing; the shared chart helpers in
packages/premium-analytics/packages/widgets-toolkitkeep it missing instead of turning it back into 0.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Setup
wordads/statsrow (today) is[ date, 0, 0, 0 ], as WordPress.com returns before the nightly run.Average CPM tab
Ads Served tab
A range with no ads served
Before is the widget's Storybook story with the same empty bucket. Only the last day should differ; the value axis is not part of this change.