-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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(utils): add campaign + promotion create / update #6077
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
🦋 Changeset detectedLatest commit: 51d62e1 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 |
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, only a couple of questions :)
@@ -8,4 +10,60 @@ export class CampaignRepository extends DALUtils.mikroOrmBaseRepositoryFactory< | |||
create: CreateCampaignDTO | |||
update: UpdateCampaignDTO | |||
} | |||
>(Campaign) {} | |||
>(Campaign) { | |||
async create( |
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: Shouldn't this logic live at the service level as per our convention? Or is this a special case?
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.
This is to add promotions to the campaign collection (through .add()
), which is purely dependent on MikroORM. If we add it to the service, we make the service mikroorm dependent.
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.
My thinking was if a feature of the module is to be able to create a campaign with promotions using a single service method, then this should be decoupled from MikroORM no? Otherwise, the behavior of service.createCampaigns
would differ depending on what ORM you choose, which I wouldn't expect.
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.
Yes, I think we're aligned on that. But it is true that ORMs handle one to many or many to many relationships differently. Shouldn't we just have to ensure that we pass a consistent shape to the repository layer and then the repositories handle them based on how they process relationships?
@adrien2p @pKorsholm can you guys take a look at this PR? |
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, just a couple of nits 😄
RESOLVES CORE-1637
RESOLVES CORE-1638