-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(fulfillment): Module service implementation first iteration #6381
Conversation
🦋 Changeset detectedLatest commit: 0d8a946 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
**What** Fix the test utils database to trully run the migrations, the migration path is deprecated and not used anymore as umzung infer the path under the hood. It also fix the schema so that it is possible to specify different schema if needed. The index helper can now create named constraints for uniqueness. extracted [from](#6381)
packages/fulfillment/src/services/fulfillment-module-service.ts
Outdated
Show resolved
Hide resolved
packages/fulfillment/src/services/fulfillment-module-service.ts
Outdated
Show resolved
Hide resolved
packages/fulfillment/src/services/fulfillment-module-service.ts
Outdated
Show resolved
Hide resolved
packages/fulfillment/src/services/fulfillment-module-service.ts
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.
Very nice work - have a few comments on the data model. Might make sense to address as part of this PR, but up to you.
packages/fulfillment/src/services/fulfillment-module-service.ts
Outdated
Show resolved
Hide resolved
packages/fulfillment/src/services/fulfillment-module-service.ts
Outdated
Show resolved
Hide resolved
…js/medusa into feat/fulfillment-implementation-1
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!
inverseJoinColumn: "service_zone_id", | ||
@OneToMany(() => ServiceZone, "fulfillment_set", { | ||
cascade: [Cascade.PERSIST, "soft-remove"] as any, | ||
orphanRemoval: true, |
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.
question: I thought we didn't use this because of its "aggressiveness". I am fine to keep it in, but just wanted to point it out in case it was an oversight
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.
Here it a requirements. Because since i am bulk deleting the entities, then when mikro orm manage the collection under the hood it will complain that thos configuration is missing. Also, i am in a case where if the fulfillment set is removed completely, then we can have orphan service zones since the foreign key is non nullable. Same goes between service zones and geo zones
inverseJoinColumn: "geo_zone_id", | ||
@OneToMany(() => GeoZone, "service_zone", { | ||
cascade: [Cascade.PERSIST, "soft-remove"] as any, | ||
orphanRemoval: true, |
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.
Same here
@adrien2p feel free to merge 🚢 |
What
Tip
The review should particularly focus on the module service implementation and the integration tests to ensure that the behavior conforms to the expectations. In Sus, the adjustments made to the models and DTO's
Warning
The delete methods are kept aside as it will require manually cascading since according to the doc, cascade remove on collection will perform one query per item. In that case I rather prefer deleting them manually in bulk.
FIXES CORE-1716
FIXES CORE-1734