-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add quizResultsConfig #142
Add quizResultsConfig #142
Conversation
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 is looking good. My only small concern is the QuizRequest
the new functions take in. This endpoint only takes in quizVersionId
and section
(and not answers
etc. like others). I'm wondering if that would be confusing for users where they won't be sure which parameters needs to be passed. WDYT?
@esezen Created a base constructorio-client/src/main/java/io/constructor/client/QuizRequestBase.java class that is a parent class of
|
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.
LGTM! Let's get one more review since we are changing some existing code
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.
Nice, this looks awesome!
There's just a couple small things to clean up, but otherwise LGTM! 🚀
* @return a Task OkHttp request | ||
* @throws ConstructorException | ||
*/ | ||
protected Request createQuizRequest(QuizRequest req, String type, UserInfo userInfo) | ||
protected Request createQuizRequest(QuizRequestBase req, String type, UserInfo userInfo) |
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.
Whoa, this is super cool! 😮 I didn't realize this was possible! 🔥
/** | ||
* Queries the quiz service for the quiz results page configurations | ||
* | ||
* @param req the Quiz request |
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.
Should this say "the Quiz Results Config request" instead?
* Queries the quiz service for the quiz results page configurations | ||
* | ||
* @param req the Quiz request | ||
* @return a Quiz Results Response |
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.
Similar to above "@return a Quiz Results Config Response"
@Rule public ExpectedException thrown = ExpectedException.none(); | ||
|
||
@Test | ||
public void QuizResultsConfigShouldReturnResuls() throws Exception { |
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.
Should we change the test name to QuizResultsConfigShouldReturnAResponse
since it doesn't return results?
This would apply to the test names below as well: Result
=> Response
public void QuizResultsShouldReturnErrorWithInvalidQuizId() throws Exception { | ||
ConstructorIO constructor = new ConstructorIO("", quizKey, true, "quizzes.cnstrc.com"); | ||
QuizResultsConfigRequest request = new QuizResultsConfigRequest("invalidQuiz"); | ||
|
||
thrown.expect(ConstructorException.class); | ||
thrown.expectMessage( | ||
"[HTTP 404] The quiz you requested, \"invalidQuiz\" was not found, please specify a" | ||
+ " valid quiz id before trying again."); | ||
QuizResultsConfigResponse response = constructor.quizResultsConfig(request, null); | ||
} | ||
|
||
@Test | ||
public void QuizResultsAsJsonShouldReturnErrorWithInvalidQuizId() throws Exception { | ||
ConstructorIO constructor = new ConstructorIO("", quizKey, true, "quizzes.cnstrc.com"); | ||
QuizResultsConfigRequest request = new QuizResultsConfigRequest("invalidQuiz"); | ||
|
||
thrown.expect(ConstructorException.class); | ||
thrown.expectMessage( | ||
"[HTTP 404] The quiz you requested, \"invalidQuiz\" was not found, please specify a" | ||
+ " valid quiz id before trying again."); | ||
String response = constructor.quizResultsConfigAsJson(request, null); | ||
} | ||
|
||
@Test | ||
public void QuizResultsShouldReturnErrorWithInvalidIndexKey() throws Exception { | ||
ConstructorIO constructor = new ConstructorIO("", "invalidKey", true, "quizzes.cnstrc.com"); | ||
QuizResultsConfigRequest request = new QuizResultsConfigRequest(quizId); | ||
|
||
thrown.expect(ConstructorException.class); | ||
thrown.expectMessage( | ||
"[HTTP 404] The quiz you requested, \"" | ||
+ quizId | ||
+ "\" was not found, please specify a valid quiz id before trying again."); | ||
QuizResultsConfigResponse response = constructor.quizResultsConfig(request, null); | ||
} | ||
|
||
@Test | ||
public void QuizResultsAsJsonShouldReturnErrorWithInvalidIndexKey() throws Exception { |
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.
Test name prefix change from QuizResults
=> QuizResultsConfig
/** | ||
* Queries the quiz service for the quiz results configuration | ||
* | ||
* @param req the Quiz request |
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.
Similar to above, should this say "the Quiz Results Config request" instead?
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.
Nice! This is looking great to me!
Thanks for working on the changes! 🚀
Added
Tests