feat(cart): Data models #5986
feat(cart): Data models #5986
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Ignored Deployments
|
olivermrbl
force-pushed
the
feat/cart-module-data-models
branch
3 times, most recently
from
9444d07 to
63bbf63
Compare
olivermrbl
force-pushed
the
feat/cart-module-data-models
branch
from
63bbf63 to
0653093
Compare
olivermrbl
requested review from
riqwan,
fPolic,
adrien2p,
carlos-r-l-rodrigues and
pKorsholm
|
|
||
| type OptionalAddressProps = DAL.EntityDateColumns // TODO: To be revisited when more clear | ||
|
|
||
| @Entity({ tableName: "cart_address" }) |
There was a problem hiding this comment.
question: any reason we store that in cart_address and not address?
There was a problem hiding this comment.
We need to couple table names to the module for when we start combining them.
E.g. @medusajs/cart and @medusajs/customer will both ship an address table. To avoid clashes/conflicts with tables across the modules, we need to differentiate them. Otherwise, the address table would be shared, which can lead to unexpected issues.
It will be the consumer's responsibility to ensure that addresses are managed correctly for these cases where the model lives in both modules. Here, with the Customer and Cart module, you would likely use the customer's address from the Customer Module to create a cart address in the Cart Module
This will introduce some data redundancy, but it is for the better. It will make the modules much more usable in a standalone context.
There was a problem hiding this comment.
yes you are entirely right, I ve indentified that with the rest of the pr :D my bad
| code: string | ||
|
|
||
| @Property({ columnType: "numeric" }) | ||
| amount: number |
There was a problem hiding this comment.
question: do we need the serializer/deserializer?
There was a problem hiding this comment.
We need a util to be used for money amounts everywhere to normalize this. Probably a good time to sync and update pricing, promotion and cart.
There was a problem hiding this comment.
Yeah, would make sense.
Were you thinking something along the lines of what this guy proposes, but instead of parseFloat use bignumber.js to return a BigNumber when reading the column?
There was a problem hiding this comment.
yes. we could install bignumber.js on our utils and use it from there.
however that will impact the calculations. but we can do it in two steps.
first normalizing all the db + serialize but returning the BigNumber .toNumber(), and next step use the methods to perform the calculations where it is needed.
There was a problem hiding this comment.
yes, that sounds good. Since bignumberjs also works with strings, this would amount to amount: string? or are we still serializing it back to a number.
There was a problem hiding this comment.
I think the property still needs to be typed as a number, so we work with numbers on data mutations:
manager.create(LineItem, { amount: 10000000 })
But I am actually not sure. Would like others to pitch in too.
There was a problem hiding this comment.
yes the property can be kept as number, it doesn't prevent us from using float numbers for example. But it would be more about the serizliation and deserialization related no?
There was a problem hiding this comment.
in a first moment we keep it as number.
later we change it to a BigNumber type and refactor all the math operations to use the methods to calculate
There was a problem hiding this comment.
just to summarize the idea from carlos
- step1: everything number with current ser/deser
- step2: include bignumbers or similar as the ser/deser to number
- step3: change number usage into all calculations
| code: string | ||
|
|
||
| @Property({ columnType: "numeric" }) | ||
| amount: number |
There was a problem hiding this comment.
We need a util to be used for money amounts everywhere to normalize this. Probably a good time to sync and update pricing, promotion and cart.
| @Property({ columnType: "text", nullable: true }) | ||
| product_title: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| product_description: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| product_subtitle: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| product_type: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| product_collection: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| product_handle: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| variant_sku: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| variant_barcode: string | null | ||
|
|
||
| @Property({ columnType: "text", nullable: true }) | ||
| variant_title: string | null | ||
|
|
||
| @Property({ columnType: "jsonb", nullable: true }) | ||
| variant_option_values: Record<string, unknown> | null |
There was a problem hiding this comment.
suggestion: I'm wondering if we could have another table to relate the property (id) and the value. That way we could have a more generic cart system not mapping product properties.
item_property
line_item
line_item_item_property
There was a problem hiding this comment.
Having the variant and product properties specifically was meant to solve for two things:
- Model properties that are easy to work with
- Cart line items similar to how many other providers define them. E.g. Shopify and commercetools.
What other properties do you think could be relevant to have – potentially via the line item property approach?
There was a problem hiding this comment.
Wil merge this now, so I can move forward.
But I will not commit any migrations yet nor will I work with items yet, so we have time next week to revisit :)
What
Adds Cart Module data models