Skip to content
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

Merged
merged 10 commits into from
Oct 2, 2023

Conversation

ilee2u
Copy link
Contributor

@ilee2u ilee2u commented Sep 19, 2023

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 call edx-platform to get this info (which would create a circular dependency), so we've decided to emit this information from edx-platform to edx-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:

  1. Test as usual

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@ilee2u ilee2u requested a review from a team as a code owner September 19, 2023 17:53
@ilee2u ilee2u marked this pull request as draft September 19, 2023 20:27
@Zacharis278
Copy link
Contributor

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.

Copy link
Contributor

@Zacharis278 Zacharis278 left a comment

Choose a reason for hiding this comment

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

looks good 👍

@ilee2u ilee2u marked this pull request as ready for review September 20, 2023 13:56
@ilee2u ilee2u mentioned this pull request Sep 21, 2023
11 tasks
"""

course_key = attr.ib(type=str)
user = attr.ib(type=UserData)
Copy link
Member

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?

Copy link
Contributor Author

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).

Copy link
Member

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!

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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.

@ilee2u
Copy link
Contributor Author

ilee2u commented Sep 29, 2023

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 manage_students, but we need to evaluate how this will align with some possible future roles such as limited_staff or ccx.

As such, the current plan is to do one of the following once the roles/permissions project's feature branch is merged to master:
1. Modify this event to "filter" by the correct permissions once the
2. As a backup plan, make a new event if this proves too difficult.

Until either of these plans are executed, the comment under the IMPORTANT header should stay put.

@ilee2u
Copy link
Contributor Author

ilee2u commented Sep 29, 2023

Just fyi that I'm going to wait for #268 to be merged before changing the version and merging this PR.

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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!

@mariajgrimaldi
Copy link
Member

I'll wait for the other PR to be merged.

@mariajgrimaldi
Copy link
Member

@ilee2u: the other PR was already merged!

@@ -1,4 +1,4 @@
11. Enable producing to event bus via settings
12. Enable producing to event bus via settings
Copy link
Member

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.

@mariajgrimaldi mariajgrimaldi merged commit 7e6e924 into openedx:main Oct 2, 2023
8 checks passed
@mariajgrimaldi
Copy link
Member

Changes release. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants