-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Local Catalog] Enable storing data for multiple sites at once #16120
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][Local Catalog] Enable storing data for multiple sites at once #16120
Conversation
|
|
jaclync
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, thanks for figuring out the compound primary keys in GRDB
there are some failing test cases in CI that seem related.
| private static func createProductAttributeTable(_ db: Database) throws { | ||
| try db.create(table: "productAttribute") { productAttributeTable in | ||
| // This table holds local product attributes only. Global attributes belong to a site. | ||
| productAttributeTable.autoIncrementedPrimaryKey("id").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.
nit: is it worth adding a separate column for the "attribute ID" from the API response, if we ever want to differentiate between local vs. global attributes? Similar question for the variation attribute entity.
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.
That's a separate task on my list (possibly not in linear yet, I can't remember) but I'm planning to make the schema support both types properly before we finalise it. It's just less urgent than this change.
| private extension PersistedProductVariationAttribute { | ||
| extension PersistedProductVariationAttribute { |
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.
nit: any reasons for making this extension internal while the same one in other classes stay 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.
In order to refer to the column names by for the variation's attributes relationship:
public static let attributes = hasMany(PersistedProductVariationAttribute.self,
using: ForeignKey([PersistedProductVariationAttribute.CodingKeys.siteID.stringValue,
PersistedProductVariationAttribute.CodingKeys.productVariationID.stringValue],
to: primaryKey))
However, that relationship isn't actually working (it's one of the causes of the failing tests) so when I've fixed it I'll check whether this can go back to private or not.
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.
Yeah, I actually needed to make the others internal too!

Description
This PR adds the ability to store more than one site's data at a time.
Changes
siteIDin their primary key.Steps to reproduce
Screenshots
The purple boxes are site 1, the orange boxes are site 2
RELEASE-NOTES.txtif necessary.