Beta 2.58 474459 #3252
Beta 2.58 474459 #3252
Conversation
Fixed signup blocker
connect qa branch with all latest phase2 pr merge work
Deleted extra files
Added safe navigation
Work as per the bugs mentioned in ticket
Making sure to setup the ConnectID user for Crashlytics on app startup.
Merging with. missed code of ola initial branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 43
🔭 Outside diff range comments (2)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
81-86: Use a single SharedPreferences.Editor instance for atomic updatesThe method creates two separate Editor instances which is inefficient and could lead to race conditions.
public static void updateFCMToken(String newToken) { SharedPreferences sharedPreferences = PreferenceManager .getDefaultSharedPreferences(CommCareApplication.instance()); - sharedPreferences.edit().putString(FCM_TOKEN, newToken).apply(); - sharedPreferences.edit().putLong(FCM_TOKEN_TIME, System.currentTimeMillis()).apply(); + sharedPreferences.edit() + .putString(FCM_TOKEN, newToken) + .putLong(FCM_TOKEN_TIME, System.currentTimeMillis()) + .apply(); }app/src/org/commcare/services/FCMMessageData.java (1)
114-127: Fix serialization mismatch between read and write methodsThe
writeExternalmethod writesaction.toString()butreadExternalexpects anActionTypesenum value. This will cause deserialization failures.@Override public void writeExternal(DataOutputStream out) throws IOException { - ExtUtil.writeString(out, action.toString()); + ExtUtil.writeString(out, actionType.toString()); ExtUtil.writeString(out, username); ExtUtil.writeString(out, domain); ExtUtil.writeNumeric(out, creationTime.getMillis()); }
🧹 Nitpick comments (33)
app/proguard.cfg (1)
238-240:-keep class kotlinx.serialization.** { *; }is very broadThis rule keeps every class + member under the entire
kotlinx.serializationhierarchy, noticeably inflating APK size and hindering obfuscation.
Consider the leaner alternative that keeps just runtime annotations and generated serializers:-keep @kotlinx.serialization.Serializable class ** -keepclassmembers class ** { @kotlinx.serialization.SerialName <fields>; }If you truly need full keep (e.g., reflective lookup of
KSerializerby name), feel free to ignore.app/res/layout/fragment_connect_results_summary_list.xml (1)
151-152: Ensure matching closing tagsMinor: keep a blank line before
</layout>to avoid accidental text after the root close.app/src/org/commcare/fragments/connectMessaging/ConnectMessageChatData.java (2)
5-13: Add documentation and consider field improvements.The class lacks documentation and could benefit from some improvements:
- Add class-level documentation explaining the purpose and usage
- Consider defining constants for message types instead of using raw integers
- Consider making fields final where they shouldn't change after construction
+/** + * Data class representing a chat message in the Connect messaging system. + * Contains message content, metadata, and read status information. + */ public class ConnectMessageChatData { + // Message type constants + public static final int TYPE_SENT = 1; + public static final int TYPE_RECEIVED = 2; - private int type; - private String message; - private String userName; - private Date timestamp; + private final int type; + private final String message; + private final String userName; + private final Date timestamp; private int countUnread; private boolean isMessageRead;
15-21: Consider initializing countUnread in constructor.The constructor doesn't initialize the
countUnreadfield, leaving it as the default value of 0. Consider whether this should be explicitly initialized or if a separate constructor parameter is needed.If
countUnreadshould have a specific initial value, consider:public ConnectMessageChatData(int type, String message, String userName, Date timestamp, boolean isMessageRead) { this.type = type; this.message = message; this.userName = userName; this.timestamp = timestamp; + this.countUnread = 0; // Explicitly initialize this.isMessageRead = isMessageRead; }Or add an overloaded constructor that accepts
countUnreadas a parameter.app/res/values-sw/strings.xml (1)
435-436: Menu title keys use PersonalId prefix – keep naming consistentThe new keys (
personalid_messaging_menu_title,personalid_sync_menu_title) live in the Connect feature but keep the PersonalId prefix. Considerconnect_for consistency with other Connect menu resources.app/res/values-pt/strings.xml (2)
316-316: Missing capitalisation / full-stop differs from EN master
connect_learn_error_startingstarts with “Caso não consiga…” (lower-case “Caso”). Other error strings in this file start with upper-case and end with a period. Align to house style.
426-427: Consistency with Swahili keysSame naming observation as in Swahili file: consider
connect_messaging_menu_title&connect_sync_menu_titlefor uniformity across locales.app/src/org/commcare/connect/ConnectDateUtils.kt (1)
25-27: Leverage java.time on API-26+ for thread-safety
SimpleDateFormatneeds explicit synchronisation. On API 26+ you can switch tojava.time.format.DateTimeFormatter, which is immutable and thread-safe. Not blocking, but consider for future cleanup.app/src/org/commcare/connect/ConnectNavHelper.kt (1)
20-25: Magic strings – promote to constants
"info"and"buttons"extras are duplicated elsewhere. Extract them toconst valinside the object to avoid typos and enable shared documentation.app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (2)
16-18: Consider making fields local to onReceive method.The fields
paymentId,opportunityId, andpaymentStatusare instance variables but are only used within theonReceivemethod and its helper methods. Consider making them local variables or method parameters to reduce instance state.- String paymentId = ""; - String opportunityId = ""; - boolean paymentStatus;Then pass them as parameters to helper methods:
- private void UpdatePayment(Context context) { + private void updatePayment(Context context, String opportunityId, String paymentId, boolean paymentStatus) {
37-45: Fix method naming convention.Method names should follow camelCase convention in Java.
- private void UpdatePayment(Context context) { + private void updatePayment(Context context) {app/src/org/commcare/adapters/ConnectProgressJobSummaryAdapter.java (1)
36-36: Consider using string resources for formatted text.The hardcoded "/" separator could be extracted to a string resource for better maintainability and localization support.
-holder.tvPrimaryVisitCount.setText(String.valueOf(primaryVisit.getPaymentUnitAmount()) + "/" + String.valueOf(primaryVisit.getPaymentUnitMaxDaily())); +holder.tvPrimaryVisitCount.setText(getString(R.string.progress_count_format, primaryVisit.getPaymentUnitAmount(), primaryVisit.getPaymentUnitMaxDaily()));Where
progress_count_formatwould be defined as%d/%din strings.xml.app/src/org/commcare/adapters/ChannelAdapter.java (2)
34-37: Consider using DiffUtil for better performance.The current implementation using
notifyDataSetChanged()is correct but could be optimized. For better performance with larger datasets, consider usingDiffUtilto calculate and animate only the changed items.
67-77: Consider caching computed values for better performance.The logic for finding the most recent message and counting unread messages is correct. However, iterating through all messages for each channel bind could be inefficient. Consider pre-computing and caching these values in the
ConnectMessagingChannelRecordor computing them at the database level.app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
144-148: Consider using DiffUtil for better RecyclerView performance.Using
notifyDataSetChanged()refreshes the entire list. For better performance, especially with large lists, consider implementing DiffUtil to calculate and dispatch only the actual changes.-public void updateDeliveries(List<ConnectJobDeliveryRecord> updatedList) { - deliveries.clear(); - deliveries.addAll(updatedList); - notifyDataSetChanged(); -} +public void updateDeliveries(List<ConnectJobDeliveryRecord> updatedList) { + DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff(new DiffUtil.Callback() { + @Override + public int getOldListSize() { + return deliveries.size(); + } + + @Override + public int getNewListSize() { + return updatedList.size(); + } + + @Override + public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) { + return deliveries.get(oldItemPosition).getId().equals( + updatedList.get(newItemPosition).getId()); + } + + @Override + public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) { + return deliveries.get(oldItemPosition).equals( + updatedList.get(newItemPosition)); + } + }); + + deliveries.clear(); + deliveries.addAll(updatedList); + diffResult.dispatchUpdatesTo(this); +}app/src/org/commcare/connect/ConnectAppUtils.kt (1)
55-57: Clean up downloadListener reference to prevent memory leaks.The downloadListener is cleared when downloading stops, but consider also clearing it in error scenarios.
override fun stopBlockingForTask(id: Int) { downloading = false + downloadListener = null } override fun taskCancelled() { + downloading = false + downloadListener = null }app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
86-94: Consider handling concurrent message retrieval calls.If the user triggers multiple navigation events quickly, multiple
retrieveMessagescalls could be in flight simultaneously. Consider adding a flag to prevent concurrent calls.+private boolean isRetrievingMessages = false; private void handleNoChannel(String channelId) { + if (isRetrievingMessages) { + return; + } + isRetrievingMessages = true; MessageManager.retrieveMessages(this, success -> { + isRetrievingMessages = false; ConnectMessagingChannelRecord connectMessagingChannelRecord = ConnectMessagingDatabaseHelper.getMessagingChannel(this, channelId); if (connectMessagingChannelRecord == null) { showFailureMessage(getString(R.string.connect_messaging_pn_wrong_channel)); } else { handleValidChannel(connectMessagingChannelRecord); } }); }app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
103-103: Add space after 'if' keyword for consistency.- if(success){ + if (success) {app/src/org/commcare/adapters/ConnectDeliveryProgressReportAdapter.java (3)
21-22: Make fields private for proper encapsulation.- List<ConnectDeliveryDetails> deliveryProgressList; - DeliveryItemOnClickListener deliveryItemOnClickListener; + private final List<ConnectDeliveryDetails> deliveryProgressList; + private final DeliveryItemOnClickListener deliveryItemOnClickListener;
38-47: Remove unnecessary @SuppressLint annotation and String.valueOf() calls.The
@SuppressLint("SetTextI18n")annotation appears unnecessary as there's no direct string concatenation. Also, severalString.valueOf()calls may be redundant if the values are already strings.- @SuppressLint("SetTextI18n") @Override public void onBindViewHolder(@NonNull ProgressBarViewHolder holder, int position) { ConnectDeliveryDetails connectDeliveryDetails = deliveryProgressList.get(position); - holder.binding.linearProgressBar.setProgress(Float.parseFloat( - String.valueOf(connectDeliveryDetails.getApprovedPercentage()))); + holder.binding.linearProgressBar.setProgress(connectDeliveryDetails.getApprovedPercentage()); holder.binding.linearProgressBar.setProgressColor(ContextCompat.getColor(context, R.color.connect_green)); - holder.binding.tvDeliveryTitle.setText(String.valueOf(connectDeliveryDetails.getDeliveryName())); - holder.binding.tvApproved.setText(String.valueOf(connectDeliveryDetails.getApprovedCount())); - holder.binding.tvDeliveryTotalAmount.setText(String.valueOf(connectDeliveryDetails.getTotalAmount())); + holder.binding.tvDeliveryTitle.setText(connectDeliveryDetails.getDeliveryName()); + holder.binding.tvApproved.setText(String.valueOf(connectDeliveryDetails.getApprovedCount())); + holder.binding.tvDeliveryTotalAmount.setText(String.valueOf(connectDeliveryDetails.getTotalAmount()));
87-89: Use a more descriptive parameter name in the interface.The parameter name
initNameis unclear. Consider usingdeliveryNameto better indicate its purpose.public interface DeliveryItemOnClickListener { - void onClick(String initName); + void onClick(String deliveryName); }app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java (1)
106-115: Consider consistent null checking approach.The method checks if
viewis null but doesn't check ifgetActivity()returns null on line 114. Based on the team's fail-fast philosophy, consider either removing the null check for consistency or adding a null check for the activity as well.app/src/org/commcare/activities/StandardHomeActivityUIController.java (2)
73-78: Cache view references to avoid repeated findViewById calls.The method performs multiple
findViewByIdcalls every time it's invoked. Consider caching these views as instance fields for better performance.Create fields for frequently accessed views:
private TextView tvJobTitle, tvViewMore, tvJobDescription, hoursTitle, tvJobTime, connectJobEndDate; // Initialize in setupUI() after finding viewJobCard private void initializeJobCardViews() { tvJobTitle = viewJobCard.findViewById(R.id.tv_job_title); tvViewMore = viewJobCard.findViewById(R.id.tv_view_more); // ... other views }
200-202: Reuse RecyclerView adapter instead of creating new instances.Creating a new adapter on every update is inefficient. Consider keeping a reference to the adapter and updating its data.
private ConnectProgressJobSummaryAdapter progressAdapter; // In updateConnectProgress() if (progressAdapter == null) { progressAdapter = new ConnectProgressJobSummaryAdapter(deliveryPaymentInfoList); recyclerView.setAdapter(progressAdapter); } else { progressAdapter.notifyDataSetChanged(); }app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (2)
97-100: Simplify ActionBar null checksThe nested null checks and instanceof check can be simplified for better readability.
- if(getActivity()!=null && getActivity() instanceof CommCareActivity && ((CommCareActivity)getActivity()).getSupportActionBar()!=null){ - ((CommCareActivity)getActivity()).getSupportActionBar().setDisplayHomeAsUpEnabled(true); - } + if (getActivity() instanceof CommCareActivity) { + ActionBar actionBar = ((CommCareActivity) getActivity()).getSupportActionBar(); + if (actionBar != null) { + actionBar.setDisplayHomeAsUpEnabled(true); + } + }
118-118: Improve code readability by breaking long lineThe navigation line is too long and difficult to read with multiple conditional operations.
- Navigation.findNavController(requireView()).navigate(channel.getConsented() ? getConnectMessageFragmentDirection(channel):getChannelConsetBottomSheetDirection(channel)); + NavDirections direction = channel.getConsented() + ? getConnectMessageFragmentDirection(channel) + : getChannelConsetBottomSheetDirection(channel); + Navigation.findNavController(requireView()).navigate(direction);app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
283-286: Extract common comparator for reusabilityThe same comparison logic is repeated multiple times. Consider extracting it to improve maintainability.
+ private static final Comparator<ConnectLoginJobListModel> LAST_ACCESSED_COMPARATOR = + Comparator.comparing(ConnectLoginJobListModel::getLastAccessed); + private void setJobListData(List<ConnectJobRecord> jobs) { // ... existing code ... - Collections.sort(learnApps, (job1, job2) -> job1.getLastAccessed().compareTo(job2.getLastAccessed())); - Collections.sort(deliverApps, (job1, job2) -> job1.getLastAccessed().compareTo(job2.getLastAccessed())); - Collections.sort(reviewLearnApps, (job1, job2) -> job1.getLastAccessed().compareTo(job2.getLastAccessed())); - Collections.sort(finishedItems, (job1, job2) -> job1.getLastAccessed().compareTo(job2.getLastAccessed())); + Collections.sort(learnApps, LAST_ACCESSED_COMPARATOR); + Collections.sort(deliverApps, LAST_ACCESSED_COMPARATOR); + Collections.sort(reviewLearnApps, LAST_ACCESSED_COMPARATOR); + Collections.sort(finishedItems, LAST_ACCESSED_COMPARATOR);app/src/org/commcare/activities/connect/ConnectActivity.java (1)
160-164: Consider debouncing message retrievalThe
prepareConnectMessagingScreenmethod is called in onCreate andretrieveMessagesis also called in onResume throughupdateMessagingIcon. This could result in duplicate API calls.Consider adding a flag to prevent duplicate calls or move the initial call to onResume only.
app/src/org/commcare/adapters/ConnectMessageAdapter.java (1)
41-47: Extract duplicate markdown formatting logicBoth ViewHolders have identical bind logic. Consider extracting to reduce duplication.
+ private static void bindMessage(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 static class LeftViewHolder extends RecyclerView.ViewHolder { // ... existing code ... 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)); + bindMessage(binding.tvChatMessage, binding.tvUserName, chat); } } public static class RightViewHolder extends RecyclerView.ViewHolder { // ... existing code ... 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)); + bindMessage(binding.tvChatMessage, binding.tvUserName, chat); } }Also applies to: 58-64
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
84-84: Use Kotlin idiomatic string checksReplace length checks with Kotlin's built-in extension functions for better readability.
- if (responseAsString.length > 0) { + if (responseAsString.isNotEmpty()) {Also applies to: 174-174
app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java (2)
93-93: Fix incorrect comments referring to paymentsThe comments incorrectly refer to "payments" instead of "channels" and "messages".
-//Delete payments that are no longer available +//Delete channels that are no longer available ... -//Delete payments that are no longer available +//Delete messages that are no longer availableAlso applies to: 167-167
180-181: Fix incorrect comments referring to deliveriesThe comments incorrectly refer to "delivery/deliveries" instead of "messages".
-//Mark the delivery for deletion +//Mark the message for deletion ... -//Now insert/update deliveries +//Now insert/update messagesAlso applies to: 190-190
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
104-108: Verify Java version compatibility for pattern variablesPattern variables require Java 14+, similar to the issue in ConnectDeliveryProgressFragment.
If using Java < 14:
-if (holder instanceof VerificationViewHolder vh) { - bindVerificationItem(vh, job.getDeliveries().get(position)); -} else if (holder instanceof PaymentViewHolder ph) { - bindPaymentItem(ph, job.getPayments().get(position), job); +if (holder instanceof VerificationViewHolder) { + bindVerificationItem((VerificationViewHolder) holder, job.getDeliveries().get(position)); +} else if (holder instanceof PaymentViewHolder) { + bindPaymentItem((PaymentViewHolder) holder, job.getPayments().get(position), job);
| public static final String CCC_MESSAGE = "ccc_message"; | ||
| public static final String CCC_PAYMENT_INFO_CONFIRMATION = "ccc_payment_info_confirmation"; | ||
| public static final String CCC_PAYMENT_INFO_CONFIRMATION_STATUS = "confirmation_status"; | ||
| public static final String OPPORTUNITY_ID = "opportunity_id"; | ||
| public static final String PAYMENT_ID = "payment_id"; | ||
| public static final String PAYMENT_STATUS = "payment_status"; | ||
| public static final String CONNECT_KEY_USERNAME = "username"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Harmonise new constant values to avoid key collisions
The added keys follow the uppercase-snake constant style, but their values ("confirmation_status", "payment_status", etc.) are very generic compared to the existing ccc_* names.
Because these strings are used as Intent/Bundle extras across multiple modules, generic values raise the risk of clashing with third-party extras or future app keys.
Consider prefixing them consistently, e.g.
- public static final String PAYMENT_STATUS = "payment_status";
+ public static final String PAYMENT_STATUS = "ccc_payment_status";Apply the same for confirmation_status, opportunity_id, and payment_id, then update the handful of send/receive sites in this PR.
Keeps the contract self-documenting and collision-free.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Showing ConnectConstants.java content ==="
sed -n '1,200p' app/src/org/commcare/connect/ConnectConstants.java || true
echo
echo "=== Searching for constant references ==="
for key in CCC_MESSAGE CCC_PAYMENT_INFO_CONFIRMATION CCC_PAYMENT_INFO_CONFIRMATION_STATUS OPPORTUNITY_ID PAYMENT_ID PAYMENT_STATUS CONNECT_KEY_USERNAME; do
echo
echo "---- Usages of $key ----"
rg -n "$key" . || echo " (none found)"
done
echo
echo "=== Searching for literal string usages ==="
for val in ccc_message ccc_payment_info_confirmation confirmation_status opportunity_id payment_id payment_status username; do
echo
echo "---- Literal \"$val\" ----"
rg -n "\"$val\"" . || echo " (none found)"
done
echo
echo "=== Searching for Intent extra operations ==="
echo
echo "---- putExtra calls ----"
rg -n "putExtra" . || echo " (none found)"
echo
echo "---- getStringExtra calls ----"
rg -n "getStringExtra" . || echo " (none found)"
echo
echo "---- getExtra calls ----"
rg -n "getExtra" . || echo " (none found)"Length of output: 63865
Harmonize new ConnectConstants keys to prevent Intent‐extra collisions
The generic extra keys introduced here (confirmation_status, opportunity_id, payment_id, payment_status) can easily clash with other modules or future extras. Prefix each with ccc_ and update the few literal send/receive sites:
• In app/src/org/commcare/connect/ConnectConstants.java, change:
– CCC_PAYMENT_INFO_CONFIRMATION_STATUS: "confirmation_status" → "ccc_confirmation_status"
– OPPORTUNITY_ID: "opportunity_id" → "ccc_opportunity_id"
– PAYMENT_ID: "payment_id" → "ccc_payment_id"
– PAYMENT_STATUS: "payment_status" → "ccc_payment_status"
• In app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java, replace:
– getArguments().getString("opportunity_id") → getArguments().getString(ConnectConstants.OPPORTUNITY_ID)
• In app/src/org/commcare/activities/connect/ConnectActivity.java, replace:
– getIntent().getStringExtra("opportunity_id") → getIntent().getStringExtra(ConnectConstants.OPPORTUNITY_ID)
– startArgs.putString("opportunity_id", opportunityId) → startArgs.putString(ConnectConstants.OPPORTUNITY_ID, opportunityId)
Diff for constants update:
--- a/app/src/org/commcare/connect/ConnectConstants.java
@@
- public static final String CCC_PAYMENT_INFO_CONFIRMATION_STATUS = "confirmation_status";
+ public static final String CCC_PAYMENT_INFO_CONFIRMATION_STATUS = "ccc_confirmation_status";
- public static final String OPPORTUNITY_ID = "opportunity_id";
+ public static final String OPPORTUNITY_ID = "ccc_opportunity_id";
- public static final String PAYMENT_ID = "payment_id";
+ public static final String PAYMENT_ID = "ccc_payment_id";
- public static final String PAYMENT_STATUS = "payment_status";
+ public static final String PAYMENT_STATUS = "ccc_payment_status";📝 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 final String CCC_MESSAGE = "ccc_message"; | |
| public static final String CCC_PAYMENT_INFO_CONFIRMATION = "ccc_payment_info_confirmation"; | |
| public static final String CCC_PAYMENT_INFO_CONFIRMATION_STATUS = "confirmation_status"; | |
| public static final String OPPORTUNITY_ID = "opportunity_id"; | |
| public static final String PAYMENT_ID = "payment_id"; | |
| public static final String PAYMENT_STATUS = "payment_status"; | |
| public static final String CONNECT_KEY_USERNAME = "username"; | |
| public static final String CCC_MESSAGE = "ccc_message"; | |
| public static final String CCC_PAYMENT_INFO_CONFIRMATION = "ccc_payment_info_confirmation"; | |
| - public static final String CCC_PAYMENT_INFO_CONFIRMATION_STATUS = "confirmation_status"; | |
| + public static final String CCC_PAYMENT_INFO_CONFIRMATION_STATUS = "ccc_confirmation_status"; | |
| - public static final String OPPORTUNITY_ID = "opportunity_id"; | |
| + public static final String OPPORTUNITY_ID = "ccc_opportunity_id"; | |
| - public static final String PAYMENT_ID = "payment_id"; | |
| + public static final String PAYMENT_ID = "ccc_payment_id"; | |
| - public static final String PAYMENT_STATUS = "payment_status"; | |
| + public static final String PAYMENT_STATUS = "ccc_payment_status"; | |
| public static final String CONNECT_KEY_USERNAME = "username"; |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/ConnectConstants.java lines 36 to 42, the string
values for CCC_PAYMENT_INFO_CONFIRMATION_STATUS, OPPORTUNITY_ID, PAYMENT_ID, and
PAYMENT_STATUS are too generic and risk collisions. Update their values by
prefixing each with "ccc_" to make them unique: change "confirmation_status" to
"ccc_confirmation_status", "opportunity_id" to "ccc_opportunity_id",
"payment_id" to "ccc_payment_id", and "payment_status" to "ccc_payment_status".
Then, in the related files
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java and
app/src/org/commcare/activities/connect/ConnectActivity.java, replace all
literal string keys for these extras with references to the updated constants
from ConnectConstants to ensure consistency and prevent collisions.
| <layout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| xmlns:app="http://schemas.android.com/apk/res-auto"> | ||
| <androidx.constraintlayout.widget.ConstraintLayout |
There was a problem hiding this comment.
<layout> wrapper added but missing <data> block
Wrapping the file in a Databinding <layout> tag is correct, but Databinding complains if neither <data> nor tools:dataBindingIgnore="true" is present.
<layout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto">
<data/> <!-- add an empty block or real bindings -->
…
</layout>Without this, the build will warn and can fail on “Expected <data> tag”.
🤖 Prompt for AI Agents
In app/res/layout/fragment_connect_delivery_list.xml at the beginning of the
file (lines 1 to 3), the <layout> tag is added but missing the required <data>
block for data binding. To fix this, add an empty <data/> block immediately
inside the <layout> tag before the root view, or include actual data binding
variables if needed. This will prevent build warnings or failures related to the
missing <data> tag.
| <layout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| xmlns:app="http://schemas.android.com/apk/res-auto"> | ||
| <ScrollView |
There was a problem hiding this comment.
Same databinding caveat as above
Add an explicit <data/> (or suppress) inside the new <layout> root to keep the Databinding compiler happy.
<layout …>
<data/>
<ScrollView …>🤖 Prompt for AI Agents
In app/res/layout/fragment_connect_results_summary_list.xml at lines 1 to 3, the
root <layout> tag is missing an explicit <data/> element, which is required by
the Databinding compiler. To fix this, add an empty <data/> tag immediately
inside the <layout> root element before the <ScrollView> tag to satisfy the
compiler's requirement.
| <layout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| xmlns:app="http://schemas.android.com/apk/res-auto" | ||
| xmlns:tools="http://schemas.android.com/tools" | ||
| xmlns:tools="http://schemas.android.com/tools"> | ||
| <androidx.constraintlayout.widget.ConstraintLayout |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Databinding wrapper lacks <data> section
Same issue as previous layouts—add <data/> or suppress warning.
🤖 Prompt for AI Agents
In app/res/layout/connect_payment_item.xml at the beginning of the file (lines 1
to 4), the databinding layout wrapper is missing the required <data> section. To
fix this, add an empty <data/> tag inside the <layout> root element or include
the necessary variable declarations if applicable. This will resolve the
databinding warning about the missing <data> section.
| } | ||
|
|
||
| public static ConnectJobAssessmentRecord fromJson(JSONObject json, int jobId) throws JSONException, ParseException { | ||
| public static ConnectJobAssessmentRecord fromJson(JSONObject json, int jobId) throws JSONException { |
There was a problem hiding this comment.
Potential issue: ParseException can still be thrown but not declared.
The method signature no longer declares ParseException but still calls DateUtils.parseDateTime() on line 61, which can throw ParseException. This creates an undeclared checked exception that could cause compilation or runtime issues.
Consider one of these solutions:
-public static ConnectJobAssessmentRecord fromJson(JSONObject json, int jobId) throws JSONException {
+public static ConnectJobAssessmentRecord fromJson(JSONObject json, int jobId) throws JSONException, ParseException {Or handle the exception internally:
record.jobId = jobId;
-record.date = DateUtils.parseDateTime(json.getString(META_DATE));
+try {
+ record.date = DateUtils.parseDateTime(json.getString(META_DATE));
+} catch (ParseException e) {
+ throw new JSONException("Invalid date format: " + e.getMessage());
+}🤖 Prompt for AI Agents
In
app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java
at line 55, the method fromJson calls DateUtils.parseDateTime which can throw a
ParseException, but this exception is not declared in the method signature. To
fix this, either add "throws ParseException" to the method signature or catch
the ParseException inside the method and handle it appropriately, such as
logging the error or converting it to a runtime exception.
| @Override | ||
| public void processFailure(int responseCode, @Nullable InputStream errorResponse, String url) { | ||
| Log.e("DEBUG_TESTING", "processFailure: " + responseCode); | ||
| //listener.connectActivityComplete(false); | ||
| getChannelEncryptionKey(context, channel, listener); | ||
| } |
There was a problem hiding this comment.
Clarify error handling logic for 400 responses
The code falls through to getChannelEncryptionKey on 400 errors with a commented callback. This behavior is unclear and potentially incorrect.
Either document why this fallthrough is intentional or fix the error handling:
@Override
public void processFailure(int responseCode, @Nullable InputStream errorResponse, String url) {
Log.e("DEBUG_TESTING", "processFailure: " + responseCode);
- //listener.connectActivityComplete(false);
- getChannelEncryptionKey(context, channel, listener);
+ if (responseCode == 400) {
+ // Document why we proceed to get encryption key on 400
+ getChannelEncryptionKey(context, channel, listener);
+ } else {
+ listener.connectActivityComplete(false);
+ }
}📝 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.
| @Override | |
| public void processFailure(int responseCode, @Nullable InputStream errorResponse, String url) { | |
| Log.e("DEBUG_TESTING", "processFailure: " + responseCode); | |
| //listener.connectActivityComplete(false); | |
| getChannelEncryptionKey(context, channel, listener); | |
| } | |
| @Override | |
| public void processFailure(int responseCode, @Nullable InputStream errorResponse, String url) { | |
| Log.e("DEBUG_TESTING", "processFailure: " + responseCode); | |
| if (responseCode == 400) { | |
| // Document why we proceed to get encryption key on 400 | |
| getChannelEncryptionKey(context, channel, listener); | |
| } else { | |
| listener.connectActivityComplete(false); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/MessageManager.java around lines 143 to 148, the
error handling for HTTP 400 responses is unclear because the callback to notify
failure is commented out and the code proceeds to call getChannelEncryptionKey.
Clarify this by either uncommenting and properly invoking the failure callback
to handle the error explicitly or add a comment explaining why continuing to
getChannelEncryptionKey on a 400 error is intentional and safe. Ensure the error
handling logic is consistent and clearly documented.
| } catch(Exception e) { | ||
| listener.connectActivityComplete(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching generic Exception
Catching generic Exception can mask unexpected errors. Consider catching specific exceptions like JSONException and IOException.
- } catch(Exception e) {
+ } catch(JSONException | IOException e) {
+ Logger.exception("Failed to process messages response", e);
listener.connectActivityComplete(false);
}📝 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.
| } catch(Exception e) { | |
| listener.connectActivityComplete(false); | |
| } | |
| } catch(JSONException | IOException e) { | |
| Logger.exception("Failed to process messages response", e); | |
| listener.connectActivityComplete(false); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/MessageManager.java around lines 85 to 87, the
code catches a generic Exception which can hide unexpected errors. Replace the
generic catch block with specific catches for exceptions such as JSONException
and IOException to handle known error cases explicitly and avoid masking other
issues.
| @Override | ||
| public String toString() { | ||
| return "AddListItem{" + | ||
| "name='" + name + '\'' + | ||
| ", id=" + id + | ||
| ", date=" + date + | ||
| ", description='" + description + '\'' + | ||
| ", organization='" + organization + '\'' + | ||
| '}'; | ||
| } |
There was a problem hiding this comment.
Fix incorrect class name in toString
The toString method references "AddListItem" instead of the actual class name.
@Override
public String toString() {
- return "AddListItem{" +
+ return "ConnectLoginJobListModel{" +
"name='" + name + '\'' +
", id=" + id +
", date=" + date +
", description='" + description + '\'' +
", organization='" + organization + '\'' +
'}';
}📝 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.
| @Override | |
| public String toString() { | |
| return "AddListItem{" + | |
| "name='" + name + '\'' + | |
| ", id=" + id + | |
| ", date=" + date + | |
| ", description='" + description + '\'' + | |
| ", organization='" + organization + '\'' + | |
| '}'; | |
| } | |
| @Override | |
| public String toString() { | |
| return "ConnectLoginJobListModel{" + | |
| "name='" + name + '\'' + | |
| ", id=" + id + | |
| ", date=" + date + | |
| ", description='" + description + '\'' + | |
| ", organization='" + organization + '\'' + | |
| '}'; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/models/connect/ConnectLoginJobListModel.java around
lines 207 to 216, the toString method incorrectly uses "AddListItem" as the
class name. Update the string to use the correct class name that matches this
class to accurately represent the object in the toString output.
| private static void handleApiError(Response<?> response) { | ||
| String message = response.message(); | ||
| if (response.code() == 400) { | ||
| // Bad request (e.g., validation failed) | ||
| Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Bad Request: " + message); | ||
| } else if (response.code() == 401) { | ||
| // Unauthorized (e.g., invalid credentials) | ||
| Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Unauthorized: " + message); | ||
| } else if (response.code() == 404) { | ||
| // Not found | ||
| Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Not Found: " + message); | ||
| } else if (response.code() >= 500) { | ||
| // Server error | ||
| Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Server Error: " + message); | ||
| } else { | ||
| Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "API Error: " + message); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove duplicate error handling method
The handleApiError method is a duplicate of logNetworkError. This violates the DRY principle.
Remove the duplicate method and use the existing logNetworkError instead:
-private static void handleApiError(Response<?> response) {
- String message = response.message();
- if (response.code() == 400) {
- // Bad request (e.g., validation failed)
- Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Bad Request: " + message);
- } else if (response.code() == 401) {
- // Unauthorized (e.g., invalid credentials)
- Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Unauthorized: " + message);
- } else if (response.code() == 404) {
- // Not found
- Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Not Found: " + message);
- } else if (response.code() >= 500) {
- // Server error
- Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Server Error: " + message);
- } else {
- Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "API Error: " + message);
- }
-}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/ApiPersonalId.java around lines 411 to
428, the handleApiError method duplicates the functionality of the existing
logNetworkError method, violating the DRY principle. Remove the handleApiError
method entirely and replace all its usages with calls to logNetworkError to
consolidate error handling logic.
| HashMap<String, String> 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); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching generic Exception in encryption logic
Catching generic Exception can mask specific encryption errors that should be handled differently.
HashMap<String, String> 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);
-}
+if (parts != null && parts.length >= 3) {
+ content.put("ciphertext", parts[0]);
+ content.put("nonce", parts[1]);
+ content.put("tag", parts[2]);
+} else {
+ Logger.exception("Invalid encryption result", new IllegalStateException("Encryption returned invalid parts"));
+ callback.processFailure(-1, null, context.getString(R.string.ConnectMessageSendURL));
+ 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.
| HashMap<String, String> 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); | |
| HashMap<String, String> content = new HashMap<>(); | |
| if (parts != null && parts.length >= 3) { | |
| content.put("ciphertext", parts[0]); | |
| content.put("nonce", parts[1]); | |
| content.put("tag", parts[2]); | |
| } else { | |
| Logger.exception("Invalid encryption result", new IllegalStateException("Encryption returned invalid parts")); | |
| callback.processFailure(-1, null, context.getString(R.string.ConnectMessageSendURL)); | |
| return; | |
| } | |
| params.put("content", content); |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/ApiPersonalId.java around lines 492 to
500, the code catches a generic Exception when putting encryption parts into the
content map. Replace the generic Exception catch with more specific exceptions
related to the encryption or array access, such as
ArrayIndexOutOfBoundsException or NullPointerException, to avoid masking other
errors and handle encryption-specific issues properly.
| private static OnCompleteListener handleFCMTokenRetrieval() { | ||
| return (OnCompleteListener<String>)task -> { | ||
| if (task.isSuccessful()) { | ||
| updateFCMToken(task.getResult()); | ||
| } else { | ||
| Logger.exception("Fetching FCM registration token failed", task.getException()); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper generics to avoid unchecked cast
The explicit cast to OnCompleteListener<String> is unnecessary and generates compiler warnings.
-private static OnCompleteListener handleFCMTokenRetrieval() {
- return (OnCompleteListener<String>)task -> {
+private static OnCompleteListener<String> handleFCMTokenRetrieval() {
+ return task -> {
if (task.isSuccessful()) {
updateFCMToken(task.getResult());
} else {
Logger.exception("Fetching FCM registration token failed", task.getException());
}
};
}📝 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.
| private static OnCompleteListener handleFCMTokenRetrieval() { | |
| return (OnCompleteListener<String>)task -> { | |
| if (task.isSuccessful()) { | |
| updateFCMToken(task.getResult()); | |
| } else { | |
| Logger.exception("Fetching FCM registration token failed", task.getException()); | |
| } | |
| }; | |
| } | |
| private static OnCompleteListener<String> handleFCMTokenRetrieval() { | |
| return task -> { | |
| if (task.isSuccessful()) { | |
| updateFCMToken(task.getResult()); | |
| } else { | |
| Logger.exception("Fetching FCM registration token failed", task.getException()); | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 96 to 104,
remove the explicit cast to OnCompleteListener<String> in the return statement
of handleFCMTokenRetrieval. Instead, declare the method return type and lambda
expression with proper generics to avoid unchecked cast warnings from the
compiler.
| if (!ConnectMessageChannelListFragment.isActive && | ||
| !channelId.equals(ConnectMessageFragment.activeChannel)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding static field access for UI state
Accessing static fields like ConnectMessageChannelListFragment.isActive and ConnectMessageFragment.activeChannel for UI state can lead to memory leaks and state inconsistencies.
Consider using a more robust state management approach such as:
- Application-level state manager
- ViewModel with LiveData/StateFlow
- Event bus with lifecycle awareness
This would prevent potential memory leaks and make the state management more testable and maintainable.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 362 to
363, avoid directly accessing static fields
ConnectMessageChannelListFragment.isActive and
ConnectMessageFragment.activeChannel for UI state as it risks memory leaks and
inconsistent state. Refactor the code to use a proper state management approach
such as an application-level state manager, or implement ViewModel with LiveData
or StateFlow to hold and observe UI state changes. Alternatively, use an event
bus with lifecycle awareness to communicate state changes safely. This will
improve maintainability and prevent memory issues.
| public static String NOTIFICATION_TITLE = "title"; | ||
| public static String NOTIFICATION_BODY = "body"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make static fields final constants
Static fields used as constants should be declared as final to prevent accidental modification.
-public static String NOTIFICATION_TITLE = "title";
-public static String NOTIFICATION_BODY = "body";
+public static final String NOTIFICATION_TITLE = "title";
+public static final String NOTIFICATION_BODY = "body";📝 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 String NOTIFICATION_TITLE = "title"; | |
| public static String NOTIFICATION_BODY = "body"; | |
| public static final String NOTIFICATION_TITLE = "title"; | |
| public static final String NOTIFICATION_BODY = "body"; |
🤖 Prompt for AI Agents
In app/src/org/commcare/services/FCMMessageData.java around lines 41 to 43, the
static fields NOTIFICATION_TITLE and NOTIFICATION_BODY are used as constants but
are not declared final. To fix this, add the final keyword to both fields to
make them immutable constants and prevent accidental modification.
Adds build number to analytics
Pm beta bug fixes
…livery progress screen
Corrected the app crash after downloading the learning/delivery app
Pm qa 7928
Solved progress display error while navigating from payment tab in delivery progress screen
Play store service check
Pm qa 7928 fix
|
@damagatchi retest this please |
|
@damagatchi retest this please |
…nto Beta_2.58_474459 # Conflicts: # app/AndroidManifest.xml
…-android into Beta_2.58_474459
|
closing this pr as its obselete with the changes merged master into it created new branch for beta release #3269 |
Product Description
Beta Release branch for 2.58
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review