-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: add/remove course staff role events #267
feat: add/remove course staff role events #267
Conversation
f5d4ff2
to
493452f
Compare
my recent comment got collapsed as resolved but can you update the descriptions to indicate this is fired as a result of assigning course staff not a signal to assign course staff. |
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.
looks good 👍
""" | ||
|
||
course_key = attr.ib(type=str) | ||
user = attr.ib(type=UserData) |
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.
which other fields do you think would be useful?
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 added some other fields based on the current parameters that CourseAccessRole currently takes: https://github.com/openedx/edx-platform/blob/master/common/djangoapps/student/roles.py#L448
Please note that this is subject to change in the future due to the concurrent roles/permissions project that is being done (see the "IMPORTANT:" comment in the event data for more details).
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.
That's better. Thank you!
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.
Just a few minor comments.
Quickly reiterating what I've written in the event data docstring about this being tech debt b/c of planned changes to roles and permissions: edX currently uses roles, and only roles, to decide what kind of access a user has. There is an ongoing project to replace this roles-only system with a system that uses roles that are made up of permissions, which is being worked on in parallel with another project to emit events whenever users are assigned any type of "Course Staff" role. It's unclear what the state of this roles/permissions project will be the time the events project is completed, so each project's respective teams will stay in touch with each other. For now, we're making a best effort to publish this an event that will regard the permission(s) we'd expect to "filter" for in the future (For more info, please check out this document: https://docs.google.com/spreadsheets/d/1htsV0eWq5-y96DZ5A245ukfZ4_qeH0KjHVaOyfqD8OA/edit#gid=908503896) and not for the roles we have now. Likely this/these permission(s) will be something like As such, the current plan is to do one of the following once the roles/permissions project's feature branch is merged to master: Until either of these plans are executed, the comment under the IMPORTANT header should stay put. |
5dde73e
to
f34982c
Compare
Just fyi that I'm going to wait for #268 to be merged before changing the version and merging this PR. |
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.
These are really useful events. Thank you!
I'll wait for the other PR to be merged. |
@ilee2u: the other PR was already merged! |
- Added a long comment describing how the new event data is tech debt. - Changed data name to "ManageStudentsPermissionData" to reflect these changes.
f34982c
to
557f8bc
Compare
@@ -1,4 +1,4 @@ | |||
11. Enable producing to event bus via settings | |||
12. Enable producing to event bus via settings |
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.
Thank you for this! I totally missed it.
Changes release. Thank you! |
Description:
We are developing a new backend for exams called
edx-exams
. This backend will be utilized by instructor dashboards to interact with student exam attempts.Circa Sept 2023,
edx-exams
has no way of knowing if a user is course staff (i.e. an instructor) or not. We'd like to avoid having this IDA calledx-platform
to get this info (which would create a circular dependency), so we've decided to emit this information fromedx-platform
toedx-exams
via the event bus (logic for the respective event producers and consumers to be deliberated later).This PR adds event definitions for adding and removing the course staff. These events are intended to be consumed by edx-exams, but they can also be consumed in other Open edX IDAs in the future as needed.
ISSUE: MST-1918 (Internal)
In order to receive course staff info in edx-exams, we must first define events to be sent. The events should be added to https://github.com/openedx/openedx-events/blob/main/openedx_events/learning/signals.py - there should be one event for adding a course staff role, and one event for removing (or revoking) a course staff role. The data for both events should consist of a user id and a course id.
Dependencies: None
Merge deadline: None (for now)
Testing instructions:
Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.