Pm Device configuration api #3117
Conversation
📝 WalkthroughWalkthroughThis update introduces a new device configuration flow for PersonalID registration. A singleton class, Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant PersonalIdApiHandler
participant ApiPersonalId
participant ApiService
participant DeviceConfigurationData
participant PersonalIdMessageFragment
User->>PersonalIdPhoneFragment: Clicks "Continue"
PersonalIdPhoneFragment->>PersonalIdApiHandler: makeConfigurationCall(activity, phone)
PersonalIdApiHandler->>ApiPersonalId: startConfiguration(context, phone, callback)
ApiPersonalId->>ApiService: startConfiguration({phone_number})
ApiService-->>ApiPersonalId: API response (success or failure)
ApiPersonalId-->>PersonalIdApiHandler: callback (success/failure)
alt Success
PersonalIdApiHandler->>DeviceConfigurationData: Update state from response
PersonalIdApiHandler->>PersonalIdPhoneFragment: onSuccess()
alt Token present
PersonalIdPhoneFragment->>PersonalIdPhoneFragment: navigateToBiometricSetup()
else Token missing
PersonalIdPhoneFragment->>PersonalIdPhoneFragment: navigateToMessageDisplay()
end
else Failure
PersonalIdApiHandler->>PersonalIdPhoneFragment: onFailure()
PersonalIdPhoneFragment->>PersonalIdPhoneFragment: navigateToMessageDisplay()
end
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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: 3
🧹 Nitpick comments (4)
app/src/org/commcare/android/database/connect/models/DeviceConfigurationData.java (2)
125-127: Ensure thread-safe clearing of singleton.
Theclear()method resetsinstancewithout synchronization, which can race withgetInstance(). Consider makingclear()synchronized or markinginstanceasvolatile.
9-13: Consider using an enum forrequiredLock.
CurrentlyrequiredLockis a rawString("device" or "biometric"), which may lead to invalid values. Defining anenumwould enforce valid states and improve type safety.app/src/org/commcare/connect/network/connectId/ApiService.java (2)
12-13: Consider renaming endpoint constant for clarity.
The methodstartConfigurationstill usesApiEndPoints.registerUser, which can be confusing. Renaming the constant (e.g.startConfiguration) or updating its value will improve maintainability.
13-13: Use a typed request object instead of a rawMap.
Defining aStartConfigurationRequestDTO would enforce compile-time checks, document the payload fields, and improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/res/navigation/nav_graph_personalid.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/android/database/connect/models/DeviceConfigurationData.java(1 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/connect/network/ApiPersonalId.java(1 hunks)app/src/org/commcare/connect/network/PersonalIdApiHandler.java(1 hunks)app/src/org/commcare/connect/network/connectId/ApiService.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-61)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (11)
app/src/org/commcare/connect/ConnectConstants.java (1)
60-60: Added new constant for device configuration failure.
The offset+16is contiguous with the existing task IDs and correctly represents the failure state.app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (4)
136-138: Good addition of device configuration failure handling.The new case properly handles the device configuration failure scenario by navigating back to the phone input fragment, allowing users to retry.
147-151: LGTM!The formatting change for line breaks in the method signature improves readability without changing functionality.
158-159: LGTM!The formatting adjustment for consistent spacing in the return statement is a good practice.
161-163: Appropriate navigation method implementation.The new method provides a clean way to navigate back to the phone fragment from the message display when device configuration fails.
app/src/org/commcare/connect/network/ApiPersonalId.java (1)
288-294: Good API simplification for device configuration.The method has been appropriately renamed from
registerUsertostartConfigurationand simplified to only require phone number input, which better reflects its purpose in the new device configuration flow.The refactoring aligns well with the PR objective of integrating the device_configuration API into the personal ID workflow.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (4)
15-17: Appropriate imports for the new device configuration flow.These imports support the integration of the device configuration API into the personal ID workflow.
151-168: Good implementation of device configuration API call.The
onContinueClicked()method has been properly refactored to use the newPersonalIdApiHandlerto make the configuration call asynchronously instead of immediately navigating forward.Note: Both success and failure callbacks call the same
navigateForward()method, which is acceptable since navigation will be determined by the presence or absence of a device token in the updatedDeviceConfigurationDatasingleton.
171-181: Good navigation logic based on device configuration state.The
navigateForward()method correctly determines where to navigate based on whether a device token was successfully obtained during configuration.
187-193: Appropriate error message display implementation.The method properly constructs a navigation action to display an error message when device configuration fails, using appropriate string resources and setting the correct calling class constant.
app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
1-81: Well-designed API handler for device configuration.This new abstract class provides a clean implementation for making the device configuration API call and handling the response. The class:
- Properly updates the
DeviceConfigurationDatasingleton with configuration details from the response- Handles various error scenarios appropriately
- Uses abstract methods to allow implementing classes to define specific success and failure behaviors
One minor suggestion for future enhancement: Consider adding specific error state information to
DeviceConfigurationDatain the exception handling block (lines 44-47) to provide more context to implementing classes about why a failure occurred.
| @Override | ||
| public String toString() { | ||
| return "DeviceConfigurationData{" + | ||
| "requiredLock='" + requiredLock + '\'' + | ||
| ", demoUser=" + demoUser + | ||
| ", token='" + token + '\'' + | ||
| ", failureCode='" + failureCode + '\'' + | ||
| ", failureSubcode='" + failureSubcode + '\'' + | ||
| ", accountExists=" + accountExists + | ||
| ", photoBase64='" + photoBase64 + '\'' + | ||
| ", username='" + username + '\'' + | ||
| ", dbKey='" + dbKey + '\'' + | ||
| ", oauthPassword='" + oauthPassword + '\'' + | ||
| ", accountOrphaned=" + accountOrphaned + | ||
| '}'; | ||
| } |
There was a problem hiding this comment.
Remove sensitive fields from toString().
Including token, oauthPassword, and dbKey in the debug output risks leaking credentials/PII. Exclude or mask these fields.
Proposed diff:
@@ @Override
public String toString() {
- return "DeviceConfigurationData{" +
- "requiredLock='" + requiredLock + '\'' +
- ", demoUser=" + demoUser +
- ", token='" + token + '\'' +
- ", failureCode='" + failureCode + '\'' +
- ", failureSubcode='" + failureSubcode + '\'' +
- ", accountExists=" + accountExists +
- ", photoBase64='" + photoBase64 + '\'' +
- ", username='" + username + '\'' +
- ", dbKey='" + dbKey + '\'' +
- ", oauthPassword='" + oauthPassword + '\'' +
- ", accountOrphaned=" + accountOrphaned +
- '}';
+ return "DeviceConfigurationData{" +
+ "requiredLock='" + requiredLock + '\'' +
+ ", demoUser=" + demoUser +
+ ", failureCode='" + failureCode + '\'' +
+ ", failureSubcode='" + failureSubcode + '\'' +
+ ", accountExists=" + accountExists +
+ ", photoBase64='" + photoBase64 + '\'' +
+ ", username='" + username + '\'' +
+ // Sensitive fields (token, dbKey, oauthPassword) have been removed
+ ", accountOrphaned=" + accountOrphaned +
+ '}';📝 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 "DeviceConfigurationData{" + | |
| "requiredLock='" + requiredLock + '\'' + | |
| ", demoUser=" + demoUser + | |
| ", token='" + token + '\'' + | |
| ", failureCode='" + failureCode + '\'' + | |
| ", failureSubcode='" + failureSubcode + '\'' + | |
| ", accountExists=" + accountExists + | |
| ", photoBase64='" + photoBase64 + '\'' + | |
| ", username='" + username + '\'' + | |
| ", dbKey='" + dbKey + '\'' + | |
| ", oauthPassword='" + oauthPassword + '\'' + | |
| ", accountOrphaned=" + accountOrphaned + | |
| '}'; | |
| } | |
| @Override | |
| public String toString() { | |
| return "DeviceConfigurationData{" + | |
| "requiredLock='" + requiredLock + '\'' + | |
| ", demoUser=" + demoUser + | |
| ", failureCode='" + failureCode + '\'' + | |
| ", failureSubcode='" + failureSubcode + '\'' + | |
| ", accountExists=" + accountExists + | |
| ", photoBase64='" + photoBase64 + '\'' + | |
| ", username='" + username + '\'' + | |
| // Sensitive fields (token, dbKey, oauthPassword) have been removed | |
| ", accountOrphaned=" + accountOrphaned + | |
| '}'; | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/android/database/connect/models/DeviceConfigurationData.java
between lines 130 and 145, the toString() method currently includes sensitive
fields such as token, oauthPassword, and dbKey, which risks exposing
credentials. Modify the toString() method to exclude these fields or replace
their values with masked placeholders to prevent leaking sensitive information
in logs or debug output.
| <action | ||
| android:id="@+id/action_personalid_message_display_to_personalid_phone_fragment" | ||
| app:destination="@id/personalid_phone_fragment" /> | ||
| </dialog> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add popUpTo attributes to clear back stack.
Other actions under personalid_message_display include app:popUpTo (and app:popUpToInclusive). Without them, this navigation may leave the dialog in the back stack. Apply this diff:
<action
android:id="@+id/action_personalid_message_display_to_personalid_phone_fragment"
+ app:popUpTo="@id/personalid_message_display"
+ app:popUpToInclusive="true"
app:destination="@id/personalid_phone_fragment" />📝 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.
| <action | |
| android:id="@+id/action_personalid_message_display_to_personalid_phone_fragment" | |
| app:destination="@id/personalid_phone_fragment" /> | |
| </dialog> | |
| <action | |
| android:id="@+id/action_personalid_message_display_to_personalid_phone_fragment" | |
| app:popUpTo="@id/personalid_message_display" | |
| app:popUpToInclusive="true" | |
| app:destination="@id/personalid_phone_fragment" /> | |
| </dialog> |
🤖 Prompt for AI Agents
In app/res/navigation/nav_graph_personalid.xml around lines 95 to 98, the
navigation action lacks app:popUpTo and app:popUpToInclusive attributes, which
can cause the dialog to remain in the back stack. Add
app:popUpTo="@id/personalid_message_display" and app:popUpToInclusive="true" to
the action element to clear the back stack properly when navigating.
app/res/values/strings.xml
Outdated
| <string name="configuration_process_failed_title">Process Failed</string> | ||
| <string name="configuration_process_failed_subtitle">"Your device isn't eligible to sign up for PersonalID at this time. Please try again on a different device."</string> |
There was a problem hiding this comment.
Remove extraneous quotation marks in string resource.
The configuration_process_failed_subtitle value currently includes literal double quotes which will appear in the UI. Update as follows:
-<string name="configuration_process_failed_subtitle">"Your device isn't eligible to sign up for PersonalID at this time. Please try again on a different device."</string>
+<string name="configuration_process_failed_subtitle">Your device isn't eligible to sign up for PersonalID at this time. Please try again on a different device.</string>🤖 Prompt for AI Agents
In app/res/values/strings.xml around lines 912 to 913, the string resource
configuration_process_failed_subtitle contains extraneous double quotes that
will display in the UI. Remove the surrounding double quotes from the string
value so it reads as a normal sentence without literal quotation marks.
|
47a7f55 @coderabbitai please see this commit to your resolution |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 90231 @pm-dimagi Thanks for sharing the commit reference. I can see that this commit has added a substantial number of files to the repository - it appears to be adding hundreds of files including source code, resources, and configuration files. However, from the information available, I can't determine the specific changes made to address any previous review comments, as I only see file additions without the content modifications. Could you please provide more details about what specific review feedback was addressed in this commit? This would help me understand how the changes relate to the device configuration API integration for the personal ID workflow. |
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="100dp" |
There was a problem hiding this comment.
we should abstract all hard coded sizes in res/values/dimens.xml instead like personalid_logo_height and reuse it on all pages.
| @@ -0,0 +1,128 @@ | |||
| package org.commcare.android.database.connect.models; | |||
|
|
|||
| public class DeviceConfigurationData { | |||
There was a problem hiding this comment.
better to call it PerosnalIdSessionData instead, I want to emphasis that this data is only lived for the length of a user session and it's tied to the PersonalId classes we have.
| private String failureCode; | ||
| private String failureSubcode; |
There was a problem hiding this comment.
names should depict what 'failures' are these codes for.
| private static DeviceConfigurationData instance; | ||
|
|
||
| // Fields from various payloads | ||
| private String requiredLock; // "device" or "biometric" |
There was a problem hiding this comment.
can we use @stringdef to denote acceptable values instead.
| @@ -0,0 +1,128 @@ | |||
| package org.commcare.android.database.connect.models; | |||
|
|
|||
| public class DeviceConfigurationData { | |||
There was a problem hiding this comment.
would be nice to use Kotlin data class here as that will get rid of most of the code in this class
| // Optional: Clear all data | ||
| public void clear() { | ||
| instance = null; | ||
| } |
There was a problem hiding this comment.
I kept this to clear the data on start and end of the each personalid creation or recovery flow to be clear so that no value is overridden
| JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(responseData))); | ||
| DeviceConfigurationData deviceData = DeviceConfigurationData.getInstance(); | ||
|
|
||
| if (json.has("required_lock")) { | ||
| deviceData.setRequiredLock(json.getString("required_lock")); | ||
| } | ||
| if (json.has("demo_user")) { | ||
| deviceData.setDemoUser(json.getBoolean("demo_user")); | ||
| } | ||
| if (json.has("token")) { | ||
| deviceData.setToken(json.getString("token")); | ||
| } | ||
| if (json.has("failure_code")) { | ||
| Logger.log(LogTypes.TYPE_USER, json.getString("failure_code")); | ||
| deviceData.setFailureCode(json.getString("failure_code")); | ||
| } | ||
| if (json.has("failure_subcode")) { | ||
| deviceData.setFailureSubcode(json.getString("failure_subcode")); | ||
| } | ||
|
|
There was a problem hiding this comment.
abstract all Json parsing logic in a separate parser class say org.commcare.connect.network.StartConfigutationResponseParser . This will allow us to unit test parsers separately in future.
|
|
||
| @Override | ||
| protected void onFailure() { | ||
| navigateForward(); |
There was a problem hiding this comment.
we should split logic for success and failures into different methods instead of using the same method to do both.
| public static synchronized DeviceConfigurationData getInstance() { | ||
| if (instance == null) { | ||
| instance = new DeviceConfigurationData(); | ||
| } | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
this should be accessed through a view model instead of being a singleton -
public class PersonalIdSessionDataViewModel extends ViewModel {
private PersonalIdSessionData personalIdSessionData = new PersonalIdSessionData()
public PersonalIdSessionData getPersonalIdSessionData() {
return personalIdSessionData;
}
public void setPersonalIdSessionData() {
return personalIdSessionData;
}
}
And then you can access this in your fragment as -
viewModel = new ViewModelProvider(requireActivity()).get(PersonalIdSessionDataViewModel.class);
viewModel.getPersonalIdSessionData();
…me it to PersonalidSessiondata
| android:id="@+id/company_logo" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_width="match_parent" |
There was a problem hiding this comment.
Curious why we are using match_parent for width here when we seem to be using wrap_content in other places ?
|
|
||
| import androidx.annotation.StringDef | ||
|
|
||
| class PersonalIdSessionData // Private constructor |
There was a problem hiding this comment.
can we use a data class for this ?
| // Optional: Clear all data | ||
| fun clear() { | ||
| instance = null | ||
| } |
There was a problem hiding this comment.
should not be needed as it's tied to the view model scoped to the activity itself i.e. it will destroy by itself with activity getting destroyed.
| private constructor() { | ||
| @StringDef(*[DEVICE_TYPE, BIOMETRIC_TYPE]) | ||
| @Retention(AnnotationRetention.SOURCE) | ||
| annotation class acceptedPinType |
There was a problem hiding this comment.
thoughts on calling it deviceAuthType ?
| * @param json The JSON object returned from the API | ||
| * @throws JSONException if expected keys are missing or malformed | ||
| */ | ||
| public static void parse(JSONObject json) throws JSONException { |
There was a problem hiding this comment.
Make it instance based instead of static i.e. someone should call this as new StartConfigurationResponseParser(response).parse(). In general we should avoid static methods unless there is a really good fit/usecase for it.
|
|
||
| import androidx.annotation.StringDef | ||
|
|
||
| class PersonalIdSessionData { |
There was a problem hiding this comment.
can we add a java doc here.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
|
|
||
| public abstract class PersonalIdApiHandler { | ||
|
|
||
| public void makeConfigurationCall(Activity activity, String phone, PersonalIdSessionData sessionData) { |
There was a problem hiding this comment.
think sessionData at this point is always empty, we should therefore remove it from here and construct the new object in processSuccess here and return it with the callback.
There was a problem hiding this comment.
there is no use session data here now
| import androidx.lifecycle.ViewModel; | ||
|
|
||
| public class PersonalIdSessionDataViewModel extends ViewModel { | ||
| private final PersonalIdSessionData personalIdSessionData = new PersonalIdSessionData(); |
There was a problem hiding this comment.
instead of initing this in view model, we should just have a setter method for this that fragment can call to set this up.
| import androidx.annotation.StringDef | ||
|
|
||
| /** | ||
| * Data holder for personal identification session state during Connect ID flows. |
There was a problem hiding this comment.
nit: remove during Connect ID flows as that's already conveyed by personal identification session state. Or atlease rename it to say Personal ID instead.
|
|
||
| private void navigateSucesss() { | ||
| NavDirections directions = null; | ||
| if (viewModel.getPersonalIdSessionData().getToken() != null) { |
There was a problem hiding this comment.
viewModel.getPersonalIdSessionData() seems always empty in the current implementaion
There was a problem hiding this comment.
this will not set the view model is this wrong
There was a problem hiding this comment.
@pm-dimagi Seems like we are not storing after parsing as mentioned here
| if (json.has("required_lock")) { | ||
| sessionData.setRequiredLock(json.getString("required_lock")); | ||
| } | ||
|
|
||
| if (json.has("demo_user")) { | ||
| sessionData.setDemoUser(json.getBoolean("demo_user")); | ||
| } | ||
|
|
||
| if (json.has("token")) { | ||
| sessionData.setToken(json.getString("token")); | ||
| } | ||
|
|
||
| if (json.has("failure_code")) { | ||
| String failureCode = json.getString("failure_code"); | ||
| Logger.log(LogTypes.TYPE_USER, failureCode); | ||
| sessionData.setSessionFailureCode(failureCode); | ||
| } | ||
|
|
||
| if (json.has("failure_subcode")) { | ||
| sessionData.setSessionFailureSubcode(json.getString("failure_subcode")); |
There was a problem hiding this comment.
use the Json extensions you created instead of has + get . Use optBoolean for booleans.
| protected void onFailure() { | ||
| navigateFailure(); | ||
| } | ||
| }.makeConfigurationCall(requireActivity(), phone); |
There was a problem hiding this comment.
rename to makeStartConfigurationCall
…esponse after success
| android:id="@+id/company_logo" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="40dp" | ||
| android:layout_height="@dimen/personalid_logo_height" |
There was a problem hiding this comment.
@pm-dimagi Giving fixed height with wrap_content width will distort the aspect ratio, instead please use wrap content for height also.
There was a problem hiding this comment.
if i am using wrap height it is covering whole screen that is the issue
There was a problem hiding this comment.
Each commcare_connect_logo.png size is in MBs. This seems too much as it will increase APK size by a lot. We can definitely downgrade it or think of converting this to vector drawables.
There was a problem hiding this comment.
@pm-dimagi Also, if app uses wrap_content for both height and width, it should have height as per the width (full screen) and image being in landscape, should not cover whole screen, thoughts?
| android:id="@+id/company_logo" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_height="@dimen/personalid_logo_height" |
There was a problem hiding this comment.
@pm-dimagi Same here, giving fixed height with wrap_content width will distort the aspect ratio, instead please use wrap content for height also.
There was a problem hiding this comment.
if i am using wrap height it is covering whole screen that is the issue
| android:id="@+id/company_logo" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="40dp" | ||
| android:layout_height="@dimen/personalid_logo_height" |
There was a problem hiding this comment.
@pm-dimagi Same here, giving fixed height with wrap_content width will distort the aspect ratio, instead please use wrap content for height also.
There was a problem hiding this comment.
if i am using wrap height it is covering whole screen that is the issue
| android:id="@+id/company_logo" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_height="@dimen/personalid_logo_height" |
There was a problem hiding this comment.
@pm-dimagi Same here, giving fixed height with wrap_content width will distort the aspect ratio, instead please use wrap content for height also.
There was a problem hiding this comment.
if i am using wrap height it is covering whole screen that is the issue
| public void processSuccess(int responseCode, InputStream responseData) { | ||
| try { | ||
| JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(responseData))); | ||
| PersonalIdSessionData sessionData = new PersonalIdSessionData(); |
There was a problem hiding this comment.
@pm-dimagi This is instantiated here, but where it is stored for further use?
| public void processNetworkFailure() { | ||
| ConnectNetworkHelper.showNetworkError(activity); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenUnavailableError() { | ||
| ConnectNetworkHelper.handleTokenUnavailableException(activity); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenRequestDeniedError() { | ||
| ConnectNetworkHelper.handleTokenDeniedException(activity); | ||
| } | ||
|
|
||
| @Override | ||
| public void processOldApiError() { | ||
| ConnectNetworkHelper.showOutdatedApiError(activity); |
There was a problem hiding this comment.
no because these are general call which tells outdated version , token issue so that can be handled centrally no use of re writting it again because of this i have not retuned onFailure from all places
There was a problem hiding this comment.
@pm-dimagi Yeah but if there is some progress bar being shown, we need to get rid of it by calling onFailure
There was a problem hiding this comment.
+1 on this, think onFailure() should take an error code like NETWORK_ERROR , OLD_API_ERROR and we should do the error handling for it inside the onFailure itself depending on the error code (best to implement as Kotlin sealed class)
There was a problem hiding this comment.
Actually taking back sealed class suggestion for now as it might be too many changes and maybe incompatible with some other constructs we have it currently.
| onFailure(); | ||
| } finally { | ||
| try { | ||
| if (responseData != null) { |
There was a problem hiding this comment.
@pm-dimagi Instead, use in first try -> try (InputStream in = responseData), this will close the stream
| * Parses a JSON response from the start configuration API call | ||
| * and populates a PersonalIdSessionData instance. | ||
| */ | ||
| public class StartConfigurationResponseParser { |
There was a problem hiding this comment.
@pm-dimagi instead you can create kotlin object StartConfigurationResponseParser{..} and define function inside with 1 parameters JSONObject json and its return type should PersonalIdSessionData. You can also call this class name as StartConfigurationResponseMapper.
There was a problem hiding this comment.
So basically ask overhere is to convert the code in the kotlin right?
As this class takes 1 object json and parse it to sessionData model which is centrally available so i think there is no need it to return from here
There was a problem hiding this comment.
@pm-dimagi Sorry I didn't get you when you say centrally available?
| viewModel = new ViewModelProvider(requireActivity()).get(PersonalIdSessionDataViewModel.class); | ||
| initializeUi(); | ||
| PersonalIdSessionData personalIdSessionData = new PersonalIdSessionData(); | ||
| viewModel.setPersonalIdSessionData(personalIdSessionData); |
There was a problem hiding this comment.
@pm-dimagi ViewModel can have function called resetPersonalIdSessionData which will instantiate new PersonalIdSessionData() and will reset the information
There was a problem hiding this comment.
The scope of the viewmodel is till the activity last so ithink there is no need of that it will get destroyed with the activity
There was a problem hiding this comment.
@pm-dimagi So to initialize the personalIdSessionData, use init method of view-model instead from calling from PersonalIdPhoneFragment
| private boolean shouldShowPhoneHintDialog = true; | ||
| private PhoneNumberHelper phoneNumberHelper; | ||
| private Activity activity; | ||
| private PersonalIdSessionDataViewModel viewModel; |
There was a problem hiding this comment.
rename appropriately to personalIdSessionDataViewModel
|
|
||
| private void onConfigurationFailure() { | ||
| Logger.log(LogTypes.TYPE_USER, viewModel.getPersonalIdSessionData().getSessionFailureCode()); | ||
| Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToMessageDisplay()); |
There was a problem hiding this comment.
think this message should be non-cancellable or we should just finish app and exit if user click ok on the message screen.
| private void navigateFailure() { | ||
| Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToMessageDisplay()); | ||
| } |
There was a problem hiding this comment.
the failure messaging here can't be same as the failure messaging for onConfigurationFailure. For onConfigurationFailure, it's a permanenet (ineligible for personal ID ) error that user should not be able to exit from and retry the whole process again. While the navigateFailure is a network error that should not really navigate to the pop up screen but show user an error on the same page with an option to retry.
| public void processNetworkFailure() { | ||
| ConnectNetworkHelper.showNetworkError(activity); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenUnavailableError() { | ||
| ConnectNetworkHelper.handleTokenUnavailableException(activity); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenRequestDeniedError() { | ||
| ConnectNetworkHelper.handleTokenDeniedException(activity); | ||
| } | ||
|
|
||
| @Override | ||
| public void processOldApiError() { | ||
| ConnectNetworkHelper.showOutdatedApiError(activity); |
There was a problem hiding this comment.
+1 on this, think onFailure() should take an error code like NETWORK_ERROR , OLD_API_ERROR and we should do the error handling for it inside the onFailure itself depending on the error code (best to implement as Kotlin sealed class)
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
left one change, but things look good otherwise.
| public static final int NETWORK_ERROR = 101; | ||
| public static final int OLD_API_ERROR = 102; | ||
| public static final int TOKEN_UNAVAILABLE_ERROR = 103; | ||
| public static final int TOKEN_DENIED_ERROR = 104; | ||
| public static final int API_ERROR = 105; | ||
| public static final int JSON_PARSING_ERROR = 106; |
There was a problem hiding this comment.
can we convert this into an enum say PersonalIDApiError , similar to this
There was a problem hiding this comment.
6b4b0e5
Introduced the enum but not put the value code as i think there is no need of that
|
|
||
| @Override | ||
| public void processFailure(int responseCode) { | ||
| onFailure(ConnectConstants.PersonalIdApiErrorCodes.API_ERROR); |
There was a problem hiding this comment.
Think this needs to be further classified based on response code or io exception. But can we call this INVALID_RESPONSE_ERROR for now to depict it's either related to invalid response or a non 200 status code.
There was a problem hiding this comment.
I am distiguishing it JSON_PARSING_ERROR,API_ERROR
There was a problem hiding this comment.
API_ERROR although seems too generic and can mean a lot of things like bad url
| Navigation.findNavController(binding.personalidPhoneContinueButton).navigate( | ||
| navigateToMessageDisplay(getString(R.string.configuration_process_api_failed), true)); |
There was a problem hiding this comment.
This should not show the message screen here but just display the error in-place on the same fragment and let user retry the process again. We only want to show the message screen to highlight very important steps and not for common network errors.
Product Description
Integrated the device_cofiguration api for the personal id wrokflow
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1220
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review