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

Allow to Change Tenant on Models Using New with_mutable_tenant Block #230

Merged
merged 7 commits into from
Dec 17, 2022

Conversation

patrotom
Copy link
Contributor

@patrotom patrotom commented Apr 7, 2020

I took the code from the fork from this PR: #210 I added some notes about this feature to README.md.

I want to use this new feature, but honestly, the PR #210 looks dead, so I decided to fork the repository again, take the code from https://github.com/ksouthworth/acts_as_tenant and add the required notes to README.md.

All credits for the code in this PR go to ksouthworth.

@excid3
Copy link
Collaborator

excid3 commented Nov 17, 2020

I feel like the name of this is a little confusing. without_tenant to me just feels like it would set tenant to nil and nothing more.

What if we called it allow_tenant_changes or something similar?

@patrotom
Copy link
Contributor Author

patrotom commented Dec 1, 2020

I feel like the name of this is a little confusing. without_tenant to me just feels like it would set tenant to nil and nothing more.

What if we called it allow_tenant_changes or something similar?

Actually not a bad idea. allow_tenant_changes sounds good. I was thinking we could use something like with_tenant_modifications. I am quite fond of the with/without prefix. What do you think?

@excid3
Copy link
Collaborator

excid3 commented Dec 1, 2020

Maybe ActsAsTenant.with_mutable_tenant ? We're already using that wording internally, and I think that makes it pretty clear for users this will temporarily allow mutation of the tenant for a record.

@patrotom
Copy link
Contributor Author

patrotom commented Dec 2, 2020

Maybe ActsAsTenant.with_mutable_tenant ? We're already using that wording internally, and I think that makes it pretty clear for users this will temporarily allow mutation of the tenant for a record.

I agree. This is probably the best name for it so far. I will adjust the PR.

@excid3
Copy link
Collaborator

excid3 commented Dec 2, 2020

Thanks @patrotom 👍

That's probably the last PR I merge before I release 1.0! 🎉

@patrotom
Copy link
Contributor Author

Alright, I rewrote the code and changed without_tenant! to with_mutable_tenant. I also changed the structure of the added code based on the master.

I apologize for the delay. I hope we can close this PR as soon as possible. :-)

@excid3 can you, please, to the code review?

Thanks.

@patrotom patrotom changed the title Allow to Change Tenant on Models Using New without_tenant! Block Allow to Change Tenant on Models Using New with_mutable_tenant Block Mar 23, 2021
@Ancez
Copy link

Ancez commented Sep 29, 2021

Can we get an update on this? Is there anything I can help with to get this merged down?

@patrotom
Copy link
Contributor Author

patrotom commented Oct 3, 2021

Can we get an update on this? Is there anything I can help with to get this merged down?

Hopefully, someone who can merge will look at it soon...

@Merovex
Copy link

Merovex commented Dec 17, 2022

@excid3 could this PR get a little love? It looks like it's been ready for a couple years. Otherwise, I'll have to drop back to belongs_to for a few models as I have records I want to transfer between owners.

@excid3
Copy link
Collaborator

excid3 commented Dec 17, 2022

It has conflicts that need to be resolved, but once those are done we can merge it. If I have time today, I will try.

Had a baby recently so it's a bit hard to keep up with everything at the moment. 👶

@excid3 excid3 merged commit 80575a3 into ErwinM:master Dec 17, 2022
@Merovex
Copy link

Merovex commented Dec 18, 2022

Congratulations on the baby. A different sort of commit...poop requests, etc.

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.

4 participants