feat(medusa,types): added store apis for products #7144
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Ignored Deployments
|
🦋 Changeset detectedLatest commit: 765f346 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
riqwan
force-pushed
the
feat/products-api
branch
from
ffee73b
to
6979c93
Compare
| @@ -0,0 +1,332 @@ | |||
| import { ModuleRegistrationName } from "@medusajs/modules-sdk" | |||
There was a problem hiding this comment.
@riqwan I know you wrote a lot of tests here, but does it make sense to actually make the integration-tests/api tests work instead? If you feel like it will take a lot of time I'm fine with doing it later on, but we really need to try and clean up the tests
There was a problem hiding this comment.
2 points:
- I'd like these tests to be run on the CI, which it won't with the api tests.
- I don't get why we do that. Wouldn't it just be better to move over the test cases to modules and make it work for v2? We're not really writing more tests for v1, so we wouldn't be missing out much. We do the same everywhere else - v2 routes, v2 dashboard, why not the same here?
There was a problem hiding this comment.
@riqwan The idea is that, unless we intended a breaking change, the test scenarios in api are still valid and should work regardless if it is v1 or v2. Eg. for the product endpoints, we have very few breaking changes, and that gives us some confidence that we are compliant with v1, except for the intentional breaking changes.
But if you feel like that is not a valid point, then we should discuss and settle on a single approach, as currently we have a split between the two
integration-tests/modules/__tests__/product/store/index.spec.ts
Outdated
Show resolved
Hide resolved
integration-tests/modules/__tests__/product/store/index.spec.ts
Outdated
Show resolved
Hide resolved
integration-tests/modules/__tests__/product/store/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/medusa/src/api-v2/utils/middlewares/pricing/set-pricing-context.ts
Outdated
Show resolved
Hide resolved
packages/medusa/src/api-v2/utils/middlewares/pricing/set-pricing-context.ts
Outdated
Show resolved
Hide resolved
integration-tests/modules/__tests__/product/store/index.spec.ts
Outdated
Show resolved
Hide resolved
| req: MedusaRequest<StoreGetProductsParamsType>, | ||
| res: MedusaResponse | ||
| ) => { | ||
| const context = isPresent(req.pricingContext) |
There was a problem hiding this comment.
Nothing to do now, I am just thinking if we can make the context-passing and calculations setup a bit nicer. Just talking out loud here 😄
| method: "ALL", | ||
| matcher: "/store/products*", | ||
| middlewares: [ | ||
| authenticate("store", ["session", "bearer"], { |
There was a problem hiding this comment.
Again, for another time - I think having a global middleware that just populates the context will be much better than relying on authenticate, as this middleware ideally should do actual authentication (and the allowUnauthenticated flag should not exist).
| const context = isPresent(req.pricingContext) | ||
| ? { | ||
| "variants.calculated_price": { context: req.pricingContext }, | ||
| } | ||
| : undefined | ||
|
|
||
| const product = await refetchProduct( | ||
| { | ||
| id: req.params.id, | ||
| context, | ||
| }, | ||
| req.scope, | ||
| req.remoteQueryConfig.fields | ||
| ) | ||
|
|
||
| res.json({ product }) |
There was a problem hiding this comment.
| const context = isPresent(req.pricingContext) | |
| ? { | |
| "variants.calculated_price": { context: req.pricingContext }, | |
| } | |
| : undefined | |
| const product = await refetchProduct( | |
| { | |
| id: req.params.id, | |
| context, | |
| }, | |
| req.scope, | |
| req.remoteQueryConfig.fields | |
| ) | |
| res.json({ product }) | |
| const refetchProductParams: object = { | |
| id: req.params.id, | |
| } | |
| if (isPresent(req.pricingContext)) { | |
| config.context = { | |
| "variants.calculated_price": { context: req.pricingContext }, | |
| } | |
| } | |
| const product = await refetchProduct( | |
| refetchProductParams, | |
| req.scope, | |
| req.remoteQueryConfig.fields | |
| ) | |
| res.json({ product }) |
| const categoriesFilter = isPresent(filters.category_id) | ||
| ? { | ||
| categories: { | ||
| ...filters.category_id, |
There was a problem hiding this comment.
suggestion: I believe the spread here will create an issue, it will assign the index as the key and the id as the value as it seems to be an array spreaded into an object
| ] | ||
|
|
||
| export const retrieveProductQueryConfig = { | ||
| defaults: defaultStoreProductFields, |
There was a problem hiding this comment.
q: do we need to limit what is accessible by the store, I am thinking if someone add more link or properties or something, the store could request for some admin data? This is more of a global question :)
| if ( | ||
| !req.remoteQueryConfig.fields.some((field) => | ||
| field.startsWith("variants.calculated_price") | ||
| ) | ||
| ) { | ||
| delete req.filterableFields.region_id | ||
| delete req.filterableFields.currency_code | ||
|
|
||
| return next() | ||
| } |
There was a problem hiding this comment.
Q: here, If I pass the region id and/or currency but I forget about the relation, I will still get a result but without the context filtering, it can make it hard to debug, what if we throw an invalid data here instead to say that it is not possible to filter the with the context without this relation, or alternatively we infer the context filtering if region id or currency code is present since they are only used in that context.
wdyt?
| "currency", | ||
| { code: query.currency_code }, | ||
| req.scope, | ||
| ["code"] |
There was a problem hiding this comment.
Q: isn't the code here the same as the one provided? Are we doing that to check if the entity exists?
| } | ||
|
|
||
| // If a region or currency_code is not present in the context, we will not be able to calculate prices | ||
| if (!isPresent(pricingContext)) { |
There was a problem hiding this comment.
suggestion: shouldn't we check for the currency and or region? Could it be possible that none of them are present but another key is? I think a check on region or currency would be more future proof. Wdyt?
what:
RESOLVES CORE-2016