fix: fix export url generation for sandbox accounts #659
Conversation
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe changes involve a modification to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
fyle_netsuite_api/utils.py (2)
48-50: Consider adding validation for account ID format.While the fix works, it would be beneficial to validate the account ID format before processing.
if '_sb' in ns_account_id: + # Validate account ID format (e.g., xxxxxx_sb1 or xxxxxx_sb2) + if not re.match(r'^[a-z0-9]+_sb[0-9]?$', ns_account_id): + logger.warning(f'Unexpected account ID format: {ns_account_id}') ns_account_id = ns_account_id.replace('_sb', '-sb')
48-50: Add a code comment explaining the sandbox URL format requirement.To improve maintainability, document why this replacement is necessary.
+# NetSuite sandbox accounts require '-sb' format in URLs instead of '_sb' if '_sb' in ns_account_id: ns_account_id = ns_account_id.replace('_sb', '-sb')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
fyle_netsuite_api/utils.py(1 hunks)
🔇 Additional comments (1)
fyle_netsuite_api/utils.py (1)
48-50: LGTM! Verify URL format with NetSuite documentation.
The fix correctly handles the NetSuite sandbox account URL format by replacing '_sb' with '-sb'. This change aligns with NetSuite's URL structure.
Let's verify if there are any other instances where this URL format needs to be updated:
✅ Verification successful
URL format change is correctly implemented and isolated
The verification shows that:
- The URL format change in
utils.pyis the only instance handling sandbox account IDs with '_sb' - Other NetSuite URL constructions in the codebase (in
apps/netsuite/connector.py) already use the correct '-SB' format - Test fixtures contain properly formatted URLs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential NetSuite URL constructions or account ID usage
rg -l "netsuite\.com" | xargs rg "_sb"
Length of output: 199
Script:
#!/bin/bash
# Let's check for any other URL construction patterns or account ID handling
rg -A 5 -B 5 "netsuite\.com"
Length of output: 9917
JustARatherRidiculouslyLongUsername
added
the
deploy
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
JustARatherRidiculouslyLongUsername
temporarily deployed
to
Staging
GitHub Actions
Inactive
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/sql/scripts/029-update-sandbox-export-urls.sql(1 hunks)
🔇 Additional comments (2)
scripts/sql/scripts/029-update-sandbox-export-urls.sql (2)
4-16: LGTM: Well-documented current state.
The comments effectively document the current state and expected changes, which is excellent for validation and future reference.
1-17: Verify the impact of URL changes.
Since this change affects production URLs, we should verify:
- That the new URL format is correct and accessible
- That there are no unintended side effects in the application
✅ Verification successful
URL format change is aligned with existing code patterns
The URL format change from _sb to -sb is consistent with the existing code in fyle_netsuite_api/utils.py which already handles this conversion for NetSuite account IDs. The codebase scan reveals:
- The same conversion logic exists in
utils.pyfor account IDs - No other hardcoded instances of the old
_sbformat were found - No test files need updating
- No configuration files contain these patterns
The migration script's change aligns with the application's existing URL handling patterns, making this a safe update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded URL patterns in the codebase that might need updating
rg -g '!*.sql' '_sb'
# Look for any tests that might need updating
rg -g '*test*.py' '_sb'
Length of output: 205
Script:
#!/bin/bash
# Let's check for both URL patterns to understand the usage
rg -A 3 -B 3 '(_sb|-sb)' .
# Also check for any configuration or environment files that might contain these patterns
fd -e yml -e yaml -e env -e config -e ini -e properties -e json . -x grep -l "_sb\|-sb" {} \;
Length of output: 2070
* fix: fix export url generation for sandbox accounts * fix: add script to update bad db data (cherry picked from commit b353f2f)
Description
The export URLs we generate today look like
This works for regular netsuite accounts, but not for sandbox ones (notice
sbin the subdomain name). Netsuite expects a dash-instead of an underscore_This change fixes the export URL generation, as well as patches the incorrect URLs in the database.
Clickup
https://app.clickup.com/t/86cwzmjkt
Summary by CodeRabbit