Handled failed job claim #3302
Handled failed job claim #3302
Conversation
📝 WalkthroughWalkthroughAdds a new user-facing error string recovery_unable_to_claim_opportunity across default (en) and localized (es, fr, hi, pt, sw, ti) resources. Introduces a new public utility method ViewUtils.showSnackBarForDismiss(View, String). Updates ConnectDeliveryDetailsFragment’s ConnectApi failure handling: when errorCode is BAD_REQUEST_ERROR, shows a Snackbar anchored to the view with the new recovery message; otherwise retains existing Toast-based error handling. No other logic or resource keys are modified. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Fragment as ConnectDeliveryDetailsFragment
participant API as Connect API
participant View as ViewUtils
participant UI as Snackbar/Toast
User->>Fragment: Trigger claim delivery
Fragment->>API: call claimOpportunity()
API-->>Fragment: onFailure(errorCode, errorBody)
alt errorCode == BAD_REQUEST_ERROR
Fragment->>View: showSnackBarForDismiss(rootView, strings.recovery_unable_to_claim_opportunity)
View->>UI: Show Snackbar (Indefinite, OK dismiss)
else other errors
Fragment->>UI: Show Toast (PersonalIdApiErrorHandler.handle(...))
end
note over Fragment: FirebaseAnalyticsUtil.reportCccApiClaimJob(false)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (2)
app/res/values/strings.xml (1)
233-233: Minor copy tweak for clarity and toneConsider avoiding the comma splice and making the message a bit more specific/contextual.
Apply this diff:
- <string name="recovery_unable_to_claim_opportunity">Unable to claim the opportunity, please contact an administrator.</string> + <string name="recovery_unable_to_claim_opportunity">Unable to claim this opportunity. Please contact an administrator.</string>Optional: This message is used within Connect flows; if you prefer strict resource grouping, consider renaming to connect_unable_to_claim_opportunity and relocating it under the Connect strings. Not required, but improves discoverability.
app/src/org/commcare/utils/ViewUtils.java (1)
16-19: Localize the action label instead of hard-coding "OK"Use the existing ok string resource to respect localization, and avoid hardcoded UI text.
Apply this diff to the new helper:
- snackbar.setAction("OK", v -> snackbar.dismiss()); + snackbar.setAction(view.getContext().getString(R.string.ok), v -> snackbar.dismiss());Also update the existing helper for consistency:
- Snackbar.make(view, message, Snackbar.LENGTH_INDEFINITE) - .setAction("OK", okClickListener) - .show(); + Snackbar.make(view, message, Snackbar.LENGTH_INDEFINITE) + .setAction(view.getContext().getString(R.string.ok), okClickListener) + .show();You will need the R import:
// add near other imports import org.commcare.dalvik.R;Also applies to: 21-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/res/values-es/strings.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-hi/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java(2 hunks)app/src/org/commcare/utils/ViewUtils.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-07-29T14:10:55.131Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
🧬 Code Graph Analysis (1)
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (2)
app/src/org/commcare/utils/ViewUtils.java (1)
ViewUtils(7-26)app/src/org/commcare/connect/network/connectId/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(26-70)
🔇 Additional comments (8)
app/res/values-hi/strings.xml (1)
181-181: LGTM: Key added and aligned across localesThe new recovery_unable_to_claim_opportunity string looks consistent with the English source and placement within the recovery section.
app/res/values-ti/strings.xml (1)
172-172: LGTM: Added localized string and consistent groupingThe recovery_unable_to_claim_opportunity key is correctly added and grouped with recovery messages. No structural issues.
app/res/values-es/strings.xml (1)
125-125: LGTM: Spanish copy reads wellThe translation is clear and matches the intent of the English source. Placement in the recovery section is consistent.
app/res/values-pt/strings.xml (1)
184-184: PT translation for recovery_unable_to_claim_opportunity looks goodKey name aligns with other locales. Wording and punctuation are clear and user-friendly.
app/res/values-sw/strings.xml (1)
185-185: SW translation for recovery_unable_to_claim_opportunity looks goodAccurate and consistent with intent; no placeholders needed.
app/res/values-fr/strings.xml (1)
184-184: FR translation for recovery_unable_to_claim_opportunity looks goodClear phrasing; consistent with English source and other locales.
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (2)
3-4: Static import for Snackbar helper is appropriateKeeps the callsite concise and readable; no conflicts observed.
113-117: Localize “OK” action and ensure UI calls run on the main threadGood catch on targeting the BAD_REQUEST case with a Snackbar. A couple of follow-ups:
• In ViewUtils.showSnackBarForDismiss, replace the hard-coded “OK” with the localized string:
public static void showSnackBarForDismiss(View view, String message) { Snackbar snackbar = Snackbar.make(view, message, Snackbar.LENGTH_INDEFINITE); - snackbar.setAction("OK", v -> snackbar.dismiss()); + snackbar.setAction(view.getContext().getString(R.string.ok), v -> snackbar.dismiss()); snackbar.show(); }• Verify that ConnectApiHandler invokes its onSuccess/onFailure callbacks on Android’s main thread. If it does not, wrap your UI calls like:
requireActivity().runOnUiThread(() -> showSnackBarForDismiss(binding.getRoot(), getString(R.string.recovery_unable_to_claim_opportunity)) );
| <string name="recovery_network_server_error">There was an error processing your request.</string> | ||
| <string name="recovery_network_cooldown">Too many request attempts, try again after an hour.</string> | ||
| <string name="recovery_network_unknown">Something went wrong, please try again.</string> | ||
| <string name="recovery_unable_to_claim_opportunity">Unable to claim the opportunity, please contact an administrator.</string> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify string usage and translation coverage
Ensure all targeted locales include this key and that the code references the resource (not a hardcoded string).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Resource entries for recovery_unable_to_claim_opportunity:"
rg -n 'name="recovery_unable_to_claim_opportunity"' app/res -C1 -g '!**/build/**'
echo
echo "Java/Kotlin references to the string:"
rg -nP '\bR\.string\.recovery_unable_to_claim_opportunity\b' app/src -C2 -g '!**/build/**'
echo
echo "Calls to showSnackBarForDismiss (should use the resource string):"
rg -nP '\bshowSnackBarForDismiss\s*\(' app/src -C2 -g '!**/build/**'Length of output: 6212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking translation coverage for 'recovery_unable_to_claim_opportunity':"
for dir in app/res/values*; do
if [ -f "$dir/strings.xml" ]; then
if grep -q 'name="recovery_unable_to_claim_opportunity"' "$dir/strings.xml"; then
echo "$dir: present"
else
echo "$dir: MISSING'
fi
fi
doneLength of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Translation coverage for 'recovery_unable_to_claim_opportunity':"
for dir in app/res/values*; do
xml="$dir/strings.xml"
if [ -f "$xml" ]; then
if grep -q 'name="recovery_unable_to_claim_opportunity"' "$xml"; then
echo "${dir##*/}: present"
else
echo "${dir##*/}: MISSING"
fi
fi
doneLength of output: 4868
Missing translations in Lithuanian and Norwegian
All code references use the resource (no hard-coded strings), and the key is present in the default and most locale folders. However, the following locale-specific strings.xml files are missing the recovery_unable_to_claim_opportunity entry:
- app/res/values-lt/strings.xml: MISSING
- app/res/values-no/strings.xml: MISSING
Please add the localized string for these languages (or confirm they aren’t targeted).
🤖 Prompt for AI Agents
In app/res/values/strings.xml around line 233 the key
recovery_unable_to_claim_opportunity exists in default strings but is missing
from locale files; add the same string resource key to
app/res/values-lt/strings.xml and app/res/values-no/strings.xml with appropriate
Lithuanian and Norwegian translations respectively (or state in a PR note that
those locales aren’t targeted), ensuring correct XML syntax, proper escaping,
and placement inside the <resources> block matching project formatting.
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
Outdated
Show resolved
Hide resolved
Localized ok string in other languages
| .setAction("OK", okClickListener) | ||
| .show(); | ||
| } | ||
| public static void showSnackBarForDismiss(View view, String message) { |
There was a problem hiding this comment.
nit: showSnackBarForDismiss -> showSnackBarWithDismissAction
jaypanchal-13
left a comment
jaypanchal-13
left a comment
There was a problem hiding this comment.
Addressed comments
|
@damagatchi retest this please |
|
@OrangeAndGreen waiting for your review inorder to merge |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Left a couple nits but all in all it looks good
| @Override | ||
| public void onFailure(@NonNull PersonalIdOrConnectApiErrorCodes errorCode, @Nullable Throwable t) { | ||
| Toast.makeText(requireContext(), PersonalIdApiErrorHandler.handle(requireActivity(), errorCode, t), Toast.LENGTH_LONG).show(); | ||
| if (errorCode == PersonalIdOrConnectApiErrorCodes.BAD_REQUEST_ERROR) { |
There was a problem hiding this comment.
Nit: We could use the if only to choose the string into a variable, then have a single line calling showSnackBarWithDismissAction (to reduce duplicated code).
| } | ||
| public static void showSnackBarWithDismissAction(View view, String message) { | ||
| Snackbar snackbar = Snackbar.make(view, message, Snackbar.LENGTH_INDEFINITE); | ||
| snackbar.setAction(view.getContext().getString(R.string.connect_register_success_button),v -> snackbar.dismiss()); |
There was a problem hiding this comment.
Nit: Seems odd to use a string that indicates it's specific to Connect here (connect_register_success_button), I wonder if there's a better common string we could use (or create a new one)?
There was a problem hiding this comment.
@OrangeAndGreen instead of using 'connect_register_success_button' changed it use 'ok'
jaypanchal-13
dismissed stale reviews from OrangeAndGreen and shubham1g5
via
ddb2aab
|
@damagatchi retest this please |
Product Description
Ticket - https://dimagi.atlassian.net/browse/CCCT-1608
Handled failed job claim
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review