Skip to content

STATS-59 - Match filename for the location stats to production #103281

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
+9 −1
Open

STATS-59 - Match filename for the location stats to production #103281

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6906030
Match filename for the location stats to production
Nikschavan May 9, 2025
71466bd
use Object.prototype.hasOwnProperty for better clarity
Nikschavan May 9, 2025
f2aa7f2
cs fixes
Nikschavan May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion client/my-sites/stats/summary/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import PageHeader from '../components/headers/page-header';
import { STATS_FEATURE_DOWNLOAD_CSV } from '../constants';
import StatsModuleLocations from '../features/modules/stats-locations';
import LocationsNavTabs from '../features/modules/stats-locations/locations-nav-tabs';
import { GEO_MODES } from '../features/modules/stats-locations/types';
import StatsModuleUTM from '../features/modules/stats-utm';
import { shouldGateStats } from '../hooks/use-should-gate-stats';
import { StatsGlobalValuesContext } from '../pages/providers/global-provider';
Expand Down Expand Up @@ -367,6 +368,11 @@ class StatsSummary extends Component {
backLink += domain;
}
const navigationItems = [ { label: backLabel, href: backLink }, { label: title } ];
const geoMode = this.props.context.query.geoMode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a Locations module dedicated scope for this query parameter?

let pathForCsvFile = path;
if ( module === 'locations' ) {
	const geoMode = this.props.context.query.geoMode;
	const geoModeLabel =
		geoMode && Object.prototype.hasOwnProperty.call( GEO_MODES, geoMode )
			? GEO_MODES[ geoMode ]
			: 'country';
	pathForCsvFile = `${ path }-${ geoModeLabel }`;
}

Copy link
Preview

Copilot AI May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Add a comment explaining the expected values in GEO_MODES and why defaulting to 'country' is appropriate to help future maintainers understand the mapping.

Suggested change
const geoMode = this.props.context.query.geoMode;
const geoMode = this.props.context.query.geoMode;
// `GEO_MODES` maps geoMode values (e.g., 'country', 'region', 'city') to their display labels.
// Defaulting to 'country' ensures a fallback label is used when geoMode is invalid or undefined,
// as 'country' is the most commonly used and broadly applicable geographic level.

Copilot uses AI. Check for mistakes.

const geoModeLabel =
geoMode && Object.prototype.hasOwnProperty.call( GEO_MODES, geoMode )
? GEO_MODES[ geoMode ]
: 'country';

return (
<Main fullWidthLayout>
Expand All @@ -391,7 +397,9 @@ class StatsSummary extends Component {
<DownloadCsv
statType={ statType }
query={ moduleQuery }
path={ path }
path={
statType === 'statsCountryViews' ? `${ path }-${ geoModeLabel }` : path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be a countryviews-country filename for the legacy Countries module, isn't it?

}
period={ this.props.period }
skipQuery={ statType === 'statsUTM' }
/>
Expand Down