Skip to content

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Sep 26, 2025

Part of WOOMOB-935

Just one review is required.

This PR should be reviewed after #16173

Overview

This PR refactors the PointOfSaleEntryPointView initializer to minimize the number of POS dependencies that need to be marked as public in preparation for moving POS functionality to a separate module. The goal is to reduce the public API surface by consolidating dependency injection at the entry point level.

Changes Made

  • Refactored PointOfSaleEntryPointView initializer to accept fewer, more consolidated dependencies
  • Moved dependency resolution logic to reduce the number of classes that need public access modifiers

Steps to reproduce

A smoke test in POS would be helpful for regression testing.

Testing Information

  • I tested POS cash and card payment on iPad A16 iOS 26 simulator.

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

…refactoring' into feat/WOOMOB-935-refactor-pos-entry-point

# Conflicts:
#	WooCommerce/Classes/POS/Presentation/PointOfSaleEntryPointView.swift
#	WooCommerce/Classes/POS/Utils/PreviewHelpers.swift
@dangermattic
Copy link
Collaborator

dangermattic commented Sep 26, 2025

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@jaclync jaclync changed the base branch from trunk to backlog/WOOMOB-869-remove-pos-tab-i2-feature-flag-with-refactoring September 26, 2025 00:55
@jaclync jaclync added type: task An internally driven task. feature: POS labels Sep 26, 2025
import struct NetworkingCore.JetpackSite

public struct PointOfSaleCouponFetchStrategyFactory {
public protocol PointOfSaleCouponFetchStrategyFactoryProtocol {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: a protocol was created so that StorageManagerType, a dependency of PointOfSaleCouponFetchStrategyFactory, does not need to be known in the future POS module.

@jaclync jaclync changed the title Refactor POS entry point initializer to minimize public dependencies [Woo POS] Modularization: refactor POS entry point initializer to minimize public dependencies Sep 26, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 26, 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 Numberpr16175-a894857
Version23.3
Bundle IDcom.automattic.alpha.woocommerce
Commita894857
Installation URL4ofeh95uep13g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync added this to the 23.4 milestone Sep 26, 2025
Base automatically changed from backlog/WOOMOB-869-remove-pos-tab-i2-feature-flag-with-refactoring to trunk September 30, 2025 06:24
# Conflicts:
#	WooCommerce/Classes/POS/Models/POSIneligibleReason.swift
#	WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsView.swift
#	WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift
@jaclync
Copy link
Contributor Author

jaclync commented Sep 30, 2025

@staskus While resolving the merge conflicts from trunk where GRDBManagerProtocol is now a dependency of POS settings, the future POS module will have to know about GRDBManagerProtocol which is currently in the Storage module along with Core Data code. I don't think GRDB has any dependencies on Core Data or its Storage protocol/implementation, we could move GRDB code somewhere else. Some options I can think of:

  1. Move Storage/GRDB from Storage to a new module like GRDBStorage, and the future POS module depends on it.
  2. Move Storage/GRDB from Storage to the future POS module.
  • Main drawback: no separation of concerns in the POS module, and would be difficult to reuse if the main app adopts GRDB in the future.
  1. Keep Storage/GRDB in Storage, and the future POS module depends on Storage.
  • Main drawback: POS module can access Core Data, manual code reviews are required to prevent misusing Core Data in POS.

WDYT? I personally would go with option 1 given the drawbacks of the other two. Also looping in @joshheald, as I vaguely recall you mentioned separating GRDB into a separate module in a previous PR.

@jaclync jaclync merged commit b5dd3c7 into trunk Sep 30, 2025
13 checks passed
@jaclync jaclync deleted the feat/WOOMOB-935-refactor-pos-entry-point branch September 30, 2025 07:10
@staskus
Copy link
Contributor

staskus commented Sep 30, 2025

@jaclync thanks for those options!

PointOfSale module still depends on Yosemite, which depends on Storage, so fundamentally, I don't think that it's a huge problem that PointOfSale would depend on Storage directly as well.

However, regardless of PointOfSale module, for GRDB it would make sense to have it in a separate module, just as @joshheald pitched to me a few days ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants