-
Notifications
You must be signed in to change notification settings - Fork 134
[POS] Better error handling for eligibility #14463
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
base: trunk
Are you sure you want to change the base?
Conversation
…e when we couldn't determine the value
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
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.
Thanks for working on this @toupper! I've reviewed the code and left some questions/suggestions. I haven't tested the behavior yet. Let me know what you think and I'll test it when we resolve these discussions.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosCanBeLaunchedInTab.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosCanBeLaunchedInTab.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosTabController.kt
Outdated
Show resolved
Hide resolved
...rce/src/main/kotlin/com/woocommerce/android/ui/woopos/util/analytics/WooPosAnalyticsEvent.kt
Show resolved
Hide resolved
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/WooPOSIsRemotelyEnabledTest.kt
Outdated
Show resolved
Hide resolved
Thanks for your comments @malinajirka! I addressed them and refactored the code a bit to make it cleaner and easier to read. Please let me know what you think |
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.
Thanks for all the changes! The tests are failing, other than that LGTM . I've left question but feel free to ignore it.
} ?: return WooPosLaunchability.NotLaunchable( | ||
WooPosLaunchability.NonLaunchabilityReason.WooCommercePluginNotFound | ||
) | ||
val cachedPositive = appPrefs.isPOSLaunchableForSite(site.id) |
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 wondering if this should be set to false when forceRefresh
is true, wdyt?
} | ||
|
||
@Test | ||
fun `given feature flag disabled, when invoked, then return false`() = testBlocking { | ||
fun `given feature flag disabled, when invoked, then return success true (tab visible anyway)`() = testBlocking { |
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.
❓ Is this test correct, why do we want to return tabVisible when remote FF is disabled?
Closes WOOMOB-1016
Only one review required, but I add you both as reviewers in case you are interested in the solution.
Description
With this PR we improve the logic for handling errors when opening POS, including when there is no connection. We implement this logic:
POS tab eligibility
When entering POS tab and eligibility check errors again:
For that we:
AppPrefs
to cache also whether POS was launchable. We also update the tab logic to only make it possible to cache positive cases.WooPOSIsRemotelyEnabled
,WooPosCanBeLaunchedInTab
andWooPosTabShouldBeVisible
to return a result instead of a booleanWooPosCanBeLaunchedInTab
with the new logic, including setting the value in cache.Steps to reproduce
With all conditions met:
With the POS feature switch disabled in wp-admin:
With a store in a country other than US or UK:
Testing information
The tests that have been performed
See above
Images/gif
On this video we can see how the tab is shown in airplane mode because it was shown before, but given that we never cached whether the POS was launched, we prevent accessing it:
Screen_Recording_20250813_143918_Woo.Pre-Alpha.mp4
Here we show how if previously the user accessed POS, it can access it even if there's no connection:
Screen_Recording_20250813_144433_Woo.Pre-Alpha.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.