CCCT-467 || User Payment Info #2912
Conversation
Setup payment info feature
Set OTP verification screen and APIs
Add name on post params
Solved OTP verification API issues
Set resend button on OTP verification screen
Set payment info in local DB
Added payment info in local DB
|
@j13panchal Can we make sure to populate PR template abundantly here with all details that you think can make life easy for a reviewer including screenshots and demo videos for any UI/xml changes. Thanks! |
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive enhancement to the payment setup and verification process within the application. The changes span multiple components, including new layout files, navigation graph modifications, and updates to various Java classes. The primary focus is on implementing a user-friendly flow for collecting and verifying payment information. Key additions include two new fragments: Sequence DiagramsequenceDiagram
participant User
participant PaymentSetupFragment
participant ApiConnectId
participant PhoneVerificationFragment
User->>PaymentSetupFragment: Enter name and phone
PaymentSetupFragment->>ApiConnectId: Submit payment info
ApiConnectId-->>PaymentSetupFragment: Request successful
PaymentSetupFragment->>PhoneVerificationFragment: Navigate to verification
User->>PhoneVerificationFragment: Enter OTP
PhoneVerificationFragment->>ApiConnectId: Verify OTP
ApiConnectId-->>PhoneVerificationFragment: Verification successful
PhoneVerificationFragment->>User: Payment setup complete
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 19
🧹 Nitpick comments (9)
app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java (1)
119-156: Add loading state during API callThe UI should show a loading indicator while the API call is in progress to provide better user feedback.
Apply this change:
private void submitPaymentDetail() { + binding.progressBar.setVisibility(View.VISIBLE); + binding.continueButton.setEnabled(false); String phone = PhoneNumberHelper.buildPhoneNumber(binding.countryCode.getText().toString(), binding.connectPrimaryPhoneInput.getText().toString()); ConnectUserRecord user = ConnectDatabaseHelper.getUser(getActivity()); boolean isBusy = !ApiConnectId.paymentInfo(requireActivity(), phone, user.getUserId(), user.getPassword(), binding.nameTextValue.getText().toString(), new IApiCallback() { @Override public void processSuccess(int responseCode, InputStream responseData) { try { + binding.progressBar.setVisibility(View.GONE); Navigation.findNavController(binding.continueButton).navigate( ConnectPaymentSetupFragmentDirections.actionConnectPaymentSetupFragmentToConnectPaymentSetupPhoneVerificationFragment(phone,binding.nameTextValue.getText().toString(),user.getUserId(),user.getPassword())); } catch (Exception e) { Logger.exception("Parsing return from confirm_secondary_otp", e); + binding.continueButton.setEnabled(true); } } @Override public void processFailure(int responseCode, IOException e) { + binding.progressBar.setVisibility(View.GONE); + binding.continueButton.setEnabled(true); binding.errorTextView.setVisibility(View.VISIBLE); binding.errorTextView.setText(String.format(Locale.getDefault(), "Registration error: %d", responseCode)); }app/src/org/commcare/fragments/connect/ConnectPaymentSetupPhoneVerificationFragment.java (1)
47-48: Consider using an enum for method constants.The
MethodUserDeactivateconstant appears to be unused in this file and its purpose is unclear. Consider using an enum for better type safety and documentation.- public static final int MethodUserDeactivate = 5; - public static final int REQ_USER_CONSENT = 200; + private enum VerificationMethod { + USER_DEACTIVATE(5), + USER_CONSENT(200); + + private final int value; + VerificationMethod(int value) { + this.value = value; + } + + public int getValue() { + return value; + } + }app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
98-103: Remove commented-out code.The commented-out click listener code should be removed if it's no longer needed.
- /* view.findViewById(R.id.connect_jobs_last_update).setOnClickListener(new View.OnClickListener() { - @Override - public void onClick(View view) { - Navigation.findNavController(view).navigate(ConnectJobsListsFragmentDirections.actionConnectJobsListFragmentToConnectPaymentSetupFragment()); - } - });*/app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (1)
357-357: Improve user record initialization.The user record initialization with empty payment fields could be more explicit.
- binding.nameTextValue.getText().toString(), "","",""); + binding.nameTextValue.getText().toString(), + /* alternatePhone */ "", + /* paymentName */ "", + /* paymentPhone */ "");app/res/layout/fragment_connect_payment_setup.xml (2)
10-20: Consider improving the header text accessibility.The header text lacks content description and proper accessibility attributes.
Add these attributes to improve accessibility:
<org.commcare.views.connect.connecttextview.ConnectRegularTextView android:id="@+id/connectMediumTextView" + android:contentDescription="@string/connect_payment_setup_header_description" + android:importantForAccessibility="yes" ... />
141-157: Improve save button accessibility and state handling.The save button lacks proper state handling and accessibility support.
Add these attributes:
<org.commcare.views.connect.RoundedButton android:id="@+id/continue_button" + android:enabled="false" + android:contentDescription="@string/connect_payment_setup_save_description" + android:stateListAnimator="@animator/button_state_list_anim" ... />app/res/layout/screen_connect_payment_phone_verify.xml (1)
12-19: Add proper content description for logo.The company logo ImageView lacks proper content description.
<ImageView android:id="@+id/company_logo" - android:contentDescription="@null" + android:contentDescription="@string/connect_company_logo_description" ... />app/res/navigation/nav_graph_connect.xml (1)
160-168: Add pop behavior for better navigation.The payment setup fragment should define pop behavior to handle back navigation properly.
<action android:id="@+id/action_connectPaymentSetupFragment_to_connectPaymentSetupPhoneVerificationFragment" app:destination="@id/connectPaymentSetupPhoneVerificationFragment" + app:popUpTo="@id/connectPaymentSetupFragment" + app:popUpToInclusive="true" />app/res/values/strings.xml (1)
881-882: Follow string naming conventions.The payment-related strings should follow the established prefix pattern.
- <string name="connect_payment_info">Payment Info</string> - <string name="connect_payment_info_resend_otp">Resend OTP</string> + <string name="connect_payment_setup_info">Payment Info</string> + <string name="connect_payment_setup_resend_otp">Resend OTP</string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/res/layout/fragment_connect_payment_setup.xml(1 hunks)app/res/layout/screen_connect_payment_phone_verify.xml(1 hunks)app/res/navigation/nav_graph_connect.xml(2 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(3 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(4 hunks)app/src/org/commcare/connect/ConnectConstants.java(3 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(7 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(5 hunks)app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectPaymentSetupPhoneVerificationFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java(3 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(4 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- app/src/org/commcare/connect/ConnectConstants.java
- app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
- app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java
🔇 Additional comments (2)
app/res/navigation/nav_graph_connect.xml (1)
32-34: LGTM! Navigation action properly configured.The action from jobs list to payment setup is correctly defined.
app/res/values/strings.xml (1)
45-46: Secure payment URLs using HTTPS.The payment URLs are correctly using HTTPS for secure communication.
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
Show resolved
Hide resolved
| phoneNumber = Identity.getSignInClient(requireActivity()).getPhoneNumberFromIntent(data); | ||
| displayNumber(phoneNumber); | ||
| } catch (ApiException e) { | ||
| Toast.makeText(getContext(), R.string.error_occured, Toast.LENGTH_SHORT).show(); | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Improve exception handling
The current implementation throws a RuntimeException which could crash the app. Consider handling the ApiException gracefully.
Apply this change:
try {
phoneNumber = Identity.getSignInClient(requireActivity()).getPhoneNumberFromIntent(data);
displayNumber(phoneNumber);
} catch (ApiException e) {
Toast.makeText(getContext(), R.string.error_occured, Toast.LENGTH_SHORT).show();
- throw new RuntimeException(e);
+ Logger.exception("Error getting phone number from intent", e);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Changing this to log the exception then fail silently. This is only a helper to auto-populate the user's phone number, if it doesn't work they can still type the number in manually and proceed.
There was a problem hiding this comment.
@OrangeAndGreen Understood, that makes sense since it's just a convenience helper.
✏️ Learnings added
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:57.767Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java
Outdated
Show resolved
Hide resolved
| <EditText | ||
| android:id="@+id/countryCode" | ||
| android:layout_width="64dp" | ||
| android:layout_height="55dp" | ||
| android:background="@null" | ||
| android:gravity="center" | ||
| android:hint="@string/connect_phone_country_code_default" | ||
| android:inputType="number" | ||
| android:text="@string/connect_phone_country_code_default" | ||
| android:textColorHint="@color/connect_grey" /> |
There was a problem hiding this comment.
Enhance phone number input security.
The country code input field allows direct text editing which could lead to invalid input.
Apply these changes to improve input validation:
<EditText
android:id="@+id/countryCode"
android:layout_width="64dp"
android:layout_height="55dp"
- android:background="@null"
+ android:background="?android:attr/selectableItemBackground"
- android:inputType="number"
+ android:inputType="phone"
+ android:maxLength="4"
+ android:digits="+0123456789"
... />📝 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.
| <EditText | |
| android:id="@+id/countryCode" | |
| android:layout_width="64dp" | |
| android:layout_height="55dp" | |
| android:background="@null" | |
| android:gravity="center" | |
| android:hint="@string/connect_phone_country_code_default" | |
| android:inputType="number" | |
| android:text="@string/connect_phone_country_code_default" | |
| android:textColorHint="@color/connect_grey" /> | |
| <EditText | |
| android:id="@+id/countryCode" | |
| android:layout_width="64dp" | |
| android:layout_height="55dp" | |
| android:background="?android:attr/selectableItemBackground" | |
| android:gravity="center" | |
| android:hint="@string/connect_phone_country_code_default" | |
| android:inputType="phone" | |
| android:maxLength="4" | |
| android:digits="+0123456789" | |
| android:text="@string/connect_phone_country_code_default" | |
| android:textColorHint="@color/connect_grey" /> |
| <AutoCompleteTextView | ||
| android:id="@+id/connect_primary_phone_input" | ||
| android:layout_width="fill_parent" | ||
| android:layout_height="55dp" | ||
| android:background="@null" | ||
| android:digits="1234567890" | ||
| android:focusable="true" | ||
| android:focusableInTouchMode="true" | ||
| android:hint="@string/connect_phone_number_hint" | ||
| android:inputType="numberDecimal" | ||
| android:paddingStart="16dp" | ||
| android:textAlignment="textStart" | ||
| android:textColorHint="@color/connect_grey" | ||
| tools:ignore="RtlSymmetry" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve phone number input validation.
The phone number input field needs stronger validation constraints.
Apply these changes:
<AutoCompleteTextView
android:id="@+id/connect_primary_phone_input"
- android:digits="1234567890"
+ android:digits="0123456789"
+ android:maxLength="15"
android:inputType="phone"
+ android:imeOptions="actionDone"
... />📝 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.
| <AutoCompleteTextView | |
| android:id="@+id/connect_primary_phone_input" | |
| android:layout_width="fill_parent" | |
| android:layout_height="55dp" | |
| android:background="@null" | |
| android:digits="1234567890" | |
| android:focusable="true" | |
| android:focusableInTouchMode="true" | |
| android:hint="@string/connect_phone_number_hint" | |
| android:inputType="numberDecimal" | |
| android:paddingStart="16dp" | |
| android:textAlignment="textStart" | |
| android:textColorHint="@color/connect_grey" | |
| tools:ignore="RtlSymmetry" /> | |
| <AutoCompleteTextView | |
| android:id="@+id/connect_primary_phone_input" | |
| android:layout_width="fill_parent" | |
| android:layout_height="55dp" | |
| android:background="@null" | |
| android:digits="0123456789" | |
| android:focusable="true" | |
| android:focusableInTouchMode="true" | |
| android:hint="@string/connect_phone_number_hint" | |
| android:inputType="phone" | |
| android:maxLength="15" | |
| android:imeOptions="actionDone" | |
| android:paddingStart="16dp" | |
| android:textAlignment="textStart" | |
| android:textColorHint="@color/connect_grey" | |
| tools:ignore="RtlSymmetry" /> |
… into jp/CCCT-467-user-payment-info
… into jp/CCCT-467-user-payment-info Checking for field in API call. Error-handling and string cleanup.
Cleaned up some obsolete code.
…a common helper function. Cleaned up some obsolete imports. Improved logging for a couple errors.
…align with text fields
Changed a RoundedButton to MaterialButton.
|
@damagatchi retest this please |
…d in ConnectActivity. Cleaned up some obsolete code related to code flow with phone number picker (using onActivityResult).
| android:focusable="true" | ||
| android:focusableInTouchMode="true" | ||
| android:hint="@string/connect_phone_number_hint" | ||
| android:inputType="numberDecimal" |
There was a problem hiding this comment.
any reason inputType is not phone here ?
| <string name="ConnectPaymentPhoneNumberURL">https://connectid.dimagi.com/users/profile/payment_phone_number</string> | ||
| <string name="ConnectConfirmPaymentOtpURL">https://connectid.dimagi.com/users/profile/confirm_payment_otp</string> |
There was a problem hiding this comment.
think these should be relative urls to host so that we can easily configure host separately. (eg. prod vs staging)
| if(fragment instanceof ConnectPaymentSetupFragment) { | ||
| if (requestCode == ConnectConstants.CONNECTID_REQUEST_CODE) { | ||
| navController.navigate(ConnectJobsListsFragmentDirections. | ||
| actionConnectJobsListFragmentToConnectJobIntroFragment()); | ||
| } |
There was a problem hiding this comment.
Can you describe what's happening here. I am confused as to what workflow is this code handling and why are we using actionConnectJobsListFragmentToConnectJobIntroFragment for the ConnectPaymentSetupFragment
| String phone = intent.getStringExtra("phone"); | ||
| String name = intent.getStringExtra("name"); |
There was a problem hiding this comment.
nit: as this only seems relevant to beginPaymentPhoneVerification, we can just directly pass the intent to that method and have it extract these values from intent.
| public boolean onSingleTapUp(MotionEvent arg0) { | ||
| if(performingOptionalPhoneVerification) { | ||
| finish(); | ||
| return true; | ||
| } else { | ||
| return super.onSingleTapUp(arg0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this mean that we will close the screen user is on if user taps the screen while phone verification ?
|
|
||
| private void launchPaymentInfo(ConnectJobRecord job) { | ||
| ConnectManager.setActiveJob(job); | ||
| FirebaseAnalyticsUtil.reportPaymentInfoPage("Config required"); |
There was a problem hiding this comment.
can this be logged from the corresponding Fragment itself ? Also don't think we need to pass a reason if there is only one reason we land on the payment info page.
separately, all analytics parameter values strictly needs to be a constant reference so that we don't create random values in the analytics platform.
| private void launchPaymentInfo(ConnectJobRecord job) { | ||
| ConnectManager.setActiveJob(job); | ||
| FirebaseAnalyticsUtil.reportPaymentInfoPage("Config required"); | ||
| Navigation.findNavController(view).navigate(ConnectJobsListsFragmentDirections.actionConnectJobsListFragmentToConnectPaymentSetupFragment()); |
There was a problem hiding this comment.
nit: code formatting seems off
| public class ConnectPaymentSetupFragment extends Fragment { | ||
|
|
||
| private FragmentConnectPaymentSetupBinding binding; | ||
| private boolean showhPhoneDialog = true; |
| }); | ||
| binding.countryCode.addTextChangedListener(new TextWatcher() { |
|
|
||
| import java.util.Locale; | ||
|
|
||
| public class ConnectPaymentSetupFragment extends Fragment { |
There was a problem hiding this comment.
There seems to be code in this class which is quite similar to some other connect classes around phone number handling. We need to try very hard to not repeat such blocks of code in the codebase as much as possible.
Summary
https://dimagi.atlassian.net/browse/CCCT-467
cross-request: dimagi/commcare-core#1455
Description
This PR introduces user payment info in the mobile code. The new mechanism allows opportunities to be configured so they require the user to specify digital payment info (an official name and a phone number for the payment account).
The user table in the database now includes two additional fields for the user's official name and phone number for them to receive digital payments. Each opportunity can now be configured to require that the user have configured their payment info before beginning the opportunity, and there is an additional field in the opportunity DB table to hold this flag. When this flag is set for an opportunity and the user attempts to enter the opportunity, the code checks to see if the user has already configured their payment info. If not, the user is first directed to a new page where they are prompted to enter the info before they can proceed to the opportunity.
This is the page the user sees when prompted to configure payment info:
When configuring the payment info, the user must verify that they have control of the payment phone by matching an OTP sent via SMS to the payment phone.
On successful account recovery, the server sends the previously configured payment info to the device so it can be stored locally and shown to the user.
PR Checklist
Automated test coverage
No automated tests have been configured for this or any other Connect functionality yet.
Safety story
QA Note
New test plan should be created for this feature, including:
Release Note
Connect users can now specify digital payment information so funds for some opportunities can be transferred to them via a mobile device and e-banking transaction.