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

Overlapping tenants #191

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

hazelsparrow
Copy link

@hazelsparrow hazelsparrow commented Nov 28, 2018

Problem

As it stands, we can only have one tenant per model. Sometimes a need arises to define multiple tenants owning the same model, which acts_as_tenant does not currently support. For example, Manager belongs to Project and Account, but only acts as tenant with Account; you can't set the current tenant to Project to limit your scope of managers to all managers belonging to the project.

Desired functionality

class Manager < ApplicationRecord
  acts_as_tenant :account
  acts_as_tenant :project
end

ActsAsTenant.with_tenant(Project.first) { Manager.all } # == "select * from managers where project_id=1"
ActsAsTenant.with_tenant(Account.first) { Manager.all } # == "select * from managers where account_id=1"

Changes introduced in this PR

This PR allows you to specify as many acts_as_tenant directives on the model as you want. If current tenant is of type Account, then only the acts_as_tenant :account will be used in the default scope / validation callbacks / etc. Similarly, if current tenant is Project, only the acts_as_tenant :project will take effect.

Backwards compatibility

This PR introduces a breaking change by removing some public API (ActsAsTenant.fkey, etc.) Since this API was not covered by the original test suite, I'm assuming it to be a non-documented API which was not supposed to be used directly by the users of this gem anyway. However, this may break things for some people, so we might want to put those back with a deprecation warning and remove it in the next major release of the gem. Obviously those functions are meaningless when using more than one type of tenant, so we can raise if that's the case. That way all current users of the gem will get deprecation warnings but won't have anything broken.

Included changes

This PR basically includes changes from #190, since without fixing the issue mentioned in that PR tests related to polymorphic tenants would not pass.

@borisd
Copy link

borisd commented Feb 17, 2020

Any chance to merge this?

@hazelsparrow
Copy link
Author

Any chance to merge this?

Just curious, what is your use case for overlapping tenants? Merging of this pr has been on my plate for quite some time, but I've been hesitant to do it given how little demand for this feature seems to be.

@borisd
Copy link

borisd commented Feb 26, 2020

In our case we have devices that both belong to a manufacturer and a client (each with their own login and management features).

@pldavid2
Copy link

pldavid2 commented Jul 4, 2020

Hello, any chance of having this merged? It's a really useful feature.

@yshmarov
Copy link
Contributor

yshmarov commented Sep 8, 2020

This looks more like a feature for your own fork, not general-purpose

@excid3 excid3 removed the request for review from ErwinM November 17, 2020 18:54
@excid3
Copy link
Collaborator

excid3 commented Nov 17, 2020

I'm undecided on this one without a good example.

class Manager < ApplicationRecord
  acts_as_tenant :account
  acts_as_tenant :project
end

In this example, Project is a global model that is not in the Account tenant as well?

@borisd
Copy link

borisd commented Nov 19, 2020

Perhaps the names here are misleading.

class  Project < ApplicationRecord do
  acts_as_tenant :client
  acts_as_tenant :freelancer
end

A client might have multiple projects in the platform, the same for a freelancer doing projects. Both different subsets of project.

ps. We are actively using this feature on a large project now

@hazelsparrow
Copy link
Author

In this example, Project is a global model that is not in the Account tenant as well?

it could be either way: it could be a global model, and it could be in the Account tenant. regardless, ActsAsTenant.with_tenant(Project.first) { Manager.all } returns all managers scoped to the given Project, whereas ActsAsTenant.with_tenant(Account.first) { Manager.all } returns all managers scoped to the given Account.

@yshmarov
Copy link
Contributor

@hazelsparrow

ActsAsTenant.with_tenant(Project.first) { Manager.all } returns all managers scoped to the given Project, whereas ActsAsTenant.with_tenant(Account.first) { Manager.all } returns all managers scoped to the given Account.

maybe I don't get it... why is the purpose of using ActsAsTenant here? Why not just belongs_to :project and do

Project.first.managers
Account.first.managers

@borisd so in your case, Client and Freelancer are global models...
And there can be such a scenario:

class  Project < ApplicationRecord do
  acts_as_tenant :client
  acts_as_tenant :freelancer
end
class  Task < ApplicationRecord do
  acts_as_tenant :freelancer
end
class  Charge < ApplicationRecord do
  acts_as_tenant :client
end

...And in different controller actions you set_current_tenant for either client or freelancer?


Just trying to understand in what scenarios this could be useful...


In my imagination, there is

  • schema/database multitenancy
  • row-based multitenancy with one global model

and this looks like some sort of deviation from multitenancy to build platform features.
Not bad (great coding actually), but IMHO not what the primary design is meant to be...

@borisd
Copy link

borisd commented Nov 19, 2020

We are running two different "sub sites" on the same database.

One group of controllers set tenant as Client while the other group as Freelancer.

Quite a number of different models belong have two inheritance columns.

With this system I am (99%) certain that a client or freelancer never see any data they have no access to. But there is still some common models between them.

Specifically the project (the most sensitive model) is correctly scoped away by tenancy instead of worrying to filter in the app.

@hazelsparrow
Copy link
Author

@yshmarov the purpose of using ActsAsTenant is the same as it is when there's only one tenant. I think @borisd has a point that the models used in the tests in this PR (Account, Manager, and Project) are a little confusing and do not serve the purpose of communicating the idea of it very well. There were chosen simply because they were existing models. Maybe a real example of how this feature is used will be more useful, so here it goes.

Here at Collage we've been using this feature for about 2 years now to support two types of users, or accounts: companies and brokers.

class Broker
  # global model
end

class Company
  acts_as_tenant(:broker)
end

class Employee
  acts_as_tenant(:company)
  acts_as_tenant(:broker)
end

When you log into Collage as a company, you can see all employees in your company, since Employee acts_as_tenant(:company). When you log into Collage as a broker, you can see employees from all the companies that belong to your brokerage, since Employee also acts_as_tenant(:broker). Regardless of which account is currently in use, everywhere in the code we can simply do

Employee.all

and rely on ActsAsTenant to correctly scope Employees to either company or broker, whichever is relevant at the moment.

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

Successfully merging this pull request may close these issues.

6 participants