Fixes whole campaign not being fetched #97115

j6ll · 2024-12-05T13:09:46Z

Proposed Changes

When switching to the "whole campaign" it was not re-fetching data
Whole campaign was loading 7 days by default, not the actual campaign dates
Switch data to hourly when the campaign is less than a 3 days old
Switch hourly labelling to include date, since it could now be multi-day

Testing Instructions

Screen.Recording.2024-12-05.at.13.01.11.mov

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-12-05T13:09:59Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-126632

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-126632&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-126632&env=a8c-for-agencies

matticbot · 2024-12-05T13:19:34Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

blaze-dashboard
notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug jamesgill23/fix-whole-campaign-fetching on your sandbox.

matticbot · 2024-12-05T13:20:17Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~6 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
promote-post-i2        +34 B  (+0.0%)       +6 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

j6ll · 2024-12-05T13:34:55Z

client/my-sites/promote-post-i2/components/campaign-item-details/index.tsx

@@ -875,7 +879,7 @@ export default function CampaignItemDetails( props: Props ) {
 														label={ chartSource }
 													/>
 													{ getCampaignStatsChart(
-														campaignStats?.series[ chartSource ] ?? [],
+														campaignStats?.series[ chartSource ] ?? null,


empty array can be a valid response for no data, so I intend to handle these cases differently and so intentionally passing null here

therocket-gr

i couldnt test everything as i dont have a lot of active campaigns (in prod or in local)
I tested the refetching of the data when I switch date ranges and did a thorough code review
everything LGTM

timur987 · 2024-12-06T10:41:10Z

i couldnt test everything as i dont have a lot of active campaigns (in prod or in local)
I tested the refetching of the data when I switch date ranges and did a thorough code review
everything LGTM

Hey @therocket-gr,

just letting you know that I manage having prod data locally with the trick James provided in his earlier PR. Basically, you change the ID of your test campaign to 110111 and run SQL query to populate data copied from production.

Druid has data for that campaign, so you can operate with start_date and end_date values for your local campaign and test short and long campaigns.

The intention of this message is not to ask you to test again (I'm testing it right now) since you've already do CR, but to let you know about an approach how to test similar code changes in the future.

therocket-gr · 2024-12-06T10:44:54Z

i couldnt test everything as i dont have a lot of active campaigns (in prod or in local)
I tested the refetching of the data when I switch date ranges and did a thorough code review
everything LGTM

Hey @therocket-gr,

just letting you know that I manage having prod data locally with the trick James provided in his earlier PR. Basically, you change the ID of your test campaign to 110111 and run SQL query to populate data copied from production.

Druid has data for that campaign, so you can operate with start_date and end_date values for your local campaign and test short and long campaigns.

The intention of this message is not to ask you to test again (I'm testing it right now) since you've already do CR, but to let you know about an approach how to test similar code changes in the future.

thanks for the extra info :)

j6ll · 2024-12-06T10:48:13Z

Yeah, this is a tricky one to test, so whatever you can test plus a code review should be adequate as I provided a demo video to show the behaviour :). The main issue here is that the data is not being re-fetched.

Which was because of this, whole campaign was not calling the same function as the other date options.

timur987

Thanks @j6ll!

The code looks great and works as advertised.

Although I left a comment about a suggestion from VS Code, it's definitely not a blocker to go ahead.

timur987 · 2024-12-06T10:11:08Z

...promote-post-i2/components/campaign-stats-line-chart/index.tsx/campaign-stats-line-chart.tsx

@@ -63,7 +63,7 @@ const CampaignStatsLineChart = ( { data, source, resolution }: GraphProps ) => {

 	const formatDate = ( date: Date, hourly: boolean ) => {
 		const options: Intl.DateTimeFormatOptions = hourly
-			? { hour: 'numeric', minute: 'numeric' }
+			? { month: 'short', day: 'numeric', hour: 'numeric', minute: 'numeric' }
 			: { month: 'short', day: 'numeric' };
 		return new Intl.DateTimeFormat( locale, options ).format( date );
 	};


VS Code suggests to move formatDate function to useMemo hook callback entirely.

Also, I noticed that useMemo's dependencies can start including locale value.

Here's the patch.txt.

Thank you timur, I've got another PR ready for review very soon. I will move it in that PR and ship this one.

Added to #97075

https://github.com/Automattic/wp-calypso/pull/97075/files#diff-aaf45e41c89183f0b29923806184e52ba5160e979fd46ccf11ce6f61e67d56d7R66-R72

https://github.com/Automattic/wp-calypso/pull/97075/files#diff-aaf45e41c89183f0b29923806184e52ba5160e979fd46ccf11ce6f61e67d56d7R184

Fixes whole campaign not being fetched

97e266a

j6ll requested a review from therocket-gr December 5, 2024 13:32

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 5, 2024

j6ll requested a review from timur987 December 5, 2024 13:33

j6ll commented Dec 5, 2024

View reviewed changes

Fixes whole campaign not being fetched

48ead18

therocket-gr approved these changes Dec 6, 2024

View reviewed changes

timur987 assigned j6ll Dec 6, 2024

timur987 approved these changes Dec 6, 2024

View reviewed changes

j6ll merged commit 26a3b5e into trunk Dec 6, 2024
11 checks passed

j6ll deleted the jamesgill23/fix-whole-campaign-fetching branch December 6, 2024 12:51

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes whole campaign not being fetched #97115

Fixes whole campaign not being fetched #97115

j6ll commented Dec 5, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited by matticbot

Loading

matticbot commented Dec 5, 2024

matticbot commented Dec 5, 2024 •

edited

Loading

j6ll Dec 5, 2024 •

edited

Loading

therocket-gr left a comment

timur987 commented Dec 6, 2024

therocket-gr commented Dec 6, 2024

j6ll commented Dec 6, 2024 •

edited

Loading

timur987 left a comment

timur987 Dec 6, 2024

j6ll Dec 6, 2024

j6ll Dec 6, 2024 •

edited

Loading

Fixes whole campaign not being fetched #97115

Fixes whole campaign not being fetched #97115

Conversation

j6ll commented Dec 5, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Dec 5, 2024 • edited by matticbot Loading

matticbot commented Dec 5, 2024

matticbot commented Dec 5, 2024 • edited Loading

j6ll Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

therocket-gr left a comment

Choose a reason for hiding this comment

timur987 commented Dec 6, 2024

therocket-gr commented Dec 6, 2024

j6ll commented Dec 6, 2024 • edited Loading

timur987 left a comment

Choose a reason for hiding this comment

timur987 Dec 6, 2024

Choose a reason for hiding this comment

j6ll Dec 6, 2024

Choose a reason for hiding this comment

j6ll Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

j6ll commented Dec 5, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited by matticbot

Loading

matticbot commented Dec 5, 2024 •

edited

Loading

j6ll Dec 5, 2024 •

edited

Loading

j6ll commented Dec 6, 2024 •

edited

Loading

j6ll Dec 6, 2024 •

edited

Loading