Use Lite for the payments pages and add coupons tabs#2563
Conversation
Crabcyborg
changed the title
Crabcyborg
changed the title
Crabcyborg
changed the title
…ter so coupons can be prevented in repeaters
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/helpers/FrmAppHelper.php (1)
1-4: Pipeline failure: Fix blank line formatting issue.The static analysis tool reports a
blank_line_before_statementviolation at the beginning of the file. This is likely a coding standards issue that should be addressed to pass the pipeline checks.Based on typical PHP coding standards, ensure proper spacing after the opening PHP tag. The current structure may need adjustment to satisfy the linter.
♻️ Duplicate comments (9)
classes/helpers/FrmFieldsHelper.php (1)
2492-2504: The@since x.xplaceholder needs to be updated before release.This was already noted in a previous review.
js/src/admin/admin.js (1)
909-914: Drag-start limit check should key offui.draggable, notevent.targetUsing
event.target.classList.contains('frm_at_limit')misses the case where the mousedown starts on a child element inside a limited button (the original concern from the earlier review), allowing drags from buttons that should be at their limit. Instead, useui.draggable[0]to decide whether the source is at the limit.- function handleDragStart( event, ui ) { - if ( event.target.classList.contains( 'frm_at_limit' ) ) { + function handleDragStart( event, ui ) { + if ( ui.draggable && ui.draggable[ 0 ].classList.contains( 'frm_at_limit' ) ) { showLimitModal(); return false; }This reliably blocks drags whenever the draggable button itself is marked
frm_at_limit, regardless of which child was clicked.stripe/controllers/FrmTransLiteListsController.php (1)
118-134: Well-structured coupons routing with extensibility.The implementation correctly handles the coupons action with proper permission checks and provides an extensible routing mechanism via the
frm_trans_lite_routefilter.However, the version placeholder
x.xat line 126 still needs to be updated before release (previously flagged for similar placeholders in FrmTransLiteAppHelper.php).stripe/helpers/FrmTransLiteAppHelper.php (1)
604-621: Clean feature detection and proper deprecation.The new
payments_submodule_or_paypal_is_active()method provides clear feature detection, andshould_fallback_to_paypal()is properly deprecated with a deprecation notice.The version placeholders (
x.xat lines 605, 614, 619) need to be updated before release, as previously noted.stripe/helpers/FrmTransLiteListHelper.php (5)
183-195: LGTM: Conditional bulk actions.The method correctly gates bulk delete actions based on the presence of payment addon helpers, aligning with the Lite vs full feature distinction.
Version placeholder at line 184 needs updating before release.
287-296: LGTM: Feature detection and dynamic column rendering.The
payments_addon_list_helper_exists()helper provides clean feature detection. Theget_cb_column()method is called dynamically via theget_column_value()pattern at line 306 (method name constructed as'get_' . $column_name . '_column'), so the static analysis warning is a false positive.Version placeholders at lines 290 and 379 need updating before release.
Also applies to: 375-387
404-423: LGTM: Improved link generation encapsulation.The refactoring centralizes payment link generation through
get_view_payment_link()andget_url_to_payment(), improving maintainability and reducing duplication.Version placeholder at line 410 needs updating.
456-456: LGTM: Clean edit link feature detection.The
supports_edit_link()method properly encapsulates the logic for determining edit link availability, excluding subscriptions and checking for the presence of required controllers.Version placeholder at line 467 needs updating.
Also applies to: 466-477
648-663: LGTM: Conditional tabs rendering.The
render_tabs()method properly gates the new coupons feature behind permissions and a feature flag. The$active_tabparameter is used by the included view file (tabs.php) via variable scope, so the static analysis warning is a false positive.Version placeholder at line 649 needs updating before release.
🧹 Nitpick comments (4)
classes/helpers/FrmAppHelper.php (1)
4662-4685: Consider adding json_encode error handling.The method logic is solid, but there's a minor concern on line 4670 where
json_encode( $link )is called without error handling. While unlikely to fail in this context, encoding errors could occur if$linkcontains invalid UTF-8 sequences or if there are circular references.Consider adding error handling:
if ( ! empty( $link['url'] ) && self::pro_is_installed() ) { - $params['oneclick'] = json_encode( $link ); + $encoded = json_encode( $link ); + if ( false !== $encoded ) { + $params['oneclick'] = $encoded; + } unset( $params['message'] );Alternatively, you could use
wp_json_encode()which provides some additional safeguards, though in this case the difference is minimal.js/src/admin/admin.js (3)
1916-1937: Repeater drop restrictions and filter hook look good; tidy up JSDoc and signature nowThe new
droppableIsARepeater/droppableIsInsideOfARepeaterchecks plus the GDPR guard andfrm_deny_drop_in_repeaterfilter are a sensible way to centralize repeater‑specific restrictions, and skipping the group‑size check when dropping on a repeater section is consistent with the rest ofallowDrop. No functional issues stand out.Two small follow‑ups:
- The JSDoc block for
frm_deny_drop_in_repeatercurrently has@since x.xand is tripping ESLint (“Expected JSDoc block lines to be aligned”). Before release, set the real version and align the*/@lines to satisfy the linter.- If you think consumers of
frm_deny_drop_in_repeatermight ever need the droppable context, consider adding it to the hook signature now (e.g.applyFilters( 'frm_deny_drop_in_repeater', false, draggable, droppable )) while it’s still easy to change.
2241-2257:showLimitModalcorrectly reuses the confirm modal and hides the confirm actionThe temporary anchor +
confirmLinkClickreuse pattern is a clean way to surface the limit message, and explicitly hiding#frm-confirmed-clickensures the dialog is informational only. Combined with theconfirmModalchange that restores the button’s display on the next open, this keeps other confirm flows intact.If you later add additional confirm dialogs, consider tagging the limit modal instance (e.g. a class on
#frm_confirm_modal) instead of relying solely on toggling the button’sdisplayto make the state more explicit, but this isn’t blocking.
5244-5273: Re-enabling limited field buttons after delete is correct but timing-sensitiveHooking
maybeEnableFieldButtonAtLimit( $thisField.data( 'type' ) )indeleteField’s success handler and computingcountFieldTypeInForm( type ) - 1(because the DOM node hasn’t been removed yet) correctly clearsfrm_at_limitonce the count drops below the button’sdata-limit. The implementation is safe given the current ordering (helper called beforefadeOut’s removal callback).Just be aware that this helper assumes it runs before the field is actually removed from
#frm-show-fields; if you ever refactor the delete flow, you may want to move the call into the fadeOut callback and drop the- 1adjustment to make it less order‑dependent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
images/icons.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (12)
bin/zip-plugin.sh(1 hunks)classes/controllers/FrmAppController.php(2 hunks)classes/helpers/FrmAppHelper.php(1 hunks)classes/helpers/FrmFieldsHelper.php(2 hunks)classes/models/FrmField.php(2 hunks)js/src/admin/admin.js(8 hunks)stripe/controllers/FrmStrpLiteAppController.php(1 hunks)stripe/controllers/FrmTransLiteEntriesController.php(0 hunks)stripe/controllers/FrmTransLiteListsController.php(2 hunks)stripe/controllers/FrmTransLitePaymentsController.php(1 hunks)stripe/helpers/FrmTransLiteAppHelper.php(2 hunks)stripe/helpers/FrmTransLiteListHelper.php(8 hunks)
💤 Files with no reviewable changes (1)
- stripe/controllers/FrmTransLiteEntriesController.php
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/controllers/FrmAppController.php
- classes/models/FrmField.php
🧰 Additional context used
🧬 Code graph analysis (4)
classes/helpers/FrmFieldsHelper.php (2)
js/src/admin/admin.js (1)
limit(7907-7907)classes/models/FrmDb.php (2)
FrmDb(6-870)get_count(231-235)
stripe/controllers/FrmStrpLiteAppController.php (1)
stripe/helpers/FrmTransLiteAppHelper.php (2)
FrmTransLiteAppHelper(6-622)payments_table_exists(39-56)
stripe/controllers/FrmTransLiteListsController.php (1)
stripe/helpers/FrmTransLiteAppHelper.php (2)
FrmTransLiteAppHelper(6-622)plugin_path(11-13)
stripe/controllers/FrmTransLitePaymentsController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5092)simple_get(815-824)
🪛 GitHub Actions: Inspections
stripe/helpers/FrmTransLiteListHelper.php
[error] 1-1: phpdoc_separation, blank_line_before_statement
stripe/controllers/FrmTransLiteListsController.php
[error] 1-1: blank_line_before_statement
classes/helpers/FrmAppHelper.php
[error] 1-1: blank_line_before_statement
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 1925-1925:
Expected JSDoc block lines to be aligned
🪛 PHPMD (2.15.0)
stripe/helpers/FrmTransLiteListHelper.php
385-387: Avoid unused private methods such as 'get_cb_column'. (undefined)
(UnusedPrivateMethod)
654-654: Avoid unused parameters such as '$active_tab'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (14)
classes/helpers/FrmAppHelper.php (1)
4638-4649: LGTM! Coupons feature flag logic is appropriate.The addition of the coupons-specific check using
class_exists( 'FrmCouponsAppController' )is a clean approach for feature detection. The method correctly returns true when the feature should be shown based on either the install_link status or the presence of the coupons controller class.bin/zip-plugin.sh (1)
105-106: LGTM!The exclusion of
formidable-coupons/js/frontend.jsandformidable-coupons/js/admin.jsfollows the established pattern for other addon JS source files (e.g.,formidable-hubspot/js/admin.js).stripe/controllers/FrmTransLitePaymentsController.php (1)
17-24: LGTM!The addition of
'bulk_delete'to the action list ensures that bulk delete operations are correctly routed through the PayPal addon when active. Theis_callablecheck provides a safe fallback.stripe/controllers/FrmStrpLiteAppController.php (1)
66-82: LGTM!The refactoring into segmented early-return guards improves readability. The new guard for
FrmPaymentsControllercorrectly prevents unwanted redirects when the PayPal add-on is active, allowing it to handle its own routing.classes/helpers/FrmFieldsHelper.php (2)
2455-2457: LGTM!The conditional correctly gates the limit-check logic to only run when the field is not an upgrade UI and has a defined limit, preventing unnecessary database queries.
2505-2521: LGTM!The implementation correctly counts existing fields of the specified type within the form using
FrmDb::get_count()and applies thefrm_at_limitclass when the limit is reached. Thedata-limitattribute enables JavaScript to enforce limits client-side.js/src/admin/admin.js (3)
332-336: Confirm button visibility reset looks correctRe‑showing
#frm-confirmed-clickon everyconfirmModalopen prevents it staying hidden aftershowLimitModal()hides it; this keeps other confirm flows working as expected.
2193-2200: Click guard for field-type limits is consistent with modal behaviorShort‑circuiting
addFieldClickwhen$button.hasClass('frm_at_limit')and showingshowLimitModal()(withreturn falseto suppress the link’s default) correctly mirrors the drag‑start guard and prevents further insertions via sidebar clicks once a type has reached its limit.
2854-2879: Per-type button disabling on reaching the field limit is straightforwardCalling
maybeDisableFieldButtonAtLimit( type )fromafterAddFieldand implementing it as a simpledata-limit+countFieldTypeInFormcheck is clear and self‑contained. The guard onbutton && button.dataset.limitmakes it safe for types without limits, and the logic will behave as expected as long as the builder markup keeps#frm-show-fields li.form-field[data-ftype="{type}"]in sync with the sidebar button IDs.stripe/controllers/FrmTransLiteListsController.php (1)
72-74: LGTM: Conditional mode column rendering.The logic correctly adds the mode column only in appropriate contexts by checking the action and the presence of specific list helper classes.
stripe/helpers/FrmTransLiteAppHelper.php (1)
78-78: LGTM: Safer status check.The change from
if ( $payment->status )toif ( ! empty( $payment->status ) )is more explicit and avoids potential issues with falsy but valid values.stripe/helpers/FrmTransLiteListHelper.php (3)
276-285: LGTM: Consistent checkbox column gating.Removing the checkbox column for Lite builds (when payment addons aren't present) is consistent with the conditional bulk actions logic.
425-441: LGTM: Centralized URL generation.The
get_url_to_payment()helper properly encapsulates payment URL generation, ensuring consistency across the list UI.
576-576: LGTM: Safer status check.Using the null coalescing operator prevents undefined property warnings when the status property doesn't exist.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/admin/admin.js (1)
323-355: Confirm modal button reset is good, but staledata-*attributes are never clearedThe new block that forces
continueButton.style.display = 'block'when opening the confirm modal is a good fix so the button isn’t permanently hidden aftershowLimitModal()hides it. However, in the same function the attribute‑clearing loop appears to use the wrong variable:removeAtts = continueButton.dataset; for ( i in dataAtts ) { continueButton.removeAttribute( 'data-' + i ); } dataAtts = link.dataset;This never iterates over
removeAtts, so olddata-*attributes on the confirm button can linger across uses.Consider:
- removeAtts = continueButton.dataset; - for ( i in dataAtts ) { - continueButton.removeAttribute( 'data-' + i ); - } - - dataAtts = link.dataset; + removeAtts = continueButton.dataset; + for ( i in removeAtts ) { + continueButton.removeAttribute( 'data-' + i ); + } + + dataAtts = link.dataset;This preserves your new visibility fix while ensuring each open starts from a clean
data-*state.
♻️ Duplicate comments (1)
js/src/admin/admin.js (1)
909-914: Drag-start limit check should useclosest('.frm_at_limit')on the event targetRight now the drag guard only fires if the direct
event.targethasfrm_at_limit, which will often be a child inside the button (e.g.,<span>,<svg>). Those drags won’t be blocked.To reliably intercept drags on a limited field button and avoid errors when
event.targetisn’t an Element, useclosestwith a guard:function handleDragStart( event, ui ) { - if ( event.target.classList.contains( 'frm_at_limit' ) ) { - showLimitModal(); - return false; - } + const atLimitEl = event.target && event.target.closest && event.target.closest( '.frm_at_limit' ); + if ( atLimitEl ) { + showLimitModal(); + return false; + }This matches the intent from the earlier review and ensures all clicks within a limited button are caught.
🧹 Nitpick comments (4)
js/src/admin/admin.js (4)
1916-1938: Repeater drop restrictions and hook look correct; consider small polishThe new logic to:
- Detect repeater context (
droppableIsARepeater/droppableIsInsideOfARepeater),- Block GDPR fields in repeaters, and
- Expose a
frm_deny_drop_in_repeaterfilteris consistent and will effectively prevent unsupported drops into repeaters.
Minor follow‑ups you might consider:
- Explicitly coerce the filter’s return value to boolean to be defensive against truthy strings:
- const shouldDenyDropInRepeater = wp.hooks.applyFilters( 'frm_deny_drop_in_repeater', false, draggable ); - if ( shouldDenyDropInRepeater ) { + const shouldDenyDropInRepeater = !! wp.hooks.applyFilters( + 'frm_deny_drop_in_repeater', + false, + draggable + ); + if ( shouldDenyDropInRepeater ) { return false; }
- Fix the ESLint JSDoc alignment warning on the
@sinceblock above this filter to keep checks green (align the*columns with the opening/**). This is purely cosmetic.
2243-2260:showLimitModal()works, but you could avoid the temporary anchor indirection
showLimitModal()correctly:
- Reuses the existing confirm modal UI to display a “field type has reached its limit” message, and
- Hides the confirm button afterward so it’s informational-only.
The temporary anchor click approach is a clever reuse, but slightly indirect. If you ever want to simplify, you could:
- Call
confirmModal()(orconfirmLinkClick()) directly with a constructed link-like object, or- Expose a small helper like
openConfirmModalWithMessage(message)that internally wires up the confirm modal without needing to inject a DOM node.Not urgent, just an opportunity to reduce DOM churn and indirection later.
2857-2882: Button disabling on reaching a per-type limit is sound; consider explicit number coercionHooking
maybeDisableFieldButtonAtLimit( type )fromafterAddField()is a good place to enforce per‑type limits. The logic of:
- Finding the button by id (
type),- Checking
button.dataset.limit, and- Comparing
countFieldTypeInForm( type )against that limitis correct.
For clarity and to avoid any surprises from string comparison, you might coerce
dataset.limitonce:function maybeDisableFieldButtonAtLimit( type ) { const button = document.getElementById( type ); - if ( button && button.dataset.limit && countFieldTypeInForm( type ) >= button.dataset.limit ) { + if ( ! button || ! button.dataset.limit ) { + return; + } + + const limit = parseInt( button.dataset.limit, 10 ); + if ( Number.isFinite( limit ) && countFieldTypeInForm( type ) >= limit ) { button.classList.add( 'frm_at_limit' ); } }This keeps behavior the same while making the intent explicit.
5247-5276: Re-enabling limited field buttons after delete is correct; small safety guard suggestedCalling
maybeEnableFieldButtonAtLimit( $thisField.data( 'type' ) )after a successfulfrm_delete_fieldis the right place to re-enable buttons when counts drop below the limit. ThecountFieldTypeInForm( type ) - 1adjustment accounts for the still-present DOM node during the fade-out.Two small improvements to consider:
- Guard for missing builder DOM so this helper is safe if
deleteField()is ever reused outside the builder:function countFieldTypeInForm( type ) { - return document.getElementById( 'frm-show-fields' ).querySelectorAll( - 'li.form-field[data-ftype="' + type + '"]' - ).length; + const container = document.getElementById( 'frm-show-fields' ); + if ( ! container ) { + return 0; + } + return container.querySelectorAll( + 'li.form-field[data-ftype="' + type + '"]' + ).length; }
- As with the disable path, parse
button.dataset.limitonce to a number before comparing, to avoid implicit coercion.Behavior stays the same but the helpers become more robust and self‑documenting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (1)
js/src/form/modal.js (1)
wp(5-5)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 1925-1925:
Expected JSDoc block lines to be aligned
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
js/src/admin/admin.js (1)
2193-2202: Click guard when at field-type limit behaves correctlyThe new guard in
addFieldClick():
- Checks
$button.hasClass( 'frm_at_limit' ),- Shows
showLimitModal(), and- Returns
falseto suppress the default anchor behavior,which fixes the earlier issue where the link still navigated when at limit. The explanatory comment around
shouldStopInsertingField()is also helpful for future readers. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stripe/views/lists/coupons.php (1)
25-42: Replace placeholder version numbers before release.The
@since x.xplaceholders on lines 10 and 32 should be replaced with the actual version number before this PR is merged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
classes/controllers/FrmAppController.php(2 hunks)classes/helpers/FrmAppHelper.php(1 hunks)stripe/controllers/FrmTransLiteListsController.php(2 hunks)stripe/helpers/FrmTransLiteListHelper.php(8 hunks)stripe/views/lists/coupons.php(1 hunks)stripe/views/lists/tabs.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/controllers/FrmAppController.php
- classes/helpers/FrmAppHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
stripe/helpers/FrmTransLiteListHelper.php (1)
stripe/helpers/FrmTransLiteAppHelper.php (2)
FrmTransLiteAppHelper(6-622)plugin_path(11-13)
🪛 PHPMD (2.15.0)
stripe/helpers/FrmTransLiteListHelper.php
386-388: Avoid unused private methods such as 'get_cb_column'. (undefined)
(UnusedPrivateMethod)
657-657: Avoid unused parameters such as '$active_tab'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (15)
stripe/views/lists/tabs.php (2)
6-15: LGTM! Clean tab structure with proper escaping and localization.The tabs array is well-structured with clear URL and label definitions for both payments and coupons tabs. Proper use of
admin_url(),__()for i18n, and action-based routing.
18-40: No issue found —$active_tabis properly available through the function parameter scope.The variable is passed as a parameter to
render_tabs($active_tab = 'payments')instripe/helpers/FrmTransLiteListHelper.php, and when the template is included within that function, it automatically has access to all function-scope variables. The template also defines$tabslocally, so both variables are available.Likely an incorrect or invalid review comment.
stripe/views/lists/coupons.php (1)
7-23: LGTM! Proper header rendering with extensibility hook.The admin header is correctly configured with label, form parameter, and publish flag sourced from the
frm_coupons_list_buttonfilter for extensibility.stripe/controllers/FrmTransLiteListsController.php (2)
118-135: LGTM! Clean routing with proper permission checks and extensibility.The new coupons route handling includes:
- Proper permission check using
frm_change_settings- Clean include path construction
- Early return to prevent further processing
- Extensible routing via
frm_trans_lite_routefilter with fallback todisplay_list()This follows good patterns for WordPress admin routing and maintains security.
72-74: Clarify the Mode column conditional logic - it differs from addon presence checks elsewhere in code.The Mode column is added only when:
- Action is not 'bulk_delete' AND
FrmTransListHelperdoes NOT exist ANDFrmPaymentsListHelperexistsThis conditional logic is inconsistent with the
payments_addon_list_helper_exists()helper method in FrmTransLiteListHelper.php, which checks for addon presence using OR logic:class_exists( 'FrmTransListHelper' ) || class_exists( 'FrmPaymentsListHelper' ). The Mode column's AND condition is significantly narrower and will only appear when the Payments addon is active with a specific class presence pattern. Confirm whether this is intentional for version-specific compatibility or if it should align with the helper method's pattern.stripe/helpers/FrmTransLiteListHelper.php (10)
183-196: LGTM! Bulk actions properly gated by addon presence.The
get_bulk_actions()method correctly returns a bulk_delete action only when the Payments addon list helper exists, ensuring bulk operations are only available in the appropriate context.
277-286: LGTM! Conditional checkbox column removal for Lite context.The modified
get_column_info()correctly removes the checkbox column when the Payments addon list helper doesn't exist, ensuring bulk action checkboxes only appear when bulk operations are supported.
288-297: LGTM! Clear helper method for detecting addon presence.The
payments_addon_list_helper_exists()method provides a clean abstraction for checking whether the Payments submodule (Stripe, Authorize.Net) or PayPal is active.
376-388: LGTM! Checkbox column correctly implements WP_List_Table pattern.The
get_cb_column()method follows the WordPress list table convention where column methods are called via magic naming (get_{$column}_column). The static analysis warning about it being unused is a false positive.
405-425: LGTM! Centralized link generation improves maintainability.The refactored
get_action_column()and newget_view_payment_link()method centralize URL generation logic, reducing code duplication and making future changes easier to maintain.
427-443: LGTM! Consolidated URL builder with proper escaping.The
get_url_to_payment()method centralizes payment URL generation with consistent parameter handling. The method supports both 'show' and 'edit' actions and properly usesadmin_url()andadd_query_arg().
450-466: LGTM! Row actions respect edit capability.The modified
get_row_actions()method now conditionally includes the Edit action based onsupports_edit_link(), ensuring edit links only appear when the necessary controllers are available.
468-479: LGTM! Edit link support properly scoped.The
supports_edit_link()method correctly determines when edit functionality is available by:
- Excluding subscriptions (read-only)
- Checking for the presence of
FrmTransAppControllerorFrmPaymentsController
575-583: LGTM! Defensive null coalescing prevents undefined property warnings.The null coalescing operator on line 578 (
$item->status ?? '') prevents potential undefined property notices whenstatusis not set, improving code robustness.
650-666: Code correctly gates rendering with capability and feature flag integration.The
render_tabs()method implements proper access control through:
frm_change_settingscapability check (consistent with codebase usage patterns)FrmAppHelper::show_new_feature('coupons')feature flagThe
$active_tabparameter is correctly extracted for use in the included template (tabs.php, line 22:$is_active = $tab_key === $active_tab;), so the static analysis warning about unused parameters is a false positive. Integration and capability usage follow established patterns throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
stripe/helpers/FrmTransLiteAppHelper.php (1)
599-617: Version placeholders need updating before release.The
@since x.xand@deprecated x.xannotations, as well as the second argument to_deprecated_function(), still contain placeholder version strings.
🧹 Nitpick comments (2)
classes/helpers/FrmAppHelper.php (1)
4623-4657: LGTM! Well-structured method with proper edge case handling.The new
get_upgrade_data_paramsmethod is well-implemented:
- Properly checks for empty
$linkbefore proceeding- Safely accesses array keys with appropriate conditionals
- Correctly uses the null coalescing operator on line 4649
- Handles both Pro and non-Pro scenarios appropriately
Optional: Consider adding type hints for better type safety.
-public static function get_upgrade_data_params( $plugin, $params, $detailed = false ) { +public static function get_upgrade_data_params( string $plugin, array $params, bool $detailed = false ): array {This would provide better IDE support and catch type-related issues earlier, though it's not critical for functionality.
stripe/helpers/FrmTransLiteListHelper.php (1)
179-181: Update version placeholders before release.Multiple
@since x.xannotations throughout this file (lines 180, 287, 376, 407, 423, 465, 645) need to be updated to the actual release version number.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
classes/controllers/FrmAppController.php(2 hunks)classes/helpers/FrmAppHelper.php(1 hunks)classes/helpers/FrmFieldsHelper.php(2 hunks)classes/models/FrmField.php(2 hunks)stripe/helpers/FrmTransLiteAppHelper.php(2 hunks)stripe/helpers/FrmTransLiteListHelper.php(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/models/FrmField.php
- classes/controllers/FrmAppController.php
- classes/helpers/FrmFieldsHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
stripe/helpers/FrmTransLiteListHelper.php (1)
stripe/helpers/FrmTransLiteAppHelper.php (1)
FrmTransLiteAppHelper(6-618)
🪛 PHPMD (2.15.0)
stripe/helpers/FrmTransLiteListHelper.php
382-384: Avoid unused private methods such as 'get_cb_column'. (undefined)
(UnusedPrivateMethod)
651-651: Avoid unused parameters such as '$active_tab'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (9)
classes/helpers/FrmAppHelper.php (1)
4609-4621: LGTM! Clean feature gating for coupons.The addition of the coupons-specific check is well-implemented. Using
class_exists('FrmCouponsAppController')is the appropriate way to gate this feature based on the presence of the Coupons add-on.stripe/helpers/FrmTransLiteAppHelper.php (1)
78-83: LGTM!The change from a truthy check to
!empty($payment->status)is more defensive and correctly handles edge cases where status might be an empty string or0.stripe/helpers/FrmTransLiteListHelper.php (7)
179-192: LGTM!The bulk actions implementation correctly gates the delete action behind the presence of the payments addon list helper, preventing Lite users from accessing functionality they can't use.
273-293: LGTM with minor note.The conditional removal of the checkbox column for Lite-only contexts is appropriate. However, note the
@since x.xplaceholder at line 287 needs to be updated before release.
372-384: Static analysis false positive - method is used via dynamic dispatch.PHPMD flags
get_cb_column()as unused, but this is a false positive. The method is invoked dynamically byget_column_value()(line 305-306) when rendering thecbcolumn, following the same pattern as other column methods likeget_receipt_id_column()andget_item_id_column().
401-439: LGTM!Good refactoring to centralize URL generation with
get_url_to_payment()and link rendering withget_view_payment_link(). The proper use ofesc_url()andesc_html()ensures safe output.
569-577: LGTM!The null coalescing operator at line 572 is a good defensive coding practice that prevents potential undefined property warnings.
643-660: Static analysis false positive -$active_tabis used in the included view.PHPMD flags
$active_tabas unused, but this is a false positive. The variable is in scope whentabs.phpis included at line 658 and is used by that view to render the active tab state.The permission check and feature flag gating are appropriate.
464-475: Verify whether the class_exists autoload parameter difference is intentional.Both
supports_edit_link()andpayments_submodule_or_paypal_is_active()check for the same controller classes (FrmTransAppControllerandFrmPaymentsController), but use different autoload parameters:supports_edit_link()usesclass_exists(..., false)to check only already-loaded classes, whilepayments_submodule_or_paypal_is_active()uses the default autoload behavior. Since both methods serve similar gating purposes, this inconsistency could lead to different behavior depending on class loading order. Either align the autoload parameter usage, or document why they differ.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
classes/models/FrmField.php (1)
363-363: Update the placeholder release date before merging.The release date
'2026-01-13'is still a placeholder in the future. This will cause the "NEW" badge to display on the coupon field until April 13, 2026 (90 days after the release date). This should be updated to the actual release date when the feature ships.Note: A previous reviewer flagged the same issue with date
'2026-01-01'.stripe/helpers/FrmTransLiteAppHelper.php (1)
605-613: Verify migration from deprecated function.This deprecation represents a significant behavioral change—the function previously contained complex fallback logic and now simply returns
false. Confirm that all call sites have been updated to usepayments_submodule_or_paypal_is_active()or otherwise no longer rely on the old behavior.Note: The version placeholder issue (
x.x) has already been flagged in a previous review.#!/bin/bash # Description: Find all usages of the deprecated should_fallback_to_paypal function. # Search for calls to the deprecated function echo "=== Calls to deprecated should_fallback_to_paypal ===" rg -nP '\bshould_fallback_to_paypal\s*\(' --type=php -C3 # Check for any conditional logic based on this function echo -e "\n=== Conditional logic using should_fallback_to_paypal ===" rg -nP 'if\s*\([^)]*should_fallback_to_paypal' --type=php -C2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmAppController.php(2 hunks)classes/helpers/FrmAppHelper.php(1 hunks)classes/helpers/FrmFieldsHelper.php(2 hunks)classes/models/FrmField.php(2 hunks)stripe/helpers/FrmTransLiteAppHelper.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/controllers/FrmAppController.php
- classes/helpers/FrmFieldsHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmField.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5054)show_new_feature(4608-4616)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Psalm
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (4)
classes/helpers/FrmAppHelper.php (2)
4610-4615: LGTM!The feature gating logic is well-structured. The dual check approach (install link status/class OR coupons-specific controller check) provides appropriate flexibility for different addon states.
4629-4652: LGTM!The upgrade data enhancement logic is solid. The method properly handles both Pro-installed and Lite scenarios, embedding appropriate data for each case.
classes/models/FrmField.php (1)
331-340: LGTM!The coupon field configuration follows the established pattern for pro fields. The feature gating via
show_new_feature('coupons'), placement in the 'pricing' section, and limit of 1 per form are all appropriate for this field type.stripe/helpers/FrmTransLiteAppHelper.php (1)
78-78: LGTM: More defensive status check.The change from
if ( $payment->status )toif ( ! empty( $payment->status ) )is a better defensive check that explicitly handles edge cases like empty strings, null, and false values more consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/helpers/FrmAppHelper.php (1)
4629-4652: Strengthen type checking for$linkvariable.The method checks
if ( ! $link )at line 4632 but then assumes$linkis an array in multiple places (lines 4636, 4644, 4648) without explicit type verification. Ifinstall_link()returns a truthy non-array value (e.g., a string or integer), accessing array keys will cause errors.Additionally,
json_encode()at line 4637 can returnfalseon failure, but the code doesn't handle this case.🔎 Suggested fix
public static function get_upgrade_data_params( $plugin, $params, $detailed = false ) { $link = FrmAddonsController::install_link( $plugin ); - if ( ! $link ) { + if ( ! $link || ! is_array( $link ) ) { return $params; } if ( ! empty( $link['url'] ) && self::pro_is_installed() ) { - $params['oneclick'] = json_encode( $link ); + $encoded = json_encode( $link ); + if ( false !== $encoded ) { + $params['oneclick'] = $encoded; + } unset( $params['message'] ); if ( ! isset( $params['medium'] ) ) { $params['medium'] = $plugin; } } else { $params['requires'] = $params['requires'] ?? FrmFormsHelper::get_plan_required( $link ); } if ( $detailed ) { $params['plugin-status'] = $link['status'] ?? ''; } return $params; }
🧹 Nitpick comments (1)
stripe/helpers/FrmTransLiteListHelper.php (1)
423-439: Add a fallback for the page parameter.If
FrmAppHelper::simple_get('page')returns an empty value when the page parameter is not set, the generated URL could be malformed. Consider adding a fallback to 'formidable-payments' to ensure URLs are always valid.🔎 Suggested improvement
private function get_url_to_payment( $payment_id, $action = 'show' ) { + $page = FrmAppHelper::simple_get( 'page' ); + if ( empty( $page ) ) { + $page = 'formidable-payments'; + } + return add_query_arg( array( 'action' => $action, 'id' => $payment_id, 'type' => $this->table, - 'page' => FrmAppHelper::simple_get( 'page' ), + 'page' => $page, ), admin_url( 'admin.php' ) ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/helpers/FrmAppHelper.phpclasses/models/FrmField.phpstripe/helpers/FrmTransLiteListHelper.php
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmField.php
🧰 Additional context used
🧬 Code graph analysis (2)
stripe/helpers/FrmTransLiteListHelper.php (2)
classes/helpers/FrmListHelper.php (2)
get_bulk_actions(356-358)row_actions(500-520)stripe/helpers/FrmTransLiteAppHelper.php (2)
FrmTransLiteAppHelper(6-614)plugin_path(11-13)
classes/helpers/FrmAppHelper.php (2)
classes/controllers/FrmSMTPController.php (1)
link(82-85)js/src/admin/admin.js (2)
link(5612-5613)link(9439-9439)
🪛 PHPMD (2.15.0)
stripe/helpers/FrmTransLiteListHelper.php
382-384: Avoid unused private methods such as 'get_cb_column'. (undefined)
(UnusedPrivateMethod)
650-650: Avoid unused parameters such as '$active_tab'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (6)
stripe/helpers/FrmTransLiteListHelper.php (6)
179-192: LGTM! Clean feature-gating logic.The bulk actions are appropriately restricted to when the Payments addon is active, and the helper method uses class existence checks to detect the addon. This is consistent with the conditional removal of the checkbox column in
get_column_info().Also applies to: 284-293
372-384: LGTM! Static analysis warning is a false positive.The
get_cb_column()method follows the WordPress list table pattern where column methods are called dynamically based on the column name (e.g.,get_{column_name}_column()). When the 'cb' column is present in the columns array (which happens when the Payments addon exists), this method will be invoked automatically by the parent class's column rendering system.
401-421: LGTM! Clean refactoring.The extraction of link generation logic into
get_view_payment_link()and the centralization of URL building inget_url_to_payment()improves code organization and maintainability. Proper escaping is in place.
454-457: LGTM! Well-structured edit link visibility logic.The
supports_edit_link()method appropriately gates the edit functionality based on the table type (no edit for subscriptions) and the availability of required controller classes. Usingclass_exists(..., false)is good for performance as it avoids unnecessary autoloading.Also applies to: 464-475
568-576: LGTM! Good defensive coding.The null coalescing operator (
??) prevents potential undefined property warnings for$item->status. This is a good defensive programming practice.
643-659: The$active_tabparameter is actively used in the template at line 22 ($is_active = $tab_key === $active_tab), where it determines which tab should be highlighted as active. No changes needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stripe/helpers/FrmTransLiteListHelper.php (1)
647-663: Consider clarifying the$active_tabparameter usage.The
$active_tabparameter is declared but never used within the method body and is not passed to the included view file. Consider one of the following:
- If the view file needs this variable, make it available via
compact()or extract before the include- If the view determines the active tab from request parameters, remove the unused parameter
- If this is for future extensibility, add a comment explaining the intent
Option 1: Pass to view if needed
public static function render_tabs( $active_tab = 'payments' ) { if ( ! current_user_can( 'frm_change_settings' ) ) { // Coupons are only available to people who can access global settings. return; } if ( FrmAppHelper::show_new_feature( 'coupons' ) ) { + // Make $active_tab available to the view. include FrmTransLiteAppHelper::plugin_path() . '/views/lists/tabs.php'; } }Note: Variables in the current scope are automatically available to included files, so no explicit passing is needed unless you want to be more explicit about the contract.
Option 2: Remove if truly unused
- public static function render_tabs( $active_tab = 'payments' ) { + public static function render_tabs() { if ( ! current_user_can( 'frm_change_settings' ) ) { // Coupons are only available to people who can access global settings. return; } if ( FrmAppHelper::show_new_feature( 'coupons' ) ) { include FrmTransLiteAppHelper::plugin_path() . '/views/lists/tabs.php'; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stripe/helpers/FrmTransLiteListHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
stripe/helpers/FrmTransLiteListHelper.php (1)
stripe/helpers/FrmTransLiteAppHelper.php (3)
FrmTransLiteAppHelper(6-614)get_gateways(517-519)plugin_path(11-13)
🪛 PHPMD (2.15.0)
stripe/helpers/FrmTransLiteListHelper.php
383-385: Avoid unused private methods such as 'get_cb_column'. (undefined)
(UnusedPrivateMethod)
654-654: Avoid unused parameters such as '$active_tab'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (11)
stripe/helpers/FrmTransLiteListHelper.php (11)
179-192: LGTM! Clean conditional bulk action implementation.The bulk delete action is correctly gated behind the presence of the full Payments addon, preventing Lite users from accessing premium functionality.
228-228: LGTM! Gateway data properly initialized and passed to columns.The gateways array is correctly retrieved and included in the arguments for column rendering, enabling proper processor label display.
Also applies to: 233-233
274-283: LGTM! Proper conditional removal of bulk action checkboxes.The checkbox column is correctly hidden in Lite-only installations where bulk actions are unavailable, maintaining UI consistency.
285-294: LGTM! Clean abstraction for addon detection.The helper method properly encapsulates the logic for detecting whether the full Payments addon is active, making the code more maintainable and readable.
373-385: LGTM! Checkbox column correctly implemented.The method follows the established naming convention (
get_{column_name}_column) and is invoked dynamically at line 304. The PHPMD warning is a false positive due to the dynamic method resolution pattern.
402-405: LGTM! Good separation of concerns.The refactoring properly delegates link generation to a dedicated method, improving code organization and reusability.
407-422: LGTM! Secure link generation with proper escaping.The method correctly generates view links with proper URL escaping and attribute handling.
424-440: LGTM! Excellent URL generation centralization.The helper method consolidates URL building logic, eliminating duplication and making the codebase more maintainable.
455-455: LGTM! Appropriate edit link gating.The method correctly restricts edit functionality to non-subscription payments when the full addon is active, using proper class existence checks.
Also applies to: 465-476
572-572: LGTM! Good defensive programming with null coalescing.The null coalescing operator properly guards against undefined status properties, preventing potential PHP notices.
619-630: LGTM! Processor column properly handles multiple gateway sources.The implementation correctly prioritizes gateway labels from the registry, with appropriate fallbacks. Note the comment at lines 625-627 indicating the PayPal special case is temporary technical debt planned for future cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
classes/helpers/FrmFieldsHelper.php (1)
2429-2431: Fix@since x.xplaceholder and keep limit logicThe new
update_params_with_limit_data()helper correctly addsfrm_at_limitand adata-limitattribute based on a per‑form field count, and the call site only applies it to non‑upsell fields with alimitset, which matches the intended behavior for things like coupons. However, the docblock still uses the placeholder@since x.x; please replace this with the actual release version before shipping.Also applies to: 2466-2495
classes/helpers/FrmAppHelper.php (1)
4608-4615: Addis_arrayguard aroundarray_key_existsinshow_new_feature()
FrmAddonsController::install_link( $feature )may return a non‑array value; callingarray_key_exists()on that in PHP 8+ can throw aTypeError. Wrap the key checks in anis_array( $link )condition while keeping the new coupons fallback:Proposed fix
public static function show_new_feature( $feature ) { $link = FrmAddonsController::install_link( $feature ); - if ( array_key_exists( 'status', $link ) || array_key_exists( 'class', $link ) ) { + if ( is_array( $link ) && ( array_key_exists( 'status', $link ) || array_key_exists( 'class', $link ) ) ) { return true; } return 'coupons' === $feature && class_exists( 'FrmCouponsAppController' ); }stripe/helpers/FrmTransLiteAppHelper.php (2)
596-603: Previous review comments still apply.The version placeholder and inconsistent usage issues flagged in the previous review remain unresolved. Please address those concerns before merging.
605-613: Verify impact of deprecated function returning false.The function is correctly deprecated using WordPress patterns. However, since this previously had logic (per the AI summary) and now always returns
false, ensure all callers handle this behavior change appropriately.Run the following script to identify any remaining callers:
#!/bin/bash # Description: Find all calls to should_fallback_to_paypal to verify they handle the new behavior. # Search for calls to should_fallback_to_paypal echo "=== Calls to should_fallback_to_paypal ===" rg -nP '\bshould_fallback_to_paypal\s*\(' --type=php -C3 # Search for any conditional logic that might depend on this function echo -e "\n=== Conditional checks using should_fallback_to_paypal ===" rg -nP 'if\s*\([^)]*should_fallback_to_paypal' --type=php -C2js/src/admin/admin.js (1)
909-924: Drag-start limit guard still only checks the direct target
handleDragStartstill relies onevent.target.classList.contains('frm_at_limit'), so starting a drag by mousing down on a child element inside a limited button (icon, span, etc.) will bypass the limit and permit dragging. Using aclosest('.frm_at_limit')check is more robust and aligns with how the buttons are structured.Suggested update for the drag-start guard
function handleDragStart( event, ui ) { - if ( event.target.classList.contains( 'frm_at_limit' ) ) { + const atLimitTarget = event.target.closest && event.target.closest( '.frm_at_limit' ); + if ( atLimitTarget ) { showLimitModal(); return false; }
🧹 Nitpick comments (3)
stripe/views/payments/show.php (1)
60-64: Null‑safety and feature‑gated “Edit Payment” link are appropriateConditionally calling
show_in_table()only wheninvoice_idis set avoids notices while still relying on its internal empty check, and wrapping the Edit Payment action inFrmTransLiteAppHelper::payments_submodule_or_paypal_is_active()ensures the edit UI only appears when the payments submodule or PayPal add‑on is available; if you want extra hardening, consider wrapping$payment->idinabsint()in the edit URL, but it’s not strictly required here.Also applies to: 159-170
js/src/admin/admin.js (2)
1916-1938: Repeater drop rules and hook look good; align JSDoc and version tagThe new repeater logic cleanly:
- Blocks GDPR fields in repeaters.
- Exposes a
frm_deny_drop_in_repeaterfilter for other field types.- Skips the field‑group capacity check only when the droppable is the repeater container itself.
Behaviorally this is solid. Two small cleanups:
- Fix the JSDoc alignment that ESLint is flagging on this block.
- Replace
@since x.xwith the actual version before release so docs stay accurate.
2182-2241: Field-type limit UI flow (click, drag, add, delete) is coherent; only minor nitsOverall the field-type limiting behavior hangs together well:
addFieldClicknow checks$button.hasClass('frm_at_limit'), shows the limit modal, and returnsfalse, correctly preventing the click’s default navigation.showLimitModal()sensibly reuses the existing confirm-modal pipeline and hides the confirm button, whileconfirmModal()now always re‑shows it for non‑limit confirmations.afterAddFieldcallsmaybeDisableFieldButtonAtLimit(type), anddeleteFieldcallsmaybeEnableFieldButtonAtLimit($thisField.data('type')), keeping the palette button’s.frm_at_limitstate synced with the actual field count viacountFieldTypeInForm.A couple of small polish suggestions:
- Consider explicitly coercing
button.dataset.limitto a number inmaybeDisableFieldButtonAtLimit/maybeEnableFieldButtonAtLimit(e.g.,const limit = Number( button.dataset.limit );) to make the numeric comparison intent clearer.- Update the
@since x.xtags onmaybeDisableFieldButtonAtLimit/maybeEnableFieldButtonAtLimitto the real version before shipping.Functionally this looks good to ship as-is; the above are minor clarity/maintenance improvements.
Also applies to: 2857-2882, 5247-5276
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
images/icons.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (13)
bin/zip-plugin.shclasses/controllers/FrmAppController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/models/FrmField.phpcss/frm_admin.cssjs/formidable_admin.jsjs/src/admin/admin.jsstripe/controllers/FrmStrpLiteAppController.phpstripe/helpers/FrmTransLiteAppHelper.phpstripe/helpers/FrmTransLiteListHelper.phpstripe/views/payments/show.phpstripe/views/payments/sidebar_actions.php
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmAppController.php
🧰 Additional context used
🧬 Code graph analysis (5)
stripe/controllers/FrmStrpLiteAppController.php (2)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5054)is_admin_page(495-509)stripe/helpers/FrmTransLiteAppHelper.php (2)
FrmTransLiteAppHelper(6-614)payments_table_exists(39-56)
stripe/views/payments/show.php (1)
stripe/helpers/FrmTransLiteAppHelper.php (3)
FrmTransLiteAppHelper(6-614)show_in_table(406-416)payments_submodule_or_paypal_is_active(601-603)
classes/models/FrmField.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5054)show_new_feature(4608-4616)
stripe/views/payments/sidebar_actions.php (1)
stripe/controllers/FrmTransLitePaymentsController.php (2)
FrmTransLitePaymentsController(6-254)show_refund_link(123-126)
classes/helpers/FrmFieldsHelper.php (1)
classes/models/FrmDb.php (2)
FrmDb(6-867)get_count(231-235)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 1925-1925:
Expected JSDoc block lines to be aligned
🪛 PHPMD (2.15.0)
stripe/helpers/FrmTransLiteListHelper.php
383-385: Avoid unused private methods such as 'get_cb_column'. (undefined)
(UnusedPrivateMethod)
654-654: Avoid unused parameters such as '$active_tab'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (19)
bin/zip-plugin.sh (1)
106-107: The exclusions forformidable-coupons/js/frontend.jsandadmin.jsare pre-configured patterns for external add-on plugins, consistent with how other add-ons (formidable-views, formidable-chat, formidable-api, formidable-hubspot) are handled. These directories do not exist in the repository and only appear in the packaging script as part of the standard build configuration. No verification is needed.Likely an incorrect or invalid review comment.
stripe/controllers/FrmStrpLiteAppController.php (1)
66-77: Redirect guards look correct and saferThe new early-return checks for the payments admin page, PayPal add‑on presence, and existing payments table cleanly separate concerns and avoid unnecessary redirects; no issues spotted here.
classes/models/FrmField.php (1)
331-340: Coupon field registration and “new” flag wiring look consistentThe Coupon pro field entry is correctly gated via
FrmAppHelper::show_new_feature( 'coupons' ), placed in the pricing section, limited to one per form, and hooked intofield_is_new( 'coupon' )with a concrete release date; this matches the intended UI behavior with no obvious issues.Also applies to: 361-364
stripe/views/payments/sidebar_actions.php (1)
20-26: Stricter payment display guard and new sidebar hook look goodGuarding on
isset( $payment->status ), checking for'complete', and requiring a non‑emptyreceipt_idbefore rendering the refund section should prevent notices and avoid showing refund UI when it’s not actionable; the newfrm_pay_{$payment->paysys}_sidebarhook also provides a clean extension point assuming$payment->paysysis always set in this view.Also applies to: 28-30
stripe/helpers/FrmTransLiteAppHelper.php (1)
78-78: LGTM!The change from a truthy check to
!empty()is a good defensive programming practice. This ensures that falsy but defined values (empty string, 0, etc.) correctly trigger the PayPal fallback logic.js/src/admin/admin.js (1)
323-337: Confirm modal reset for reused button looks correctRe‑showing
continueButtonon everyconfirmModalopen guards against the hidden state left byshowLimitModal, without changing other behavior. This is a clean, localized fix and looks good.stripe/helpers/FrmTransLiteListHelper.php (13)
179-192: LGTM! Clean bulk actions implementation.The conditional bulk delete action based on payment addon presence is well-structured and follows WordPress list table conventions.
228-228: LGTM! Efficient gateway data preparation.Retrieving gateways once and passing them to all rows is a good performance optimization that enables proper gateway label display.
Also applies to: 233-233
274-283: LGTM! Checkbox column properly gated.The conditional removal of the checkbox column for Lite-only installations is consistent with the bulk actions availability and well-implemented.
285-294: LGTM! Clear addon detection helper.The method provides a clean abstraction for detecting payment addon presence with a helpful docblock explaining the specific addons checked.
373-385: LGTM! Checkbox column properly implemented.The static analysis warning about this method being unused is a false positive. The method is called dynamically via
get_column_value()which constructs the method name from the column name (line 304:'get_' . $column_name . '_column'). This follows the established pattern used by other column methods in this class.
402-405: LGTM! Clean action column refactor.Extracting link generation to
get_view_payment_link()improves maintainability and centralizes URL building logic.
407-422: LGTM! Well-structured link builder.The method properly handles URL generation, escaping, and HTML attribute building with good separation of concerns.
424-440: LGTM! Centralized URL generation.This helper effectively centralizes payment URL building using standard WordPress functions and improves maintainability across the class.
455-455: LGTM! Improved readability.Extracting the edit link condition to
supports_edit_link()improves code clarity and maintainability.
465-476: LGTM! Appropriate edit link gating.The logic correctly excludes subscriptions and requires payment controller presence. Good use of
class_exists()withautoload=falsefor performance.
572-572: LGTM! Defensive null-coalescing.The null-coalescing operator guards against potential undefined
statusproperty, preventing PHP notices.
619-630: LGTM! Gateway label lookup with documented fallback.The implementation properly uses the gateways array for label lookup. The PayPal special case at lines 623-627 is well-documented as temporary and can be removed once PayPal Commerce is integrated into Lite.
647-663: LGTM! Proper tabs rendering with feature gating.The static analysis warning about the unused
$active_tabparameter is a false positive. When PHP includes a file (line 661), all variables in the current scope—including$active_tab—are available to the included template. The capability and feature flag checks properly gate the coupons functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
js/src/admin/admin.js (1)
909-912: Drag-start limit guard can be bypassed via child elementsChecking only
event.target.classList.contains('frm_at_limit')misses drags that begin on child elements inside the limited button (e.g. SVG, span). In those cases the field can still be dragged, effectively bypassing the type limit.Consider resolving this by walking up from the event target to a parent with the limit class, and guarding against non-Element targets:
- function handleDragStart( event, ui ) { - if ( event.target.classList.contains( 'frm_at_limit' ) ) { - showLimitModal(); - return false; - } + function handleDragStart( event, ui ) { + const rawTarget = event.target; + const elementTarget = rawTarget && rawTarget.closest ? rawTarget : null; + const limitButton = elementTarget ? elementTarget.closest( '.frmbutton.frm_at_limit' ) : null; + + if ( limitButton ) { + showLimitModal(); + return false; + }
🧹 Nitpick comments (2)
js/src/admin/admin.js (2)
1915-1936: Repeater/GDPR drop restriction logic is sound; tidy up JSDocThe new repeater-aware logic (
droppableIsARepeater/droppableIsInsideOfARepeater) correctly:
- Blocks GDPR fields (
edit_field_type_gdprand#gdpr) from being dropped into or inside repeaters.- Exposes a
frm_deny_drop_in_repeaterfilter for additional denial rules without affecting non-repeater drops.Two small follow‑ups:
- The JSDoc block above
frm_deny_drop_in_repeateris flagged by ESLint for alignment; adjust the leading*spacing to match the surrounding JSDoc style.- Replace
@since x.xwith the actual version number before release.
2856-2881: Field-type limit bookkeeping is generally correct; ensure type identifiers stay consistentThe new helpers and calls behave coherently:
afterAddField→maybeDisableFieldButtonAtLimit(type)marks the palette button asfrm_at_limitwhen the count ofli.form-field[data-ftype="type"]reachesbutton.dataset.limit.- On delete,
deleteFieldcallsmaybeEnableFieldButtonAtLimit( $thisField.data('type') ); subtracting 1 fromcountFieldTypeInForm(type)compensates for the soon‑to‑be-removed field so the limit is only lifted when the post‑delete count is below the limit.countFieldTypeInFormscopes the count to#frm-show-fields, avoiding side effects elsewhere.One thing to keep in mind going forward: this relies on the inserted field’s
data-typematching both the palette button’sidand the field’sdata-ftype. If any field type diverges from that convention, its limit won’t be enforced correctly; in that case you may want a small mapping helper rather than usingtypedirectly.Also applies to: 5246-5275
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/formidable_admin.jsjs/src/admin/admin.js
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (2)
js/admin/style.js (1)
wp(13-13)js/src/form/modal.js (1)
wp(5-5)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 1924-1924:
Expected JSDoc block lines to be aligned
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (2)
js/src/admin/admin.js (2)
332-336: Confirm modal button visibility reset looks correctEnsuring
#frm-confirmed-clickis forced visible on everyconfirmModalopen guards against state leaked fromshowLimitModal, and is a safe, localized change.
2192-2200: Field-type limit handling andshowLimitModalwiring look consistent
- In
addFieldClick, short‑circuiting when$button.hasClass('frm_at_limit')and returningfalseis appropriate for jQuery click handlers and cleanly prevents insertion + shows the limit modal.showLimitModal()reuses the existing confirm modal flow via a temporary<a data-frmverify>…</a>, then hides the confirm button so the dialog is informational-only, which matches the intended UX.- The new comment clarifying why
shouldStopInsertingFieldmust not returnfalseon click helps avoid regressions around slider fields.No functional issues spotted in this block.
Also applies to: 2242-2259
Use Lite for the payments pages and add coupons tabs
1 participant
This update also includes support for the Coupons add-on.
Main Coupons PR https://github.com/Strategy11/formidable-coupons/pull/1