From 4fb2bb23c7cb1ce5d1a3b09e5010c84d95bf3220 Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Tue, 26 Nov 2019 12:42:35 -1000 Subject: [PATCH 1/7] Fix existing test and create new failing test for the multi-classroom org member --- spec/jobs/organization_event_job_spec.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/spec/jobs/organization_event_job_spec.rb b/spec/jobs/organization_event_job_spec.rb index cefbefd0ac..da95469e11 100644 --- a/spec/jobs/organization_event_job_spec.rb +++ b/spec/jobs/organization_event_job_spec.rb @@ -42,9 +42,23 @@ OrganizationEventJob.perform_now(payload) - expect { @organization.users.find(uid: @user.uid) }.to raise_error(ActiveRecord::RecordNotFound) + expect(@organization.users).not_to include(@user) expect(assignment.reload.creator_id).not_to eq(@user.id) end + + it "deletes user from multiple organizations (classrooms) mapped to the same GitHub organization" do + org2 = create(:organization, github_id: payload.dig("organization", "id")) + @user.organizations << org2 + @user.save + + expect(@organization.users).to include(@user) + expect(org2.users).to include(@user) + + OrganizationEventJob.perform_now(payload) + + expect(@organization.users).not_to include(@user) + expect(org2.users).not_to include(@user) + end end end end From 32f8152d5f4aba0251774b40f724a59f2b220d93 Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Tue, 26 Nov 2019 14:09:26 -1000 Subject: [PATCH 2/7] Ensure user is removed from all classrooms tied to the given github_organization_id --- app/jobs/organization_event_job.rb | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/app/jobs/organization_event_job.rb b/app/jobs/organization_event_job.rb index 44d740d36f..b3b75656bb 100644 --- a/app/jobs/organization_event_job.rb +++ b/app/jobs/organization_event_job.rb @@ -10,16 +10,25 @@ def perform(payload_body) github_user_id = payload_body.dig("membership", "user", "id") github_organization_id = payload_body.dig("organization", "id") - @organization = Organization.find_by(github_id: github_organization_id) + organizations = Organization.where(github_id: github_organization_id) - return false if @organization.blank? - return false if @organization.users.count == 1 + return false if organizations.empty? - @user = @organization.users.find_by(uid: github_user_id) + failed_removals = organizations.reject do |org| + # return false if org.users.count == 1 + user = org.users.find_by(uid: github_user_id) - return true unless @user - TransferAssignmentsService.new(@organization, @user).transfer - @organization.users.delete(@user) + if user + TransferAssignmentsService.new(org, user).transfer + org.users.delete(user) + else + false + end + end + + # This method should only report success (by returning true) if we were able to remove + # the user from all of the organizations tied to the given github_organization_id + failed_removals.empty? end # rubocop:enable Metrics/AbcSize end From 01cd5f1c4deeb752818d8233afac29e368fe5b45 Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Tue, 26 Nov 2019 14:29:11 -1000 Subject: [PATCH 3/7] Disable the rubocop linter for methodlength on the removal job --- app/jobs/organization_event_job.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/jobs/organization_event_job.rb b/app/jobs/organization_event_job.rb index b3b75656bb..2de7bc65e2 100644 --- a/app/jobs/organization_event_job.rb +++ b/app/jobs/organization_event_job.rb @@ -5,6 +5,7 @@ class OrganizationEventJob < ApplicationJob queue_as :github_event # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/MethodLength def perform(payload_body) return true unless payload_body.dig("action") == "member_removed" @@ -31,4 +32,5 @@ def perform(payload_body) failed_removals.empty? end # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/MethodLength end From ae71fcbe79aa950c410a62e3780c8b73e2b5602f Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Wed, 27 Nov 2019 11:15:59 -1000 Subject: [PATCH 4/7] In progress method starting wtih orgs We attepted to start with orgs as there were fewer orgs than users and this seemed like a way to limit the amount of api calls. However, in order to check if someone is an admin on an org, we have to make an api call to that org authenticated as someone who is already an admin. That would require going through users in that org to see if one is an admin before running through the remaining users to see if they should be removed or not. It seems simpler now to start with a user and iterate through their orgs (removing them from any they're not an admin for). --- ...sroom_users_without_github_org_access.rake | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 lib/tasks/remove_classroom_users_without_github_org_access.rake diff --git a/lib/tasks/remove_classroom_users_without_github_org_access.rake b/lib/tasks/remove_classroom_users_without_github_org_access.rake new file mode 100644 index 0000000000..e6db51f990 --- /dev/null +++ b/lib/tasks/remove_classroom_users_without_github_org_access.rake @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# Checks if classroom admins have access to the associated GitHub organization. If not +# the admin is removed from the classroom +task :remove_classroom_users_without_github_org_access, [:current_user] do |t, args| + current_user = User.find_by(github_login: args[:current_user]) + github_org_ids = Organization.pluck(:github_id).uniq + + github_org_ids.each_slice(100) do |batch| + batch.each do |gh_id| + # test with 15126061 a multi classroom org + classrooms = Organization.where(github_id: gh_id) + classroom_admins = User.includes(:organizations).where(organizations: { id: classrooms.pluck(:id) }) + + next unless classrooms.any? && classroom_admins.any? + + github_org = GitHubOrganization.new(current_user.github_client, gh_id) + github_org = GitHubOrganization.new(classroom_admins.first.github_client, gh_id) + + classroom_admins.each do |user| + next if github_org.admin?(user.github_user.login) + end + end + end +end \ No newline at end of file From 2a3479007a844a8a8f8099666da97fb094c465fe Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Wed, 27 Nov 2019 13:04:53 -1000 Subject: [PATCH 5/7] Rake task that checks that users are still org members and removes them if they aren't. --- app/jobs/organization_event_job.rb | 1 - ...sroom_users_without_github_org_access.rake | 33 ++++++++++--------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/jobs/organization_event_job.rb b/app/jobs/organization_event_job.rb index 2de7bc65e2..dfa6fc1140 100644 --- a/app/jobs/organization_event_job.rb +++ b/app/jobs/organization_event_job.rb @@ -16,7 +16,6 @@ def perform(payload_body) return false if organizations.empty? failed_removals = organizations.reject do |org| - # return false if org.users.count == 1 user = org.users.find_by(uid: github_user_id) if user diff --git a/lib/tasks/remove_classroom_users_without_github_org_access.rake b/lib/tasks/remove_classroom_users_without_github_org_access.rake index e6db51f990..7788983ab2 100644 --- a/lib/tasks/remove_classroom_users_without_github_org_access.rake +++ b/lib/tasks/remove_classroom_users_without_github_org_access.rake @@ -2,24 +2,25 @@ # Checks if classroom admins have access to the associated GitHub organization. If not # the admin is removed from the classroom -task :remove_classroom_users_without_github_org_access, [:current_user] do |t, args| - current_user = User.find_by(github_login: args[:current_user]) - github_org_ids = Organization.pluck(:github_id).uniq +task :remove_classroom_users_without_github_org_access do + User.find_in_batches(batch_size: 250) do |group, batch| + puts "Processing group ##{batch}" + group.each do |user| + github_org_ids = user.organizations.pluck(:github_id).uniq + next unless github_org_ids.any? - github_org_ids.each_slice(100) do |batch| - batch.each do |gh_id| - # test with 15126061 a multi classroom org - classrooms = Organization.where(github_id: gh_id) - classroom_admins = User.includes(:organizations).where(organizations: { id: classrooms.pluck(:id) }) + github_org_ids.each do |gh_id| + github_org = GitHubOrganization.new(user.github_client, gh_id) - next unless classrooms.any? && classroom_admins.any? + if !github_org.admin?(user.github_user.login) + payload = { + "action": "member_removed", + "membership": { "user": { "id": user.github_user.id } }, + "organization": { "id": gh_id } } - github_org = GitHubOrganization.new(current_user.github_client, gh_id) - github_org = GitHubOrganization.new(classroom_admins.first.github_client, gh_id) - - classroom_admins.each do |user| - next if github_org.admin?(user.github_user.login) - end + OrganizationEventJob.perform_later(payload) end + end end -end \ No newline at end of file + end +end From 3d00951820f000232b011178898cb9990eba0daa Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Wed, 27 Nov 2019 13:12:55 -1000 Subject: [PATCH 6/7] Changes to make the linters happy. --- ...e_classroom_users_without_github_org_access.rake | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/tasks/remove_classroom_users_without_github_org_access.rake b/lib/tasks/remove_classroom_users_without_github_org_access.rake index 7788983ab2..d9457174dc 100644 --- a/lib/tasks/remove_classroom_users_without_github_org_access.rake +++ b/lib/tasks/remove_classroom_users_without_github_org_access.rake @@ -11,14 +11,15 @@ task :remove_classroom_users_without_github_org_access do github_org_ids.each do |gh_id| github_org = GitHubOrganization.new(user.github_client, gh_id) + next unless github_org.admin?(user.github_user.login) - if !github_org.admin?(user.github_user.login) - payload = { - "action": "member_removed", - "membership": { "user": { "id": user.github_user.id } }, - "organization": { "id": gh_id } } + payload = { + "action": "member_removed", + "membership": { "user": { "id": user.github_user.id } }, + "organization": { "id": gh_id } + } - OrganizationEventJob.perform_later(payload) + OrganizationEventJob.perform_later(payload) end end end From 36bae66db04f25568758f6148c48cd6d248fbe72 Mon Sep 17 00:00:00 2001 From: Jess Rudder Date: Wed, 27 Nov 2019 13:43:51 -1000 Subject: [PATCH 7/7] Simplify and add some output --- .../remove_classroom_users_without_github_org_access.rake | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/tasks/remove_classroom_users_without_github_org_access.rake b/lib/tasks/remove_classroom_users_without_github_org_access.rake index d9457174dc..6e84d497a8 100644 --- a/lib/tasks/remove_classroom_users_without_github_org_access.rake +++ b/lib/tasks/remove_classroom_users_without_github_org_access.rake @@ -3,15 +3,15 @@ # Checks if classroom admins have access to the associated GitHub organization. If not # the admin is removed from the classroom task :remove_classroom_users_without_github_org_access do - User.find_in_batches(batch_size: 250) do |group, batch| - puts "Processing group ##{batch}" + User.find_in_batches(batch_size: 250).with_index do |group, batch| + puts "Processing batch ##{batch}" group.each do |user| github_org_ids = user.organizations.pluck(:github_id).uniq next unless github_org_ids.any? github_org_ids.each do |gh_id| github_org = GitHubOrganization.new(user.github_client, gh_id) - next unless github_org.admin?(user.github_user.login) + next if github_org.admin?(user.github_login) payload = { "action": "member_removed", @@ -19,8 +19,8 @@ task :remove_classroom_users_without_github_org_access do "organization": { "id": gh_id } } + puts "Removing #{user.github_login} from org with id ##{gh_id}" OrganizationEventJob.perform_later(payload) - end end end end