Square payments#2229
Conversation
Crabcyborg
changed the title
…e is a merchant ID instead of the Connect button. Start checking for the redirect
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
square/js/action.js (2)
88-97: Hard-coded label & missed localisation / optional-chaining
- The literal
'Repeat'replaces the original label but bypasseswp.i18n.__, so non-English admins will see an untranslated string.- Both
labelandstripeLabelare already null-checked, but the innerincludescall can be shortened with optional chaining − the Biome hint points this out.-if ( label && label.textContent.includes( 'Repeat Every' ) ) { - label.textContent = 'Repeat'; +if ( label?.textContent.includes( 'Repeat Every' ) ) { + label.textContent = __( 'Repeat', 'formidable' ); } ... -if ( stripeLabel && stripeLabel.textContent.includes( 'Repeat Every' ) ) { - stripeLabel.textContent = 'Repeat'; +if ( stripeLabel?.textContent.includes( 'Repeat Every' ) ) { + stripeLabel.textContent = __( 'Repeat', 'formidable' ); }🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
102-135:syncCurrencymixes concerns & repeats DOM work
syncCurrencyboth creates and selects the Square-specific<option>each call. As a result it:• Touches the DOM on every toggle even when nothing changed.
• Deletes and recreates the same node repeatedly.A simpler, more robust approach is to:
- Add the Square placeholder once on initialisation.
- Toggle only
disabled/hiddenstates afterwards.This reduces DOM churn and makes the function idempotent, which is safer when hooks fire multiple times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
square/controllers/FrmSquareLiteActionsController.php(1 hunks)square/helpers/FrmSquareLiteAppHelper.php(1 hunks)square/js/action.js(1 hunks)stripe/controllers/FrmTransLiteAppController.php(2 hunks)stripe/controllers/FrmTransLiteHooksController.php(3 hunks)stripe/models/FrmTransLiteAction.php(2 hunks)stripe/views/action-settings/payments-options.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- stripe/controllers/FrmTransLiteAppController.php
- stripe/models/FrmTransLiteAction.php
- stripe/controllers/FrmTransLiteHooksController.php
- stripe/views/action-settings/payments-options.php
- square/helpers/FrmSquareLiteAppHelper.php
- square/controllers/FrmSquareLiteActionsController.php
🧰 Additional context used
🪛 Biome (1.9.4)
square/js/action.js
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
square/js/action.js (3)
21-23:⚠️ Potential issueNull-dereference risk still present
settings.querySelector(...)can returnnull. Accessing.checkedwithout a guard will still crash admin JS when the Square radio does not exist (e.g. legacy forms or other gateways only).
Same issue was pointed out previously and remains unresolved.-const squareIsEnabled = settings.querySelector('[name*="[post_content][gateway]"][value="square"]').checked; +const squareGateway = settings.querySelector('[name*="[post_content][gateway]"][value="square"]'); +const squareIsEnabled = squareGateway?.checked ?? false;
8-13:⚠️ Potential issueCurrency-lock bug resurfaced
syncCurrencyis still invoked withgatewayeven whencheckedisfalse.
If the user unchecks Square,gatewaystays"square"and the currency dropdown remains disabled until a page reload.Pass
null/''(or extendsyncCurrencyto acceptchecked) so the control unlocks:- syncCurrency( gateway, settings.get( 0 ) ); + const activeGateway = checked ? gateway : null; + syncCurrency( activeGateway, settings.get( 0 ) );
27-33: 🛠️ Refactor suggestionNo teardown path for cloned repeat-cadence UI
syncRepeatclones.frm_trans_sub_optson first activation but never removes
.show_squareclones or unhides.show_stripeoriginals when the admin toggles
away from Square. Multiple toggles leave hidden duplicates in the DOM.Provide a reversal branch (e.g.
unsyncRepeat) or re-runsyncRepeatwith a
“teardown” flag whenchecked === falseto keep the markup clean.
🧹 Nitpick comments (2)
square/js/action.js (2)
81-100: Label/text mutation leaks into Stripe UIBoth Square and Stripe branches overwrite “Repeat Every” → “Repeat”.
Because the Stripe element is permanently hidden (frm_hidden) the label text
never gets restored, so if the user disables Square later and Stripe becomes
visible again it will show the Square wording.Store original text in
data-*before mutating or rebuild the element on undo.🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
102-135: Hard-coded strings & missing translation wrappers
'Use Square Merchant Currency'is inserted without__()so the text is not
localisable. All UI strings must pass throughwp.i18n.__.- option.textContent = 'Use Square Merchant Currency'; + option.textContent = __( 'Use Square Merchant Currency', 'formidable' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
square/js/action.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
square/js/action.js (2)
stripe/js/frmtrans_admin.js (3)
gateway(32-32)checked(33-33)settings(38-38)js/admin/dom.js (2)
$container(313-313)__(4-4)
🪛 Biome (1.9.4)
square/js/action.js
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
…curring settings appearing after toggling gateway
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
square/js/action.js (3)
8-15: Currency selector remains locked when Square is untoggled
syncCurrency()is always invoked with the rawgatewayvalue, even whencheckedisfalse, so the currency dropdown stays disabled after Square is deselected.
Forward the effective gateway (or thecheckedflag) instead.- syncCurrency( gateway, settings.get( 0 ) ); + const activeGateway = checked ? gateway : null; // ‼️ unlock when unchecked + syncCurrency( activeGateway, settings.get( 0 ) );
43-46: Possible null-dereference when Square radio is absent
querySelector()may returnnull, yet.checkedis accessed directly.-const squareIsEnabled = settings - .querySelector('[name*="[post_content][gateway]"][value="square"]').checked; +const squareGateway = settings + .querySelector('[name*="[post_content][gateway]"][value="square"]'); +const squareIsEnabled = squareGateway?.checked ?? false;
24-34: 🛠️ Refactor suggestionNo teardown when toggling away from Square
syncRepeat()only runs on the enable path; cloned.show_squarenodes and the hidden Stripe markup are never restored, so repeated toggles accumulate stale DOM.
Add a corresponding cleanup branch whenchecked === false.if ( 'square' === gateway && checked ) { syncRepeat( settings.get( 0 ) ); -} +} else if ( 'square' === gateway && ! checked ) { + cleanupRepeat( settings.get( 0 ) ); // <- remove .show_square, unhide .show_stripe +}You already track the cloned block with
.show_square; teardown can be a trivial selector-based removal.
🧹 Nitpick comments (1)
square/js/action.js (1)
109-120: Hard-coded “Repeat” label bypasses translation & linter hint
The new label text should be run through__()for localisation and Biome is flagging the non-optional call site. Combine both fixes:- if ( label && label.textContent.includes( 'Repeat Every' ) ) { - label.textContent = 'Repeat'; + if ( label?.textContent.includes( 'Repeat Every' ) ) { + label.textContent = __( 'Repeat', 'formidable' ); }🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
square/js/action.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
square/js/action.js
[error] 110-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
square/js/action.js (3)
11-18: Currency lock still persists when Square is untoggled
syncCurrency( gateway, … )is invoked with the rawgatewaystring even whenchecked === false, so the currency dropdown remains disabled after the admin un-checks Square. Passnull/''(or add thecheckedflag) to restore the selector.- syncCurrency( gateway, settings.get( 0 ) ); + const activeGateway = checked ? gateway : null; + syncCurrency( activeGateway, settings.get( 0 ) );
46-49: Possible null-dereference when Square radio is missing
querySelector()can returnnull; accessing.checkedwill throw and break all subsequent JS on the page. Guard it:-const squareIsEnabled = settings - .querySelector('[name*="[post_content][gateway]"][value="square"]').checked; +const squareGateway = settings + .querySelector('[name*="[post_content][gateway]"][value="square"]'); +const squareIsEnabled = squareGateway?.checked ?? false;
55-57: Second unchecked.checkedaccess
Same null-safety issue insideonToggleSub:-const squareIsActive = settings - .querySelector('[name*="[post_content][gateway]"][value="square"]').checked; +const squareIsActive = settings + .querySelector('[name*="[post_content][gateway]"][value="square"]')?.checked ?? false;
🧹 Nitpick comments (1)
square/js/action.js (1)
128-138: Use optional chaining to satisfy linter & avoid undefined access
Biome flags these lines. Switching to optional chaining is a safe micro-cleanup:-const label = newDropdown.closest('.frm_trans_sub_opts')?.querySelector('label'); -if ( label && label.textContent.includes('Repeat Every') ) { +const label = newDropdown.closest('.frm_trans_sub_opts')?.querySelector('label'); +if ( label?.textContent?.includes('Repeat Every') ) { label.textContent = 'Repeat'; }🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
square/js/action.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
square/js/action.js
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
square/js/action.js (4)
46-47: Guard against null when the Square radio isn’t rendered
querySelector()may returnnull, so dereferencing.checkedcan throw and break all admin JS.
16-17: Currency lock persists when Square is untoggled
syncCurrency()is still called withgateway === 'square'even whenchecked === false, so the dropdown remains disabled.
68-141:syncRepeat()still has no teardown path
Cloned.show_squarenodes accumulate and original Stripe nodes stay hidden when the admin switches gateways repeatedly.🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-57:⚠️ Potential issueSame null-deref risk in
onToggleSub
settings.querySelector(...).checkedis unguarded here as well. Add a safe check:-const squareIsActive = settings - .querySelector('[name*="[post_content][gateway]"][value="square"]').checked; +const squareGateway = settings + .querySelector('[name*="[post_content][gateway]"][value="square"]'); +const squareIsActive = squareGateway?.checked ?? false;
🧹 Nitpick comments (2)
square/js/action.js (2)
128-138: Optional chaining would simplify null checksBiome flags these two spots. Minor, but keeps the code terse:
-const label = newDropdown.closest( '.frm_trans_sub_opts' )?.querySelector( 'label' ); +const label = newDropdown.closest('.frm_trans_sub_opts')?.querySelector?.('label'); ... -const stripeLabel = intervalCount.closest( '.frm_trans_sub_opts' )?.querySelector( 'label' ); +const stripeLabel = intervalCount.closest('.frm_trans_sub_opts')?.querySelector?.('label');🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
165-174: String should be translatable
'Use Square Merchant Currency'is user-facing but isn’t wrapped in__().-option.textContent = 'Use Square Merchant Currency'; +option.textContent = __( 'Use Square Merchant Currency', 'formidable' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
square/js/action.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
square/js/action.js
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHPStan
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
square/js/action.js (3)
11-18:⚠️ Potential issueCurrency lock still sticks when Square is untoggled
syncCurrency()is called with the rawgatewayvalue even whenchecked === false, so the currency drop-down remains disabled after Square is deselected (same issue flagged earlier).- syncCurrency( gateway, settings.get( 0 ) ); + // Pass `null` when the toggle is OFF so syncCurrency unlocks the selector. + const activeGateway = checked ? gateway : null; + syncCurrency( activeGateway, settings.get( 0 ) );
69-142: 🛠️ Refactor suggestion
syncRepeat()has no teardown path — cloned nodes accumulate & originals stay hiddenWhen Square is disabled, the
.show_squareclone is left in the DOM and the original Stripe inputs keepfrm_hidden. Re-enabling Square works, but repeated toggles leave orphaned hidden nodes and progressively bloat the markup.Add a cleanup branch that:
- Removes existing
.show_squareelements (or unhides/re-use them).- Removes
frm_hidden&.show_stripefrom the original Stripe block.Example sketch:
function syncRepeat( settings ) { + // If Square was previously enabled, clean up first. + settings.querySelectorAll( '.frm_trans_sub_opts.show_square' ).forEach( el => el.remove() ); + settings.querySelectorAll( '.frm_trans_sub_opts.show_stripe.frm_hidden' ) + .forEach( el => { + el.classList.remove( 'frm_hidden', 'show_stripe' ); + } ); + // existing “setup” logic continues...🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 137-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
55-60:⚠️ Potential issuePossible null-dereference when the Square radio isn’t present
querySelector()may returnnull; directly accessing.checkedcan throw and break all subsequent JS.- const squareIsActive = settings.querySelector( '[name*="[post_content][gateway]"][value="square"]' ).checked; + const squareGateway = settings.querySelector( '[name*="[post_content][gateway]"][value="square"]' ); + const squareIsActive = squareGateway?.checked ?? false;
🧹 Nitpick comments (2)
square/js/action.js (2)
151-161: Hard-coding fallback currency tousdmay override a merchant’s preferred defaultIf a site’s default currency isn’t USD, forcibly resetting to
'usd'on every non-Square toggle is surprising. Consider restoring the previously selected option instead of hard-coding:- option.remove(); - currencySetting.value = 'usd'; + const previous = currencySetting.dataset.prevCurrency || currencySetting.value; + option.remove(); + currencySetting.value = previous;Before locking, store
currencySetting.dataset.prevCurrency = currencySetting.value.
129-140: Minor: optional chaining can simplify null-checksStatic analysis already hints these spots. A concise form:
- const label = newDropdown.closest( '.frm_trans_sub_opts' )?.querySelector( 'label' ); - if ( label && label.textContent.includes( 'Repeat Every' ) ) { + const label = newDropdown.closest( '.frm_trans_sub_opts' )?.querySelector( 'label' ); + if ( label?.textContent.includes( 'Repeat Every' ) ) { label.textContent = 'Repeat'; }Same applies to
stripeLabelbelow.🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 137-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
square/controllers/FrmSquareLiteActionsController.php(1 hunks)square/js/action.js(1 hunks)stripe/views/action-settings/payments-options.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- stripe/views/action-settings/payments-options.php
- square/controllers/FrmSquareLiteActionsController.php
🧰 Additional context used
🪛 Biome (1.9.4)
square/js/action.js
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 137-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
|
@lauramekaj1 I'm going to merge this now. I don't plan to release it until the morning. Let me know if you find any issues! I'll just handle changes in new PRs. |
1 participant
Fixes https://github.com/Strategy11/formidable-pro/issues/5598
Fixes https://github.com/Strategy11/formidable-pro/issues/5004
More info in Slack https://strategy11.slack.com/archives/C799A2R61/p1749572899502199