Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pm retrofit #2890

Open
wants to merge 8 commits into
base: connect_qa
Choose a base branch
from
Open

Pm retrofit #2890

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a6efad0
-retrofit implementation
pm-dimagi Oct 23, 2024
e9832b4
-retrofit implementation
pm-dimagi Nov 6, 2024
93b6467
Merge branch 'connect_qa' of https://github.com/dimagi/commcare-andro…
OrangeAndGreen Nov 18, 2024
ec9d3ba
Merge branch 'connect_qa' of https://github.com/dimagi/commcare-andro…
OrangeAndGreen Jan 2, 2025
e146111
Cleaned up some obsolete strings, and fixed a path with a double slash
OrangeAndGreen Jan 2, 2025
b3cea59
Removed unused imports
OrangeAndGreen Jan 2, 2025
e369698
Fixed a couple compile errors after merge.
OrangeAndGreen Jan 4, 2025
84546a2
Merge branch 'connect_qa' into pm_retrofit
OrangeAndGreen Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ dependencies {
implementation 'androidx.sqlite:sqlite:2.2.0'
implementation 'com.google.android.gms:play-services-auth:21.2.0'
implementation 'com.google.android.gms:play-services-auth-api-phone:18.1.0'
implementation 'com.squareup.retrofit2:retrofit:2.9.0'
implementation 'com.squareup.okhttp3:logging-interceptor:4.11.0'
implementation 'com.squareup.retrofit2:converter-gson:2.9.0'
Comment on lines +90 to +92
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Android SDK 35 is not yet released, update required

The verification confirms that compileSdk 35 is indeed used in both app/build.gradle and commcare-support-library/build.gradle. As of January 2025, Android 14 (API 34) is the latest stable release, making SDK 35 an invalid configuration that will cause build failures.

  • app/build.gradle: Line containing compileSdk 35
  • commcare-support-library/build.gradle: Line containing compileSdk 35

Both files need to be updated to use compileSdk 34 to ensure compatibility with the latest stable Android SDK.

🔗 Analysis chain

Verify Android SDK compatibility.

The project is configured to use compileSdk 35, but Android 14 (API 34) is the latest release. This might cause build issues or unexpected behavior.

Consider updating the compileSdk version in the build configuration:

android {
-    compileSdk 35
+    compileSdk 34
}

Let me verify the Android SDK version configuration in the build files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found

Script:

#!/bin/bash
# Search for compileSdk configuration in Gradle build files
rg "compileSdk" -A 2 --type gradle

Length of output: 302

💡 Codebase verification

⚠️ Logging interceptor needs to be configured for debug builds only

The logging interceptor is currently unconditionally enabled with BODY level logging, which could expose sensitive information in production. The implementation in app/src/org/commcare/network/connectId/network/ApiClient.java needs to be updated to enable logging only in debug builds.

  • Update ApiClient.java to conditionally enable logging:
HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
if (BuildConfig.DEBUG) {
    logging.setLevel(HttpLoggingInterceptor.Level.BODY);
} else {
    logging.setLevel(HttpLoggingInterceptor.Level.NONE);
}
🔗 Analysis chain

LGTM with a security consideration.

The Retrofit dependencies are appropriate for implementing type-safe HTTP client functionality. However, ensure that the OkHttp logging interceptor is only enabled in debug builds to prevent potential sensitive information leakage in production.

Run this script to verify if logging interceptor configuration exists in the codebase:

If no configuration is found, consider adding a debug-only configuration for the logging interceptor.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if logging interceptor is properly configured for debug/release builds
rg -l "LoggingInterceptor"

Length of output: 88

Script:

#!/bin/bash
# Check the implementation of LoggingInterceptor in ApiClient.java
rg -A 5 "LoggingInterceptor" app/src/org/commcare/network/connectId/network/ApiClient.java

Length of output: 655

Script:

#!/bin/bash
# Check if there's any build variant or debug configuration check before enabling logging
rg -B 5 -A 2 "HttpLoggingInterceptor" app/src/org/commcare/network/connectId/network/ApiClient.java

Length of output: 800

implementation 'com.google.android.gms:play-services-identity:18.1.0'
implementation 'com.google.android.gms:play-services-base:18.5.0'

Expand Down
24 changes: 2 additions & 22 deletions app/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,40 +8,20 @@
<string name="title_bar_name">@string/application_name</string>
<!-- string name="PostURL">https://pact.dimagi.com/receiver/submit/pact</string -->
<!-- string name="ota_restore_url">https://pact.dimagi.com/provider/caselist</string -->

<string name="default_app_server">jr://asset/testing/profile.ccpr</string>
<string name="PostURL">https://staging.commcarehq.org/receiver/submit/pf</string>
<string name="ota_restore_url">https://staging.commcarehq.org/ota_restore</string>

<string name="ConnectTokenURL">https://connectid.dimagi.com/o/token/</string>
<string name="ConnectHeartbeatURL">https://connectid.dimagi.com/users/heartbeat</string>
<string name="ConnectFetchDbKeyURL">https://connectid.dimagi.com/users/fetch_db_key</string>
<string name="ConnectChangePasswordURL">https://connectid.dimagi.com/users/change_password</string>
<string name="ConnectResetPasswordURL">https://connectid.dimagi.com/users/recover/reset_password</string>
<string name="ConnectConfirmPasswordURL">https://connectid.dimagi.com/users/recover/confirm_password</string>
<string name="ConnectSetPinURL">https://connectid.dimagi.com/users/set_recovery_pin</string>
<string name="ConnectConfirmPinURL">https://connectid.dimagi.com/users/recover/confirm_pin</string>
<string name="ConnectUpdateProfileURL">https://connectid.dimagi.com/users/update_profile</string>
<string name="ConnectChangePhoneURL">https://connectid.dimagi.com/users/change_phone</string>
<string name="ConnectPhoneAvailableURL">https://connectid.dimagi.com/users/phone_available</string>
<string name="ConnectRecoverURL">https://connectid.dimagi.com/users/recover</string>
<string name="ConnectRecoverSecondaryURL">https://connectid.dimagi.com/users/recover/secondary</string>
<string name="ConnectVerifySecondaryURL">https://connectid.dimagi.com/users/validate_secondary_phone</string>
<string name="ConnectValidatePhoneURL">https://connectid.dimagi.com/users/validate_phone</string>
<string name="ConnectRecoverConfirmOTPURL">https://connectid.dimagi.com/users/recover/confirm_otp</string>
<string name="ConnectRecoverConfirmSecondaryOTPURL">https://connectid.dimagi.com/users/recover/confirm_secondary_otp</string>

<string name="ConnectVerifyConfirmSecondaryOTPURL">https://connectid.dimagi.com/users/confirm_secondary_otp</string>
<string name="ConnectConfirmOTPURL">https://connectid.dimagi.com/users/confirm_otp</string>
<string name="ConnectRegisterURL">https://connectid.dimagi.com/users/register</string>

<string name="ConnectOpportunitiesURL">https://%s/api/opportunity/</string>
<string name="ConnectStartLearningURL">https://%s/users/start_learn_app/</string>
<string name="ConnectLearnProgressURL">https://%s/api/opportunity/%d/learn_progress</string>

<string name="ConnectClaimJobURL">https://%s/api/opportunity/%d/claim</string>
<string name="ConnectDeliveriesURL">https://%s/api/opportunity/%d/delivery_progress</string>
<string name="ConnectPaymentConfirmationURL">https://%s/api/payment/%s/confirm</string>
<string name="ConnectInitiateUserAccountDeactivationURL">https://connectid.dimagi.com/users/recover/initiate_deactivation</string>
<string name="ConnectConfirmUserAccountDeactivationURL">https://connectid.dimagi.com/users/recover/confirm_deactivation</string>

<!-- region: All strings for multiple apps and app-agnostic properties -->

Expand Down
388 changes: 221 additions & 167 deletions app/src/org/commcare/connect/network/ApiConnectId.java
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 2 additions & 6 deletions app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,18 +162,14 @@ public void processOldApiError() {
};

//Update the phone number with the server
boolean isBusy;
if (method.equals(ConnectConstants.METHOD_CHANGE_ALTERNATE)) {
isBusy = !ApiConnectId.updateUserProfile(getContext(), user.getUserId(), user.getPassword(),
ApiConnectId.updateUserProfile(getContext(), user.getUserId(), user.getPassword(),
null, phone, callback);
} else {
isBusy = !ApiConnectId.changePhone(getContext(), user.getUserId(), user.getPassword(),
ApiConnectId.changePhone(getContext(), user.getUserId(), user.getPassword(),
existing, phone, callback);
}

if (isBusy) {
Toast.makeText(getContext(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}

} else {
finish(true, phone);
Expand Down
20 changes: 10 additions & 10 deletions app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,10 +272,11 @@ public void checkPhoneNumber() {
binding.errorTextView.setText(getString(R.string.connect_phone_not_alt));
} else {
//Make sure the number isn't already in use
phone = phone.replaceAll("\\+", "%2b");
// phone = phone.replaceAll("\\+", "%2b");
binding.errorTextView.setVisibility(View.VISIBLE);
binding.errorTextView.setText(getString(R.string.connect_phone_checking));
boolean isBusy = !ApiConnectId.checkPhoneAvailable(getContext(), phone,

ApiConnectId.checkPhoneAvailable(getContext(), phone,
new IApiCallback() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
Expand All @@ -293,6 +294,12 @@ public void processSuccess(int responseCode, InputStream responseData) {
@Override
public void processFailure(int responseCode, IOException e) {
skipPhoneNumberCheck = false;
if(responseCode==406){
skipPhoneNumberCheck = false;
updateButtonEnabled();
binding.errorTextView.setVisibility(View.VISIBLE);
binding.errorTextView.setText(getString(R.string.recovery_network_outdated));
}
if (e != null) {
Logger.exception("Checking phone number", e);
}
Expand Down Expand Up @@ -327,9 +334,6 @@ public void processOldApiError() {
}
});

if (isBusy) {
Toast.makeText(getContext(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
}
}
case ConnectConstants.CONNECT_UNLOCK_ALT_PHONE_CHANGE -> {
Expand Down Expand Up @@ -357,7 +361,7 @@ public void createAccount() {
binding.nameTextValue.getText().toString(), "");

final Context context = getActivity();
boolean isBusy = !ApiConnectId.registerUser(requireActivity(), tempUser.getUserId(), tempUser.getPassword(),
ApiConnectId.registerUser(requireActivity(), tempUser.getUserId(), tempUser.getPassword(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Validate user credentials upon registration.
Consider adding server-side validations for name, phone format, or password strength. This avoids storing incomplete or invalid user records.

tempUser.getName(), phoneNo, new IApiCallback() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
Expand Down Expand Up @@ -407,10 +411,6 @@ public void processOldApiError() {
Toast.makeText(requireActivity(), R.string.recovery_network_outdated, Toast.LENGTH_SHORT).show();
}
});

if (isBusy) {
Toast.makeText(requireActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
}

private String generateUserId() {
Expand Down
6 changes: 1 addition & 5 deletions app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public void handleButtonPress() {
}
} else {
final Context context = requireActivity();
boolean isBusy = !ApiConnectId.checkPassword(requireActivity(), phone, secretKey, password, new IApiCallback() {
ApiConnectId.checkPassword(requireActivity(), phone, secretKey, password, new IApiCallback() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
String username = null;
Expand Down Expand Up @@ -237,10 +237,6 @@ public void processOldApiError() {
ConnectNetworkHelper.showOutdatedApiError(requireActivity().getApplicationContext());
}
});

if (isBusy) {
Toast.makeText(requireActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
}
}

Expand Down
18 changes: 4 additions & 14 deletions app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,18 +276,13 @@ public void processOldApiError() {
};

//Update the phone number with the server
boolean isBusy;
if (callingClass == ConnectConstants.CONNECT_UNLOCK_ALT_PHONE_CHANGE || callingClass == ConnectConstants.CONNECT_REGISTRATION_ALTERNATE_PHONE) {
isBusy = !ApiConnectId.updateUserProfile(getContext(), user.getUserId(), user.getPassword(),
ApiConnectId.updateUserProfile(getContext(), user.getUserId(), user.getPassword(),
null, phone, callback);
} else {
isBusy = !ApiConnectId.changePhone(getContext(), user.getUserId(), user.getPassword(),
ApiConnectId.changePhone(getContext(), user.getUserId(), user.getPassword(),
existing, phone, callback);
}

if (isBusy) {
Toast.makeText(getContext(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
} else {
finish(true, phone);
}
Expand All @@ -314,11 +309,10 @@ public void checkPhoneNumber() {
binding.connectPrimaryPhoneButton.setEnabled(false);
} else {
//Make sure the number isn't already in use
phone = phone.replaceAll("\\+", "%2b");
// phone = phone.replaceAll("\\+", "%2b");
binding.connectPrimaryPhoneAvailability.setText(getString(R.string.connect_phone_checking));
binding.connectPrimaryPhoneButton.setEnabled(false);

boolean isBusy = !ApiConnectId.checkPhoneAvailable(getContext(), phone,
ApiConnectId.checkPhoneAvailable(getContext(), phone,
new IApiCallback() {
private void completeCall() {
skipPhoneNumberCheck = false;
Expand Down Expand Up @@ -355,10 +349,6 @@ public void processOldApiError() {
binding.errorTextView.setText(getString(R.string.recovery_network_outdated));
}
});

if (isBusy) {
Toast.makeText(getContext(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
}
}
case ConnectConstants.METHOD_CHANGE_ALTERNATE -> {
Expand Down
27 changes: 11 additions & 16 deletions app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,22 +351,22 @@ public void processOldApiError() {
boolean isBusy;
switch (method) {
case MethodRecoveryPrimary -> {
isBusy = !ApiConnectId.requestRecoveryOtpPrimary(requireActivity(), username, callback);
ApiConnectId.requestRecoveryOtpPrimary(requireActivity(), username, callback);
}
case MethodRecoveryAlternate -> {
isBusy = !ApiConnectId.requestRecoveryOtpSecondary(requireActivity(), username, password, callback);
ApiConnectId.requestRecoveryOtpSecondary(requireActivity(), username, password, callback);
}
case MethodVerifyAlternate -> {
isBusy = !ApiConnectId.requestVerificationOtpSecondary(requireActivity(), username, password, callback);
ApiConnectId.requestVerificationOtpSecondary(requireActivity(), username, password, callback);
}
default -> {
isBusy = !ApiConnectId.requestRegistrationOtpPrimary(requireActivity(), username, password, callback);
ApiConnectId.requestRegistrationOtpPrimary(requireActivity(), username, password, callback);
}
}

if (isBusy) {
Toast.makeText(requireActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
// if (isBusy) {
// Toast.makeText(requireActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show();
// }
}

public void verifySmsCode() {
Expand Down Expand Up @@ -452,25 +452,20 @@ public void processOldApiError() {
}
};

boolean isBusy;
switch (method) {
case MethodRecoveryPrimary -> {
isBusy = !ApiConnectId.confirmRecoveryOtpPrimary(getActivity(), username, password, token, callback);
ApiConnectId.confirmRecoveryOtpPrimary(getActivity(), username, password, token, callback);
}
case MethodRecoveryAlternate -> {
isBusy = !ApiConnectId.confirmRecoveryOtpSecondary(requireActivity(), username, password, token, callback);
ApiConnectId.confirmRecoveryOtpSecondary(requireActivity(), username, password, token, callback);
}
case MethodVerifyAlternate -> {
isBusy = !ApiConnectId.confirmVerificationOtpSecondary(requireActivity(), username, password, token, callback);
ApiConnectId.confirmVerificationOtpSecondary(requireActivity(), username, password, token, callback);
}
default -> {
isBusy = !ApiConnectId.confirmRegistrationOtpPrimary(requireActivity(), username, password, token, callback);
ApiConnectId.confirmRegistrationOtpPrimary(requireActivity(), username, password, token, callback);
}
}

if (isBusy) {
Toast.makeText(requireActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
}

private void resetPassword(Context context, String phone, String secret, String username, String name) {
Expand Down
9 changes: 2 additions & 7 deletions app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,10 @@ public void handleButtonPress() {
String pin = binding.connectPinInput.getText().toString();
ConnectUserRecord user = ConnectDatabaseHelper.getUser(getActivity());

boolean isBusy = false;
final Context context = getActivity();
if (isChanging) {
//Change PIN
isBusy = !ApiConnectId.changePin(getActivity(), user.getUserId(), user.getPassword(), pin,
ApiConnectId.changePin(getActivity(), user.getUserId(), user.getPassword(), pin,
new IApiCallback() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
Expand All @@ -244,7 +243,7 @@ public void processOldApiError() {
});
} else if (isRecovery) {
//Confirm PIN
isBusy = !ApiConnectId.checkPin(getActivity(), phone, secret, pin,
ApiConnectId.checkPin(getActivity(), phone, secret, pin,
new IApiCallback() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
Expand Down Expand Up @@ -316,10 +315,6 @@ public void processOldApiError() {
//Local failure
handleWrongPin();
}

if (isBusy) {
Toast.makeText(getActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show();
}
}

private void resetPassword(Context context, String phone, String secret, ConnectUserRecord user) {
Expand Down
56 changes: 56 additions & 0 deletions app/src/org/commcare/network/connectId/network/ApiClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package org.commcare.network.connectId.network;

import org.commcare.connect.network.ApiConnectId;

import java.io.IOException;
import java.util.Map;
import java.util.concurrent.TimeUnit;

import okhttp3.Interceptor;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
import okhttp3.logging.HttpLoggingInterceptor;
import retrofit2.Retrofit;
import retrofit2.converter.gson.GsonConverterFactory;


///Todo retry part of the api fails

public class ApiClient {
private static final String BASE_URL = "https://connectid.dimagi.com"; // Replace with actual base URL
private static final String API_VERSION = ApiConnectId.API_VERSION_CONNECT_ID; // Replace with actual version value

private static Retrofit retrofit;

public static Retrofit getClient() {
HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
// set your desired log level
logging.setLevel(HttpLoggingInterceptor.Level.BODY);
if (retrofit == null) {
OkHttpClient okHttpClient = new OkHttpClient.Builder()
.addInterceptor(logging)
.addInterceptor(new Interceptor() {
@Override
public Response intercept(Chain chain) throws IOException {
Request originalRequest = chain.request();
Request requestWithHeaders = originalRequest.newBuilder()
.header("Accept", "application/json;version=" + API_VERSION)
.build();
return chain.proceed(requestWithHeaders);
}
})
.connectTimeout(30, TimeUnit.SECONDS)
.readTimeout(30, TimeUnit.SECONDS)
.writeTimeout(30, TimeUnit.SECONDS)
.build();

retrofit = new Retrofit.Builder()
.baseUrl(BASE_URL)
.client(okHttpClient)
.addConverterFactory(GsonConverterFactory.create())
.build();
}
return retrofit;
}
}
Loading