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

Models won't save if they already have a belongs_to :tenant and acts_as_taggable_tenant is used #1036

Closed
jbescoyez opened this issue Jun 7, 2021 · 8 comments

Comments

@jbescoyez
Copy link

How to reproduce:

class Tenant < ApplicationRecord
  has_many :users
end

class User < ApplicationRecord
  belongs_to :tenant
  acts_as_taggable_on :tags
  acts_as_taggable_tenant :tenant_id
end

Tenant.create!

Expected behavior

The tenant is created

Actual behavior

The following error is thrown.

NoMethodError - undefined method `marked_for_destruction?' for ID:Integer

Investigation

This is due to: https://github.com/mbleigh/acts-as-taggable-on/pull/1000/files#diff-f3570227aa94bb5887e3cb70c6a69fb30861dbecaa1ee498153371a1e10fc314R217

This line overrides the tenant method and returns an integer instead of an object.
This can be easily fixed by namescoping the method (e.g. taggable_tenant_id).

cc @lunaru (BTW, thank you for your work 👍 )

@jbescoyez jbescoyez changed the title Models won't save if they already have a belgons_to :tenant and acts_as_taggable_tenant is used Models won't save if they already have a belongs_to :tenant and acts_as_taggable_tenant is used Jun 7, 2021
@ngouy
Copy link
Contributor

ngouy commented Jun 15, 2021

same here, tenant is already an existing thing in our data modelisation, and we3 can't use the acts_as_taggable_tenant of the 8.0.0, it just breaks everything

@ngouy
Copy link
Contributor

ngouy commented Jun 15, 2021

@lunaru a solution could be to rename tenants to less generic/more taggable specific term such as "taggable_tenant"

@lunaru
Copy link
Contributor

lunaru commented Jun 16, 2021

@jbescoyez @ngouy namespacing this field (taggable_tenant and taggable_tenant_id) makes sense. PRs welcome!

@lunaru
Copy link
Contributor

lunaru commented Jun 16, 2021

Please note that method-level changes would be preferred since that would remove the need to run a migration for the tenant column in the taggings table

@ngouy
Copy link
Contributor

ngouy commented Jun 16, 2021

From a db perspective indeed we don't need any updates. Will check it out when I have time

ngouy added a commit to ngouy/acts-as-taggable-on that referenced this issue Jun 17, 2021
ngouy added a commit to ngouy/acts-as-taggable-on that referenced this issue Jun 17, 2021
@seuros seuros closed this as completed in e2d211b Jun 17, 2021
@ngouy
Copy link
Contributor

ngouy commented Jun 17, 2021

@jbescoyez once new version is out check it out and tell me if it works for you. It worked here

@ngouy
Copy link
Contributor

ngouy commented Jun 17, 2021

@jbescoyez @ngouy namespacing this field (taggable_tenant and taggable_tenant_id) makes sense. PRs welcome!

I didn't rename tenant_id since it's only internal logic, taggable models didn't inherit this attribute

@jbescoyez
Copy link
Author

@ngouy I finally moved on without this gem. However, your PR is exactly the approach I would have taken. Tnx!

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

No branches or pull requests

3 participants