-
Notifications
You must be signed in to change notification settings - Fork 699
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
Update exam.question_count calculation #12196
Update exam.question_count calculation #12196
Conversation
Build Artifacts
|
@@ -278,7 +278,7 @@ export default function useQuizCreation() { | |||
*/ | |||
function saveQuiz() { | |||
const totalQuestions = get(allSections).reduce((acc, section) => { | |||
acc += parseInt(section.question_count); |
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.
@nucleogenesis - this value again feels like something we maybe shouldn't even store in the backend, as all it seems to represent is a user intention of how many questions there should be in the 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.
Yeah that's a good point - it can always be derived from the length of questions anyway.
@AlexVelezLl I'll include the removal of saving this in my PR #12200
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 like the right change for the existing method of saving the question count from the frontend - my only question is whether this is something that needs to happen on the frontend at all?
If the totalQuestions is only set during saving to be set onto the backend (and is otherwise ignored), we could just set this value in the backend by doing a similar operation in the serializer, and then the frontend doesn't need to worry about it at all.
Thank you @rtibbles. That makes a lot of sense. I will then move this logic to the serializer. |
@@ -137,6 +137,12 @@ def to_internal_value(self, data): | |||
else: | |||
# Otherwise we are just updating the exam, so allow a partial update | |||
self.partial = True | |||
|
|||
question_sources = data.get("question_sources", []) |
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.
Might be good to update this: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/exams/test/test_exam_api.py#L45 to remove this from the data we send to the backend in tests, and then add a test to assert that the value is properly set on create?
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 can do this in #12200 to keep the clean-up separate from the fix here.
Summary
Update the calculation of
exam.question_count
to take into account the length of thesection.questions
array instead of thesection.question_count
since the latter can have a value assigned to it, but the actual value of thesection.questions
array be empty because it does not has assigned resources.For both examples below, I created a quiz with 7 questions on the first section and 10 on the second, but in the latter I didnt assinged any resource, so no questions appears on this section.
Before:
Here we have an exam created before these changes:
Students see 17 total questions, and "next" button appears, but there are no more questions.
Coach reports show x/17 total questions answered.
After:
Here we have an exam created after these changes:
Students see right amount of total questions.
Coach reports show "all questions answered"
References
Closes #12149
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)