diff --git a/app/commands/github/team_member/destroy.rb b/app/commands/github/team_member/destroy.rb index abc23cea05..dafce8782d 100644 --- a/app/commands/github/team_member/destroy.rb +++ b/app/commands/github/team_member/destroy.rb @@ -4,11 +4,16 @@ class Github::TeamMember::Destroy initialize_with :user_id, :team_name def call - ::Github::TeamMember.where(user_id:, team_name:).destroy_all.tap do - User::UpdateMaintainer.(user) if user - end + return unless team_member + + team_member.destroy + User::UpdateMaintainer.(user) if user end + private + memoize + def team_member = ::Github::TeamMember.find_by(user_id:, team_name:) + memoize def user = User.find_by(uid: user_id) end diff --git a/app/commands/github/team_member/sync_members.rb b/app/commands/github/team_member/sync_members.rb index 2b51e2a483..d09cc4ffd1 100644 --- a/app/commands/github/team_member/sync_members.rb +++ b/app/commands/github/team_member/sync_members.rb @@ -17,7 +17,7 @@ def add_team_members! def delete_team_members! Github::TeamMember.find_each do |team_member| - next if org_team_members[team_member.team_name].include?(team_member.user_id) + next if org_team_members[team_member.team_name].to_a.include?(team_member.user_id) Github::TeamMember::Destroy.(team_member.user_id, team_member.team_name) end diff --git a/app/models/github/team_member.rb b/app/models/github/team_member.rb index 139e11fa67..01d0bc72f2 100644 --- a/app/models/github/team_member.rb +++ b/app/models/github/team_member.rb @@ -1,8 +1,5 @@ class Github::TeamMember < ApplicationRecord - has_one :user, - foreign_key: :uid, - primary_key: :user_id, - class_name: "User", - inverse_of: :github_team_memberships, - dependent: :destroy + belongs_to :user, + primary_key: :uid, + inverse_of: :github_team_memberships end diff --git a/test/commands/github/team_member/create_test.rb b/test/commands/github/team_member/create_test.rb index 0d768d59aa..097de7a27c 100644 --- a/test/commands/github/team_member/create_test.rb +++ b/test/commands/github/team_member/create_test.rb @@ -4,6 +4,7 @@ class Github::TeamMember::CreateTest < ActiveSupport::TestCase test "creates team member" do user_id = '137131' team_name = 'fsharp' + create(:user, uid: user_id) team_name_member = Github::TeamMember::Create.(user_id, team_name) @@ -30,6 +31,8 @@ class Github::TeamMember::CreateTest < ActiveSupport::TestCase user_id = '137131' team_name = 'fsharp' + create(:user, uid: user_id) + assert_idempotent_command do Github::TeamMember::Create.(user_id, team_name) end diff --git a/test/commands/github/team_member/destroy_test.rb b/test/commands/github/team_member/destroy_test.rb index 7276fd7424..801eccb638 100644 --- a/test/commands/github/team_member/destroy_test.rb +++ b/test/commands/github/team_member/destroy_test.rb @@ -5,6 +5,7 @@ class Github::TeamMember::DestroyTest < ActiveSupport::TestCase user_id = '137131' team_name = 'fsharp' + create(:user, uid: user_id) create(:github_team_member, team_name:, user_id:) # Sanity check @@ -15,13 +16,29 @@ class Github::TeamMember::DestroyTest < ActiveSupport::TestCase refute Github::TeamMember.where(user_id:, team_name:).exists? end - test "update maintainer role" do + test "does not remove user" do user_id = '137131' team_name = 'fsharp' + user = create(:user, uid: user_id) create(:github_team_member, team_name:, user_id:) + # Sanity check + assert Github::TeamMember.where(user_id:, team_name:).exists? + + Github::TeamMember::Destroy.(user_id, team_name) + + refute Github::TeamMember.where(user_id:, team_name:).exists? + assert User.where(id: user.id).exists? + end + + test "update maintainer role" do + user_id = '137131' + team_name = 'fsharp' + user = create(:user, uid: user_id) + create(:github_team_member, team_name:, user_id:) + User::UpdateMaintainer.expects(:call).with(user).once Github::TeamMember::Destroy.(user_id, team_name) @@ -30,6 +47,7 @@ class Github::TeamMember::DestroyTest < ActiveSupport::TestCase test "idempotent" do user_id = '137131' team_name = 'fsharp' + create(:user, uid: user_id) assert_idempotent_command do Github::TeamMember::Destroy.(user_id, team_name) diff --git a/test/commands/github/team_member/sync_members_test.rb b/test/commands/github/team_member/sync_members_test.rb index 56243942a0..f1bf759903 100644 --- a/test/commands/github/team_member/sync_members_test.rb +++ b/test/commands/github/team_member/sync_members_test.rb @@ -5,6 +5,11 @@ class Github::TeamMember::SyncMembersTest < ActiveSupport::TestCase team_members = { 'ruby' => [12_412, 82_462], 'fsharp' => [12_412, 56_653] } Github::Organization.any_instance.stubs(:team_members).returns(team_members) + create(:user, uid: '12412') + create(:user, uid: '82462') + create(:user, uid: '56653') + Github::TeamMember::Destroy.expects(:call).never + Github::TeamMember::SyncMembers.() assert ::Github::TeamMember.where(team_name: 'ruby', user_id: '12412').exists? @@ -17,6 +22,7 @@ class Github::TeamMember::SyncMembersTest < ActiveSupport::TestCase team_members = { 'ruby' => [12_412] } Github::Organization.any_instance.stubs(:team_members).returns(team_members) + create(:user, uid: '12412') create :github_team_member, team_name: 'ruby', user_id: '12412' Github::TeamMember::SyncMembers.() @@ -29,6 +35,9 @@ class Github::TeamMember::SyncMembersTest < ActiveSupport::TestCase team_members = { 'ruby' => [82_462], 'fsharp' => [12_412] } Github::Organization.any_instance.stubs(:team_members).returns(team_members) + create(:user, uid: '56653') + create(:user, uid: '82462') + create(:user, uid: '12412') create :github_team_member, team_name: 'ruby', user_id: '56653' Github::TeamMember::SyncMembers.() @@ -38,4 +47,21 @@ class Github::TeamMember::SyncMembersTest < ActiveSupport::TestCase assert ::Github::TeamMember.where(team_name: 'ruby', user_id: '82462').exists? assert ::Github::TeamMember.where(team_name: 'fsharp', user_id: '12412').exists? end + + test "delete when group does not exist" do + team_members = { 'ruby' => [82_462], 'fsharp' => [12_412] } + Github::Organization.any_instance.stubs(:team_members).returns(team_members) + + create(:user, uid: '12412') + create(:user, uid: '82462') + create(:user, uid: '56653') + create :github_team_member, team_name: 'prolog', user_id: '56653' + + Github::TeamMember::SyncMembers.() + + assert_equal 2, ::Github::TeamMember.count + refute ::Github::TeamMember.where(team_name: 'prolog', user_id: '56653').exists? + assert ::Github::TeamMember.where(team_name: 'ruby', user_id: '82462').exists? + assert ::Github::TeamMember.where(team_name: 'fsharp', user_id: '12412').exists? + end end diff --git a/test/commands/webhooks/process_membership_update_test.rb b/test/commands/webhooks/process_membership_update_test.rb index 1e2495d048..ab3b14da37 100644 --- a/test/commands/webhooks/process_membership_update_test.rb +++ b/test/commands/webhooks/process_membership_update_test.rb @@ -6,6 +6,8 @@ class Webhooks::ProcessMembershipUpdateTest < ActiveSupport::TestCase team_name = 'team11' org = 'exercism' + create(:user, uid: user_id) + Github::Organization.any_instance.stubs(:name).returns(org) Webhooks::ProcessMembershipUpdate.('added', user_id, team_name, org) @@ -17,6 +19,7 @@ class Webhooks::ProcessMembershipUpdateTest < ActiveSupport::TestCase user_id = 12_348_521 team_name = 'team11' org = 'exercism' + create(:user, uid: user_id) create(:github_team_member, user_id:, team_name:) Github::Organization.any_instance.stubs(:name).returns(org) diff --git a/test/factories/github/team_members.rb b/test/factories/github/team_members.rb index 92440b60b5..5f24915eb4 100644 --- a/test/factories/github/team_members.rb +++ b/test/factories/github/team_members.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :github_team_member, class: 'Github::TeamMember' do - user_id { SecureRandom.hex } + user { create(:user, uid: SecureRandom.hex) } team_name { SecureRandom.hex } end end diff --git a/test/models/github/team_member_test.rb b/test/models/github/team_member_test.rb index bee28d2ff8..a8d4949b7e 100644 --- a/test/models/github/team_member_test.rb +++ b/test/models/github/team_member_test.rb @@ -2,10 +2,12 @@ class Github::TeamMemberTest < ActiveSupport::TestCase test "user" do - team_member = create :github_team_member - assert_nil team_member.user + uid = "123" + team_name = "ruby" + user = create(:user, uid:) - user = create :user, uid: team_member.user_id - assert_equal user, team_member.reload.user + team_member = create(:github_team_member, user_id: uid, team_name:) + assert_equal user, team_member.user + assert_includes user.github_team_memberships, team_member end end