Notification after PersonalID recovery when previous login invalidated #3493
Conversation
… via NotificationUtil. Showing notification about second device login (when appropriate) after successful recovery.
📝 WalkthroughWalkthroughThis PR adds internationalized UI strings and session handling for PersonalID second device login scenarios. It introduces two new string keys across seven language variants, extends the Sequence Diagram(s)sequenceDiagram
participant Server as Server/API
participant Parser as ConfirmBackupCodeResponseParser
participant SessionData as PersonalIdSessionData
participant Fragment as PersonalIdBackupCodeFragment
participant Notif as NotificationUtil
Server->>Parser: ConfirmBackupCode Response<br/>(with previousDevice & lastAccessed)
Parser->>Parser: Parse JSON fields
Parser->>SessionData: Create with previousDevice<br/>& lastAccessed set
Parser-->>Fragment: Return parsed response
Fragment->>Fragment: handleSuccessfulRecovery()
Fragment->>Fragment: handleSecondDeviceLogin()
alt previousDevice != null AND lastAccessed != null
Fragment->>Notif: showNotification(title, message,<br/>previousDevice, lastAccessed)
Notif->>Notif: Display notification
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/res/values-sw/strings.xml`:
- Around line 490-491: Update the string resource
personalid_second_device_login_message to use positional format arguments
(replace non-positional %s with %1$s and %2$s) and keep the term "PersonalID"
untranslated (replace "Kitambulisho cha Kibinafsi" with "PersonalID") so the
message matches the codebase standard and other locales; also ensure
personalid_second_device_login_title remains the same wording for the title but
verify any other occurrences of "Kitambulisho cha Kibinafsi" (e.g., the other
string entries nearby) are replaced with "PersonalID" for consistency.
In
`@app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java`:
- Around line 48-53: The parser sets sessionData.previousDevice then directly
assigns the parsed date from DateUtils.parseDate, which can return null and
leave sessionData in an inconsistent state; update
ConfirmBackupCodeResponseParser so that when previousDevice != null and
lastAccessedString != null you first call
DateUtils.parseDate(lastAccessedString) into a local Date, validate it is
non-null (and optionally catch parsing exceptions), only then call
sessionData.setLastAccessed(date); if parsing fails throw a JSONException (or
rethrow the caught exception) instead of setting a null date to ensure
consistent state between sessionData.setPreviousDevice and
sessionData.setLastAccessed.
🧹 Nitpick comments (6)
app/res/values-es/strings.xml (1)
481-482: Use positional format args (%1$s,%2$s) for i18n safety.Proposed change
- <string name="personalid_second_device_login_message">Se ha cerrado su sesión anterior en %s, que se utilizó por última vez el %s. Tenga en cuenta que solo se puede iniciar sesión en PersonalID en un dispositivo a la vez.</string> + <string name="personalid_second_device_login_message">Se ha cerrado su sesión anterior en %1$s, que se utilizó por última vez el %2$s. Tenga en cuenta que solo se puede iniciar sesión en PersonalID en un dispositivo a la vez.</string>app/res/values-ti/strings.xml (1)
476-477: Use positional format args (%1$s,%2$s) for i18n safety.Proposed change
- <string name="personalid_second_device_login_message">ካብቲ ዝሓለፈ ክፍለ ግዜኻ ኣብ %s ወጺእካ ኣለኻ፣ እዚ ድማ ንመወዳእታ ግዜ ኣብ ዝተጠቕመሉ %s እዩ። PersonalID ኣብ ሓደ እዋን ኣብ ሓደ መሳርሒ ጥራይ ክትኣቱ ከም እትኽእል ኣስተውዕሉ።</string> + <string name="personalid_second_device_login_message">ካብቲ ዝሓለፈ ክፍለ ግዜኻ ኣብ %1$s ወጺእካ ኣለኻ፣ እዚ ድማ ንመወዳእታ ግዜ ኣብ ዝተጠቕመሉ %2$s እዩ። PersonalID ኣብ ሓደ እዋን ኣብ ሓደ መሳርሒ ጥራይ ክትኣቱ ከም እትኽእል ኣስተውዕሉ።</string>app/res/values-pt/strings.xml (1)
491-492: Use positional format args (%1$s,%2$s) for i18n safety.Proposed change
- <string name="personalid_second_device_login_message">Foi desligado da sua sessão anterior em %s, que foi utilizada pela última vez em %s. Note que o PersonalID só pode ser utilizado num dispositivo de cada vez.</string> + <string name="personalid_second_device_login_message">Foi desligado da sua sessão anterior em %1$s, que foi utilizada pela última vez em %2$s. Note que o PersonalID só pode ser utilizado num dispositivo de cada vez.</string>app/res/values/strings.xml (1)
642-643: Use positional format args to improve localization robustness. Replace%swith%1$sand%2$sin the personalid_second_device_login_message string. The call site correctly passespreviousDevice(first) andDateUtils.getShortStringValue(lastAccessed)(second), ensuring proper date formatting, but positional placeholders prevent accidental reordering by translators.Proposed change
- <string name="personalid_second_device_login_message">You have been logged out of your previous session on %s, which was last used %s. Please note that PersonalID can only be logged in on one device at a time.</string> + <string name="personalid_second_device_login_message">You have been logged out of your previous session on %1$s, which was last used %2$s. Please note that PersonalID can only be logged in on one device at a time.</string>app/src/org/commcare/utils/NotificationUtil.java (1)
30-36: LGTM!The null-guard for
actionIntentis a sensible improvement that allows informational notifications without tap actions. This aligns well with the second device login notification use case.Optional consideration: For notifications without an action intent, you might want to call
setAutoCancel(true)so the notification dismisses on tap, providing better UX for informational notifications.💡 Optional: Auto-cancel for action-less notifications
if(actionIntent != null) { int pendingIntentFlags = PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE; PendingIntent contentIntent = PendingIntent.getActivity(context, 0, actionIntent, pendingIntentFlags); notification.setContentIntent(contentIntent); + } else { + notification.setAutoCancel(true); }app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
258-270: Consider guarding against detached fragment state.
requireContext()can throwIllegalStateExceptionif the fragment is detached when the API callback executes. SincehandleSecondDeviceLogin()is called from an async callback inconfirmBackupCode(), the fragment could theoretically be detached before the callback fires.Also, using
titleId(a string resource ID) as thenotificationIdparameter is unconventional. While it works since both are integers, string resource IDs can be large values that might collide with other notification IDs. Consider defining a dedicated constant for this notification ID in a central location.♻️ Suggested improvements
private void handleSecondDeviceLogin() { - if(personalIdSessionData.getPreviousDevice() != null && personalIdSessionData.getLastAccessed() != null) { + Context context = getContext(); + if(context == null) { + return; + } + if(personalIdSessionData.getPreviousDevice() != null && personalIdSessionData.getLastAccessed() != null) { int titleId = R.string.personalid_second_device_login_title; - NotificationUtil.showNotification(requireContext(), + NotificationUtil.showNotification(context, CommCareNoficationManager.NOTIFICATION_CHANNEL_SERVER_COMMUNICATIONS_ID, - titleId, + CommCareNoficationManager.NOTIFICATION_ID_SECOND_DEVICE_LOGIN, // Define this constant getString(titleId), getString(R.string.personalid_second_device_login_message, personalIdSessionData.getPreviousDevice(), DateUtils.getShortStringValue(personalIdSessionData.getLastAccessed())), null); } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/res/values-es/strings.xmlapp/res/values-fr/strings.xmlapp/res/values-hi/strings.xmlapp/res/values-pt/strings.xmlapp/res/values-sw/strings.xmlapp/res/values-ti/strings.xmlapp/res/values/strings.xmlapp/src/org/commcare/android/database/connect/models/PersonalIdSessionData.ktapp/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/utils/NotificationUtil.java
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR `#3048` introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/activities/StandardHomeActivityUIController.java:0-0
Timestamp: 2025-06-20T13:21:20.908Z
Learning: User OrangeAndGreen prefers to handle code issues in separate PRs rather than immediately fixing them in the current PR when they are not directly related to the main changes.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/network/ApiPersonalId.java:0-0
Timestamp: 2025-07-29T14:56:40.681Z
Learning: User OrangeAndGreen prefers to defer ApiPersonalId refactoring (specifically InputStream resource management improvements) to separate PRs rather than mixing them with Connect feature work, even when the code is already in master.
📚 Learning: 2025-12-08T12:01:48.545Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3455
File: app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt:64-93
Timestamp: 2025-12-08T12:01:48.545Z
Learning: In the CommCare Android Connect notification system (RetrieveNotificationsResponseParser.kt), messages are only decrypted using channels that already exist in the database with their encryption keys. When new channels arrive without keys, a separate network flow fetches the channel keys, and then the notification API is re-called to decrypt any messages for those channels. This explains why parseAndSeparateNotifications() correctly uses existingChannels from the database rather than newly parsed channels.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR `#3048` "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
📚 Learning: 2025-06-04T19:17:21.213Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
📚 Learning: 2025-01-21T17:27:58.754Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using NotNull.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
📚 Learning: 2025-05-22T14:32:53.133Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
Applied to files:
app/res/values-pt/strings.xmlapp/res/values/strings.xmlapp/res/values-es/strings.xml
📚 Learning: 2025-02-04T21:22:56.537Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
📚 Learning: 2025-01-21T18:20:43.883Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
📚 Learning: 2025-06-13T15:53:12.951Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
📚 Learning: 2025-12-10T16:39:05.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3448
File: app/src/org/commcare/gis/EntityMapUtils.kt:214-234
Timestamp: 2025-12-10T16:39:05.007Z
Learning: In Kotlin files (here: app/src/org/commcare/gis/EntityMapUtils.kt), follow fail-fast guidance: treat conditions that indicate a programmer error or broken invariant as exceptions rather than defensive handling. If a condition represents a bug (e.g., internal API assumptions like array/list size mismatches), crash early to alert developers instead of attempting to recover. Use clear runtime exceptions (e.g., IllegalStateException, IllegalArgumentException) or assertions where appropriate, and avoid swallowing or masking bugs. Ensure invariants are checked and failures are surfaced with actionable messages for quick debugging.
Applied to files:
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt
🧬 Code graph analysis (1)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (2)
app/src/org/commcare/CommCareNoficationManager.java (1)
CommCareNoficationManager(34-212)app/src/org/commcare/utils/NotificationUtil.java (1)
NotificationUtil(20-51)
🔇 Additional comments (4)
app/res/values-fr/strings.xml (1)
485-486: LGTM!The French translations for the second device login notification are properly added with correct placeholder usage for the device name and last accessed date.
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
46-49: LGTM!The new
previousDeviceandlastAccessedfields are well-documented and appropriately nullable, aligning with the optional nature of these values in the server response. TheDatetype is serializable, maintaining the class'sSerializablecontract.app/res/values-hi/strings.xml (1)
478-479: LGTM!The Hindi translations for the second device login notification are properly added with correct placeholder usage, consistent with other locale files.
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
221-234: LGTM!The placement of
handleSecondDeviceLogin()after storing the user record and logging the recovery result is appropriate. The notification is posted to the system tray (non-blocking), allowing the success navigation to proceed immediately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
app/res/values-sw/strings.xml
Outdated
| <string name="personalid_second_device_login_title">Ingia kwenye Kifaa cha Pili</string> | ||
| <string name="personalid_second_device_login_message">Umetoka kwenye kipindi chako cha awali kwenye %s, ambacho kilitumika mara ya mwisho %s. Tafadhali kumbuka kuwa Kitambulisho cha Kibinafsi kinaweza kuingia kwenye kifaa kimoja tu kwa wakati mmoja.</string> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure
find . -name "strings.xml" -type f | head -20Repository: dimagi/commcare-android
Length of output: 558
🏁 Script executed:
# Check the English version of this string
grep -n "personalid_second_device_login_message" $(find . -name "strings.xml" -type f)Repository: dimagi/commcare-android
Length of output: 1968
🏁 Script executed:
# Look at the file mentioned in the review
cat -n app/res/values-sw/strings.xml | sed -n '485,495p'Repository: dimagi/commcare-android
Length of output: 1559
🏁 Script executed:
# Check other locale files to see how "PersonalID" is handled
grep -r "PersonalID\|Kitambulisho" $(find . -path "*/values-*/strings.xml")Repository: dimagi/commcare-android
Length of output: 17233
🏁 Script executed:
# Check other strings with multiple format placeholders in English to see the pattern
grep -n "%s.*%s\|%d.*%d" app/res/values/strings.xml | head -20Repository: dimagi/commcare-android
Length of output: 1690
🏁 Script executed:
# Check if other locales use positional format args anywhere
grep -rn "%1\$s\|%2\$s" app/res/values*/Repository: dimagi/commcare-android
Length of output: 11973
🏁 Script executed:
# Check consistency of "PersonalID" vs "Kitambulisho cha Kibinafsi" in Swahili file
grep -n "PersonalID\|Kitambulisho cha Kibinafsi" app/res/values-sw/strings.xml | grep -E "personalid_"Repository: dimagi/commcare-android
Length of output: 1249
Use positional format args and keep "PersonalID" untranslated for consistency.
This string uses non-positional placeholders (%s), but the codebase standard is positional format args (%1$s, %2$s). Additionally, "PersonalID" should remain untranslated here to match all other locales and most other strings in the Swahili file itself (only lines 491 and 528 use the translated "Kitambulisho cha Kibinafsi").
Proposed change
- <string name="personalid_second_device_login_message">Umetoka kwenye kipindi chako cha awali kwenye %s, ambacho kilitumika mara ya mwisho %s. Tafadhali kumbuka kuwa Kitambulisho cha Kibinafsi kinaweza kuingia kwenye kifaa kimoja tu kwa wakati mmoja.</string>
+ <string name="personalid_second_device_login_message">Umetoka kwenye kipindi chako cha awali kwenye %1$s, ambacho kilitumika mara ya mwisho %2$s. Tafadhali kumbuka kuwa PersonalID kinaweza kuingia kwenye kifaa kimoja tu kwa wakati mmoja.</string>🤖 Prompt for AI Agents
In `@app/res/values-sw/strings.xml` around lines 490 - 491, Update the string
resource personalid_second_device_login_message to use positional format
arguments (replace non-positional %s with %1$s and %2$s) and keep the term
"PersonalID" untranslated (replace "Kitambulisho cha Kibinafsi" with
"PersonalID") so the message matches the codebase standard and other locales;
also ensure personalid_second_device_login_title remains the same wording for
the title but verify any other occurrences of "Kitambulisho cha Kibinafsi"
(e.g., the other string entries nearby) are replaced with "PersonalID" for
consistency.
| String previousDevice = JsonExtensions.optStringSafe(json, "previous_device", null); | ||
| String lastAccessedString = JsonExtensions.optStringSafe(json, "last_accessed", null); | ||
| if(previousDevice != null && lastAccessedString != null) { | ||
| sessionData.setPreviousDevice(previousDevice); | ||
| sessionData.setLastAccessed(DateUtils.parseDate(lastAccessedString)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for DateUtils class definition
find . -type f -name "DateUtils.java" | head -20Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Also search for imports of DateUtils in the file under review
rg "DateUtils" --type java -B2 -A2 | head -50Repository: dimagi/commcare-android
Length of output: 4477
🏁 Script executed:
# Search for parseDate method implementation
rg "parseDate" --type java -A5 | head -100Repository: dimagi/commcare-android
Length of output: 10573
🏁 Script executed:
# Search for the DateUtils class definition (custom implementation)
rg "class DateUtils" --type java -A10Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Also look for the import of DateUtils in ConfirmBackupCodeResponseParser
rg "import.*DateUtils" app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.javaRepository: dimagi/commcare-android
Length of output: 114
🏁 Script executed:
# Check the full context of DateUtils usage in ConnectJobPaymentRecord to see the exception handling pattern
rg "DateUtils.parseDate" app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java -B5 -A5Repository: dimagi/commcare-android
Length of output: 899
🏁 Script executed:
# Find if DateUtils source is in the repository (javarosa library code)
find . -type f -name "DateUtils.java" | grep -i javarosaRepository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check other usages of DateUtils.parseDate to understand the pattern
rg "DateUtils\.parseDate" --type java -B2 -A2Repository: dimagi/commcare-android
Length of output: 8336
🏁 Script executed:
# Look at the ConfirmBackupCodeResponseParser file more closely
rg "ConfirmBackupCodeResponseParser" --type java -l | head -1 | xargs catRepository: dimagi/commcare-android
Length of output: 12487
🏁 Script executed:
# Search for how DateUtils.parseDate is used without try-catch in other places
rg "DateUtils\.parseDate" --type java -B3 -A3 | grep -E "(parseDate|try|catch)" -A3 -B3Repository: dimagi/commcare-android
Length of output: 10974
🏁 Script executed:
# Look at ConfirmBackupCodeResponseParser in full to understand the context
cat app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.javaRepository: dimagi/commcare-android
Length of output: 2621
🌐 Web query:
org.javarosa.core.model.utils.DateUtils parseDate method behavior
💡 Result:
Do you mean the DateUtils class in the getodk/javarosa project? If so, what specific behavior do you want to know (e.g., accepted formats, lenient vs strict parsing, null/exception on failure, timezone handling)?
🏁 Script executed:
# Look for pattern of null checks after DateUtils.parseDate calls
rg "parseDate.*null|null.*parseDate" --type java -B2 -A2Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check if there are any tests for DateUtils behavior
find . -type f -name "*DateUtils*Test*.java" -o -name "*Test*DateUtils*.java" | head -10Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Look for ParseException handling in relation to date parsing
rg "ParseException|DateParsingException" --type java -l | head -20Repository: dimagi/commcare-android
Length of output: 965
🏁 Script executed:
# Check imports and usage of DateUtils more carefully - look for method signatures
rg "import.*DateUtils|parseDate|parseDateTime" --type java -A0 | sort | uniq -c | sort -rn | head -30Repository: dimagi/commcare-android
Length of output: 3749
🏁 Script executed:
# Check if DateUtils.parseDate might return null by looking at null checks after calls
rg "DateUtils\.parseDate\(" --type java -A3 | grep -E "null|!=" | head -20Repository: dimagi/commcare-android
Length of output: 313
🏁 Script executed:
# Look for exception handling patterns around date parsing
rg "catch.*Exception" --type java -B5 | grep -B5 "parseDate\|parseDateTime" | head -40Repository: dimagi/commcare-android
Length of output: 807
🏁 Script executed:
# Check the method signature of parse() in ConfirmBackupCodeResponseParser to see if it declares exceptions
cat app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java | grep -A2 "public void parse"Repository: dimagi/commcare-android
Length of output: 278
🏁 Script executed:
# Compare exception handling between ConnectJobPaymentRecord and ConfirmBackupCodeResponseParser
echo "=== ConnectJobPaymentRecord ===" && \
rg "DateUtils\.parseDate" app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java -B3 -A5 && \
echo -e "\n=== ConfirmBackupCodeResponseParser ===" && \
rg "DateUtils\.parseDate" app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java -B3 -A5Repository: dimagi/commcare-android
Length of output: 1220
🏁 Script executed:
# Verify the null-check pattern in ConnectJobRecord more closely
rg "parseDate.*projectStartDate\|projectStartDate.*parseDate" app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java -B2 -A5Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check usage of lastAccessed field to understand downstream impact
rg "getLastAccessed\|setLastAccessed" --type java -B2 -A2Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Look more broadly at ConnectJobRecord for the null check pattern
cat app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java | grep -A2 "projectStartDate\|projectEndDate" | head -30Repository: dimagi/commcare-android
Length of output: 851
🏁 Script executed:
# Search for PersonalIdSessionData to understand lastAccessed usage
rg "PersonalIdSessionData" --type java -l | head -5 | xargs grep -l "lastAccessed"Repository: dimagi/commcare-android
Length of output: 51
🏁 Script executed:
# Check what PersonalIdSessionData looks like
find . -type f -name "PersonalIdSessionData.java"Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Look for PersonalIdSessionData class file
find . -type f -name "PersonalIdSessionData.java" -exec cat {} \;Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check if there are any null checks or validations for lastAccessed elsewhere
rg "lastAccessed" --type java -B3 -A3 | head -50Repository: dimagi/commcare-android
Length of output: 4614
Add null validation after DateUtils.parseDate() to prevent inconsistent state.
The condition checks whether lastAccessedString is non-null before parsing, but DateUtils.parseDate() can return null for invalid date formats (as evidenced by null-checks after similar calls in ConnectJobRecord). This means lastAccessed could silently be set to null while previousDevice is set, creating an inconsistent state. Wrap the DateUtils.parseDate() call in a try-catch block (similar to ConnectJobPaymentRecord) or validate the result before assignment:
Suggested fix
if(previousDevice != null && lastAccessedString != null) {
sessionData.setPreviousDevice(previousDevice);
try {
Date lastAccessedDate = DateUtils.parseDate(lastAccessedString);
if (lastAccessedDate != null) {
sessionData.setLastAccessed(lastAccessedDate);
} else {
throw new JSONException("Failed to parse lastAccessed date: " + lastAccessedString);
}
} catch (Exception e) {
throw new JSONException("Error parsing last_accessed date: " + e.getMessage());
}
}🤖 Prompt for AI Agents
In
`@app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java`
around lines 48 - 53, The parser sets sessionData.previousDevice then directly
assigns the parsed date from DateUtils.parseDate, which can return null and
leave sessionData in an inconsistent state; update
ConfirmBackupCodeResponseParser so that when previousDevice != null and
lastAccessedString != null you first call
DateUtils.parseDate(lastAccessedString) into a local Date, validate it is
non-null (and optionally catch parsing exceptions), only then call
sessionData.setLastAccessed(date); if parsing fails throw a JSONException (or
rethrow the caught exception) instead of setting a null date to ensure
consistent state between sessionData.setPreviousDevice and
sessionData.setLastAccessed.
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
looks good, left one minor non-blocking suggestion
| } | ||
|
|
||
| private void handleSecondDeviceLogin() { | ||
| if(personalIdSessionData.getPreviousDevice() != null && personalIdSessionData.getLastAccessed() != null) { |
There was a problem hiding this comment.
Should we show the notification even if lastAccessed is null
There was a problem hiding this comment.
Hmm, I guess we could but since we designed this in parallel with the server team I think it's safe to assume that we'll always get both or neither. That being the case, I'm inclined not to have to have a separate string (with translations) for that special case.
There was a problem hiding this comment.
Think what I am most concerned about here is if the lastAccessed format change due to any reason and we encounter any parsing errors, we would not end up showing the notification. I agree it would be a rare case but one we should handle in some way even if it's not optimum user experience, for example by just passing empty string instead here
There was a problem hiding this comment.
I'm not sure I understand what you mean by "format change", like the server would no longer send the date in the same format?
I noticed when grabbing the translations for the current message that the sentence can be somewhat rearranged depending on the language, so I'm worried about simply trying to include the lastAccessed part as an optional addendum vs. empty string. In other words, I don't think this will work:
String datePart = validDate ? String.format(", last used %s", date) : "";
String message = String.format("Logged out of %s%s. PersonalID can only be logged in on one device at a time.", previousDevice, datePart);
Instead, I think we'll need to create a different string entirely that doesn't include the date part to be safe.
There was a problem hiding this comment.
like the server would no longer send the date in the same format?
Yes or the parsing is wrong on mobile (given we can't test it yet). Having this date as optional will provide better backward compatibility for older mobile versions in future in case there is any change like this.
There was a problem hiding this comment.
After some discussion offline, we decided:
- No need to handle date formatting errors here, as any change to the way the server is formatting dates will have much more serious effects elsewhere and will require a larger fix.
- We should be ready to handle the server not sending a lastAccessed date at all, i.e. if we decide to make a change in the future such that we don't want to indicate to the user when the previous device had last accessed its session.
The change for the second bullet is here: 158e288
| } | ||
|
|
||
| private void handleSecondDeviceLogin() { | ||
| if(personalIdSessionData.getPreviousDevice() != null && personalIdSessionData.getLastAccessed() != null) { |
There was a problem hiding this comment.
@OrangeAndGreen Just a question here: If user forgot personalId and recover account in the same device, this notification will be shown?
There was a problem hiding this comment.
No, the server will only send this info down if the new device is different than the old one (specified here)
There was a problem hiding this comment.
How does server distinguish that it's new device?
There was a problem hiding this comment.
That's based simply on comparing the "device" string from the previous and latest session, since we don't have access to any truly unique identifier. So if a user logs in on two instances of the same device model they won't see this notification. But I think that's preferred to the opposite issue, where the user uninstalls/reinstalls the CommCare app on their device and recovers their account. When that happens we don't want to indicate that they were previously logged in to a separate device. So we can't use something like device_id.
| } | ||
|
|
||
| private void handleSecondDeviceLogin() { | ||
| if(personalIdSessionData.getPreviousDevice() != null && personalIdSessionData.getLastAccessed() != null) { |
| // identifier of the device that was previously configured for this user | ||
| var previousDevice: String? = null, | ||
| // timestamp of the last time this account was accessed from the previous device | ||
| var lastAccessed: Date? = null, |
There was a problem hiding this comment.
Kinda wish this name was different but I think it's good to keep this as-is to match server's last_accessed 👍
OrangeAndGreen
dismissed stale reviews from conroy-ricketts and shubham1g5
via
158e288
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
|
@damagatchi retest this please |
2 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
https://dimagi.atlassian.net/browse/CCCT-2027
Product Description
When the user successfully configures their PersonalID account and the server indicates that a login on another device was invalidated, a notification will be shown in the system tray indicating such and informing the user that they can only be logged in on one device at a time.
The notification appears as follows:
Technical Summary
Configuration session data holds previousDevice and lastAccessed fields that might be populated from the server response on successful recovery.
When present, the Phone fragment adds the notification.
Feature Flag
None
Safety Assurance
Safety story
Server changes are not ready yet, so dev tested by manually inserting values for previous device and last accessed date (via temporary code). The resulting notification is shown in the screenshot above.
Automated test coverage
None
QA Plan
Login to a PersonalID account that is currently logged in on a different device and has recently been accessed.
Verify that on successful login the new device shows the notification indicating that the old device was logged out.