-
Notifications
You must be signed in to change notification settings - Fork 826
Forms: Add integrations feature flag #45037
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
Changes from all commits
2b0ba32
e63fcf8
553d796
fb3aeeb
a26d8dc
f37e9c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: minor | ||
Type: changed | ||
|
||
Forms: Add integrations feature flag. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
import { ThemeProvider } from '@automattic/jetpack-components'; | ||
import { isSimpleSite } from '@automattic/jetpack-script-data'; | ||
import { useModuleStatus } from '@automattic/jetpack-shared-extension-utils'; | ||
import { | ||
URLInput, | ||
|
@@ -27,6 +26,7 @@ import { store as editorStore } from '@wordpress/editor'; | |
import { useRef, useEffect, useCallback, lazy, Suspense } from '@wordpress/element'; | ||
import { __ } from '@wordpress/i18n'; | ||
import clsx from 'clsx'; | ||
import useFormsConfig from '../../hooks/use-forms-config'; | ||
import { store as singleStepStore } from '../../store/form-step-preview'; | ||
import { | ||
PREVIOUS_BUTTON_TEMPLATE, | ||
|
@@ -99,6 +99,8 @@ function JetpackContactFormEdit( { name, attributes, setAttributes, clientId, cl | |
formTitle, | ||
variationName, | ||
} = attributes; | ||
const formsConfig = useFormsConfig(); | ||
const showFormIntegrations = Boolean( formsConfig?.isIntegrationsEnabled ); | ||
const instanceId = useInstanceId( JetpackContactFormEdit ); | ||
|
||
const steps = useFormSteps( clientId ); | ||
|
@@ -114,48 +116,41 @@ function JetpackContactFormEdit( { name, attributes, setAttributes, clientId, cl | |
block => block.name === 'jetpack/button' | ||
); | ||
|
||
const { | ||
postTitle, | ||
canUserInstallPlugins, | ||
hasAnyInnerBlocks, | ||
postAuthorEmail, | ||
selectedBlockClientId, | ||
onlySubmitBlock, | ||
} = useSelect( | ||
select => { | ||
const { getBlocks, getBlock, getSelectedBlockClientId, getBlockParentsByBlockName } = | ||
select( blockEditorStore ); | ||
const { getEditedPostAttribute } = select( editorStore ); | ||
const selectedBlockId = getSelectedBlockClientId(); | ||
const selectedBlock = getBlock( selectedBlockId ); | ||
let selectedStepBlockId = selectedBlockId; | ||
|
||
if ( selectedBlock && selectedBlock.name !== 'jetpack/form-step' ) { | ||
selectedStepBlockId = getBlockParentsByBlockName( | ||
selectedBlockId, | ||
'jetpack/form-step' | ||
)[ 0 ]; | ||
} | ||
|
||
const { getUser, canUser } = select( coreStore ); | ||
const innerBlocksData = getBlocks( clientId ); | ||
|
||
const title = getEditedPostAttribute( 'title' ); | ||
const authorId = getEditedPostAttribute( 'author' ); | ||
const authorEmail = authorId && getUser( authorId )?.email; | ||
const { postTitle, hasAnyInnerBlocks, postAuthorEmail, selectedBlockClientId, onlySubmitBlock } = | ||
useSelect( | ||
select => { | ||
const { getBlocks, getBlock, getSelectedBlockClientId, getBlockParentsByBlockName } = | ||
select( blockEditorStore ); | ||
const { getEditedPostAttribute } = select( editorStore ); | ||
const selectedBlockId = getSelectedBlockClientId(); | ||
const selectedBlock = getBlock( selectedBlockId ); | ||
let selectedStepBlockId = selectedBlockId; | ||
|
||
if ( selectedBlock && selectedBlock.name !== 'jetpack/form-step' ) { | ||
selectedStepBlockId = getBlockParentsByBlockName( | ||
selectedBlockId, | ||
'jetpack/form-step' | ||
)[ 0 ]; | ||
} | ||
|
||
return { | ||
postTitle: title, | ||
canUserInstallPlugins: canUser( 'create', 'plugins' ), | ||
hasAnyInnerBlocks: innerBlocksData.length > 0, | ||
postAuthorEmail: authorEmail, | ||
selectedBlockClientId: selectedStepBlockId, | ||
onlySubmitBlock: | ||
innerBlocksData.length === 1 && innerBlocksData[ 0 ].name === 'jetpack/button', | ||
}; | ||
}, | ||
[ clientId ] | ||
); | ||
const { getUser } = select( coreStore ); | ||
const innerBlocksData = getBlocks( clientId ); | ||
|
||
const title = getEditedPostAttribute( 'title' ); | ||
const authorId = getEditedPostAttribute( 'author' ); | ||
const authorEmail = authorId && getUser( authorId )?.email; | ||
|
||
return { | ||
postTitle: title, | ||
hasAnyInnerBlocks: innerBlocksData.length > 0, | ||
postAuthorEmail: authorEmail, | ||
selectedBlockClientId: selectedStepBlockId, | ||
onlySubmitBlock: | ||
innerBlocksData.length === 1 && innerBlocksData[ 0 ].name === 'jetpack/button', | ||
}; | ||
}, | ||
[ clientId ] | ||
); | ||
|
||
useEffect( () => { | ||
if ( submitButton && ! submitButton.attributes.lock ) { | ||
|
@@ -754,8 +749,7 @@ function JetpackContactFormEdit( { name, attributes, setAttributes, clientId, cl | |
setAttributes={ setAttributes } | ||
/> | ||
</PanelBody> | ||
|
||
{ ! isSimpleSite() && canUserInstallPlugins && ( | ||
{ showFormIntegrations && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main change. We no longer check ifSimpleSite or check permissions. We just the feature flag. Any given environment (WordPress.com, CIAB, VIP) is now responsible for turning this off if needed. In a separate, already merged PR, we still check plugin permissions, and disable plugin install/activate buttons if needed. |
||
<Suspense fallback={ <div /> }> | ||
<IntegrationControls attributes={ attributes } setAttributes={ setAttributes } /> | ||
</Suspense> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1028,7 +1028,7 @@ public function get_forms_config( WP_REST_Request $request ) { // phpcs:ignore V | |
'siteURL' => ( new Status() )->get_site_suffix(), | ||
'hasFeedback' => ( new Forms_Dashboard() )->has_feedback(), | ||
'hasAI' => $has_ai, | ||
'enableIntegrationsTab' => apply_filters( 'jetpack_forms_enable_integrations_tab', true ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were using this filter on wpcom to hide integrations. We've updated this to use the new filter already in 191699-ghe-Automattic/wpcom ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are deprecating a filter here. Should we be more careful and use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I was flagging this because I figured someone might ask or raise the question. IMO, this is a month or two old, obscure, and wan't well documented to begin with. I highly doubt anyone is using it (except us) and would be OK just removing it. But I'd also be fine leaving it here and updating to apply_filters_deprecated() if we prefer. |
||
'isIntegrationsEnabled' => Jetpack_Forms::is_integrations_enabled(), | ||
'renderMigrationPage' => $switch->is_jetpack_forms_announcing_new_menu(), | ||
'dashboardURL' => add_query_arg( 'jetpack_forms_migration_announcement_seen', 'yes', $switch->get_forms_admin_url() ), | ||
// New data. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,13 +42,6 @@ class Dashboard { | |
*/ | ||
const MENU_PRIORITY = 999; | ||
|
||
/** | ||
* Whether the integrations tab is enabled. | ||
* | ||
* @var bool | ||
*/ | ||
public static $show_integrations = false; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were setting this up to pass it to JS via the config object below in the same file. We now get the equivalent value from the forms config endpoint. |
||
/** | ||
* Dashboard_View_Switch instance | ||
* | ||
|
@@ -63,9 +56,6 @@ class Dashboard { | |
*/ | ||
public function __construct( ?Dashboard_View_Switch $switch = null ) { | ||
$this->switch = $switch ?? new Dashboard_View_Switch(); | ||
|
||
// Set the integrations tab feature flag | ||
self::$show_integrations = apply_filters( 'jetpack_forms_enable_integrations_tab', true ); | ||
} | ||
|
||
/** | ||
|
@@ -226,7 +216,6 @@ public function render_dashboard( $extra_config = array() ) { | |
'siteURL' => ( new Status() )->get_site_suffix(), | ||
'hasFeedback' => $this->has_feedback(), | ||
'hasAI' => $has_ai, | ||
'enableIntegrationsTab' => self::$show_integrations, | ||
'renderMigrationPage' => $this->switch->is_jetpack_forms_announcing_new_menu(), | ||
'dashboardURL' => add_query_arg( 'jetpack_forms_migration_announcement_seen', 'yes', $this->switch->get_forms_admin_url() ), | ||
'isMailpoetEnabled' => Jetpack_Forms::is_mailpoet_enabled(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import { Outlet, useLocation, useNavigate } from 'react-router'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import useFormsConfig from '../../../hooks/use-forms-config'; | ||
import EmptySpamButton from '../../components/empty-spam-button'; | ||
import EmptyTrashButton from '../../components/empty-trash-button'; | ||
import ExportResponsesButton from '../../inbox/export-responses'; | ||
|
@@ -26,8 +27,9 @@ const Layout = () => { | |
const location = useLocation(); | ||
const navigate = useNavigate(); | ||
const [ isSm ] = useBreakpointMatch( 'sm' ); | ||
const formsConfig = useFormsConfig(); | ||
|
||
const enableIntegrationsTab = config( 'enableIntegrationsTab' ); | ||
const enableIntegrationsTab = Boolean( formsConfig?.isIntegrationsEnabled ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting value from form config endpoint and hook, rather than from config object. |
||
|
||
const { currentStatus } = useSelect( | ||
select => ( { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,14 +49,10 @@ window.addEventListener( 'load', () => { | |
path: 'responses', | ||
element: <Inbox />, | ||
}, | ||
...( config( 'enableIntegrationsTab' ) | ||
? [ | ||
{ | ||
path: 'integrations', | ||
element: <Integrations />, | ||
}, | ||
] | ||
: [] ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This change means that the /integrations path will always be present on the forms dashboard. But if the integrations flag is off, we will not show the Integrations TAB/navigation, so the only way to get to this screen is to know and enter the path directly. If we want to go further, I'd say that on the integrations screen, if integrations are turned off, we display a message that integrations are disabled for this site. But I don't personally think that's needed. |
||
{ | ||
path: 'integrations', | ||
element: <Integrations />, | ||
}, | ||
{ | ||
path: 'about', | ||
element: <About />, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: minor | ||
Type: compat | ||
|
||
Forms: Add integrations feature flag. |
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 you are seeing a lot of changes here, try hiding white space on the diff. It's easier to read. We just remove canUserInstallPlugins here since we are using the feature flag instead of this permissions check.