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

fix #1036 scope taggable#tenant method name to #taggable_tenant #1037

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

ngouy
Copy link
Contributor

@ngouy ngouy commented Jun 17, 2021

If your taggable model already have a #tenant attribute/method/relationship, it would broke your code.

A small modification is required to scope this (non-documented and normally non directly used) method to the taggable lexical scope to fix the issue

@@ -7,5 +7,6 @@ group :local_development do
gem 'guard-rspec'
gem 'appraisal'
gem 'rake'
gem 'sqlite3'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added that because it's a requirement when working on the gem

without it:

>$ bundle
...
>$ bundle exec rake rspec

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange , i never needed to add it. You can use appraisal to run a specific version.

@ngouy
Copy link
Contributor Author

ngouy commented Jun 17, 2021

tested on a real project with already existing tenants and it works (I pass from a broken app to a working app)

@@ -214,7 +214,7 @@ def tagging_contexts
self.class.tag_types.map(&:to_s) + custom_contexts
end

def tenant
def taggable_tenant
if self.class.tenant_column
read_attribute(self.class.tenant_column)
Copy link
Contributor Author

@ngouy ngouy Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaru small question: why did we specificly use #read_attribute?

If I have a taggable resource that is:

class MyTaggableModel
  has_one :tenant, through: :something
  
  acts_as_taggable_on :tags
  acts_as_taggable_tenant :tenant_id
  
  def tenant_id
    something.tenant_id
  end
end

It just doesn't works, and that's kinda unfortunate.

@seuros
Copy link
Collaborator

seuros commented Jun 17, 2021

Can you bump version 8.1 and update the Changelog ?

@ngouy
Copy link
Contributor Author

ngouy commented Jun 17, 2021

@seuros can we also address the #read_attribute issue before please ? really need that on my hand

If we have no specific reason for it's usage I can just use #public_send instead

@seuros
Copy link
Collaborator

seuros commented Jun 17, 2021

Sure thing

@ngouy ngouy force-pushed the scope-tenant-method-names-to-taggable branch from c043ab3 to 5dc7994 Compare June 17, 2021 20:56
@ngouy
Copy link
Contributor Author

ngouy commented Jun 17, 2021

@seuros we should be good, minor version bump and changelog updated included

@seuros seuros merged commit e2d211b into mbleigh:master Jun 17, 2021
@seuros
Copy link
Collaborator

seuros commented Jun 17, 2021

Awesome work

@ngouy
Copy link
Contributor Author

ngouy commented Jun 18, 2021

@seuros when do you think will you release it (no pressure just wondering in terms of planning)

@seuros
Copy link
Collaborator

seuros commented Jun 18, 2021

Tomorrow max

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

Successfully merging this pull request may close these issues.

2 participants