-
Notifications
You must be signed in to change notification settings - Fork 714
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
Exams: Create them, take them, view reports #12111
Exams: Create them, take them, view reports #12111
Conversation
Build Artifacts
|
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.
A few questions from a first read through - I may be wrong, but would like more assurance as to why I am wrong :)
@@ -15,7 +14,7 @@ import { ExamResource, ContentNodeResource } from 'kolibri.resources'; | |||
* @returns {array} - pseudo-randomized list of question objects compatible with v1 like: | |||
* { exercise_id, question_id } | |||
*/ | |||
function convertExamQuestionSourcesV0V2(questionSources, seed, questionIds) { | |||
function convertExamQuestionSourcesV0V1(questionSources, seed, questionIds) { |
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 function name has been changed, but the output of this function hasn't changed at all, so what justifies the name change here?
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.
From what I see in the JSDoc, it seems that the purpouse was converting v0 to v1, so probably the function had a wrong name?
I looked for convertExamQuestionSourcesV0V2
matches in the code, and I found this in a comment: https://github.com/AlexVelezLl/kolibri/blob/366b38342aa43930ce1c0f2a2c9b695258b40841/kolibri/core/exams/models.py#L134. I think it would be valuable also update this name there.
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.
Just realized that this function indeed returns a V2 structure, since it includes the counter_in_exercise
prop.
@@ -63,16 +62,14 @@ function convertExamQuestionSourcesV0V2(questionSources, seed, questionIds) { | |||
|
|||
function convertExamQuestionSourcesV1V2(questionSources) { | |||
// In case a V1 quiz already has this with the old name, rename it | |||
if (every(questionSources, 'counterInExercise')) { |
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 am not sure I understand the purpose of changing these functions - is there a bug here we are fixing?
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.
When I read this I thought it might miss a case where question sources has some camelCase but not every ... so naturally I overthought it. Pushed a change where it'll just use some
instead of every
which seems reasonable unless we're sure it's impossible to have both types in the same sources list.
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.
@AlexVelezLl this also does away with the copy / reference issue altogether using the for loop to update things in place
resolve(exam); | ||
} | ||
}).then(exam => { | ||
if (data_model_version <= 1) { |
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.
Surely after each step here, we would need to update the data_model_version, otherwise we will update to v2 and no further, if the data_model_version is 1?
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 subsequent if-statements will run too because 1 <= 2 == true then the next because 1 <= 3, etc.
If we updated the data_model_version I don't think that would impact how this works, though, and the checks could be made to be ===
checks rather than relying on this fall-through approach.
I think that my reasoning for not updating the data_model_version
was thinking the data on the front-end should reflect what is persisted to the DB in this case -- but thinking a bit more now it does make sense to have that particular value reflect the structure of the data as it is now as it is passed around the front-end.
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.
Yeah - I think my question here was connected to the v0v2 function - which converts from v0v2 (not v0 to v1, I think) and so as you correctly point out it got passed along here, we would be passing v2 stuff into the v1 to v2 because it had already been converted (although now, I do understand your desire to make each function do a single version leap).
@@ -447,14 +443,14 @@ describe('exam utils', () => { | |||
title: 'Count with small numbers', | |||
}, | |||
]; | |||
expect(converted).toEqual( | |||
expect(converted.question_sources[0].questions).toEqual( | |||
expectedOutput.map(q => { | |||
q.item = `${q.exercise_id}:${q.question_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.
I see the composite id we were using previously is being set as item
on the questions by our functions here, is this not being used in the coach reports (i.e. why did we have to set it up as id
separately?)
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 item
property seems to be used in some parts of the front-end. I just kept it here rather than taking on the task of unifying them under one name right now.
['A1', 'A2', 'A3'], | ||
['B1', 'B2', 'B3', 'B4', 'B5', 'B6', 'B7', 'B8'], | ||
['C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'C8', 'C9'], | ||
['A:A1', 'A:A2', 'A:A3'], |
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 looks a little suspicious to me - why are we updating this to the colon separated item values, when this is specifically the QUESTION_IDS with no other changes to the tests - or are we just not making the correct assertions about this in the tests for this to matter, and this should actually be the ITEM_IDS not the QUESTION_IDS?
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 that it's worth updating the API for the selectQuestions function a bit because it now uses the unique composite IDs rather than the question/exercise ids separately.
I'll look at it again and reconsider some of the naming and add comments -- might come up with a follow-up "Refactor selectQuestions & tests" issue
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 made this commit hoping to clear up the expectations in the test & selectQuestions function call.
I have been inclined to just make id
the key for the composite value rather than item
and have a follow-up issue on deck to update the places that use item
to call it id
and add comments
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 there might be an argument to double down on the item instead. Can double check when I am back at my desk, but it is used fairly consistently into the logging layer 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.
See here where it ultimately ends up in the backend
kolibri/kolibri/core/logger/models.py
Line 277 in 7368f26
item = models.CharField(max_length=200, validators=[MinLengthValidator(1)]) |
I think there's probably some systemic updates to be made to be more consistent.
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.
Awesome thanks for this! I've created this issue to align quiz creation with the use of item
for the unique ID in particular.
@@ -15,7 +14,7 @@ import { ExamResource, ContentNodeResource } from 'kolibri.resources'; | |||
* @returns {array} - pseudo-randomized list of question objects compatible with v1 like: | |||
* { exercise_id, question_id } | |||
*/ | |||
function convertExamQuestionSourcesV0V2(questionSources, seed, questionIds) { | |||
function convertExamQuestionSourcesV0V1(questionSources, seed, questionIds) { |
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.
From what I see in the JSDoc, it seems that the purpouse was converting v0 to v1, so probably the function had a wrong name?
I looked for convertExamQuestionSourcesV0V2
matches in the code, and I found this in a comment: https://github.com/AlexVelezLl/kolibri/blob/366b38342aa43930ce1c0f2a2c9b695258b40841/kolibri/core/exams/models.py#L134. I think it would be valuable also update this name there.
copy.counter_in_exercise = copy.counterInExercise; | ||
return annotateQuestionSourcesWithCounter( | ||
questionSources.map(question => { | ||
const copy = question; |
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 copy
object here is doing nothing I think. The code is equivalent as if we replace all copy
references with question
, as the pointer is the same. If the intention of the first implementation was to avoid editing the original object, we need to make a copy like const copy = { ...question }
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.
Great catch thank you!
if (data_model_version <= 2) { | ||
exam.question_sources = convertExamQuestionSourcesV2toV3(exam); | ||
} | ||
if (data_model_version <= 3) { |
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 we can remove this if here, and just excecute the body for all cases. If in the future we have a v4, Wouldn't we need to annotate the questions with item?
@@ -15,7 +14,7 @@ import { ExamResource, ContentNodeResource } from 'kolibri.resources'; | |||
* @returns {array} - pseudo-randomized list of question objects compatible with v1 like: | |||
* { exercise_id, question_id } | |||
*/ | |||
function convertExamQuestionSourcesV0V2(questionSources, seed, questionIds) { | |||
function convertExamQuestionSourcesV0V1(questionSources, seed, questionIds) { |
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.
Just realized that this function indeed returns a V2 structure, since it includes the counter_in_exercise
prop.
export async function convertExamQuestionSources(exam) { | ||
const { data_model_version } = exam; | ||
|
||
return new Promise(resolve => { |
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.
If we are already using the async
keyword in the function signature, wouldnt be better to use await instead of this promise? So the code is flatter.
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.
Old habits die hard 😅 - good idea! I'll revise a bit
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.
@AlexVelezLl looks so much nicer using await - lemme know what you think
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.
Yees, this looks so much nicer! 👐
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.
Still a couple of things that need to be tweaked here. And one question about the reports.
return exam; | ||
} | ||
|
||
// TODO: Reports will eventually want to have the proper section-specific data to render |
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 not, as in within this PR, or not within this release? Won't that just give us the same extremely long question issue in reports that we have been trying to avoid in the taking of quizzes?
} | ||
|
||
return annotateQuestionsWithItem(exam.question_sources); | ||
// TODO This avoids updating older code that used `item` to refer to the unique question 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.
Looks like this needs to be updated/removed in line with this issue, which is binding us to the opposite course of action: #12127
Maybe we should just update the annotateQuestionsWithItem to operate on a question_sources in the v3 shape?
methods: { | ||
saveQuizAndRedirect() { | ||
this.saveQuiz().then(() => { | ||
this.$router.replace({ name: PageNames.EXAMS }); |
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 route requires a classId parameter: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/coach/assets/src/routes/planExamRoutes.js#L31
@@ -55,61 +55,63 @@ export function showExam(store, params, alreadyOnQuiz) { | |||
contentNodes => { | |||
if (shouldResolve()) { | |||
// If necessary, convert the question source info | |||
const question_sources = convertExamQuestionSourcesToV3(exam, { contentNodes }); |
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.
Not here, but in the unedited code above, couldn't this use the updated util function that you made in this PR that fetches the exam and the content nodes all at once?
Also gets reports working
…ront-end data as it is converted
…osite :-separated ID
this may be better suited for an svg though
7ddd571
to
c67bf2c
Compare
This continues to be intentional, although we could potentially add some leeway here in the case that the quiz has never been activated. The issues arise when a coach edits a quiz when students have already taken it, as without some sort of version control in place, we would not be able to properly parse responses from older versions. |
this seems like it could be useful, as with the new editing options, being able to edit/revise the quiz (before made visible to learners), seems more like expected behavior than with our current quiz selection/creation flow. But I don't know that it necessarily needs to be MVP. If it's not though, maybe some helper text at "save" would be useful. |
Ditto to what Marcella said: we're adding more complex (enhanced) features to quiz creation, but we're not offering any options for revision and corrections? 🤔 Agreed that the limitation needs to be before the quiz is made visible to learners, but I can only imagine the frustration of the coach who invested time and effort to carefully craft the sections, add instructions and descriptions, select and order questions, and then they have to do it all over from scratch just because they discovered a typo after saving... 😭 Not to mention that error prevention is part of the WCAG 😉 |
At a minimum if we decide not to include it in the MVP, we must add the |
Users can duplicate a quiz, so in that case I think it makes even more sense to include the ability to "re-open" a quiz for editing within the quiz creation tool. I think that in the medium-term it'd be ideal for us to revise some of the UX around this, but in the short term I think that we could get @tomiwaoLE 's thoughts on how/where to put the button / option to edit a quiz. I think maybe having an "Edit quiz" button next to / near the "Start quiz" button might be worth considering along with adding text to the "Start quiz" modal (where we say 'this will make learners download 1234kb') to say "You will no longer be able to edit this quiz" or something? |
I've fixed the accordion expand/collapse issue. I also fixed the issue where you ended up with 107 questions when it was supposed to be 17... Javascript is fun because it ended up that rather than trying to add You should be able now to create a quiz and test it. I'm honestly not super sure exactly why the number was being passed around as a string rather than a number -- like, I feel I should have noticed this with all of the quizzes I've been taking. I think that maybe it was to do with whether or not you click the +/- buttons to change the number or type in the number. When I selected the box and typed in 7 on the second section, it ended up thinking there were 107 questions. But as far as I can remember I virtually always click the buttons rather than type it in 🤷🏻♂️ Fun and silly bug :) I mentioned to @marcellamaki in a chat today that I did find it weird that I couldn't submit a quiz so I looked back at 0.16 and it's always visible there. I looked in the Figma thinking that I'd find a discussion around the fact that the new designs remove the previously always-visible "Submit quiz" button that you'd see in 0.16. So I'll ask for them to weigh in on that separately from this PR. |
Hi @nucleogenesis, great to see so much progress made here! Wow! Today I've started testing scenarios for users upgrading from Kolibri 0.15.2 to this latest version. So far I am happy to report that almost everything is working perfectly fine and as expected.
Missing.File.size.to.download.mp4
Cannot.open.a.question.in.Difficult.questions.mp4
Here are both home folders in case you need them: |
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.
Nothing blocking, but still some questions.
/** | ||
* @returns {Promise} - resolves to an object with the exam and the exercises | ||
*/ | ||
export async function fetchExamWithContent(exam) { |
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 function name is mildly misleading, as it suggests this function fetches the exam, whereas it actually requires the exam to already have been fetched and passed in as the only argument.
@@ -557,6 +561,7 @@ | |||
left: 0; | |||
padding: 1em; | |||
margin-top: 1em; | |||
background-color: #ffffff; |
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 be setting this to themeTokens.surface instead? https://github.com/learningequality/kolibri-design-system/blob/release-v4/lib/styles/colorsDefault.js#L18
Summary
I've tried my best to sort of fold together the existing data architecture code for Coach quiz reports and Learner quiz reports with the updated data structures.
I've kept front-end architecture shaped the same, but updated the shape of the data, basically testing out the reports and such, finding bugs, then fixing them. The changes were in relatively self-contained / purpose-built modules -- that is to say that I've tried to be sure that there are no chances of regressions.
The
exam.utils
module has been significantly updated. Now there are functions going from eachdata_model_version
to the next (ie, v0-v1, v1-v2, etc).There appear to be two things needed from this module at this time:
Now there is a general function for the first part and the second function does the conversion and fetches the nodes.
References
Closes #12097
Reviewer guidance
Backward compatibility
A big part of this change involves ensuring backward compatibility. There are automated tests that do this, but I've not tested with any "data model version" below 2.
Seems like to get an exam < v2 you will need to create a quiz using Kolibri <=0.11 as V2 seems to have been added into 0.12.
Here you should be able to do the following: