Premium Content Block: Move to Jetpack #17907

apeatling · 2020-11-25T22:37:10Z

Changes proposed in this Pull Request:

This PR moves the Premium Content block code from Editing toolkit into Jetpack. It reformats the block code to match the Jetpack block organization and removes any duplicate functionality.

Note: In order for users to seamlessly transition from the editing toolkit version to the Jetpack version of this block, the namespace has to remain as premium-content/container and not include /jetpack (this is handled in this PR). Doing this means we do not have to maintain the old code in Editing Toolkit forever.

Fixes https://github.com/Automattic/view-design/issues/144

Jetpack product discussion

Post: p1HpG7-aFT-p2

Does this pull request change what data or activity we track or use?

I don't think so, but I might be missing something.

Testing instructions:

Please test this on a local Jetpack install using Jurassic.tube, and on WordPress.com.

Testing a Jetpack Site:

Please use https://github.com/Automattic/a8c-wp-env to test as it will make your life much easier.
Start up a site with npm run start, and DO NOT include Editing Toolkit (just use Gutenberg, and Jetpack).
Check out this branch and activate jurassic.tube with npm run tube:init
Add a "Security Daily" Jetpack plan to your jurassic.tube site.
Create a new post and insert the Premium Content block. Confirm this works.
Connect to Stripe using the ryan.ray test stripe account in the secret store. Confirm this connection works.
Add a payment plan through the block, confirm this works.
Add some content to the subscriber and visitor views.
Publish the page, and confirm you can see only the subscriber content in the front end.
Open an incognito window, confirm you can see only the visitor content in the front end.
Click the Subscribe button in the incognito window, confirm you see the correct payment plan amount and payment modal.
Purchase a subscription. You can use this instructions: PCYsg-lW4-p2#testing
Confirm that you now see the subscriber content in the front end.

Testing a WordPress.com Site:

Please repeat the above on a WordPress.com site -- with the following steps first
Use npm run sync and select Jetpack
Use npm run sync and select Editing Toolkit
Open wp-calypso/apps/editing-toolkit/editing-toolkit-plugin/full-site-editing-plugin.php and remove add_action( 'plugins_loaded', __NAMESPACE__ . '\load_premium_content' ); on line 297 so the block doesn't load from Editing Toolkit.
Wait for everything to sync and go ahead and test as above on a sandboxed WordPress.com site.

Some other tests to note:

Try adding a Premium content block from Editing Toolkit on a sandboxed WordPress.com site, and then load up this PR and make sure it seamlessly transitions to the Jetpack version in the editor and front end.
Remove your Jetpack plan on your Jurassic.tube site, and confirm the Premium content no longer shows up.
Load up a free WordPress.com site sandboxed, and confirm you still see the in-editor upgrade nudges.

Privacy Changes:

This introduces a new cookie that is set to remember if you are a subscriber to the site and show you subscription based content without the need to log in again.

Proposed changelog entry for your changes:

WIP

jetpackbot · 2020-11-25T22:41:08Z

	Warnings
⚠️	`pre-commit` hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6770974

jeherve · 2020-11-26T19:56:23Z

Do you think you could post on +jetpackp2 about the feature, so Product can weigh in on whether this would be a good fit for the Jetpack plugin or not? This may help us decide where to ship the block, even when developed in the Jetpack monorepo.

If there is already a discussion about this somewhere, I may have missed it, sorry! Could you add a link to it in the PR description?

Thank you!

matticbot · 2020-11-30T20:09:35Z

Caution: This PR has changes that must be merged to WordPress.com
Hello apeatling! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D53531-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

apeatling · 2020-11-30T20:39:47Z

Functionality looks to be working correctly now, and it is set up to seamlessly transition from the existing code in Editing Toolkit. Still some polishing to do.

aaronrobertshaw

Nice work @apeatling ! 👍

The code looks good, the structural and organisational improvements are nice.

I created diffs between the editing toolkit version and the code in this PR. Everything stacked up and made seeing what had to be tweaked for use in Jetpack much easier.

Unfortunately, I ran into some troubles with my dev environment this afternoon so I'll have to do the actual hands-on testing first thing next week.

There were a few minor things that came up while reviewing the code, most of which are already the case in the editing toolkit version.

Linting Errors

There are quite a few linting errors that might be good to address while we are bringing this across to Jetpack. In particular, on the PHP side of things, there are some PHP 5.6 compatibility issues.

I've also taken the liberty of pushing a commit to fix some minor linting errors with icon.js

Deprecations

With what we've learnt recently about block deprecations, it looks like the version numbers of the logged-out-view deprecations are out. This is already the case in the editing toolkit so not something that has been introduced. As deprecations are attempted in reverse chronological order, trying v1 before v2 would, by the names, be back to front for me.

Continuing with the nitpicking, the subscriber-view deprecation array is actually within the subscriber-view/deprecated/v1/index.js file. I think it would make more sense if the deprecated/v1 file only contained the v1 deprecation and the array done via a deprecated/index.js file or in the block's index.js settings.

Keywords

In the editing toolkit version, the premium content block's keywords contained premium content, the keywords in Jetpack don't. Should we add that back?

Dashicons

In wp-calypso, the linter provides a warning against importing Dashicon. In Jetpack, that warning doesn't occur. Should we take the opportunity to update that while we are here?

'Dashicon' import from '@wordpress/components' is restricted. Please use `@wordpress/icons` instead

stacimc · 2020-12-04T22:48:49Z

I'm unable to add a Premium Content block to a Jetpack site with a free plan. If I add the block to a Jetpack site with the necessary plan and then downgrade, I get the warning that the block is unsupported:

Is that we want in this case, or would we expect to see the block render with an upgrade nudge (similar to Recurring Payments)?

apeatling · 2020-12-07T18:34:09Z

Is that we want in this case, or would we expect to see the block render with an upgrade nudge (similar to Recurring Payments)?

Yes I think we'd want the nudge, I'm not sure exactly how this works though. I do see on my local site that the unavailability reason is "missing_plan_no_upgrade_nudge". There must be a way to enable the nudge.

apeatling · 2020-12-07T21:26:31Z

Lint errors from the moved over code fixed up. 👍

aaronrobertshaw · 2020-12-08T02:36:12Z

This mostly tested as advertised for me.

The only issue I encountered, not raised already, was related to an edge case where there were no subscription plans previously created. I came across this when I had the store sandboxed. The result was the subscribe (payments) button displaying a nudge but then showing a 0 beneath it as per the screenshot below.

This can be ignored if it has been addressed already in other PRs or isn't a scenario that can be encountered in production.

apeatling · 2020-12-08T20:58:31Z

I came across this when I had the store sandboxed. The result was the subscribe (payments) button displaying a nudge but then showing a 0 beneath it as per the screenshot below.

This was on a WordPress.com site?

apeatling · 2020-12-08T20:59:06Z

In the editing toolkit version, the premium content block's keywords contained premium content, the keywords in Jetpack don't. Should we add that back?

Because this is in the title, we shouldn't need it in the keywords.

stacimc

I really appreciate some of the restructuring of the block -- it's now much easier/more intuitive to navigate the code 👏

When testing on a jetpack site with a free plan, I believe both Premium Content and Payments should render but with upgrade nudges. I'm seeing Premium Content render without the upgrade nudge, although there is a nudge on the nested payments button:

The nudge on the Payments button should disappear when #17702 is merged, but I think the missing nudge on Premium Content needs to be added back in. There is already a shouldUpgrade boolean correctly being populated on the container which we can use.

Previously this wasn't an issue because the Premium Content block was in ETK and wasn't even available on Jetpack sites unless there was a sufficient plan.

stacimc · 2020-12-08T21:15:02Z

extensions/blocks/premium-content/_inc/access-check.php

+		return false;
+	}
+	return true;
+}


There is an edge case here where the site (Jetpack or wpcom) does not have a sufficient plan ugprade, but does have a Stripe account connected. The user will get the upgrade nudge in the edtior, but since we only check for the Stripe account in the pre_render_checks, the block will render just fine on the frontend. I believe this is a pre-existing problem.

aaronrobertshaw · 2020-12-08T23:27:02Z

This was on a WordPress.com site?

It was a sandboxed WordPress.com site. My apologies for leaving out crucial info 🙁

apeatling · 2020-12-09T20:27:02Z

All of @stacimc's work in Automattic/wp-calypso#47388 has now been integrated into this PR.

apeatling · 2020-12-09T21:00:41Z

This one is ready for a crew review now.

jeherve

I've done another pass at this. I pushed a few commits to try to fix some of the issues I saw, I hope that's okay.

It does seem to work for me at the moment, but with the size of this PR it's entirely possible that I missed something. At this point, I think it would be nice to merge this as it is, and then get a few people to test all this to catch every possible issue before the release.

In the future, I think it would be nice to use our own Button block for those buttons, and take the opportunity to improve our own block with the color editing features we're adding to the Premium Content buttons in this PR.

jeherve · 2021-01-19T16:51:38Z

projects/plugins/jetpack/extensions/blocks/premium-content/_inc/stripe-nudge.js

@@ -0,0 +1,85 @@
+/**


I believe it's already in progress in another PR, but just taking note of it here just in case; it would be nice to get rid of this in favor of the existing nudge in the future.

jeherve

It looks like my approval may not be enough; there are some linting problems that will need to be fixed it seems:
https://github.com/Automattic/jetpack/pull/17907/checks?check_run_id=1729352685

…iew is disarmed The fix was originally merged into wp-calypso in: Automattic/wp-calypso#48988

…-block

This reverts commit f1498e9.

I've taken the opportunity to suppress some of those warnings for all extensions, as I do not think they can be realistically all filled for all our blocks at the moment.

jeherve

Let's merge this! 💥

jeherve · 2021-02-03T07:53:26Z

r220419-wpcom

apeatling added [Status] In Progress [Block] Paid Content aka Premium Content labels Nov 25, 2020

apeatling self-assigned this Nov 25, 2020

matticbot added Touches WP.com Files labels Nov 25, 2020

apeatling marked this pull request as draft November 25, 2020 22:37

jeherve added the [Status] Needs Product Review Consider posting about this on an internal P2 for discussion label Nov 26, 2020

apeatling force-pushed the add/premium-content-block branch from 5b1e516 to da52549 Compare December 2, 2020 18:55

apeatling marked this pull request as ready for review December 3, 2020 22:41

apeatling requested a review from a team December 3, 2020 23:00

apeatling added [Status] Needs Team Review and removed [Status] Needs Product Review Consider posting about this on an internal P2 for discussion labels Dec 3, 2020

aaronrobertshaw reviewed Dec 4, 2020

View reviewed changes

apeatling force-pushed the add/premium-content-block branch from 0941573 to e44cbf4 Compare December 7, 2020 18:08

apeatling requested a review from a team December 7, 2020 20:08

stacimc suggested changes Dec 8, 2020

View reviewed changes

apeatling force-pushed the add/premium-content-block branch from 0d76064 to 1ea502e Compare December 9, 2020 19:04

jeherve added 2 commits January 19, 2021 17:38

Fix reference to Subscription_Service

c8b40c0

Add missing arguments when rendering legacy buttons

3ef927c

jeherve previously approved these changes Jan 19, 2021

View reviewed changes

jeherve reviewed Jan 19, 2021

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 19, 2021

Merge in fix for block transformations menu, to ensure the block prev…

0777f36

…iew is disarmed The fix was originally merged into wp-calypso in: Automattic/wp-calypso#48988

andrewserong dismissed jeherve’s stale review via 0777f36 January 20, 2021 06:48

jeherve added 6 commits January 20, 2021 17:45

Try fixing linting issues

375d1b5

Merge remote-tracking branch 'origin/master' into add/premium-content…

9c4eb65

…-block

Add our new js files to the lint exclude list for now

f1498e9

Revert "Add our new js files to the lint exclude list for now"

cabe1b4

This reverts commit f1498e9.

Try to address all linting warnings in new files

a3cddc1

I've taken the opportunity to suppress some of those warnings for all extensions, as I do not think they can be realistically all filled for all our blocks at the moment.

Remove file from excludelist now that it is free of linting errors

6770974

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 20, 2021

jeherve approved these changes Jan 20, 2021

View reviewed changes

apeatling merged commit fee24dc into master Jan 20, 2021

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 20, 2021

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Jan 26, 2021

jeherve added a commit that referenced this pull request Jan 26, 2021

Changelog: add #17907

6feba29

andrewserong mentioned this pull request Feb 3, 2021

E2E test: Move check for the Premium Content block from Earn to Grow Automattic/wp-calypso#49642

Merged

jeherve deleted the add/premium-content-block branch February 3, 2021 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Premium Content Block: Move to Jetpack #17907

Premium Content Block: Move to Jetpack #17907

apeatling commented Nov 25, 2020 •

edited

Loading

jetpackbot commented Nov 25, 2020 •

edited

Loading

jeherve commented Nov 26, 2020

matticbot commented Nov 30, 2020

apeatling commented Nov 30, 2020

aaronrobertshaw left a comment

stacimc commented Dec 4, 2020

apeatling commented Dec 7, 2020

apeatling commented Dec 7, 2020

aaronrobertshaw commented Dec 8, 2020

apeatling commented Dec 8, 2020

apeatling commented Dec 8, 2020

stacimc left a comment

stacimc Dec 8, 2020

aaronrobertshaw commented Dec 8, 2020

apeatling commented Dec 9, 2020

apeatling commented Dec 9, 2020

jeherve left a comment

jeherve Jan 19, 2021

jeherve left a comment

jeherve left a comment

jeherve commented Feb 3, 2021

Premium Content Block: Move to Jetpack #17907

Premium Content Block: Move to Jetpack #17907

Conversation

apeatling commented Nov 25, 2020 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Privacy Changes:

Proposed changelog entry for your changes:

jetpackbot commented Nov 25, 2020 • edited Loading

jeherve commented Nov 26, 2020

matticbot commented Nov 30, 2020

apeatling commented Nov 30, 2020

aaronrobertshaw left a comment

Choose a reason for hiding this comment

Linting Errors

Deprecations

Keywords

Dashicons

stacimc commented Dec 4, 2020

apeatling commented Dec 7, 2020

apeatling commented Dec 7, 2020

aaronrobertshaw commented Dec 8, 2020

apeatling commented Dec 8, 2020

apeatling commented Dec 8, 2020

stacimc left a comment

Choose a reason for hiding this comment

stacimc Dec 8, 2020

Choose a reason for hiding this comment

aaronrobertshaw commented Dec 8, 2020

apeatling commented Dec 9, 2020

apeatling commented Dec 9, 2020

jeherve left a comment

Choose a reason for hiding this comment

jeherve Jan 19, 2021

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

jeherve commented Feb 3, 2021

apeatling commented Nov 25, 2020 •

edited

Loading

jetpackbot commented Nov 25, 2020 •

edited

Loading