Support tenants for taggings

[lunaru]

@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]

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]

@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]

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

[hengwoon]

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

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

[lunaru]

@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]

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

[johnisom]

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

[jeanschdev]

Hey this is great, any plan to merge?

[apuntovanini]

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]

@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]

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

[jbescoyez]

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

[seuros]

Okay

[seuros]

voila, released

[jbescoyez]

Thank you :) I wrote a bit too fast => #1036 (minor version bump soon)

[ngouy]

#1036 is fixed (or at least PR in review #1037) but raises 2 new issues:

• the migration is not rollbackable (PR ready for review)
  fix non rollbackable tenant migration #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?