Add latest changes from gitlab-org/gitlab@master

4 files changed, 110 insertions, 188 deletions

diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 52533a1087c..b1f66c76f95 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -1298,125 +1298,9 @@ RSpec.describe Group, feature_category: :groups_and_projects do
     end
   end
 
-  describe '#member_last_blocked_owner?' do
-    let!(:blocked_user) { create(:user, :blocked) }
-
-    let!(:member) { group.add_member(blocked_user, GroupMember::OWNER) }
-
-    context 'when last_blocked_owner is set' do
-      before do
-        expect(group).not_to receive(:member_owners_excluding_project_bots)
-      end
-
-      it 'returns true' do
-        member.last_blocked_owner = true
-
-        expect(group.member_last_blocked_owner?(member)).to be(true)
-      end
-
-      it 'returns false' do
-        member.last_blocked_owner = false
-
-        expect(group.member_last_blocked_owner?(member)).to be(false)
-      end
-    end
-
-    context 'when last_blocked_owner is not set' do
-      it { expect(group.member_last_blocked_owner?(member)).to be(true) }
-
-      context 'with another active owner' do
-        before do
-          group.add_member(create(:user), GroupMember::OWNER)
-        end
-
-        it { expect(group.member_last_blocked_owner?(member)).to be(false) }
-      end
-
-      context 'with another active project_bot owner' do
-        before do
-          group.add_member(create(:user, :project_bot), GroupMember::OWNER)
-        end
-
-        it { expect(group.member_last_blocked_owner?(member)).to be(true) }
-      end
-
-      context 'with 2 blocked owners' do
-        before do
-          group.add_member(create(:user, :blocked), GroupMember::OWNER)
-        end
-
-        it { expect(group.member_last_blocked_owner?(member)).to be(false) }
-      end
-
-      context 'with owners from a parent' do
-        context 'when top-level group' do
-          it { expect(group.member_last_blocked_owner?(member)).to be(true) }
-
-          context 'with group sharing' do
-            let!(:subgroup) { create(:group, parent: group) }
-
-            before do
-              create(:group_group_link, :owner, shared_group: group, shared_with_group: subgroup)
-              create(:group_member, :owner, group: subgroup)
-            end
-
-            it { expect(group.member_last_blocked_owner?(member)).to be(true) }
-          end
-        end
-
-        context 'when subgroup' do
-          let!(:subgroup) { create(:group, :nested) }
-
-          let!(:member) { subgroup.add_member(blocked_user, GroupMember::OWNER) }
-
-          it { expect(subgroup.member_last_blocked_owner?(member)).to be(true) }
-
-          context 'with two owners' do
-            before do
-              create(:group_member, :owner, group: subgroup.parent)
-            end
-
-            it { expect(subgroup.member_last_blocked_owner?(member)).to be(false) }
-          end
-        end
-      end
-    end
-  end
-
   context 'when analyzing blocked owners' do
     let_it_be(:blocked_user) { create(:user, :blocked) }
 
-    describe '#single_blocked_owner?' do
-      context 'when there is only one blocked owner' do
-        before do
-          group.add_member(blocked_user, GroupMember::OWNER)
-        end
-
-        it 'returns true' do
-          expect(group.single_blocked_owner?).to eq(true)
-        end
-      end
-
-      context 'when there are multiple blocked owners' do
-        let_it_be(:blocked_user_2) { create(:user, :blocked) }
-
-        before do
-          group.add_member(blocked_user, GroupMember::OWNER)
-          group.add_member(blocked_user_2, GroupMember::OWNER)
-        end
-
-        it 'returns true' do
-          expect(group.single_blocked_owner?).to eq(false)
-        end
-      end
-
-      context 'when there are no blocked owners' do
-        it 'returns false' do
-          expect(group.single_blocked_owner?).to eq(false)
-        end
-      end
-    end
-
     describe '#blocked_owners' do
       let_it_be(:user) { create(:user) }
 
@@ -1490,40 +1374,25 @@ RSpec.describe Group, feature_category: :groups_and_projects do
         expect(empty_group.member_owners_excluding_project_bots).to be_empty
       end
     end
-  end
 
-  describe '#member_last_owner?' do
-    let_it_be(:user) { create(:user) }
-
-    let(:member) { group.members.last }
-
-    before do
-      group.add_member(user, GroupMember::OWNER)
-    end
-
-    context 'when last_owner is set' do
-      before do
-        expect(group).not_to receive(:last_owner?)
-      end
-
-      it 'returns true' do
-        member.last_owner = true
-
-        expect(group.member_last_owner?(member)).to be(true)
+    context 'when user is blocked' do
+      let(:blocked_user) { create(:user, :blocked) }
+      let!(:blocked_member) do
+        group.add_member(blocked_user, GroupMember::OWNER)
       end
 
-      it 'returns false' do
-        member.last_owner = false
-
-        expect(group.member_last_owner?(member)).to be(false)
+      context 'and it is a direct member' do
+        it 'does include blocked user' do
+          expect(group.member_owners_excluding_project_bots).to include(blocked_member)
+        end
       end
-    end
 
-    context 'when last_owner is not set' do
-      it 'returns true' do
-        expect(group).to receive(:last_owner?).and_call_original
+      context 'and it is a member of a parent' do
+        let!(:subgroup) { create(:group, parent: group) }
 
-        expect(group.member_last_owner?(member)).to be(true)
+        it 'does include blocked user' do
+          expect(subgroup.member_owners_excluding_project_bots).to include(blocked_member)
+        end
       end
     end
   end
@@ -1807,12 +1676,14 @@ RSpec.describe Group, feature_category: :groups_and_projects do
     let!(:developer) { group.add_member(create(:user), GroupMember::DEVELOPER) }
     let!(:pending_maintainer) { create(:group_member, :awaiting, :maintainer, group: group.parent) }
     let!(:pending_developer) { create(:group_member, :awaiting, :developer, group: group) }
+    let!(:inactive_developer) { group.add_member(create(:user, :deactivated), GroupMember::DEVELOPER) }
 
     it 'returns parents active members' do
       expect(group.members_with_parents).to include(developer)
       expect(group.members_with_parents).to include(maintainer)
       expect(group.members_with_parents).not_to include(pending_developer)
       expect(group.members_with_parents).not_to include(pending_maintainer)
+      expect(group.members_with_parents).not_to include(inactive_developer)
     end
 
     context 'group sharing' do
@@ -1829,6 +1700,18 @@ RSpec.describe Group, feature_category: :groups_and_projects do
           include(pending_developer))
       end
     end
+
+    context 'when only_active_users is false' do
+      subject { group.members_with_parents(only_active_users: false) }
+
+      it 'returns parents all members' do
+        expect(subject).to include(developer)
+        expect(subject).to include(maintainer)
+        expect(subject).not_to include(pending_developer)
+        expect(subject).not_to include(pending_maintainer)
+        expect(subject).to include(inactive_developer)
+      end
+    end
   end
 
   describe '#members_with_parents' do
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index cf4aa362189..6ffb929a7ab 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -185,6 +185,12 @@ RSpec.describe Member, feature_category: :groups_and_projects do
     before_all do
       @owner_user = create(:user).tap { |u| group.add_owner(u) }
       @owner = group.members.find_by(user_id: @owner_user.id)
+      @blocked_owner_user = create(:user).tap do |u|
+        group.add_owner(u)
+
+        u.block!
+      end
+      @blocked_owner = group.members.find_by(user_id: @blocked_owner_user.id)
 
       @maintainer_user = create(:user).tap { |u| project.add_maintainer(u) }
       @maintainer = project.members.find_by(user_id: @maintainer_user.id)
@@ -473,6 +479,7 @@ RSpec.describe Member, feature_category: :groups_and_projects do
 
     describe '.owners_and_maintainers' do
       it { expect(described_class.owners_and_maintainers).to include @owner }
+      it { expect(described_class.owners_and_maintainers).not_to include @blocked_owner }
       it { expect(described_class.owners_and_maintainers).to include @maintainer }
       it { expect(described_class.owners_and_maintainers).not_to include @invited_member }
       it { expect(described_class.owners_and_maintainers).not_to include @accepted_invite_member }
@@ -481,6 +488,28 @@ RSpec.describe Member, feature_category: :groups_and_projects do
       it { expect(described_class.owners_and_maintainers).not_to include @blocked_maintainer }
     end
 
+    describe '.owners' do
+      it { expect(described_class.owners).to include @owner }
+      it { expect(described_class.owners).not_to include @blocked_owner }
+      it { expect(described_class.owners).not_to include @maintainer }
+      it { expect(described_class.owners).not_to include @invited_member }
+      it { expect(described_class.owners).not_to include @accepted_invite_member }
+      it { expect(described_class.owners).not_to include @requested_member }
+      it { expect(described_class.owners).not_to include @accepted_request_member }
+      it { expect(described_class.owners).not_to include @blocked_maintainer }
+    end
+
+    describe '.all_owners' do
+      it { expect(described_class.all_owners).to include @owner }
+      it { expect(described_class.all_owners).to include @blocked_owner }
+      it { expect(described_class.all_owners).not_to include @maintainer }
+      it { expect(described_class.all_owners).not_to include @invited_member }
+      it { expect(described_class.all_owners).not_to include @accepted_invite_member }
+      it { expect(described_class.all_owners).not_to include @requested_member }
+      it { expect(described_class.all_owners).not_to include @accepted_request_member }
+      it { expect(described_class.all_owners).not_to include @blocked_maintainer }
+    end
+
     describe '.has_access' do
       subject { described_class.has_access.to_a }
 
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index c416e63b915..d9420a37879 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe GroupMember do
+RSpec.describe GroupMember, feature_category: :cell do
   describe 'default values' do
     subject(:goup_member) { build(:group_member) }
 
@@ -121,26 +121,64 @@ RSpec.describe GroupMember do
   end
 
   describe '#last_owner_of_the_group?' do
+    let_it_be(:parent_group) { create(:group) }
+    let_it_be(:group) { create(:group, parent: parent_group) }
+    let_it_be(:group_member) { create(:group_member, :owner, source: group) }
+
+    subject { group_member.last_owner_of_the_group? }
+
+    context 'when overridden by last_owner instance variable' do
+      before do
+        group_member.last_owner = last_owner
+      end
+
+      after do
+        group_member.last_owner = nil
+      end
+
+      context 'and it is set to true' do
+        let(:last_owner) { true }
+
+        it { is_expected.to be(true) }
+      end
+
+      context 'and it is set to false' do
+        let(:last_owner) { false }
+
+        it { is_expected.to be(false) }
+      end
+    end
+
     context 'when member is an owner' do
-      let_it_be(:group_member) { build(:group_member, :owner) }
+      context 'and there are no other owners' do
+        it { is_expected.to be(true) }
+
+        context 'and member is also owner of a parent group' do
+          before do
+            parent_group.add_owner(group_member.user)
+          end
 
-      using RSpec::Parameterized::TableSyntax
+          after do
+            parent_group.members.delete_all
+          end
 
-      where(:member_last_owner?, :member_last_blocked_owner?, :expected) do
-        false | false | false
-        true  | false | true
-        false | true  | true
-        true  | true  | true
+          it { is_expected.to be(false) }
+        end
       end
 
-      with_them do
-        it "returns expected" do
-          allow(group_member.group).to receive(:member_last_owner?).with(group_member).and_return(member_last_owner?)
-          allow(group_member.group).to receive(:member_last_blocked_owner?)
-            .with(group_member)
-            .and_return(member_last_blocked_owner?)
+      context 'and there is another owner' do
+        context 'and that other owner is a project bot' do
+          let(:project_bot) { create(:user, :project_bot) }
+          let!(:other_owner_bot) { create(:group_member, :owner, source: group, user: project_bot) }
 
-          expect(group_member.last_owner_of_the_group?).to be(expected)
+          it { is_expected.to be(true) }
+        end
+
+        context 'and that other owner is not a project bot' do
+          let(:other_user) { create(:user) }
+          let!(:other_owner) { create(:group_member, :owner, source: group, user: other_user) }
+
+          it { is_expected.to be(false) }
         end
       end
     end
@@ -148,8 +186,6 @@ RSpec.describe GroupMember do
     context 'when member is not an owner' do
       let_it_be(:group_member) { build(:group_member, :guest) }
 
-      subject { group_member.last_owner_of_the_group? }
-
       it { is_expected.to be(false) }
     end
   end
diff --git a/spec/models/members/last_group_owner_assigner_spec.rb b/spec/models/members/last_group_owner_assigner_spec.rb
index a0a829221de..2539388c667 100644
--- a/spec/models/members/last_group_owner_assigner_spec.rb
+++ b/spec/models/members/last_group_owner_assigner_spec.rb
@@ -24,8 +24,6 @@ RSpec.describe LastGroupOwnerAssigner do
         specify do
           expect { assigner.execute }.to change(group_member, :last_owner)
                                            .from(nil).to(true)
-                                           .and change(group_member, :last_blocked_owner)
-                                                  .from(nil).to(false)
         end
       end
 
@@ -35,8 +33,6 @@ RSpec.describe LastGroupOwnerAssigner do
         specify do
           expect { assigner.execute }.to change(group_member, :last_owner)
                                            .from(nil).to(false)
-                                           .and change(group_member, :last_blocked_owner)
-                                                  .from(nil).to(false)
         end
 
         it "has many members passed" do
@@ -44,12 +40,8 @@ RSpec.describe LastGroupOwnerAssigner do
 
           expect { assigner.execute }.to change(group_member, :last_owner)
                                            .from(nil).to(false)
-                                           .and change(group_member, :last_blocked_owner)
-                                                  .from(nil).to(false)
                                                   .and change(unblocked_owner_member, :last_owner)
                                                          .from(nil).to(false)
-                                                         .and change(unblocked_owner_member, :last_blocked_owner)
-                                                                .from(nil).to(false)
         end
       end
 
@@ -66,8 +58,6 @@ RSpec.describe LastGroupOwnerAssigner do
             specify do
               expect { assigner.execute }.to change(group_member, :last_owner)
                 .from(nil).to(true)
-                .and change(group_member, :last_blocked_owner)
-                .from(nil).to(false)
             end
           end
         end
@@ -81,8 +71,6 @@ RSpec.describe LastGroupOwnerAssigner do
           specify do
             expect { assigner.execute }.to change(group_member_2, :last_owner)
               .from(nil).to(false)
-              .and change(group_member_2, :last_blocked_owner)
-              .from(nil).to(false)
           end
         end
       end
@@ -96,9 +84,7 @@ RSpec.describe LastGroupOwnerAssigner do
       context "with one blocked owner" do
         specify do
           expect { assigner.execute }.to change(group_member, :last_owner)
-                                           .from(nil).to(false)
-                                           .and change(group_member, :last_blocked_owner)
-                                                  .from(nil).to(true)
+                                           .from(nil).to(true)
         end
       end
 
@@ -108,8 +94,6 @@ RSpec.describe LastGroupOwnerAssigner do
 
           expect { assigner.execute }.to change(group_member, :last_owner)
                                            .from(nil).to(false)
-                                           .and change(group_member, :last_blocked_owner)
-                                                  .from(nil).to(false)
         end
       end
 
@@ -119,8 +103,6 @@ RSpec.describe LastGroupOwnerAssigner do
 
           expect { assigner.execute }.to change(group_member, :last_owner)
                                            .from(nil).to(false)
-                                           .and change(group_member, :last_blocked_owner)
-                                                  .from(nil).to(false)
         end
       end
 
@@ -136,8 +118,6 @@ RSpec.describe LastGroupOwnerAssigner do
 
             specify do
               expect { assigner.execute }.to change(group_member, :last_owner)
-                .from(nil).to(false)
-                .and change(group_member, :last_blocked_owner)
                 .from(nil).to(true)
             end
           end
@@ -152,8 +132,6 @@ RSpec.describe LastGroupOwnerAssigner do
 
           specify do
             expect { assigner.execute }.to change(group_member, :last_owner)
-              .from(nil).to(false)
-              .and change(group_member, :last_blocked_owner)
               .from(nil).to(true)
           end
 
@@ -165,8 +143,6 @@ RSpec.describe LastGroupOwnerAssigner do
             specify do
               expect { assigner.execute }.to change(group_member, :last_owner)
                 .from(nil).to(false)
-                .and change(group_member, :last_blocked_owner)
-                .from(nil).to(false)
             end
           end
         end
@@ -180,8 +156,6 @@ RSpec.describe LastGroupOwnerAssigner do
 
           expect { assigner.execute }.to change(group_member, :last_owner)
             .from(nil).to(true)
-            .and change(group_member, :last_blocked_owner)
-            .from(nil).to(false)
         end
       end
     end