diff --git a/app/commands/github/team_member/create.rb b/app/commands/github/team_member/create.rb index 84f3332af7..8aa7c26f6f 100644 --- a/app/commands/github/team_member/create.rb +++ b/app/commands/github/team_member/create.rb @@ -1,14 +1,16 @@ class Github::TeamMember::Create include Mandate - initialize_with :user_id, :team_name + initialize_with :github_uid, :team_name def call - ::Github::TeamMember.find_create_or_find_by!(user_id:, team_name:).tap do - User::UpdateMaintainer.(user) if user + return unless user + + user.github_team_memberships.find_or_create_by!(team_name:).tap do |team_member| + User::UpdateMaintainer.(user) if team_member.previously_new_record? end end memoize - def user = User.find_by(uid: user_id) + def user = User.find_by(uid: github_uid) end diff --git a/app/commands/github/team_member/destroy.rb b/app/commands/github/team_member/destroy.rb index abc23cea05..2b9df6386b 100644 --- a/app/commands/github/team_member/destroy.rb +++ b/app/commands/github/team_member/destroy.rb @@ -1,14 +1,19 @@ class Github::TeamMember::Destroy include Mandate - initialize_with :user_id, :team_name + initialize_with :github_uid, :team_name def call - ::Github::TeamMember.where(user_id:, team_name:).destroy_all.tap do - User::UpdateMaintainer.(user) if user - end + return unless user + return unless team_member + + team_member.delete + User::UpdateMaintainer.(user) end memoize - def user = User.find_by(uid: user_id) + def user = User.find_by(uid: github_uid) + + memoize + def team_member = Github::TeamMember.find_by(user:, team_name:) end diff --git a/app/commands/github/team_member/sync_members.rb b/app/commands/github/team_member/sync_members.rb index 2b51e2a483..04c7532c3d 100644 --- a/app/commands/github/team_member/sync_members.rb +++ b/app/commands/github/team_member/sync_members.rb @@ -8,18 +8,18 @@ def call private def add_team_members! - org_team_members.each do |team_name, user_ids| - user_ids.each do |user_id| - ::Github::TeamMember::Create.(user_id, team_name) + org_team_members.each do |team_name, github_uids| + github_uids.each do |github_uid| + ::Github::TeamMember::Create.(github_uid, team_name) end end end 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) + Github::TeamMember.includes(:user).find_each do |team_member| + next if org_team_members[team_member.team_name].to_a.include?(team_member.user.uid) - Github::TeamMember::Destroy.(team_member.user_id, team_member.team_name) + Github::TeamMember::Destroy.(team_member.user.uid, team_member.team_name) end end diff --git a/app/controllers/maintaining/site_updates_controller.rb b/app/controllers/maintaining/site_updates_controller.rb index 129a8cb025..a897ce869f 100644 --- a/app/controllers/maintaining/site_updates_controller.rb +++ b/app/controllers/maintaining/site_updates_controller.rb @@ -44,7 +44,7 @@ def update private def setup_new_form @tracks = current_user.admin? ? Track.all : - Track.where(slug: Github::TeamMember.where(user_id: current_user.uid).pluck(:team_name)).order(:title) + Track.where(slug: Github::TeamMember.where(user: current_user).pluck(:team_name)).order(:title) end # Whitelist allowed parameters diff --git a/app/models/github/team_member.rb b/app/models/github/team_member.rb index 139e11fa67..828d237610 100644 --- a/app/models/github/team_member.rb +++ b/app/models/github/team_member.rb @@ -1,8 +1,3 @@ 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 end diff --git a/app/models/user.rb b/app/models/user.rb index 00559e3d8b..a8af3660a0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -123,11 +123,7 @@ class User < ApplicationRecord has_many :cohort_memberships, dependent: :destroy - has_many :github_team_memberships, - class_name: "Github::TeamMember", - primary_key: :uid, - inverse_of: :user, - dependent: :destroy + has_many :github_team_memberships, class_name: "Github::TeamMember", dependent: :destroy has_many :challenges, dependent: :destroy has_many :watched_videos, class_name: "User::WatchedVideo", dependent: :destroy diff --git a/db/migrate/20240813125214_change_user_id_to_foreign_key_in_github_team_members.rb b/db/migrate/20240813125214_change_user_id_to_foreign_key_in_github_team_members.rb new file mode 100644 index 0000000000..d10a4b783b --- /dev/null +++ b/db/migrate/20240813125214_change_user_id_to_foreign_key_in_github_team_members.rb @@ -0,0 +1,14 @@ +class ChangeUserIdToForeignKeyInGithubTeamMembers < ActiveRecord::Migration[7.0] + def change + return if Rails.env.production? + + # We'll re-sync the data using Github::TeamMember::SyncMembers.() + Github::TeamMember.delete_all + + remove_index :github_team_members, name: "index_github_team_members_on_user_id_and_team_name" + remove_column :github_team_members, :user_id, :string + + add_reference :github_team_members, :user, null: false, foreign_key: { to_table: :users }, if_not_exists: true + add_index :github_team_members, %i[user_id team_name], unique: true, if_not_exists: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 0441d99c40..ed1168b0d6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_07_25_080340) do +ActiveRecord::Schema[7.0].define(version: 2024_08_13_125214) do create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -526,11 +526,12 @@ end create_table "github_team_members", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| - t.string "user_id", null: false t.string "team_name", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.bigint "user_id", null: false t.index ["user_id", "team_name"], name: "index_github_team_members_on_user_id_and_team_name", unique: true + t.index ["user_id"], name: "index_github_team_members_on_user_id" end create_table "iterations", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| @@ -1671,6 +1672,7 @@ add_foreign_key "github_issue_labels", "github_issues" add_foreign_key "github_pull_request_reviews", "github_pull_requests" add_foreign_key "github_tasks", "tracks" + add_foreign_key "github_team_members", "users" add_foreign_key "mentor_discussion_posts", "iterations" add_foreign_key "mentor_discussion_posts", "mentor_discussions", column: "discussion_id" add_foreign_key "mentor_discussion_posts", "users" diff --git a/test/commands/github/team_member/create_test.rb b/test/commands/github/team_member/create_test.rb index 0d768d59aa..9093316f20 100644 --- a/test/commands/github/team_member/create_test.rb +++ b/test/commands/github/team_member/create_test.rb @@ -2,39 +2,52 @@ class Github::TeamMember::CreateTest < ActiveSupport::TestCase test "creates team member" do - user_id = '137131' + github_uid = '137131' team_name = 'fsharp' + user = create(:user, uid: github_uid) - team_name_member = Github::TeamMember::Create.(user_id, team_name) + team_name_member = Github::TeamMember::Create.(github_uid, team_name) assert_equal 1, Github::TeamMember.count - assert_equal user_id, team_name_member.user_id + assert_equal user, team_name_member.user assert_equal team_name, team_name_member.team_name end test "update maintainer role" do - user_id = '137131' + github_uid = '137131' team_name = 'fsharp' - user = create(:user, uid: user_id) + user = create(:user, uid: github_uid) User::UpdateMaintainer.expects(:call).with(user).once - team_name_member = Github::TeamMember::Create.(user_id, team_name) + team_name_member = Github::TeamMember::Create.(github_uid, team_name) assert_equal 1, Github::TeamMember.count - assert_equal user_id, team_name_member.user_id + assert_equal user, team_name_member.user assert_equal team_name, team_name_member.team_name end + test "noop when already created" do + github_uid = '137131' + team_name = 'fsharp' + user = create(:user, uid: github_uid) + create(:github_team_member, team_name:, user:) + + User::UpdateMaintainer.expects(:call).with(user).never + + Github::TeamMember::Create.(github_uid, team_name) + end + test "idempotent" do user_id = '137131' team_name = 'fsharp' + user = create(:user, uid: user_id) assert_idempotent_command do Github::TeamMember::Create.(user_id, team_name) end assert_equal 1, Github::TeamMember.count - assert Github::TeamMember.where(user_id:, team_name:).exists? + assert Github::TeamMember.where(user:, team_name:).exists? end end diff --git a/test/commands/github/team_member/destroy_test.rb b/test/commands/github/team_member/destroy_test.rb index 7276fd7424..4476c75fe6 100644 --- a/test/commands/github/team_member/destroy_test.rb +++ b/test/commands/github/team_member/destroy_test.rb @@ -2,39 +2,42 @@ class Github::TeamMember::DestroyTest < ActiveSupport::TestCase test "removes team member" do - user_id = '137131' + github_uid = '137131' team_name = 'fsharp' - create(:github_team_member, team_name:, user_id:) + user = create(:user, uid: github_uid) + create(:github_team_member, team_name:, user:) # Sanity check - assert Github::TeamMember.where(user_id:, team_name:).exists? + assert Github::TeamMember.where(user:, team_name:).exists? - Github::TeamMember::Destroy.(user_id, team_name) + Github::TeamMember::Destroy.(github_uid, team_name) - refute Github::TeamMember.where(user_id:, team_name:).exists? + refute Github::TeamMember.where(user:, team_name:).exists? end test "update maintainer role" do - user_id = '137131' + github_uid = '137131' team_name = 'fsharp' - create(:github_team_member, team_name:, user_id:) + user = create(:user, uid: github_uid) + create(:github_team_member, team_name:, user:) - user = create(:user, uid: user_id) User::UpdateMaintainer.expects(:call).with(user).once - Github::TeamMember::Destroy.(user_id, team_name) + Github::TeamMember::Destroy.(github_uid, team_name) end test "idempotent" do - user_id = '137131' + github_uid = '137131' team_name = 'fsharp' + user = create(:user, uid: github_uid) + assert_idempotent_command do - Github::TeamMember::Destroy.(user_id, team_name) + Github::TeamMember::Destroy.(github_uid, team_name) end - refute Github::TeamMember.where(user_id:, team_name:).exists? + refute Github::TeamMember.where(user:, team_name:).exists? end end diff --git a/test/commands/github/team_member/sync_members_test.rb b/test/commands/github/team_member/sync_members_test.rb index 56243942a0..8524a99fbe 100644 --- a/test/commands/github/team_member/sync_members_test.rb +++ b/test/commands/github/team_member/sync_members_test.rb @@ -3,39 +3,63 @@ class Github::TeamMember::SyncMembersTest < ActiveSupport::TestCase test "adds new members" do team_members = { 'ruby' => [12_412, 82_462], 'fsharp' => [12_412, 56_653] } + user_1 = create(:user, uid: '12412') + user_2 = create(:user, uid: '82462') + user_3 = create(:user, uid: '56653') Github::Organization.any_instance.stubs(:team_members).returns(team_members) Github::TeamMember::SyncMembers.() - assert ::Github::TeamMember.where(team_name: 'ruby', user_id: '12412').exists? - assert ::Github::TeamMember.where(team_name: 'ruby', user_id: '82462').exists? - assert ::Github::TeamMember.where(team_name: 'fsharp', user_id: '12412').exists? - assert ::Github::TeamMember.where(team_name: 'fsharp', user_id: '56653').exists? + assert ::Github::TeamMember.where(team_name: 'ruby', user: user_1).exists? + assert ::Github::TeamMember.where(team_name: 'ruby', user: user_2).exists? + assert ::Github::TeamMember.where(team_name: 'fsharp', user: user_1).exists? + assert ::Github::TeamMember.where(team_name: 'fsharp', user: user_3).exists? end test "keeps existing members" do team_members = { 'ruby' => [12_412] } + user = create(:user, uid: '12412') Github::Organization.any_instance.stubs(:team_members).returns(team_members) - create :github_team_member, team_name: 'ruby', user_id: '12412' + create(:github_team_member, team_name: 'ruby', user:) Github::TeamMember::SyncMembers.() assert_equal 1, ::Github::TeamMember.count - assert ::Github::TeamMember.where(team_name: 'ruby', user_id: '12412').exists? + assert ::Github::TeamMember.where(team_name: 'ruby', user:).exists? end test "removes former members" do team_members = { 'ruby' => [82_462], 'fsharp' => [12_412] } + user_1 = create(:user, uid: '12412') + user_2 = create(:user, uid: '82462') + user_3 = create(:user, uid: '56653') Github::Organization.any_instance.stubs(:team_members).returns(team_members) - create :github_team_member, team_name: 'ruby', user_id: '56653' + create :github_team_member, team_name: 'ruby', user: user_3 Github::TeamMember::SyncMembers.() assert_equal 2, ::Github::TeamMember.count - refute ::Github::TeamMember.where(team_name: 'ruby', 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? + refute ::Github::TeamMember.where(team_name: 'ruby', user: user_3).exists? + assert ::Github::TeamMember.where(team_name: 'ruby', user: user_2).exists? + assert ::Github::TeamMember.where(team_name: 'fsharp', user: user_1).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) + + user_1 = create(:user, uid: '12412') + user_2 = create(:user, uid: '82462') + user_3 = create(:user, uid: '56653') + create :github_team_member, team_name: 'prolog', user: user_3 + + Github::TeamMember::SyncMembers.() + + assert_equal 2, ::Github::TeamMember.count + refute ::Github::TeamMember.where(team_name: 'prolog', user: user_3).exists? + assert ::Github::TeamMember.where(team_name: 'ruby', user_id: user_2).exists? + assert ::Github::TeamMember.where(team_name: 'fsharp', user_id: user_1).exists? end end diff --git a/test/commands/user/update_maintainer_test.rb b/test/commands/user/update_maintainer_test.rb index b173c6062f..98e1ac5e72 100644 --- a/test/commands/user/update_maintainer_test.rb +++ b/test/commands/user/update_maintainer_test.rb @@ -15,7 +15,7 @@ class User::UpdateMaintainerTest < ActiveSupport::TestCase test "becomes maintainer when member of track github team" do user = create(:user, uid: SecureRandom.uuid) track = create :track - create(:github_team_member, user_id: user.uid, team_name: track.slug) + create(:github_team_member, user:, team_name: track.slug) # Sanity check refute user.maintainer? @@ -39,7 +39,7 @@ class User::UpdateMaintainerTest < ActiveSupport::TestCase test "retains maintainership when still member of track github team" do user = create(:user, :maintainer, uid: SecureRandom.uuid) track = create :track - create(:github_team_member, user_id: user.uid, team_name: track.slug) + create(:github_team_member, user:, team_name: track.slug) # Sanity check assert user.maintainer? diff --git a/test/commands/user_track/generate_summary_data/exercises_available_test.rb b/test/commands/user_track/generate_summary_data/exercises_available_test.rb index 67303254b9..ae45b1ea02 100644 --- a/test/commands/user_track/generate_summary_data/exercises_available_test.rb +++ b/test/commands/user_track/generate_summary_data/exercises_available_test.rb @@ -286,7 +286,7 @@ class UserTrack::GenerateSummaryData::ExercisesUnlockedTest < ActiveSupport::Tes wip_practice_exercise = create :practice_exercise, :random_slug, track:, status: :wip user = create :user, roles: [:maintainer], uid: '1232134' - create :github_team_member, user_id: user.uid, team_name: track.github_team_name + create(:github_team_member, user:, team_name: track.github_team_name) user_track = create(:user_track, track:, user:) hw_solution = create(:hello_world_solution, :completed, track:, user:) hello_world = hw_solution.exercise diff --git a/test/commands/webhooks/process_membership_update_test.rb b/test/commands/webhooks/process_membership_update_test.rb index 1e2495d048..e00bab6b5b 100644 --- a/test/commands/webhooks/process_membership_update_test.rb +++ b/test/commands/webhooks/process_membership_update_test.rb @@ -2,28 +2,30 @@ class Webhooks::ProcessMembershipUpdateTest < ActiveSupport::TestCase test "add team member when action is 'added'" do - user_id = 12_348_521 + github_uid = 12_348_521 team_name = 'team11' org = 'exercism' + user = create(:user, uid: github_uid) Github::Organization.any_instance.stubs(:name).returns(org) - Webhooks::ProcessMembershipUpdate.('added', user_id, team_name, org) + Webhooks::ProcessMembershipUpdate.('added', github_uid, team_name, org) - assert Github::TeamMember.where(user_id:, team_name:).exists? + assert Github::TeamMember.where(user:, team_name:).exists? end test "removes team member when action is 'removed'" do - user_id = 12_348_521 + github_uid = 12_348_521 team_name = 'team11' org = 'exercism' - create(:github_team_member, user_id:, team_name:) + user = create(:user, uid: github_uid) + create(:github_team_member, user:, team_name:) Github::Organization.any_instance.stubs(:name).returns(org) - Webhooks::ProcessMembershipUpdate.('removed', user_id, team_name, org) + Webhooks::ProcessMembershipUpdate.('removed', github_uid, team_name, org) - refute Github::TeamMember.where(user_id:, team_name:).exists? + refute Github::TeamMember.where(user:, team_name:).exists? end test "does not do anything if organization does not match" do diff --git a/test/factories/github/team_members.rb b/test/factories/github/team_members.rb index 92440b60b5..0e13fe931f 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 } 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..d2ce38971d 100644 --- a/test/models/github/team_member_test.rb +++ b/test/models/github/team_member_test.rb @@ -2,10 +2,10 @@ class Github::TeamMemberTest < ActiveSupport::TestCase test "user" do - team_member = create :github_team_member - assert_nil team_member.user + user = create(:user, uid: SecureRandom.uuid) + team_member = create(:github_team_member, user:) - user = create :user, uid: team_member.user_id - assert_equal user, team_member.reload.user + assert_equal user, team_member.user + assert_includes user.github_team_memberships, team_member end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a4fb046be6..36044146aa 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -368,14 +368,14 @@ class UserTest < ActiveSupport::TestCase other_user = create :user, uid: '769032' assert_empty user.github_team_memberships - team_member_1 = create :github_team_member, user_id: user.uid + team_member_1 = create(:github_team_member, user:) assert_equal [team_member_1], user.reload.github_team_memberships - team_member_2 = create :github_team_member, user_id: user.uid + team_member_2 = create(:github_team_member, user:) assert_equal [team_member_1, team_member_2].sort, user.reload.github_team_memberships.sort # Sanity check: other user - create :github_team_member, user_id: other_user.uid + create :github_team_member, user: other_user assert_equal [team_member_1, team_member_2].sort, user.reload.github_team_memberships.sort end diff --git a/test/models/user_track_test.rb b/test/models/user_track_test.rb index f6c2166024..595c930170 100644 --- a/test/models/user_track_test.rb +++ b/test/models/user_track_test.rb @@ -639,7 +639,7 @@ class UserTrackTest < ActiveSupport::TestCase active_practice_exercise ].map(&:slug).sort, user_track.reload.exercises.map(&:slug).sort - create :github_team_member, user_id: user.uid, team_name: track.github_team_name + create :github_team_member, user:, team_name: track.github_team_name # wip and concept exercises are included when user is track maintainer assert_equal [ @@ -678,7 +678,7 @@ class UserTrackTest < ActiveSupport::TestCase # Sanity check: concept exercises are included for track without course but user is track maintainer user.update(roles: [:maintainer], uid: '21312312') - create :github_team_member, user_id: user.uid, team_name: track.github_team_name + create :github_team_member, user:, team_name: track.github_team_name assert_equal [ beta_concept_exercise, active_concept_exercise, @@ -955,7 +955,7 @@ class UserTrackTest < ActiveSupport::TestCase user.update(roles: [:maintainer]) refute user_track.reload.maintainer? - create :github_team_member, user_id: user.uid, team_name: track.github_team_name + create :github_team_member, user:, team_name: track.github_team_name assert user_track.reload.maintainer? end @@ -979,7 +979,7 @@ class UserTrackTest < ActiveSupport::TestCase refute maintainer_user_track.course? # Only track maintainer that are in the track's GitHub team are excempted - create :github_team_member, user_id: maintainer.uid, team_name: track.github_team_name + create :github_team_member, user: maintainer, team_name: track.github_team_name refute non_maintainer_user_track.reload.course? assert maintainer_user_track.reload.course? end diff --git a/test/system/flows/maintainer/maintainer_adds_arbitrary_site_update_test.rb b/test/system/flows/maintainer/maintainer_adds_arbitrary_site_update_test.rb index 1231c91a7a..602e0ffc4c 100644 --- a/test/system/flows/maintainer/maintainer_adds_arbitrary_site_update_test.rb +++ b/test/system/flows/maintainer/maintainer_adds_arbitrary_site_update_test.rb @@ -9,9 +9,9 @@ class MaintainerAddsArbitrarySiteUpdateTest < ApplicationSystemTestCase test 'maintainer adds arbitrary site update' do maintainer = create :user, :maintainer, uid: '136131' maintainer.dismiss_introducer!('welcome-modal') - track = create :track, slug: 'fsharp', repo_url: 'exercism/fsharp' + track = create :track, title: 'F#', slug: 'fsharp', repo_url: 'exercism/fsharp' create(:user_track, user: maintainer, track:) - create :github_team_member, team_name: track.github_team_name, user_id: maintainer.uid + create :github_team_member, team_name: track.github_team_name, user: maintainer pr = create :github_pull_request, repo: track.repo_url use_capybara_host do @@ -22,7 +22,7 @@ class MaintainerAddsArbitrarySiteUpdateTest < ApplicationSystemTestCase fill_in 'Title', with: 'F# track now supports .NET 7' fill_in 'Description', with: 'The F# has added support for the _lovely_ .NET 7' - select track.title, from: 'site_update_track_id' + select 'F#', from: 'site_update_track_id' fill_in 'Pull Request number (https://github.com/exercism//pull/)', with: pr.number click_on 'Create Site Update' diff --git a/test/system/flows/user_views_track_test.rb b/test/system/flows/user_views_track_test.rb index 0df7b6da25..9c09f6633f 100644 --- a/test/system/flows/user_views_track_test.rb +++ b/test/system/flows/user_views_track_test.rb @@ -80,7 +80,7 @@ class UserViewsTrackTest < ApplicationSystemTestCase create(:concept_exercise, status: :wip, track:) user = create :user, :maintainer, uid: '256123' create(:user_dismissed_introducer, slug: "track-welcome-modal-#{track.slug}", user:) - create :github_team_member, user_id: user.uid, team_name: track.github_team_name + create :github_team_member, user:, team_name: track.github_team_name create(:user_track, track:, user:) create(:hello_world_solution, :completed, track:, user:) stub_latest_track_forum_threads(track)