Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RecommendationsRequest request = new RecommendationsRequest("item_page_1"); | ||
| request.setItemIds(Arrays.asList("power_drill")); | ||
| request.setVariationId("power_drill_variation"); | ||
| RecommendationsResponse response = constructor.recommendations(request, userInfo); |
There was a problem hiding this comment.
This test sets variationId, but it doesn’t assert that the client actually sent variation_id (or that the server understood it). Since RecommendationsResponse exposes the parsed request map, add an assertion that response.getRequest().get("variation_id") matches the value you set so the test validates the new behavior.
| 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.
@quiteeasy I think this makes sense. What do you think?
| @@ -54,6 +55,7 @@ public void settersShouldSet() throws Exception { | |||
| request.setPodId("zero_results_1"); | |||
| request.setNumResults(3); | |||
| request.setItemIds(Arrays.asList("1", "2", "3")); | |||
There was a problem hiding this comment.
setVariationId is documented as only valid when exactly one item_id is specified, but this test sets itemIds to three values and then sets variationId. Consider updating the test setup to use a single itemId when setting variationId (or split into separate cases) so the tests align with the documented constraint.
| request.setItemIds(Arrays.asList("1", "2", "3")); | |
| request.setItemIds(Arrays.asList("1")); |
esezen
left a comment
esezen
left a comment
There was a problem hiding this comment.
@quiteeasy this is looking good. Copilot left some good comments. Do you mind taking a look?
| RecommendationsRequest request = new RecommendationsRequest("item_page_1"); | ||
| request.setItemIds(Arrays.asList("power_drill")); | ||
| request.setVariationId("power_drill_variation"); | ||
| RecommendationsResponse response = constructor.recommendations(request, userInfo); |
There was a problem hiding this comment.
@quiteeasy I think this makes sense. What do you think?
| @@ -54,6 +55,7 @@ public void settersShouldSet() throws Exception { | |||
| request.setPodId("zero_results_1"); | |||
| request.setNumResults(3); | |||
| request.setItemIds(Arrays.asList("1", "2", "3")); | |||
There was a problem hiding this comment.
Code Review
This PR adds variationId support to RecommendationsRequest, including server-side validation that enforces it must be used with exactly one itemId. The implementation is clean and consistent with the existing codebase patterns, with good test coverage for both the happy path and error cases.
Inline comments: 4 discussions added
Overall Assessment:
constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java
Show resolved
Hide resolved
constructorio-client/src/test/java/io/constructor/client/ConstructorIORecommendationsTest.java
Show resolved
Hide resolved
constructorio-client/src/test/java/io/constructor/client/ConstructorIORecommendationsTest.java
Show resolved
Hide resolved
constructorio-client/src/main/java/io/constructor/client/RecommendationsRequest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
constructorio-client/src/test/java/io/constructor/client/ConstructorIORecommendationsTest.java
Show resolved
Hide resolved
esezen
deleted the
cdx-380-java-sdk-add-support-for-variation-id-param-in
branch
No description provided.