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 | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
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
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Outdated
Show resolved
Hide resolved
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.
…nto story/CCCT-1995-payment-card-enhancement
Tweaked payment confirmations API endpoint to take multiple payments at a time.
|
@shubham1g5 @OrangeAndGreen @jaypanchal-13 @Jignesh-dimagi Even though we are still blocked by the Server ticket (CCCT-2031), I went ahead and updated this PR assuming that the new API works so that everyone can take a look. These changes will not be merged in until that blocking Server ticket is complete and I can thoroughly test the changes in this PR. I'll let y'all know when I'm able to test this. |
Fixed merge conflict with master
Fixed values in API request body for confirming payments.
|
@shubham1g5 @OrangeAndGreen @jaypanchal-13 @Jignesh-dimagi This PR is now ready for a full review! I tested the new API endpoint from Server and confirmed that the latest changes work as expected. I updated the PR description with more details |
Ran Ktlint.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Show resolved
Hide resolved
…nto story/CCCT-1995-payment-card-enhancement
Added check for if user is already on payment tab before redirecting.
Simplified logic for getting payments to confirm.
…nto story/CCCT-1995-payment-card-enhancement
Simplified logic for hiding the payment confirmation tile.
Moved some logic for resetting the payment confirmation tile timer outside of for loop.
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
One translation to fix but otherwise all looks good to me
Remove Italian strings and add missing payment-related translations for Lithuanian locale.
…nto story/CCCT-1995-payment-card-enhancement
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 implement the new API endpoint from this Server ticket: CCCT-2031.
I updated our code to no longer use the old API endpoint for confirming singular payments.
So instead of this:
"/api/payment/{id}/confirm"We are now only using this:
"/api/payment/confirm"I figured that this was the best course of action so that we did not have to create a bunch of new code to support two different API endpoints. The new API endpoint takes in a list of payments to confirm, so now when we want to confirm just one singular payment, we just simply give a list of one singular payment to confirm.
Safety Assurance
Safety story
Edit:
QA Plan