- Sponsor
-
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(medusa): add middleware filters + scope products #7178
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Ignored Deployments
|
🦋 Changeset detectedLatest commit: 1bf9561 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 💪 couple of comments
@@ -349,6 +394,34 @@ medusaIntegrationTestRunner({ | |||
) | |||
}) | |||
|
|||
// TODO: There are 2 problems that need to be solved to enable this test | |||
// 1. When adding product to another category, the product is being removed from earlier assigned categories | |||
// 2. MikroORM seems to be doing a join strategy to load relationships, we need to send a separate query to fetch relationships |
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.
info: for the second point you can pass the strategy as part of the options. But it would require to add it as part of the FindConfig type, the underlying service and repository will then consume it
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.
yup, will fix this as a follow up.
// If a region or currency_code is not present in the context, we will not be able to calculate prices | ||
if (!isPresent(pricingContext)) { | ||
// If a currency_code is not present in the context, we will not be able to calculate prices | ||
if (!isPresent(pricingContext.currency_code)) { |
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.
todo: based on the error message it seems that we should add a check on the region id, probably something along the line of !pricingContext.currency_code && !pricingContext.region_id
then we throw
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.
the currency code will be set as a part of the region, so we only need to check currency_code here.
@@ -349,6 +394,34 @@ medusaIntegrationTestRunner({ | |||
) | |||
}) | |||
|
|||
// TODO: There are 2 problems that need to be solved to enable this test | |||
// 1. When adding product to another category, the product is being removed from earlier assigned categories |
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.
@sradevski do you know anything about this?
No description provided.