feat(medusa): categories can be ranked based on position #3341
Conversation
🦋 Changeset detectedLatest commit: 94a370e 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/ranking-categories
branch
from
ba9a574
to
6ec9963
Compare
riqwan
force-pushed
the
feat/ranking-categories
branch
from
de71f7e
to
0d55773
Compare
packages/medusa/src/api/routes/admin/product-categories/update-product-category.ts
Outdated
Show resolved
Hide resolved
riqwan
force-pushed
the
feat/ranking-categories
branch
from
7d7b709
to
c0baf45
Compare
| const targetPosition = input.position | ||
| const shouldChangeParent = | ||
| targetParentId !== undefined && targetParentId !== originalParentId | ||
| const shouldIncrementPosition = false |
There was a problem hiding this comment.
suggestion(non-blocking): It seems, we don't need a const for this one. Return false directly.
| }) | ||
|
|
||
| productCategory = await simpleProductCategoryFactory(dbConnection, { | ||
| name: "sweater", | ||
| parent_category: productCategoryParent, | ||
| is_internal: true, | ||
| position: 0 |
There was a problem hiding this comment.
suggestion: Should we call it rank to be coherent with the variant ranking as well?
| @@ -275,15 +340,31 @@ describe("/admin/product-categories", () => { | |||
| category_children: [ | |||
| expect.objectContaining({ | |||
| id: productCategoryChild2.id, | |||
| category_children: [] | |||
| category_children: [], | |||
There was a problem hiding this comment.
suggestion: Should we also check the name for the different object?
There was a problem hiding this comment.
we only care about position in this one.
There was a problem hiding this comment.
Yeah but since the handle is part of the order, to avoid regression it should be checked as well right? Otherwise someone can remove the order by handle and it will still be green even though the result is different
|
|
||
| const response = await api.post( | ||
| `/admin/product-categories`, | ||
| { |
There was a problem hiding this comment.
suggestion(nit): Move that to a const payload and then use it here plus line 461/4462 you can check payload.name and payload.handle. The idea is that if we need to change the values later for any reason, there is less things to change
| ) | ||
| }) | ||
|
|
||
| it("when position is greater than list count, position is updated to last element", async () => { |
There was a problem hiding this comment.
suggestion: ... is updated to elements count + 1
| ) | ||
| }) | ||
|
|
||
| it("when parent is updated with position, position accurately updated", async () => { |
There was a problem hiding this comment.
suggestion: ..., position should be updated
| @@ -16,6 +16,7 @@ describe("/store/product-categories", () => { | |||
| let productCategoryParent = null | |||
| let productCategoryChild2 = null | |||
| let productCategoryChild3 = null | |||
| let productCategoryChild4 = null | |||
There was a problem hiding this comment.
suggestion: same changes as the admin file
| expect(productCategoryRepository.findDescendantsTree).toHaveBeenCalledTimes(1) | ||
| expect(productCategoryRepository.getFreeTextSearchResultsAndCount).toHaveBeenCalledWith( | ||
| { | ||
| order: { |
There was a problem hiding this comment.
suggestion: Since we are ordering by default, does it make sense to have this one?
| @@ -59,14 +62,15 @@ describe("ProductCategoryService", () => { | |||
| expect(productCategoryRepository.getFreeTextSearchResultsAndCount).toHaveBeenCalledWith( | |||
| { | |||
| order: { | |||
| created_at: "DESC", | |||
There was a problem hiding this comment.
suggestion: Since we are ordering by default, does it make sense to have this one?
| }, | ||
| treeSelector: QuerySelector<ProductCategory> = {} | ||
| ): Promise<[ProductCategory[], number]> { | ||
| const includeDescendantsTree = selector.include_descendants_tree | ||
| const includeDescendantsTree = selector.include_descendants_tree || false |
There was a problem hiding this comment.
suggestion(nit): !!selector.include_descendants_tree here if it is false it will go to the || condition
| q, | ||
| treeSelector, | ||
| includeDescendantsTree | ||
| ) |
There was a problem hiding this comment.
todo: update jsdoc for the retrieve method
|
/snapshot-this |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
What:
Introduces ranking into product categories.
Why:
For categories to be displayed in a particular way that isn't based on the existing sorting algos we provide.
How:
Introduces a position column that is unique by tree node and position number that cannot go negative.
It also introduces some heavy logic in the services that manages ranking information on create/update/delete.
RESOLVES CORE-1174
RESOLVES CORE-1183