Commit 5b4e9938 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'group-owners-association' into 'master'

Turn Group#owners into a has_many association

## What does this MR do?

This turns the regular method `Group#owners` into a `has_many` association.

## Are there points in the code the reviewer needs to double check?

As far as I can tell there's no way to do this without using an intermediate association, but perhaps I'm missing something. The reason an intermediate association is needed is because the supplied Proc is applied to the _final_ association (the one returning users), this means that when using a single `has_many` you can't filter out any intermediate rows (e.g. group members).

## Why was this MR needed?

This code being a regular method would prevent eager loading of the owners of a Group, turning it into a `has_many` association resolves this problem. This was discovered in !4410.

## What are the relevant issue numbers?

None.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- [x] ~~API support added~~
- [ ] Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4676
parents 1d20eefd 84e2be5a
......@@ -9,6 +9,12 @@ class Group < Namespace
has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
alias_method :members, :group_members
has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members
has_many :owners,
-> { where(members: { access_level: Gitlab::Access::OWNER }) },
through: :group_members,
source: :user
has_many :project_group_links, dependent: :destroy
has_many :shared_projects, through: :project_group_links, source: :project
has_many :notification_settings, dependent: :destroy, as: :source
......@@ -88,10 +94,6 @@ class Group < Namespace
end
end
def owners
@owners ||= group_members.owners.includes(:user).map(&:user)
end
def add_users(user_ids, access_level, current_user = nil)
user_ids.each do |user_id|
Member.add_user(self.group_members, user_id, access_level, current_user)
......
......@@ -158,6 +158,18 @@ describe Group, models: true do
it { expect(group.has_master?(@members[:requester])).to be_falsey }
end
describe '#owners' do
let(:owner) { create(:user) }
let(:developer) { create(:user) }
it 'returns the owners of a Group' do
group.add_owner(owner)
group.add_developer(developer)
expect(group.owners).to eq([owner])
end
end
def setup_group_members(group)
members = {
owner: create(:user),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment