fix: add error handing for limit mismatch #1390
Conversation
WalkthroughThe changes introduce enhanced error handling for gift card purchases, specifically detecting and responding to spending limit errors. New string resources and constants support user messaging and reporting via email. The dialog now offers users a direct way to contact support when a limit error occurs, with structured error details included in the report. Additionally, unit tests were added to verify the new exception behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dialog
participant ViewModel
participant Repository
participant EmailApp
User->>Dialog: Confirm gift card purchase
Dialog->>ViewModel: purchaseGiftCard()
ViewModel->>Repository: Attempt purchase
Repository-->>ViewModel: Throw CTXSpendException (limit error)
ViewModel-->>Dialog: Exception thrown
Dialog->>User: Show limit error dialog with "Contact CTX Support"
User->>Dialog: Click "Contact CTX Support"
Dialog->>ViewModel: createEmailIntent(subject, exception)
ViewModel-->>Dialog: Email Intent
Dialog->>EmailApp: Launch email intent chooser
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (4)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (1)
159-159: Remove commented out else branch.The commented-out else branch should be removed as it's unnecessary and creates noise in the code.
- //else -> {}features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (1)
53-55: Improve type safety in nested map traversalThe nested map traversal to determine
isLimitErroruses multiple unsafe casts which could lead to issues with unexpected JSON structures.Consider using a more explicit traversal:
val isLimitError get() = errorCode == 400 && - ((errorMap["fields"] as? Map<*, *>)?.get("fiatAmount") as? List<*>)?.firstOrNull() == "above threshold" + errorMap["fields"]?.let { fields -> + when (fields) { + is Map<*, *> -> fields["fiatAmount"]?.let { fiatAmount -> + when (fiatAmount) { + is List<*> -> fiatAmount.firstOrNull() == "above threshold" + else -> false + } + } ?: false + else -> false + } + } ?: falseOr use an extension function for more readable code:
private fun Map<String, Any>.getNestedValue(path: List<String>): Any? { var current: Any? = this for (key in path) { current = (current as? Map<*, *>)?.get(key) if (current == null) return null } return current }features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (2)
111-140: Comprehensive error handling for limit errorsGood implementation of specialized error handling for spending limit errors, with a dedicated UI flow for contacting support.
However, there's a TODO comment on line 121 that should be addressed:
- // TODO: shareAlso, consider providing more informative error messages to the user about:
- What the spending limit is
- When they can try again
- What information will be included in the support email
127-128: Use string resources for email subjectThe email subject "CTX Issue: Spending Limit Problem" is hardcoded rather than using a string resource.
Consider moving this to a string resource:
+// Add to strings-explore-dash.xml: +<string name="gift_card_limit_error_email_subject">CTX Issue: Spending Limit Problem</string> // In code: -"CTX Issue: Spending Limit Problem", +getString(R.string.gift_card_limit_error_email_subject),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/src/main/res/values/strings.xml(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(4 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt(4 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/CTXSpendConstants.kt(1 hunks)features/exploredash/src/main/res/values/strings-explore-dash.xml(2 hunks)wallet/res/values/strings.xml(0 hunks)
💤 Files with no reviewable changes (1)
- wallet/res/values/strings.xml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/CTXSpendConstants.kt (1)
25-25: LGTM: Support email constant added.The addition of this constant is appropriate for centralizing the support email address used in error reporting.
common/src/main/res/values/strings.xml (1)
113-113: LGTM: String resource for email intent chooser.This string resource is appropriately positioned and will be used when launching the email client selection dialog.
features/exploredash/src/main/res/values/strings-explore-dash.xml (2)
231-231: LGTM: Clear error message for limit errors.This string resource provides a specific error message for the purchase limit error case, directing users to contact support.
241-241: LGTM: Support contact button text.This string resource will be used for the button to contact CTX support, providing a clear action for users.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (3)
153-158: LGTM: Enhanced error handling.The improved error handling now includes error code and error body in the exception, which provides more context for better troubleshooting.
294-303: LGTM: Email intent creation.This function creates a properly formatted email intent with the support email address, subject, and report body.
305-332: LGTM: Detailed report email generation.This function generates a comprehensive error report including merchant details, purchase amount, and error information, which will be extremely helpful for troubleshooting.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (1)
36-40: Enhanced error handling with structured dataGood enhancement to the
CTXSpendExceptionclass by addingerrorCodeanderrorBodyparameters. This allows for more precise error handling and diagnosis.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (2)
70-72: Good implementation of Activity Result APIUsing the modern Activity Result API is a good practice for handling intent results.
122-129:❓ Verification inconclusive
Validate email intent launch
The email intent may not resolve if the user doesn't have an email app installed.
🏁 Script executed:
#!/bin/bash # Check if the launcher.launch() call is handling cases where no email app is installed # Look for Intent.resolveActivity() check or similar validation rg -A 5 -B 5 "createEmailIntent|Intent.createChooser" --type kotlinLength of output: 8455
I need to inspect the dialog’s Kotlin code around the intent launch to confirm whether it guards against missing email apps. Let me pull the surrounding lines.
...loredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
Outdated
Show resolved
Hide resolved
...loredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (1)
111-118: 💡 Verification agent🧩 Analysis chain
Verify hardcoded currency values
The
purchaseGiftCardmethod uses hardcoded values "DASH" forcryptoCurrencyand "USD" forfiatCurrencyinstead of using the parameters passed to the function.Is this intentional? The implementation ignores the function parameters and always uses hardcoded values. This could limit the functionality if multiple currencies need to be supported.
🏁 Script executed:
#!/bin/bash # Investigate if these hardcoded values are intentional by checking: # 1. If there are other usages of this function with different currencies # 2. If there are related TODO comments or discussions # Check for usages of this function with different currency parameters echo "Checking for usages of purchaseGiftCard with different currencies:" rg "purchaseGiftCard\(.+,.+,.+,.+\)" -A 1 -B 1 # Check for any related TODOs about currency hardcoding echo "Checking for related TODOs about currency hardcoding:" rg "TODO.*currency" -iLength of output: 333
#!/bin/bash # Locate CTXSpendRepository.kt and inspect the repository's purchaseGiftCard function signature file=$(fd CTXSpendRepository.kt) echo "File located at: $file" echo "Extracting signature and parameters of repository's purchaseGiftCard method:" rg "fun purchaseGiftCard" -n "$file" -A 3 -B 1
🏁 Script executed:
#!/bin/bash # Locate CTXSpendRepository.kt and inspect the purchaseGiftCard function signature repo_file=$(fd CTXSpendRepository.kt) echo "CTXSpendRepository.kt path: $repo_file" rg -n "fun purchaseGiftCard" -A3 -B1 "$repo_file"Length of output: 889
Replace hardcoded currency values with method parameters
In features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (lines 111–118), the override of purchaseGiftCard ignores its cryptoCurrency and fiatCurrency parameters and always uses
"DASH"/"USD". This breaks multi-currency support.• File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt
Lines 111–118Suggested fix:
- api.purchaseGiftCard( - purchaseGiftCardRequest = PurchaseGiftCardRequest( - cryptoCurrency = "DASH", - fiatCurrency = "USD", - fiatAmount = fiatAmount, - merchantId = merchantId - ) - ) + api.purchaseGiftCard( + purchaseGiftCardRequest = PurchaseGiftCardRequest( + cryptoCurrency = cryptoCurrency, + fiatCurrency = fiatCurrency, + fiatAmount = fiatAmount, + merchantId = merchantId + ) + )Or, if only DASH/USD are supported, remove the unused parameters from the method signature.
🧹 Nitpick comments (2)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (2)
56-58: Consider logging the swallowed exceptionWhile providing a default empty map is appropriate for error recovery, the caught exception is swallowed without any logging, which might make debugging difficult in case of unexpected JSON parsing errors.
} catch (e: Exception) { + Log.w("CTXSpendException", "Failed to parse error body", e) emptyMap() }🧰 Tools
🪛 detekt (1.23.7)
[warning] 56-56: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
61-63: Consider improving readability of nested property accessThe property access chain with multiple casts and null checks is complex and could be improved for better readability and maintainability.
val isLimitError get() = errorCode == 400 && - ((errorMap["fields"] as? Map<*, *>)?.get("fiatAmount") as? List<*>)?.firstOrNull() == "above threshold" + getNestedErrorValue("fields", "fiatAmount") == "above threshold" + + @Suppress("UNCHECKED_CAST") + private fun getNestedErrorValue(firstKey: String, secondKey: String): String? { + val fieldsMap = errorMap[firstKey] as? Map<*, *> ?: return null + val valueList = fieldsMap[secondKey] as? List<*> ?: return null + return valueList.firstOrNull() as? String + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.208Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
🪛 detekt (1.23.7)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt
[warning] 56-56: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (1)
36-59: Good implementation of error handling with proper JSON parsingThe changes to the
CTXSpendExceptionclass properly implement error handling for limit mismatch scenarios. The implementation correctly follows the previous review suggestion for safe JSON parsing with appropriate null checks and exception handling.🧰 Tools
🪛 detekt (1.23.7)
[warning] 56-56: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt (1)
25-41: Consider testing additional limit error conditions.The current test only verifies "above threshold" limit errors. Based on the PR objectives, there are likely other limit conditions (like "below threshold") that should also be tested.
@Test fun limitErrorTest() { val errorBody = """ { "fields": { "fiatAmount": [ "above threshold" ], "message": "Bad request" } } """ val exception = CTXSpendException("response error", 400, errorBody) assertEquals(400, exception.errorCode) assertTrue(exception.isLimitError) + + // Test "below threshold" limit error + val belowThresholdErrorBody = """ + { + "fields": { + "fiatAmount": [ + "below threshold" + ], + "message": "Bad request" + } + } + """ + val belowThresholdException = CTXSpendException("response error", 400, belowThresholdErrorBody) + assertEquals(400, belowThresholdException.errorCode) + assertTrue(belowThresholdException.isLimitError) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(4 hunks)features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt (3)
1-15: LGTM: Standard license header.The license header is well-formed and includes appropriate copyright and GPL licensing information.
17-23: Appropriate imports.All necessary imports are included for the test class, including the target exception class and JUnit test assertions.
43-48: LGTM: Effectively tests non-limit error case.The test appropriately verifies that non-limit errors are correctly identified when no error code is provided.
...ures/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt
Outdated
Show resolved
Hide resolved
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes