-
Notifications
You must be signed in to change notification settings - Fork 176
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
Consolidate mastery criteria #3426
Consolidate mastery criteria #3426
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.
Added some comments regarding the extra_fields
data in your tests
metadata = self.contentnode_db_metadata | ||
metadata["extra_fields"] = { | ||
"options": { | ||
"threshold": { |
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 structure of threshold
and model
are new, so having those directly underneath options
doesn't currently exist. What's your intent here-- to emulate the old/existing structure?
The old/existing structure should be:
metadata["extra_fields"] = {
"options": {
"m": 3,
"n": 6,
"mastery_model": exercises.M_OF_N,
}
}
Since you added your consolidate_extra_fields
to field_map
, which is invoked during serialization of a content node, a good test would be to ensure that serializing a content node with the old mastery structure, like in a GET request, produces the new structure.
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 a mistake here. The old format was at the top level of extra_fields
, not extra_fields.options
. So in your tests, the old data should be moved up a level, and a change to your consolidate function to match. I got hung up on the existence of threshold
that you had previously, I missed that the old format wasn't inside options
. My apologies!
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 change requested regarding print
statement.
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.
Sorry! Just noticed one last thing - the old style data is being set as this:
extra_fields={
"options": {
"m": None,
"n": None,
"mastery_model": None,
}
}
but it should be at the top level of extra_fields like this:
extra_fields={
"m": None,
"n": None,
"mastery_model": None,
}
Will need a couple of tweaks in the consolidate_extra_fields
function too.
Summary
Description of the change(s) you made
Manual verification steps performed
Comments
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)