Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inverse of has_members missing as a feature? #47

Open
juhazi opened this issue Jun 13, 2016 · 8 comments
Open

Inverse of has_members missing as a feature? #47

juhazi opened this issue Jun 13, 2016 · 8 comments
Labels

Comments

@juhazi
Copy link
Contributor

juhazi commented Jun 13, 2016

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.

@dwbutler
Copy link
Owner

dwbutler commented Jun 13, 2016

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.

@juhazi
Copy link
Contributor Author

juhazi commented Jun 14, 2016

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)

@dwbutler
Copy link
Owner

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'.

juhazi added a commit to juhazi/groupify that referenced this issue Jun 15, 2016
Provide a helper for member classes to create associations scoped to
certain subclasses of the group class.

Having `has_groups :group_subtypes` creates an association
`has_many :group_subtypes, through: :group_memberships_as_member`.

Differs in exposed interface from mongoid adapter.

Issue: dwbutler#47
( dwbutler#47 )
@juhazi
Copy link
Contributor Author

juhazi commented Jun 15, 2016

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.

@dwbutler
Copy link
Owner

@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.

@juhazi
Copy link
Contributor Author

juhazi commented Jun 27, 2016

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.

@dwbutler
Copy link
Owner

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.

@joelvh
Copy link
Collaborator

joelvh commented Aug 10, 2017

@juhazi this is in #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants