Api call for credentials and insert data in table #3202
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThis change introduces a new API flow for retrieving personal ID credentials. It adds a Sequence Diagram(s)sequenceDiagram
participant UI/Caller
participant PersonalIdApiHandler
participant ApiPersonalId
participant ApiService
participant PersonalIdCredentialParser
participant Storage
UI/Caller->>PersonalIdApiHandler: retrieveCredentials(context, userName, password)
PersonalIdApiHandler->>ApiPersonalId: retrieveCredentials(context, userName, password, callback)
ApiPersonalId->>ApiService: retrieveCredentials(token)
ApiService-->>ApiPersonalId: Response (JSON with credentials)
ApiPersonalId-->>PersonalIdApiHandler: Callback with response
PersonalIdApiHandler->>PersonalIdCredentialParser: parse(json, sessionData)
PersonalIdCredentialParser->>Storage: Clear all credentials
PersonalIdCredentialParser->>Storage: Write valid credentials
Possibly related PRs
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: 1
🧹 Nitpick comments (4)
app/src/org/commcare/connect/network/connectId/ApiEndPoints.java (1)
19-19: Use consistent constant naming (connectCredentialsURL)All other endpoint constants follow the pattern
connectXyzURLor at least end withURL.
Sticking to the same convention prevents confusion and makes grepping easier.- public static final String credentials = "/users/credentials"; + public static final String connectCredentialsURL = "/users/credentials";app/src/org/commcare/connect/network/connectId/ApiService.java (1)
35-36: Update reference if constant is renamedIf you rename the constant per previous comment, remember to adjust the annotation here:
- @POST(ApiEndPoints.credentials) + @POST(ApiEndPoints.connectCredentialsURL)app/src/org/commcare/connect/network/ApiPersonalId.java (1)
335-340: Add null-safety guard fortokenAuthto match surrounding methodsAll other helpers (
addOrVerifyName,validateFirebaseIdToken, …) validate the generated credential string.
Doing the same here keeps the behaviour uniform and avoids a possible NPE ifHttpUtils.getCredentialunexpectedly returnsnull.- String tokenAuth = HttpUtils.getCredential(authInfo); + String tokenAuth = HttpUtils.getCredential(authInfo); + Objects.requireNonNull(tokenAuth, "Failed to create Authorization header");app/src/org/commcare/connect/network/parser/PersonalIdCredentialParser.java (1)
25-31: Batch writes inside a transaction & log corrupt entriesFor large credential sets, writing one row at a time outside a transaction can be slow.
Also, corrupt credentials are silently dropped; logging them helps troubleshooting.- storage.removeAll(); - - // Save valid credentials - for (PersonalIdCredential credential : parseResult.getValidCredentials()) { - storage.write(credential); - } + storage.removeAll(); + storage.beginTransaction(); + try { + for (PersonalIdCredential credential : parseResult.getValidCredentials()) { + storage.write(credential); + } + storage.commitTransaction(); + } finally { + storage.endTransaction(); + } + + if (!parseResult.getCorruptCredentials().isEmpty()) { + Logger.log("API Warning", + parseResult.getCorruptCredentials().size() + " corrupt credentials skipped"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/org/commcare/connect/network/ApiPersonalId.java(1 hunks)app/src/org/commcare/connect/network/PersonalIdApiHandler.java(3 hunks)app/src/org/commcare/connect/network/connectId/ApiEndPoints.java(1 hunks)app/src/org/commcare/connect/network/connectId/ApiService.java(1 hunks)app/src/org/commcare/connect/network/parser/PersonalIdCredentialParser.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
app/src/org/commcare/connect/network/connectId/ApiService.java (1)
35-36: Confirm verb & path – consider switching to@GETor supplying body
retrieveCredentialsis declared as aPOSTwith no body.
If the server is performing a read-only operation, aGET(or at least an empty @FormUrlEncoded body) would be semantically clearer.
Please double-check the backend contract.No action needed if the endpoint explicitly expects
POST /users/credentialswith no payload.app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
163-167:sessionDatacreated here is empty – verify consumer expectations
retrieveCredentialsinstantiates a freshPersonalIdSessionData, but the credential parser doesn’t populate it, so the object passed toonSuccess()carries no useful state.
If downstream code expects tokens or flags insidesessionData, this will lead to NPEs or logic errors.Please ensure either
a) the parser populatessessionData, or
b) the handler’s consumers do not assume anything inside it.app/src/org/commcare/connect/network/ApiPersonalId.java (1)
335-335: Constructor overload differs from existing callsElsewhere in the file you call
new AuthInfo.ProvidedAuth(user, pass, false).
Here the 2-arg overload is used. If the third parameter controls Base64 wrapping or other security behaviour, the call might deviate from expectations.Double-check the correct overload (or add a clarifying comment).
app/src/org/commcare/connect/network/parser/PersonalIdCredentialParser.java
Outdated
Show resolved
Hide resolved
| callApi(context, call, callback); | ||
| } | ||
|
|
||
| public static void retrieveCredentials(Context context, String userName, String password, IApiCallback callback) { |
There was a problem hiding this comment.
naming is not clear to me
There was a problem hiding this comment.
for me its sounds like incomplete name retriveCredentials of what
There was a problem hiding this comment.
i accept we are referring everywhere as credential only but it sounds incomplete
There was a problem hiding this comment.
i accept we are referring everywhere as credential only but it sounds incomplete
| public static final String connectClaimJobURL = "https://%s/api/opportunity/%d/claim"; | ||
| public static final String connectDeliveriesURL = "https://%s/api/opportunity/%d/delivery_progress"; | ||
| public static final String connectPaymentConfirmationURL = "https://%s/api/payment/%s/confirm"; | ||
| public static final String credentials = "/users/credentials"; |
There was a problem hiding this comment.
variable name should reflect the apiendpoint name
There was a problem hiding this comment.
this should use Java Const formatting like CREDENTIALS (would be nice to correct other constants in this class as well in a different PR)
There was a problem hiding this comment.
@pm-dimagi I don't get your comment, it seems to be following the right naming as per the API endpoint.
There was a problem hiding this comment.
may be i read it something wrong
app/src/org/commcare/connect/network/parser/PersonalIdCredentialParser.java
Outdated
Show resolved
Hide resolved
| // Get storage for this model | ||
| SqlStorage<PersonalIdCredential> storage = | ||
| CommCareApplication.instance().getUserStorage(PersonalIdCredential.class); | ||
|
|
There was a problem hiding this comment.
we can also create the wokerhistoryDb helper class which will perform all the datatbase related queries from there like ConnectUserDatabaseUtil
There was a problem hiding this comment.
@pm-dimagi SqlStorage is also a helper class why to create extra filtration for it as it is used only on this place
There was a problem hiding this comment.
@jaypanchal-13 Even I am not sure but putting it here so others (Shubham/Dave) can comment.
Better/mandatory to use?
CommCareApplication.instance().getUserStorage(PersonalIdCredential.STORAGE_KEY,PersonalIdCredential.class)
| import org.json.JSONException; | ||
| import org.json.JSONObject; | ||
|
|
||
| public class PersonalIdCredentialParser implements PersonalIdApiResponseParser { |
There was a problem hiding this comment.
@jaypanchal-13 I don't see any reason to implement PersonalIdApiResponseParser. Now, there is no use of PersonalIdSessionData.
There was a problem hiding this comment.
Agree but think we should have a more generic interface for parsers independent of PersonalIdApiResponseParser
| // Get storage for this model | ||
| SqlStorage<PersonalIdCredential> storage = | ||
| CommCareApplication.instance().getUserStorage(PersonalIdCredential.class); | ||
|
|
There was a problem hiding this comment.
@jaypanchal-13 Even I am not sure but putting it here so others (Shubham/Dave) can comment.
Better/mandatory to use?
CommCareApplication.instance().getUserStorage(PersonalIdCredential.STORAGE_KEY,PersonalIdCredential.class)
|
|
||
| public void retrieveCredentials(Context context, String userName, String password) { | ||
| PersonalIdSessionData sessionData = new PersonalIdSessionData(); | ||
| ApiPersonalId.retrieveCredentials(context, userName, password, createCallback(sessionData, |
There was a problem hiding this comment.
@jaypanchal-13 createCallback is instantiating IApiCallback which in turn is sending sessionData on success. I don't think we need sessionData this time. We might need to create some generic ApiCallback which in turn will render the required response.
There was a problem hiding this comment.
@Jignesh-dimagi correct instead of creating new callback can we update sessionData nullable in createCallback?
@shubham1g5
There was a problem hiding this comment.
we probably should have 2 separate classes here abstracting from one another PersonaIdApiCallback and PersonaIdSessionDataCallback: PersonaIdApiCallback
…credential-api-call
| @Override | ||
| public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException { | ||
| // Parse credentials array from response | ||
| JSONArray credentialsArray = json.getJSONArray("credentials"); |
There was a problem hiding this comment.
this might not be present if there are no credentials and we should handle using optJsonArray
|
|
||
| // Get storage for this model | ||
| SqlStorage<PersonalIdCredential> storage = | ||
| CommCareApplication.instance().getUserStorage(PersonalIdCredential.class); |
There was a problem hiding this comment.
PersonalIdCredential is stored in Personal ID database and user storage looks in CommCare user DB, look at how we are writing to other tables in Personal ID database to see the right way to do this.
There was a problem hiding this comment.
@shubham1g5 updated this to store in Connect db
-Solve comments
Product Description
Api call for credentials and insert data in credential table
ticket -> https://dimagi.atlassian.net/browse/CCCT-1337
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review