CCCT-1996 Sort Payments Tab

[conroy-ricketts]

CCCT-1996

Product Description

I sorted the payments tab from newest to oldest with the unconfirmed payments at the top.

Before (on the left side) and after (on the right side):

Technical Summary

I think that this one is fairly straightforward. In the adapter, I basically created and sorted a payments list to base the UI off of.

Safety Assurance

Safety story

• Verified that the list is sorted correctly.
• Verified that the received/revert buttons still work correctly.

QA Plan

• Verify that the list is sorted correctly.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request modifies the ResultsAdapter class within ConnectResultsSummaryListFragment.java to introduce a dedicated payments list that organizes payment records for display. The adapter now builds a separate list by partitioning job payments into confirmed and unconfirmed groups, sorts each group by date in descending order, and concatenates unconfirmed payments before confirmed ones. The adapter's item binding and item count logic are updated to reference this new payments list instead of directly accessing the job's payment collection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Pm phase4 coderabbit #3200: Modifies the same ResultsAdapter class in ConnectResultsSummaryListFragment.java and changes how payment items are handled in PaymentViewHolder binding.

Suggested labels

skip-integration-tests

Suggested reviewers

• OrangeAndGreen
• shubham1g5
• Jignesh-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'CCCT-1996 Sort Payments Tab' clearly and specifically describes the main change: sorting the payments tab.
Description check	✅ Passed	The PR description includes Product Description with screenshots, Technical Summary, and Safety Assurance sections with testing details, though the QA Plan is minimal and some optional template sections are not addressed.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch story/CCCT-1996-sort-payments-tab

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0b18ff and fe84af7.

📒 Files selected for processing (1)

• app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java

🧰 Additional context used

🧠 Learnings (7)

📓 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.

📚 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/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.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/fragments/connect/ConnectResultsSummaryListFragment.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/ConnectResultsSummaryListFragment.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/ConnectResultsSummaryListFragment.java

📚 Learning: 2025-07-29T14:10:58.243Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:10:58.243Z
Learning: In ConnectJobsListsFragment.java, the team intentionally uses different error handling strategies: JSONException should throw RuntimeException to crash the app (fail-fast for data contract violations), while IOException should be logged and allow graceful continuation (for network/stream issues). This follows the established Connect module pattern of treating different exception types differently based on their nature.

Applied to files:

• app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.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/ConnectResultsSummaryListFragment.java

⏰ 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 (4)

app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (4)

35-37: LGTM!

The import additions are appropriate for the sorting implementation.

---

109-109: Correctly references the sorted payments list.

The change to use payments.get(position) instead of job.getPayments().get(position) is correct for displaying the sorted list. However, ensure the list is kept fresh (see comment on lines 84-94).

---

161-161: LGTM!

Correctly uses the sorted payments list size for the item count.

---

273-279: LGTM! The sorting logic correctly orders payments from newest to oldest.

The comparator payment2.getDate().compareTo(payment1.getDate()) correctly sorts in descending order (newest first), which matches the PR requirement.

[OrangeAndGreen]

Noting that the list won't re-sort when the user confirms or unconfirms a payment, but I think that's probably the UX we want. This way the UI doesn't jump when the user presses the button, and when the user comes back to the list later any changed payments will be sorted correctly.

[OrangeAndGreen]

Noting that this function could somewhat easily be refactored to be static (taking job as input and returning the sorted payments list). Functionally it would be the same but in my history static methods have been preferred to:

• Ensure limited scope when possible (so code readers easily know whether a function will modify an object's state)
• Simplify testing (i.e. the method can be tested without instantiating the fragment)

Is that something we want to prioritize in our codebase? Searching through the code, it doesn't seem like something that's necessarily been a focus in the past (most of the static functions I see are in PersonalID/Connect code).

This could be a bigger conversation for the team to contribute to and I'm fine with leaving the code as is here.

[conroy-ricketts]

Interesting, I could go either way on this.

On one hand, I see the argument for private non-static methods when there is no reason for the methods to be called from outside of their class. For example, I originally created sortPaymentsByDate() just for readability. And I created buildPaymentsDisplayList() to just basically build a displayable list of UI data in a specific way for a specific Fragment.

On the other hand, I see where you're coming from. I could see an argument for sortPaymentsByDate() being moved or converted to a public static function because it can easily be decoupled from the UI and reused in the future for something else. And it could also benefit from unit testing. Also, buildPaymentsDisplayList() can be decoupled and refactored to just take a list of payments and return an ordered list like you said and it could be viewed more as a reusable utility function.

I'm very curious what your thoughts are here @Jignesh-dimagi @shubham1g5

[shubham1g5]

Agree that there are pros and cons of private vs private static but I don't think we necessarily wants to write private static methods as a practice. To me it's more imporatnt, that we define these methods correctly, that is if a function doesn't depend on class state it should be defined as static to communicate that the method doesn't affect the class state.

Also to make these testables, these would need to be define as public static and not only as static as otherwise the method won't be accessible in test classes.

Think a rule of thumb here is to use static when a method has a pure input-> output contract but I would not worry a lot about converting methods to static deliberately unless we see a solid benefit to do that.

[conroy-ricketts]

Noting that the list won't re-sort when the user confirms or unconfirms a payment, but I think that's probably the UX we want. This way the UI doesn't jump when the user presses the button, and when the user comes back to the list later any changed payments will be sorted correctly.

@OrangeAndGreen Oh that's an interesting take, I just pushed a commit to resort the list when that happens. @shubham1g5 what are your thoughts here?

[shubham1g5]

My general instinct here is to re-sort and reload the UI in case of changes triggered by user actions (more consistent and user expects it to change given user just clicked something), so would go with that unless it causes a big issue in UX.