-
Notifications
You must be signed in to change notification settings - Fork 822
Forms: add field file as paid with custom setup #43177
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
base: trunk
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 3 files.
1 file is newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
0bd1c60
to
e208d8c
Compare
e24793e
to
d807a98
Compare
bf9f993
to
141cdc3
Compare
projects/packages/forms/src/blocks/contact-form/class-contact-form-block.php
Outdated
Show resolved
Hide resolved
@@ -530,6 +531,11 @@ public static function gutenblock_render_field_consent( $atts, $content ) { | |||
* @return string HTML for the file upload field. | |||
*/ | |||
public static function gutenblock_render_field_file( $atts, $content ) { | |||
if ( apply_filters( 'jetpack_unauth_file_upload_plan_check', true ) && ! Plan::supports( 'field-file' ) ) { |
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.
Is this filter just to help with testing?
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.
Testing and development. Setting it to false
will allow you to add the block without having to get the upgrade.
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.
We can consider removing the filter entirely once we're done with the development? It's comfy though
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 left a few comments :)
@@ -704,6 +705,10 @@ class WPCOM_Features { | |||
self::WPCOM_BUSINESS_AND_HIGHER_PLANS, | |||
self::WPCOM_PRO_PLANS, | |||
), | |||
self::FIELD_FILE => array( | |||
self::WPCOM_PERSONAL_AND_HIGHER_PLANS, |
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 think we should exclude the WOO trial from this.
This way we don't worry about folks that are not paying us and us storing info for free for them.
'jetpack_register_gutenberg_extensions', | ||
function () { | ||
if ( ! apply_filters( 'jetpack_unauth_file_upload_plan_check', true ) ) { | ||
\Jetpack_Gutenberg::set_extension_available( 'field-file' ); |
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.
Instead of having set_extension_available on the Jetpack_Gutenberg here and creating a Jetpack plugin depenency. It would be good to move it to blocks package. Which we already include.
This way we avoid this dependency.
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.
Mmm... I see what you mean, but Blocks depends on Jetpack_Gutenberg. Maybe we can add some set_extension_available
on Blocks class, but it feels like duped code and actually out of its scope?
@@ -50,6 +50,10 @@ | |||
flex-direction: row; | |||
gap: var(--wp--style--block-gap, 1.5rem); | |||
|
|||
& > div { |
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.
This rule seems really generic. Should it really apply to all the divs?
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.
It is, it was the only way to make the nudge coexist with our flexbox layout. I could narrow it a bit with:
& > div:first-child
EDIT: scratch that, doesn't work. I was looking at the wrong div. Agreed it is generic, feel free to try alternatives when the nudge is showing, I couldn't think of any other option.
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.
In my testing it doesn't seem to make a difference. What theme are you using? What exactly is the difference?
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.
@@ -79,6 +79,7 @@ class Current_Plan { | |||
'akismet', | |||
'payments', | |||
'videopress', | |||
'field-file', // Forms |
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.
Do we need to specific this both places? Once in wpcom_features and here?
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 think this takes care of providing the minimum required plan when you are self-hosted or not connected.
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 don't think we have a personal plan anymore just complete. See https://cloud.jetpack.com/pricing
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.
Yeah, I think we'll be better off just pointing at complete for now.
On my jetpack site the nudge takes me to. Is this expected? I don't see anything telling about the file upload field. Also I thought I only need to buy Jetpack complete. |
@@ -81,7 +81,8 @@ | |||
"videopress/video-chapters", | |||
"ai-assistant-backend-prompts", | |||
"ai-list-to-table-transform", | |||
"ai-use-chrome-ai-sometimes" | |||
"ai-use-chrome-ai-sometimes", | |||
"field-file" |
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.
Do we need this here? Since the field-file is not part of the jetpack/extensions package at all.
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.
If we remove it, the process no longer registers the block as intended and there's no plan check, meaning the block appears as free and no nudge is shown. Unsure if there's another way, but all the other plan_check
blocks are there (send-a-message, calendly, opentable, simple-payments, etc)
We can make the Dotcom side work first and focus on Jetpack side flows separately. |
After removing the support from Personal plan, the upgrade flow now takes you straight to Complete checkout. |
Closes #41677
Closes JETPACK-335
Proposed changes:
The changes include a combination of:
Other information:
Jetpack product discussion
peKye1-1vf-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
On all cases, test the upgrade flow works and lands on the right plans (for now, personal and above for dotcom, Complete on Jetpack).
Locally:
Build packages/plans, packages/forms, plugins/wpcomsh and plugins/jetpack.
Open the editor, add a form and then insert a File Upload field block. Test with and without
add_filter( 'jetpack_block_editor_enable_upgrade_nudge', '__return_true' );
Without the filter (custom styled nudge):

With the filter (as will likely look on dotcom):

Use the PR links to test on sandbox and simple/business sites, you need to checkout 181551-ghe-Automattic/wpcom
Test on AT sites.