[Woo POS][Local catalog] Add relationship mapping and saving for POS models #16063
Conversation
joshheald
added
type: task
joshheald
force-pushed
the
woomob-1210-relationship-based-mapping-and-saving-for-POS-entities
branch
from
10ff73e to
f416265
Compare
|
|
| ) | ||
| } | ||
|
|
||
| func toPOSProduct(db: GRDBDatabaseConnection) throws -> POSProduct { |
There was a problem hiding this comment.
❓ what are the use cases for fetching products, then their relationships separately, as this method seems to do? I'd think the products (with some filter) are fetched including the relationships?
There was a problem hiding this comment.
I don't think they are – relationships are just foreign keys. If you fetch a record from SQLite, you'll get the id for looking up the relationship, but it won't walk the graph for you. I'll double check that GRDB doesn't do it for us, but I don't think it does.
| let images = try db.read { db in | ||
| return try request(for: PersistedProduct.images).fetchAll(db) | ||
| } | ||
| let attributes = try db.read { db in | ||
| return try request(for: PersistedProduct.attributes).fetchAll(db) | ||
| } |
There was a problem hiding this comment.
nit: could these be combined into one database transaction, potentially saving some overhead from each db.read:
| let images = try db.read { db in | |
| return try request(for: PersistedProduct.images).fetchAll(db) | |
| } | |
| let attributes = try db.read { db in | |
| return try request(for: PersistedProduct.attributes).fetchAll(db) | |
| } | |
| let (images, attributes) = try db.read { db in | |
| let images = try request(for: PersistedProduct.images).fetchAll(db) | |
| let attributes = try request(for: PersistedProduct.attributes).fetchAll(db) | |
| return (images, attributes) | |
| } |
| // MARK: - POSProduct Storage Extensions | ||
| extension POSProduct { | ||
| public func save(to db: GRDBDatabaseConnection) throws { | ||
| try db.write { db in |
There was a problem hiding this comment.
do you know if it's recommended to weak self in the db.write closure?
There was a problem hiding this comment.
I don't know of a recommendation.
The updates closure that we pass to write is non-escaping, so it can't be stored by GRDB, and we don't hold it beyond our function scope either.
I don't see any reason to make self weak here, at least not to avoid retain cycles.
Modules/Sources/Yosemite/Model/Storage/PersistedProductVariation+Conversions.swift
Outdated
Show resolved
Hide resolved
|
|
||
| // Save related attributes | ||
| for attribute in self.attributes { | ||
| var persistedAttribute = PersistedProductVariationAttribute(from: attribute, productVariationID: self.productVariationID) |
There was a problem hiding this comment.
No – insert is mutating for attributes, because of the autoincremented ID.
…d' into woomob-1210-relationship-based-mapping-and-saving-for-POS-entities
…OS-entities' into woomob-1210-tests-for-saving-pos-models
joshheald
merged commit 2159f0d
into
woomob-1210-basic-mapping-from-POS-entities-to-persisted
joshheald
deleted the
woomob-1210-relationship-based-mapping-and-saving-for-POS-entities
branch
Part of: WOOMOB-1210
Merge after: #16062
Description
This PR add functions to save whole POSProduct and POSProductVariation models, complete with their sub-models, i.e. attributes and images.
Tests got a bit out of hand, I've put them in a separate PR
Steps to reproduce
None, these are not used in the app yet, just check unit tests.
RELEASE-NOTES.txtif necessary.