Merge branch 'peterhegman/fix-membership-source-calculation' into 'master'

Change how direct vs indirect members are calculated

See merge request gitlab-org/gitlab!52541

app/assets/javascripts/members/components/table/members_table.vue

@@ -32,7 +32,7 @@ export default {
       import('ee_component/members/components/ldap/ldap_override_confirmation_modal.vue'),
   },
   computed: {
-    ...mapState(['members', 'tableFields', 'tableAttrs', 'currentUserId', 'sourceId']),
+    ...mapState(['members', 'tableFields', 'tableAttrs', 'currentUserId']),
     filteredFields() {
       return FIELDS.filter(
         (field) => this.tableFields.includes(field.key) && this.showField(field),
@@ -55,9 +55,9 @@ export default {
   methods: {
     hasActionButtons(member) {
       return (
-        canRemove(member, this.sourceId) ||
+        canRemove(member) ||
         canResend(member) ||
-        canUpdate(member, this.currentUserId, this.sourceId) ||
+        canUpdate(member, this.currentUserId) ||
         canOverride(member)
       );
     },

app/assets/javascripts/members/components/table/members_table_cell.vue

@@ -19,7 +19,7 @@ export default {
     },
   },
   computed: {
-    ...mapState(['sourceId', 'currentUserId']),
+    ...mapState(['currentUserId']),
     isGroup() {
       return isGroup(this.member);
     },
@@ -41,19 +41,19 @@ export default {
       return MEMBER_TYPES.user;
     },
     isDirectMember() {
-      return isDirectMember(this.member, this.sourceId);
+      return isDirectMember(this.member);
     },
     isCurrentUser() {
       return isCurrentUser(this.member, this.currentUserId);
     },
     canRemove() {
-      return canRemove(this.member, this.sourceId);
+      return canRemove(this.member);
     },
     canResend() {
       return canResend(this.member);
     },
     canUpdate() {
-      return canUpdate(this.member, this.currentUserId, this.sourceId);
+      return canUpdate(this.member, this.currentUserId);
     },
   },
   render() {

app/assets/javascripts/members/utils.js

@@ -35,26 +35,24 @@ export const isGroup = (member) => {
   return Boolean(member.sharedWithGroup);
 };
 
-export const isDirectMember = (member, sourceId) => {
-  return isGroup(member) || member.source?.id === sourceId;
+export const isDirectMember = (member) => {
+  return isGroup(member) || member.isDirectMember;
 };
 
 export const isCurrentUser = (member, currentUserId) => {
   return member.user?.id === currentUserId;
 };
 
-export const canRemove = (member, sourceId) => {
-  return isDirectMember(member, sourceId) && member.canRemove;
+export const canRemove = (member) => {
+  return isDirectMember(member) && member.canRemove;
 };
 
 export const canResend = (member) => {
   return Boolean(member.invite?.canResend);
 };
 
-export const canUpdate = (member, currentUserId, sourceId) => {
-  return (
-    !isCurrentUser(member, currentUserId) && isDirectMember(member, sourceId) && member.canUpdate
-  );
+export const canUpdate = (member, currentUserId) => {
+  return !isCurrentUser(member, currentUserId) && isDirectMember(member) && member.canUpdate;
 };
 
 export const parseSortParam = (sortableFields) => {

app/helpers/groups/group_members_helper.rb

@@ -18,7 +18,7 @@ module Groups::GroupMembersHelper
   end
 
   def members_data_json(group, members)
-    MemberSerializer.new.represent(members, { current_user: current_user, group: group }).to_json
+    MemberSerializer.new.represent(members, { current_user: current_user, group: group, source: group }).to_json
   end
 
   # Overridden in `ee/app/helpers/ee/groups/group_members_helper.rb`

app/helpers/projects/project_members_helper.rb

@@ -32,7 +32,7 @@ module Projects::ProjectMembersHelper
   end
 
   def project_members_data_json(project, members)
-    MemberSerializer.new.represent(members, { current_user: current_user, group: project.group }).to_json
+    MemberSerializer.new.represent(members, { current_user: current_user, group: project.group, source: project }).to_json
   end
 
   def project_members_list_data_attributes(project, members)

app/serializers/member_entity.rb

@@ -23,6 +23,10 @@ class MemberEntity < Grape::Entity
     member.can_remove?
   end
 
+  expose :is_direct_member do |member, options|
+    member.source == options[:source]
+  end
+
   expose :access_level do
     expose :human_access, as: :string_value
     expose :access_level, as: :integer_value

ee/spec/frontend/members/components/table/member_table_cell_spec.js

@@ -80,7 +80,7 @@ describe('MemberTableCell', () => {
     describe('canOverride', () => {
       it('returns `true` when `canOverride` is `true`', () => {
         createComponent({
-          member: { memberMock, canOverride: true },
+          member: { ...memberMock, canOverride: true },
         });
 
         expect(findWrappedComponent().props('permissions').canOverride).toBe(true);
@@ -88,7 +88,7 @@ describe('MemberTableCell', () => {
 
       it('returns `false` when `canOverride` is `false`', () => {
         createComponent({
-          member: { memberMock, canOverride: false },
+          member: { ...memberMock, canOverride: false },
         });
 
         expect(findWrappedComponent().props('permissions').canOverride).toBe(false);

spec/fixtures/api/schemas/entities/member.json

@@ -9,7 +9,8 @@
     "source",
     "valid_roles",
     "can_update",
-    "can_remove"
+    "can_remove",
+    "is_direct_member"
   ],
   "properties": {
     "id": { "type": "integer" },
@@ -18,6 +19,7 @@
     "requested_at": { "type": ["date-time", "null"] },
     "can_update": { "type": "boolean" },
     "can_remove": { "type": "boolean" },
+    "is_direct_member": { "type": "boolean" },
     "access_level": {
       "type": "object",
       "required": ["integer_value", "string_value"],

spec/frontend/members/components/table/members_table_cell_spec.js

 import { mount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
 import { MEMBER_TYPES } from '~/members/constants';
-import { member as memberMock, group, invite, accessRequest } from '../../mock_data';
+import {
+  member as memberMock,
+  directMember,
+  inheritedMember,
+  group,
+  invite,
+  accessRequest,
+} from '../../mock_data';
 import MembersTableCell from '~/members/components/table/members_table_cell.vue';
 
 describe('MembersTableCell', () => {
@@ -75,19 +82,12 @@ describe('MembersTableCell', () => {
 
   const createComponentWithDirectMember = (member = {}) => {
     createComponent({
-      member: {
-        ...memberMock,
-        source: {
-          ...memberMock.source,
-          id: 1,
-        },
-        ...member,
-      },
+      member: { ...directMember, ...member },
     });
   };
   const createComponentWithInheritedMember = (member = {}) => {
     createComponent({
-      member: { ...memberMock, ...member },
+      member: { ...inheritedMember, ...member },
     });
   };
 

spec/frontend/members/components/table/members_table_spec.js

@@ -15,7 +15,7 @@ import RoleDropdown from '~/members/components/table/role_dropdown.vue';
 import ExpirationDatepicker from '~/members/components/table/expiration_datepicker.vue';
 import MemberActionButtons from '~/members/components/table/member_action_buttons.vue';
 import * as initUserPopovers from '~/user_popovers';
-import { member as memberMock, invite, accessRequest } from '../../mock_data';
+import { member as memberMock, directMember, invite, accessRequest } from '../../mock_data';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -74,11 +74,6 @@ describe('MembersTable', () => {
   });
 
   describe('fields', () => {
-    const directMember = {
-      ...memberMock,
-      source: { ...memberMock.source, id: 1 },
-    };
-
     const memberCanUpdate = {
       ...directMember,
       canUpdate: true,

spec/frontend/members/mock_data.js

@@ -4,6 +4,7 @@ export const member = {
   canRemove: false,
   canOverride: false,
   isOverridden: false,
+  isDirectMember: false,
   accessLevel: { integerValue: 50, stringValue: 'Owner' },
   source: {
     id: 178,
@@ -71,3 +72,6 @@ export const accessRequest = {
 export const members = [member];
 
 export const membersJsonString = JSON.stringify(members);
+
+export const directMember = { ...member, isDirectMember: true };
+export const inheritedMember = { ...member, isDirectMember: false };

spec/frontend/members/utils_spec.js

@@ -13,10 +13,16 @@ import {
   groupLinkRequestFormatter,
 } from '~/members/utils';
 import { DEFAULT_SORT } from '~/members/constants';
-import { member as memberMock, group, invite, membersJsonString, members } from './mock_data';
+import {
+  member as memberMock,
+  directMember,
+  inheritedMember,
+  group,
+  invite,
+  membersJsonString,
+  members,
+} from './mock_data';
 
-const DIRECT_MEMBER_ID = 178;
-const INHERITED_MEMBER_ID = 179;
 const IS_CURRENT_USER_ID = 123;
 const IS_NOT_CURRENT_USER_ID = 124;
 const URL_HOST = 'https://localhost/';
@@ -59,11 +65,11 @@ describe('Members Utils', () => {
 
   describe('isDirectMember', () => {
     test.each`
-      sourceId               | expected
-      ${DIRECT_MEMBER_ID}    | ${true}
-      ${INHERITED_MEMBER_ID} | ${false}
-    `('returns $expected', ({ sourceId, expected }) => {
-      expect(isDirectMember(memberMock, sourceId)).toBe(expected);
+      member             | expected
+      ${directMember}    | ${true}
+      ${inheritedMember} | ${false}
+    `('returns $expected', ({ member, expected }) => {
+      expect(isDirectMember(member)).toBe(expected);
     });
   });
 
@@ -78,18 +84,13 @@ describe('Members Utils', () => {
   });
 
   describe('canRemove', () => {
-    const memberCanRemove = {
-      ...memberMock,
-      canRemove: true,
-    };
-
     test.each`
-      member             | sourceId               | expected
-      ${memberCanRemove} | ${DIRECT_MEMBER_ID}    | ${true}
-      ${memberCanRemove} | ${INHERITED_MEMBER_ID} | ${false}
-      ${memberMock}      | ${INHERITED_MEMBER_ID} | ${false}
-    `('returns $expected', ({ member, sourceId, expected }) => {
-      expect(canRemove(member, sourceId)).toBe(expected);
+      member                                     | expected
+      ${{ ...directMember, canRemove: true }}    | ${true}
+      ${{ ...inheritedMember, canRemove: true }} | ${false}
+      ${{ ...memberMock, canRemove: false }}     | ${false}
+    `('returns $expected', ({ member, expected }) => {
+      expect(canRemove(member)).toBe(expected);
     });
   });
 
@@ -98,25 +99,20 @@ describe('Members Utils', () => {
       member                                                           | expected
       ${invite}                                                        | ${true}
       ${{ ...invite, invite: { ...invite.invite, canResend: false } }} | ${false}
-    `('returns $expected', ({ member, sourceId, expected }) => {
-      expect(canResend(member, sourceId)).toBe(expected);
+    `('returns $expected', ({ member, expected }) => {
+      expect(canResend(member)).toBe(expected);
     });
   });
 
   describe('canUpdate', () => {
-    const memberCanUpdate = {
-      ...memberMock,
-      canUpdate: true,
-    };
-
     test.each`
-      member             | currentUserId             | sourceId               | expected
-      ${memberCanUpdate} | ${IS_NOT_CURRENT_USER_ID} | ${DIRECT_MEMBER_ID}    | ${true}
-      ${memberCanUpdate} | ${IS_CURRENT_USER_ID}     | ${DIRECT_MEMBER_ID}    | ${false}
-      ${memberCanUpdate} | ${IS_CURRENT_USER_ID}     | ${INHERITED_MEMBER_ID} | ${false}
-      ${memberMock}      | ${IS_NOT_CURRENT_USER_ID} | ${DIRECT_MEMBER_ID}    | ${false}
-    `('returns $expected', ({ member, currentUserId, sourceId, expected }) => {
-      expect(canUpdate(member, currentUserId, sourceId)).toBe(expected);
+      member                                     | currentUserId             | expected
+      ${{ ...directMember, canUpdate: true }}    | ${IS_NOT_CURRENT_USER_ID} | ${true}
+      ${{ ...directMember, canUpdate: true }}    | ${IS_CURRENT_USER_ID}     | ${false}
+      ${{ ...inheritedMember, canUpdate: true }} | ${IS_CURRENT_USER_ID}     | ${false}
+      ${{ ...directMember, canUpdate: false }}   | ${IS_NOT_CURRENT_USER_ID} | ${false}
+    `('returns $expected', ({ member, currentUserId, expected }) => {
+      expect(canUpdate(member, currentUserId)).toBe(expected);
     });
   });
 

spec/serializers/member_entity_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe MemberEntity do
   let_it_be(:current_user) { create(:user) }
-  let(:entity) { described_class.new(member, { current_user: current_user, group: group }) }
+  let(:entity) { described_class.new(member, { current_user: current_user, group: group, source: source }) }
   let(:entity_hash) { entity.as_json }
 
   shared_examples 'member.json' do
@@ -40,8 +40,27 @@ RSpec.describe MemberEntity do
     end
   end
 
+  shared_examples 'is_direct_member' do
+    context 'when `source` is the same as `member.source`' do
+      let(:source) { direct_member_source }
+
+      it 'exposes `is_direct_member` as `true`' do
+        expect(entity_hash[:is_direct_member]).to be(true)
+      end
+    end
+
+    context 'when `source` is not the same as `member.source`' do
+      let(:source) { inherited_member_source }
+
+      it 'exposes `is_direct_member` as `false`' do
+        expect(entity_hash[:is_direct_member]).to be(false)
+      end
+    end
+  end
+
   context 'group member' do
     let(:group) { create(:group) }
+    let(:source) { group }
     let(:member) { GroupMemberPresenter.new(create(:group_member, group: group), current_user: current_user) }
 
     it_behaves_like 'member.json'
@@ -52,11 +71,19 @@ RSpec.describe MemberEntity do
       it_behaves_like 'member.json'
       it_behaves_like 'invite'
     end
+
+    context 'is_direct_member' do
+      let(:direct_member_source) { group }
+      let(:inherited_member_source) { create(:group) }
+
+      it_behaves_like 'is_direct_member'
+    end
   end
 
   context 'project member' do
     let(:project) { create(:project) }
     let(:group) { project.group }
+    let(:source) { project }
     let(:member) { ProjectMemberPresenter.new(create(:project_member, project: project), current_user: current_user) }
 
     it_behaves_like 'member.json'
@@ -67,5 +94,12 @@ RSpec.describe MemberEntity do
       it_behaves_like 'member.json'
       it_behaves_like 'invite'
     end
+
+    context 'is_direct_member' do
+      let(:direct_member_source) { project }
+      let(:inherited_member_source) { group }
+
+      it_behaves_like 'is_direct_member'
+    end
   end
 end

spec/serializers/member_serializer_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe MemberSerializer do
 
   let_it_be(:current_user) { create(:user) }
 
-  subject { described_class.new.represent(members, { current_user: current_user, group: group }) }
+  subject { described_class.new.represent(members, { current_user: current_user, group: group, source: source }) }
 
   shared_examples 'members.json' do
     it 'matches json schema' do
@@ -17,6 +17,7 @@ RSpec.describe MemberSerializer do
 
   context 'group member' do
     let(:group) { create(:group) }
+    let(:source) { group }
     let(:members) { present_members(create_list(:group_member, 1, group: group)) }
 
     it_behaves_like 'members.json'
@@ -24,6 +25,7 @@ RSpec.describe MemberSerializer do
 
   context 'project member' do
     let(:project) { create(:project) }
+    let(:source) { project }
     let(:group) { project.group }
     let(:members) { present_members(create_list(:project_member, 1, project: project)) }