feat: requesting new integration ui to accept users preferences about the new integrations #38012
Conversation
WalkthroughThe pull request introduces several enhancements related to the integration request feature. It adds a new constant for user messages, a feature flag for enabling the integration request functionality, and updates to analytics event names. Additionally, new components are introduced to facilitate the submission of integration requests, including a form for user input and a modal for displaying the request interface. These changes collectively enhance the user experience for requesting new integrations. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Integrations Pod General
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
app/client/src/components/utils/ReduxFormTextField.tsx (1)
33-35: Consider consolidating prop handling logicThe conditional prop spreading and renderAs assignment could be simplified into a single derived props object.
- {...(componentProps.type === "textarea" - ? omit(componentProps, "type") - : componentProps)} - renderAs={componentProps.type === "textarea" ? "textarea" : "input"} + {...(componentProps.type === "textarea" + ? { ...omit(componentProps, "type"), renderAs: "textarea" } + : { ...componentProps, renderAs: "input" })}Also applies to: 42-42
app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx (1)
47-49: Enhance analytics event with additional contextThe analytics event could include more context about where the request was initiated from.
- AnalyticsUtil.logEvent("REQUEST_INTEGRATION_CTA"); + AnalyticsUtil.logEvent("REQUEST_INTEGRATION_CTA", { + source: "datasources_list", + location: "bottom_bar" + });app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/form.tsx (2)
27-35: Consider enhancing form submission handling.The form submission implementation could be improved in the following areas:
- Add error handling for failed submissions
- Include loading state during submission
- Consider debouncing the submit action
const onSubmit = (values: RequestIntegrationFormValues) => { + const [isSubmitting, setIsSubmitting] = useState(false); + try { + setIsSubmitting(true); AnalyticsUtil.logEvent("REQUEST_INTEGRATION_SUBMITTED", { integration_name: values.integration, use_case_description: values.useCase || "", email: values.email, }); + } catch (error) { + // Handle error appropriately + } finally { + setIsSubmitting(false); props.closeModal(); + } };
112-128: Strengthen email validation.The current email validation could be enhanced to handle edge cases.
if (!values.email || !isEmail(values.email)) { + // Add additional validation for common email patterns + const emailRegex = /^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; + if (!emailRegex.test(values.email)) { errors.email = createMessage( REQUEST_NEW_INTEGRATIONS.REQUEST_MODAL_EMAIL.ERROR, ); + } }app/client/src/ce/constants/messages.ts (2)
2560-2565: Consider adding validation message for useCase fieldWhile the integration name and email fields have error messages, the useCase field doesn't have one. For consistency, consider adding an error message:
REQUEST_MODAL_USECASE: { LABEL: () => "Tell us more about your case", PLACEHOLDER: () => "E.g. I want to create an app to manage my customers' account.", NAME: "useCase", + ERROR: () => "Please describe your use case", },
2566-2572: Consider adding email format validation messageThe email field has a basic validation message but doesn't include a specific message for invalid email format. Consider adding one:
REQUEST_MODAL_EMAIL: { LABEL: () => "Email", DESCRIPTION: () => "Appsmith might use this email exclusively to follow up on your integration request.", NAME: "email", ERROR: () => "Please enter email", + FORMAT_ERROR: () => "Please enter a valid email address", },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/ce/utils/analyticsUtilTypes.ts(1 hunks)app/client/src/components/utils/ReduxFormTextField.tsx(2 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(2 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/form.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (1)
Learnt from: sneha122
PR: appsmithorg/appsmith#29377
File: app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx:316-329
Timestamp: 2024-11-12T08:11:25.416Z
Learning: The user clarified that the feature being introduced is experimental and that automated tests will be added once the decision to keep the feature is made. This context is important for future reviews as it explains the absence of tests for certain features under development.
🔇 Additional comments (4)
app/client/src/ce/entities/FeatureFlag.ts (1)
45-45: LGTM! Feature flag follows naming convention
The feature flag is correctly implemented following the documented naming convention for AB testing flags.
Also applies to: 83-83
app/client/src/components/utils/ReduxFormTextField.tsx (1)
53-53: LGTM! Type definition update is correct
The FormTextFieldProps type extension properly includes the new textarea type.
app/client/src/ce/utils/analyticsUtilTypes.ts (1)
355-357: LGTM: Analytics events properly defined.
The new analytics events are correctly added and align with the feature requirements.
app/client/src/ce/constants/messages.ts (1)
2547-2573: LGTM! The message constants are well-structured and organized.
The constants follow the established pattern and provide clear, user-friendly messages for the integration request form.
app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12196800182. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12197323304. |
|
Deploy-Preview-URL: https://ce-38012.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12224710839. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12224760953. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/constants/messages.ts (1)
2571-2571: Update error message for invalid email formatSince the email field is optional, the error message should indicate an invalid email format when provided. Please update the error message accordingly.
Apply this diff to update the error message:
ERROR: () => "Please enter email", + ERROR: () => "Please enter a valid email address",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/form.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx
app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/form.tsx
Outdated
Show resolved
Hide resolved
| if (!values.email || !isEmail(values.email)) { | ||
| errors.email = createMessage( | ||
| REQUEST_NEW_INTEGRATIONS.REQUEST_MODAL_EMAIL.ERROR, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Email field should be optional in validation
The email field is currently mandatory in the validation logic. As per the PR objectives, it should be optional. Please update the validation to reflect this.
Apply this diff to fix the validation logic:
- if (!values.email || !isEmail(values.email)) {
+ if (values.email && !isEmail(values.email)) {
errors.email = createMessage(
REQUEST_NEW_INTEGRATIONS.REQUEST_MODAL_EMAIL.ERROR,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!values.email || !isEmail(values.email)) { | |
| errors.email = createMessage( | |
| REQUEST_NEW_INTEGRATIONS.REQUEST_MODAL_EMAIL.ERROR, | |
| ); | |
| } | |
| if (values.email && !isEmail(values.email)) { | |
| errors.email = createMessage( | |
| REQUEST_NEW_INTEGRATIONS.REQUEST_MODAL_EMAIL.ERROR, | |
| ); | |
| } |
|
Deploy-Preview-URL: https://ce-38012.dp.appsmith.com |
…7854-request-integration
|
Merged this PR. Will update any new design changes later cc @carinanfonseca |
… the new integrations (appsmithorg#38012)
Description
This PR introduces a way to accept user's preferences about adding new types of integrations.
This feature is behind a feature flag :
ab_request_new_integration_enabledThis fires two events :
REQUEST_INTEGRATION_CTA&REQUEST_INTEGRATION_SUBMITTEDFixes #37854
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12231251196
Commit: cb2cf80
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Mon, 09 Dec 2024 08:42:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation