-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: send course role events to the event bus #34158
Conversation
74bf041
to
29eb740
Compare
Emit an event to the event-bus when a CourseAccessRole is created | ||
""" | ||
user = instance.user | ||
# do we want to guard against updating an existing role instance in place |
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.
torn here, leaving this as is make everything a lot more simple but leaves a gap if down the line we were to write code that took an existing CourseAccessRole instance and changed the role on it rather than creating a new instance for a different role.
The only option is do some pre_save code to keep track of the former value or do this in the save() method on the model without signals. I'm leaning towards the latter.
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.
Do we anticipate changes being made to the CourseAccessRole soon? If not, I feel like the gap is fine. If this area were in active development, then we would probably be better off closing that gap like you suggested.
I am curious is any future work with roles and permissions could affect this 🤔
If the gap is too much of a risk (which it very well could be, given the discovery work already happening with roles and permissions), I agree that doing this in the save() method would be a good solution.
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.
soon no, and long term this will be removed and replaced with an event for a granted permission instead. I tried using the save() method but it still adds some complexity keeping track of the former value so I'm just going to keep this short version for now.
29eb740
to
d695466
Compare
2affb8a
to
f719ecc
Compare
cdf841f
to
b2fbbc2
Compare
b2fbbc2
to
b11a21f
Compare
5f1126a
to
3b4d6c7
Compare
3b4d6c7
to
7891e2c
Compare
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.
🚢
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.
👍 Let's do this!
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Notify the event bus when a user's role in a course is added or removed. These will eventually be consumed by edx-exams to sync access instructor endpoints.
event definitions: openedx/openedx-events#309
2u-internal ticket: COSMO-156