-
Notifications
You must be signed in to change notification settings - Fork 130
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
[Products] Use Authenticated WebView for previewing products #13494
base: issue/13467-improve-authenticated-webview-3
Are you sure you want to change the base?
[Products] Use Authenticated WebView for previewing products #13494
Conversation
Generated by 🚫 Danger |
<action | ||
android:id="@+id/action_global_AuthenticatedWebViewFragment" | ||
app:destination="@id/AuthenticatedWebViewFragment" /> |
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.
Because the multi-pane setup, we previously duplicated the destination in this nested navgraph, this is because when the app is using two panes, the when calling findNavController
from the details pane, it will return the nested NavController and not the main one, and this one won't know about destinations declared in the main nav graph.
With the new extension findNavController(@IdRes navHostId: Int)
, we don't need this duplication, and the logic is simpler. This also avoids the issues of this duplication: making the maintenance harder, because if we needed to change the list of arguments for the destination, then we need to do it for every nav graph that defines, or we might get crypted build failures or worse runtime crashes (check this PR for an example).
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
@@ -215,7 +214,7 @@ class EditShippingLabelPaymentFragment : | |||
} | |||
|
|||
private fun setupResultHandlers() { | |||
handleNotice(AuthenticatedWebViewFragment.WEBVIEW_RESULT) { | |||
handleNotice(AuthenticatedWebViewFragment.WEBVIEW_RESULT, navHostId = R.id.nav_host_fragment_main) { |
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.
We need to set the navHostId for the result handler too to make sure we are using the correct NavController.
ffadbf6
to
9ca142e
Compare
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
9ca142e
to
f3a63c0
Compare
f3a63c0
to
3c7010d
Compare
d04c5c1
to
80506c6
Compare
The button will now be shown when we can authenticate the user in the WebView even if the product is not public
The extension is useful when the app is in two-pane layout, as it allows child fragment to retrieve the parent NavController.
3c7010d
to
9b387c6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## issue/13467-improve-authenticated-webview-3 #13494 +/- ##
=================================================================================
- Coverage 37.93% 37.93% -0.01%
- Complexity 8986 8987 +1
=================================================================================
Files 2054 2054
Lines 112180 112203 +23
Branches 14179 14185 +6
=================================================================================
+ Hits 42558 42564 +6
- Misses 65730 65745 +15
- Partials 3892 3894 +2 ☔ View full report in Codecov by Sentry. |
Closes: #13353
Note
Depends on #13505
Description
Currently, when the user taps on the "View Product On Store", we open a Chrome Custom Tab, and given we can't ensure that the user is authenticated in the custom tab, then there could be some issues: if the product is not published or the store is not public.
So far, we have been hiding the button when the product is not public, but the issue was still present if the store is not public (either using WPCom visibility settings, or the new "coming soon" mode of Woo Core), and this PR attempts to improve the situation by using the Authenticated WebView whenever we can authenticate the user.
Given that the Product Details fragment can be hosted in a nested NavHostFragment (when the app is using a multi-pane layout), this PR adds a new way for handling the navigation from nested child fragments to destinations that are part of the root NavGraph, more details about this below in the comments.
Important: This doesn't solve all cases, because the issue will still be present for sites that don't support auto-autentication, and for this we are planning to research an alternative solution using a new API in the future.
Steps to reproduce
TC1: Can auto authenticate
TC2: Can't auto authenticate
TC3: Confirm non-regression for shipping labels payment screen
Testing information
The tests that have been performed
^
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: