-
Notifications
You must be signed in to change notification settings - Fork 11
[CSL-2208] Migrate Quizzes /finalize endpoint to new /results endpoint #244
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
[CSL-2208] Migrate Quizzes /finalize endpoint to new /results endpoint #244
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.
Main functionality is working great. Thank you for adding this and giving great attention to types. Just added some comments. let me know what you think
| expect(res).to.have.property('quiz_id').to.be.an('string'); | ||
| expect(fetchSpy).to.have.been.called; | ||
| expect(requestedUrlParams).to.have.property('section').to.equal(section); | ||
| }); |
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.
Is section relevant to quizzes. I thought it's not
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.
Also news to me but yes different sections can have different Quizzes
| expect(res.response).to.have.property('results').to.be.an('array'); | ||
| expect(requestedUrlParams).to.have.property('key'); | ||
| expect(requestedUrlParams).to.have.property('i'); | ||
| expect(requestedUrlParams).to.have.property('s'); |
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 also add an expect statement to check for quiz_session_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.
Also as I remember that if we provide the quiz_session_id as a parameter it should be returned in the response. If what I'm remembering is correct can we add a test case for this as well?
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 be handled here #245
spec/src/modules/quizzes.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('Should return a result given valid API key, answers, and resultsPerPage parameters', () => { |
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.
I think you meant and "filters" here instead of "resultsPerPage"
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.
Good catch! Thank you
| } | ||
|
|
||
| export interface QuizzesResultsParameters extends QuizzesParameters { | ||
| answers: any[]; |
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.
answers already part of QuizzesParameters did you intend to have it here also
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.
They are optional there but required here. Is this the right way to do it?
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.
Looks right to me didn't notice that it's required here
| * @param {string} [parameters.versionId] - Specific version identifier for the quiz | ||
| * @param {number} [parameters.page] - The page number of the results | ||
| * @param {number} [parameters.resultsPerPage] - The number of results per page to return | ||
| * @param {object} [parameters.filters] - Key / value mapping (dictionary) of filters used to refine results |
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.
we're missing the quiz_session_id as a parameter
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.
Handled here #245
|
|
||
| if (parameters) { | ||
| const { section, answers, versionId } = parameters; | ||
| const { section, answers, versionId, page, resultsPerPage, filters } = parameters; |
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.
reminder to destruct quiz_session_id here as well?
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.
Handled here #245
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!
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! 🚀
No description provided.