mbleigh / acts-as-taggable-on

A tagging plugin for Rails applications that allows for custom tagging along dynamic contexts.
http://mbleigh.lighthouseapp.com/projects/10116-acts-as-taggable-on
MIT License
4.96k stars 1.19k forks source link

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

Closed jbescoyez closed 3 years ago

jbescoyez commented 3 years ago

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 👍 )

ngouy commented 3 years ago

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 commented 3 years ago

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

lunaru commented 3 years ago

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

lunaru commented 3 years ago

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 commented 3 years ago

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

ngouy commented 3 years ago

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

ngouy commented 3 years ago

@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 commented 3 years ago

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