keep roles inherited from different group on role adding

app/services/groups/update_roles_service.rb

@@ -78,15 +78,20 @@ module Groups
           WHERE member_roles.member_id = :member_id
         ),
         -- delete all roles assigned to users that group no longer has but keep those that the user
-        -- has independently of the group (not inherited)
+        -- has independently of the group (not inherited) or inherited from a different group
         remove_roles AS (
-          DELETE FROM #{MemberRole.table_name}
+          DELETE FROM #{MemberRole.table_name} delete_member_roles
           USING #{MemberRole.table_name} user_member_roles
           JOIN user_members ON user_members.id = user_member_roles.member_id
-          LEFT JOIN group_member_roles ON user_member_roles.role_id = group_member_roles.role_id
-          WHERE user_member_roles.inherited_from IS NOT NULL AND group_member_roles.role_id IS NULL
-          AND member_roles.id = user_member_roles.id
-          RETURNING #{MemberRole.table_name}.id, #{MemberRole.table_name}.member_id, #{MemberRole.table_name}.role_id
+          LEFT JOIN #{MemberRole.table_name} inheriting_member_roles
+            ON user_member_roles.role_id = inheriting_member_roles.role_id
+            AND user_member_roles.inherited_from = inheriting_member_roles.id
+          WHERE user_member_roles.inherited_from IS NOT NULL AND inheriting_member_roles.id IS NULL
+          AND delete_member_roles.id = user_member_roles.id
+          RETURNING
+            delete_member_roles.id,
+            delete_member_roles.member_id,
+            delete_member_roles.role_id
         ),
         -- add all roles to the user memberships
         add_roles AS (

db/migrate/20220426132637_refix_inherited_group_member_roles.rb

@@ -0,0 +1,14 @@
+class RefixInheritedGroupMemberRoles < ActiveRecord::Migration[6.1]
+  def up
+    # When the FixInheritedGroupMemberRoles ran initially, Members
+    # where the MemberRoles were inherited from more than one Group where
+    # applied incorrectly. Only the MemberRoles of the last Group where kept.
+    require Rails.root.join('db/migrate/20200625133727_fix_inherited_group_member_roles.rb')
+
+    # created_on has been renamed to created_at
+    Member.reset_column_information
+    Principal.reset_column_information
+
+    FixInheritedGroupMemberRoles.new.up
+  end
+end

spec/services/groups/update_roles_service_integration_spec.rb

@@ -360,6 +360,47 @@ describe Groups::UpdateRolesService, 'integration', type: :model do
     end
   end
 
+  context 'when adding a role and the user has a role already granted by a different group' do
+    let(:other_role) { create(:role) }
+
+    let!(:second_group) do
+      create(:group,
+             members: users).tap do |group|
+        create(:member,
+               project: project,
+               principal: group,
+               roles: [other_role])
+
+        ::Groups::AddUsersService
+          .new(group, current_user: User.system, contract_class: EmptyContract)
+          .call(ids: users.map(&:id))
+      end
+    end
+
+    let(:users) { [create(:user)] }
+    let(:added_role) { create(:role) }
+
+    before do
+      member.roles << added_role
+    end
+
+    it 'is successful' do
+      expect(service_call)
+        .to be_success
+    end
+
+    it 'keeps the roles the user already had before and adds the new one' do
+      service_call
+
+      expect(Member.find_by(principal: users.first).roles.uniq)
+        .to match_array([role, other_role, added_role])
+    end
+
+    it_behaves_like 'sends notification' do
+      let(:user) { users }
+    end
+  end
+
   context 'when not allowed' do
     let(:current_user) { User.anonymous }