Add VideoPress "benefit" card to plan removal screen #67181

jgcaruso · 2022-08-30T21:31:40Z

Proposed Changes

Add details about how many videos will be not accessible when removing a Jetpack plan when VideoPress is used

Jetpack VideoPress Product

Jetpack Complete (includes VideoPress)

Testing Instructions

Pre-testing: Create a new site with a VideoPress plan and upload a few videos to it (if you don't already have one)
visit /me/purchases/
select your Jetpack site
Click "Remove [Jetpack Plan]" button
The modal that appears should include a Video section with the correct VideoPress video count
Select another Jetpack site that doesn't have a plan that includes VideoPress (eg add the Security plan to a new site)
Click "Remove plan" button -> You should NOT see the VideoPress "benefit" card
Manual Regression test: Visit /media/[some site], flip quickly between the filters (Images, Videos) and ensure the correct data is shown (from this issue fix(media): make sure we reset 'nextPageHandle' in case query changes #44579)
Manual Regression test 2: Visit /stats/month/[some site], ensure the Videos section has data (this is an example of <QueryMedia> being used without a query specified.

Pre-merge Checklist

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)?

Related to #66076

github-actions · 2022-08-30T21:31:53Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

matticbot · 2022-08-30T21:39:19Z

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

Sections (~1374 bytes added 📈 [gzipped])

name            parsed_size           gzip_size
site-purchases      +4583 B  (+0.3%)    +1329 B  (+0.4%)
purchases           +4583 B  (+0.3%)    +1329 B  (+0.3%)
domains             +4583 B  (+0.3%)    +1329 B  (+0.3%)
stats                 +70 B  (+0.0%)      +21 B  (+0.0%)
media                 +70 B  (+0.0%)      +24 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.

…aceholder, need to add logic to retrieve correct value

… route, so other uses of MediaQuery component will continue to work

jgcaruso · 2022-09-07T21:34:31Z

client/state/data-layer/wpcom/sites/media/index.js

+			getCurrentRoute( getState() )?.startsWith( '/media/' ) &&
 			! isEqual(
 				omit( query, 'page_handle' ),
 				omit( getNextPageQuery( getState(), siteId ), 'page_handle' )


@flootr Tagging you here since you originally make this fix for #44579

The query check was preventing the request from the QueryMedia react component from completing anywhere except the Media Library in calypso because the "next page query" url always had number and none of the passed in query filters (like mime_type) as properties and the custom query that I provided to the component just had my mime_type property. As a result it would always fall into this condition and not populate the media state property, and my component would never get the updated data from the request.

It doesn't look like this change I'm making is causing the original issue to come back, unless I'm not testing correctly. Just hoping to get a second set of eyes on this incase you see an issue with the change.

Alternatively, it sounds like the original issue that was being fixed has something to do with old requests not being cancelled... I wonder if there is another way to solve for that that wouldn't require a special case to skip this check for the /media route? Something like storing a "current query" instead and just comparing that with whatever is coming back in the result?

I've actually replaced this change with a bug fix. Took a bit of investigating, but it looks like the <QueryMedia> component wasn't setting the query state, which was causing the isEqual check in the above code to trigger when it shouldn't.

This has now been fixed directly in the <QueryMedia> component!

roundhill

Testing steps worked well and code LGTM.

jgcaruso · 2022-09-12T16:16:48Z

Testing steps worked well and code LGTM.

Thanks! Before deploying I gave it another look and found out that one of the fixes I did was actually not necessary since the behaviour was caused by something completely different in client/components/data/query-media/index.jsx. I fixed that instead and added an extra last step to the test plan.

Could you give the PR another review when you have time?

roundhill

Still works great, the new regression tests were fine.

a8ci18n · 2022-09-13T19:17:52Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7526400

Thank you @jgcaruso for including a screenshot in the description! This is really helpful for our translators.

a8ci18n · 2022-09-21T12:31:34Z

Translation for this Pull Request has now been finished.

* add check and benefit card for videopress -- video count is just a placeholder, need to add logic to retrieve correct value * change label to be less vague (and not misleading) * (maybe) fix for media request not being being in state * refactored videopress benefits card into separate file * use placeholder while data is loading * only allow early return for media library filter issue on the `media` route, so other uses of MediaQuery component will continue to work * cleanup comments * remove unused prop (copy-pasta) * handle nulls for current route * replace `/media` route special case with a bugfix

jgcaruso added the [Status] In Progress label Aug 30, 2022

jgcaruso self-assigned this Aug 30, 2022

jgcaruso added 5 commits September 7, 2022 09:18

add check and benefit card for videopress -- video count is just a pl…

749f74f

…aceholder, need to add logic to retrieve correct value

change label to be less vague (and not misleading)

78e20f0

(maybe) fix for media request not being being in state

87b74a6

refactored videopress benefits card into separate file

b6199bf

use placeholder while data is loading

00a3a0d

jgcaruso force-pushed the update/videopress-add-benefits-screen branch from 9cf81fa to 00a3a0d Compare September 7, 2022 13:18

jgcaruso added 4 commits September 7, 2022 15:13

only allow early return for media library filter issue on the media…

94c127b

… route, so other uses of MediaQuery component will continue to work

cleanup comments

88e7396

remove unused prop (copy-pasta)

4f0d424

handle nulls for current route

bfdcb37

jgcaruso changed the title ~~[WIP] Add VideoPress "benefit" card to plan removal screen~~ Add VideoPress "benefit" card to plan removal screen Sep 7, 2022

jgcaruso commented Sep 7, 2022

View reviewed changes

jgcaruso requested a review from flootr September 7, 2022 21:36

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

jgcaruso requested review from roundhill, thedebian and pgk September 7, 2022 21:36

jgcaruso removed the [Status] In Progress label Sep 7, 2022

jgcaruso marked this pull request as ready for review September 7, 2022 21:36

roundhill approved these changes Sep 9, 2022

View reviewed changes

jgcaruso added 2 commits September 12, 2022 12:08

replace /media route special case with a bugfix

a0bb7c3

Merge branch 'trunk' into update/videopress-add-benefits-screen

2055ac6

jgcaruso requested a review from roundhill September 12, 2022 16:17

Merge branch 'trunk' into update/videopress-add-benefits-screen

0c4fbde

roundhill approved these changes Sep 13, 2022

View reviewed changes

jgcaruso merged commit 090aa75 into trunk Sep 13, 2022

jgcaruso deleted the update/videopress-add-benefits-screen branch September 13, 2022 17:26

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 Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VideoPress "benefit" card to plan removal screen #67181

Add VideoPress "benefit" card to plan removal screen #67181

jgcaruso commented Aug 30, 2022 •

edited

Loading

github-actions bot commented Aug 30, 2022 •

edited by matticbot

Loading

matticbot commented Aug 30, 2022 •

edited

Loading

jgcaruso Sep 7, 2022 •

edited

Loading

jgcaruso Sep 12, 2022

roundhill left a comment

jgcaruso commented Sep 12, 2022

roundhill left a comment

a8ci18n commented Sep 13, 2022

a8ci18n commented Sep 21, 2022

Add VideoPress "benefit" card to plan removal screen #67181

Add VideoPress "benefit" card to plan removal screen #67181

Conversation

jgcaruso commented Aug 30, 2022 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Aug 30, 2022 • edited by matticbot Loading

matticbot commented Aug 30, 2022 • edited Loading

jgcaruso Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

jgcaruso Sep 12, 2022

Choose a reason for hiding this comment

roundhill left a comment

Choose a reason for hiding this comment

jgcaruso commented Sep 12, 2022

roundhill left a comment

Choose a reason for hiding this comment

a8ci18n commented Sep 13, 2022

a8ci18n commented Sep 21, 2022

jgcaruso commented Aug 30, 2022 •

edited

Loading

github-actions bot commented Aug 30, 2022 •

edited by matticbot

Loading

matticbot commented Aug 30, 2022 •

edited

Loading

jgcaruso Sep 7, 2022 •

edited

Loading