[Draft] Added snackbar to show success / failure of network data #3459
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis pull request introduces a network state management framework for the Connect delivery feature. A new base ViewModel ( Sequence DiagramsequenceDiagram
participant Fragment as ConnectDeliveryProgressFragment
participant VM as ConnectDeliveryProgressFragmentViewModel
participant Helper as ConnectJobHelper
participant Base as BaseConnectFragment
participant Observer as NetworkState Observer
participant UI as SnackBar UI
Fragment->>Fragment: onResume() triggered
Fragment->>VM: fetchDeliveryProgress()
VM->>VM: launchTask()
VM->>VM: networkStateLiveData = Loading
VM->>Helper: updateDeliveryProgress(context, job, showLoading, view)
Helper-->>VM: onComplete(success)
VM->>VM: networkStateLiveData = Success/Error
VM->>Observer: LiveData observes networkState change
Observer->>Base: Observer triggered
alt Success State
Base->>Base: showNetworkSuccessSnackBar()
Base->>UI: Display "New data available" + Refresh action
Base->>Fragment: refreshUiWithNetworkData()
Fragment->>Fragment: Update UI (timestamps, messages, adapter)
else Error State
Base->>Base: showNetworkFailureSnackBar()
Base->>UI: Display "Something went wrong" + Show Old Data action
Base->>Fragment: showOldDataOnUi()
Fragment->>Fragment: Display cached/old data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (8)
app/src/org/commcare/fragments/connect/base/BaseNetworkStateViewModel.kt (1)
14-16: Consider a more descriptive parameter name forSuccess.The
stateboolean parameter is vague. Based on the PR objectives, this appears to indicate whether new data is available. A clearer name would improve readability.data class Success( - val state: Boolean, + val hasNewData: Boolean, ) : NetworkState()app/src/org/commcare/utils/ViewUtils.java (1)
24-27: Minor formatting inconsistencies.Missing spaces after commas in method signature and
setActioncall.- public static void showSnackBarWithDismissAction(String btnText,View view, String message) { + public static void showSnackBarWithDismissAction(String btnText, View view, String message) { Snackbar snackbar = Snackbar.make(view, message, Snackbar.LENGTH_INDEFINITE); - snackbar.setAction(btnText,v -> snackbar.dismiss()); + snackbar.setAction(btnText, v -> snackbar.dismiss()); snackbar.show(); }app/src/org/commcare/fragments/base/BaseConnectFragment.kt (1)
86-106: ClarifyNetworkState.Success.statesemantics to avoid misusing the failure snackbarRight now the ViewModel only ever emits
NetworkState.Success(true)(errors go throughNetworkState.Error), so theelsebranch forSuccess(false)is effectively dead code. Once you wire inlastUpdate > previousLastUpdatedOnand start usingstateto mean “new data available”, you probably don’t wantSuccess(false)to show the same “old data / something went wrong” snackbar as the true error path.It may be clearer to:
- Treat
NetworkState.Erroras “network failed, you might be seeing old data”.- Treat
NetworkState.Success(true)as “new data available” (show success snackbar).- Treat
NetworkState.Success(false)as “no new data” and show nothing.That keeps the UX aligned with the PR description and avoids conflating “no new data” with “request failed”.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragmentViewModel.kt (1)
11-14: UnusedpreviousLastUpdatedOnsuggests partially implemented last‑modified logic
previousLastUpdatedOnis never read yet, and the related TODOs are commented out. That’s fine for a draft, but it will show up as dead state and can confuse future readers.Either:
- Wire it into the comparison logic once the server exposes
lastUpdate, or- Remove it (or add a brief comment) until that behavior is actually implemented.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (4)
119-122: ViewModel wiring looks good; consider a typed accessor to avoid repeated castsUsing
ViewModelProvider(this).get(ConnectDeliveryProgressFragmentViewModel.class)and feeding it into the base network‑state machinery is clean and consistent with the new infra.Since you cast
getBaseNetworkStateViewModel()toConnectDeliveryProgressFragmentViewModelin multiple places, you might consider a small helper likeprivate ConnectDeliveryProgressFragmentViewModel getVm()to centralize the cast, but that’s purely a readability/maintainability nicety.
125-130: Use job’s actual last‑update timestamp instead ofnew Date()for the “Last updated” labelIn
refreshUiWithNetworkData, you now call:updateLastUpdatedText(new Date());On initial load you use
job.getLastDeliveryUpdate(), which presumably reflects the job’s stored timestamp. For consistency and to keep the label tied to the data’s actual last‑update time (especially once server timestamps are wired in), it may be more accurate to pass the job’s updated value instead, e.g.:updateLastUpdatedText(job.getLastDeliveryUpdate());That way the UI doesn’t drift to “time when snackbar was tapped” vs “time when data actually changed”.
132-141: Confirm manual refresh UX: sync + blocking progress currently still requires a snackbar tap to update UIWith the new flow:
refresh()setsshowLoadingto true and callsfetchDeliveryProgress().- The ViewModel reports success through
NetworkState.Success(true).BaseConnectFragmentalways responds toSuccess(true)by showing the “new data is available; press here to refresh” snackbar.- The UI is only updated when the user taps that snackbar (
refreshUiWithNetworkData).This means even after pressing the explicit sync button (with blocking progress), the user must still press a second button in the snackbar to see refreshed data. The PR description could also be interpreted as “sync button triggers fetch and UI update once complete”.
If the desired UX for manual sync is “press once, data refreshes when done”, you might want to:
- Auto‑invoke
refreshUiWithNetworkData()on success whenshowLoading == true, and- Reserve the snackbar “tap to refresh” behavior for the silent background fetch.
That would require some coordination between the ViewModel’s
showLoadingflag and how you handleNetworkState.Successin the fragment or base class.
188-193: Repeated fetch ononResume()is fine; just be aware of network frequencyCalling
fetchDeliveryProgress()inonResume()ensures the screen always attempts a background refresh when the user returns and is logged in, which matches the design goal of a silent check.Just be mindful that this will trigger on every resume (e.g., app switch), so if the endpoint is expensive or subject to rate limits you may eventually want simple debouncing (time‑based or state‑based) here. For now it’s reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/fragments/base/BaseConnectFragment.kt(3 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(4 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragmentViewModel.kt(1 hunks)app/src/org/commcare/fragments/connect/base/BaseNetworkStateViewModel.kt(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(2 hunks)app/src/org/commcare/utils/ViewUtils.java(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 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: 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-02-04T21:29:29.594Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
📚 Learning: 2025-03-10T08:16:29.416Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:173-247
Timestamp: 2025-03-10T08:16:29.416Z
Learning: In the ConnectIdPasswordVerificationFragment, password comparisons should use MessageDigest.isEqual() rather than equals() to prevent timing attacks, and empty password validation should be implemented before verification attempts.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.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/personalId/PersonalIdBiometricConfigFragment.java
📚 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/ConnectDeliveryProgressFragmentViewModel.ktapp/src/org/commcare/fragments/base/BaseConnectFragment.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-06-20T15:51:14.157Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/models/connect/ConnectLoginJobListModel.java:79-92
Timestamp: 2025-06-20T15:51:14.157Z
Learning: The ConnectLoginJobListModel class in app/src/org/commcare/models/connect/ConnectLoginJobListModel.java does not need to implement Parcelable interface as it is not passed between Android activities or fragments.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragmentViewModel.ktapp/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/ConnectDeliveryProgressFragmentViewModel.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.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/ConnectDeliveryProgressFragmentViewModel.ktapp/src/org/commcare/fragments/base/BaseConnectFragment.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.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/base/BaseConnectFragment.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-03-10T08:16:59.436Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Applied to files:
app/src/org/commcare/fragments/base/BaseConnectFragment.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.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.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-04-21T15:02:17.492Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3042
File: app/src/org/commcare/fragments/BreadcrumbBarViewModel.java:50-55
Timestamp: 2025-04-21T15:02:17.492Z
Learning: ViewModels should not store View or Activity references as this can cause memory leaks. Unlike Fragments with setRetainInstance(true), ViewModels don't have automatic view detachment mechanisms. When migrating from Fragments to ViewModels, view references should be replaced with data-only state in the ViewModel.
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/ConnectDeliveryDetailsFragment.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 the Connect error handling flow of CommCare Android, error messages are shown once and then automatically cleared because the underlying error record gets deleted after being displayed. This is why there's no need for explicit methods to hide these messages.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.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/ConnectDeliveryDetailsFragment.java
🧬 Code graph analysis (3)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
app/src/org/commcare/utils/ViewUtils.java (1)
ViewUtils(9-29)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragmentViewModel.kt (1)
app/src/org/commcare/fragments/connect/base/BaseNetworkStateViewModel.kt (1)
launchTask(31-36)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (2)
app/src/org/commcare/fragments/base/BaseConnectFragment.kt (2)
startListeningToNetworkState(86-106)refreshUiWithNetworkData(130-130)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragmentViewModel.kt (1)
fetchData(15-43)
⏰ 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 (5)
app/res/values/strings.xml (1)
702-703: LGTM!The new string resources are clear and appropriately placed. The user-facing messages effectively communicate network state to users.
app/src/org/commcare/fragments/connect/base/BaseNetworkStateViewModel.kt (1)
31-36: LGTM!The
launchTaskfunction correctly sets theLoadingstate before executing the task and uses the coroutine exception handler for error propagation.app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
43-43: LGTM!The import and method call are correctly updated to use the new
showSnackBarWithAPI with an explicit button label.Also applies to: 300-301
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (1)
110-110: LGTM!The snackbar call correctly uses the updated
showSnackBarWithDismissActionsignature with an explicit "OK" button label.app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragmentViewModel.kt (1)
15-43: EnsureconnectActivityCompleteruns on the main thread before callingnetworkStateLiveData.value = …Inside
connectActivityComplete, you callnetworkStateLiveData.value = ….LiveData.setValuemust be invoked on the main thread; ifConnectJobHelper.updateDeliveryProgressever invokes the listener from a background thread, this will crash intermittently.Please confirm the threading guarantees of
ConnectJobHelper.updateDeliveryProgress:
- If the callback is always on the main thread, this is fine.
- If not guaranteed, wrap updates in
postValueor a main‑dispatcher launch, e.g.:// inside connectActivityComplete if (success) { networkStateLiveData.postValue(NetworkState.Success(true)) } else { networkStateLiveData.postValue(NetworkState.Error(Exception("UNKNOWN_EXCEPTION"))) }This keeps the network‑state infra robust regardless of how the helper dispatches callbacks.
| protected val handler = | ||
| CoroutineExceptionHandler { _, exception -> | ||
| networkStateLiveData.value = NetworkState.Error(exception as Exception) | ||
| } |
There was a problem hiding this comment.
Unsafe cast from Throwable to Exception.
The CoroutineExceptionHandler receives a Throwable, which may not always be an Exception (e.g., Error subclasses like OutOfMemoryError). The cast on line 28 could throw ClassCastException at runtime.
Apply this diff to handle the cast safely:
protected val handler =
CoroutineExceptionHandler { _, exception ->
- networkStateLiveData.value = NetworkState.Error(exception as Exception)
+ networkStateLiveData.value = NetworkState.Error(
+ if (exception is Exception) exception else Exception(exception)
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected val handler = | |
| CoroutineExceptionHandler { _, exception -> | |
| networkStateLiveData.value = NetworkState.Error(exception as Exception) | |
| } | |
| protected val handler = | |
| CoroutineExceptionHandler { _, exception -> | |
| networkStateLiveData.value = NetworkState.Error( | |
| if (exception is Exception) exception else Exception(exception) | |
| ) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/connect/base/BaseNetworkStateViewModel.kt
around lines 26 to 29, the CoroutineExceptionHandler currently unsafely casts
the received Throwable to Exception which can throw ClassCastException; change
the handler to safely handle non-Exception Throwables by passing either the
Throwable when NetworkState.Error accepts Throwable or by converting: use
exception as? Exception ?: Exception(exception) so NetworkState.Error always
gets an Exception instance (or update NetworkState.Error to accept Throwable and
pass the throwable directly). Ensure no direct cast is used.
| * @param btnText Button text | ||
| * @param view The view to find a parent from. This view is used to create the SnackBar. | ||
| * @param message The message text to show in the SnackBar. | ||
| * @param okClickListener The callback to be invoked when the "OK" button is clicked. |
There was a problem hiding this comment.
Javadoc references hardcoded "OK" button.
The @param okClickListener description still says "OK" button, but the method now supports customizable button text via btnText.
- * @param okClickListener The callback to be invoked when the "OK" button is clicked.
+ * @param okClickListener The callback to be invoked when the action button is clicked.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @param okClickListener The callback to be invoked when the "OK" button is clicked. | |
| * @param okClickListener The callback to be invoked when the action button is clicked. |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/ViewUtils.java around line 17, the Javadoc for the
parameter okClickListener still references a hardcoded "OK" button even though
the method now accepts customizable btnText; update the @param description to
refer to the positive/confirmation button (or the button with customizable text)
rather than "OK", e.g. say "The callback invoked when the positive/confirmation
button is clicked (button text is provided via btnText)"; ensure the phrasing
matches the method's parameter names and behavior.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
@Jignesh-dimagi I see that there's been some discussion regarding the UI in Slack and I also noticed "[Draft]" in the title. Is this ready for a full code review?
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Generally looks pretty good, a couple things to address. Also, do we have additional tickets to implement this on the other relevant pages or should that be included here? I'm thinking specifically about learning progress.
app/res/values/strings.xml
Outdated
|
|
||
| <string name="network_forbidden_error">Access denied. Please contact customer support if the problem persists.</string> | ||
| <string name="new_network_data_success">New data is available; press here to refresh.</string> | ||
| <string name="new_network_data_failure">Something went wrong. You may be viewing outdated data.</string> |
There was a problem hiding this comment.
Per the discussion in #connect-tech-demos, I think we should use "Failed to refresh" instead of "Something went wrong". Also, in cases where we know the issue was a bad network connection it would be great if we could say "Unable to reach network" instead.
| ) : NetworkState() | ||
|
|
||
| data class Error( | ||
| val exception: Exception, |
There was a problem hiding this comment.
Per the coderabbit comment in this file: Given the CoroutineExceptionHandler provides a Throwable instead of Exception, any reason not to store it as a Throwable here?
| viewPagerAdapter.refresh(); | ||
| } | ||
| }); | ||
| ((ConnectDeliveryProgressFragmentViewModel)getBaseNetworkStateViewModel()).fetchData(requireContext(), job, this); |
There was a problem hiding this comment.
Nit: Might be nice to have a getViewModel helper to handle retrieving the base view model and casting it, since that code is a bit clunky
There was a problem hiding this comment.
@OrangeAndGreen We might have to handle the same way as ViewBinding but as many other fragments are using the BaseConnectFragment, so might not be worth having generic type parameter now so done casting directly.
Yes, PR description has reason for [Draft]. |
Eventually, we will create separate ticket for each screen and work on it. |
| <string name="new_network_data_success">New data is available; press here to refresh.</string> | ||
| <string name="new_network_data_failure">Failed to refresh. You may be viewing outdated data.</string> |
There was a problem hiding this comment.
I think these messages are good, but I'd love it if we could get some input from the product team (i.e. Kim, Mary, Ashish, or Brendon) if not already
| is BaseNetworkStateViewModel.NetworkState.Error -> { | ||
| showNetworkFailureSnackBar() | ||
| } | ||
| Loading -> { |
There was a problem hiding this comment.
Optional nit: can add is here for consistency with the other branches
| Loading -> { | |
| is Loading -> { |
| /** | ||
| * Displays a SnackBar with the given message and an "OK" button. | ||
| * The SnackBar will remain visible indefinitely until the "OK" button is pressed. | ||
| * Displays a SnackBar with the given message and an button with provided text. | ||
| * The SnackBar will remain visible indefinitely until the button is pressed. | ||
| * | ||
| * @param btnText Button text | ||
| * @param view The view to find a parent from. This view is used to create the SnackBar. | ||
| * @param message The message text to show in the SnackBar. | ||
| * @param okClickListener The callback to be invoked when the "OK" button is clicked. | ||
| */ | ||
| public static void showSnackBarWithOk(View view, String message, View.OnClickListener okClickListener) { | ||
| public static void showSnackBarWith(String btnText, View view, String message, View.OnClickListener okClickListener) { | ||
| Snackbar.make(view, message, Snackbar.LENGTH_INDEFINITE) | ||
| .setAction("OK", okClickListener) | ||
| .setAction(btnText, okClickListener) | ||
| .show(); | ||
| } |
There was a problem hiding this comment.
Can we refactor this function (and its comment) to not reference "ok" for the click listener (i.e. clickListener and not okClickListener) since the button text is not always "OK".
|
This is looking pretty good so far! |
Product Description
This PR has user facing UI changes.
Application fetches the data from network and show the status of the network response to user:
If network call has succeeded and last modified date of the data is latest than available in local DB, it will inform user that new data is available and user can click on it to refresh the data. After user click's on it, UI will get updated with latest data from server.
If network call has failed, it will inform user that user might be seeing the old data.
Another important point: Whenever user enters the screen, application will load the data from network silently i.e. without blocking progress bar. Whenever user presses the sync button either from top or from central sync available on screen, application will load the data with blocking progress bar to give user feedback that sync has started.
This changes are done for delivery progress screen. Eventually, it will get implemented in all other screens as mentioned here.
Whenever network call succeed and last modified date of data is latest than local DB:
Whenever network call fails:
Currently, server is not sending the last modified date for the delivery progress screen i.e. for the URL (https://connect.dimagi.com/api/opportunity/{id}/delivery_progress), the current PR doesn't have that logic working (its implemented but commented). So marking this PR as draft.
Also, regarding the translation i.e.
new_network_data_successandnew_network_data_failurePR has only for English. If the text is approved, I will add the translation for remaining languages.Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1700
https://dimagi.atlassian.net/browse/CCCT-1701
QA Plan
QA should try to test with and without network to have call success and failure which will give different snackbar to user.
Labels and Review