-
Notifications
You must be signed in to change notification settings - Fork 132
[POS as a Tab] Tab & Visibility #14183
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
Generated by 🚫 Danger |
📲 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.
|
@toupper Nice progress with small amount of changes! From the screencasts, it looks like the POS tab is displayed right after logging in / switching stores, is it because it's shown by default, or the eligibility check results are cached for each connected site? Or the internet was so fast that all API calls from site settings (for store country/currency), remote feature flag, active plugins just returned with minimal delay? In iOS, I'm exploring ways to optimize the POS eligibility check for the POS tab so that we can show the tab as soon as possible. This includes caching the eligibility result for each site for like 3 days in WOOMOB-599. |
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.
Pull Request Overview
This PR adds a “Point of Sale” tab to the bottom navigation, controlled by a feature flag and POS availability, and integrates its visibility and navigation handling into the main activity.
- Adds a new string and menu item for the POS tab
- Introduces
WooPosTabController
& feature-flag helper - Integrates setup and visibility refresh into
MainActivity
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
WooCommerce/src/main/res/values/strings.xml | Added point_of_sale string |
WooCommerce/src/main/res/menu/menu_bottom_bar.xml | Added POS menu item entry |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosTabController.kt | New controller managing POS tab visibility & navigation |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosIsPOSAsATabEnabled.kt | Added feature-flag helper for POS tab |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/main/MainActivity.kt | Injected and wired up POS tab controller calls |
Comments suppressed due to low confidence (2)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosIsPOSAsATabEnabled.kt:6
- [nitpick] The class name is verbose and repeats 'POS'. Consider renaming to
IsWooPosTabEnabled
orWooPosTabEnabledChecker
for clarity and consistency with other feature-flag helpers.
class WooPosIsPOSAsATabEnabled @Inject constructor() {
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/tab/WooPosTabController.kt:51
- There are no unit tests covering the visibility logic in
refreshPOSTabVisibility
andupdatePOSTabVisibility
. Consider adding tests to verify the tab shows and hides correctly based on feature flag and POS availability.
fun refreshPOSTabVisibility() {
@@ -388,6 +394,8 @@ class MainActivity : | |||
// Track if App was opened from a widget | |||
trackIfOpenedFromWidget() | |||
|
|||
posTabController.refreshPOSTabVisibility() |
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.
To handle returning from background and app resume events, consider calling refreshPOSTabVisibility()
in onResume
or onStart
rather than only during onCreate
, matching the intended visibility update triggers.
Copilot uses AI. Check for mistakes.
binding.bottomNav.setOnItemSelectedListener { item -> | ||
when (item.itemId) { | ||
R.id.point_of_sale -> { | ||
activity.startActivity(Intent(activity, WooPosActivity::class.java)) | ||
false // return false to *not* keep the tab selected | ||
} | ||
else -> NavigationUI.onNavDestinationSelected(item, navController) |
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.
[nitpick] Overriding the entire BottomNavigation listener may unintentionally remove existing behaviors. Consider preserving the original listener for non-POS items or wrapping it so you only inject your POS case.
binding.bottomNav.setOnItemSelectedListener { item -> | |
when (item.itemId) { | |
R.id.point_of_sale -> { | |
activity.startActivity(Intent(activity, WooPosActivity::class.java)) | |
false // return false to *not* keep the tab selected | |
} | |
else -> NavigationUI.onNavDestinationSelected(item, navController) | |
val originalListener = binding.bottomNav.onItemSelectedListener | |
binding.bottomNav.setOnItemSelectedListener { item -> | |
when (item.itemId) { | |
R.id.point_of_sale -> { | |
activity.startActivity(Intent(activity, WooPosActivity::class.java)) | |
false // return false to *not* keep the tab selected | |
} | |
else -> { | |
originalListener?.onNavigationItemSelected(item) | |
NavigationUI.onNavDestinationSelected(item, navController) | |
} |
Copilot uses AI. Check for mistakes.
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.
@toupper Great job, the PR seems short but I'm sure this one was tricky to figure out!
I haven't tested the app yet as I left a few comments which might result in changes and a need for a re-test. Let me know what you think about them - the only one I believe we should fix is the database access on the UI thread, the others are more of a food for thought.
import com.woocommerce.android.util.FeatureFlag | ||
import javax.inject.Inject | ||
|
||
class WooPosIsPOSAsATabEnabled @Inject constructor() { |
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.
🔍 POS
might be cleaner with camel case Pos
. I'd also consider dropping the article A
, we usually don't include it in the names even though it's dramatically incorrect => WooPosIsPosAsTabEnabled. Wdyt?
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.
Good point, done in e0769c1
|
||
private fun updatePOSTabVisibility() { | ||
activity.lifecycleScope.launch { | ||
setPOSTabVisibility(isWooPosEnabled()) |
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.
GetWooCorePluginCachedVersion
doesn't change the dispatcher and it hits the database. Similarly woocommerceStore.getSiteSettings also hits the database.
I see three options:
- Change the coroutine context here to the IO dispatcher.
- Change the coroutine context in the
WooPosIsEnabled.invoke()
. (The solution I'd personally go with here) - Or the most correct but the most time consuming approach, update all the coresponding sites and change the context as close to the time consuming operation as possible (eg. right before we hit the database).
Wdyt? Have I missed something or it truly runs on the UI thread?
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.
Good point, I implemented option 2 in 2aab706
@@ -388,6 +394,8 @@ class MainActivity : | |||
// Track if App was opened from a widget | |||
trackIfOpenedFromWidget() | |||
|
|||
posTabController.refreshPOSTabVisibility() |
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 think we might want to consider registering the WooPosTabController to the activity lifecycle and calling this method within the WooPosTabController. I think we should be able to move there even the two methods we invoke in onCreate
, ultimately keeping the MainActivity completely clean of the controller's logic -> all the logic would be in the controller and the only code in the activity would be the registration for the lifecycle. Wdyt?
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.
That's great, I didn't know about that, thanks a lot! Done in a75db86
@@ -725,6 +733,7 @@ class MainActivity : | |||
) | |||
finish() | |||
startActivity(intent) | |||
posTabController.refreshPOSTabVisibility() |
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.
❓ Honest question as I haven't tested this and I'm not sure. Is this truly necessary even though we invoke the same method in onResume
? I'd expect both onCreate and onResume should be called when we finish the activity and start a new one 🤔.
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.
Indeed that works, done in a75db86
…o be called manually from MainActivity
…ther POS is enabled
Thanks for your message @jaclync!
It was still showing the previous value, but that wasn't correct, so I implement hiding it by default here a4decdf. In next PRs we can consider caching the value as you suggested. |
Got it, I created an issue for caching the eligibility value in Android in WOOMOB-620. |
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.
@toupper Thanks for the changes, I believe the encapsulation will help with long-term maintenance. However, the performance changes seem to be affecting the behavior - there is some race condition. The Tab isn't visible after login or after (some) site switches.
I added some logs and it seems the WooIsPosEnabled doesn't have access to the site settings and therefore returns false:
11:44:25.734 D WooPosTabController: POS as tab enabled, updating visibility
11:44:25.734 D WooPosTabController: updatePOSTabVisibility - checking isWooPosEnabled
11:44:25.734 D WooPosIsEnabled: checking if POS is enabled
11:44:25.748 D WooPosIsEnabled: WooCommerce version 9.8.5 supports product filtering: true
11:44:25.753 D WooPosIsEnabled: WooCommerce version 9.8.5 supports feature switch: false
11:44:25.755 D WooPosIsEnabled: Failed - couldn't retrieve site settings
11:44:25.824 D WooPosTabController: isWooPosEnabled returned: false
11:44:25.824 D WooPosTabController: Setting POS tab visibility to false
11:44:25.824 D WooPosTabController: POS menu item found: true, visibility set to false
It seems it currently loads the siteSettings from the local cache and if it's not there, it fails. I think we might need to either add some delay and check if the site settings is available after the delay and/or fetch the site settings if they are not available.
This issue was likely present even before, but it was almost impossible to reproduce since the user would need to switch to More
menu very quickly, before the siteSettings request finishes.
Patch with logs
Logs_for_POS_tab.patch
Description
With this PR, we add a tab for POS and show it when:
When visibility is updated
We update the POS tab visibility in the following cases:
Navigation behavior
When the POS tab is tapped:
Steps to reproduce
On app startup and coming to foreground, when logged in
Open the app with a site where POS is enabled. The tab should be visible.
Open the app with a site where POS is not enabled. The tab should not be visible.
On site changed
Change your site in menu and check that the tab visibility is updated according to the the new site POS availability.
On login
Login and check that the tab visibility is shoen according to the the login site POS availability.
Testing information
See above
The tests that have been performed
See above
Images/gif
Switching sites
Screen_Recording_20250612_095737_Woo.Pre-Alpha.mp4
Login
Screen_Recording_20250612_095852_Woo.Pre-Alpha.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.