Odyssey Stats: Fix redirection with query string for custom date range #97658

dognose24 · 2024-12-19T17:02:20Z

Follow-up of fixing redirection on Odyssey Stats from #97591.

Proposed Changes

Refactor the appendQueryStringForRedirection function to append the query string for redirection on Odyssey Stats.
Fix applying stored period and date range shortcut on Odyssey Stats.

Why are these changes being made?

The custom date range is only available on context.query rather than window.location.search for Odyssey Stats. So, the redirection that happens on Odyssey Stats may need an extra query object parsing process.

Testing Instructions

Spin this change up with the local self-hosted site: cd /some-path-to/wp-calypso/apps/odyssey-stats && STATS_PACKAGE_PATH=/some-path-to/jetpack/projects/packages/stats-admin yarn dev.
Navigate to Stats > Traffic page.
Open the date range picker.
Click on the Yesterday shortcut.
Ensure the page is redirected to the hourly view for Yesterday.

Before the change

2024-12-20.12.52.00.mov

After the change

2024-12-20.12.55.13.mov

Select the chart period to Weeks and a date range shortcut other than the default Last 7 Days.
Click on the sidebar Stats item.
Ensure the default Stats view applies the stored chart period and date range shortcut.

2024-12-20.12.59.54.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-19T17:02:35Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-12-19T17:03:16Z

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

Sections (~4 bytes removed 📉 [gzipped])

name   parsed_size           gzip_size
stats        +16 B  (+0.0%)       -4 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.

Async-loaded Components (~35 bytes added 📈 [gzipped])

name                                       parsed_size           gzip_size
async-load-store-app-store-stats-listview        +95 B  (+0.1%)      +35 B  (+0.1%)
async-load-store-app-store-stats                 +95 B  (+0.0%)      +35 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

a8ck3n · 2024-12-20T08:14:06Z

client/my-sites/stats/site.jsx

@@ -918,7 +926,8 @@ export default connect(
 			supportUserFeedback,
 		} = getEnvStatsFeatureSupportChecks( state, siteId );

-		const hasSiteLoadedFeatures = hasLoadedSiteFeatures( state, siteId );
+		// Odyssey Stats does not need loading features to determine gated features.
+		const hasSiteLoadedFeatures = hasLoadedSiteFeatures( state, siteId ) || isOdyssey;


Suggested change

const hasSiteLoadedFeatures = hasLoadedSiteFeatures( state, siteId ) || isOdyssey;

const hasSiteLoadedFeatures = isOdyssey || hasLoadedSiteFeatures( state, siteId );

If we swap these conditions, we'll avoid calling the function at all inside Odyssey.

a8ck3n · 2024-12-20T08:16:06Z

client/my-sites/stats/site.jsx

@@ -326,6 +326,7 @@ class StatsSite extends Component {
 			isJetpack,
 			isSitePrivate,
 			isOdysseyStats,
+			isOdyssey,


This is a bit confusing having both isOdyssey and isOdysseyStats here. Is this the case where one represents API routing vs direct calls to wpcom? If so, we should probably rename it now to avoid confusion.

Update: I see two more instances of this below confirming the above comment.

If so, we should probably rename it now to avoid confusion.

I think so. That would be the refactoring work, as we will tackle them all at once later. Context: p1733887222490759-slack-C82FZ5T4G.

My bad, maybe isWPAdmin a better name for isOdyssey?

Addressed in c0df942.

a8ck3n

This doesn't quite work correctly in my testing. I'm seeing that in some cases, the page=stats is added to the params twice. Then, if the user refreshes the page, the browser tries to encode the parameter list. I end up seeing the following:

?page%5B0%5D=stats&page%5B1%5D=stats&chartStart=2024-12-20&chartEnd=2024-12-20

That decodes to:

?page[0]=stats&page[1]=stats

So it appears the browser is trying to number the duplicate params?

I think the fix is probably to dedupe the params before updating the string maybe?

a8ck3n · 2024-12-20T09:45:07Z

client/my-sites/stats/utils.js

+	const queryString = isOdyssey && query ? `&${ new URLSearchParams( query ).toString() }` : '';
+
+	return `${ baseUrl }${ queryString }`;
+};


I tested the following and it's working in my testing.

export const parseQueryStringForOdysseyRedirect = ( baseUrl, isOdyssey, query = {} ) => { const newParams = new URLSearchParams( window.location.search ); if ( isOdyssey ) { const queryParams = new URLSearchParams( query ); queryParams.forEach( ( value, key ) => { newParams.set( key, value ); } ); } return `${ baseUrl }?${ newParams.toString() }`; };

We'd then need to upate the two call sites to not include window.location.search in the base URL value.

What do you think?

I would not prefer to make the function process inputs along with a global variable simultaneously, which would make it quite MAGIC.

Yes, good point. We could make the window.location.search a passed-in value to avoid that. BUT… @kangzj 's comment below about not using window.location.search at all probably makes more sense.

dognose24 · 2024-12-20T14:01:51Z

the page=stats is added to the params twice.

I saw it when testing, but reproducing it was not easy. Could you provide a way to reproduce it constantly?

…or_odyssey

matticbot · 2024-12-23T22:39:35Z

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

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

command-palette-wp-admin
help-center
notifications
odyssey-stats

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/stats_fix_redirection_query_string_for_odyssey on your sandbox.

kangzj · 2024-12-23T23:05:38Z

client/my-sites/stats/controller.jsx

-		isWpAdmin && context.query ? `&${ new URLSearchParams( context.query ).toString() }` : '';
-	const url = `/stats/day/${ context.params.module }/${ context.params.site }${ window.location.search }${ query }`;
+	const url = parseQueryStringForOdysseyRedirect(
+		`/stats/day/${ context.params.module }/${ context.params.site }${ window.location.search }`,


We might not need ${ window.location.search } here, which probably caused the issue of multiple page=stats issue.

I think we could use query, path and/or pathname from context, which are unified in both Odyssey and Calypso, which apparently I missed when adding the redirection logic.

Sounds good to me!

It seems we only need the base pathname and append the context.query params string to it, which works on both Caylpso and Odyssey Stats. Updated in 0fc48b5.

a8ck3n · 2024-12-26T10:25:44Z

I saw it when testing, but reproducing it was not easy. Could you provide a way to reproduce it constantly?

So I tested this again today. It's very easy to reproduce for me. The steps are:

Visit Stats → Traffic. Should default to 7-day view.
Select Yesterday shortcut and apply.
The URL should update and show ?page=stats&page=stats in the URL params.
If the user refreshes the page via "command-R" or the refresh button in the browser, they'll see the URL encoded version noted above. — ?page%5B0%5D=stats&page%5B1%5D=stats&chartStart=2024-12-20&chartEnd=2024-12-20

The actual full URL I'm seeing is this:

https://hanabi.ap.ngrok.io/wp-admin/admin.php?page=stats#!/stats/hour/hanabi.ap.ngrok.io?page=stats&page=stats&chartStart=2024-12-25&chartEnd=2024-12-25

So page=stats is in there three times but it's only the bits after the #! that are relevant here, I think.

…or_odyssey

…e provided pathname

dognose24 · 2024-12-29T17:59:16Z

I updated the URL path processor to use the base pathname and append the query string from context.query to it, which seems to work for both Calypso and Odyssey Stats.

kangzj

Works well and looks good 👍

dognose24 added 3 commits December 20, 2024 00:34

Refactor parseQueryStringForOdysseyRedirect for sharing

953df4a

Fix stored period and date range shorcut by isOdyssey

67b8acc

Fix redirection with query string on Odyssey Stats

f0fe80c

dognose24 added the [Feature] Stats Everything related to our analytics product at /stats/ label Dec 19, 2024

dognose24 requested a review from a team December 19, 2024 17:02

dognose24 self-assigned this Dec 19, 2024

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 19, 2024

dognose24 added the Odyssey Stats Calypso Stats in Jetpack label Dec 19, 2024

dognose24 requested a review from a8ck3n December 19, 2024 17:06

a8ck3n reviewed Dec 20, 2024

View reviewed changes

a8ck3n requested changes Dec 20, 2024

View reviewed changes

a8ck3n reviewed Dec 20, 2024

View reviewed changes

dognose24 requested a review from kangzj December 23, 2024 12:49

Merge branch 'trunk' into update/stats_fix_redirection_query_string_f…

c08d75f

…or_odyssey

kangzj reviewed Dec 23, 2024

View reviewed changes

dognose24 added 3 commits December 28, 2024 03:41

Merge branch 'trunk' into update/stats_fix_redirection_query_string_f…

f90b02a

…or_odyssey

Update the utility only to parse the query object and append it to th…

0fc48b5

…e provided pathname

Rename isOdyssey and update condition usage

c0df942

dognose24 requested review from a8ck3n and kangzj December 29, 2024 17:59

kangzj approved these changes Dec 29, 2024

View reviewed changes

kangzj merged commit 1bb823f into trunk Dec 29, 2024
14 checks passed

kangzj deleted the update/stats_fix_redirection_query_string_for_odyssey branch December 29, 2024 22:20

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 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Odyssey Stats: Fix redirection with query string for custom date range #97658

Odyssey Stats: Fix redirection with query string for custom date range #97658

dognose24 commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited by matticbot

Loading

matticbot commented Dec 19, 2024 •

edited

Loading

a8ck3n Dec 20, 2024

a8ck3n Dec 20, 2024 •

edited

Loading

dognose24 Dec 20, 2024

kangzj Dec 23, 2024

dognose24 Dec 29, 2024

a8ck3n left a comment

a8ck3n Dec 20, 2024

dognose24 Dec 20, 2024 •

edited

Loading

a8ck3n Dec 26, 2024

dognose24 commented Dec 20, 2024

matticbot commented Dec 23, 2024 •

edited

Loading

kangzj Dec 23, 2024

dognose24 Dec 27, 2024 •

edited

Loading

dognose24 Dec 29, 2024 •

edited

Loading

a8ck3n commented Dec 26, 2024 •

edited

Loading

dognose24 commented Dec 29, 2024

kangzj left a comment

	const hasSiteLoadedFeatures = hasLoadedSiteFeatures( state, siteId ) \|\| isOdyssey;
	const hasSiteLoadedFeatures = isOdyssey \|\| hasLoadedSiteFeatures( state, siteId );

Odyssey Stats: Fix redirection with query string for custom date range #97658

Odyssey Stats: Fix redirection with query string for custom date range #97658

Conversation

dognose24 commented Dec 19, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Before the change

After the change

Pre-merge Checklist

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

matticbot commented Dec 19, 2024 • edited Loading

a8ck3n Dec 20, 2024

Choose a reason for hiding this comment

a8ck3n Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

dognose24 Dec 20, 2024

Choose a reason for hiding this comment

kangzj Dec 23, 2024

Choose a reason for hiding this comment

dognose24 Dec 29, 2024

Choose a reason for hiding this comment

a8ck3n left a comment

Choose a reason for hiding this comment

a8ck3n Dec 20, 2024

Choose a reason for hiding this comment

dognose24 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

a8ck3n Dec 26, 2024

Choose a reason for hiding this comment

dognose24 commented Dec 20, 2024

matticbot commented Dec 23, 2024 • edited Loading

kangzj Dec 23, 2024

Choose a reason for hiding this comment

dognose24 Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

dognose24 Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

a8ck3n commented Dec 26, 2024 • edited Loading

dognose24 commented Dec 29, 2024

kangzj left a comment

Choose a reason for hiding this comment

dognose24 commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited by matticbot

Loading

matticbot commented Dec 19, 2024 •

edited

Loading

a8ck3n Dec 20, 2024 •

edited

Loading

dognose24 Dec 20, 2024 •

edited

Loading

matticbot commented Dec 23, 2024 •

edited

Loading

dognose24 Dec 27, 2024 •

edited

Loading

dognose24 Dec 29, 2024 •

edited

Loading

a8ck3n commented Dec 26, 2024 •

edited

Loading