[CI 4155] getRecommendations support variation id param #220
Conversation
Mudaafi
left a comment
Mudaafi
left a comment
There was a problem hiding this comment.
Looks good for the most part. I had a couple of questions but would you mind adding variation_id to RecommendationsRequestType and updating item_id to have the type string | Array as well?
| @@ -1,9 +1,10 @@ | |||
| import { ConstructorClientOptions, NetworkParameters, UserParameters, VariationsMap } from '.'; | |||
| import { ConstructorClientOptions, NetworkParameters, UserParameters, VariationsMap, FilterExpression } from '.'; | |||
There was a problem hiding this comment.
Thanks for fixing this! I wonder how long it was missing :x
src/modules/recommendations.js
Outdated
| @@ -120,6 +129,7 @@ class Recommendations { | |||
| * @param {string} podId - Pod identifier | |||
| * @param {object} [parameters] - Additional parameters to refine results | |||
| * @param {string|array} [parameters.itemIds] - Item ID(s) to retrieve recommendations for (strategy specific) | |||
There was a problem hiding this comment.
For the itemIds field, can we add "required if passing variationId"?
| } | ||
|
|
||
| if (variationId) { | ||
| if (!itemIds) { |
There was a problem hiding this comment.
Should we check if it's an empty array here and error out if it is?
src/modules/recommendations.js
Outdated
| * @param {string} podId - Pod identifier | ||
| * @param {object} [parameters] - Additional parameters to refine results | ||
| * @param {string|array} [parameters.itemIds] - Item ID(s) to retrieve recommendations for (strategy specific) | ||
| * @param {string|array} [parameters.variationId] - Variation ID to retrieve recommendations for (strategy specific) |
There was a problem hiding this comment.
I believe the type for this should just be string? I saw the RFC stating they'd want to prohibit multiple variation ids too.
jjl014
left a comment
jjl014
left a comment
There was a problem hiding this comment.
I went ahead and added a red path test, but otherwise LGTM! 🚀
jjl014
deleted the
ci-4155-node-client-get-recommendations-support-variation-id-param
branch
https://constructor.slab.com/posts/adding-variation-id-as-request-parameter-for-recommendation-requests-1sxpt91x#hcqxk-open-questions-uncertainties-risks