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.2k forks source link

Support tenants for taggings #1000

Closed lunaru closed 3 years ago

lunaru commented 4 years ago

@seuros Let me know if I should be tagging (pun intended) other contributors to get attention to this PR.

I'm opening up this PR to add tenant capabilities to acts-as-taggable-on. Specifically this would be helpful for multi-tenant applications that will want to use a tenant column to track ownership.

I know there's an owner concept already in this gem, but that doesn't handle the case where you want to track the user who tagged the object as well as the tenant_id for partitioning purposes.

We use this gem (via a fork right now) across millions of rows and this helps keep performance sane. However, we'd love to get this change merged back into the root repo for future compatibility. It's also being introduced here in a way that will not impact existing usage syntax.

Would love to get your thoughts on the acceptability on this idea.

seuros commented 4 years ago

Hi @lunaru

I this PR should be a documented opt in feature and not hardcoded into the code.

the developper will have to require a file before having the tenant feature. require "acts_as_taggable_on/tenant_column"

I dont want to cause issue with existing codebases since it contain a feature migration and will change the sql queries.

this feature will have a rake task to generate the migration.

lunaru commented 4 years ago

@seuros thanks for the review --

The code is actually already built so that anyone not explicitly asking for tenants (via call to acts_as_taggable_tenant {tenant_column}) will not have any queries altered. This is accomplished by checking against the existence of the tenant_column value before create. The only read-query that's new is for_tenant (and related scopes) and that will not be called by existing codebases anyway. So it's already an opt-in feature and it's fully transparent to anyone not using tenants.

As for the migration, because this gem abides by the Engines rules, the migrations will be generated. It would difficult to put it into a separate rake since the modus operandi of this gem is to utilize the Engine standard. However, I've now modified the README file to indicate that it's fully optional, so existing projects can completely ignore the generated migration. New projects can choose to keep it or discard it as they see fit.

See the latest commit at 9a09ecf which accommodates this transparency.

TL;DR: This PR will not impact existing production code. The new migration can be discarded with no problems.

Let me know what you think of this approach

seuros commented 4 years ago

That sound good . Let have some approval from the community for the PR and i will merge it.

hengwoon commented 4 years ago

👍 often times the owner of a tag has a parent owner, for example a Post can have tags, and the Post can belong to an Account, and being able to specify a tenant (Account) for querying will help performance. Will also help querying for all tags belonging to a particular Account.

pablo-co commented 4 years ago

👍 to this one! Was just about to monkey patch this library to add this feature.

lunaru commented 4 years ago

@pablo-co Glad you found this! Feel free to use the fork at https://github.com/reamaze/acts-as-taggable-on (branch name is tenants-2) if you need this immediately while this awaits merging

lunaru commented 4 years ago

@pablo-co @seuros Any further thoughts on this? Would love to see it get incorporated.

johnisom commented 3 years ago

@pablo-co @seuros @mbleigh This is a fantastic PR. Any plan to finish review and merge soon or any other thoughts?

jeanschdev commented 3 years ago

Hey this is great, any plan to merge?

apuntovanini commented 3 years ago

This is a fantastic feature, any plan to merge it soon? @lunaru is your fork safe for production usage (seems out of sync from mbleigh:master)? Thanks!

lunaru commented 3 years ago

@apuntovanini Yes it’s safe for production usage.

Why it hasn’t been merged after all this time is a mystery. It’s clearly a needed feature for any multi-tenant system and it had no negative implications for existing production deployments.

@pablo-co ping one more time.

seuros commented 3 years ago

@lunaru I will test it this weekend and have it merged. Sorry for delay.

jbescoyez commented 3 years ago

@seuros Wouldn't this PR deserve a release :) - Needing it for a production project 😇

seuros commented 3 years ago

Okay

seuros commented 3 years ago

voila, released

jbescoyez commented 3 years ago

Thank you :) I wrote a bit too fast => https://github.com/mbleigh/acts-as-taggable-on/issues/1036 (minor version bump soon)

ngouy commented 3 years ago

1037) but raises 2 new issues:

the migration is not rollbackable (PR ready for review) https://github.com/mbleigh/acts-as-taggable-on/pull/1038
this doesn't work for nested child models: https://github.com/mbleigh/acts-as-taggable-on/pull/1037/files#r653889768

if an account has users that have pets, pets has_one account through user. But I can't leverage acts_as_taggable_tenant on my pet records, because we explicitly use #read_attribute to retrieve the chosen tenant attribute, rather than just calling it on the taggable model, which prevents any non-direct tenant child to leverage this feature, even when you declare a method in your sub-child such as

def account_id
  user.account_id
end

Any particular reason for this?

softbrada commented 2 years ago

Are there any plans in making tenant as a polymorphic association? would help more with joining records than a simple string column