-
Notifications
You must be signed in to change notification settings - Fork 7
Verawood release branch #859
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
base: opencraft/verawood
Are you sure you want to change the base?
Changes from all commits
610da02
9fab2fd
036cd1a
23b2c92
31d4d84
d6fb45e
17b42d8
243e782
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3127,7 +3127,7 @@ def get(self, request, course_id): | |
| editable = request.query_params.get('editable', 'false').lower() == 'true' | ||
|
|
||
| roles = set(ROLES.keys()) | set(FORUM_ROLES) | ||
|
|
||
| roles -= {'eshe_instructor', 'teaching_assistant'} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xitij2000 I'm unsure about what kind of effects this could have - if this removes them from the roles returned here, does this make it impossible to select these roles? cc @viadanna Also, if we did need to include these updates, I'd be in favour of squashing them into "feat: all eSHE role features squashed in one commit", and updating that commit message to be a little friendlier.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a completely new API, and there have been huge changes to roles and permissions. Adding these were causing test failures and I didn't want to break something else. @viadanna Do you know what would be the best approach here? how are these used? are they needed anymore? I can remove this line and look into fixing the tests, but unlike the display names these could have more sideeffects that I don't fully understand. |
||
| ccx_enabled = settings.FEATURES.get('CUSTOM_COURSES_EDX', False) and course.enable_ccx | ||
| if not ccx_enabled: | ||
| roles.discard('ccx_coach') | ||
|
|
||
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 looks fine to me; only updating display names.
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.
Yes, this is new for roles I think, since there is now a UI to manage them