-
Notifications
You must be signed in to change notification settings - Fork 120
[Local Catalog] Filter trashed products #16361
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
[Local Catalog] Filter trashed products #16361
Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…yncRemote 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…arameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add tests verifying include_status parameter is correctly added/omitted - Add test verifying status is included in _fields request parameter - Update MockPOSCatalogSyncRemote to support includeStatus parameter - Update test fixtures to include required statusKey field 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add tests verifying both regular and trashed products are fetched - Add test ensuring same modifiedAfter date is used for both requests - Add test verifying includeStatus parameter values - Add tests for combined persistence of regular and trashed products - Add tests for edge cases (empty trash, only trash updates) - Enhance MockPOSCatalogSyncRemote to support separate trashed product results 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace non-thread-safe array append with actor-based IncludeStatusTracker to prevent EXC_BAD_ACCESS crash when concurrent requests modify the array. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add "status": "publish" to product fixtures that were missing the field, fixing decoding errors in POSCatalogSyncRemoteTests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Wrap single product objects in {"data": [...]} envelope to match
standard WooCommerce REST API response format expected by tests.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Add statusKey field with default value "publish" to TestProduct struct to match updated PersistedProduct schema, fixing NOT NULL constraint errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
|
The catalog download parser decodes all items as POSProduct first before filtering by type, so variations also need the status field to avoid decoding errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
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.
LGTM, just a comment about excluding private products
🚀
| productTable.column("stockQuantity", .double) | ||
| productTable.column("stockStatusKey", .text).notNull() | ||
|
|
||
| productTable.column("statusKey", .text).notNull() |
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 curious, with GRDB is as easy to update the model? Or we just happen to work with the initial schema so it's straight-forward?
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.
Once it's released, we'll need to make new schemas and migrations for changes like this... that's the reason for the V001 in the name. It's still very lightweight to change.
During development, we delete the database when a schema change is detected and make it fresh. I guess we could even do that in production, but it risks someone having to do an extra full sync at an inconvenient time.
| /// Returns a request for POS-supported products (simple and variable, non-downloadable) for a given site, ordered by name | ||
| /// Filters out products with trash, draft, pending, or private status to ensure only published and 3rd party custom status products are shown | ||
| static func posProductsRequest(siteID: Int64) -> QueryInterfaceRequest<PersistedProduct> { | ||
| let excludedStatuses = [ |
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.
Would make sense to import Networking here and use the raw values for ProductStatus rather than the string directly?
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.
You can't import Networking from Storage as they're siblings
| "trash", | ||
| "draft", | ||
| "pending", | ||
| "private" |
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 not sure we want to exclude private products. While these are hidden from public view on the shop page and in search results, this is a web-based thing, for POS it would make sense to keep it included as the product is still visible to any logged-in user that is not a customer (admins, editors, etc, ... )
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.
Great catch, thank you!
Modules/Sources/Yosemite/Tools/POS/POSCatalogIncrementalSyncService.swift
Outdated
Show resolved
Hide resolved
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.
All the try await sut.startIncrementalSync(... in this file throw a warning since the result is unused, let's make them let _ = or discardableresult

Merge after: #16353
Description
This PR ensures that trashed products are removed from POS after an incremental sync.
We already hide them if they're trashed when we do a full sync – because the API doesn't return them for a normal request. However, that works against us if they've been trashed when we do an incremental sync, because the newly-trashed product won't be returned for those requests either.
To resolve that, we do an extra request for any newly trashed products, and include those in the local database. When we've done that, we filter those products from all our queries.
Note that Variations can't be trashed, they just get deleted.
Test Steps
Set up a new product with a GTIN that you can scan with a barcode scanner
Not founderror is shown, same as for deleted productsSearch isn't affected by this, as it doesn't use the local catalog yet.
Fully deleted products are still not removed by incremental sync, but will be by the next full sync. Without extensive backend work, this is unavoidable.
Screenshots
This shows:
trashed.products.mp4
RELEASE-NOTES.txtif necessary.