Add banner about updating default email address to email action #2239
Conversation
WalkthroughThis pull request introduces a feature that displays a warning about potential email delivery issues when the 'To' and 'From' addresses are the same. A new conditional block in the view renders this warning along with buttons for acknowledgment and redirection to the email setup. Additionally, new CSS rules enhance the visual styling of the warning message. AJAX functionality is implemented, including an action hook, a new controller method, and a JavaScript event handler, allowing users to dismiss the warning message. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
classes/views/frm-form-actions/_action_inside.php (2)
14-14: Remove empty style attribute.The
<p>tag contains an empty style attribute that should be removed.- <p class="frm10" style=""> + <p class="frm10">
13-22: Improve semantic HTML structure and error handling.The notice should use more semantic HTML elements and include error handling for translation functions.
-<div class="frm_grid_container frm_no_p_margin frm_default_email_notice"> - <p class="frm10"> - <b><?php esc_html_e( 'Heads up!', 'formidable' ); ?></b> - <?php esc_html_e( 'Using the same \'To\' and \'From\' email address can sometimes cause delivery issues. We recommend updating your default email addresses to maximize deliverability.', 'formidable' ); ?> - </p> - <p class="frm2"> +<div class="frm_grid_container frm_no_p_margin frm_default_email_notice" role="alert"> + <div class="frm10"> + <h4 class="frm_warning_heading"> + <?php + $heading = esc_html__( 'Heads up!', 'formidable' ); + if ( false === $heading ) { + $heading = 'Heads up!'; // Fallback if translation fails + } + echo $heading; + ?> + </h4> + <?php + $message = esc_html__( 'Using the same \'To\' and \'From\' email address can sometimes cause delivery issues. We recommend updating your default email addresses to maximize deliverability.', 'formidable' ); + if ( false === $message ) { + $message = 'Please update your email settings to ensure proper delivery.'; // Fallback if translation fails + } + echo $message; + ?> + </div> + <div class="frm2">css/frm_admin.css (1)
5447-5451: Consider adding a hover/focus state for better interactivity.If this notice contains any interactive elements like buttons or links, consider adding hover/focus states to provide visual feedback.
.frm_email_settings .frm_default_email_notice { background-color: var(--primary-25); padding:var(--gap-sm) var(--gap-md); border-radius:var(--small-radius); + transition: background-color 0.2s ease; } +.frm_email_settings .frm_default_email_notice:hover { + background-color: var(--primary-50); +} +.frm_email_settings .frm_default_email_notice:focus-within { + outline: 2px solid var(--primary-500); + outline-offset: 2px; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/views/frm-form-actions/_action_inside.php(1 hunks)css/frm_admin.css(1 hunks)
🔇 Additional comments (4)
classes/views/frm-form-actions/_action_inside.php (2)
10-24: LGTM! Well-placed notice block.The placement of the email notice block at the beginning of the form action settings is appropriate and follows a logical flow.
19-20:⚠️ Potential issueAdd click handlers and improve accessibility for buttons.
The buttons have
href="#"but no JavaScript handlers to process the clicks. Additionally, they need ARIA labels for better accessibility.- <a href="#" class="button frm-button-secondary"><?php esc_html_e( 'Got it', 'formidable' ); ?></a> - <a href="#" class="button frm-button-primary"><?php esc_html_e( 'Setup emails', 'formidable' ); ?></a> + <a href="#" + class="button frm-button-secondary" + onclick="frmDismissEmailNotice(this); return false;" + aria-label="<?php esc_attr_e( 'Dismiss email setup notice', 'formidable' ); ?>"> + <?php esc_html_e( 'Got it', 'formidable' ); ?> + </a> + <a href="#" + class="button frm-button-primary" + onclick="frmShowEmailSetup(this); return false;" + aria-label="<?php esc_attr_e( 'Open email setup configuration', 'formidable' ); ?>"> + <?php esc_html_e( 'Setup emails', 'formidable' ); ?> + </a>Please ensure the JavaScript functions
frmDismissEmailNoticeandfrmShowEmailSetupare defined in your JS files:css/frm_admin.css (2)
5447-5451: LGTM! The styling for the default email notice is well-structured.The new CSS rule for
.frm_email_settings .frm_default_email_noticeuses appropriate variables for consistent styling and follows best practices:
- Uses the primary color variable for background
- Applies consistent padding using gap variables
- Uses border-radius variable for rounded corners
5447-5451: Verify the background color contrast meets accessibility standards.The background color uses
var(--primary-25)which appears to be a light blue. Let's verify that this provides sufficient contrast with the text color for accessibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
2528-2537: LGTM! The implementation looks secure and follows WordPress best practices.The function properly verifies the AJAX nonce before updating user meta. However, the
@sinceversion tag is not defined yet.Update the
@since x.xversion number to the actual version this feature will be released in.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmFormsController.php(1 hunks)classes/controllers/FrmHooksController.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmHooksController.php
🔇 Additional comments (1)
classes/controllers/FrmFormsController.php (1)
2549-2576: LGTM! The validation logic is thorough and handles different action types appropriately.The function properly validates:
- Redirect URLs through the
frm_redirect_urlfilter- Page existence and status for page actions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2239 +/- ##
============================================
+ Coverage 26.82% 26.85% +0.02%
- Complexity 8320 8361 +41
============================================
Files 129 129
Lines 27488 27626 +138
============================================
+ Hits 7375 7418 +43
- Misses 20113 20208 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@AbdiTolesa The buttons don't display well in my laptop. Can you fix it? 
|
|
@AbdiTolesa My last styling issue hasn't been fixed. You can check with the screen width less than 1700px. |
@truongwp I think my new commit fixes that for you. |
|
@AbdiTolesa Now this is what I see on iPad pro: |
@truongwp I originally wanted it to stack vertically if it must be responsive and I think you didn't like that on your laptop size. Should we stop forcing it to be side by side and make it stack vertically based on the screen size? So if there is no enough room to show it side by side (like in your previous comment) even if it is desktop size. The last resort would be defining a new media query in |
|
@AbdiTolesa In my opinion, displaying them vertically doesn't look good. Can you make them horizontal in the bottom of the message? I also want to hear from @tuguirazvan. |
|
@AbdiTolesa Now on small screen, the buttons are horizontally aligned. They should be left aligned. |
@truongwp Sorry I did the left alignment targeting the wrong class in 333a11c It is fixed now. |
|
@AbdiTolesa Only one more thing, the color of |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
classes/controllers/FrmFormActionsController.php (1)
432-445: Update the version placeholder and consider usingget_current_user_id()The
@since x.xshould be updated with the actual version number. Also, consider usingget_current_user_id()instead ofwp_get_current_user()->IDfor better performance and consistency with WordPress coding standards.- * @since x.x + * @since 6.17 /* or whatever the actual version is */Also, in line 444:
- return $form_action->post_excerpt === 'email' && ! get_user_meta( wp_get_current_user()->ID, 'frm_dismiss_default_email_message', true ); + return $form_action->post_excerpt === 'email' && ! get_user_meta( get_current_user_id(), 'frm_dismiss_default_email_message', true );classes/controllers/FrmFormsController.php (1)
2527-2542: Update the version placeholder and add permission checkSimilar to the previous file, the
@since x.xshould be updated with the actual version number.Additionally, while there is a nonce check for CSRF protection, it would be more secure to explicitly check if the user has the permission to edit forms using
current_user_can('frm_edit_forms').- * @since x.x + * @since 6.17 /* or whatever the actual version is */ public static function dismiss_default_email_message() { $permission_error = FrmAppHelper::permission_nonce_error( 'frm_edit_forms', 'nonce', 'frm_ajax' ); if ( $permission_error !== false ) { wp_send_json_error( $permission_error, 403 ); } + if ( ! current_user_can( 'frm_edit_forms' ) ) { + wp_send_json_error( __( 'You do not have permission to perform this action.', 'formidable' ), 403 ); + } update_user_meta( get_current_user_id(), 'frm_dismiss_default_email_message', 1 ); wp_send_json_success(); }js/admin/settings.js (1)
25-39: Add missing semicolon in the .then() callbackThe code for handling the dismiss default email message click event is well structured, but there's a missing semicolon after the callback function in the
.then()method.doJsonPost( 'dismiss_default_email_message', formData ).then( () => { - e.target.closest( '.frm_default_email_message' ).remove(); + e.target.closest( '.frm_default_email_message' ).remove(); }Also, consider enhancing the error handling to provide user feedback rather than just logging to the console.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/controllers/FrmFormActionsController.php(1 hunks)classes/controllers/FrmFormsController.php(1 hunks)classes/views/frm-form-actions/_action_inside.php(1 hunks)js/admin/settings.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/frm-form-actions/_action_inside.php
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: Cypress
🔇 Additional comments (2)
js/admin/settings.js (2)
3-4: Good code structure for importing Ajax functionalityThe explicit import of
doJsonPostfromfrmDom.ajaxis a good practice for clarity and maintainability.
8-8: Well-integrated event listenerThe addition of the click event listener follows the existing pattern in the file, making it consistent with the rest of the code.
Fix https://github.com/Strategy11/formidable-pro/issues/5453
Design: https://www.figma.com/design/QnMV8Njb7k5hDSdgrvNveY/Form-Actions-(Explorations)?node-id=6-207&t=Y9cwZI7PmScrnuVF-0
Test procedure
Got itdismisses the banner and it would never be displayed for that user again, even after form reload.Setup emailsredirects user to the email setup page.