Skip to content
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): Retrieve (service + controller) a product category #3004

Merged
merged 7 commits into from
Jan 12, 2023

Conversation

riqwan
Copy link
Contributor

@riqwan riqwan commented Jan 12, 2023

What:

Introduces a store endpoint to retrieve a product category

Why:

This is part of a greater goal of allowing products to be added to multiple categories.

How:

  • Creates an endpoint in store routes

RESOLVES CORE-967

@changeset-bot
Copy link

changeset-bot bot commented Jan 12, 2023

🦋 Changeset detected

Latest commit: bd5dc41

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@medusajs/medusa Patch

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 riqwan force-pushed the feat/storefront-retrieve-endpoint branch from 7c5f89d to 8718a6c Compare January 12, 2023 07:47
@riqwan riqwan force-pushed the feat/storefront-retrieve-endpoint branch 2 times, most recently from d3c54ae to e6c91bd Compare January 12, 2023 08:01
@riqwan riqwan force-pushed the feat/storefront-retrieve-endpoint branch from e6c91bd to b6ecce9 Compare January 12, 2023 08:02
@riqwan riqwan marked this pull request as ready for review January 12, 2023 08:02
@riqwan riqwan requested a review from a team as a code owner January 12, 2023 08:02

const productCategory = await productCategoryService.retrieve(
id,
retrieveConfig
Copy link
Contributor Author

@riqwan riqwan Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, now that i look at this, i do need to add a { where: { is_internal: false, is_active: true } } scope to the store endpoints. How is this typically done? Do i create a new repo method that accepts the extra scope arg? Because ExtendedFindConfig is used exclusively for list endpoints. @adrien2p @pKorsholm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a method in the service retrieveActiveNonInternal or something like that (can't find a good name)

Copy link
Member

@adrien2p adrien2p Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes what I do is the following:

  • create a protected retrieve method that accept selector, config
  • create a retrieveById that accept the actual retrieve arguments, call the protected method
  • create a retrieveInternalActive, call the protected method

you can look at the product service as an example

Copy link
Contributor

@olivermrbl olivermrbl Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add fixed filter params for store endpoint instead of a dedicated method?

E.g. in /store/products we add "published" as a fixed filter in the controllers.

Copy link
Contributor

@olivermrbl olivermrbl Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't create dedicated retrieve methods for what is just a filter. That does not scale super well 🤔

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think creating a retrieve method that takes a selector (maybe string | selector, where string is just an id) as @adrien2p suggested is a good approach, then you could add the parameters as a where object.

In that way you get all the retrieveByEmail etc. we have elsewhere as well and can create the filter in the controller as Oli suggests 😄

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, few minors

const [process, connection] = await startServerWithEnvironment({
cwd,
env: { MEDUSA_FF_PRODUCT_CATEGORIES: true },
verbose: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: rm

@olivermrbl olivermrbl changed the title feat(medusa): added store endpoint to retrieve a product category feat(medusa): Retrieve (service + controller) a product category Jan 12, 2023
Copy link
Contributor

@fPolic fPolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💪
I like the approach with transformTreeNodesWithConfig!

Copy link
Contributor

@pKorsholm pKorsholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a reminder to add tests for the additional where 😄


const productCategory = await productCategoryService.retrieve(
id,
retrieveConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think creating a retrieve method that takes a selector (maybe string | selector, where string is just an id) as @adrien2p suggested is a good approach, then you could add the parameters as a where object.

In that way you get all the retrieveByEmail etc. we have elsewhere as well and can create the filter in the controller as Oli suggests 😄

@riqwan riqwan force-pushed the feat/storefront-retrieve-endpoint branch from ade8d7f to 3f27cf9 Compare January 12, 2023 10:22
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is missing the test on the handler and it is good to go

Copy link
Contributor

@fPolic fPolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kodiakhq kodiakhq bot merged commit b2839e2 into develop Jan 12, 2023
@kodiakhq kodiakhq bot deleted the feat/storefront-retrieve-endpoint branch January 12, 2023 16:19
kodiakhq bot pushed a commit that referenced this pull request Jan 16, 2023
…3023)

**What:**

Introduces a store endpoint to retrieve a list of product categories

**Why:**

This is part of a greater goal of allowing products to be added to multiple categories.

**How:**

- Creates an endpoint in store routes

RESOLVES CORE-968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants