Make user_id foreign key for github team members

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

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

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
 

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

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

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

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

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"

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

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

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

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?

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