Add Default from address setting#1967
Conversation
WalkthroughThe changes introduce enhancements to the email configuration functionalities within the application. Key modifications include the addition of a new "From Address" input field in the onboarding wizard, updates to email handling methods to utilize a centralized default email retrieval function, and improvements in error handling mechanisms. These alterations collectively aim to provide users with better configurability and validation during email setup processes. Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks @truongwp!
I haven't tested this yet.
I added Sherv as a reviewer since he worked on the onboarding wizard. I figure some extra eyes on this wouldn't hurt.
🚀
|
Thanks @shervElmi and @Crabcyborg. I resolved all the feedback. Please check this again. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
js/src/onboarding-wizard/ui/showError.js (1)
15-16: The changes look good! Consider using template literals for better readability.The addition of the
inputparameter and the updated error display logic improve the flexibility and specificity of error handling.To further enhance the readability of the code, consider using template literals as suggested in the previous review comment:
showFormError(`#${input.id}`, `#${input.nextElementSibling.id}`, type);js/src/onboarding-wizard/events/setupEmailStepButtonListener.js (1)
21-31: Consider simplifying the function usingArray.prototype.every().The function logic is correct and handles multiple email fields as expected. However, you can slightly simplify it by using
Array.prototype.every()instead offorEach().Apply this diff to simplify the function:
-const validateEmails = emailInputs => { - let isValid = true; - emailInputs.forEach( input => { +const validateEmails = emailInputs => { + return emailInputs.every( input => { const emailAddress = input.value.trim(); if ( ! isValidEmail( emailAddress ) ) { showEmailAddressError( 'invalid', input ); - isValid = false; + return false; } + return true; }); - return isValid; };
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- classes/helpers/FrmAppHelper.php (1 hunks)
- classes/views/form-templates/modals/code-from-email-modal.php (1 hunks)
- classes/views/form-templates/modals/leave-email-modal.php (1 hunks)
- classes/views/onboarding-wizard/steps/default-email-address-step.php (1 hunks)
- classes/views/onboarding-wizard/steps/install-formidable-pro-step.php (1 hunks)
- js/formidable_styles.js (1 hunks)
- js/src/onboarding-wizard/events/setupEmailStepButtonListener.js (3 hunks)
- js/src/onboarding-wizard/ui/showError.js (1 hunks)
Additional context used
Biome
js/formidable_styles.js
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (13)
js/src/onboarding-wizard/ui/showError.js (1)
9-12: Documentation looks good!The JSDoc comment block has been updated to accurately reflect the new
inputparameter, including the@sinceand@paramtags. This provides clear documentation for the function's usage and parameters.classes/views/form-templates/modals/code-from-email-modal.php (1)
25-37: LGTM!The refactor enhances the maintainability and readability of the code by centralizing the error handling logic. The
FrmAppHelper::print_setting_errormethod is correctly used to dynamically generate error messages with the appropriate parameters.The code segment is well-structured and follows the best practices.
classes/views/form-templates/modals/leave-email-modal.php (1)
53-64: LGTM!The change enhances the maintainability and readability of the code by centralizing error handling logic. It allows for easier updates and modifications to error messages in the future. The change also streamlines the error display process by using a helper function. The logic and syntax of the changed code segment are correct.
js/src/onboarding-wizard/events/setupEmailStepButtonListener.js (1)
43-46: LGTM!The changes enhance the robustness of the email setup process by validating both the "default email" and "from email" fields using the new
validateEmailsfunction. The early return if either email is invalid is the correct approach to prevent further execution.classes/views/onboarding-wizard/steps/install-formidable-pro-step.php (1)
40-47: LGTM!The refactoring of the error handling logic to use the
FrmAppHelper::print_setting_error()method is a good change. It improves maintainability by centralizing the error handling, allowing for easier updates and consistency across the application.The new implementation still conveys the same error message, so the functionality remains unchanged.
classes/views/onboarding-wizard/steps/default-email-address-step.php (5)
20-20: LGTM!The change in the section title from "Default Email Address" to "Email Setup" is appropriate given the expanded functionality introduced in this diff to configure both the "From" and "To" email addresses.
22-22: LGTM!The revised description accurately reflects the changes made in this diff to allow users to specify both the "From" and "To" email addresses. It provides clarity on the purpose of each email address being configured.
27-39: LGTM!The label change to "From Address" is appropriate and consistent with the expanded functionality.
Using the
FrmAppHelper::print_setting_errorfunction to handle validation errors is a good practice. It improves code maintainability and ensures consistency in error handling across the email fields.
42-42: LGTM!The addition of the "To Address" field aligns with the expanded functionality to allow users to specify both the sender and recipient email addresses. It is a necessary change to support the new feature.
45-55: LGTM!Using the
FrmAppHelper::print_setting_errorfunction to handle validation errors for the "To Address" field is consistent with the error handling approach used for the "From Address" field. It improves code maintainability and ensures a unified error handling mechanism across the email fields.js/formidable_styles.js (2)
Line range hint
1-38: [Needs Verification] Ensure the minified code is thoroughly tested.The AI-generated summary indicates significant changes to the code structure and logic, including:
- Restructuring and renaming of functions and variables
- Introduction of new logic for handling radio button components and sliders
- Enhancements to event handling and UI updates
Given the minified nature of the code, it's crucial to ensure that these changes are thoroughly tested for regressions. Recommend performing comprehensive unit testing, integration testing, and manual QA to verify that the form elements function as expected and the user experience is not negatively impacted.
Tools
Biome
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
1-38: The code changes look good based on the summary. No obvious issues found.Scanned through the minified code and did not find any obvious bugs or inconsistencies with the AI-generated summary of changes. The code appears to align with the described enhancements.
However, due to the low readability of the minified code, there may be subtle issues that are not apparent. Defer to the testing results for the final verdict on correctness.
Tools
Biome
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
classes/helpers/FrmAppHelper.php (1)
4338-4364: Looks good!The new
print_setting_errorfunction is implemented safely:
- It uses
wp_parse_argsto merge the provided$argswith default values.- The output is properly escaped using
esc_attrandesc_htmlto prevent XSS.- It handles both string and array types for the
errorsparameter.Overall, this is a useful utility function to display setting errors. Nice work!
|
@truongwp and @Crabcyborg, just a heads-up: some files will change when the New Add-Ons Page PR is merged. Both are planned for the 6.15 milestone, so we need to ensure that if the New Add-Ons Page is merged first, everything works smoothly with the new changes. |
|
@shervElmi Thansk for letting us know. We will ask Garret to test again after that PR is merged. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
js/formidable_styles.js (1)
Line range hint
1-874: Avoid Committing Minified Code; Include Source Files InsteadThe file
js/formidable_styles.jsappears to be minified JavaScript code. Committing minified code to the repository can hinder code reviews and maintainability. It is recommended to commit the original unminified source code and configure the build process to generate the minified version during deployment or build time.Tools
Biome
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- classes/helpers/FrmAppHelper.php (1 hunks)
- classes/views/frm-settings/general.php (1 hunks)
- js/formidable_styles.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- classes/helpers/FrmAppHelper.php
Additional context used
Biome
js/formidable_styles.js
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 1-1: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (1)
classes/views/frm-settings/general.php (1)
25-31: ****
The following comment from the previous review is still valid and applicable:Approve the addition of the "Default From Address" setting.
The implementation of the new setting is correct and follows WordPress best practices. The use of
esc_html_efor the label andesc_attrfor the input value ensures proper internationalization and security.Suggestion for Improvement:
Consider adding a default value for thefrm_from_emailin the plugin's default settings initialization. This ensures that there is a fallback value if the setting has not been configured yet.
4 participants
Fixes https://github.com/Strategy11/formidable-pro/issues/5147
This PR adds the Default From Address setting in 2 places: the Global settings and onboarding process.
To test:
Site name <fromemail>. The recording below shows how to see theFromheader with Gmail:Screen.Recording.2024-09-05.at.22.58.29.mov
@Crabcyborg I request you because this is more technical to test. You can request Garret if you need.