-
Notifications
You must be signed in to change notification settings - Fork 45
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
WIP: Add filter by availability on products search #37
Conversation
node/resolvers/catalog/index.ts
Outdated
}) | ||
let productsFiltered = products | ||
|
||
if (isAvailable) { |
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.
Não acho uma boa fazer esse tratamento aqui, de certa forma vai retornar um resultado errado para o usuário.
Exemplo:
- na query de products peço os 4 primeiros produtos disponívels
- o resolver faz o request para o rest pedindo os 4 primeiros elementos (nesses 4 podem vir por exemplo 2 disponíveis e os outros 2 não, sendo que podem existir outros 100 produtos disponívels mais a frente)
- o algoritmo faz o filter e retorna para o front-end somente esses 2 elementos
- o que é um resultado errado pois eu pedi 4 disponíveis e só me retornou 2, existem outros dois que poderiam estar dentro do retorno, mas não foram retornados na requisição do REST
existe um filtro de disponibilidade, dá uma olhada -> https://documenter.getpostman.com/view/845/search-103/Hs43#e0ec3923-faf7-5049-8634-f6d617ce5f67
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 filter doesn't work as you are expecting! Example, the product can be available in one salesChannel but have 0 items in stock.
node/resolvers/catalog/index.ts
Outdated
}) | ||
let productsFiltered = products | ||
|
||
if (isAvailable) { |
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.
Extract to separated well-documented function.
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.
Where should this function be located?
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 same file, just extract to a helper function and document.
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.
👍
node/resolvers/catalog/index.ts
Outdated
@@ -25,6 +25,25 @@ const extractSlug = item => { | |||
return item.criteria? `${href[3]}/${href[4]}` : href[3] | |||
} | |||
|
|||
/** | |||
* Filter the list of products by the available quantity in stock |
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.
Filter that list products by the available quantity in stock
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.
That? Which one? Wouldn't it be better to use: Filter a list...
node/resolvers/catalog/index.ts
Outdated
).length > 0 | ||
) | ||
}) | ||
|
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.
remove line blank
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.
👍
node/resolvers/catalog/index.ts
Outdated
|
||
let productsFiltered = products | ||
if (isAvailable) { | ||
productsFiltered = filterProductsAvailableQuantity(productsFiltered) |
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.
Cria um issue para o problema que falei, vou aprovar, mas esse resolver tá retornando um resultado enganoso.
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.
👍
node/resolvers/catalog/index.ts
Outdated
|
||
let productsFiltered = products | ||
if (isAvailable) { | ||
productsFiltered = filterProductsAvailableQuantity(productsFiltered) |
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.
filterProductsAvailable
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.
👍
What is the purpose of this pull request?
Add filter by availability on products search
What problem is this solving?
Now the products can be filtered by availability
How should this be manually tested?
GraphiQL
Screenshots or example usage
Types of changes