Skip to content

Store creation using a webview #7602

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

Merged
merged 16 commits into from
Oct 25, 2022
Merged

Store creation using a webview #7602

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
430f4e5
Add logic to extract a new store URL from a webview
0nko Oct 18, 2022
fdcabaf
Add new store creation buttons
0nko Oct 18, 2022
a558a5b
Select the newly created site and display the store
0nko Oct 18, 2022
48cb8ea
Try multiple different URLs returned from a site creation flow
0nko Oct 22, 2022
6040d21
Fix the site reload logic to make sure the new site is ready to be lo…
0nko Oct 22, 2022
43eb48c
Prevent the navigation logic from being executed in a paused webview …
0nko Oct 22, 2022
2f4a98b
Remove unused code
0nko Oct 22, 2022
1b61062
Fix ktlint issues
0nko Oct 22, 2022
fd11b02
Add a feature flag
0nko Oct 22, 2022
6f9f490
Merge branch 'trunk' into feature/webview-store-creation
0nko Oct 23, 2022
28d47e2
Fix the feature flag
0nko Oct 23, 2022
a1d5f6c
Update the fragment title for store creation
0nko Oct 23, 2022
61f6895
Fix the lint issues
0nko Oct 23, 2022
eda1ab1
Fix the way a repeated result returning is handled
0nko Oct 24, 2022
71c6a95
Rename the STORE_CREATION_FLOW feature flag to ACCOUNT_CREATION_FLOW
0nko Oct 25, 2022
6627d58
Merge branch 'trunk' into feature/webview-store-creation
0nko Oct 25, 2022
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
2 changes: 2 additions & 0 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/analytics/AnalyticsEvent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ enum class AnalyticsEvent(val siteless: Boolean = false) {
SITE_PICKER_SITE_DISCOVERY(siteless = true),
SITE_PICKER_JETPACK_TIMEOUT_ERROR_SHOWN(siteless = true),
SITE_PICKER_JETPACK_TIMEOUT_CONTACT_SUPPORT_CLICKED(siteless = true),
SITE_PICKER_CREATE_SITE_TAPPED(siteless = true),
LOGIN_WOOCOMMERCE_SITE_CREATED(siteless = true),

// -- Dashboard
DASHBOARD_PULLED_TO_REFRESH,
Expand Down
40 changes: 38 additions & 2 deletions ...ce/src/main/kotlin/com/woocommerce/android/ui/common/wpcomwebview/WPComWebViewFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@ import android.view.View
import android.webkit.WebChromeClient
import android.webkit.WebView
import androidx.core.view.isVisible
import androidx.lifecycle.lifecycleScope
import androidx.navigation.fragment.navArgs
import com.woocommerce.android.R
import com.woocommerce.android.databinding.FragmentWpcomWebviewBinding
import com.woocommerce.android.extensions.navigateBackWithNotice
import com.woocommerce.android.extensions.navigateBackWithResult
import com.woocommerce.android.ui.base.BaseFragment
import com.woocommerce.android.ui.common.wpcomwebview.WPComWebViewFragment.UrlComparisonMode.EQUALITY
import com.woocommerce.android.ui.common.wpcomwebview.WPComWebViewFragment.UrlComparisonMode.PARTIAL
import com.woocommerce.android.ui.main.MainActivity.Companion.BackPressListener
import com.woocommerce.android.util.WooLog
import com.woocommerce.android.util.WooLog.T
import dagger.hilt.android.AndroidEntryPoint
import org.wordpress.android.fluxc.network.UserAgent
import javax.inject.Inject
Expand All @@ -28,11 +32,15 @@ import javax.inject.Inject
class WPComWebViewFragment : BaseFragment(R.layout.fragment_wpcom_webview), UrlInterceptor, BackPressListener {
companion object {
const val WEBVIEW_RESULT = "webview-result"
const val WEBVIEW_RESULT_WITH_URL = "webview-result-with-url"
const val WEBVIEW_DISMISSED = "webview-dismissed"
const val WEBVIEW_STORE_CHECKOUT_STRING = "checkout/thank-you/"
const val WEBVIEW_STORE_URL_KEY = "store-url-key"
Comment on lines +36 to +37
Copy link
Member

@hichamboushaba hichamboushaba Oct 24, 2022

Choose a reason for hiding this comment

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

Before starting a full review for the PR @0nko, I wanted to raise the point below 🙂, if you feel like what I'm raising below doesn't make sense, or if the time doesn't allow it, please let me know, and I'll review the PR as is.

I feel like these constants and the logic below to extract the site URL don't belong in this class, this fragment was supposed to be a reusable screen for the authenticated WPCom logic, with the sole responsibility of loading a URL and then informing the parent screens when a specific URL was loaded as a termination of a specific flow, but with the current logic, it's responsible for more than this, and it has logic that's not reusable or part of other flows.
If the siteUrl extraction logic needed only the last URL of the flow, then I would've suggested to update the logic of the screen to return the last URL, and make the parsing of this URL a responsibility of the calling Fragment.
But given how it has to be done, I suggest we create a separate screen for this, I believe we can use the Composable component WCWebView, it supports WPCom authentication, and it's flexible enough, so that we can have the URL parsing and extraction easily separated. This will have some other advantages:

  1. Remove the responsibility of waiting from the site picker, it already handles too much, and by being implemented in the new screen, we can use a better ui/text for the loading if needed (something other than "verifying your site" which sounds weird for a site that we were responsible for creating 😅).
  2. Then in the site picker, the onStoreCreated can be very simplified, basically the same implementation as onSiteAddressReceived.

Copy link
Contributor Author

@0nko 0nko Oct 24, 2022

Choose a reason for hiding this comment

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

Thanks for the great suggestions, @hichamboushaba! I've been so focused on making the store creation flow load the new site automatically that I didn't stop and think about how to properly separate the new functionality from the existing components, thinking this is all just a temporary solution anyway...

As you suggested, we can use the WCWebView in its own Composable screen, with a ViewModel, which can then be used as a base for Iteration 2. I started working on it but I can already see it will be a bit more work.

Considering the circumstances, could we go ahead with the current PR (since it's already providing the desired functionality), and in the meantime, I'll refactor the code, as I mentioned above?

Copy link
Member

@hichamboushaba hichamboushaba Oct 24, 2022

Choose a reason for hiding this comment

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

Sounds good @0nko 🙂, thanks for this.
For the review, I added a comment about a potential issue that we want to fix before releasing this, and for the rest, I will review the code and functionality today or tomorrow morning.

}

private val webViewClient by lazy { WPComWebViewClient(this) }
private val navArgs: WPComWebViewFragmentArgs by navArgs()
private var siteUrls = ArrayList<String>()
Copy link
Contributor Author

@0nko 0nko Oct 23, 2022

Choose a reason for hiding this comment

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

There are multiple address aliases associated with a new site. That's why a list of addresses is returned so that the site can be loaded and selected.


@Inject lateinit var wpcomWebViewAuthenticator: WPComWebViewAuthenticator

Expand All @@ -52,20 +60,48 @@ class WPComWebViewFragment : BaseFragment(R.layout.fragment_wpcom_webview), UrlI
}
}
this.settings.javaScriptEnabled = true
this.settings.domStorageEnabled = true
settings.userAgentString = userAgent.userAgent
}

siteUrls = savedInstanceState?.getStringArrayList(WEBVIEW_STORE_URL_KEY) ?: siteUrls

wpcomWebViewAuthenticator.authenticateAndLoadUrl(binding.webView, navArgs.urlToLoad)
}

override fun onSaveInstanceState(outState: Bundle) {
outState.putStringArrayList(WEBVIEW_STORE_URL_KEY, siteUrls)
super.onSaveInstanceState(outState)
}

override fun onLoadUrl(url: String) {
fun String.matchesUrl(url: String) = when (navArgs.urlComparisonMode) {
PARTIAL -> url.contains(this, ignoreCase = true)
EQUALITY -> equals(url, ignoreCase = true)
}

if (isAdded && navArgs.urlToTriggerExit?.matchesUrl(url) == true) {
navigateBackWithNotice(WEBVIEW_RESULT)
extractSiteUrl(url)

lifecycleScope.launchWhenResumed {
Copy link
Contributor Author

@0nko 0nko Oct 23, 2022

Choose a reason for hiding this comment

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

This was necessary because the app kept crashing for me after a result was returned after site creation. Turns out the onLoadUrl() callback was still getting called even after that, which would the trigger the navigation again and crash.

Copy link
Member

@hichamboushaba hichamboushaba Oct 24, 2022

Choose a reason for hiding this comment

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

This will have an unwanted side effect, let's say the user started loading the page that we want to detect to close the screen after, then they navigated to another app, the listener then will ignore the URL that's being loaded, and we'll end up showing a page that the user is not expected to navigate to.

We have the isAdded check below to avoid this crash, I think since the app crashed for you, then it's not enough (maybe because fragment transactions are async), so as an alternative, we can simply use a boolean to track whether the navigation was triggered by the listener, and then use it to ignore all subsequent calls.

Copy link
Contributor Author

@0nko 0nko Oct 24, 2022

Choose a reason for hiding this comment

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

Good point! It's now fixed.

if (isAdded && navArgs.urlToTriggerExit?.matchesUrl(url) == true) {
if (siteUrls.isEmpty()) {
navigateBackWithNotice(WEBVIEW_RESULT)
} else {
navigateBackWithResult(WEBVIEW_RESULT_WITH_URL, siteUrls)
WooLog.d(T.LOGIN, "Site creation URLs: ${siteUrls.joinToString()}")
}
}
}
}

private fun extractSiteUrl(url: String) {
"$WEBVIEW_STORE_CHECKOUT_STRING.+/".toRegex().find(url)?.range?.let { range ->
val start = range.first + WEBVIEW_STORE_CHECKOUT_STRING.length
val end = range.last
val siteUrl = url.substring(start, end)
if (!siteUrls.contains(siteUrl)) {
siteUrls.add(siteUrl)
}
}
}

Expand Down
16 changes: 15 additions & 1 deletion WooCommerce/src/main/kotlin/com/woocommerce/android/ui/sitepicker/SitePickerFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import com.woocommerce.android.ui.sitepicker.SitePickerViewModel.SitePickerState
import com.woocommerce.android.ui.sitepicker.SitePickerViewModel.SitePickerState.WooNotFoundState
import com.woocommerce.android.ui.sitepicker.sitediscovery.SitePickerSiteDiscoveryFragment
import com.woocommerce.android.util.ChromeCustomTabUtils
import com.woocommerce.android.util.FeatureFlag
import com.woocommerce.android.viewmodel.MultiLiveEvent.Event.Exit
import com.woocommerce.android.viewmodel.MultiLiveEvent.Event.Logout
import com.woocommerce.android.viewmodel.MultiLiveEvent.Event.ShowDialog
Expand Down Expand Up @@ -92,6 +93,11 @@ class SitePickerFragment : BaseFragment(R.layout.fragment_site_picker), LoginEma
binding.loginEpilogueButtonBar.buttonSecondary.setOnClickListener {
viewModel.onTryAnotherAccountButtonClick()
}

binding.loginEpilogueButtonBar.buttonTertiary.isVisible = FeatureFlag.STORE_CREATION_WEBVIEW_FLOW.isEnabled()
binding.loginEpilogueButtonBar.buttonTertiary.setOnClickListener {
viewModel.onCreateSiteButtonClick()
}
}

@Suppress("LongMethod", "ComplexMethod")
Expand Down Expand Up @@ -176,6 +182,13 @@ class SitePickerFragment : BaseFragment(R.layout.fragment_site_picker), LoginEma
AnalyticsTracker.track(AnalyticsEvent.LOGIN_WOOCOMMERCE_SETUP_COMPLETED)
viewModel.onWooInstalled()
}
handleResult<List<String>>(WPComWebViewFragment.WEBVIEW_RESULT_WITH_URL) { urls ->
AnalyticsTracker.track(
AnalyticsEvent.LOGIN_WOOCOMMERCE_SITE_CREATED,
mapOf(AnalyticsTracker.KEY_URL to urls.joinToString())
)
viewModel.onSiteCreated(urls)
}
handleNotice(WPComWebViewFragment.WEBVIEW_DISMISSED) {
AnalyticsTracker.track(AnalyticsEvent.LOGIN_WOOCOMMERCE_SETUP_DISMISSED)
}
Expand Down Expand Up @@ -275,7 +288,8 @@ class SitePickerFragment : BaseFragment(R.layout.fragment_site_picker), LoginEma
findNavController().navigate(
NavGraphMainDirections.actionGlobalWPComWebViewFragment(
urlToLoad = event.url,
urlToTriggerExit = event.validationUrl
urlToTriggerExit = event.validationUrl,
title = event.title
)
)
}
Expand Down
64 changes: 61 additions & 3 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/sitepicker/SitePickerViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.woocommerce.android.ui.sitepicker.SitePickerViewModel.SitesListItem.N
import com.woocommerce.android.ui.sitepicker.SitePickerViewModel.SitesListItem.WooSiteUiModel
import com.woocommerce.android.util.FeatureFlag
import com.woocommerce.android.util.WooLog
import com.woocommerce.android.util.WooLog.T
import com.woocommerce.android.viewmodel.LiveDataDelegate
import com.woocommerce.android.viewmodel.MultiLiveEvent
import com.woocommerce.android.viewmodel.MultiLiveEvent.Event.Exit
Expand All @@ -38,8 +39,10 @@ import com.woocommerce.android.viewmodel.ResourceProvider
import com.woocommerce.android.viewmodel.ScopedViewModel
import com.woocommerce.android.viewmodel.navArgs
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import kotlinx.parcelize.Parcelize
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.network.rest.wpcom.wc.WooErrorType
Expand Down Expand Up @@ -67,6 +70,9 @@ class SitePickerViewModel @Inject constructor(
companion object {
private const val WOOCOMMERCE_INSTALLATION_URL = "https://wordpress.com/plugins/woocommerce/"
private const val WOOCOMMERCE_INSTALLATION_DONE_URL = "marketplace/thank-you/woocommerce"
private const val WOOCOMMERCE_STORE_CREATION_URL = "https://woocommerce.com/start"
private const val WOOCOMMERCE_STORE_CREATION_DONE_URL = "calypso/images/wpcom-ecommerce"
private const val WOOCOMMERCE_STORE_CREATION_RETRY_INTERVAL = 2000L
}

private val navArgs: SitePickerFragmentArgs by savedState.navArgs()
Expand All @@ -78,7 +84,7 @@ class SitePickerViewModel @Inject constructor(
val sites: LiveData<List<SitesListItem>> = _sites

private var loginSiteAddress: String?
get() = savedState.get("key") ?: appPrefsWrapper.getLoginSiteAddress()
get() = savedState["key"] ?: appPrefsWrapper.getLoginSiteAddress()
set(value) = savedState.set("key", value)

val shouldShowToolbar: Boolean
Expand Down Expand Up @@ -112,11 +118,12 @@ class SitePickerViewModel @Inject constructor(
}
}

private suspend fun fetchSitesFromApi(showSkeleton: Boolean) {
private suspend fun fetchSitesFromApi(showSkeleton: Boolean, delayTime: Long = 0) {
sitePickerViewState = sitePickerViewState.copy(
isSkeletonViewVisible = showSkeleton
)

delay(delayTime)
val startTime = System.currentTimeMillis()
val result = repository.fetchWooCommerceSites()
val duration = System.currentTimeMillis() - startTime
Expand Down Expand Up @@ -406,6 +413,17 @@ class SitePickerViewModel @Inject constructor(
triggerEvent(SitePickerEvent.NavigateToSiteAddressEvent)
}

fun onCreateSiteButtonClick() {
analyticsTrackerWrapper.track(AnalyticsEvent.SITE_PICKER_CREATE_SITE_TAPPED)
triggerEvent(
NavigateToWPComWebView(
url = WOOCOMMERCE_STORE_CREATION_URL,
validationUrl = WOOCOMMERCE_STORE_CREATION_DONE_URL,
title = resourceProvider.getString(string.create_new_store)
)
)
}

fun onTryAnotherAccountButtonClick() {
trackLoginEvent(clickEvent = UnifiedLoginTracker.Click.TRY_ANOTHER_ACCOUNT)
launch {
Expand Down Expand Up @@ -530,6 +548,42 @@ class SitePickerViewModel @Inject constructor(
}
}

fun onSiteCreated(urls: List<String>) {
launch {
sitePickerViewState = sitePickerViewState.copy(
isSkeletonViewVisible = false, isProgressDiaLogVisible = true
)

var newSite: SiteModel? = null
while (newSite?.hasWooCommerce != true) {
newSite = urls.firstNotNullOfOrNull { url -> repository.getSiteBySiteUrl(url) }
if (newSite != null && newSite.hasWooCommerce) {
Copy link
Contributor Author

@0nko 0nko Oct 23, 2022

Choose a reason for hiding this comment

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

When a new site is first fetched, the Woo plugin is still not ready and the store can't be selected. So we need to wait for the hasWooCommerce to be true.

onSiteSelected(newSite)
onContinueButtonClick(true)
WooLog.d(T.LOGIN, "Found new site: ${newSite.url}")
} else {
WooLog.d(T.LOGIN, "New site not found, retrying...")
val result = fetchSitesAfterCreation()
if (result.isFailure) {
triggerEvent(ShowSnackbar(string.site_picker_error))
break
} else {
displaySites(result.getOrDefault(emptyList()))
}
}
}
}
}

private suspend fun fetchSitesAfterCreation(): Result<List<SiteModel>> {
val result = withContext(Dispatchers.Default) {
delay(WOOCOMMERCE_STORE_CREATION_RETRY_INTERVAL)
repository.fetchWooCommerceSites()
}
return if (result.isError) Result.failure(Exception(result.error.message))
else Result.success(result.model ?: emptyList())
}

fun onWooInstalled() {
suspend fun fetchSite(site: SiteModel, retries: Int = 0): Result<SiteModel> {
delay(retries * TimeUnit.SECONDS.toMillis(2))
Expand Down Expand Up @@ -660,7 +714,11 @@ class SitePickerViewModel @Inject constructor(
object NavigateToNewToWooEvent : SitePickerEvent()
object NavigateToSiteAddressEvent : SitePickerEvent()
data class NavigateToHelpFragmentEvent(val origin: HelpActivity.Origin) : SitePickerEvent()
data class NavigateToWPComWebView(val url: String, val validationUrl: String) : SitePickerEvent()
data class NavigateToWPComWebView(
val url: String,
val validationUrl: String,
val title: String? = null
) : SitePickerEvent()
data class NavigateToAccountMismatchScreen(
val primaryButton: AccountMismatchPrimaryButton,
val siteUrl: String,
Expand Down
2 changes: 2 additions & 0 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ enum class FeatureFlag {
WC_SHIPPING_BANNER,
UNIFIED_ORDER_EDITING,
ORDER_CREATION_CUSTOMER_SEARCH,
STORE_CREATION_WEBVIEW_FLOW,
STORE_CREATION_FLOW;

fun isEnabled(context: Context? = null): Boolean {
Expand All @@ -29,6 +30,7 @@ enum class FeatureFlag {
MORE_MENU_INBOX,
WC_SHIPPING_BANNER,
STORE_CREATION_FLOW -> PackageUtils.isDebugBuild()
STORE_CREATION_WEBVIEW_FLOW -> PackageUtils.isDebugBuild()
Copy link
Member

@hichamboushaba hichamboushaba Oct 25, 2022

Choose a reason for hiding this comment

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

np, I think these two feature flags might be confusing, especially with how we are going to interact with them for the simplified login project.
I think we should updateSTORE_CREATION to be called ACCOUNT_CREATION, as this is what's really controls right now.

}
}
}
12 changes: 12 additions & 0 deletions WooCommerce/src/main/res/layout-land/view_login_epilogue_button_bar.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,16 @@
android:text="@string/login_view_connected_stores"
android:textAllCaps="false"
tools:visibility="visible"/>

<com.google.android.material.button.MaterialButton
android:id="@+id/button_tertiary"
style="@style/Woo.Button.Outlined"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_weight="1"
android:layout_marginStart="@dimen/minor_100"
android:layout_marginEnd="@dimen/minor_00"
android:text="@string/create_new_store"
android:textAllCaps="false"
tools:visibility="visible"/>
</LinearLayout>
9 changes: 9 additions & 0 deletions WooCommerce/src/main/res/layout/view_login_epilogue_button_bar.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,13 @@
android:text="@string/login_view_connected_stores"
android:textAllCaps="false"
tools:visibility="visible"/>

<com.google.android.material.button.MaterialButton
android:id="@+id/button_tertiary"
style="@style/Woo.Button.Outlined"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="@string/create_new_store"
android:textAllCaps="false"
tools:visibility="visible"/>
</LinearLayout>
3 changes: 3 additions & 0 deletions WooCommerce/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2647,4 +2647,7 @@
<string name="stats_widget_last_updated_message">As of %1$s</string>
<!-- TODO: Remove or change this placeholder text -->
<string name="hello_blank_fragment">Hello blank fragment</string>

<!-- Store creation -->
<string name="create_new_store">Create a new store</string>
</resources>