groupify icon indicating copy to clipboard operation
groupify copied to clipboard

Inverse of has_members missing as a feature?

Open juhazi opened this issue 8 years ago • 8 comments

Taking the active_record_spec example setup with only the relevant parts:

class User < ActiveRecord::Base
  groupify :group_member
end

class Manager < User
end

class Group < ActiveRecord::Base
  groupify :group
end

class Organization < Group
  has_members :managers
end

With this, Organization class has a has_many :managers association.

How do I get an inverse_of association of that as has_many :organizations relation in the Manager class?

I would expect there to be a has_groups method available for group members.

juhazi avatar Jun 13 '16 08:06 juhazi

There are a couple important things to understand about the current design of Groupify.

  • A member can only belong to one type of group. This must be defined in the groups association.
  • When using inheritance in models, child classes inherit the association of their parent.

In the sample code above, User and Manager both share the same groups association pointing to the Group class. Since Organization inherits from Group, it is valid for a Manager to belong to either a Group or an Organization.

If you want to enforce that a Manager can only belong to an Organization rather than a generic Group, you can try adding this into the Manager class:

groupify :group_member, group_class_name: 'Organization'

This should work because it will define a new groups association on the Manager class which would override the one defined in the User class.

Let me know if this works for you, and I can add a note to the README about this use case.

dwbutler avatar Jun 13 '16 18:06 dwbutler

I wrote some specs to clarify my intention.

has_groups :organizations association

First off we define a counterpart to the has_many :managers association generated into Organization class:

class Manager < User
  has_many :organizations,
    -> { uniq },
    through: :group_memberships_as_member,
    source: :group,
    source_type: 'Group'
end

then I would expect the following spec to pass(which it does):

      it "adds a manager to an organization" do
        organization = Organization.create!
        manager = Manager.create!

        organization.add manager

        expect(organization.managers).to include(manager)
        expect(manager.organizations).to include(organization)
      end

So my wish here would be to have a helper to define the association like with has_members.

has_members :managers scoping issue

But then I noticed that organization.managers does not list only managers, but all users:

      it "adds only managers to an organizations managers" do
        organization = Organization.create!
        user = User.create!
        manager = Manager.create!

        organization.add manager
        organization.add user

        expect(organization.managers).to include(manager)
        expect(organization.managers).to_not include(user) #TODO failing
      end

Is this expected behaviour or is this a bug?

(Note that my custom organizations association above has the same scoping issue)

juhazi avatar Jun 14 '16 09:06 juhazi

So the desired behavior is to have a way to define a custom association which returns a subset of possible groups (filtered by type)? That seems reasonable.

The behavior with managers seems like a bug. I would expect the organization.managers association to filter by users.type = 'Manager'.

dwbutler avatar Jun 15 '16 03:06 dwbutler

I have sketched out a suggestion for the helper at the commit referenced above by paraphrasing your work on the PR above.

How should the has_groups method be named to avoid confusion about its usage? aka. it's not meant to restrict membership but to act as a counterpart to has_members. Also it is only relevant with STI so that could be included somehow.

The commit still needs to be merged with your open PR above to ensure compatibility when a model is both a group and a member and also uses both has_groups and has_members.

juhazi avatar Jun 15 '16 15:06 juhazi

@juhazi I started doing similar work last week to support allowing a member to belong to multiple groups (of any class, not just of one base class). Here is as far as I got: https://github.com/dwbutler/groupify/compare/feature/multiple-group-associations

It's very similar to the work you did. I ended up calling the method belongs_to_groups instead of has_groups, otherwise our work is nearly identical.

I abandoned this approach when I realized how much work it will be. It will essentially require a rewrite of the entire gem. Everywhere there is encoded the assumption that there is a single groups association that contains all the groups. This would no longer be possible if there are multiple, independent, group associations that have to query different tables. All of the membership predicate methods, (such as shares_any_group?) would have to be rewritten to query the group_memberships table directly.

Furthermore, it's not clear to me that this approach will be compatible with how the Mongoid adapter is currently implemented, which might necessitate storing custom data structures instead of using the built in Mongoid relations.

In any case, we can implement a similar interface here, with the documented restriction that the membership classes must be subclasses of Group in order for the gem to work correctly. And then in the future, perhaps the same interface can be reused with that restriction lifted.

dwbutler avatar Jun 18 '16 06:06 dwbutler

So feature-wise my commit would check all the boxes. I'll change the name to yours or should it be belongs_to_group_subclasses or something to reflect the restriction?

This feature should probably be merged after #48 because it has the same Mongoid issue and filtering should work correctly both ways.

juhazi avatar Jun 27 '16 10:06 juhazi

I would stick to naming it belongs_to_groups and document its usage and limitations. Reason being that I don't want the public interface to change too much (if at all) when the internal implementation changes.

dwbutler avatar Jun 27 '16 20:06 dwbutler

@juhazi this is in #61

joelvh avatar Aug 10 '17 20:08 joelvh