-
Notifications
You must be signed in to change notification settings - Fork 4
[CDX-380] Add support for variationId param in RecommendationsRequest #183
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
Changes from all commits
7c488f3
07c0301
1a46519
1a6c884
6a38c34
c975218
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1811,6 +1811,17 @@ public String recommendationsAsJSON(RecommendationsRequest req, UserInfo userInf | |
| } | ||
| } | ||
|
|
||
| if (StringUtils.isNotBlank(req.getVariationId())) { | ||
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (req.getItemIds() == null || req.getItemIds().size() != 1) { | ||
| throw new IllegalArgumentException( | ||
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
esezen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "variationId requires exactly one itemId to be specified"); | ||
| } | ||
| url = | ||
| url.newBuilder() | ||
| .addQueryParameter("variation_id", req.getVariationId()) | ||
| .build(); | ||
| } | ||
|
Comment on lines
+1814
to
+1823
|
||
|
|
||
| if (StringUtils.isNotBlank(req.getTerm())) { | ||
| url = url.newBuilder().addQueryParameter("term", req.getTerm()).build(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -29,6 +29,47 @@ public void getRecommendationsShouldReturnAResultWithSingleItemId() throws Excep | |||||||||||||
| assertTrue("recommendation result id exists", response.getResultId() != null); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| @Test | ||||||||||||||
| public void getRecommendationsShouldReturnAResultWithItemIdAndVariationId() throws Exception { | ||||||||||||||
| ConstructorIO constructor = new ConstructorIO("", apiKey, true, null); | ||||||||||||||
| UserInfo userInfo = new UserInfo(3, "c62a-2a09-faie"); | ||||||||||||||
| RecommendationsRequest request = new RecommendationsRequest("item_page_1"); | ||||||||||||||
| request.setItemIds(Arrays.asList("power_drill")); | ||||||||||||||
| request.setVariationId("power_drill_variation"); | ||||||||||||||
| RecommendationsResponse response = constructor.recommendations(request, userInfo); | ||||||||||||||
|
||||||||||||||
| RecommendationsResponse response = constructor.recommendations(request, userInfo); | |
| RecommendationsResponse response = constructor.recommendations(request, userInfo); | |
| assertEquals( | |
| "variation_id in request should match the variationId set", | |
| "power_drill_variation", | |
| response.getRequest().get("variation_id")); |
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.
@quiteeasy I think this makes sense. What do you think?
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
constructor-claude-bedrock[bot] marked this conversation as resolved.
Show resolved
Hide resolved
esezen marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Mar 25, 2026
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 new test’s assertion response.getResponse().getResults().size() >= 0 is always true for any non-null list, so it won’t catch regressions. Prefer asserting that the results list is non-null (and/or size() > 0 if non-empty results are expected) and/or asserting the server-understood request includes the expected variation_id value.
Uh oh!
There was an error while loading. Please reload this page.