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

Support for deferred scope apply for Models booted before Tenant setup #66

Merged
merged 3 commits into from
Aug 6, 2017

Conversation

renanwilliam
Copy link
Contributor

Solution provided by pimski for deferred scope apply to models booted before Tenants setup.

@ryross
Copy link

ryross commented Jul 13, 2017

Any chance we can get this merged or reviewed?

@ryross
Copy link

ryross commented Jul 13, 2017

I was actually hoping this solved the issue where applying the tenant scope in a group middleware doesn't work, but that isn't the case with this PR

@ryross
Copy link

ryross commented Jul 13, 2017

Nevermind on my last comment. I missed the part where I need to run Landlord::applyTenantScopesToDeferredModels(); after the Landlord::addTenant() call. Works for me! Thanks @renanwilliam and @pimski

@ryross
Copy link

ryross commented Jul 21, 2017

@renanwilliam would you mind explaining what the 2nd and 3rd commits fix? I have two tenant scopes and the first() logic is incorrectly choosing which tenant scope to apply

@renanwilliam
Copy link
Contributor Author

@ryross it's for the cases when I change the tenant before a scope is applied.

@hipsterjazzbo hipsterjazzbo merged commit cdb0390 into hipsterjazzbo:master Aug 6, 2017
@ryross
Copy link

ryross commented Aug 7, 2017

@renanwilliam You might want to find a different way to do that. Your logic breaks the code for folks who have two tenants. It resets the ID. @hipsterjazzbo Did you test this?

@renanwilliam
Copy link
Contributor Author

Hi @ryross ,
I have executed all previous test cases and the new test case write for the change and it not happens, no problems for me (we are also using it in a production project with 5 tenants and everything looks fine). Do you have a test case or instructions do check this error?

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.

3 participants