-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Historical Orders] Orders Fetching #16036
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
[Woo POS][Historical Orders] Orders Fetching #16036
Conversation
… pagination of POS orders
Generated by 🚫 Danger |
|
|
|
👋 I'll review this one later today, for the time being seems the test target won't compile: Periphery seems to complain as well but there is no output in buildkite, perhaps because of the build failure hitting before the scan. If needed objects can be marked with |
|
@iamgabrielma yes, sorry! I need to get used to periphery. In these internal project PRs, I sometimes implement something that is supposed to be used in another PR but is not yet used now. I'll fix those warnings |
iamgabrielma
left a comment
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.
Works well and LGTM! Just left a couple of non-blocking comments.
✅ Confirm the loading state appears when orders are loading
✅ Confirm correct POS orders are loaded, compare with wp-admin and Woo app orders
✅ Confirm pull-to-refresh fetches new data
✅ Confirm scrolling down triggers pagination
✅ Close the orders view and reopen
✅ Confirm cached data is immediately shown
| import struct NetworkingCore.Order | ||
| import protocol NetworkingCore.POSOrdersRemoteProtocol | ||
|
|
||
| public final class PointOfSaleOrderService: PointOfSaleOrderServiceProtocol { |
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.
Just a thought, the naming of PointOfSaleOrderService is pretty similar to the existing POSOrderService and protocols which may lead to confusion. I have no specific suggestion at the moment, but I think we should start to differentiate them more, perhaps renaming to PointOfSaleOrdersService for now would suffice 🤔
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. We could call it OrderList{Controller/Service/etc..} so it would be as clear as possible...
| return .init(items: posOrders, | ||
| hasMorePages: pagedOrders.hasMorePages, | ||
| totalItems: pagedOrders.totalItems) | ||
| } catch AFError.explicitlyCancelled { |
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.
TIL that we will separate these two, what's the use case for the request being explicitly cancelled? We may cancel it for performance/cache reasons?
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.
It was more related to the search #15580, where inputting more letters would cancel the previous request. However, we don't want to show any error, so we explicitly handle and treat the request as successful.
| func loadNextOrders() async | ||
| } | ||
|
|
||
| @Observable final class PointOfSaleOrdersController: PointOfSaleOrdersControllerProtocol { |
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.
Same as above: PointOfSaleOrderController vs PointOfSaleOrdersController are pretty close
| default: | ||
| return "Unknown Order" | ||
| } | ||
| @Environment(PointOfSaleOrdersModel.self) private var ordersModel |
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'm curious about the usage of this model by injecting directly into the environment, rather than passing it through the aggregate model. Would you say it breaks the existing design or we should be fine to adopt it? Mainly asking because I tried the same approach for POS settings, but ultimately went through the aggregate model since I needed related services that were already injected.
PD: It could be just because UI is temporary, sorry if that's the case :D
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 question. I injected it separately because it didn't have any connection to the existing aggregate model. It looked like a completely different feature that depends on different kinds of information. Settings could be a different case since it makes sense that it shares dependencies with the main part of the POS.
| private let purchasableItemsSearchController: PointOfSaleSearchingItemsControllerProtocol | ||
| private let couponsController: PointOfSaleCouponsControllerProtocol | ||
| private let couponsSearchController: PointOfSaleSearchingItemsControllerProtocol | ||
| private let ordersController: PointOfSaleOrdersControllerProtocol |
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.
Same here with ordersController vs orderController, maybe makes sense to rename the original one in the sense of order creation, fulfilment, factory, ... and this one as order list? Naming is indeed a difficult problem 😅
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 points, I'll rename!

Description
WOOMOB-1131
Infrastructure for remotely fetching required data from API, pagination, and in-memory cache. Implement a remote-first approach with the 25 most recent POS orders, automatic pagination when scrolling to the bottom, and in-memory caching for immediate display.
For consistency, I implemented a similar approach that we use with the items. This PR contains a lot of boilerplate, but I thought it's easier to do it in one go.
PointOfSaleOrdersModelsince this view state is completely separate from the aggregate model (and wider POS)POSOrder,POSOrderItem,POSOrderRefundSteps to reproduce
UI is out of scope, generated to facilitate testing
Testing information
Tested on iPad Air M2 18.5 simulator
Screenshots
Order.List.and.Details.mov
RELEASE-NOTES.txtif necessary.