CCCT-1995 Payment Card Enhancement #3478
Conversation
Changed the payment card's string text.
Refactored payment confirmation logic to include the sum total of all unconfirmed payments. Tweaked the card's string text to contain a number amount followed by a string currency.
Added logic to hide the payment confirmation tile for one week if the user taps the Ask Later button.
Optimized imports.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThe pull request updates payment confirmation handling to support multiple pending payments rather than a single payment. Changes include: translations of the confirmation message across nine languages to use a numeric amount placeholder; addition of a new constant for tracking when the confirmation tile was hidden; and significant refactoring of ConnectDeliveryProgressFragment to manage a list of payments, calculate total unconfirmed amounts, implement a 7-day cooldown before the confirmation tile reappears, and route confirmation actions appropriately to payment updates or tile hiding via preference storage. Sequence DiagramsequenceDiagram
participant User
participant Fragment as ConnectDeliveryProgressFragment
participant PrefMgr as PreferenceManager
participant PaymentSvc as Payment Service
participant PaymentTab as Payment Tab
User->>Fragment: Opens Delivery Progress
Fragment->>Fragment: Check paymentsToConfirm list
Fragment->>PrefMgr: Read PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME
alt Payments exist & network available & cooldown expired
Fragment->>Fragment: Display payment confirmation tile<br/>with totalUnconfirmedPaymentAmount
User->>Fragment: Click Yes button
Fragment->>PaymentSvc: Iterate & call updatePaymentConfirmed<br/>for each payment
PaymentSvc-->>Fragment: Confirmations processed
Fragment->>PaymentTab: Redirect to payment tab
Fragment->>Fragment: Trigger refresh
else User clicks No button
Fragment->>PrefMgr: Store current timestamp to<br/>PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME
Fragment->>Fragment: Hide confirmation tile immediately
else Cooldown active or no network
Fragment->>Fragment: Hide confirmation tile
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/res/values-es/strings.xmlapp/res/values-fr/strings.xmlapp/res/values-hi/strings.xmlapp/res/values-lt/strings.xmlapp/res/values-no/strings.xmlapp/res/values-pt/strings.xmlapp/res/values-sw/strings.xmlapp/res/values-ti/strings.xmlapp/res/values/strings.xmlapp/src/org/commcare/connect/ConnectConstants.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:54:47.940Z
Learning: User OrangeAndGreen organizes messaging-related changes into separate PRs rather than mixing them with other Connect work, which aligns with their preference for handling code issues in separate PRs when they are not directly related to the main changes.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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/res/values/strings.xmlapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-01-21T18:19:05.799Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
Applied to files:
app/src/org/commcare/connect/ConnectConstants.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-06-04T19:17:21.213Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-07-29T14:11:36.386Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-07-29T14:10:55.131Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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/ConnectDeliveryProgressFragment.java
🧬 Code graph analysis (1)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (3)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-57)app/src/org/commcare/connect/ConnectJobHelper.kt (1)
updatePaymentConfirmed(139-169)app/src/org/commcare/fragments/RefreshableFragment.kt (2)
refresh(3-5)refresh(4-4)
⏰ 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). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (12)
app/res/values-ti/strings.xml (1)
352-352: LGTM! Localization update is consistent with the payment confirmation enhancement.The updated string format correctly uses
%dfor the numeric payment amount, followed by%sfor currency and%sfor the opportunity name. This aligns with the PR's objective to confirm the sum total of unconfirmed payments rather than individual payments.app/res/values-hi/strings.xml (1)
350-350: LGTM! Hindi translation follows the updated payment confirmation pattern.The placeholder format
%d %s %scorrectly supports displaying the payment amount, currency, and opportunity name. This is consistent with the broader localization effort across all supported languages.app/res/values-no/strings.xml (1)
129-129: LGTM! Norwegian translation added to complete localization coverage.This addition ensures Norwegian locale users see the payment confirmation prompt in their language. The placeholder format and phrasing are consistent with the other locale implementations.
app/res/values-sw/strings.xml (1)
354-354: LGTM! Swahili translation completes the coordinated localization update.The placeholder format is consistent with all other locale files, properly supporting the enhanced payment confirmation flow. The use of "Je," at the beginning is correct Swahili grammar for forming questions.
All locale files in this PR follow the same pattern and support the feature change to confirm multiple unconfirmed payments rather than individual payments.
app/res/values/strings.xml (1)
473-473: LGTM! Base English string correctly formatted for multiple payment confirmation.The updated format uses
%dfor the total unconfirmed payment amount, and%splaceholders for currency and opportunity name, in exactly the right order. This aligns with the PR's objective to confirm the sum total of payments rather than individual payments.Usage in
ConnectDeliveryProgressFragment.javacorrectly passes:totalUnconfirmedPaymentAmount(numeric),job.getCurrency(), andjob.getTitle().app/res/values-fr/strings.xml (1)
351-351: LGTM! String format updated to support multi-payment confirmation.The updated French translation correctly implements the new payment confirmation message format, using a numeric placeholder for the total amount followed by currency and opportunity name. This aligns with the PR's goal of confirming the sum total of unconfirmed payments.
app/res/values-es/strings.xml (1)
353-353: LGTM! Spanish translation consistent with new format.The Spanish localization correctly follows the new payment confirmation format with numeric amount, currency, and opportunity name placeholders.
app/res/values-pt/strings.xml (1)
368-368: LGTM! Portuguese translation follows the correct format.The Portuguese localization correctly implements the new payment confirmation message format with the numeric amount placeholder followed by currency and opportunity name.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (4)
51-52: LGTM! Clean refactoring to support multiple payments.The change from a single payment to a list of payments, along with tracking the total amount, properly supports the new requirement to confirm multiple unconfirmed payments at once.
148-155: Good extraction of click handlers.Moving the button click logic to dedicated methods improves code organization and readability.
157-163: LGTM! Correct implementation of "Ask Later" behavior.The method properly hides the tile and stores the timestamp for the 7-day cooldown period.
192-199: LGTM! Standard tab navigation logic.The method correctly handles programmatic tab switching and sets the flag to prevent unintended analytics triggers.
| <string name="connect_register_success_message">Jūsų paskyra sukurta ir paruošta naudojimui!</string> | ||
| <string name="connect_backup_code_length">Būtinai atsiminkite savo atsarginį kodą! Jį turi sudaryti %d skaitmenys. Laikykite jį saugioje vietoje, nes jo prireiks norint vėl prisijungti prie paskyros.</string> | ||
| <string name="no_channel_msg">Jūs neturite jokių dabartinių susirašinėjimo kanalų</string> | ||
| <string name="connect_payment_confirm_text">Ar gavote %d %s už %s galimybę?</string> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other payment confirmation strings exist in Lithuanian locale
echo "=== Checking for payment-related strings in Lithuanian ==="
rg -n "connect_payment" app/res/values-lt/strings.xml || echo "No matches found"
echo -e "\n=== Checking for payment-related strings in default locale ==="
rg -n "connect_payment" app/res/values/strings.xml || echo "No matches found"
# Let's also check what else is in the Lithuanian file
echo -e "\n=== Total lines in Lithuanian file ==="
wc -l app/res/values-lt/strings.xml
# Check around lines 77-87 mentioned in the review for Italian strings
echo -e "\n=== Content around lines 77-87 (Italian strings mentioned) ==="
sed -n '75,90p' app/res/values-lt/strings.xmlRepository: dimagi/commcare-android
Length of output: 2852
🏁 Script executed:
#!/bin/bash
# List all locale files to understand the scope
echo "=== All locale resource files ==="
find app/res -name "strings.xml" -type f | sort
# Check if Lithuanian is a new addition by looking at git status
echo -e "\n=== Git status of Lithuanian file ==="
git ls-files app/res/values-lt/strings.xml || echo "File not tracked or doesn't exist in git"
# Check the structure of the Lithuanian file
echo -e "\n=== First 10 and last 10 lines of Lithuanian file ==="
echo "--- First 10 lines ---"
head -10 app/res/values-lt/strings.xml
echo "--- Last 10 lines ---"
tail -10 app/res/values-lt/strings.xmlRepository: dimagi/commcare-android
Length of output: 2527
Remove Italian strings and add missing payment-related translations for Lithuanian locale.
The Lithuanian file contains Italian translations (lines 77-87) that are clearly a copy-paste artifact and should be removed:
nav_drawer_signin_register,nav_drawer_help,nav_drawer_not_signed_in_to_personal_id,nav_drawer_opportunities,nav_drawer_payments,nav_drawer_manage_profile,connect_message_you,connect_message_them, andnav_drawer_open/nav_drawer_closeare in Italian.
Additionally, only connect_payment_confirm_text is translated to Lithuanian, but the default locale includes 15 other payment-related strings that should also be translated:
connect_payment_confirm_yes,connect_payment_confirm_no,connect_payment_confirm_failedconnect_payment_acknowledge_notification_yes,connect_payment_acknowledge_notification_noconnect_payment,connect_payment_received,connect_payment_transferred,connect_payment_confirm_transferred,connect_payment_revoke_transferred,connect_payment_dialog_no,connect_payment_dialog_yes,connect_payment_revert
🤖 Prompt for AI Agents
In app/res/values-lt/strings.xml around lines 129 (and remove the Italian
entries around lines 77–87), the Lithuanian file contains Italian copy-paste
strings that must be deleted and several payment-related keys are missing
Lithuanian translations; remove the Italian entries for
nav_drawer_signin_register, nav_drawer_help,
nav_drawer_not_signed_in_to_personal_id, nav_drawer_opportunities,
nav_drawer_payments, nav_drawer_manage_profile, connect_message_you,
connect_message_them, nav_drawer_open and nav_drawer_close, then add Lithuanian
translations for the missing payment keys: connect_payment_confirm_yes,
connect_payment_confirm_no, connect_payment_confirm_failed,
connect_payment_acknowledge_notification_yes,
connect_payment_acknowledge_notification_no, connect_payment,
connect_payment_received, connect_payment_transferred,
connect_payment_confirm_transferred, connect_payment_revoke_transferred,
connect_payment_dialog_no, connect_payment_dialog_yes and connect_payment_revert
using the same formatting/placeholders as the default locale.
| public final static String PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME = | ||
| "payment_confirmation_hidden_since_time"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME constant
echo "=== Searching for usage of PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME ==="
rg -n "PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME" app/src/
echo -e "\n=== Searching for preference storage patterns with this key ==="
rg -n "payment_confirmation_hidden_since_time" app/src/Repository: dimagi/commcare-android
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Get context around the storage and retrieval usage
echo "=== Context around line 162 (storage) ==="
sed -n '155,170p' app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
echo -e "\n=== Context around line 271 (retrieval) ==="
sed -n '265,285p' app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
echo -e "\n=== Search for any clearing/reset of this preference ==="
rg -n -B 2 -A 2 "remove.*PAYMENT_CONFIRMATION|PAYMENT_CONFIRMATION.*remove" app/src/
echo -e "\n=== Search for 7-day or day constant in payment context ==="
rg -n "7.*day|day.*7|86400000|604800000" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaRepository: dimagi/commcare-android
Length of output: 1975
🏁 Script executed:
#!/bin/bash
# Search for preference clearing/removal patterns
echo "=== Search for preference removal/clear methods ==="
rg -n "remove\(|clear\(|putLong.*\-1" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
echo -e "\n=== Check handlePaymentConfirmYesButtonClick full method ==="
rg -n -A 30 "private void handlePaymentConfirmYesButtonClick" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
echo -e "\n=== Search for where payments are synced/updated ==="
rg -n "paymentsToConfirm|refreshPayments|updatePayments" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java | head -20
echo -e "\n=== Check if there's a method to reset/refresh payment state ==="
rg -n "private void.*[Pp]ayment|private void.*[Tt]ile" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaRepository: dimagi/commcare-android
Length of output: 2174
🏁 Script executed:
#!/bin/bash
# Find updatePaymentConfirmationTile method implementation
echo "=== Find updatePaymentConfirmationTile method ==="
rg -n -A 20 "private void updatePaymentConfirmationTile" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
echo -e "\n=== Check the refresh method to see if it resets the preference ==="
rg -n -A 15 "private void refresh\(\)" app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaRepository: dimagi/commcare-android
Length of output: 1349
Add logic to clear the payment confirmation hidden preference when appropriate.
The constant PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME is used consistently for storage and retrieval, and the 7-day calculation correctly uses DateUtils.DAY_IN_MS * 7. However, the preference is never cleared when user confirms payments or when new payments arrive. Currently, once a user clicks "No" to hide the tile, it will remain hidden for 7 days regardless of whether the pending payments are confirmed or new payments arrive. The preference should be cleared in updatePaymentConfirmationTile() or in the callback after ConnectJobHelper.updatePaymentConfirmed() succeeds to ensure the tile can appear immediately again when new payments require confirmation.
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/ConnectConstants.java around lines 55-56, the
PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME preference is never cleared so the
payment confirmation tile stays hidden for the full 7 days even after payments
are confirmed or new ones arrive; update the logic to remove/clear this
preference (e.g.,
SharedPreferences.edit().remove(PAYMENT_CONFIRMATION_HIDDEN_SINCE_TIME).apply()
or set it to 0) when a payment confirmation succeeds or when
updatePaymentConfirmationTile detects new pending confirmations so the tile can
reappear immediately; perform the clear in the success callback of
ConnectJobHelper.updatePaymentConfirmed() or at the start of
updatePaymentConfirmationTile() after verifying new/confirmed payment state,
ensuring edits are applied on the appropriate thread/context.
There was a problem hiding this comment.
@shubham1g5 @Jignesh-dimagi what are your thoughts here? Do we want to essentially reset the timer when there is a new payment during the 7 days? I was assuming that Product wants the payment confirmation tile to remain hidden if the user taps on "Ask Later" for 7 days no matter what new payments occur during that timeframe
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Based on discussion we had in backlog meeting, think we want to display it again if there is a new payment.
There was a problem hiding this comment.
@coderabbitai @shubham1g5 Let me know what you think of this commit - c212637. It's a bit tricky to know if there was a new payment dated after the user hiding the payment confirmation card but before the end of the 7 days. This is because the dates on each ConnectJobPaymentRecord are not precise (i.e. the date is not aware of the exact hour/minute - they are essentially rounded down to 00:00 for whatever day). So I opted to check for the total number of unconfirmed payments rather than if a payment was received at a later date.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the commit details
echo "=== Commit details ==="
git show c212637697620eb5b69800c99f85d84bd5772fc9 --stat
echo -e "\n=== Full commit changes ==="
git show c212637697620eb5b69800c99f85d84bd5772fc9
Length of output: 4887
@conroy-ricketts Your approach looks solid! Using the count comparison is a smart solution given the date precision constraint. Here's my assessment:
Strengths:
- ✅ Pragmatic solution: Checking total count avoids the complexity of date comparison when payment dates only have day-level precision
- ✅ Correct detection: If
totalUnconfirmedPayments > lastTotalUnconfirmedPayments, at least one new payment must have arrived - ✅ Preserves intended behavior: The 7-day hide period still works when no new payments arrive
- ✅ Handles confirmations correctly: If a payment gets confirmed during the hide period, the count decreases, so the tile won't reappear (correct behavior)
Minor edge case (acceptable trade-off):
If exactly one payment gets confirmed AND one new payment arrives during the hide period, the counts would be equal and the tile wouldn't show until the 7 days pass. However, this is a reasonable trade-off given the date precision constraints, and the scenario is relatively unlikely.
The implementation integrates cleanly with the existing logic and achieves the product requirement of showing the tile again when new payments arrive. 👍
🧠 Learnings used
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
Learnt from: conroy-ricketts
Repo: dimagi/commcare-android PR: 3477
File: app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java:265-284
Timestamp: 2026-01-02T16:07:46.905Z
Learning: In ConnectJobPaymentRecord and related Connect payment sorting code, the date field returned by getDate() is guaranteed to be non-null through the application's initialization logic and data flow. Following the fail-fast philosophy used throughout the Connect feature, null pointer exceptions from date comparisons should be allowed to crash rather than adding defensive null checks, as this helps catch programming errors during development.
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Show resolved
Hide resolved
Tweaked code to prevent potential race conditions when confirming multiple payments.
…nto story/CCCT-1995-payment-card-enhancement
…nto story/CCCT-1995-payment-card-enhancement
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Outdated
Show resolved
Hide resolved
| for (ConnectJobPaymentRecord paymentToConfirm : paymentsToConfirm) { | ||
| ConnectJobHelper.INSTANCE.updatePaymentConfirmed( | ||
| getContext(), | ||
| paymentToConfirm, | ||
| true, | ||
| success -> { |
There was a problem hiding this comment.
Sorry that I didn't realise this before prioritizing this ticket but think this must be one HTTP call that can take multiple payment ids in the request body and confirm all of them together. Otherwise this can be real slow on slow network connections with multiple payments to confirm.
Are you able to add a server ticket for the same ?
There was a problem hiding this comment.
Yeah good call, I'll update this ticket to blocked until the new ticket is implemented on server.
There was a problem hiding this comment.
Linking the server ticket here for future reference: CCCT-2031
| getBinding().connectPaymentConfirmYesButton.setEnabled(true); | ||
| getBinding().connectPaymentConfirmNoButton.setEnabled(true); |
There was a problem hiding this comment.
do we need to re-enable here ? If so, it should happen even if allPaymentsUpdated is false ?
There was a problem hiding this comment.
I was also considering if this was a good place to re-enable them because we can also re-enable them whenever we show the payment confirmation card. Anyways, I only did this to prevent potential race conditions. So this will be removed once server gives us a way to update all payments in a single API call as discussed here
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Outdated
Show resolved
Hide resolved
Ensured that the card is shown when a new payment is received after the user hides it.
Let the app crash if the payment confirmation card is shown but there are no payments to confirm.
Refactored logic for showing/hiding the payment confirmation card.
CCCT-1995
Product Description
I updated the payment confirmation card.
Screen_Recording_20260102_164107_CommCare.Debug.mp4
Technical Summary
The biggest change I made is that instead of confirming a single payment, we are now confirming the sum total of unconfirmed payments. Also, if the user taps "Ask Later" the payment confirmation card is hidden for a week.
Edit: These changes are blocked until this server ticket is implemented: CCCT-2031. I will update this PR after that is done.
Safety Assurance
Safety story
QA Plan
Verify that the payment card is updated as expected.