Skip to content

[Frontend/Course Admin] Additional fields feature#825

Merged
anthonygego merged 19 commits into
masterfrom
tags_additional_fields
Oct 17, 2022
Merged

[Frontend/Course Admin] Additional fields feature#825
anthonygego merged 19 commits into
masterfrom
tags_additional_fields

Conversation

@Drumor
Copy link
Copy Markdown
Collaborator

@Drumor Drumor commented Jun 20, 2022

a) Move tags page
b) add "additional fields" feature

@Drumor Drumor added this to the Additional fields for a course milestone Jun 20, 2022
@anthonygego
Copy link
Copy Markdown
Member

anthonygego commented Jul 12, 2022

Please separate a) and b) so that the review of b) can be done easier, as a) review should be quite straightforward.
Please also fix the remaining conflict.

@Drumor Drumor force-pushed the tags_additional_fields branch from a86b49a to c763035 Compare July 13, 2022 09:44
@Drumor
Copy link
Copy Markdown
Collaborator Author

Drumor commented Jul 13, 2022

A part is now in PR #841

@anthonygego
Copy link
Copy Markdown
Member

anthonygego commented Jul 15, 2022

Can you rebase this PR now ?

@Drumor Drumor force-pushed the tags_additional_fields branch from c763035 to 41bde36 Compare July 15, 2022 07:47
@Drumor Drumor changed the title Tags additional fields [Frontend/Course Admin] Additional fields feature Jul 15, 2022
anthonygego
anthonygego requested changes Jul 25, 2022
View reviewed changes
Comment thread inginious/frontend/pages/course_admin/settings.py Outdated
Comment thread inginious/frontend/pages/course_admin/settings.py Outdated
Comment thread inginious/frontend/pages/course_admin/settings.py Outdated
@Drumor Drumor force-pushed the tags_additional_fields branch 2 times, most recently from ddec9cf to aee2176 Compare August 4, 2022 12:47
anthonygego
anthonygego requested changes Aug 10, 2022
View reviewed changes
Comment thread inginious/frontend/pages/course_admin/settings.py Outdated
@Drumor Drumor force-pushed the tags_additional_fields branch from 6755ba0 to dd5c8c2 Compare August 10, 2022 13:54
anthonygego
anthonygego requested changes Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@anthonygego anthonygego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are never parsing and loading the fields into the course object, neither here or in #832. This will therefore never check YAML validity.

This is done for tags here: https://github.com/UCL-INGI/INGInious/blob/c9f38af3a72e6fa98dcd01004e3075a335483933/inginious/frontend/courses.py#L82

Your are therefore using get_course_descriptor_content in #832 to access the data which should only be used to populate the dictionary before dumping to YAML file. The OOP principles are already too poorly respected in the code to worsen it.

@Drumor Drumor force-pushed the tags_additional_fields branch from 9e4d381 to 861715f Compare September 12, 2022 11:34
anthonygego
anthonygego requested changes Sep 13, 2022
View reviewed changes
Comment thread inginious/frontend/courses.py
@Drumor Drumor force-pushed the tags_additional_fields branch from 40125ee to 77ae877 Compare September 14, 2022 06:54
replace field name by type in course file.
2fcc170 
Add check to verify that the value is actually valid and raise an exception if this is not the case.
@anthonygego anthonygego merged commit b8523f7 into master Oct 17, 2022
@anthonygego anthonygego deleted the tags_additional_fields branch October 17, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anthonygego anthonygego anthonygego approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Drumor @anthonygego