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

Feature/8182 iap single subcription requirement #8206

Merged
merged 18 commits into from
Feb 1, 2023
Merged

Feature/8182 iap single subcription requirement #8206

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d0474b4
Ensure user has no active IAP subscription
JorgeMucientes Jan 17, 2023
389cd22
Refactor error handling
JorgeMucientes Jan 17, 2023
b7aea27
Fix and add tests
JorgeMucientes Jan 17, 2023
bc1e515
Fix detekt indentation issues
JorgeMucientes Jan 17, 2023
583e447
Add missing assertion
JorgeMucientes Jan 18, 2023
00989fc
Update message for use case where user has an active IAP Woo subscrip…
JorgeMucientes Jan 19, 2023
ea8d561
Add new error handling for RemoteCommunication IAP errors
JorgeMucientes Jan 20, 2023
4485f34
Update FluxC version to include new CreateOrderErrorType network error
JorgeMucientes Jan 20, 2023
0fac0d0
Remove duplicated implementations of IAPMobilePayAPI
JorgeMucientes Jan 20, 2023
a061571
Merge branch 'trunk' into feature/8182-iap-single-subcription-require…
JorgeMucientes Jan 20, 2023
5b5d96a
Add curated message for cases where plan is purchased but not acknowl…
JorgeMucientes Jan 20, 2023
b131154
Fix detekt indentation issue
JorgeMucientes Jan 20, 2023
68ebb47
Fix and add unit tests
JorgeMucientes Jan 20, 2023
ea981c8
Merge branch 'trunk' into feature/8182-iap-single-subcription-require…
JorgeMucientes Jan 23, 2023
1d3c232
Add FluxC latest trunk changeset to fix compile issues
JorgeMucientes Jan 23, 2023
0ec9cb1
Merge branch 'trunk' into feature/8182-iap-single-subcription-require…
JorgeMucientes Feb 1, 2023
49b9389
Fix error mapping after bad merge
JorgeMucientes Feb 1, 2023
50e07db
Remove empty file after merge
JorgeMucientes Feb 1, 2023
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
77 changes: 64 additions & 13 deletions ...main/kotlin/com/woocommerce/android/ui/login/storecreation/iap/IapEligibilityViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
package com.woocommerce.android.ui.login.storecreation.iap

import androidx.annotation.StringRes
import androidx.lifecycle.LiveData
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.asLiveData
import com.woocommerce.android.R
import com.woocommerce.android.analytics.AnalyticsEvent
import com.woocommerce.android.analytics.AnalyticsTracker
import com.woocommerce.android.analytics.AnalyticsTrackerWrapper
import com.woocommerce.android.iap.pub.PurchaseWPComPlanActions
import com.woocommerce.android.iap.pub.PurchaseWpComPlanSupportChecker
import com.woocommerce.android.iap.pub.model.IAPError
import com.woocommerce.android.iap.pub.model.IAPSupportedResult
import com.woocommerce.android.iap.pub.model.PurchaseStatus
import com.woocommerce.android.iap.pub.model.WPComIsPurchasedResult
import com.woocommerce.android.ui.login.storecreation.iap.IapEligibilityViewModel.IapEligibilityEvent.NavigateToNextStep
import com.woocommerce.android.viewmodel.MultiLiveEvent
import com.woocommerce.android.viewmodel.ScopedViewModel
Expand All @@ -22,7 +27,8 @@ class IapEligibilityViewModel @Inject constructor(
savedStateHandle: SavedStateHandle,
private val planSupportChecker: PurchaseWpComPlanSupportChecker,
private val analyticsTrackerWrapper: AnalyticsTrackerWrapper,
private val isIAPEnabled: IsIAPEnabled
private val isIAPEnabled: IsIAPEnabled,
private val iapManager: PurchaseWPComPlanActions
) : ScopedViewModel(savedStateHandle, planSupportChecker) {
private val _isCheckingIapEligibility = savedState.getStateFlow(scope = this, initialValue = true)
val isCheckingIapEligibility: LiveData<Boolean> = _isCheckingIapEligibility.asLiveData()
Expand All @@ -32,28 +38,30 @@ class IapEligibilityViewModel @Inject constructor(
launch {
when (val result = planSupportChecker.isIAPSupported()) {
is IAPSupportedResult.Success -> onSuccess(result)
is IAPSupportedResult.Error -> onError(result)
is IAPSupportedResult.Error -> onIAPError(result.errorType)
}
}
} else {
triggerEvent(NavigateToNextStep)
}
}

private fun onError(result: IAPSupportedResult.Error) {
analyticsTrackerWrapper.track(
AnalyticsEvent.SITE_CREATION_IAP_ELIGIBILITY_ERROR,
mapOf(AnalyticsTracker.KEY_ERROR_TYPE to result.errorType.toString())
private fun onUserNotEligibleForIAP(
@StringRes title: Int = R.string.store_creation_iap_eligibility_check_error_title,
@StringRes message: Int
) {
_isCheckingIapEligibility.value = false
triggerDialogError(
title = title,
message = message
)
onUserNotEligibleForIAP()
}

private fun onUserNotEligibleForIAP() {
_isCheckingIapEligibility.value = false
private fun triggerDialogError(@StringRes title: Int, @StringRes message: Int) {
triggerEvent(
MultiLiveEvent.Event.ShowDialog(
titleId = R.string.store_creation_iap_eligibility_check_error_title,
messageId = R.string.store_creation_iap_eligibility_check_error_description,
titleId = title,
messageId = message,
negativeBtnAction = { dialog, _ ->
triggerEvent(MultiLiveEvent.Event.Exit)
dialog.dismiss()
Expand All @@ -69,9 +77,52 @@ class IapEligibilityViewModel @Inject constructor(
mapOf(AnalyticsTracker.KEY_IAP_ELIGIBLE to result.isSupported)
)
when {
result.isSupported -> triggerEvent(NavigateToNextStep)
else -> onUserNotEligibleForIAP()
result.isSupported -> checkIfWooPlanAlreadyPurchased()
else -> onUserNotEligibleForIAP(
message = R.string.store_creation_iap_eligibility_check_error_not_available_for_country
)
}
}

private fun checkIfWooPlanAlreadyPurchased() {
launch {
when (val result = iapManager.isWPComPlanPurchased()) {
is WPComIsPurchasedResult.Success -> {
when (result.purchaseStatus) {
PurchaseStatus.PURCHASED_AND_ACKNOWLEDGED,
PurchaseStatus.PURCHASED -> onUserNotEligibleForIAP(
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this case. So the item is purchased but not acknowledged by our backend, which means that the WP plan is not given to a user as well. We could call purchaseWPComPlan and it internally will call the backend and the backend should acknowledge the purchase on the GP side, but at this moment we actually not sure if this is the same site a user wanted to purchase a subscription to at the begging 🤔

On other hand, it's the user's decision and we should hope that they know what they are doing, so maybe we still should purchaseWPComPlan in this case? I am not saying to call it here, but let a user go further, and then when the website is picked use an existing GP subscription in order to give a WP plan to the picked site

Another option would be to at least show to a user that yes we have your purchase, please contact support so they will link the GP subscription to a site that needs an upgrade

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @kidinov I'm not sure we can do what you suggest. First of all I'd like to ask under which circumstance the purchase can be placed but not acknowledged?

Regarding this:

We could call purchaseWPComPlan and it internally will call the backend and the backend should acknowledge the purchase on the GP side, but at this moment we actually not sure if this is the same site a user wanted to purchase a subscription to at the begging

We do not have a siteId available at this point and we have no clue about the site that has the active IAP WooCommerce subscription, as this subscription is tied to Google account. Keep in mind these checks run when user clicks on "create a new store", so the user hasn't even entered a site name or anything yet.

I agree we could redirect them to support to troubleshoot the problem. I'll submit a curated message for that scenario with the appropriate CTA to create a Zendesk ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

First of all I'd like to ask under which circumstance the purchase can be placed but not acknowledged?

The IAP becomes purchased after the user from the app makes a purchase. It gets acknowledged by the backend after we do the create_order call. So any network issue between the mobile and the backend, or between the backend and google play will lead to the purchased and not acknowledged state. Also, any error on the backend or the user kills the app at a specific moment will lead to the same result. So there are many places where it can go wrong.

Having said that, after some time not acknowledged purchase will get refunded, so it's not the end of the world

We do not have a siteId available at this point and we have no clue about the site that has the active IAP WooCommerce subscription, as this subscription is tied to Google account.

Yep, that's why I mentioned:

I am not saying to call it here, but let a user go further, and then when the website is picked use an existing GP subscription in order to give a WP plan to the picked site

So you could just let a user go further, and let them click the "purchase" button and then the library will do the rest of the work, but thinking about this now, it would feel weird for a user, to buy the same thing for the second time. It's not like the "try again" button, so I guess it is not something that people will do, and saying that to talk to support is probably the best approach in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying the scenarios when purchase is done but couldn't be acknoledged.

but thinking about this now, it would feel weird for a user, to buy the same thing for the second time

I agree with that. The user would be taken through a flow that expects a new site at all times. So it would be super confusing for them to have to enter all the same information again (in some cases they wouldn't be able to do this as the domain for example would be already assigned to their previous site), and then be requested to pay again for something they had already paid. Offer a "Retry" option for this case would require an entirely different flow where we request the site address the site they have purchased an eCommerce subscription for but is not acknowledged.

Once more question on this. Do HE have an straightforward way to check if users have an active IAP eCommerce, and if they do, link the subscription to their site?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this for reference @kidinov
If the purchase fails to be acknowledged it won't matter if we retry to confirm the purchase as the associated purchaseToken expires.
Full context on this discussion p1674662344895449/1673441948.587159-slack-C025A8VV728

message = R.string.store_creation_iap_eligibility_check_error_existing_subscription
)
PurchaseStatus.NOT_PURCHASED -> triggerEvent(NavigateToNextStep)
}
}
is WPComIsPurchasedResult.Error -> onIAPError(result.errorType)
}
}
}

private fun onIAPError(error: IAPError) {
analyticsTrackerWrapper.track(
AnalyticsEvent.SITE_CREATION_IAP_ELIGIBILITY_ERROR,
mapOf(AnalyticsTracker.KEY_ERROR_TYPE to error.toString())
)
val message = when (error) {
is IAPError.RemoteCommunication,
is IAPError.Billing.DeveloperError,
is IAPError.Billing.ServiceDisconnected,
is IAPError.Billing.FeatureNotSupported,
is IAPError.Billing.BillingUnavailable,
is IAPError.Billing.Unknown,
is IAPError.Billing.ItemUnavailable,
is IAPError.Billing.ServiceTimeout,
is IAPError.Billing.ServiceUnavailable,
is IAPError.Billing.ItemNotOwned,
is IAPError.Billing.UserCancelled,
is IAPError.RemoteCommunication.Network -> R.string.store_creation_iap_eligibility_check_generic_error
Copy link
Contributor

@kidinov kidinov Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to use offline_error string for the RemoteCommunication.Network error? To be a bit more specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure about displaying any non-generic message here.
From what I've seen in the code is that IAPError.RemoteCommunication.Network is returned when BaseRequest.GenericErrorType.TIMEOUT is returned. But that doesn't necessarily mean the user is offline. In fact there is another type of error for that BaseRequest.GenericErrorType.NO_CONNECTION, which is mapped to CreateOrderErrorType.API_ERROR

    private fun WPComGsonRequest.WPComGsonNetworkError.toCreateOrderError() =
        when (type) {
            BaseRequest.GenericErrorType.TIMEOUT -> CreateOrderErrorType.TIMEOUT
            BaseRequest.GenericErrorType.NO_CONNECTION,
            BaseRequest.GenericErrorType.SERVER_ERROR,
            BaseRequest.GenericErrorType.INVALID_SSL_CERTIFICATE,
            BaseRequest.GenericErrorType.NETWORK_ERROR -> CreateOrderErrorType.API_ERROR
            BaseRequest.GenericErrorType.PARSE_ERROR,
            BaseRequest.GenericErrorType.NOT_FOUND,
            BaseRequest.GenericErrorType.CENSORED,
            BaseRequest.GenericErrorType.INVALID_RESPONSE -> CreateOrderErrorType.INVALID_RESPONSE
            BaseRequest.GenericErrorType.HTTP_AUTH_ERROR,
            BaseRequest.GenericErrorType.AUTHORIZATION_REQUIRED,
            BaseRequest.GenericErrorType.NOT_AUTHENTICATED -> CreateOrderErrorType.AUTH_ERROR
            BaseRequest.GenericErrorType.UNKNOWN -> CreateOrderErrorType.GENERIC_ERROR
            null -> CreateOrderErrorType.GENERIC_ERROR
        }

I wonder if ``BaseRequest.GenericErrorType.NO_CONNECTION, should be mapped instead to CreateOrderErrorType.TIMEOUT ? Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just judging by Volley's naming of the errors:

NoConnectionError
Error indicating that no connection could be established when performing a Volley request.

It might refer to the case when a TCP handshake could not be established. This can happen when the device is offline, some package loss, or any connection-related issue

TimeoutError
Indicates that the connection or the socket timed out.

The request was sent, but the response didn't come back during the timeout period. The reason can be exactly the same as the first error + the backend may just didn't answer on time

But that doesn't necessarily mean the user is offline.

True, but I guess the probability that something with a connection is high in these cases

I wonder if ``BaseRequest.GenericErrorType.NO_CONNECTION, should be mapped instead to CreateOrderErrorType.TIMEOUT? Wdyt?

Yeah, seems to be a bug in the implementation there. I'll create for myself a ticket to fix that

We might use less assertive message

<string name="error_generic_network">A network error occurred. Please check your connection and try again.</string>

in this case, wdyt?

is IAPError.Billing.ItemAlreadyOwned ->
R.string.store_creation_iap_eligibility_check_error_existing_subscription
}
onUserNotEligibleForIAP(message = message)
}

sealed class IapEligibilityEvent : MultiLiveEvent.Event() {
Expand Down
4 changes: 3 additions & 1 deletion WooCommerce/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,7 +2766,9 @@
<string name="store_creation_iap_eligibility_loading_title">We are getting ready for your store creation</string>
<string name="store_creation_iap_eligibility_loading_subtitle">Please remain connected.</string>
<string name="store_creation_iap_eligibility_check_error_title">Store Creation Unavailable </string>
<string name="store_creation_iap_eligibility_check_error_description">Store creation from the mobile app is currently unavailable for your country.</string>
<string name="store_creation_iap_eligibility_check_error_not_available_for_country">Store creation from the mobile app is currently unavailable for your country.</string>
<string name="store_creation_iap_eligibility_check_error_existing_subscription">You have an active WooCommerce subscription linked to your Google account. Only one WooCommerce subscription per Google account is allowed.</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we will give an option for what to do next to a user, something like:

Currently you are only able to purchase one eCommerce plan from the mobile app. For additional options, please visit our website

Copy link
Contributor Author

@JorgeMucientes JorgeMucientes Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, makes sense. I've updated the message. However we can't provide a link to the actual store creation flow as that would not be compliant with Google Play terms.

<string name="store_creation_iap_eligibility_check_generic_error">Store creation is currently unavailable. Please, try again later</string>
<string name="store_creation_create_new_store_label">Create a new store</string>
<string name="store_creation_cannot_load_store">Couldn\'t load your store.\nPlease try again…</string>
<string name="store_creation_in_progress">Creating your store…</string>
Expand Down
58 changes: 57 additions & 1 deletion .../kotlin/com/woocommerce/android/ui/login/storecreation/iap/IapEligibilityViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package com.woocommerce.android.ui.login.storecreation.iap

import androidx.lifecycle.SavedStateHandle
import com.woocommerce.android.R
import com.woocommerce.android.analytics.AnalyticsTrackerWrapper
import com.woocommerce.android.iap.pub.PurchaseWPComPlanActions
import com.woocommerce.android.iap.pub.PurchaseWpComPlanSupportChecker
import com.woocommerce.android.iap.pub.model.IAPError
import com.woocommerce.android.iap.pub.model.IAPSupportedResult
import com.woocommerce.android.iap.pub.model.PurchaseStatus
import com.woocommerce.android.iap.pub.model.WPComIsPurchasedResult
import com.woocommerce.android.ui.login.storecreation.iap.IapEligibilityViewModel.IapEligibilityEvent.NavigateToNextStep
import com.woocommerce.android.util.captureValues
import com.woocommerce.android.viewmodel.BaseUnitTest
Expand All @@ -25,6 +29,7 @@ class IapEligibilityViewModelTest : BaseUnitTest() {
private val planSupportChecker: PurchaseWpComPlanSupportChecker = mock()
private val analyticsTrackerWrapper: AnalyticsTrackerWrapper = mock()
private val isIAPEnabled: IsIAPEnabled = mock()
private val iapManager: PurchaseWPComPlanActions = mock()

private lateinit var viewModel: IapEligibilityViewModel

Expand All @@ -34,7 +39,8 @@ class IapEligibilityViewModelTest : BaseUnitTest() {
savedState,
planSupportChecker,
analyticsTrackerWrapper,
isIAPEnabled
isIAPEnabled,
iapManager
)
}

Expand Down Expand Up @@ -64,11 +70,13 @@ class IapEligibilityViewModelTest : BaseUnitTest() {
testBlocking {
whenever(isIAPEnabled.invoke()).thenReturn(true)
givenUserIsEligibleForIAP(isEligible = true)
givenWpPlanPurchasedReturns(PurchaseStatus.NOT_PURCHASED)

viewModel.checkIapEligibility()
val event = viewModel.event.captureValues().last()

verify(planSupportChecker).isIAPSupported()
verify(iapManager).isWPComPlanPurchased()
assertThat(event).isEqualTo(NavigateToNextStep)
}

Expand All @@ -83,6 +91,8 @@ class IapEligibilityViewModelTest : BaseUnitTest() {

verify(planSupportChecker).isIAPSupported()
assertThat(event).isExactlyInstanceOf(MultiLiveEvent.Event.ShowDialog::class.java)
assertThat((event as MultiLiveEvent.Event.ShowDialog).messageId)
.isEqualTo(R.string.store_creation_iap_eligibility_check_error_not_available_for_country)
}

@Test
Expand All @@ -100,11 +110,57 @@ class IapEligibilityViewModelTest : BaseUnitTest() {
verify(planSupportChecker).isIAPSupported()
assertFalse(isLoadingState)
assertThat(event).isExactlyInstanceOf(MultiLiveEvent.Event.ShowDialog::class.java)
assertThat((event as MultiLiveEvent.Event.ShowDialog).messageId)
.isEqualTo(R.string.store_creation_iap_eligibility_check_generic_error)
}

@Test
fun `given an active IAP subscription, when checking user eligibility, then show error dialog and hide loading`() =
testBlocking {
whenever(isIAPEnabled.invoke()).thenReturn(true)
givenUserIsEligibleForIAP(isEligible = true)
givenWpPlanPurchasedReturns(PurchaseStatus.PURCHASED)

viewModel.checkIapEligibility()
val event = viewModel.event.captureValues().last()
val isLoadingState = viewModel.isCheckingIapEligibility.captureValues().last()

assertFalse(isLoadingState)
assertThat(event).isExactlyInstanceOf(MultiLiveEvent.Event.ShowDialog::class.java)
assertThat((event as MultiLiveEvent.Event.ShowDialog).messageId)
.isEqualTo(R.string.store_creation_iap_eligibility_check_error_existing_subscription)
}

@Test
fun `given an error, when checking for active subscriptions, then show error dialog and hide loading`() =
testBlocking {
whenever(isIAPEnabled.invoke()).thenReturn(true)
givenUserIsEligibleForIAP(isEligible = true)
givenWpPlanPurchasedReturnsError(IAPError.Billing.ServiceDisconnected(debugMessage = ""))

viewModel.checkIapEligibility()
val event = viewModel.event.captureValues().last()
val isLoadingState = viewModel.isCheckingIapEligibility.captureValues().last()

assertFalse(isLoadingState)
assertThat(event).isExactlyInstanceOf(MultiLiveEvent.Event.ShowDialog::class.java)
assertThat((event as MultiLiveEvent.Event.ShowDialog).messageId)
.isEqualTo(R.string.store_creation_iap_eligibility_check_generic_error)
}

private suspend fun givenUserIsEligibleForIAP(isEligible: Boolean) {
whenever(planSupportChecker.isIAPSupported()).thenReturn(
IAPSupportedResult.Success(isSupported = isEligible)
)
}

private suspend fun givenWpPlanPurchasedReturns(purchasedStatus: PurchaseStatus) {
whenever(iapManager.isWPComPlanPurchased())
.thenReturn(WPComIsPurchasedResult.Success(purchasedStatus))
}

private suspend fun givenWpPlanPurchasedReturnsError(error: IAPError) {
whenever(iapManager.isWPComPlanPurchased())
.thenReturn(WPComIsPurchasedResult.Error(error))
}
}