Enable multiple languages to be selected#644
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Sahil590
left a comment
There was a problem hiding this comment.
Overall LGTM. I had a few comments.
Would it be worth adding a check for empty values being passed? I know that the data will be reviewed before importing, but it may be missed.
| @admin.display(description="language", ordering="language") | ||
| def get_language_display(self, obj: LearningResource) -> Any: | ||
| """Enable displaying multiple languages in the list display.""" | ||
| return obj.get_language_display() | ||
|
|
There was a problem hiding this comment.
The way it's rendered in the admin is through check boxes rather than the way a foreign key field is rendered so not the cleanest. I guess this will always be imported rather than be created manually?
There was a problem hiding this comment.
Yea, that's just the default for this multiselectfield. TBH I think the checkboxes are better than the default one that requires you to shift+click to highlight multiple.
This bit of code you've highlighted is for the list display, so that we can see the names of the languages in the table of all learning resources.
Would it be worth adding a check for empty values being passed? I know that the data will be reviewed before importing, but it may be missed.
It's already disallowing empty values for language, so the import will return an error for any empty ones
There was a problem hiding this comment.
OK that's fine then. I thought there was something you could add in that file to render it differently. But this implementation works.
7557b62 to
1e38085
Compare
Description
This PR allows multiple languages in the Learning resources to be selected. This is necessary because the data contains this.
It does this using the django-multiselectfield package. Which requires a custom
MultipleChoiceWidgetto be made for the importing resources to workFixes #609
Type of change
Key checklist
python -m pytest)mkdocs serve)pre-commit run --all-files)Further checks