classroom
classroom copied to clipboard
education
Remove user from all classrooms tied to a GH organization that has revoked their access
Addresses to https://github.com/github/education/issues/988
Our previous code assumed a 1-to-1 mapping of GitHub organizations to classrooms. However, many of our teachers are using a single organization on GitHub as a source for multiple classrooms.
This update ensures that when access is revoked for a GitHub organization for a user, the user is removed from all of the associated classrooms as well.
Previously we would not remove classroom access if a user was the only user with admin access to the classroom. With this update, we will be removing access regardless. In a small number of cases, this may result in admin-less classrooms.
Should we run a migration/transition to clean up existing user/classroom associations that should have been removed already?
@spinecone probably...since it's triggered by a webhook when someone's removed from the GitHub organization, I don't think there's a simple way of doing it. We'll likely need to iterate through every user/github_organization combo and make sure the user still has access (though I'm definitely open to a cleaner way of doing it).
Should that be part of this PR so we know when/why it was added? Or should that come in a follow on PR?
I don't know of a cleaner way than iterating through every user (or organization, whichever has fewer records), I would just make sure to use as few github.com API requests as possible so we don't hit any rate limits. It seems cleaner to me to have everything in this one PR, but I defer to what makes the most sense to you.
@spinecone I prefer having things together, but I end up getting a lot of feedback that my PRs are too big, so, I'm probably overly cautious now. I'll get it added in and re-ping for review.
@spinecone this should be ready to review again. I've added a rake task that will go through and check for any records that may need to be cleaned up. It relies on the client for the user that is being checked, so, we shouldn't end up hitting any API limits. I went with a rake task instead of a migration as this may be something we need to run again in the future.
Also, no need to rush the check on this. I won't be deploying it this late on a holiday weekend and I won't be back until Tuesday, so there's plenty of time.