-
Notifications
You must be signed in to change notification settings - Fork 22
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: removed unused permission events #314
feat: removed unused permission events #314
Conversation
26b1649
to
c100e4e
Compare
@@ -5,4 +5,4 @@ | |||
more information about the project. | |||
""" | |||
|
|||
__version__ = "9.5.1" | |||
__version__ = "9.5.2" |
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.
Given that we are removing events that were public, and might be used by any other developer that imported this code, this should be a breaking change with a major version bump.
Naturally the events are not in use by edx-plaform right now. Where they ever used by that repo?
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.
Nope, I think this would be safe to merge w/o considering a major bump since the 'permission' models this event would be based on don't even exist. I can confidently say this has never been used. That said, I don't have strong options on if this bumps a major version or not. Just figured I'd point that out if we'd like to avoid going to v10.
In hindsight, we should not have included these until we were more certain the new permission model would get merged into edx-platform.
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.
If this was never really releaseda anywhere and has never been used I'm good with not bumping for major.
I'll look into the docs failure so we can merge this soon. |
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.
A tiny change so tests pass. Thanks!
Co-authored-by: Maria Grimaldi <[email protected]>
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 good, especially if the events were never used. Thank you for the cleanup!
Description: These events were added in anticipation of the roles and permissions changes. That work is now on hold and we have instead opted to publish an event based on roles as they exist in edx-platform today. See #309. As written this event might not match the future state of that work if it is picked up again. I propose these definitions be removed to avoid confusion.
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.