Fix square location ID issues#2536
Conversation
WalkthroughAdds mode-aware (auto/test/live) handling to Square currency and location retrieval, bumps DB version to 104, and adds a migrate_to_104 migration that refreshes test/live location IDs via the new per-mode getters; also expands an E2E test's accepted H1 banner text. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller
participant Helper as FrmSquareLiteConnectHelper
participant Options as WP_Options
participant Server as Square_Backend
Caller->>Helper: get_location_id(force?, mode)
Helper->>Options: read location_id[mode]
alt cached && !force
Options-->>Helper: cached location_id
Helper-->>Caller: return location_id
else
Helper->>Server: POST /merchant/locations {frm_square_api_mode, testMode?}
Server-->>Helper: location_id
Helper->>Options: save location_id[mode]
Helper-->>Caller: return location_id
end
Caller->>Helper: get_merchant_currency(force?, mode)
Helper->>Options: read currency[mode]
alt cached && !force
Options-->>Helper: cached currency
Helper-->>Caller: return currency
else
Helper->>Server: POST /merchant/currency {frm_square_api_mode, testMode?}
Server-->>Helper: currency
Helper->>Options: save currency[mode]
Helper-->>Caller: return currency
end
sequenceDiagram
autonumber
actor Migrator as FrmMigrate
participant Helper as FrmSquareLiteConnectHelper
Migrator->>Migrator: migrate_to_104()
Migrator->>Migrator: ensure at least one Lite mode set
Migrator->>Helper: get_location_id(true, 'test')
Helper-->>Migrator: refreshed test location
Migrator->>Helper: get_location_id(true, 'live')
Helper-->>Migrator: refreshed live location
Migrator-->>Migrator: complete migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
square/helpers/FrmSquareLiteConnectHelper.php (2)
551-572: Consider refactoring to eliminate side effects and improve maintainability.The method has several concerns:
Side effect on global state (line 561): Modifying
$_POST['testMode']in a getter method is unexpected and could affect other code that reads$_POSTlater in the request. This is done to communicate the mode toget_mode_value_from_post()(line 526), but it's a fragile approach.Missing input validation: The
$modeparameter is not validated. If a caller passes an invalid value (e.g.,'invalid'), line 561 would treat it as'live'(ternary defaults to 0), andget_active_mode_option_name_suffix()would construct an incorrect option name like'frm_square_connect_merchant_location_id_invalid'.Code duplication: Lines 559-563 are duplicated in
get_merchant_currency()(lines 672-676).Consider these improvements:
Option 1 (preferred): Pass mode through the call chain instead of modifying
$_POST:// Extend post_with_authenticated_body to accept optional mode private static function post_with_authenticated_body( $action, $additional_body = array(), $mode = null ) { $body = array_merge( self::get_standard_authenticated_body( $mode ), $additional_body ); // ... rest of method } private static function get_standard_authenticated_body( $mode = null ) { if ( null === $mode ) { $mode = self::get_mode_value_from_post(); } // ... use $mode directly instead of reading from $_POST }Option 2: At minimum, validate the mode parameter:
public static function get_location_id( $force = false, $mode = 'auto' ) { // Validate mode if ( ! in_array( $mode, array( 'auto', 'live', 'test' ), true ) ) { $mode = 'auto'; } // ... rest of method }Option 3: Extract the duplicate logic into a helper method:
private static function prepare_mode_request_body( $mode ) { $request_body = array(); if ( 'auto' !== $mode ) { $_POST['testMode'] = 'test' === $mode ? 1 : 0; $request_body['frm_square_api_mode'] = $mode; } return $request_body; }
664-685: Same concerns asget_location_id().This method has the same issues as
get_location_id():
- Modifies
$_POST['testMode']as a side effect (line 674)- Missing input validation for the
$modeparameter- Duplicate code (lines 672-676 are identical to lines 559-563 in
get_location_id())Apply the same refactoring suggestions from the previous comment to eliminate side effects, add validation, and extract the duplicate logic into a shared helper method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
square/helpers/FrmSquareLiteConnectHelper.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
square/helpers/FrmSquareLiteConnectHelper.php (2)
square/js/settings.js (1)
mode(7-7)square/controllers/FrmSquareLiteActionsController.php (1)
get_location_id(554-556)
⏰ 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). (6)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Cypress
🔇 Additional comments (1)
square/helpers/FrmSquareLiteConnectHelper.php (1)
454-455: LGTM!The calls correctly pass the mode parameter to fetch mode-specific currency and location data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
classes/models/FrmMigrate.php (1)
328-328: Minor pre-existing issue: duplicate migration number.The migrations array contains
16twice. While this doesn't affect the current PR, consider removing the duplicate in a future cleanup.Apply this diff to remove the duplicate:
- $migrations = array( 16, 11, 16, 17, 23, 25, 86, 90, 97, 98, 101, 104 ); + $migrations = array( 11, 16, 17, 23, 25, 86, 90, 97, 98, 101, 104 );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(1 hunks)classes/models/FrmMigrate.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmMigrate.php (1)
square/helpers/FrmSquareLiteConnectHelper.php (1)
FrmSquareLiteConnectHelper(6-726)
🪛 PHPMD (2.15.0)
classes/models/FrmMigrate.php
406-413: Avoid unused private methods such as 'migrate_to_104'. (undefined)
(UnusedPrivateMethod)
⏰ 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). (4)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/helpers/FrmAppHelper.php (1)
13-13: LGTM!The database version increment aligns correctly with the new
migrate_to_104()method added inFrmMigrate.php.classes/models/FrmMigrate.php (1)
406-413: Verifyget_location_id()handles missing merchant credentials. We couldn’t locatepost_with_authenticated_body()in the helper—please confirm it skips the API call or returns false (without error) when no merchant ID is configured for the requested mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js (1)
33-33: Separate promotional banner update from Square integration scope.The new “No Brainer Sale” heading is unrelated to the Square location-ID fixes and makes this brittle banner test require updates on every marketing change. Consider extracting banner-text validation into its own PR (or suite) and improving resilience by:
- Matching generic promo patterns (e.g.
/get \d+% off/i) instead of exact strings- Verifying key elements (upgrade CTA presence, pricing info) rather than full text
Questions:
- Was this promo-text change intentionally bundled with the Square location-ID work?
- If so, is there any hidden dependency between the two?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js(1 hunks)
⏰ 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). (4)
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmMigrate.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmMigrate.php (1)
square/helpers/FrmSquareLiteConnectHelper.php (2)
FrmSquareLiteConnectHelper(6-726)get_merchant_id(356-361)
🪛 PHPMD (2.15.0)
classes/models/FrmMigrate.php
406-414: Avoid unused private methods such as 'migrate_to_104'. (undefined)
(UnusedPrivateMethod)
⏰ 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). (4)
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (1)
classes/models/FrmMigrate.php (1)
328-328: LGTM!The migration number is correctly added to the array and aligns with the updated database version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js (1)
21-21: LGTM! Consider extracting banner text patterns for maintainability.The addition of the new promotional text variant is correct and consistent with the existing validation logic.
However, the condition is growing long with multiple text patterns. For better maintainability, consider extracting these into a helper:
+const isValidUpgradeBanner = (text) => { + const validPatterns = [ + 'upgrading to PRO', + 'Get 60% Off Pro!', + 'Get the Deal', + 'upgrading for 60% off during our No Brainer Sale!', + ]; + return validPatterns.some(pattern => text.includes(pattern)) || + text.match(/GET \d+% OFF|SAVE \d+%/); +}; + cy.get( '.frm-upgrade-bar .frm-upgrade-bar-inner > a, #frm_sale_banner a:not(.dismiss)' ) .then( $el => { const text = $el.text().trim(); const href = $el.attr( 'href' ); - if ( href && ( text.includes( 'upgrading to PRO' ) || text.includes( 'Get 60% Off Pro!' ) || text.includes( 'Get the Deal' ) || text.match( /GET \d+% OFF|SAVE \d+%/ ) || text.includes( 'upgrading for 60% off during our No Brainer Sale!' ) ) ) { + if ( href && isValidUpgradeBanner(text) ) {This makes it easier to add/remove promotional text variants in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js(1 hunks)
⏰ 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). (4)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
Fix square location ID issues
1 participant
Related ticket https://secure.helpscout.net/conversation/3098276712/239265
Pre-release
formidable-6.25b.zip