-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
refactor: migrate pricing entities to DML models #10335
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
packages/modules/pricing/integration-tests/__tests__/services/pricing-module/price-list.spec.ts
Show resolved
Hide resolved
packages/modules/pricing/integration-tests/__tests__/services/pricing-module/price-list.spec.ts
Show resolved
Hide resolved
packages/modules/pricing/integration-tests/__tests__/services/pricing-module/price-list.spec.ts
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.
Overall LGTM, nice work!
@@ -40,7 +40,7 @@ const createPriceLists = async ( | |||
rules: object = defaultRules, | |||
prices = defaultPriceListPrices | |||
) => { | |||
return await service.createPriceLists([ | |||
return service.createPriceLists([ |
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: This intentional?
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.
Nope. By mistake, but it shouldn't change the flow or the behavior of the code. However, happy to revert it back
@@ -1,6 +1,6 @@ | |||
import { Migration } from "@mikro-orm/migrations" | |||
|
|||
export class Migration20241127223829 extends Migration { | |||
export class Migration20241129094116 extends Migration { |
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.
thought: We probably shouldn't rename migrations, as it would otherwise be re-applied no?
Fixes: FRMW-2810 ## Breaking changes There is only one breaking change - The `min_quantity` and `max_quantity` properties are now consistently typed as numbers. Earlier, it was [typed as numbers](https://github.com/medusajs/medusa/blob/develop/integration-tests/http/__tests__/price-list/admin/price-list.spec.ts#L68-L69) in some API responses and as [string in others](https://github.com/medusajs/medusa/blob/develop/integration-tests/http/__tests__/price-list/admin/price-list.spec.ts#L186-L187). I did not go to the bottom of this inconsistency, but the tests reveals them. Co-authored-by: Adrien de Peretti <25098370+adrien2p@users.noreply.github.com>
Fixes: FRMW-2810
Breaking changes
There is only one breaking change
min_quantity
andmax_quantity
properties are now consistently typed as numbers. Earlier, it was typed as numbers in some API responses and as string in others. I did not go to the bottom of this inconsistency, but the tests reveals them.