Skip to content

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Jun 10, 2025

Closes WOOMOB-584

Just one review is required.

Apology for the larger diffs, ~400 diffs are about unit test changes on MainTabBarController. A few cases in MainTabBarControllerTests were flaky from CI (example in the Tests tab even though CI passed) and locally when running the test class repeatedly for 10+ times. When a test case has waitUntil, if the main thread is occupied by other operations like from leftover previous test cases, waitUntil can become unresponsive and not be able to check the condition as expected. From debugging for a few hours, I noticed that instances of MainTabBarController from previous test cases were not deallocated. And because of a combination of ServiceLocator usage and the shared SessionManager database in tests, the site ID from previous and current test cases could trigger site ID changes observed in MainTabBarController in the unintended instance (e.g. the current MainTabBarController instance is receiving site ID from a previous test case, or previous instances are receiving site IDs from the current test case). I ended up separating a few test cases on the tabs behavior to a new test class, and with a few changes I was able to run both test classes repeatedly successfully. Please feel free to share any tips on any alternatives of waitUntil or the test setup.

Description

This pull request introduces a new feature to display the Point of Sale (POS) as a tab in the tab bar, contingent on a feature flag and user eligibility. It includes updates to feature flag handling, POS tab integration, and adjustments to existing tab bar functionality. Below is a summary of the most significant changes grouped by theme.

Feature Flag Updates

  • Added a new feature flag, pointOfSaleAsATabi1, to control the release of the POS tab. (Modules/Sources/Experiments/FeatureFlag.swift - [1] Modules/Sources/Experiments/DefaultFeatureFlagService.swift - [2]

POS Tab Integration

  • Implemented POSTabCoordinator and POSTabViewController to manage the behavior and presentation of the POS tab and the full-screen POS UI. (WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift - WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swiftR1-R152)
  • Updated WooTab enum to include the pointOfSale case and added logic to manage its visibility dynamically based on eligibility. (WooCommerce/Classes/ViewRelated/MainTabBarController.swift - [1] [2]

Tab Bar Functionality Adjustments

Hub Menu Updates

  • Adjusted HubMenuCoordinator and HubMenuViewModel to account for the new POS tab feature flag and ensure compatibility with the updated tab bar logic. (WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuCoordinator.swift - [1] WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuViewModel.swift - [2]

Steps to reproduce

Prerequisite: a WPCOM account that has at least two connected stores, one eligible for POS and the other one not eligible.

  • Log out if needed
  • Log in to the account in the prerequisite with the store that is eligible for POS --> after logging in, the POS tab should appear shortly
  • Go to the Menu tab --> there should be no POS entry point even after some wait
  • Tap on the POS tab --> it should launch POS in full-screen
  • Feel free to add item(s) to cart and complete a cash/card payment
  • Exit POS
  • Go to Menu tab and switch to a store ineligible for POS --> after switching stores, the POS tab should not appear even after a while
  • Go to the Menu tab --> there should be no POS entry point even after some wait
  • Switch to a store eligible for POS again --> after switching stores, the POS tab should appear shortly
  • Tap on the POS tab --> it should launch POS in full-screen
  • Exit POS
  • Relaunch the app --> the POS tab should be hidden first, then appears shortly

Screenshots

Only the "Sweet Maple Store" is not eligible for POS.

Simulator.Screen.Recording.-.iPad.A16.-.2025-06-13.at.13.38.00.mp4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@jaclync jaclync added this to the 22.7 milestone Jun 10, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Jun 10, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 10, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Number30415
VersionPR #15726
Bundle IDcom.automattic.alpha.woocommerce
Commit214e1e3
Installation URL3ujl4uhrjh6qg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync changed the base branch from trunk to feat/WOOMOB-584-pos-tab-eligibility-checker June 11, 2025 07:30
@jaclync jaclync changed the title [POS as a tab] Show POS tab for eligible stores behind a feature flag [POS as a tab i1] Show POS tab for eligible stores behind a feature flag Jun 11, 2025
Base automatically changed from feat/WOOMOB-584-pos-tab-eligibility-checker to trunk June 13, 2025 01:21
…nt leftover main thread work from previous test cases from singletons.
@jaclync jaclync marked this pull request as ready for review June 13, 2025 05:43
@joshheald joshheald self-assigned this Jun 13, 2025
Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

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

Works as described. I tested on an iPad running iOS 17.7.2, and an iPhone running 18.5, and an iPad simulator running iOS 16.

I tested with various combinations of currency, country, device type, and OS – the tab shows when it should and doesn't when it shouldn't.

I checked the analytics events were still correctly annotated, and that card payments are still correctly cancelled/order state reset when leaving POS.

I'm just wondering whether/when we plan to make the tab bar stable again?

I do think it's worth delaying the load until we can determine eligibility – a jumpy UI doesn't give a good impression of the app.

If we really can't do that now, it might be a bit better if we could animate the insertion of the tab.


configureTabViewControllers()
// POS tab is hidden by default.
updateTabViewControllers(isPOSTabVisible: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you investigated whether we can avoid this? Is it planned for a later iteration?

It would really be preferable to avoid the tab bar jumping around like this... if we release it this way it's kind of annoying and doesn't seem like the best impression for POS!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In i2 from the delivery plan pdfdoF-7po-p2, the POS tab visibility check is reduced to two API requests:

  • Remote feature flag: as we discussed in the team meeting, we're keeping this check in case of critical POS issues for certain app versions
  • General WC site settings for store country: ideally we can reuse the site settings fetched from other API requests made during site initialization

For i1 / initial implementation, we're hiding the POS tab by default just so that it isn't shown for ineligible stores pdfdoF-7mo-p2#ui. But as you and Povilas mentioned during PR testing, plus my own testing, I'm not liking this UX. In my next task WOOMOB-599, I'm exploring caching the POS eligibility per site and using the cached value while checking eligibility async. For most of the sites, this value is hopefully rarely changed.

@jaclync
Copy link
Contributor Author

jaclync commented Jun 16, 2025

Thank you for the thorough testing Josh!

I'm just wondering whether/when we plan to make the tab bar stable again?

I do think it's worth delaying the load until we can determine eligibility – a jumpy UI doesn't give a good impression of the app.

I personally don't think it's worth delaying the tab bar initial state just for POS eligibility checks, at least for i1 with eligibility dependence on several API requests. Most of the merchants aren't using POS at this point. The current eligibility delay is noticeable and will make the app feel slower with potential feedback in reviews and support tickets. For i2 maybe, or we can consider always showing the POS tab for all stores and inform them about the ineligibility reason. In this case, a setting to hide the POS tab in the app might help with the UX like for stores in ineligible countries. WDYT?

If we really can't do that now, it might be a bit better if we could animate the insertion of the tab.

Great suggestion, I haven't thought about animating the tab changes and will look into it this week.

@jaclync jaclync merged commit 281f7f1 into trunk Jun 16, 2025
20 checks passed
@jaclync jaclync deleted the feat/WOOMOB-567-pos-tab branch June 16, 2025 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants