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

Replace RequestStore dependency with CurrentAttributes #313

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

excid3
Copy link
Collaborator

@excid3 excid3 commented Jun 15, 2023

This simplifies ActsAsTenant by dropping the RequestStore dependency. We can use Rails' built-in CurrentAttributes feature that accomplishes the same thing.

CurrentAttributes was introduced in Rails 5.2 and we already require 5.2 or higher, so this should not be a breaking change. While this should be a drop-in replacement, it'd be nice to have this tested in several applications before releasing the change. If you have a chance to test this in your application(s), please do!

The only minor gripe I know of is that CurrentAttributes doesn't play well with the web-console gem because those requests are not in the same thread.

Copy link
Contributor

@nunommc nunommc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the gemfile.lock files have to reflect this change

acts_as_tenant.gemspec Show resolved Hide resolved
Copy link

@mybuddyandrew mybuddyandrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@tmaier
Copy link
Contributor

tmaier commented Aug 4, 2023

Great idea. Is it possible to go farther?

I am considering to get rid of the classic “current_user” and “current_tenant” methods in my code and to introduce my own Current class instead. I thinks such an would reduce even more complexities at this gem.

However, everyone would be forced to set this up themselves, when they use the gem and acts_as_tenant would need to know the name of the Current class the individual app uses and how the attributes are called…

@excid3
Copy link
Collaborator Author

excid3 commented Aug 4, 2023

However, everyone would be forced to set this up themselves, when they use the gem and acts_as_tenant would need to know the name of the Current class the individual app uses and how the attributes are called…

Users of acts_as_tenant would still use ActsAsTenant.current_tenant. This only refactors the internal storage.

@pond
Copy link

pond commented Dec 6, 2023

I just wrote my own version of this several months later for the same reason (we finally got around to moving our Rails app over from RequestStore). In particular, if you're using AAT & CurrentAttributes, the reset behaviour in test mode isn't the same and this was actually giving us compatibility headaches.

I'd really like to see this merged ASAP! In the mean time I'm monkey patching :-/

@excid3 One note - you don't need your inner Current class to include attribute test_tenant. It's not used.

lib/acts_as_tenant.rb Outdated Show resolved Hide resolved
@excid3 excid3 merged commit 0497b29 into master Dec 6, 2023
3 checks passed
@excid3 excid3 deleted the current-attributes branch December 6, 2023 21:51
@pond
Copy link

pond commented Dec 7, 2023

🎉 Thank you! 😄

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.

5 participants