Merge pull request #8473 from opf/fix/inherited-member-roles

[33659] Fix inserting on group MEMBER id not MEMBER ROLE id

app/services/groups/add_users_service.rb

@@ -83,7 +83,8 @@ def add_to_user_and_projects_cte
           SELECT members.project_id AS project_id,
                  members.user_id AS user_id,
                  members.id AS member_id,
-                 member_roles.role_id AS role_id
+                 member_roles.role_id AS role_id,
+                 member_roles.id AS member_role_id
           FROM #{MemberRole.table_name} member_roles
           JOIN #{Member.table_name} members
           ON members.id = member_roles.member_id AND members.user_id = :group_id
@@ -103,12 +104,13 @@ def add_to_user_and_projects_cte
           -- even if they already exist as members (e.g., added individually) to ensure we add all roles
           -- to mark that we reset the created_at date since replacing the member
           ON CONFLICT(project_id, user_id) DO UPDATE SET created_on = CURRENT_TIMESTAMP
-          RETURNING id, user_id
+          RETURNING id, user_id, project_id
         )
         -- copy the member roles of the group
         INSERT INTO #{MemberRole.table_name} (member_id, role_id, inherited_from)
-        SELECT new_members.id, group_roles.role_id, group_roles.member_id
-        FROM group_roles, new_members
+        SELECT new_members.id, group_roles.role_id, group_roles.member_role_id
+        FROM group_roles
+        JOIN new_members ON group_roles.project_id = new_members.project_id
         -- Ignore if the role was already inserted by us
         ON CONFLICT DO NOTHING
       SQL

db/migrate/20200625133727_fix_inherited_group_member_roles.rb

@@ -0,0 +1,22 @@
+class FixInheritedGroupMemberRoles < ActiveRecord::Migration[6.0]
+  def up
+    # Delete all member roles that should be inherited by groups
+    MemberRole.where.not(inherited_from: nil).delete_all
+
+    # For all group memberships, recreate the member_roles for all users
+    # which will auto-create members for the users if necessary
+    MemberRole
+      .joins(member: [:principal])
+      .includes(member: %i[principal member_roles])
+      .where("#{Principal.table_name}.type" => 'Group')
+      .find_each do |member_role|
+
+      # Recreate member_roles for all group members
+      member_role.send :add_role_to_group_users
+    end
+  end
+
+  def down
+    # Nothing to do
+  end
+end

spec/features/groups/group_memberships_spec.rb

@@ -93,4 +93,75 @@
       expect(page).to have_text 'There are currently no projects part of this group.'
     end
   end
+
+  describe 'with the group in two projects' do
+    let!(:project2) { FactoryBot.create :project, name: 'Project 2', identifier: 'project2' }
+    let(:members_page1) { Pages::Members.new project.identifier }
+    let(:members_page2) { Pages::Members.new project2.identifier }
+
+    before do
+      project.add_member! peter, [manager]
+      project2.add_member! peter, [manager]
+
+      project.add_member! group, [developer]
+      project2.add_member! group, [developer]
+    end
+
+    it 'can add a new user to the group with correct member roles (Regression #33659)' do
+      members_page1.visit!
+
+      expect(members_page1).to have_group 'A-Team', roles: [developer]
+      expect(members_page1).to have_user 'Peter Pan', roles: [manager, developer]
+      expect(members_page1).not_to have_user 'Hannibal Smith'
+
+      members_page2.visit!
+
+      expect(members_page2).to have_group 'A-Team', roles: [developer]
+      expect(members_page2).to have_user 'Peter Pan', roles: [manager, developer]
+      expect(members_page2).not_to have_user 'Hannibal Smith'
+
+      # Add hannibal to the group
+      group_page.visit!
+      group_page.add_user! 'Hannibal'
+      expect(page).to have_text 'Successful update'
+
+      members_page1.visit!
+      expect(members_page1).to have_group 'A-Team', roles: [developer]
+      expect(members_page1).to have_user 'Peter Pan', roles: [manager, developer]
+      expect(members_page1).to have_user 'Hannibal Smith', roles: [developer]
+
+      members_page2.visit!
+
+      expect(members_page2).to have_group 'A-Team', roles: [developer]
+      expect(members_page2).to have_user 'Peter Pan', roles: [manager, developer]
+      expect(members_page2).to have_user 'Hannibal Smith', roles: [developer]
+
+      group_member = project2.member_principals.find_by(user_id: group.id)
+      expect(group_member.member_roles.count).to eq 1
+      group_member_role = group_member.member_roles.first
+      expect(group_member_role.role).to eq developer
+
+      # Expect hannibal's role to be inherited by the group role
+      hannibal_member = project2.members.find_by(user_id: hannibal.id)
+      expect(hannibal_member.member_roles.count).to eq 1
+      expect(hannibal_member.member_roles.first.inherited_from).to eq group_member_role.id
+      expect(hannibal_member.member_roles.first.role).to eq developer
+
+      # Remove the group from members page
+      members_page2.remove_group! 'A-Team'
+      expect(page).to have_text 'Removed A-Team from project.', wait: 10
+      expect(members_page2).to have_user 'Peter Pan', roles: [manager]
+
+      expect(members_page2).not_to have_group 'A-Team'
+      expect(members_page2).not_to have_user 'Hannibal Smith'
+
+      # Expect we can remove peter pan now
+      members_page2.remove_user! 'Peter Pan'
+
+      expect(page).to have_text 'Removed Peter Pan from project.', wait: 10
+      expect(members_page2).not_to have_user 'Peter Pan'
+      expect(members_page2).not_to have_group 'A-Team'
+      expect(members_page2).not_to have_user 'Hannibal Smith'
+    end
+  end
 end