[POS as a tab i2] Move i1 eligibility checker to `LegacyPOSTabEligibilityChecker` with i1 only implementation #15864

jaclync · 2025-07-03T18:10:43Z

Just one review is required.

Now that the differences between POS as a tab i1 and i2 result in more code changes, I am working on a 2-part refactoring with the ultimate goal to remove cases for the tab visibility check from the POS ineligible enum in WOOMOB-756 while not affecting i1 significantly. In retrospect, I could have created a separate POSTabEligibilityChecker for i1/i2 at the beginning with the tradeoff of some duplicate code. Before making more changes that touch on i1, I decided to update POSTabEligibilityChecker to only contain the i2 logic, and move the i1 only logic to LegacyPOSTabEligibilityChecker. i2 only implementation will be in the next PR.

To hopefully make it easier to understand the differences between i1 and i2:

milestone	POS tab visibility check	POS eligibility check
i1	all conditions: tablet, iOS 17+, store country, store currency, remote feature flag, WC plugin version, POS feature switch for WC version 10.0+	none
i2	tablet, store country, remote feature flag	iOS 17+, store currency, WC plugin version, POS feature switch for WC version 10.0+

This PR focuses on duplicating POSTabEligibilityChecker to LegacyPOSTabEligibilityChecker, then updating LegacyPOSTabEligibilityChecker to be just i1 implementation. Same for the unit tests.

Steps to reproduce

POS tab visibility/eligibility behavior should be the same as before:

Store eligible for POS

Log in to a store eligible for POS --> the POS tab should be shown after a bit

Store in eligible country but not eligible for POS

Log in to a store in eligible country but not eligible for POS --> the POS tab should be shown after a bit
Tap on the POS tab --> after loading state, an ineligible UI should be shown based on the reason

Store not in eligible country

Log in to a store not in eligible country --> the POS tab should not be shown even after a while

Testing information

@jaclync tests logging in
@jaclync tests switching stores
@jaclync tests relaunching app

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

wpmobilebot · 2025-07-03T18:19:48Z

📲 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 Name	WooCommerce iOS Prototype
	Build Number	`pr15864-2a024c3`
	Version	`22.7`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`2a024c3`
	Installation URL	3p03nqr6797eg

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

…STabEligibilityCheckerI2` for i2 that still contains i1 logic.

…`POSTabEligibilityChecker` for i1.

…arated to another PR for easier review.

joshheald

Works as expected, some minor suggestions/questions inline but nothing blocking, all up to you.

joshheald · 2025-07-04T15:22:26Z

WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift

-    private var siteSettingsEligibility: POSEligibilityState?
-    private var featureFlagEligibility: POSEligibilityState?
+/// Legacy enum containing POS invisible reasons + POSIneligibleReason cases for i1.
+private enum LegacyPOSIneligibleReason: Equatable {


Good choice making the duplicate the legacy, for source control history purposes.

joshheald · 2025-07-04T15:30:31Z

WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift

+}
+
+/// POS tab eligibility checker for i1. Will be replaced by `POSTabEligibilityCheckerI2` when removing `pointOfSaleAsATabi2` feature flag.
+final class POSTabEligibilityChecker: POSEntryPointEligibilityCheckerProtocol {


It's not the biggest problem to do it this way around, but source history for the code we keep can be (more) linear if you were to duplicate POSTabEligibilityChecker to a new LegacyPOSTabEligibilityChecker file now, and make the changes there. Then update POSTabEligibilityChecker to have the i2 rules.

The downside is that you'd need to update the callsite to use LegacyPOSTabEligibilityChecker for now.

Not a must-have – it's not as though we'll make a big problem with the history doing it this way.

I was debating the two approaches, but went the other way because of the large first PR and just kept going. But great point on keeping the git history more granular if we move i1 to a separate class and make changes to i2 in POSTabEligibilityChecker. LegacyPOSTabEligibilityChecker is a better fit for i1 too since the internal enums are already named Legacy*. It wasn't a hard change, so I went with your suggestion with just moving file content without code changes:

Copy and paste the current POSTabEligibilityChecker from this branch (i1 only, reviewed in this PR) to LegacyPOSTabEligibilityChecker. Same for the tests.

Copy and paste the POSTabEligibilityChecker in trunk (i1 + i2) to POSTabEligibilityChecker in this branch. Same for the tests. There are no diffs on these two files now.

Later in POSTabEligibilityCheckerI2: i2 logic #15865, the POSTabEligibilityCheckerI2 content (i2 only, reviewed in that PR) will be copied to POSTabEligibilityChecker. Same for the tests.

joshheald · 2025-07-04T15:32:20Z

WooCommerce/WooCommerceTests/POS/Controllers/POSEntryPointControllerTests.swift


 struct POSEntryPointControllerTests {
    @available(iOS 17.0, *)
    @Test func eligibilityState_is_set_to_eligible_when_i2_feature_is_disabled() async throws {


This test is a bit confusing... why do we set it to eligible if the feature's disabled?

Yea the test case name sounds confusing now that I read it a bit after implementation. This is for the behavior of directly returning eligible for POS entry point for i1 since the store already meets the eligibility check if POS tab is visible / being tapped.

I updated the name to eligibilityState_is_always_eligible_when_i2_feature_is_disabled_regardless_of_eligibility_checker in 24412fc, hopefully it's easier to read.

…ker*` for i1 implementation.

…t is separated to another PR for easier review." This reverts commit 9e611a7.

…cker` for i1.

…erTests`.

…unk. Will be modified to i2 in the next PR.

jaclync · 2025-07-04T19:36:52Z

Thanks for the review and suggestions, Josh! I updated the PR and the description with most changes in #15864 (comment) moving content around, tested again, and I am going to merge it.

Remove POS ineligible cases that are moved to visibility checks in i2.

94edda0

jaclync added type: task An internally driven task. feature: POS labels Jul 3, 2025

jaclync added this to the 22.8 milestone Jul 3, 2025

jaclync added 6 commits July 3, 2025 15:01

Revert POSTabEligibilityChecker back to i1 specific, and create `PO…

d22a9b9

…STabEligibilityCheckerI2` for i2 that still contains i1 logic.

Remove i1 logic from POSTabEligibilityCheckerI2 and revert test cases.

0ffe2f9

Update POSTabEligibilityCheckerTests to only contain test cases on …

c75d809

…`POSTabEligibilityChecker` for i1.

Revert removal of a comment to minimize diffs.

b0059a6

Revert changes to use POSTabEligibilityCheckerI2 now that it is sep…

9e611a7

…arated to another PR for easier review.

Remove i2 files for a separate PR.

14afb3d

jaclync changed the title ~~Refactor POSIneligibleReason enum and simplify visibility logic~~ [POS as a tab i2] Temporarily disable i2 to refactor POSTabEligibilityChecker to i1 implementation only Jul 3, 2025

jaclync changed the title ~~[POS as a tab i2] Temporarily disable i2 to refactor POSTabEligibilityChecker to i1 implementation only~~ [POS as a tab i2] Temporarily disable i2 from refactoring POSTabEligibilityChecker to i1 implementation only Jul 3, 2025

jaclync requested review from iamgabrielma, joshheald and staskus July 3, 2025 20:35

jaclync mentioned this pull request Jul 3, 2025

POSTabEligibilityCheckerI2: i2 logic #15865

Merged

3 tasks

joshheald self-assigned this Jul 4, 2025

joshheald approved these changes Jul 4, 2025

View reviewed changes

jaclync added 8 commits July 4, 2025 14:45

Duplicate POSTabEligibilityChecker* to `LegacyPOSTabEligibilityChec…

10cbc7d

…ker*` for i1 implementation.

Revert "Revert changes to use POSTabEligibilityCheckerI2 now that i…

20ecee2

…t is separated to another PR for easier review." This reverts commit 9e611a7.

Use POSTabEligibilityChecker for i2 and `LegacyPOSTabEligibilityChe…

2bf4da7

…cker` for i1.

Fix incorrect implementation of LegacyPOSTabEligibilityCheckerTests.

a9c8b9a

Use LegacyPOSTabEligibilityChecker in `LegacyPOSTabEligibilityCheck…

f3b84c8

…erTests`.

Revert POSTabEligibilityChecker changes to be mostly the same as tr…

e3f2e94

…unk. Will be modified to i2 in the next PR.

Rename test case name to hopefully make it more readable.

24412fc

POSTabEligibilityCheckerTests: copy content from trunk to branch.

2a024c3

jaclync changed the title ~~[POS as a tab i2] Temporarily disable i2 from refactoring POSTabEligibilityChecker to i1 implementation only~~ [POS as a tab i2] Move i1 eligibility checker to LegacyPOSTabEligibilityChecker with i1 only implementation Jul 4, 2025

jaclync merged commit da958db into trunk Jul 4, 2025
15 checks passed

jaclync deleted the feat/WOOMOB-756-separate-ineligible-enum-to-visibility-and-eligibility branch July 4, 2025 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[POS as a tab i2] Move i1 eligibility checker to `LegacyPOSTabEligibilityChecker` with i1 only implementation #15864

[POS as a tab i2] Move i1 eligibility checker to `LegacyPOSTabEligibilityChecker` with i1 only implementation #15864

Uh oh!

jaclync commented Jul 3, 2025 •

edited

Loading

Uh oh!

wpmobilebot commented Jul 3, 2025 •

edited

Loading

Uh oh!

joshheald left a comment

Uh oh!

joshheald Jul 4, 2025

Uh oh!

joshheald Jul 4, 2025

Uh oh!

jaclync Jul 4, 2025

Uh oh!

joshheald Jul 4, 2025

Uh oh!

jaclync Jul 4, 2025

Uh oh!

jaclync commented Jul 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[POS as a tab i2] Move i1 eligibility checker to LegacyPOSTabEligibilityChecker with i1 only implementation #15864

[POS as a tab i2] Move i1 eligibility checker to LegacyPOSTabEligibilityChecker with i1 only implementation #15864

Uh oh!

Conversation

jaclync commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to reproduce

Store eligible for POS

Store in eligible country but not eligible for POS

Store not in eligible country

Testing information

Uh oh!

wpmobilebot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshheald left a comment

Choose a reason for hiding this comment

Uh oh!

joshheald Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

joshheald Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

joshheald Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync commented Jul 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[POS as a tab i2] Move i1 eligibility checker to `LegacyPOSTabEligibilityChecker` with i1 only implementation #15864

[POS as a tab i2] Move i1 eligibility checker to `LegacyPOSTabEligibilityChecker` with i1 only implementation #15864

jaclync commented Jul 3, 2025 •

edited

Loading

wpmobilebot commented Jul 3, 2025 •

edited

Loading