-
Notifications
You must be signed in to change notification settings - Fork 770
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 ExamSerializers according to v3 model #11674
Update ExamSerializers according to v3 model #11674
Conversation
self.assertEqual(response.status_code, 400) | ||
self.assertFalse(models.Exam.objects.filter(title=title).exists()) | ||
|
||
def test_quiz_section_with_no_resource_pool(self): |
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.
Was curious about the resource pool and if it's required!
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.
One quick change requested to update a docstring -- otherwise, this all looks good to me.
With the docstring updated this can be merged.
@@ -195,8 +200,8 @@ export default function useQuizCreation(DEBUG = false) { | |||
* @affects _channels - Calls _fetchChannels to bootstrap the list of needed channels | |||
* Adds a new section to the quiz and sets the activeSectionID to it, preparing the module for | |||
* use */ | |||
function initializeQuiz() { | |||
set(_quiz, objectWithDefaults({}, Quiz)); | |||
function initializeQuiz(collection) { |
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.
Could you add a @param {string} collection - The collection (aka current class ID) to associate the exam with
to the doc string above here?
123b1d0
to
cb88abc
Compare
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.
I am fairly sure that this will break horribly if we attempt to load an exam object from the database that predates the v3 model.
Did you test this scenario? It would be good to retain backend tests for that case as well to give us assurance (noting that the mapping between versions happens in the frontend).
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 are still two places here where breakage will happen.
kolibri/core/exams/serializers.py
Outdated
def to_representation(self, instance): | ||
data = super().to_representation(instance) | ||
if "question_sources" in data and data["question_sources"]: | ||
first_question_source = data["question_sources"][0] |
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.
Seems like this conditionality could just be done based on the data_model_version
rather than this sniff test?
Docs failure is unrelated to this PR, merging. |
Summary
References
closes #11580
Reviewer guidance
debug=true
onfunction useQuizCreation
in the code.Testing checklist
PR process
Reviewer checklist
yarn
andpip
)