[Woo POS] Coupons: Enable site coupons when tap on CTA #15478

iamgabrielma · 2025-04-03T07:05:54Z

Description

This PR adds the ability of enabling the site's coupons when we tap the CTA. At the moment we're still using the full-screen error view to handle loading errors for products and coupons, but from the latests designs it seems we'll be moving to a partial screen (pdfdoF-6PX#comment-7945-p2) so I focused on the functionality and reusing the error view over the UI details.

Screen.Recording.2025-04-03.at.13.56.36.mov

Testing information

Disable coupons in your site
Load POS, navigate to coupons
Tap on Enable, wait a moment (loading state to be handled)
Observe Coupons being loaded

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

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:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

wpmobilebot · 2025-04-03T07:17:54Z

📲 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	`29234`
	Version	`PR #15478`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`b85c7bc`
	Installation URL	0qf65mut3ohug

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

staskus

Thanks!

I left the logic about the dashboard view state.

Could you create the remaining tasks in Linear so we wouldn't miss any details to properly implement error, empty, and disabled states according to the designs and with the localized copy?

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

staskus · 2025-04-03T15:41:22Z

@iamgabrielma I suggest checking, reviewing, testing, and merging [Woo POS] Restore checkStoreCouponSettings to PointOfSaleCouponService before proceeding to avoid additional conflicts.

Also worth considering consistent place to call settings. We do it within PointOfSaleCouponService so we could continue doing it as well for enabling them 👍 .

iamgabrielma · 2025-04-04T04:23:24Z

I suggest checking, reviewing, testing, and merging #15481 before proceeding to avoid additional conflicts.

Sounds good, I'll do that first as otherwise working on this one will become messy, then apply your suggestions.

Could you create the remaining tasks in Linear so we wouldn't miss any details to properly implement error, empty, and disabled states according to the designs and with the localized copy?

Sure, I see you've already created these, I've added WOOMOB-253 so we don't forget to localize, or I'll do it just here when addressing the rest of changes.

iamgabrielma · 2025-04-04T08:06:28Z

This is ready for another pass when you have the chance @staskus

I've moved the call to settings to the coupons service: a2283f2
Added an internal comment to follow-up with loading state 0ffcc10
Added missing localizations to errors, we can edit them once we have the final copy: fac5a07

staskus

Thanks! Looks good 👍

It would be good to add a couple of missing pieces so the solution would be full and only UI missing:

Handle in controller when service fails to enable coupons
Add tests

Thanks 👍

staskus · 2025-04-04T09:15:23Z

WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift


-
+@available(iOS 17.0, *)
+extension PointOfSaleItemsControllerProtocol {


Do we need this extension with default implementation?

We do unfortunately, since we need to call couponsController.enableCoupons() in the aggregate, and this is a specialization of couponsController PointOfSaleItemsControllerProtocol

Oh, I didn't realize that at first. I don't love that ItemController gets affected this way.

This is what we could do:

Create PointOfSaleCouponsControllerProtocol which extends PointOfSaleItemsControllerProtocol:

protocol PointOfSaleCouponsControllerProtocol: PointOfSaleItemsControllerProtocol { /// Enables coupons in store settings, if needed func enableCoupons() async }

Implement it final class PointOfSaleCouponsController: PointOfSaleCouponsControllerProtocol

Use it in aggregate model: private let couponsController: PointOfSaleCouponsControllerProtocol

Profit

This way coupons controller can still share common funtionality with items controller but also grow with additional coupons related functionality.

Yosemite/Yosemite/PointOfSale/PointOfSaleCouponService.swift

# Conflicts: # WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift # WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

iamgabrielma · 2025-04-07T05:12:02Z

👋 I'll need to iterate on the approach we took here, as with the changes on the controller and item view state we've recently merged this use case doesn't work anymore:

From what I've checked so far, when switching to coupons, and coupons throw the .errorCouponsDisabled error on loading items, the ItemsViewState we set does not seem to longer render the correct case, as the PointOfSaleDashboardView does not switch again between cases when performing switch posModel.itemsViewState.containerState {, so we keep loading the previous contentView case which doesn't display anything as coupons are 0 when disabled. I'll look into this today.

iamgabrielma · 2025-04-07T06:10:49Z

switch posModel.itemsViewState.containerState

Issue seems to be here, since we do not use a "current controller", when we render the dashboard we're only checking for changes in the "item controller" which not really an item controller but a product controller, changes on couponsViewState are not handled so views are not re-rendered.

When view state changed, driven by changes in coupons, these were never propagated to the dashboard view since we only switched in “itemsViewState” which only covered the products controller.

iamgabrielma · 2025-04-07T09:28:38Z

Thanks for the review again @staskus

It seems we still need to expose some sort of "current" controller or view state to the views from the aggregate, otherwise we were only publishing changes in the "product view state", not on the coupons view state.I've added this on 562c1c1

I've also added some minimal renaming for "items", and named them "products". Otherwise gets very confusing to work with these lists and state and be sure what its actually updating, and one of the reasons we missed this issue originally (we're calling "items" to products, and calling "items" to items). I think we still need to rename this further across the board, but we can do this separately.

Xcode has decided today that doesn't want to compile the tests locally (I've cleared derived data a few times, and rebuilt, but is not very useful) but they're passing CI. If you're ok with this I'll add the tests separately, for now I'll leave this PR for review to move the ball forward as I'm EOD:

iamgabrielma · 2025-04-07T09:32:17Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

            .trackSize(size: $floatingSize)
            .accessibilitySortPriority(1)
-            .renderedIf(posModel.itemsViewState.containerState != .loading)
+            .renderedIf(posModel.currentViewState.containerState != .loading)


This took me a while: By leaving this one as posModel.itemViewstate, when switching to Coupons we were always stuck in .loading state, not showing coupons correctly. Of course the compiler didn't complain, but was tricky to find why the view state was wrong. I think we can move those ones to some computed property, or grab them directly from reading a variable from the aggregate model.

In general, this containerState logic doesn't work for products and coupons anymore, since we don't want to be showing coupons errors in full screen. This structure may change 🤔

staskus

Thank you! I understand the need for currentViewState 👍 , I think we could go this way but leave only one source of truth.

Also, I noticed that when coupons enabled, products tab is opened. Coupons tab should still be selected:

loading.switches.to.products.mp4

staskus · 2025-04-07T11:23:15Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

    func trackCardPaymentsOnboardingShown()

-    var itemsViewState: ItemsViewState { get }
+    var productsViewState: ItemsViewState { get }


I encourage not to make this renaming unless we rename all the items to products in the codebase.

As far as our internal codebase and business case knowledge goes - items include products and product variations

Gotcha, I've reverted it on e1979c9 I still think we should rename this at some point across the codebase as coupons should be considered items as well in certain layers.

staskus · 2025-04-07T11:26:24Z

WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift


 @available(iOS 17.0, *)
-@Observable final class PointOfSaleItemsController: PointOfSaleSearchingItemsControllerProtocol {
+@Observable final class PointOfSaleItemsController: PointOfSaleItemsControllerProtocol, PointOfSaleSearchingItemsControllerProtocol {


PointOfSaleSearchingItemsControllerProtocol extends PointOfSaleItemsControllerProtocol, no need to add it again.

Fixed! f7db770

staskus · 2025-04-07T11:29:15Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

            .trackSize(size: $floatingSize)
            .accessibilitySortPriority(1)
-            .renderedIf(posModel.itemsViewState.containerState != .loading)
+            .renderedIf(posModel.currentViewState.containerState != .loading)


In general, this containerState logic doesn't work for products and coupons anymore, since we don't want to be showing coupons errors in full screen. This structure may change 🤔

staskus · 2025-04-07T11:33:58Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

 @available(iOS 17.0, *)
 extension PointOfSaleAggregateModel {
+    func updateCurrentViewState(base: ItemListBaseItem) {
+        let viewState = base.itemType == .products ? productsViewState : couponsViewState


If currentViewState is required, I wonder if we should approach it differently.

ItemListView already holds selectedItemType. Maybe we should track selection in a single place—an aggregate model? That would give a source of truth and allows to keep currentViewState and even currentController as computed helper variables.

ItemListView also doesn't need to switch for itemsStack anymore and can use posModel.currentViewState.itemsStack.

Wdyt?

Yes, I think so. This also should help to keep proper state on which tabs we should be loading (products v coupons) when something from outside the list view happens (eg: errors, retry, etc, ... ). I've logged this on WOOMOB-269

staskus · 2025-04-07T11:47:24Z

WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift


-
+@available(iOS 17.0, *)
+extension PointOfSaleItemsControllerProtocol {


Oh, I didn't realize that at first. I don't love that ItemController gets affected this way.

This is what we could do:

Create PointOfSaleCouponsControllerProtocol which extends PointOfSaleItemsControllerProtocol:

protocol PointOfSaleCouponsControllerProtocol: PointOfSaleItemsControllerProtocol { /// Enables coupons in store settings, if needed func enableCoupons() async }

Implement it final class PointOfSaleCouponsController: PointOfSaleCouponsControllerProtocol

Use it in aggregate model: private let couponsController: PointOfSaleCouponsControllerProtocol

Profit

This way coupons controller can still share common funtionality with items controller but also grow with additional coupons related functionality.

Yosemite/Yosemite/PointOfSale/PointOfSaleCouponService.swift

iamgabrielma · 2025-04-08T11:50:38Z

I've split the feedback between changes I'm doing in this one vs changes we can follow-up with on different issues, so I can scope this one, as the protocol changes come with updates to ~30 files and can get messy:

Changes:

Reverted renaming
Removed duplicated conformance
Made a new protocol and mock/preview for coupons

To do:

Additional tests will come on a follow-up PR
Error handling from failing to enable settings is logged on WOOMOB-267
Selecting the coupons tab after enabling it successfully is logged on WOOMOB-268. I've time-boxed 30 minutes here but I couldn't figure out why it's behaving like this. We only seem to toggle the tab state internally via displayItemType(). That said, this could be resolved by itself when we move from full-screen to side-screen.
Centralizing the view state into a single source of truth is logged on WOOMOB-269

iamgabrielma · 2025-04-08T11:57:41Z

I noticed that when coupons enabled, products tab is opened. Coupons tab should still be selected.

Just to add: At some point it worked well, but then I could replicate the issue every time I've tried. I think once we make further changes regarding handling the view correctly (not full-screen) and move the source of truth of selected item somewhere else it should be either resolved or easier to resolve. For now is logged on WOOMOB-268

Screen.Recording.2025-04-08.at.17.59.31.mov

staskus

Thank you for the fixes and creating additional tasks. 👍

I think we took this task too early, or tried to achieve too much with this PR. We really needed an infrastructure of showing a state within the list to properly tackle this. Now, we have to merge half-baked happy and unhappy paths.

iamgabrielma added 3 commits April 3, 2025 13:37

Add error type to POS error state

a57403f

adapt error view action to accept different callbacks

9265da8

enable coupons when tap on cta

451bc55

iamgabrielma added type: task An internally driven task. feature: coupons Related to basic fulfillment such as order tracking. feature: POS labels Apr 3, 2025

iamgabrielma added this to the 22.1 milestone Apr 3, 2025

refactor for simplicity

c1cc174

iamgabrielma commented Apr 3, 2025

View reviewed changes

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift Outdated Show resolved Hide resolved

iamgabrielma requested a review from staskus April 3, 2025 07:11

make test compile

1c84292

staskus reviewed Apr 3, 2025

View reviewed changes

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift Outdated Show resolved Hide resolved

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift Outdated Show resolved Hide resolved

iamgabrielma modified the milestones: 22.1, 22.2 Apr 4, 2025

iamgabrielma added 6 commits April 4, 2025 13:59

Merge branch 'trunk' into task/15348-followup-enable-coupon-via-cta

f2df5ac

moves enable coupons call to coupons controller

009c621

move enable coupons to service

a2283f2

add follow-up issue reference

0ffcc10

remove unnecessary import

1044f68

localizations

fac5a07

iamgabrielma requested a review from staskus April 4, 2025 08:06

make test compile

85aef17

staskus reviewed Apr 4, 2025

View reviewed changes

iamgabrielma added 2 commits April 7, 2025 11:49

Merge branch 'trunk' into task/15348-followup-enable-coupon-via-cta

8395fe7

# Conflicts: # WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift # WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

make compile. ammend merge conflict

15b6568

iamgabrielma added 3 commits April 7, 2025 15:15

Add a shared current item view state based on item type

562c1c1

When view state changed, driven by changes in coupons, these were never propagated to the dashboard view since we only switched in “itemsViewState” which only covered the products controller.

remove unnecessary associated values

093fc72

update enableCoupons signature to be throwable

fb126bc

iamgabrielma commented Apr 7, 2025

View reviewed changes

staskus self-assigned this Apr 7, 2025

staskus self-requested a review April 7, 2025 11:17

staskus reviewed Apr 7, 2025

View reviewed changes

iamgabrielma added 6 commits April 8, 2025 17:00

update issue references

b248310

revert itemsViewState rename

e1979c9

remove duplicated conformance

f7db770

make PointOfSaleCouponsControllerProtocol

0abe0bb

update tests with new mock

342f887

lint

b85c7bc

iamgabrielma requested a review from staskus April 8, 2025 12:01

staskus approved these changes Apr 8, 2025

View reviewed changes

staskus merged commit d175652 into trunk Apr 8, 2025
13 checks passed

staskus deleted the task/15348-followup-enable-coupon-via-cta branch April 8, 2025 12:22

staskus mentioned this pull request Apr 8, 2025

[Woo POS] Coupons: Centralize view state source of truth #15495

Merged

4 tasks



		@available(iOS 17.0, *)
		extension PointOfSaleItemsControllerProtocol {

[Woo POS] Coupons: Enable site coupons when tap on CTA #15478

[Woo POS] Coupons: Enable site coupons when tap on CTA #15478

Uh oh!

Conversation

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

Uh oh!

Description

Testing information

Reviewer (or Author, in the case of optional code reviews):

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

staskus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

staskus commented Apr 3, 2025

Uh oh!

iamgabrielma commented Apr 4, 2025

Uh oh!

iamgabrielma commented Apr 4, 2025

Uh oh!

staskus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iamgabrielma commented Apr 7, 2025

Uh oh!

iamgabrielma commented Apr 7, 2025

Uh oh!

iamgabrielma commented Apr 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iamgabrielma commented Apr 8, 2025

Uh oh!

iamgabrielma commented Apr 8, 2025

Uh oh!

staskus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

iamgabrielma commented Apr 3, 2025 •

edited

Loading

wpmobilebot commented Apr 3, 2025 •

edited

Loading