-
Notifications
You must be signed in to change notification settings - Fork 715
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
Add customizable demographic field entry to facility admin interface #12032
Add customizable demographic field entry to facility admin interface #12032
Conversation
… on a per facility basis.
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.
Overall the front-end looks great - super clean and straightforward implementation.
My comments are a couple questions and a picked nit but nothing blocking from me.
return value | ||
|
||
|
||
@deconstructible |
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 looked up what @deconstructible
does and thought it was weird that the FacilityUserDemographicValidator
class didn't have the decorator -- but then I looked at JSON_Schema_Validator
and saw that it was decorated.
So, just to be sure then, class decorators are inherited -- or, are they not inherited and the class below doesn't need the @deconstructible
decorator at all?
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 didn't explicitly add it to the FacilityUserDemographicValidator
because it isn't used in the validation for any field definitions within a model (and so doesn't need to be deconstructed and used within a migration) - it may incidentally be marked as such because of the inheritance though.
"items": { | ||
"properties": { | ||
"language": { | ||
"enum": [ |
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 blocking as it's a reasonably unlikely edge case.
Looks like if we ever add new languages then we'll need to update this list? Do we have any way to be sure we keep this list up-to-date w/ our available languages?
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 update the list of languages, Django will complain that we have model changes that need migrations, and we'll have to generate a new migration - this will fail CI checks, so we will notice :)
}, | ||
}, | ||
methods: { | ||
setInput(key, value) { |
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.
Non-blocking nitpick - it took me a couple reads of this to recognize this.value
and value
where indeed completely different things here 😅 -- it just read a little weird because of the duplicated name so might be helpful to call the param newVal
or something
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.
Fair!
When a new facility dataset is created, use default demographic settings if available.
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.
Much flavour 😋 💯
Yes - at the moment I have added these as 'admin only' fields, similarly to the id number. Possible that we will want to allow either customization of that on a per field basis, or just let it be user editable as well (other permissions permitting), but I've not heard that use case yet! |
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've reviewed it all and tested it. Everything worked fine, including the frontend.
After thinking I understand the code I still have a couple of doubts:
- Bulk export and import commands ignore all these changes. Obviously they're out of the scope of this PR but I wonder if creating a follow up issue to include them in the future makes sense. Is this a feature to be used in situations where moving many users between courses or creating many of them at once?
- Validation of changes in users only happens inside the serializer, thus, it will only used when DRF is used. Are we suppossing validation is not needed when creating or updating users in the future via the backend (i.e. are we suppossing our future code will always be correct? )
Per conversation in standup today - @jredrejo and @nucleogenesis both mentioned that their comments are non-blocking and that we can proceed, although some follow up discussion (and possible issues) might be helpful. Merging for patch 1 release, and will defer to @jredrejo and @rtibbles to sort out next steps |
033234c
into
learningequality:release-v0.16.x
Summary
extra_demographics
on the FacilityUser modelextra_demographics
of a facility user as thevalue
and returns updates to the whole object via the input eventReferences
Fixes #11999
Reviewer guidance
I can't attach Python files, but save this to a Python script:
and then run it with a fresh Kolibri home setup. Then run through the setup wizard, and when you go to the Facility user creation and editing confirm that the "Flavour" demographic field is now shown, and that it is editable.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)