-
-
Notifications
You must be signed in to change notification settings - Fork 599
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.
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
Google Pay integration. #4624
Google Pay integration. #4624
Conversation
…edia into gPay_design
if (Prefs.paymentMethodsMerchantId.isEmpty() || Prefs.paymentMethodsGatewayId.isEmpty()) { | ||
uiState.value = NoPaymentMethod() | ||
} else { | ||
uiState.value = Resource.Success(donationConfig!!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be safer if we handle the null-safey check for donationConfig
?
donationConfig?.let {
} ?: run {
uiState.value = Resouce.Error("...")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure this would be safer... If donationConfig
is null, it will throw a NullPointerException, which will be caught by the CoroutineExceptionHandler
at the top, and will automatically provide a Resource.Error
.
app/src/main/java/org/wikipedia/page/campaign/CampaignDialog.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions:
- When I changed the country code to TW (Taiwan), it showed the currency correctly, but it did not show the suggested amounts, is that expected?
- It looks like the input field allows the user to add multiple zeros to the beginning of the amount, and it does not affect the final amount. Just wonder if we need to remove the unnecessary zero automatically.
- When processing the payment, the activity title still showed the "Select amount" title with an empty content. Do we need to change the activity title to "Processing payment" or other proper title?
@cooltey Excellent observations!
This actually might be a bug in the fundraising API -- it's not giving us the preset amounts for the TWD currency.
Ehhh I think that's an acceptable behavior. As long as the final amount is parsed correctly, why not let people enter 000001 when they really mean 1.
¯\_(ツ)_/¯ this is how it was designed, and it's probably perfectly good for a first iteration. We will surely make refinements as we go along, and I'll make an additional note to discuss this step at the next prioritization. |
Great! Thanks!
That makes sense to me.
Got it! I have one final question before merging: From the article donate campaign dialog, when the user selects "Donate now", the GPay bottomsheet pops up. If the user accidentally closes the bottomsheet by tapping on the dimmed side, the entire donation flow will still be considered as "Completed". Do we need to update the preference logic to make sure the user clicks one of the options in the GPay bottomsheet instead of saving the preference when tapping on the the dialog "Donate now"? |
https://phabricator.wikimedia.org/T362698