Pm messaging merge #2956
Pm messaging merge #2956
Conversation
|
@coderabbitai full review please |
|
@pm-dimagi: I'll perform a comprehensive review of all changes in this PR. ✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive messaging feature into the application. New drawable resources, XML layout files, and navigation graphs have been added to define various UI elements such as messaging screens, chat bubbles, consent dialogs, and channel lists. Several new Java classes are created to manage messaging logic, including activities, fragments, adapters, and models for both channels and messages. The changes encompass handling consent for messaging channels, encryption and decryption of messages, periodic API calls for message retrieval, and integration with Firebase Analytics to log navigation and notification events. The networking layer has been updated to use more flexible parameter types, supporting a broader range of data for messaging operations. Overall, these changes establish a detailed infrastructure for a robust messaging system. Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Nitpick comments (29)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
146-146: Fix typo in error logging message.There's a typo in the error logging message: "Netowrk Error" should be "Network Error".
- Logger.log("Netowrk Error", error); + Logger.log("Network Error", error);
136-161: Consider enhancing error handling with more specific error types.The current error handling could be improved by:
- Distinguishing between different types of network errors
- Providing more context in error messages
- Adding specific handling for common error scenarios (timeout, SSL, etc.)
+ private static class NetworkError extends Exception { + public final int responseCode; + public NetworkError(int responseCode, String message) { + super(message); + this.responseCode = responseCode; + } + } try { Response<ResponseBody> response = requester.makeRequest(); responseCode = response.code(); if (response.isSuccessful()) { stream = requester.getResponseStream(response); } else if (response.errorBody() != null) { String error = response.errorBody().string(); - Logger.log("Network Error", error); + Logger.log("Network Error", String.format("HTTP %d: %s", responseCode, error)); + throw new NetworkError(responseCode, error); } } catch (IOException e) { exception = e; - Logger.exception("Exception during POST", e); + Logger.exception(String.format("Network error during POST to %s", url), e); }app/res/layout/fragment_channel_list.xml (1)
9-15: RecyclerView Configuration and Constraint ConsiderationsThe
<RecyclerView>is set to match the parent's dimensions with padding clipping disabled, which is good for a list display. However, theandroid:layout_marginTop="0dp"attribute is redundant—removing it won’t change the layout. Also, when using aConstraintLayout, it is generally best practice to explicitly set constraints (e.g., usingapp:layout_constraintTop_toTopOf="parent", etc.) to ensure predictable behavior on different screen sizes. If the intention is simply to fill the entire container, the current configuration works, but adding constraints can improve maintainability and clarity.app/res/layout/item_chat_left_view.xml (1)
8-17: RelativeLayout Container Review
The RelativeLayout is properly constrained within the ConstraintLayout using the provided guideline and constraint attributes. While it works correctly, consider whether using a full ConstraintLayout for nested elements might improve performance and consistency across messaging layouts in the future.app/res/layout/item_channel.xml (3)
7-9: Remove Unnecessary Orientation Attribute
The rootConstraintLayoutincludes anandroid:orientation="vertical"attribute which is not used by ConstraintLayout. Removing this attribute will clean up the layout definition.
53-65: Add Top Constraint for tvChannelDescription
ThetvChannelDescriptionview currently only sets a bottom and horizontal constraints. To avoid potential ambiguity in vertical positioning, it is recommended to specify a top constraint (for example,app:layout_constraintTop_toBottomOf="@+id/tvChannelName") to clearly define its vertical placement.
79-93: Consider Unread Count Display for Larger Numbers
ThetvUnreadCountview is designed to display a small number (currently set to "15") with fixed padding and text size. If the unread count might exceed two digits, consider evaluating the design (for example, applying a formatting rule like "99+" or adjusting padding and text size) to ensure the UI remains visually balanced.app/res/layout/item_chat_right_view.xml (4)
8-20: RelativeLayout Container – Styling & ConstraintsThe
<RelativeLayout>is set up with proper constraint attributes and a width of0dpto leverage constraint sizing. However, it usesapp:cardCornerRadiusandapp:cardElevation, which are attributes typically associated with aCardView. If the desired effect is to have a card-like appearance with shadows and rounded corners, consider using aCardViewor ensuring that custom styling applies these attributes as intended.
28-35: Chat Message Container (LinearLayout)The
<LinearLayout>used as the container for the chat message is appropriately set with alayout_toStartOfattribute (a valid RelativeLayout property) to position itself relative to the edge indicator.
Suggestion: If variable-length chat messages are expected, consider adding amaxWidthattribute to prevent the layout from expanding excessively on larger screens.
37-44: Chat Message Text DisplayThe
<androidx.appcompat.widget.AppCompatTextView>displays a sample message. While the text "Some dummy text message from user A" is acceptable for a placeholder, it is advisable to use a string resource (e.g.,@string/sample_chat_message) for better localization and maintenance.
46-69: User Details and Read Status IndicatorThis section features a nested
<LinearLayout>containing an AppCompatTextView and an ImageView. The text in the AppCompatTextView is set to "12:00 AM", which appears to represent a timestamp rather than a username—even though the view’s id istvUserName. Consider renaming the id to something liketvTimestamp(or updating the text to reflect a username) for clarity and consistency.app/src/org/commcare/connect/database/ConnectMessageUtils.java (3)
13-26: Consider potential performance overhead when joining messages to channels.
You're iterating over all messages and channels to populate each channel’s message list. For large datasets, this approach can be expensive (O(N*M)). If performance becomes an issue, consider a more efficient query or an index-based lookup strategy.
27-50: Validate preview generation for multi-line or special characters.
The substring logic at lines 40-43 only handles truncation by length, which may break if the string contains special Unicode characters or if there are line breaks. Consider using a more robust method for preview generation to avoid splitting or truncating important text.
81-85: Remove or revise the outdated “payments/deliveries” comment.
The code comments reference “payments” and “deliveries,” which seem unrelated to the actual logic of storing messaging channels and messages. Cleaning these up helps avoid confusion and keeps the code more maintainable.- //Delete payments that are no longer available + //Remove stale channels that are no longer availableAlso applies to: 155-156
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
168-221: Remove or clarify the commented-out payment code block.
This extensive commented region adds clutter. If it’s no longer needed, remove it. If the code is pending future implementation, add a clearTODOcomment explaining its intended use.app/src/org/commcare/connect/MessageManager.java (2)
66-68: Remove or reduce “DEBUG_TESTING” logs in production code.
Excess logging can obscure relevant information in production environments. Either use a dedicated debug flag or remove these logs to keep the console free of development messages.Also applies to: 118-124, 156-163
232-235: Correct spelling in log statements for clarity and professionalism.
The current messages contain “Chcek,” which can diminish code clarity and raise questions during maintenance or debugging.- Log.d("DEBUG", "Chcek"); + Log.d("DEBUG", "Check");Also applies to: 240-241
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChatData.java (2)
5-13: Add class and field documentation.The class and its fields lack documentation explaining their purpose and valid values. Consider adding Javadoc to improve maintainability.
+/** + * Data class representing a chat message in the Connect messaging system. + */ public class ConnectMessageChatData { + /** Message type: 0 for incoming (left view), 1 for outgoing (right view) */ private int type; + /** Content of the message */ private String message; + /** Name of the user who sent the message */ private String userName; + /** Timestamp when the message was sent */ private Date timestamp; + /** Count of unread messages in the conversation */ private int countUnread; + /** Whether this message has been read */ private boolean isMessageRead;
7-7: Replace magic number with enum for message type.Using an integer for message type is error-prone. Consider using an enum to make the code more maintainable and type-safe.
+public enum MessageType { + INCOMING, // Left view + OUTGOING // Right view +} + public class ConnectMessageChatData { - private int type; + private MessageType type;app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelConsentBottomSheet.java (1)
21-21: Consider handling configuration changes.The fragment might be recreated during configuration changes (e.g., rotation). Consider saving and restoring the channel state.
public class ConnectMessageChannelConsentBottomSheet extends BottomSheetDialogFragment { + private static final String KEY_CHANNEL_ID = "channel_id"; + + @Override + public void onSaveInstanceState(@NonNull Bundle outState) { + super.onSaveInstanceState(outState); + ConnectMessageChannelConsentBottomSheetArgs args = ConnectMessageChannelConsentBottomSheetArgs + .fromBundle(getArguments()); + outState.putString(KEY_CHANNEL_ID, args.getChannelId()); + }app/src/org/commcare/adapters/ConnectMessageAdapter.java (1)
41-48: Extract common bind logic to reduce code duplication.The bind methods in both ViewHolders contain identical code. Consider extracting the common logic.
+private static void bindCommonMessageData(TextView messageView, TextView userNameView, + ConnectMessageChatData chat) { + SpannableStringBuilder builder = new SpannableStringBuilder(); + builder.append(chat.getMessage()); + MarkupUtil.setMarkdown(messageView, builder, new SpannableStringBuilder()); + userNameView.setText(DateUtils.formatDateTime(chat.getTimestamp(), + DateUtils.FORMAT_HUMAN_READABLE_SHORT)); +} public void bind(ConnectMessageChatData chat) { - SpannableStringBuilder builder = new SpannableStringBuilder(); - builder.append(chat.getMessage()); - MarkupUtil.setMarkdown(binding.tvChatMessage, builder, new SpannableStringBuilder()); - binding.tvUserName.setText(DateUtils.formatDateTime(chat.getTimestamp(), - DateUtils.FORMAT_HUMAN_READABLE_SHORT)); + bindCommonMessageData(binding.tvChatMessage, binding.tvUserName, chat); }Also applies to: 58-65
app/src/org/commcare/adapters/ChannelAdapter.java (1)
64-74: Consider optimizing the message iteration logic.The current implementation iterates through all messages to find the latest date and count unread messages. This could be inefficient for channels with many messages.
Consider maintaining these values at the channel level and updating them when messages are added/updated:
Date lastDate = null; int unread = 0; -for (ConnectMessagingMessageRecord message : channel.getMessages()) { - if (lastDate == null || lastDate.before(message.getTimeStamp())) { - lastDate = message.getTimeStamp(); - } - - if (!message.getUserViewed()) { - unread++; - } -} +lastDate = channel.getLastMessageDate(); // Add this method to ConnectMessagingChannelRecord +unread = channel.getUnreadCount(); // Add this method to ConnectMessagingChannelRecordapp/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java (1)
155-157: Return unmodifiable list from getMessages.The current implementation allows external modification of the internal messages list.
Protect the list:
public List<ConnectMessagingMessageRecord> getMessages() { - return messages; + return Collections.unmodifiableList(messages); }app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
21-22: Make NavController private and final.The NavController is currently public and non-final, which violates encapsulation.
Apply this change:
-public NavController controller; +private final NavController controller;app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (2)
39-41: Consider using a constant for the API refresh interval.The 30-second interval for API refresh is hardcoded. Consider extracting this to a named constant or configuration value for better maintainability.
- private static final int INTERVAL = 30000; + private static final int API_REFRESH_INTERVAL_MS = 30000;
98-102: Add loading state handling.The
makeApiCallmethod should show a loading state while fetching messages to improve user experience.private void makeApiCall() { + binding.progressBar.setVisibility(View.VISIBLE); MessageManager.retrieveMessages(requireActivity(), success -> { + binding.progressBar.setVisibility(View.GONE); refreshUi(); }); }app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
468-471: Add input validation for notification type.The
reportNotificationTypemethod should validate the notification type parameter.public static void reportNotificationType(String notificationType) { + if (notificationType == null || notificationType.trim().isEmpty()) { + Logger.log(LogTypes.TYPE_ERROR_WORKFLOW, "Invalid notification type"); + return; + } reportEvent(CCAnalyticsEvent.CCC_NOTIFICATION_TYPE, CCAnalyticsParam.NOTIFICATION_TYPE, notificationType); }app/res/layout/fragment_connect_message.xml (2)
9-21: RecyclerView Configuration – Minor Redundancy
The RecyclerView is set up correctly with proper constraints and margins. Note that the attributeandroid:orientation="vertical"is specified even though the layout manager (LinearLayoutManager) already controls the orientation. Consider removing it to avoid redundancy.
34-59: Nested Layout for Message Entry – Consider Cleanup
The inner ConstraintLayout housing the AppCompatEditText is well organized and applies the intended styling. There is a commented-out attribute on line 38 (android:drawableEnd="@drawable/ic_connect_message_photo_camera") that might no longer be needed. It is recommended to remove such commented code once confirmed unnecessary to keep the XML clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/res/drawable/ic_connect_message_send.pngis excluded by!**/*.png
📒 Files selected for processing (37)
app/res/drawable/ic_connect_message_editext_bg_24_border.xml(1 hunks)app/res/drawable/ic_connect_message_photo_camera.xml(1 hunks)app/res/drawable/ic_connect_message_read.xml(1 hunks)app/res/drawable/ic_connect_message_receiver_bg.xml(1 hunks)app/res/drawable/ic_connect_message_receiver_edge.xml(1 hunks)app/res/drawable/ic_connect_message_sender_bg.xml(1 hunks)app/res/drawable/ic_connect_message_sender_edge.xml(1 hunks)app/res/drawable/ic_contact_support_30.xml(1 hunks)app/res/drawable/shape_border_10px_blue.xml(1 hunks)app/res/layout/activity_connect_messaging.xml(1 hunks)app/res/layout/fragment_channel_consent_bottom_sheet.xml(1 hunks)app/res/layout/fragment_channel_list.xml(1 hunks)app/res/layout/fragment_connect_message.xml(1 hunks)app/res/layout/item_channel.xml(1 hunks)app/res/layout/item_chat_left_view.xml(1 hunks)app/res/layout/item_chat_right_view.xml(1 hunks)app/res/navigation/nav_graph_connect_messaging.xml(1 hunks)app/res/values/colors.xml(2 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java(1 hunks)app/src/org/commcare/adapters/ChannelAdapter.java(1 hunks)app/src/org/commcare/adapters/ConnectMessageAdapter.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java(1 hunks)app/src/org/commcare/connect/MessageManager.java(1 hunks)app/src/org/commcare/connect/database/ConnectMessageUtils.java(1 hunks)app/src/org/commcare/connect/network/ApiConnect.java(6 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(23 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(4 hunks)app/src/org/commcare/connect/network/connectId/ApiService.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelConsentBottomSheet.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChatData.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(1 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(2 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/connect/network/connectId/ApiService.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:8-63
Timestamp: 2025-01-28T09:38:59.882Z
Learning: The ConnectID API service methods should use Map<String, String> for request bodies and ResponseBody for responses, as per team preference.
🔇 Additional comments (36)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
204-208: LGTM! Gson handles Object serialization appropriately.The implementation correctly uses Gson for JSON serialization of the Object parameters when form encoding is not used.
92-94:❓ Verification inconclusive
Verify all callers handle the new parameter type safely.
The parameter type change from
HashMap<String, String>toHashMap<String, Object>provides more flexibility but requires careful handling of null values and type safety in calling code.Also applies to: 110-111, 164-165
🏁 Script executed:
#!/bin/bash # Search for all callers of these methods to verify they handle the new parameter type safely rg -A 3 "ConnectNetworkHelper\.(post|postSync|postInternal)\s*\("Length of output: 4762
Validate Caller Type Safety for Updated Parameter Types
The conversion from using
HashMap<String, String>toHashMap<String, Object>in theConnectNetworkHelpermethods (lines 92–94, 110–111, and 164–165) increases flexibility but also introduces potential runtime risks. Our search shows that the calls in bothApiConnect.javaandApiConnectId.javapass theparamsvariable as before. Please ensure that:
- All callers properly handle the possibility of receiving objects of varied types.
- There are appropriate null and type-safety checks in place where string values are expected.
- Defensive measures or additional documentation are added if necessary to safeguard against runtime type errors.
app/res/layout/fragment_channel_list.xml (1)
1-7: Layout Container Structure VerificationThe root
<ConstraintLayout>is well-configured with full-screen dimensions and a white background, and thetools:contextattribute correctly points to the associated fragment for design-time previews. Consider whether using a ConstraintLayout is ideal here if no specific constraints on child views are applied; a simpler container might suffice if advanced positioning isn’t needed.app/res/layout/item_chat_left_view.xml (4)
1-6: Root Layout Setup and Declaration Review
The XML declaration and the ConstraintLayout configuration are well-defined. The use ofandroid:layout_gravity="start"on the root view is acceptable if the parent container requires it, but double-check if it is necessary for this layout’s intended usage.
19-25: ImageView Usage and Accessibility Consideration
The ImageView is set up to display theic_connect_message_receiver_edgedrawable resource. Theandroid:contentDescription="@null"is appropriate if this image is purely decorative; otherwise, a descriptive text should be added to support accessibility.
26-54: Chat Message and Timestamp Layout Examination
The LinearLayout, which houses the customConnectRegularTextViewelements for the chat message and timestamp, is correctly implemented with proper padding, background, and orientation settings. The design clearly separates message text from metadata. It might be beneficial to confirm that the custom fonts (e.g.,@font/roboto_regular) are supported on all targeted API levels.
57-63: Guideline Configuration for Responsive Design
The Guideline is configured at 70% of the parent's width and is used effectively to position elements within the layout. This is a sound approach for maintaining a responsive and well-aligned UI design.app/res/layout/item_channel.xml (1)
97-106: Divider View Configuration is Appropriate
The dividerViewis correctly configured with appropriate margins, height, and constraints to visually separate list items. No changes are necessary here.app/src/org/commcare/connect/network/ApiConnect.java (1)
47-48:❓ Verification inconclusive
Verify type safety with the new HashMap<String, Object>.
The change from
HashMap<String, String>toHashMap<String, Object>allows for more flexible parameter types, but currently, all values are still being stored as Strings. This change seems to be in preparation for the messaging feature, but it introduces potential type safety concerns.Please ensure that:
- The server API endpoints can handle non-String values
- Type checking is implemented in
ConnectNetworkHelper.post()method- Proper serialization/deserialization is in place for non-String objects
Let's verify the implementation of
ConnectNetworkHelper.post():Also applies to: 86-88, 125-126
🏁 Script executed:
#!/bin/bash # Search for ConnectNetworkHelper.post implementation ast-grep --pattern 'class ConnectNetworkHelper { $$$ post($$$) { $$$ } $$$ }'Length of output: 91
The script above should help us locate the ConnectNetworkHelper implementation manually. Since the previous AST grep did not produce output, please verify that the ConnectNetworkHelper.post() method exists and correctly handles non-String types. Once confirmed, ensure that:
- Server API endpoints can process mixed-type values.
- The ConnectNetworkHelper.post() method includes proper type checking and serialization for non-String values.
- Integration tests cover these changes where applicable.
Verify HashMap Type Safety in API Calls
The change from
HashMap<String, String>toHashMap<String, Object>increases parameter flexibility—presumably to support messaging feature requirements—but may risk type safety if not properly handled.
- Locations to verify:
app/src/org/commcare/connect/network/ApiConnect.java(lines 47-48, and similarly in claimJob and setPaymentConfirmed methods)- Implementation of
ConnectNetworkHelper.post()for type checking and serializationPlease manually confirm that the API call and helper method address non-String object handling, ensuring consistent serialization/deserialization across endpoints.
🏁 Script executed:
#!/bin/bash # Re-run search for ConnectNetworkHelper and its post method using ripgrep rg "class ConnectNetworkHelper" -nLength of output: 138
Verify HashMap Parameter Handling in API Calls
The recent change from using
HashMap<String, String>toHashMap<String, Object>increases flexibility but may introduce type-safety risks if non-string values are passed unchecked. Currently, the implementation inApiConnect.java(e.g., instartLearnApp,claimJob, andsetPaymentConfirmed) still stores values as Strings. However, since the messaging feature appears to be driving this change, it’s important to ensure that:
- The
ConnectNetworkHelper.post()method (located inapp/src/org/commcare/connect/network/ConnectNetworkHelper.java) properly handles non-String objects, including appropriate type checking and serialization.- Server API endpoints are confirmed to accept the more flexible mixed-type payloads.
- Integration tests validate the handling of non-string types to avoid runtime issues.
Please manually verify the detailed implementation of
ConnectNetworkHelper.post()to ensure robust type management.app/res/layout/item_chat_right_view.xml (3)
1-6: XML Root Layout and DeclarationThe XML declaration and the root
<androidx.constraintlayout.widget.ConstraintLayout>are well-formed.
Note: The attributeandroid:layout_gravity="end"on the root might be redundant unless the parent container uses it for positioning. Please verify if this is intentional.
21-26: Edge Indicator ImageViewThe
<ImageView>withid="@+id/edge"is correctly positioned withandroid:layout_alignParentEnd="true"and references the drawable@drawable/ic_connect_message_sender_edge.
73-79: Guideline for Layout PositioningThe guideline is correctly defined to help with the positioning of the chat bubble within the ConstraintLayout. Its usage with
app:layout_constraintGuide_percent="0.3"is appropriate for achieving a dynamic layout.app/src/org/commcare/connect/MessageManager.java (1)
38-52: Handle the null or non-consented channel case more explicitly.
Currently,handleReceivedMessagesimply returnsnullif the channel is unknown or unconsented. Consider logging or surfacing a warning to help diagnose missing or unconsented channels that might indicate data sync issues.app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
14-14: LGTM!The new constant follows the established naming convention and is appropriately placed within the analytics parameters.
app/res/drawable/ic_connect_message_sender_edge.xml (1)
1-9: Vector Drawable: Confirm the Sender Edge Icon Rendering
This new vector drawable correctly specifies an 8dp icon with matching viewport dimensions and uses the color resource@color/connect_message_sender_bg. Please verify that the custom path data (line 7) yields the intended visual effect according to the design specifications.app/res/drawable/ic_connect_message_receiver_edge.xml (1)
1-9: Vector Drawable: Validate the Receiver Edge Icon
This vector drawable is structured similarly to its sender counterpart but uses a mirrored path with a negative coordinate (line 7). Ensure that this path data produces the desired mirrored effect for received messages and that the negative value is handled reliably by the Android vector renderer.app/res/drawable/ic_connect_message_sender_bg.xml (1)
1-9: Drawable Shape: Validate Design Intent for Sender Background
The drawable defines a rectangle with a solid background color (@color/connect_message_sender_bg) and rounds three of its four corners (bottomLeft, bottomRight, topLeft). Confirm that leaving the top-right corner unrounded is intentional to achieve the intended speech-bubble effect for sent messages.app/res/drawable/ic_connect_message_editext_bg_24_border.xml (1)
1-9: Drawable Shape: Confirm Corner Radius and Filename Nomenclature
This shape drawable applies a solid color (@color/connect_message_sender_bg) with a significantly large corner radius (50dp) and a subtle stroke (0.5dp) in@color/connect_divider_color. Please verify that the 50dp rounding meets the design requirements—especially for input fields or buttons. Also, double-check that the filename “editext” is intentional and not a typographical error for “edittext.”app/res/drawable/ic_connect_message_receiver_bg.xml (1)
1-9: Drawable Shape: Validate the Receiver Background Design
This new drawable sets a solid background color (@color/connect_message_receiver_bg) for received messages with rounded corners on the bottomLeft, bottomRight, and topRight edges. Confirm that the omission of a rounded top-left corner is deliberate to achieve the intended styling of the message bubble.app/res/drawable/ic_connect_message_read.xml (1)
1-10: Approved: Vector Drawable for Read Message Icon Looks Correct.
The vector drawable is well-defined with appropriate width, height, viewport, and styling. The path data and fill attributes are correctly provided.app/res/drawable/ic_contact_support_30.xml (1)
1-5: Approved: Contact Support Icon is Well-Constructed.
The vector drawable properly utilizes attributes such asandroid:autoMirrored, tinting, and specifies dimensions along with the path data. The structure is consistent and adheres to standard practices.app/res/layout/activity_connect_messaging.xml (1)
1-22: Approved: Messaging Activity Layout is Correctly Configured.
The layout usesConstraintLayouteffectively, and the embeddedFragmentContainerViewis fully constrained and linked to the correct navigation graph. This setup should efficiently host the messaging fragments.app/res/drawable/ic_connect_message_photo_camera.xml (1)
1-13: Approved: Photo Camera Icon Vector Drawable is Detailed and Consistent.
The drawable is well-formed, including a grouping for the clip-path and a detailed path with a dark fill color. Ensure consistency with other drawable resources regarding naming conventions and color usage across the messaging feature.app/res/navigation/nav_graph_connect_messaging.xml (1)
1-46: Approved: Navigation Graph for Messaging Feature is Structured Correctly.
The navigation graph clearly defines the start destination and correctly declares the dialog and fragments with their respective actions and arguments. The explicit non-nullable requirement forchannel_idin theconnectMessageFragmentensures clarity. One minor note: consider reviewing the static label "Channel Name" if dynamic labeling is intended later.app/res/layout/fragment_connect_message.xml (3)
1-7: Root Layout Definition Looks Solid
The root ConstraintLayout is well defined with all necessary namespaces and attributes. The use of full‑parent dimensions and a white background is appropriate for the fragment context.
23-32: Input Container Layout is Appropriate
The RelativeLayout serving as the container for the message input area is configured well with constraint attributes. Using a width of 0dp (match constraints in ConstraintLayout) is appropriate given the surrounding constraints.
62-71: Send Button Setup is Clear
The ImageView for the send message button is properly configured, with dimensions and constraints that align it with the input container. The initialvisibility="gone"setup makes sense if the send action is controlled dynamically.app/res/layout/fragment_channel_consent_bottom_sheet.xml (5)
1-4: Root Layout (Data Binding Container) is Correct
The use of the<layout>tag with the proper XML namespaces signals readiness for data binding. This is an excellent, modern approach to Android layout design.
5-9: FrameLayout Container is Well Configured
The FrameLayout sets the stage for the consent interface with proper dimensions and a transparent background tint.
17-37: Header Section with Icon and Title is Clear
The horizontal LinearLayout that displays the icon and the consent title text (via a custom ConnectMediumTextView) is neatly arranged with appropriate margins and text styling.
39-47: Channel Name Display is Adequate
The TextView for displaying the channel name uses a placeholder text ("(channel name)") as expected, and the styling (font, color, margins) is consistent with the design guidelines.
49-56: Consent Description is Well Implemented
The TextView providing the consent description is effectively designed with adequate margins, font settings, text color, and size for readability.app/res/values/colors.xml (1)
200-203: Connect Messaging Color Additions are Clear and Consistent
The three new color definitions introduced for the messaging feature—connect_message_receiver_bg,connect_message_sender_bg, andconnect_message_sender_text_color—are defined with appropriate hex codes and follow a consistent naming scheme that aids in maintainability and clarity.app/res/values/strings.xml (3)
835-844: Messaging Channel Consent Strings are Well Defined
The new string resources for the messaging UI (including titles, channel list labels, and consent texts) are concise and self-explanatory. They follow a consistent naming convention that will support localization efforts effectively.
845-848: Messaging Notification Strings Look Apt
The notification texts for new messages and channels are clearly worded and correctly employ placeholders (e.g.%s) for dynamic content. This should ensure a clear user experience when notifications are triggered.
849-853: Messaging API URL Strings are Secure and Maintainable
The new string resources for messaging URLs (retrieve, consent update, send, and confirmation) use secure HTTPS endpoints. Their naming is explicit, making future updates or environment changes straightforward.
| @@ -12,56 +12,56 @@ public interface ApiService { | |||
| Call<ResponseBody> checkPhoneNumber(@Query("phone_number") String phoneNumber); | |||
|
|
|||
| @POST(ApiEndPoints.registerUser) | |||
| Call<ResponseBody> registerUser(@Body Map<String, String> registrationRequest); | |||
| Call<ResponseBody> registerUser(@Body Map<String, Object> registrationRequest); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Consider implications of changing request body type from Map<String, String> to Map<String, Object>.
This change deviates from the team's established preference for using Map<String, String> in ConnectID API service methods. While this might be necessary for the messaging feature to support non-string data types, it requires careful consideration:
- Previous team feedback indicated a preference for
Map<String, String>(see PR ConnectID Foundation #2847). - The change affects all POST methods, not just messaging-related endpoints.
- The more flexible type might make it harder to catch type-related issues at compile time.
Please verify:
- Is this change necessary for all endpoints or only messaging-related ones?
- What non-string data types are being sent in the request bodies?
- Are there any backward compatibility concerns with existing code?
Consider these alternatives:
- Keep
Map<String, String>for non-messaging endpoints and create separate messaging-specific endpoints withMap<String, Object>. - Document the expected data types for each endpoint's request body fields.
- Create strongly-typed request objects instead of using maps to improve type safety.
Also applies to: 18-18, 21-21, 24-24, 27-27, 30-30, 33-33, 36-36, 39-39, 42-42, 45-45, 48-48, 51-51, 54-54, 57-57, 60-60, 63-63, 66-66
🏁 Script executed:
#!/bin/bash
# Search for existing usages of these API methods to assess impact
echo "Searching for API method usages..."
rg -A 2 "registerUser|changePhoneNo|updateProfile|validatePhone|requestOTPPrimary|validateSecondaryPhone|recoverConfirmOTPSecondary|confirmOTPSecondary|accountDeactivation|confirmDeactivation|recoverConfirmOTP|confirmOTP|recoverSecondary|confirmPIN|changePIN|resetPassword|changePassword|checkPassword" --type javaLength of output: 21540
Attention: Verify the API request body type and related changes for ConnectID endpoints
While the API methods in app/src/org/commcare/connect/network/connectId/ApiService.java now consistently use Map<String, Object>, please review the following:
- The change differs from the earlier team preference for
Map<String, String(see PR ConnectID Foundation #2847). Confirm whether all endpoints indeed require the flexibility for non-string values or if only the messaging-related calls need it. - Verify what non-string data types are being sent, and ensure that the new types are well documented for each endpoint.
- Double-check that these changes maintain backward compatibility across the codebase. Current call sites (e.g., in ApiConnectId.java) appear consistent, but please ensure no accidental type mismatches occur.
Consider:
- Splitting the API: retaining
Map<String, String>for non-messaging endpoints while creating dedicated messaging endpoints (or strongly-typed request objects) where non-string handling is needed. - Updating the documentation to clearly list expected types for each endpoint's request body.
| public static void storeMessagingMessages(Context context, List<ConnectMessagingMessageRecord> messages, boolean pruneMissing) { | ||
| SqlStorage<ConnectMessagingMessageRecord> storage = ConnectDatabaseHelper.getConnectStorage(context, ConnectMessagingMessageRecord.class); | ||
|
|
||
| List<ConnectMessagingMessageRecord> existingList = getMessagingMessagesAll(context); | ||
|
|
||
| //Delete payments that are no longer available | ||
| Vector<Integer> recordIdsToDelete = new Vector<>(); | ||
| for (ConnectMessagingMessageRecord existing : existingList) { | ||
| boolean stillExists = false; | ||
| for (ConnectMessagingMessageRecord incoming : messages) { | ||
| if (existing.getMessageId().equals(incoming.getMessageId())) { | ||
| incoming.setID(existing.getID()); | ||
| stillExists = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!stillExists && pruneMissing) { | ||
| //Mark the delivery for deletion | ||
| //Remember the ID so we can delete them all at once after the loop | ||
| recordIdsToDelete.add(existing.getID()); | ||
| } | ||
| } | ||
|
|
||
| if (pruneMissing) { | ||
| storage.removeAll(recordIdsToDelete); | ||
| } | ||
|
|
||
| //Now insert/update deliveries | ||
| for (ConnectMessagingMessageRecord incomingRecord : messages) { | ||
| storage.write(incomingRecord); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repeated O(N*M) lookups during bulk message storage.
Similar to storing channels, the nested loops in storeMessagingMessages can scale poorly. If performance becomes an issue as data grows, consider optimizing via indexing or database constraints to identify existing records more efficiently.
app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
Outdated
Show resolved
Hide resolved
| public static void retrieveMessages(Context context, String username, String password, IApiCallback callback) { | ||
| AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); | ||
|
|
||
| Multimap<String, String> params = ArrayListMultimap.create(); | ||
| ConnectNetworkHelper.get(context, | ||
| context.getString(R.string.ConnectMessageRetrieveMessagesURL), | ||
| API_VERSION_CONNECT_ID, authInfo, params, true, callback); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling for message retrieval.
The retrieveMessages method should include a timeout to prevent indefinite waiting.
public static void retrieveMessages(Context context, String username, String password, IApiCallback callback) {
AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false);
+ int timeoutMs = 30000; // 30 seconds timeout
Multimap<String, String> params = ArrayListMultimap.create();
ConnectNetworkHelper.get(context,
context.getString(R.string.ConnectMessageRetrieveMessagesURL),
- API_VERSION_CONNECT_ID, authInfo, params, true, callback);
+ API_VERSION_CONNECT_ID, authInfo, params, true, timeoutMs, callback);
}📝 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.
| public static void retrieveMessages(Context context, String username, String password, IApiCallback callback) { | |
| AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); | |
| Multimap<String, String> params = ArrayListMultimap.create(); | |
| ConnectNetworkHelper.get(context, | |
| context.getString(R.string.ConnectMessageRetrieveMessagesURL), | |
| API_VERSION_CONNECT_ID, authInfo, params, true, callback); | |
| } | |
| public static void retrieveMessages(Context context, String username, String password, IApiCallback callback) { | |
| AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); | |
| int timeoutMs = 30000; // 30 seconds timeout | |
| Multimap<String, String> params = ArrayListMultimap.create(); | |
| ConnectNetworkHelper.get(context, | |
| context.getString(R.string.ConnectMessageRetrieveMessagesURL), | |
| API_VERSION_CONNECT_ID, authInfo, params, true, timeoutMs, callback); | |
| } |
| public static void sendMessagingMessage(Context context, String username, String password, | ||
| ConnectMessagingMessageRecord message, String key, IApiCallback callback) { | ||
| AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); | ||
|
|
||
| String[] parts = ConnectMessagingMessageRecord.encrypt(message.getMessage(), key); | ||
|
|
||
| HashMap<String, Object> params = new HashMap<>(); | ||
| params.put("channel", message.getChannelId()); | ||
|
|
||
| HashMap<String, Object> content = new HashMap<>(); | ||
| try { | ||
| content.put("ciphertext", parts[0]); | ||
| content.put("nonce", parts[1]); | ||
| content.put("tag", parts[2]); | ||
| } catch (Exception e) { | ||
| Logger.exception("Sending message", e); | ||
| } | ||
| params.put("content", content); | ||
| params.put("timestamp", DateUtils.formatDateTime(message.getTimeStamp(), DateUtils.FORMAT_ISO8601)); | ||
| params.put("message_id", message.getMessageId()); | ||
|
|
||
| ConnectNetworkHelper.post(context, | ||
| context.getString(R.string.ConnectMessageSendURL), | ||
| API_VERSION_CONNECT_ID, authInfo, params, false, true, callback); | ||
| } |
There was a problem hiding this comment.
Enhance error handling in sendMessagingMessage.
The message sending implementation should handle encryption failures more robustly.
public static void sendMessagingMessage(Context context, String username, String password,
ConnectMessagingMessageRecord message, String key, IApiCallback callback) {
+ if (message == null || key == null) {
+ callback.processFailure(400, new IllegalArgumentException("Message or key is null"));
+ return;
+ }
AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false);
String[] parts = ConnectMessagingMessageRecord.encrypt(message.getMessage(), key);
+ if (parts == null) {
+ callback.processFailure(500, new Exception("Message encryption failed"));
+ return;
+ }📝 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.
| public static void sendMessagingMessage(Context context, String username, String password, | |
| ConnectMessagingMessageRecord message, String key, IApiCallback callback) { | |
| AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); | |
| String[] parts = ConnectMessagingMessageRecord.encrypt(message.getMessage(), key); | |
| HashMap<String, Object> params = new HashMap<>(); | |
| params.put("channel", message.getChannelId()); | |
| HashMap<String, Object> content = new HashMap<>(); | |
| try { | |
| content.put("ciphertext", parts[0]); | |
| content.put("nonce", parts[1]); | |
| content.put("tag", parts[2]); | |
| } catch (Exception e) { | |
| Logger.exception("Sending message", e); | |
| } | |
| params.put("content", content); | |
| params.put("timestamp", DateUtils.formatDateTime(message.getTimeStamp(), DateUtils.FORMAT_ISO8601)); | |
| params.put("message_id", message.getMessageId()); | |
| ConnectNetworkHelper.post(context, | |
| context.getString(R.string.ConnectMessageSendURL), | |
| API_VERSION_CONNECT_ID, authInfo, params, false, true, callback); | |
| } | |
| public static void sendMessagingMessage(Context context, String username, String password, | |
| ConnectMessagingMessageRecord message, String key, IApiCallback callback) { | |
| if (message == null || key == null) { | |
| callback.processFailure(400, new IllegalArgumentException("Message or key is null")); | |
| return; | |
| } | |
| AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); | |
| String[] parts = ConnectMessagingMessageRecord.encrypt(message.getMessage(), key); | |
| if (parts == null) { | |
| callback.processFailure(500, new Exception("Message encryption failed")); | |
| return; | |
| } | |
| HashMap<String, Object> params = new HashMap<>(); | |
| params.put("channel", message.getChannelId()); | |
| HashMap<String, Object> content = new HashMap<>(); | |
| try { | |
| content.put("ciphertext", parts[0]); | |
| content.put("nonce", parts[1]); | |
| content.put("tag", parts[2]); | |
| } catch (Exception e) { | |
| Logger.exception("Sending message", e); | |
| } | |
| params.put("content", content); | |
| params.put("timestamp", DateUtils.formatDateTime(message.getTimeStamp(), DateUtils.FORMAT_ISO8601)); | |
| params.put("message_id", message.getMessageId()); | |
| ConnectNetworkHelper.post(context, | |
| context.getString(R.string.ConnectMessageSendURL), | |
| API_VERSION_CONNECT_ID, authInfo, params, false, true, callback); | |
| } |
| <RelativeLayout | ||
| android:id="@+id/relativeLayout1" | ||
| android:layout_width="fill_parent" | ||
| android:layout_height="wrap_content"> | ||
|
|
||
| <com.google.android.material.button.MaterialButton | ||
| style="@style/NegativeButtonStyle" | ||
| android:id="@+id/decline_button" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_alignParentLeft="true" | ||
| android:layout_margin="16dp" | ||
| android:gravity="start|center" | ||
| android:text="@string/connect_messaging_channel_consent_decline" /> | ||
|
|
||
| <com.google.android.material.button.MaterialButton | ||
| android:id="@+id/accept_button" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_alignParentRight="true" | ||
| android:layout_margin="16dp" | ||
| android:gravity="start|center" | ||
| android:text="@string/connect_messaging_channel_consent_accept" | ||
| android:drawableRight="@drawable/connect_right_arrow" | ||
| /> | ||
| </RelativeLayout> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consent Action Buttons – Update Alignment Attributes
The RelativeLayout hosting the MaterialButtons for "decline" and "accept" functions is mostly well constructed. However, please note:
- Width Attribute: On line 60,
android:layout_width="fill_parent"is used. It is recommended to change this tomatch_parentsince fill_parent is deprecated. - Button Alignment Attributes:
- For the decline button (line 68), instead of
android:layout_alignParentLeft="true", useandroid:layout_alignParentStart="true"to support RTL layouts. - Similarly, for the accept button (line 77), replace
android:layout_alignParentRight="true"withandroid:layout_alignParentEnd="true".
- For the decline button (line 68), instead of
A possible diff for this segment:
- <RelativeLayout
- android:id="@+id/relativeLayout1"
- android:layout_width="fill_parent"
- android:layout_height="wrap_content">
+ <RelativeLayout
+ android:id="@+id/relativeLayout1"
+ android:layout_width="match_parent"
+ android:layout_height="wrap_content">- <com.google.android.material.button.MaterialButton
- style="@style/NegativeButtonStyle"
- android:id="@+id/decline_button"
- android:layout_width="wrap_content"
- android:layout_height="wrap_content"
- android:layout_alignParentLeft="true"
- android:layout_margin="16dp"
- android:gravity="start|center"
- android:text="@string/connect_messaging_channel_consent_decline" />
+ <com.google.android.material.button.MaterialButton
+ style="@style/NegativeButtonStyle"
+ android:id="@+id/decline_button"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_alignParentStart="true"
+ android:layout_margin="16dp"
+ android:gravity="start|center"
+ android:text="@string/connect_messaging_channel_consent_decline" />- <com.google.android.material.button.MaterialButton
- android:id="@+id/accept_button"
- android:layout_width="wrap_content"
- android:layout_height="wrap_content"
- android:layout_alignParentRight="true"
- android:layout_margin="16dp"
- android:gravity="start|center"
- android:text="@string/connect_messaging_channel_consent_accept"
- android:drawableRight="@drawable/connect_right_arrow"
- />
+ <com.google.android.material.button.MaterialButton
+ android:id="@+id/accept_button"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_alignParentEnd="true"
+ android:layout_margin="16dp"
+ android:gravity="start|center"
+ android:text="@string/connect_messaging_channel_consent_accept"
+ android:drawableRight="@drawable/connect_right_arrow"
+ />📝 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.
| <RelativeLayout | |
| android:id="@+id/relativeLayout1" | |
| android:layout_width="fill_parent" | |
| android:layout_height="wrap_content"> | |
| <com.google.android.material.button.MaterialButton | |
| style="@style/NegativeButtonStyle" | |
| android:id="@+id/decline_button" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_alignParentLeft="true" | |
| android:layout_margin="16dp" | |
| android:gravity="start|center" | |
| android:text="@string/connect_messaging_channel_consent_decline" /> | |
| <com.google.android.material.button.MaterialButton | |
| android:id="@+id/accept_button" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_alignParentRight="true" | |
| android:layout_margin="16dp" | |
| android:gravity="start|center" | |
| android:text="@string/connect_messaging_channel_consent_accept" | |
| android:drawableRight="@drawable/connect_right_arrow" | |
| /> | |
| </RelativeLayout> | |
| <RelativeLayout | |
| android:id="@+id/relativeLayout1" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content"> | |
| <com.google.android.material.button.MaterialButton | |
| style="@style/NegativeButtonStyle" | |
| android:id="@+id/decline_button" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_alignParentStart="true" | |
| android:layout_margin="16dp" | |
| android:gravity="start|center" | |
| android:text="@string/connect_messaging_channel_consent_decline" /> | |
| <com.google.android.material.button.MaterialButton | |
| android:id="@+id/accept_button" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_alignParentEnd="true" | |
| android:layout_margin="16dp" | |
| android:gravity="start|center" | |
| android:text="@string/connect_messaging_channel_consent_accept" | |
| android:drawableRight="@drawable/connect_right_arrow" | |
| /> | |
| </RelativeLayout> |
|
@coderabbitai I pushed a fix in commit c38aeb6, please review it. |
🧩 Analysis chain@pm-dimagi: I'll review the changes in commit c38aeb6. 🏁 Script executed: Length of output: 25322 @pm-dimagi: I've reviewed the changes in commit c38aeb6. The changes look good and include several important improvements:
Great work on making the code more robust and maintainable! 🎉 If you found this review helpful, would you consider giving us a shout-out on X? ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
app/src/org/commcare/connect/network/ApiConnectId.java (2)
476-483:⚠️ Potential issueAdd timeout handling for message retrieval.
The
retrieveMessagesmethod should include a timeout to prevent indefinite waiting.Apply this diff:
public static void retrieveMessages(Context context, String username, String password, IApiCallback callback) { AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); + int timeoutMs = 30000; // 30 seconds timeout Multimap<String, String> params = ArrayListMultimap.create(); ConnectNetworkHelper.get(context, context.getString(R.string.ConnectMessageRetrieveMessagesURL), - API_VERSION_CONNECT_ID, authInfo, params, true, callback); + API_VERSION_CONNECT_ID, authInfo, params, true, timeoutMs, callback); }
522-550:⚠️ Potential issueEnhance error handling in sendMessagingMessage.
The message sending implementation should handle encryption failures more robustly.
Apply this diff:
public static void sendMessagingMessage(Context context, String username, String password, ConnectMessagingMessageRecord message, String key, IApiCallback callback) { if (message == null || key == null) { - callback.processFailure(400, new IOException("Message or key is null")); + callback.processFailure(400, new IllegalArgumentException("Message or key is null")); return; } AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false); String[] parts = ConnectMessagingMessageRecord.encrypt(message.getMessage(), key); if (parts == null) { - callback.processFailure(500, new IOException("Message encryption failed")); + callback.processFailure(500, new Exception("Message encryption failed")); return; } + if (parts.length != 3) { + callback.processFailure(500, new Exception("Invalid encryption result")); + return; + }
🧹 Nitpick comments (14)
app/res/layout/item_channel.xml (2)
2-9: Remove Unnecessaryandroid:orientationAttribute
The rootConstraintLayoutis assigned anandroid:orientationattribute, which is not used by ConstraintLayout and can be removed to clean up the layout.
22-37: Add Content Description for Accessibility
TheImageViewwithin theCardViewlacks acontentDescriptionattribute. For better accessibility, either add a descriptive text or set it to@nullif it is purely decorative.app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (3)
68-79: Reconsider storingchannelIdin a static variable.Using
public static String activeChannelmight cause confusion or concurrency issues if multiple messaging fragments are active or if the static reference lingers. Consider a non-static member or a ViewModel-based approach for tracking the active channel.
98-102: Add minimal error handling forretrieveMessages.Currently, the callback ignores the
successflag and callsrefreshUi()regardless of the result. Consider logging or indicating an error to the user ifsuccessis false, to improve visibility of failed fetches.
173-196: Potential overhead in marking messages viewed.
refreshUi()re-saves each message if.getUserViewed() == false. Although functional, this might repeatedly write to the database in short intervals if incremental updates occur. It’s a minor concern, but consider optimizing to reduce frequent writes.app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelConsentBottomSheet.java (1)
31-37: Returning null fromonCreateViewcan be tricky.If
channelisn’t found, the fragment returnsnullrather than a valid view. This is functional because you immediately popBackStack(), but consider verifying that it doesn’t cause unexpected behavior in certain lifecycle scenarios.app/src/org/commcare/adapters/ConnectMessageAdapter.java (2)
41-47: Extract common code into a helper method.The
bindmethods in bothLeftViewHolderandRightViewHoldercontain duplicated code for formatting messages and timestamps.Apply this diff to improve code reusability:
+ private static void bindCommon(TextView messageView, TextView userNameView, ConnectMessageChatData chat) { + SpannableStringBuilder builder = new SpannableStringBuilder(); + builder.append(chat.getMessage()); + MarkupUtil.setMarkdown(messageView, builder, new SpannableStringBuilder()); + userNameView.setText(DateUtils.formatDateTime(chat.getTimestamp(), DateUtils.FORMAT_HUMAN_READABLE_SHORT)); + } public void bind(ConnectMessageChatData chat) { - SpannableStringBuilder builder = new SpannableStringBuilder(); - builder.append(chat.getMessage()); - MarkupUtil.setMarkdown(binding.tvChatMessage, builder, new SpannableStringBuilder()); - binding.tvUserName.setText(DateUtils.formatDateTime(chat.getTimestamp(), DateUtils.FORMAT_HUMAN_READABLE_SHORT)); + bindCommon(binding.tvChatMessage, binding.tvUserName, chat); }Also applies to: 58-64
41-47: Extract common code into a helper method.The
bindmethods in bothLeftViewHolderandRightViewHoldercontain identical code for formatting messages and timestamps. Consider extracting this into a helper method to avoid code duplication.Apply this diff to improve code reusability:
+private static void bindCommon(TextView messageView, TextView timestampView, ConnectMessageChatData chat) { + SpannableStringBuilder builder = new SpannableStringBuilder(); + builder.append(chat.getMessage()); + MarkupUtil.setMarkdown(messageView, builder, new SpannableStringBuilder()); + timestampView.setText(DateUtils.formatDateTime(chat.getTimestamp(), DateUtils.FORMAT_HUMAN_READABLE_SHORT)); +} public void bind(ConnectMessageChatData chat) { - SpannableStringBuilder builder = new SpannableStringBuilder(); - builder.append(chat.getMessage()); - MarkupUtil.setMarkdown(binding.tvChatMessage, builder, new SpannableStringBuilder()); - binding.tvUserName.setText(DateUtils.formatDateTime(chat.getTimestamp(), DateUtils.FORMAT_HUMAN_READABLE_SHORT)); + bindCommon(binding.tvChatMessage, binding.tvUserName, chat); }Also applies to: 58-64
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
473-488: Consider extracting string constants and adding null checks.The implementation is robust and handles null labels correctly. However, a few improvements could make it even better:
public class FirebaseAnalyticsUtil { + private static final String UNKNOWN_DESTINATION = "UnknownDestination"; + private static final String UNKNOWN_LABEL = "Unknown"; + public static NavController.OnDestinationChangedListener getDestinationChangeListener() { return (navController, navDestination, args) -> { - String currentFragmentClassName = "UnknownDestination"; + if (navController == null || navDestination == null) { + return; + } + String currentFragmentClassName = UNKNOWN_DESTINATION; NavDestination destination = navController.getCurrentDestination(); - if (destination instanceof FragmentNavigator.Destination) { + if (destination != null && destination instanceof FragmentNavigator.Destination) { currentFragmentClassName = ((FragmentNavigator.Destination)destination).getClassName(); } Bundle bundle = new Bundle(); CharSequence label = navDestination.getLabel(); bundle.putString(FirebaseAnalytics.Param.SCREEN_NAME, - label != null ? label.toString() : "Unknown"); + label != null ? label.toString() : UNKNOWN_LABEL); bundle.putString(FirebaseAnalytics.Param.SCREEN_CLASS, currentFragmentClassName); reportEvent(FirebaseAnalytics.Event.SCREEN_VIEW, bundle); }; }app/src/org/commcare/adapters/ChannelAdapter.java (2)
63-106: Extract date formatting logic to a separate method.The date formatting logic in the
bindmethod could be extracted to improve readability and maintainability.Apply this diff to extract the date formatting logic:
+private String formatLastDate(Date lastDate) { + if (DateUtils.dateDiff(new Date(), lastDate) == 0) { + return DateUtils.formatTime(lastDate, DateUtils.FORMAT_HUMAN_READABLE_SHORT); + } else { + SimpleDateFormat outputFormat = new SimpleDateFormat("dd MMM, yyyy", Locale.ENGLISH); + outputFormat.setTimeZone(TimeZone.getDefault()); + return outputFormat.format(lastDate); + } +} public void bind(ItemChannelBinding binding, ConnectMessagingChannelRecord channel, OnChannelClickListener clickListener) { // ... existing code ... if (showDate) { - String lastText; - if (DateUtils.dateDiff(new Date(), lastDate) == 0) { - lastText = DateUtils.formatTime(lastDate, DateUtils.FORMAT_HUMAN_READABLE_SHORT); - } else { - SimpleDateFormat outputFormat = new SimpleDateFormat("dd MMM, yyyy", Locale.ENGLISH); - outputFormat.setTimeZone(TimeZone.getDefault()); - lastText = outputFormat.format(lastDate); - } - binding.tvLastChatTime.setText(lastText); + binding.tvLastChatTime.setText(formatLastDate(lastDate)); } // ... remaining code ... }
63-106: Extract date and unread count calculations into separate methods.The
bindmethod is doing too many things. Consider extracting the date and unread count calculations into separate methods to improve readability and maintainability.Apply this diff to improve code organization:
+private Date getLastMessageDate(List<ConnectMessagingMessageRecord> messages) { + Date lastDate = null; + for (ConnectMessagingMessageRecord message : messages) { + if (lastDate == null || lastDate.before(message.getTimeStamp())) { + lastDate = message.getTimeStamp(); + } + } + return lastDate; +} +private int getUnreadCount(List<ConnectMessagingMessageRecord> messages) { + int unread = 0; + for (ConnectMessagingMessageRecord message : messages) { + if (!message.getUserViewed()) { + unread++; + } + } + return unread; +} +private String formatLastDate(Date lastDate) { + if (DateUtils.dateDiff(new Date(), lastDate) == 0) { + return DateUtils.formatTime(lastDate, DateUtils.FORMAT_HUMAN_READABLE_SHORT); + } else { + SimpleDateFormat outputFormat = new SimpleDateFormat("dd MMM, yyyy", Locale.ENGLISH); + outputFormat.setTimeZone(TimeZone.getDefault()); + return outputFormat.format(lastDate); + } +} public void bind(ItemChannelBinding binding, ConnectMessagingChannelRecord channel, OnChannelClickListener clickListener) { binding.tvChannelName.setText(channel.getChannelName()); - Date lastDate = null; - int unread = 0; - for (ConnectMessagingMessageRecord message : channel.getMessages()) { - if (lastDate == null || lastDate.before(message.getTimeStamp())) { - lastDate = message.getTimeStamp(); - } - if (!message.getUserViewed()) { - unread++; - } - } + Date lastDate = getLastMessageDate(channel.getMessages()); + int unread = getUnreadCount(channel.getMessages()); binding.tvChannelDescription.setText(channel.getPreview()); boolean showDate = lastDate != null; binding.tvLastChatTime.setVisibility(showDate ? View.VISIBLE : View.GONE); if (showDate) { - String lastText; - if (DateUtils.dateDiff(new Date(), lastDate) == 0) { - lastText = DateUtils.formatTime(lastDate, DateUtils.FORMAT_HUMAN_READABLE_SHORT); - } else { - SimpleDateFormat outputFormat = new SimpleDateFormat("dd MMM, yyyy", Locale.ENGLISH); - outputFormat.setTimeZone(TimeZone.getDefault()); - lastText = outputFormat.format(lastDate); - } - binding.tvLastChatTime.setText(lastText); + binding.tvLastChatTime.setText(formatLastDate(lastDate)); } boolean showUnread = unread > 0; binding.tvUnreadCount.setVisibility(showUnread ? View.VISIBLE : View.GONE); if (showUnread) { binding.tvUnreadCount.setText(String.valueOf(unread)); } binding.itemRootLayout.setOnClickListener(view -> { if (clickListener != null) { clickListener.onChannelClick(channel); } }); }app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
197-217: Remove or implement commented payment-related code.The commented code for payment-related functionality makes the codebase harder to maintain. Either implement it or remove it.
Would you like me to help create an issue to track the implementation of the payment-related functionality?
224-226: Use a constant for the CCC action prefix.The "ccc_" string is used as a magic string. Consider extracting it into a constant for better maintainability.
Apply this diff to improve code maintainability:
+private static final String CCC_ACTION_PREFIX = "ccc_"; private boolean hasCccAction(String action) { - return action != null && action.contains("ccc_"); + return action != null && action.contains(CCC_ACTION_PREFIX); }app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
73-103: Improve error handling with user-friendly messages.While the function logs errors when the channel ID or channel is null, it doesn't provide any feedback to the user. Consider showing user-friendly error messages using Toast or Snackbar.
Apply this diff to improve user experience:
if (channelId == null) { Logger.log(LogTypes.TYPE_FCM, "Channel id is null"); + Toast.makeText(this, R.string.error_invalid_channel, Toast.LENGTH_SHORT).show(); return; } ConnectMessagingChannelRecord channel = ConnectMessageUtils.getMessagingChannel(this, channelId); if (channel == null) { Logger.log(LogTypes.TYPE_FCM, "Channel is null"); + Toast.makeText(this, R.string.error_channel_not_found, Toast.LENGTH_SHORT).show(); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/res/drawable/rounded_blue_bg.xml(1 hunks)app/res/layout/fragment_channel_consent_bottom_sheet.xml(1 hunks)app/res/layout/item_channel.xml(1 hunks)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java(1 hunks)app/src/org/commcare/adapters/ChannelAdapter.java(1 hunks)app/src/org/commcare/adapters/ConnectMessageAdapter.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java(1 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(23 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(4 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelConsentBottomSheet.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChatData.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(2 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/res/drawable/rounded_blue_bg.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- app/res/layout/fragment_channel_consent_bottom_sheet.xml
- app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java
- app/src/org/commcare/connect/network/ConnectNetworkHelper.java
- app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java
- app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java
👮 Files not reviewed due to content moderation or server errors (4)
- app/src/org/commcare/adapters/ConnectMessageAdapter.java
- app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
- app/src/org/commcare/adapters/ChannelAdapter.java
- app/src/org/commcare/services/CommCareFirebaseMessagingService.java
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#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.
🔇 Additional comments (33)
app/res/layout/item_channel.xml (6)
11-20: Inner ConstraintLayout Setup Looks Good
The innerConstraintLayoutis properly constrained on all sides (start, end, top, and bottom) ensuring a stable layout structure.
40-51: tvChannelName Configuration is Appropriate
TheConnectMediumTextViewfor the channel name useswrap_contentfor its height and correctly constrains to the top of its parent. This effectively addresses the earlier concern noted in previous reviews.
53-65: tvChannelDescription Layout is Well-Defined
TheConnectRegularTextViewfor displaying the channel description is set to0dpwidth with clear start, end, and bottom constraints, ensuring it adjusts properly to the available space.
67-77: tvLastChatTime Layout is Configured Correctly
The text view showing the last chat time is properly constrained to the parent's end and top with appropriate margins, ensuring correct alignment.
79-93: tvUnreadCount Styling is Consistent
The unread count badge is styled with clear padding, text sizing, and a rounded background, ensuring the information is both visible and aesthetically consistent with the rest of the UI.
97-107: Divider View Provides Clear Separation
The dividerViewis well implemented with a fixed height and appropriate margins to visually separate the channel item from others in the list.app/src/org/commcare/connect/network/ApiConnectId.java (3)
7-8: LGTM! Required imports for messaging feature.The new imports support the messaging feature:
Multimapfor handling multiple values per key in API parametersConnectMessagingMessageRecordfor message handlingAlso applies to: 13-13
485-497: LGTM! Well-structured channel consent update.The method properly handles:
- Authentication with username/password
- Channel and consent parameters
- API version and content type flags
510-520: LGTM! Proper message confirmation implementation.The method correctly:
- Authenticates the request
- Handles message IDs as a list
- Uses appropriate API version and flags
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (5)
44-66: Good initialization and UI setup inonCreateView.The fragment inflates its layout, retrieves arguments, sets the channel name, initializes the adapter, and configures periodic refresh logic. This approach is clean and straightforward.
80-89: Proper resource cleanup inonPause.Setting
activeChannel = nulland removing periodic callbacks helps prevent resource leaks. This is a good practice.
91-96: Broadcast receiver logic is concise.Registering and handling updates via
LocalBroadcastManageris sensible. TheupdateReceiversimply callsrefreshUi(), which keeps the UI responsive.
104-162: Solid input validation before sending messages.Trimming message text and checking for emptiness prevents blank messages from being sent. The rest of the logic to store and send messages looks cohesive.
164-172: Chat adapter setup is straightforward.Creating a new adapter and triggering an initial refresh keeps the UI consistent once the fragment opens.
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChatData.java (2)
17-24: Add null checks for critical fields.As previously suggested, consider validating that
message,userName, andtimestampare not null at runtime. The@NonNullannotation alone might not prevent null inputs.
[duplicate_comment, refactor_suggestion_good_to_have]
27-70: Readily understandable getters and setters.All accessors and mutators are clear and maintainable. No issues found in these methods.
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelConsentBottomSheet.java (2)
39-58: Consent acceptance flow looks solid.Setting consent flags and navigating on success, while popping the back stack on failure, ensures consistent feedback.
60-72: Decline flow mirrors acceptance.Both success and failure cases are handled properly, providing uniform user feedback.
app/src/org/commcare/adapters/ConnectMessageAdapter.java (6)
28-31: Replace notifyDataSetChanged with DiffUtil.Using
notifyDataSetChanged()is inefficient as it redraws all items. Consider using DiffUtil for better performance.Apply this diff to improve performance:
+private final DiffUtil.ItemCallback<ConnectMessageChatData> DIFF_CALLBACK = + new DiffUtil.ItemCallback<ConnectMessageChatData>() { + @Override + public boolean areItemsTheSame(@NonNull ConnectMessageChatData oldItem, + @NonNull ConnectMessageChatData newItem) { + return oldItem.equals(newItem); + } + + @Override + public boolean areContentsTheSame(@NonNull ConnectMessageChatData oldItem, + @NonNull ConnectMessageChatData newItem) { + return oldItem.getMessage().equals(newItem.getMessage()) && + oldItem.getTimestamp().equals(newItem.getTimestamp()); + } +}; public void updateData(List<ConnectMessageChatData> messages) { + DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff( + new DiffUtil.Callback() { + // Implement the required methods + }); this.messages = messages; - notifyDataSetChanged(); + diffResult.dispatchUpdatesTo(this); }
81-92: LGTM!The function includes a proper null check and correctly binds data based on the view type.
28-31: Replace notifyDataSetChanged with DiffUtil.Using
notifyDataSetChanged()is inefficient as it redraws all items. Consider using DiffUtil for better performance.Apply this diff to improve performance:
+private final DiffUtil.ItemCallback<ConnectMessageChatData> DIFF_CALLBACK = + new DiffUtil.ItemCallback<ConnectMessageChatData>() { + @Override + public boolean areItemsTheSame(@NonNull ConnectMessageChatData oldItem, + @NonNull ConnectMessageChatData newItem) { + return oldItem.getMessage().equals(newItem.getMessage()); + } + + @Override + public boolean areContentsTheSame(@NonNull ConnectMessageChatData oldItem, + @NonNull ConnectMessageChatData newItem) { + return oldItem.getMessage().equals(newItem.getMessage()) && + oldItem.getTimestamp().equals(newItem.getTimestamp()); + } +}; public void updateData(List<ConnectMessageChatData> messages) { + DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff( + new DiffUtil.Callback() { + @Override + public int getOldListSize() { + return ConnectMessageAdapter.this.messages.size(); + } + + @Override + public int getNewListSize() { + return messages.size(); + } + + @Override + public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) { + return DIFF_CALLBACK.areItemsTheSame( + ConnectMessageAdapter.this.messages.get(oldItemPosition), + messages.get(newItemPosition)); + } + + @Override + public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) { + return DIFF_CALLBACK.areContentsTheSame( + ConnectMessageAdapter.this.messages.get(oldItemPosition), + messages.get(newItemPosition)); + } + }); this.messages = messages; - notifyDataSetChanged(); + diffResult.dispatchUpdatesTo(this); }
81-92: LGTM!The function correctly handles null checks and different view types.
28-31: Replace notifyDataSetChanged with DiffUtil.Using
notifyDataSetChanged()is inefficient as it redraws all items. Consider using DiffUtil for better performance.Apply this diff to improve performance:
+private final DiffUtil.ItemCallback<ConnectMessageChatData> DIFF_CALLBACK = + new DiffUtil.ItemCallback<ConnectMessageChatData>() { + @Override + public boolean areItemsTheSame(@NonNull ConnectMessageChatData oldItem, + @NonNull ConnectMessageChatData newItem) { + return oldItem.getMessage().equals(newItem.getMessage()); + } + + @Override + public boolean areContentsTheSame(@NonNull ConnectMessageChatData oldItem, + @NonNull ConnectMessageChatData newItem) { + return oldItem.getMessage().equals(newItem.getMessage()) && + oldItem.getTimestamp().equals(newItem.getTimestamp()); + } +}; public void updateData(List<ConnectMessageChatData> messages) { + DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff( + new DiffUtil.Callback() { + @Override + public int getOldListSize() { + return ConnectMessageAdapter.this.messages.size(); + } + + @Override + public int getNewListSize() { + return messages.size(); + } + + @Override + public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) { + return DIFF_CALLBACK.areItemsTheSame( + ConnectMessageAdapter.this.messages.get(oldItemPosition), + messages.get(newItemPosition)); + } + + @Override + public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) { + return DIFF_CALLBACK.areContentsTheSame( + ConnectMessageAdapter.this.messages.get(oldItemPosition), + messages.get(newItemPosition)); + } + }); this.messages = messages; - notifyDataSetChanged(); + diffResult.dispatchUpdatesTo(this); }
81-92: LGTM!The function has proper null checks and correctly binds data to the appropriate view holder based on the view type.
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (4)
28-45: LGTM!The function properly initializes the activity, sets up navigation, and handles redirects.
73-103: Improve error handling for channel retrieval.While the function includes null checks, it could provide better user feedback when channel retrieval fails.
Apply this diff to improve error handling:
if (channelId == null) { Logger.log(LogTypes.TYPE_FCM, "Channel id is null"); + Toast.makeText(this, R.string.error_invalid_channel, Toast.LENGTH_SHORT).show(); return; } ConnectMessagingChannelRecord channel = ConnectMessageUtils.getMessagingChannel(this, channelId); if (channel == null) { Logger.log(LogTypes.TYPE_FCM, "Channel is null"); + Toast.makeText(this, R.string.error_channel_not_found, Toast.LENGTH_SHORT).show(); return; }
73-103: LGTM!The function includes proper null checks, error logging, and handles channel consent status appropriately.
53-65: LGTM!The function properly cleans up resources by removing the destination change listener and follows good practices for null checks.
app/src/org/commcare/adapters/ChannelAdapter.java (2)
63-106: LGTM!The function properly handles date formatting with timezone support and manages view visibility based on data availability.
109-121: LGTM!The function includes proper timezone handling, error logging, and user-friendly fallback.
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
197-217: Remove or implement commented payment notification code.Large blocks of commented code make the codebase harder to maintain and understand.
Either implement the payment notification functionality or remove the commented code. If this is planned for future implementation, create a separate issue to track it.
Would you like me to help create an issue to track the implementation of the payment notification functionality?
66-84: LGTM!The function properly handles incoming messages with appropriate checks and action processing.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
468-471: LGTM! Clean implementation of notification type reporting.The method follows the established pattern for analytics reporting and integrates well with the existing codebase.
app/src/org/commcare/services/CommCareFirebaseMessagingService.java
Outdated
Show resolved
Hide resolved
|
@coderabbitai I pushed a fix in commit bc59cb2, please review it. |
🧩 Analysis chain@pm-dimagi: I apologize, but I'm unable to access the changes in commit bc59cb2. Could you please ensure that:
Once confirmed, please let me know and I'll review the changes. 🏁 Script executed: Length of output: 69680 @pm-dimagi: I see that commit bc59cb2 adds several configuration and project setup files, including:
However, while the commit message mentions "fixed notification handle", I don't see the actual code changes related to notification handling in this diff. Could you point me to the specific files where the notification handling was fixed? This will help me provide a more thorough review of the functional changes. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
40-44: Consider removing unused payment-related constants.The constants
PAYMENT_ID,PAYMENT_STATUS, andOPPORTUNITY_IDappear to be related to a payment notification feature that is currently commented out. Either implement the feature or remove these unused constants to maintain code cleanliness.
97-163: Extract string literals into constants.String literals like "title", "body", and "action" should be extracted into constants for better maintainability and to avoid typos.
+ private static final String EXTRA_TITLE = "title"; + private static final String EXTRA_BODY = "body"; + private static final String EXTRA_ACTION = "action"; private void showNotification(Map<String, String> payloadData) { - String notificationTitle = payloadData.get("title"); - String notificationText = payloadData.get("body"); - String action = payloadData.get("action"); + String notificationTitle = payloadData.get(EXTRA_TITLE); + String notificationText = payloadData.get(EXTRA_BODY); + String action = payloadData.get(EXTRA_ACTION);app/src/org/commcare/connect/network/ApiConnectId.java (3)
485-497: Document the return value of updateChannelConsent.The method returns a boolean but its meaning is not documented. Add documentation to clarify what the return value indicates.
Add this documentation:
+ /** + * Updates the consent status for a messaging channel + * @return true if the request was successfully queued, false otherwise + */ public static boolean updateChannelConsent(Context context, String username, String password, String channel, boolean consented, IApiCallback callback) {
513-523: Add validation for messageIds list.The method should validate the messageIds list before making the API call.
Add input validation:
public static void confirmReceivedMessages(Context context, String username, String password, List<String> messageIds, IApiCallback callback) { + if (messageIds == null || messageIds.isEmpty()) { + callback.processFailure(400, new IOException("Message IDs list is null or empty")); + return; + } AuthInfo authInfo = new AuthInfo.ProvidedAuth(username, password, false);
525-558: Add rate limiting for message sending.While the error handling is good, consider adding rate limiting to prevent message flooding.
Consider implementing a token bucket algorithm or similar rate limiting mechanism to prevent abuse of the messaging system. This could be done at the application level or coordinated with server-side rate limiting.
Example approach:
- Track message timestamps in a sliding window
- Implement backoff when rate limits are exceeded
- Consider using the Android WorkManager for managing message queues and retries
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/org/commcare/adapters/ChannelAdapter.java(1 hunks)app/src/org/commcare/adapters/ConnectMessageAdapter.java(1 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(23 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(3 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
- app/src/org/commcare/adapters/ConnectMessageAdapter.java
- app/src/org/commcare/adapters/ChannelAdapter.java
🔇 Additional comments (5)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
67-73: Avoid logging sensitive payload data.The payload data might contain sensitive information. Consider redacting or summarizing the log entries.
165-233: Well-structured helper methods!The helper methods are well-organized with clear responsibilities, proper error handling, and good naming conventions. The separation of concerns makes the code more maintainable and testable.
app/src/org/commcare/connect/network/ApiConnectId.java (3)
476-483: Add timeout handling for message retrieval.The message retrieval implementation should include a timeout to prevent indefinite waiting.
499-511: LGTM! Good error handling for token retrieval.The implementation properly handles the case where token retrieval fails.
59-59:❓ Verification inconclusive
Verify parameter type changes across the codebase.
The parameter type has been changed from
HashMap<String, String>toHashMap<String, Object>in multiple methods. While this provides more flexibility, it's a breaking change that requires careful verification.Run this script to find potential issues:
Also applies to: 91-91, 121-121, 137-137, 240-240, 252-252, 264-264, 280-280, 296-296, 313-313, 327-327, 344-344, 351-351, 360-360, 372-372, 382-382, 392-392, 403-403, 416-416, 424-424, 434-434
🏁 Script executed:
#!/bin/bash # Description: Search for HashMap usage to verify parameter type compatibility # Search for HashMap usage in Java/Kotlin files rg -t java -t kt "HashMap<String, (String|Object)>" -A 3Length of output: 85
Below is a new shell script to re-run the verification while avoiding the unsupported
ktfile type. Please run this script manually to confirm that the parameter type changes appear consistently across all Java files:#!/bin/bash # Description: Search for HashMap usage in Java files to verify parameter type changes rg -t java "HashMap<String, (String|Object)>" -A 3Once you’ve confirmed that the changes from
HashMap<String, String>toHashMap<String, Object>are applied consistently and that no type-safety issues arise, you can update the review response accordingly.
Attention: Verify Parameter Type Changes in API Methods
The parameter type for API methods has been updated from
HashMap<String, String>toHashMap<String, Object>to allow for increased flexibility. However, this is a breaking change that may affect type safety and downstream usage. Please verify the following:
- Method Consistency: Ensure that all affected methods (e.g.,
linkHqWorker,retrieveHqTokenApi,makeHeartbeatRequestSync, etc.) are updated uniformly.- Type Safety: Check that all usages now properly handle non-string values and include appropriate type checks.
- Downstream Impact: Confirm that any code consuming these parameters has been updated to accommodate the new, more flexible parameter type.
To assist with this review, re-run the attached script on your local environment and manually verify the output.
Product Description
Messaging feature which allows user to chat with the server
Feature Flag
Messaging
Safety Assurance
Safety story
-Done local testing of the connect side code
-Not tested the effect of this on commcare
Labels and Review