Pm-bug fixes for connect, and biometric fix #3076

pm-dimagi · 2025-05-06T20:59:26Z

Product Description

Bug fixes
https://dimagi.atlassian.net/browse/QA-7728
QA-7729
QA-7730
QA-7727
QA-7707
cross-request:dimagi/commcare-core#1475

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Labels and Review

Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

OrangeAndGreen · 2025-05-06T21:38:19Z

app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java

        String dailyFinish = getDailyFinishTime();

-        if (dailyStart.length() == 0 || dailyFinish.length() == 0) {
+        if (dailyStart == "null" || dailyFinish == "null" || dailyStart.isEmpty() || dailyFinish.isEmpty()) {


We're pushing back on doing these checks in mobile code, and instead requiring server fixes when we find they're sending the "null" string. This may have been addressed already in recent server work but if not let's submit a new ticket for them to fix it and remove the check for "null" here. @shubham1g5 Your thoughts on checking for empty string here? We just want null, correct?

Think empty checks are fine to do but not "null" String checks as they seems like a server mishandling of data.

app/src/org/commcare/activities/LoginActivity.java

shubham1g5 · 2025-05-07T04:40:58Z

app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java

        String dailyFinish = getDailyFinishTime();

-        if (dailyStart.length() == 0 || dailyFinish.length() == 0) {
+        if (dailyStart == "null" || dailyFinish == "null" || dailyStart.isEmpty() || dailyFinish.isEmpty()) {


Think empty checks are fine to do but not "null" String checks as they seems like a server mishandling of data.

shubham1g5 · 2025-05-07T13:59:22Z

app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java

+    private String safeString(JSONObject obj, String key) {
+        return obj.isNull(key) ? "" : obj.optString(key);
+    }


can we make this an Kotlin extension function so that it's accessible to other classes.

Also can we reply with null instead with empty string there as that's more correct representation of the data being sent.

call this optStringSafe instead to signify it's a wrapper over optString

shubham1g5 · 2025-05-07T14:31:53Z

app/src/org/commcare/utils/JsonExtensions.kt

+
+fun JSONObject.optStringSafe(key: String): String? {
+    val value = this.optString(key, null)
+    return if (this.isNull(key) || value == "null") null else value


does isNull doesn't check for "null" ?

shubham1g5 · 2025-05-07T14:33:21Z

app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java

+            job.dailyStartTime = JsonExtensions.optStringSafe(flags, META_DAILY_START_TIME) != null
+                    ? JsonExtensions.optStringSafe(flags, META_DAILY_START_TIME) : "";


you can just pass the empty string fallback to optStringSafe itself as an argument and have the fallback pass through further to optString

shubham1g5 · 2025-05-07T14:51:26Z

app/src/org/commcare/utils/JsonExtensions.kt

+    val value = fallback?.let { this.optString(key, it) }
+    return value ?: fallback


can't we just change it to this.optString(key, fallback) irrespective of whether fallback is null or not ?

shubham1g5 · 2025-05-07T14:51:36Z

app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java

+            job.dailyStartTime =
+                    JsonExtensions.optStringSafe(flags, META_DAILY_START_TIME, "");
+            job.dailyFinishTime = JsonExtensions.optStringSafe(flags, META_DAILY_FINISH_TIME, "");


can use some formatting.

OrangeAndGreen · 2025-05-07T15:00:32Z

app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java

        String dailyFinish = getDailyFinishTime();

-        if (dailyStart.length() == 0 || dailyFinish.length() == 0) {
+        if (dailyStart == null || dailyFinish == null || dailyStart.isEmpty() || dailyFinish.isEmpty()) {


Nit: You could use Strings.isNullOrEmpty() here to simplify this slightly

-bug fixes for connect, and biometric fix

b6628cb

pm-dimagi requested review from OrangeAndGreen and shubham1g5 May 6, 2025 20:59

pm-dimagi added cross requested skip-integration-tests Skip android tests. labels May 6, 2025

OrangeAndGreen requested changes May 6, 2025

View reviewed changes

shubham1g5 reviewed May 7, 2025

View reviewed changes

pm-dimagi added 3 commits May 7, 2025 12:37

-fixed the formedstorage function name

7a4b8b2

-fixed the formedstorage function name for old pm_cluster

486a7d0

-get rid of "null" string check

46df00e

pm-dimagi requested review from OrangeAndGreen and shubham1g5 May 7, 2025 13:53

shubham1g5 requested changes May 7, 2025

View reviewed changes

-create the common extension file for jsonobject

51606a3

pm-dimagi requested a review from shubham1g5 May 7, 2025 14:29

shubham1g5 requested changes May 7, 2025

View reviewed changes

-refactored the extension function

2ca0c5a

pm-dimagi requested a review from shubham1g5 May 7, 2025 14:49

shubham1g5 requested changes May 7, 2025

View reviewed changes

-formatted code

5037989

shubham1g5 approved these changes May 7, 2025

View reviewed changes

pm-dimagi merged commit e49487d into pm_beta_relase_472005 May 7, 2025
1 check passed

OrangeAndGreen approved these changes May 7, 2025

View reviewed changes

		job.dailyStartTime = JsonExtensions.optStringSafe(flags, META_DAILY_START_TIME) != null
		? JsonExtensions.optStringSafe(flags, META_DAILY_START_TIME) : "";

		val value = fallback?.let { this.optString(key, it) }
		return value ?: fallback

Uh oh!

Pm-bug fixes for connect, and biometric fix #3076

Pm-bug fixes for connect, and biometric fix #3076

Uh oh!

Conversation

pm-dimagi commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Labels and Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pm-dimagi commented May 6, 2025 •

edited

Loading

shubham1g5 May 7, 2025 •

edited

Loading