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

failed to add a named group #65

Open
kainio opened this issue Jul 12, 2017 · 14 comments
Open

failed to add a named group #65

kainio opened this issue Jul 12, 2017 · 14 comments
Labels

Comments

@kainio
Copy link

kainio commented Jul 12, 2017

After doing
user = User.first
user.named_groups << :admin

I got
(0.3ms) ROLLBACK
ActiveRecord::RecordInvalid: Validation failed: Group must exist

Do I have to create the group first ? how?

@dwbutler
Copy link
Owner

Can I see your full Groupify configuration in your models? I suspect that something isn't set up correctly.

@kainio
Copy link
Author

kainio commented Jul 13, 2017

Here's my configuration:
app/models/organization.rb

class Organization < ApplicationRecord
  groupify :group
end

app/models/organization_membership.rb

class OrganizationMembership < ApplicationRecord
    groupify :group_membership
end

app/models/user.rb

class User < ApplicationRecord
  groupify :group_member
  groupify :named_group_member
end

config/initializers/groupify.rb

Groupify.configure do |config|
  config.group_class_name = 'Organization'
  config.group_membership_class_name = 'OrganizationMembership'
end

The migration

class GroupifyMigration < ActiveRecord::Migration[5.1]
  def change
    change_table :organizations do |t|
      t.string     :type
    end

    create_table :organization_memberships do |t|
      t.references :member, polymorphic: true, index: true, null: false
      t.references :group, polymorphic: true, index: { name: "index_org_membership_on_org_type_and_org_id" }

      # The named group to which a member belongs (if using)
      t.string     :group_name, index: true

      # The membership type the member belongs with
      t.string     :membership_type

      t.timestamps
    end
  end
end

and here is the full error

   (17.8ms)  SELECT `organization_memberships`.`group_name` FROM `organization_memberships` WHERE `organization_memberships`.`member_id` = 2 AND `organization_memberships`.`member_type` = 'User' AND (group_name IS NOT NULL)
   (0.4ms)  BEGIN
  OrganizationMembership Load (0.7ms)  SELECT  `organization_memberships`.* FROM `organization_memberships` WHERE `organization_memberships`.`member_id` = 2 AND `organization_memberships`.`member_type` = 'User' AND `organization_memberships`.`group_name` = 'admin' AND `organization_memberships`.`membership_type` IS NULL ORDER BY `organization_memberships`.`id` ASC LIMIT 1
  User Load (0.6ms)  SELECT  `users`.* FROM `users` WHERE `users`.`id` = 2 LIMIT 1
   (0.3ms)  ROLLBACK
ActiveRecord::RecordInvalid: Validation failed: Group must exist

@dwbutler
Copy link
Owner

You've defined User to be a group member by calling groupify :group_member, but there is no Group model defined. Try removing that line if you're planning to only used named groups and not groups.

@kainio
Copy link
Author

kainio commented Aug 1, 2017

This means i can't use them both, groups and named groups ?
Because I am using also groups as organisations.

@reentim
Copy link

reentim commented Aug 2, 2017

I'm having the same issue — here's a sample repo https://github.com/reentim/groupify_example, which only calls groupify :named_group_member, but still runs into ActiveRecord::RecordInvalid: Validation failed: Group must exist when adding a named group.

@joelvh
Copy link
Collaborator

joelvh commented Aug 4, 2017

@reentim @kainio Do you get this error with the updates from #61? I don't, but let me know to determine if it's a configuration or groupify internals issue. Thanks.

@dwbutler
Copy link
Owner

dwbutler commented Aug 4, 2017

After some investigation, I believe the issue is caused by this change in Rails 5: http://blog.bigbinary.com/2016/02/15/rails-5-makes-belong-to-association-required-by-default.html

In Rails 5, belongs_to associations are now required by default. Groupify's group membership model defines belongs_to :group, which means that group is now required whether you want it to be or not.

In Rails 5, you can add optional: true to the association to make it optional. But I'm not sure if doing this will break Rails 4 support. Will do some more testing.

@dwbutler
Copy link
Owner

dwbutler commented Aug 4, 2017

I verified that adding optional: true to the association fixes the issue in Rails 5. But under Rails 4, I get Unknown key: :optional 👎 😢

@dwbutler
Copy link
Owner

dwbutler commented Aug 4, 2017

I was able to fix it in both Rails 4 and 5 by adding required: false 🤕

dwbutler added a commit that referenced this issue Aug 4, 2017
Rails 5 made `belongs_to` associations required by default. When using named groups, the group association is expected to be empty. This fixes the issue in a Rails 4 compatible way by adding `required: false`

Fixes #65
@dwbutler
Copy link
Owner

dwbutler commented Aug 4, 2017

@reentim @kainio See if the above commit fixes the issue.

@dwbutler dwbutler added bug and removed support labels Aug 4, 2017
@dwbutler
Copy link
Owner

dwbutler commented Aug 4, 2017

The Travis build results are in, and this fix doesn't work in Rails 4.0 or 4.1 since they don't support the required configuration key.

The best approach will probably be to have split configuration based on the Rails version as a short term fix. And then in the next major release of Groupify we'll drop support for Rails < 4.2.

@tanega
Copy link

tanega commented Aug 4, 2017

Thanks @dwbutler ! Same as @reentim and @kainio I had the error message ActiveRecord::RecordInvalid: Validation failed: Group must exist when trying to add User to a named group (default groupify configuration on Rails 5.1). Switching to the fix/rails5-named-group branch fix the issue for me.

@kainio
Copy link
Author

kainio commented Aug 5, 2017

I confirm what @gaetan-pc said switching to the fix/rails5-named-groups fixed the problem.
Maybe it should be merge to master.

@joelvh
Copy link
Collaborator

joelvh commented Sep 7, 2017

@kainio @gaetan-pc @reentim #61 drops support for Rails < 4.2 and this fix has been merged. If you could, please let us know how the refactor fares in your applications. Thanks!

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

5 participants