-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Stats: prompt users to update the Jetpack plugin #99278
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~189 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
4394682
to
7beeb03
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
7beeb03
to
bddde31
Compare
const { | ||
data = [], | ||
data: locationsViewsData = [], | ||
isLoading: isRequestingData, | ||
isError, | ||
} = useLocationViewsQuery< StatsLocationViewsData >( siteId, geoMode, query, countryFilter, { | ||
enabled: ! shouldGate, | ||
enabled: ! shouldGate && supportsLocationsStatsFeature, | ||
} ); | ||
|
||
const countriesViewsData = useSelector( ( state ) => | ||
getSiteStatsNormalizedData( state, siteId, statType, query ) | ||
) as [ id: number, label: string ]; |
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.
Tried to check just one or the other, but there were complaints that hooks cannot be conditionally rendered. Shouldn't be an actual performance issue, given the StatsLocationViewsData
has the enabled
flag and getSiteStatsNormalizedData
makes use of cache.
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.
Sounds good to me. I assume the getSiteStatsNormalizedData
selector retrieves stats from the Redux state, which are stored thanks to QuerySiteStats
. Maybe it’s time to render that query component only when needed? Otherwise, we’ll be attempting to fetch the legacy country-views API even when we don’t need it.
Just to clarify, this isn’t an issue related to your PR. It could be addressed in another one if you prefer.
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.
I'm all in for further improving that. Would you mind opening an issue about it? I'm not sure I got 100% of the context to better describe what we need to do.
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.
Sure, I'll take a look and open the issue in the project board :)
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Outdated
Show resolved
Hide resolved
const { | ||
data = [], | ||
data: locationsViewsData = [], | ||
isLoading: isRequestingData, | ||
isError, | ||
} = useLocationViewsQuery< StatsLocationViewsData >( siteId, geoMode, query, countryFilter, { | ||
enabled: ! shouldGate, | ||
enabled: ! shouldGate && supportsLocationsStatsFeature, | ||
} ); | ||
|
||
const countriesViewsData = useSelector( ( state ) => | ||
getSiteStatsNormalizedData( state, siteId, statType, query ) | ||
) as [ id: number, label: string ]; |
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.
Sounds good to me. I assume the getSiteStatsNormalizedData
selector retrieves stats from the Redux state, which are stored thanks to QuerySiteStats
. Maybe it’s time to render that query component only when needed? Otherwise, we’ll be attempting to fetch the legacy country-views API even when we don’t need it.
Just to clarify, this isn’t an issue related to your PR. It could be addressed in another one if you prefer.
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Show resolved
Hide resolved
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.
Thanks for the review @Initsogar. I addressed your comments. Can you please take a look at it again?
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Show resolved
Hide resolved
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Outdated
Show resolved
Hide resolved
const { | ||
data = [], | ||
data: locationsViewsData = [], | ||
isLoading: isRequestingData, | ||
isError, | ||
} = useLocationViewsQuery< StatsLocationViewsData >( siteId, geoMode, query, countryFilter, { | ||
enabled: ! shouldGate, | ||
enabled: ! shouldGate && supportsLocationsStatsFeature, | ||
} ); | ||
|
||
const countriesViewsData = useSelector( ( state ) => | ||
getSiteStatsNormalizedData( state, siteId, statType, query ) | ||
) as [ id: number, label: string ]; |
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.
I'm all in for further improving that. Would you mind opening an issue about it? I'm not sure I got 100% of the context to better describe what we need to do.
41fb65b
to
c8af5e7
Compare
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.
Looks good to me! ✅
I tried testing with a site with the following conditions:
- Older Jetpack plugin
- No commercial license
✅ Clicking on the regions or cities tab, shows me the upsell prompt
✅ After purchasing a commercial license, it shows me the Jetpack upgrade prompt
✅ It redirected me to the plugins admin page (/wp-admin/plugins.php
) and after updating my Jetpack to the latest version, and looking Stats again, I can see the regions and cities tabs unlocked properly!
✅ In addition to the happy path, I made some tests to ensure the upsell prompt is working as usual when using the latest Jetpack version, and it works as expected.
client/my-sites/stats/features/modules/stats-locations/stats-locations.tsx
Show resolved
Hide resolved
It didn't let me reply in the same thread, so I'm adding an additional comment here: I just created a follow-up task to address this issue: https://github.com/Automattic/jetpack-roadmap/issues/2287 |
Awesome. Thanks for the careful review and creating the follow-up issue, Rafa! Merging... |
Closes Automattic/jetpack-roadmap#2219.
Proposed Changes
Why are these changes being made?
Testing Instructions
Note
This feature is meant for Odyssey/self-hosted users. Jurassic Ninja should work perfectly for this scenario, specially when used with the Jetpack Beta Tester plugin.
Jetpack > Stats
on your self-hosted site.&flags=stats/locations
to the URL.Outdated (14.1) or recent (14.2) Jetpack plugin, no commercial license
Outdated (14.1) Jetpack plugin, commercial license available
Recent (14.2) Jetpack plugin, commercial license available
Pre-merge Checklist