Connect Phase 4 Merge Work #3248
Conversation
… in updateJobTileDetails).
…after each update. Renamed some fields in ConnectProgressJobSummaryAdapter to better describe functionality.
Added several new ConnectConstant strings and cleaned up obsolete ones. Using string constants to pass extras between intents.
📝 WalkthroughWalkthroughThis pull request introduces the core CommCare Connect feature set, adding a comprehensive suite of new activities, fragments, adapters, helpers, and utilities for managing Connect jobs, messaging, delivery progress, and payment acknowledgments. It adds new activities for Connect navigation and messaging, multiple fragments for job lists, job details, learning and delivery progress, messaging channels, and unlock flows. New adapters are introduced for job lists, messaging, delivery progress, and payment summaries. New database helpers and models support messaging, jobs, and user data. The PR also integrates new notification channels, updates the manifest, adds new string resources in multiple languages, and modifies existing classes to delegate Connect-related logic to new helpers. Notification handling, job progress updates, and payment acknowledgment flows are refactored and extended. Minor layout and resource updates are included for UI consistency. Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectActivity
participant ConnectUnlockFragment
participant ConnectJobHelper
participant API
participant JobStoreManager
participant ConnectJobsListsFragment
participant JobListAdapter
User->>ConnectActivity: Launch ConnectActivity
ConnectActivity->>ConnectUnlockFragment: Show unlock screen (if needed)
ConnectUnlockFragment->>ConnectJobHelper: Unlock Connect, fetch opportunities
ConnectJobHelper->>API: Fetch job opportunities
API-->>ConnectJobHelper: Return job list JSON
ConnectJobHelper->>JobStoreManager: Store jobs locally
ConnectUnlockFragment->>ConnectActivity: Redirect to job list
ConnectActivity->>ConnectJobsListsFragment: Show job lists
ConnectJobsListsFragment->>JobListAdapter: Display jobs to user
User->>ConnectJobsListsFragment: Select job
ConnectJobsListsFragment->>ConnectActivity: Navigate to job details or learning/delivery
sequenceDiagram
participant User
participant ConnectMessagingActivity
participant MessageManager
participant API
participant ConnectMessagingDatabaseHelper
participant MessagingUI
User->>ConnectMessagingActivity: Launch Messaging
ConnectMessagingActivity->>MessageManager: Retrieve messages
MessageManager->>API: Fetch channels and messages
API-->>MessageManager: Return channels/messages JSON
MessageManager->>ConnectMessagingDatabaseHelper: Store channels/messages
ConnectMessagingActivity->>MessagingUI: Display channels/messages
User->>MessagingUI: Send message
MessagingUI->>MessageManager: Send message
MessageManager->>API: Send encrypted message
API-->>MessageManager: Confirm message sent
MessageManager->>ConnectMessagingDatabaseHelper: Update message status
MessagingUI->>User: Update UI with sent message
sequenceDiagram
participant FCM
participant CommCareFirebaseMessagingService
participant FirebaseMessagingUtil
participant NotificationManager
participant PaymentAcknowledgeReceiver
participant ConnectJobHelper
FCM->>CommCareFirebaseMessagingService: Receive message
CommCareFirebaseMessagingService->>FirebaseMessagingUtil: handleNotification()
FirebaseMessagingUtil->>NotificationManager: Show notification (with action)
User->>NotificationManager: Tap action (e.g., payment acknowledge)
NotificationManager->>PaymentAcknowledgeReceiver: Broadcast intent
PaymentAcknowledgeReceiver->>ConnectJobHelper: Update payment status
ConnectJobHelper->>API: Confirm payment
API-->>ConnectJobHelper: Success/failure
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 40
🔭 Outside diff range comments (2)
app/res/layout/fragment_connect_jobs_list.xml (1)
21-31: RecyclerView overlaps the empty-state label
connect_no_jobs_textis constrained only to the top of the parent, whilervJobListis constrained top→top of the parent as well.
When the “no jobs” label becomesVISIBLE, the two views will sit on top of each other.A minimal, non-breaking fix:
- app:layout_constraintTop_toTopOf="parent" + app:layout_constraintTop_toBottomOf="@id/connect_no_jobs_text"(You may also want to tweak margins.)
app/src/org/commcare/services/FCMMessageData.java (1)
114-127: Fix serialization mismatch between write and read methodsThe
writeExternalmethod writes theactionstring field, butreadExternalexpects theactionTypeenum name. This will cause deserialization failures.@Override public void writeExternal(DataOutputStream out) throws IOException { - ExtUtil.writeString(out, action.toString()); + ExtUtil.writeString(out, actionType.name()); ExtUtil.writeString(out, username); ExtUtil.writeString(out, domain); ExtUtil.writeNumeric(out, creationTime.getMillis()); }
🧹 Nitpick comments (32)
app/res/values/dimens.xml (1)
154-154: Remove the superfluous blank line for cleaner XML formattingThe added empty line after
<dimen name="dp48">48dp</dimen>breaks the otherwise compact, consistent style used throughout this file. Dropping it keeps the resource file tidy.-app/res/layout/fragment_connect_job_intro.xml (1)
73-100: Minor layout polish: duplicate padding & ineffectivelayout_gravity
- Both
cardButtonViewand its childLinearLayoutadd16dppadding, doubling the intended inset.- Inside a vertical
LinearLayout,android:layout_gravity="end"on theMaterialButtononly aligns it horizontally; the button still appears below the text blocks. If the design calls for the button to be on the same row as the label, keep the childLinearLayouthorizontal; if the button should be below, drop the gravity attribute.Optional clean-up diff (button below the texts):
- android:padding="16dp"> + android:padding="0dp"> <!-- keep padding only on the CardView --> @@ - android:layout_gravity="end" + android:gravity="end"(Consider using
app:icon/app:iconGravityinstead ofdrawableRightfor MaterialButton.)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChatData.java (2)
5-22: Add basic documentation & make immutable where possibleThis is a public data-model that will be passed around adapters and fragments. Adding a short class-level Javadoc and marking the truly fixed fields as
finalimproves readability and thread-safety at almost no cost.- private String messageId; - private int type; - private String message; - private String userName; - private Date timestamp; + /** Server-assigned unique id (never null). */ + private final String messageId; + /** One of ConnectMessageAdapter.VIEW_TYPE_* */ + private int type; + private String message; + private String userName; + private final Date timestamp;(You can still mutate
type,message, etc. via setters.)
57-71: Consider implementingequals/hashCode(or making it a Kotlin data class)Most RecyclerView adapters rely on object equality (e.g. in
DiffUtil.ItemCallback) to animate updates efficiently.
Without a stable equality definition every update may cause full rebinds.Up to you, but worth keeping in mind before the messaging UI reaches production scale.
app/src/org/commcare/connect/ConnectDateUtils.kt (1)
25-27: Consider thread safety for formatDateTime.The
formatDateTimefunction usesSimpleDateFormat.getDateTimeInstance()which creates a new instance each time, making it thread-safe. However, this might be less efficient than reusing a synchronized instance like the other formatters.If this function is called frequently, consider using a synchronized DateFormat instance similar to the other formatters:
+val dateTimeFormat: DateFormat = SimpleDateFormat.getDateTimeInstance() + fun formatDateTime(date: Date?): String { - return SimpleDateFormat.getDateTimeInstance().format(date) + synchronized(dateTimeFormat) { + return dateTimeFormat.format(date) + } }app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)
37-37: Consider thread safety for the static boolean flag.The static
isActiveboolean could face race conditions if multiple instances exist or if accessed from different threads. Consider usingAtomicBooleanor synchronization if thread safety is a concern.-public static boolean isActive; +public static final AtomicBoolean isActive = new AtomicBoolean(false);Then update the usage:
-isActive = true; +isActive.set(true);app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java (1)
150-151: Hardcoded color usage should use theme attributes.The code uses hardcoded color resources (
R.color.blue,R.color.dark_grey) which may not adapt to theme changes. Consider using theme attributes instead.-confirmText.setTextColor( - context.getResources().getColor(enabled ? R.color.blue : R.color.dark_grey)); +confirmText.setTextColor( + ContextCompat.getColor(context, enabled ? R.color.blue : R.color.dark_grey));Or better yet, use theme attributes:
-confirmText.setTextColor( - context.getResources().getColor(enabled ? R.color.blue : R.color.dark_grey)); +TypedValue typedValue = new TypedValue(); +context.getTheme().resolveAttribute(enabled ? android.R.attr.textColorPrimary : android.R.attr.textColorSecondary, typedValue, true); +confirmText.setTextColor(typedValue.data);app/src/org/commcare/adapters/ConnectProgressJobSummaryAdapter.java (1)
53-56: Consider using DiffUtil for better performance.The adapter uses
notifyDataSetChanged()which can be inefficient for large datasets. Consider usingDiffUtilfor more efficient updates.public void setDeliverySummaries(List<ConnectDeliveryPaymentSummaryInfo> deliverySummaries) { + DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff(new DiffUtil.Callback() { + @Override + public int getOldListSize() { + return ConnectProgressJobSummaryAdapter.this.deliverySummaries.size(); + } + + @Override + public int getNewListSize() { + return deliverySummaries.size(); + } + + @Override + public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) { + return ConnectProgressJobSummaryAdapter.this.deliverySummaries.get(oldItemPosition).getPaymentUnitName() + .equals(deliverySummaries.get(newItemPosition).getPaymentUnitName()); + } + + @Override + public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) { + ConnectDeliveryPaymentSummaryInfo oldItem = ConnectProgressJobSummaryAdapter.this.deliverySummaries.get(oldItemPosition); + ConnectDeliveryPaymentSummaryInfo newItem = deliverySummaries.get(newItemPosition); + return oldItem.getPaymentUnitAmount() == newItem.getPaymentUnitAmount() && + oldItem.getPaymentUnitMaxDaily() == newItem.getPaymentUnitMaxDaily(); + } + }); this.deliverySummaries = deliverySummaries; - notifyDataSetChanged(); + diffResult.dispatchUpdatesTo(this); }app/res/values/strings.xml (1)
637-639: Consider consolidating duplicate camera permission strings.The new
camera_permission_titleandcamera_permission_msgstrings appear to duplicate the existingpersonalid_camera_permission_titleandpersonalid_camera_permission_msgstrings. Consider reusing existing strings to reduce duplication.-<string name="camera_permission_title">Permission for camera</string> -<string name="camera_permission_msg">In order to take a picture, CommCare needs permission to use your device camera.</string>And update references to use the existing PersonalID strings:
-getString(R.string.camera_permission_title) +getString(R.string.personalid_camera_permission_title)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (1)
72-72: Replace deprecated getColor() method.Use
ContextCompat.getColor()instead of the deprecatedgetResources().getColor()method.- binding.getRoot().setBackgroundColor(getResources().getColor(R.color.white)); + binding.getRoot().setBackgroundColor(ContextCompat.getColor(requireContext(), R.color.white));app/src/org/commcare/connect/ConnectAppUtils.kt (1)
21-22: Good thread safety for download state managementUsing
@VolatileforisAppDownloadingand checking/setting it atomically helps prevent concurrent downloads. However, there's still a small race condition window between the check and set.Consider using
AtomicBooleanfor more robust thread safety:-@Volatile -private var isAppDownloading = false +private val isAppDownloading = AtomicBoolean(false) fun downloadApp(installUrl: String?, listener: ResourceEngineListener?) { - if (!isAppDownloading) { - isAppDownloading = true + if (isAppDownloading.compareAndSet(false, true)) {Also applies to: 29-30, 41-42, 44-45, 48-49
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
47-47: Use != instead of !== for enum comparisonIn Kotlin,
!=is the standard equality operator for enums. The reference equality operator!==works but is unconventional for enum comparisons.-return !record.isLearning || activeJob!!.status !== ConnectJobRecord.STATUS_DELIVERING +return !record.isLearning || activeJob!!.status != ConnectJobRecord.STATUS_DELIVERINGapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (3)
69-78: Use HashMap instead of Hashtable and StringBuilder for string concatenationHashtable is a legacy class. Since thread safety isn't needed here, HashMap would be more efficient. Also, use StringBuilder for better performance when concatenating in a loop.
if (job.isMultiPayment() && completed > 0) { - Hashtable<String, Integer> paymentCounts = job.getDeliveryCountsPerPaymentUnit(false); + HashMap<String, Integer> paymentCounts = job.getDeliveryCountsPerPaymentUnit(false); for (ConnectPaymentUnitRecord unit : job.getPaymentUnits()) { String key = String.valueOf(unit.getUnitId()); int count = paymentCounts.containsKey(key) ? paymentCounts.get(key) : 0; completedText.append(String.format(Locale.getDefault(), "\n%s: %d", unit.getName(), count)); } }
88-98: Use getOrDefault() for cleaner codeReplace containsKey() checks with getOrDefault() for more concise and readable code.
for (ConnectPaymentUnitRecord unit : job.getPaymentUnits()) { String unitIdKey = String.valueOf(unit.getUnitId()); - HashMap<String, Integer> statusCounts = statusMap.containsKey(unitIdKey) ? statusMap.get(unitIdKey) - : new HashMap<>(); - int approved = statusCounts.containsKey("approved") ? statusCounts.get("approved") : 0; + HashMap<String, Integer> statusCounts = statusMap.getOrDefault(unitIdKey, new HashMap<>()); + int approved = statusCounts.getOrDefault("approved", 0); int remaining = unit.getMaxTotal() - approved;
115-137: Simplify status counting logic using modern HashMap methodsUse getOrDefault() and merge() for cleaner, more concise code.
private HashMap<String, HashMap<String, Integer>> getStatusMap(ConnectJobRecord job) { HashMap<String, HashMap<String, Integer>> statusMap = new HashMap<>(); for (ConnectJobDeliveryRecord delivery : job.getDeliveries()) { if (delivery == null) continue; String slug = delivery.getSlug(); - HashMap<String, Integer> countMap; - - if (statusMap.containsKey(slug)) { - countMap = statusMap.get(slug); - } else { - countMap = new HashMap<>(); - statusMap.put(slug, countMap); - } - - String status = delivery.getStatus(); - int count = countMap.containsKey(status) ? countMap.get(status) : 0; - countMap.put(status, count + 1); + HashMap<String, Integer> countMap = statusMap.computeIfAbsent(slug, k -> new HashMap<>()); + String status = delivery.getStatus(); + countMap.merge(status, 1, Integer::sum); } return statusMap; }app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
233-241: Simplify dialog margin calculationsThe margin calculation is overly complex. Consider using dimension resources for consistent spacing.
- int marginInPx = (int)(10 * context.getResources().getDisplayMetrics().density); - float marginFraction = marginInPx / - (float) context.getResources().getDisplayMetrics().widthPixels; - layoutParams.horizontalMargin = marginFraction; + int marginInPx = context.getResources().getDimensionPixelSize(R.dimen.dialog_horizontal_margin); dialog.getWindow().setAttributes(layoutParams); dialog.getWindow().setLayout(WindowManager.LayoutParams.MATCH_PARENT, WindowManager.LayoutParams.WRAP_CONTENT); dialog.getWindow().getDecorView().setPadding(marginInPx, 0, marginInPx, 0);You'll need to define the dimension resource in dimens.xml.
app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java (3)
23-65: Consider performance optimization for large message datasetsLoading all messages into memory could cause performance issues with large datasets. Consider pagination or lazy loading for better scalability.
As the message count grows, this approach of loading all messages at once could become a bottleneck. Consider:
- Implementing pagination for message loading
- Using database joins or queries to get only recent messages per channel
- Caching the preview text in the database to avoid recalculation
93-93: Fix incorrect commentThe comment references "payments" but this code handles messaging channels.
- //Delete payments that are no longer available + //Delete channels that are no longer available
167-167: Fix incorrect commentThe comment references "payments" but this code handles messages.
- //Delete payments that are no longer available + //Delete messages that are no longer availableapp/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java (1)
76-93: Consider caching parsed arguments to avoid redundant parsingThe fragment parses arguments twice - once in
onCreateand again inonCreateView. Consider storing the title in a field duringonCreateto avoid re-parsing.private boolean getLearnApp; +private String downloadTitle; @Override public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); ConnectDownloadingFragmentArgs args = ConnectDownloadingFragmentArgs.fromBundle(getArguments()); getLearnApp = args.getLearning(); + downloadTitle = args.getTitle(); @Override public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) { - ConnectDownloadingFragmentArgs args = ConnectDownloadingFragmentArgs.fromBundle(getArguments()); requireActivity().setTitle(job.getTitle()); View view = inflater.inflate(R.layout.fragment_connect_downloading, container, false); TextView titleTv = view.findViewById(R.id.connect_downloading_title); - titleTv.setText(args.getTitle()); + titleTv.setText(downloadTitle);app/src/org/commcare/activities/connect/ConnectActivity.java (1)
180-188: Consider moving database query off the UI threadThe
getUnviewedMessages()call queries the database on the UI thread, which could cause ANRs with large datasets.Consider using a background thread or coroutine:
public void updateMessagingIcon() { if(messagingMenuItem != null) { - int icon = R.drawable.ic_connect_messaging_base; - if(ConnectMessagingDatabaseHelper.getUnviewedMessages(this).size() > 0) { - icon = R.drawable.ic_connect_messaging_unread; - } - messagingMenuItem.setIcon(ResourcesCompat.getDrawable(getResources(), icon, null)); + new Thread(() -> { + boolean hasUnviewed = ConnectMessagingDatabaseHelper.getUnviewedMessages(this).size() > 0; + runOnUiThread(() -> { + int icon = hasUnviewed ? R.drawable.ic_connect_messaging_unread : R.drawable.ic_connect_messaging_base; + messagingMenuItem.setIcon(ResourcesCompat.getDrawable(getResources(), icon, null)); + }); + }).start(); } }app/src/org/commcare/connect/MessageManager.java (1)
89-113: Add error logging for better debuggingAll error callbacks silently fail without logging, making debugging difficult.
Consider adding appropriate logging:
@Override public void processFailure(int responseCode, @Nullable InputStream errorResponse, String url) { + Logger.log(LogTypes.TYPE_WARNING_NETWORK, "Message retrieval failed with code: " + responseCode); listener.connectActivityComplete(false); } @Override public void processNetworkFailure() { + Logger.log(LogTypes.TYPE_WARNING_NETWORK, "Message retrieval failed due to network error"); listener.connectActivityComplete(false); }app/src/org/commcare/utils/FirebaseMessagingUtil.java (2)
257-257: Use primitive boolean instead of Boolean wrapperThe parameter type should be
booleaninstead ofBooleansince null values aren't being utilized.- private static Intent handleResumeLearningOrDeliveryJobPushNotification(Boolean isLearning, Context context, FCMMessageData fcmMessageData) { + private static Intent handleResumeLearningOrDeliveryJobPushNotification(boolean isLearning, Context context, FCMMessageData fcmMessageData) {
456-458: UsestartsWith()for more precise action checkingUsing
contains()could match "ccc_" anywhere in the string. Consider usingstartsWith()for more precise matching.private static boolean hasCccAction(String action) { - return action != null && action.contains("ccc_"); + return action != null && action.startsWith("ccc_"); }app/src/org/commcare/adapters/JobListConnectHomeAppsAdapter.java (3)
35-36: Use final modifiers and conventional view type valuesView type constants should be
finaland use conventional values starting from 0.- private static int NON_CORRUPT_JOB_VIEW = 4983; - private static int CORRUPT_JOB_VIEW = 9533; + private static final int NON_CORRUPT_JOB_VIEW = 0; + private static final int CORRUPT_JOB_VIEW = 1;
142-142: Use ContextCompat.getColor() consistentlyReplace deprecated
getResources().getColor()withContextCompat.getColor()for consistency with the rest of the codebase.- progressColor = context.getResources().getColor(R.color.connect_blue_color); + progressColor = ContextCompat.getColor(context, R.color.connect_blue_color);- progressColor = context.getResources().getColor(R.color.connect_green); + progressColor = ContextCompat.getColor(context, R.color.connect_green);Also applies to: 145-145
31-31: Remove redundant context fieldThe
mContextfield is redundant since context can always be obtained from the ViewHolder's itemView.Remove the instance field and use
holder.itemView.getContext()when needed, or pass context as a parameter to bind methods.Also applies to: 40-40, 49-49
app/src/org/commcare/adapters/ChannelAdapter.java (2)
34-37: Consider using DiffUtil for better performanceUsing
notifyDataSetChanged()causes the entire RecyclerView to rebind all items. For better performance, especially with larger lists, consider implementing DiffUtil to calculate and dispatch only the actual changes.+import androidx.recyclerview.widget.DiffUtil; public void setChannels(List<ConnectMessagingChannelRecord> channels) { + DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff(new DiffUtil.Callback() { + @Override + public int getOldListSize() { + return ChannelAdapter.this.channels.size(); + } + + @Override + public int getNewListSize() { + return channels.size(); + } + + @Override + public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) { + return ChannelAdapter.this.channels.get(oldItemPosition).getChannelId() + .equals(channels.get(newItemPosition).getChannelId()); + } + + @Override + public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) { + return ChannelAdapter.this.channels.get(oldItemPosition).equals( + channels.get(newItemPosition)); + } + }); this.channels = channels; - notifyDataSetChanged(); + diffResult.dispatchUpdatesTo(this); }
12-13: Remove unused importsThese imports are not used in the code and should be removed:
ConnectMessagingMessageRecordConnectDatabaseHelperParseException-import org.commcare.android.database.connect.models.ConnectMessagingMessageRecord; -import org.commcare.connect.database.ConnectDatabaseHelper; import org.commcare.dalvik.R; -import java.text.ParseException; import java.text.SimpleDateFormat;Also applies to: 18-18
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
81-81: Follow Java naming conventions for method namesMethod name should use camelCase convention.
-private void jobCardDataHandle(ConnectJobRecord job) { +private void handleJobCardData(ConnectJobRecord job) {Also update the method call on line 76:
-jobCardDataHandle(job); +handleJobCardData(job);app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (2)
47-53: Update JavaDoc to match method signatureThe JavaDoc is incomplete and needs to be updated after refactoring to pass parameters.
/** * Go through job records to find the matching payment using payment-id * - * @param payments payment list fetched data from local DB * @param context + * @param payments payment list fetched from local DB + * @param paymentId the payment ID to search for + * @param paymentStatus the payment status to update */ -private void getPaymentsFromJobs(Context context, List<ConnectJobPaymentRecord> payments) { +private void getPaymentsFromJobs(Context context, List<ConnectJobPaymentRecord> payments, String paymentId, boolean paymentStatus) {
62-66: Improve callback comment and update method signatureThe comment should be more descriptive about why nothing needs to be done in the callback.
-private void handlePayment(Context context, ConnectJobPaymentRecord payment) { +private void handlePayment(Context context, ConnectJobPaymentRecord payment, boolean paymentStatus) { ConnectJobHelper.INSTANCE.updatePaymentConfirmed(context, payment, paymentStatus, success -> { - //Nothing to do + // No further action needed - payment confirmation status has been updated in the database }); }
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelConsentBottomSheet.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java
Outdated
Show resolved
Hide resolved
… back to string). Cleaned up a couple other small warnings
Fixed a typo in "aqua".
ConnectActivity handles sync menu item and calls child fragment when it's a RefreshableFragment.
DispatchActivity now clears IS_LAUNCH_FROM_CONNECT after retrieving it (so it only launches LoginActivity with the flag once). LoginActivity performs auto-login once in onResume (rather than buried in evaluateConnectAppState). Refactored ConnectAppUtils.checkAutoLoginAndOverridePassword to two simpler functions.
…able. Code sets active jobId with CommCareApplication so it can be included in analytics events when available. Login and Home activities use seated app to determine active job. ConnectActivity maintains active job when outside of CC app context (available for fragments to use). Cleaned up some obsolete methods in ConnectJobUtils.
Using TextUtils.join (to support minSdk).
…ng (for thread safety).
…c for proper visibility.
app/src/org/commcare/adapters/JobListConnectHomeAppsAdapter.java
Outdated
Show resolved
Hide resolved
| app:strokeWidth="15" | ||
| app:progressColor="@color/connect_aqua" | ||
| app:progressBackgroundColor="@color/connect_blackist_dark_blue_color" |
There was a problem hiding this comment.
optional: would be nice for these values to be set in the app theme instead in order to not specify them everytime we use this progress bar plus in order to use more standard values across the app.
There was a problem hiding this comment.
Leaving this work for a different PR
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
Show resolved
Hide resolved
| if (record.isUsingLocalPassphrase) { | ||
| //Report to analytics so we know when this stops happening | ||
| FirebaseAnalyticsUtil.reportCccAppAutoLoginWithLocalPassphrase(seatedAppId) | ||
| } |
There was a problem hiding this comment.
can we validate if this is still happening and remove if not ?
There was a problem hiding this comment.
I'm glad you asked! A quick search in BigQuery showed this event happening 403 times in the past 7 days. I would have expected zero or very close to it! I'll create a ticket to look into this, seems like something isn't working as expected and we should investigate it further. But I'd also say this event should stick around for a while as we figure it out and possible apply additional fixes.
There was a problem hiding this comment.
that seems quite concerning to me and feels like it would at-le be a P2 if that has happened these many times. Would you be able to ping the slack with ticket and more details on it once you have had a chance to ticket it with some details.
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java
Outdated
Show resolved
Hide resolved
|
|
||
| public class ConnectMessageFragment extends Fragment { | ||
| public static String activeChannel; | ||
| private static String activeChannel; |
There was a problem hiding this comment.
Whils this is a good compromise to not leak the static state, I would highly avoid having static variables in classes with a lifecycle and think that can cause a bunch of highly cryptic bugs.
Curious if this can't be a non-static variable by enforcing that anyone wanting to access this should first get the fragment instance from the fragment manger instead ?
There was a problem hiding this comment.
This one is proving to be a bit tricky. FragmentManager isn't an option since the only accessor to this variable is the FirebaseMessagingService, which only has Application context (FragmentManager requires Activity context). I'm having trouble finding the proper solution for this, so suggest we address this in a separate PR. In the meantime, I think the current solution is pretty safe since the static variable is only modified by the fragment when it resumes and pauses, and is only accessed by the messaging service to determine whether to create push notifications.
Prevented ConnectDeliveryProgressFragment from refreshing child fragments before they've been loaded.
Addressing comments from the main Connect PR in order to prepare for the final merge to master.