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

update logic inside the tenant_klass_defined? method #202

Merged
merged 16 commits into from
Jun 16, 2023

Conversation

msasaki666
Copy link
Contributor

I think options[:class_name] should also be a determining factor in tenant_klass_defined? method.

I think the following issue is related.
#105

@msasaki666
Copy link
Contributor Author

@microsoft-github-policy-service agree

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #202 (373b8b1) into master (fd42dbd) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   82.61%   82.68%   +0.07%     
==========================================
  Files          14       14              
  Lines         719      722       +3     
==========================================
+ Hits          594      597       +3     
  Misses        125      125              
Impacted Files Coverage Δ
lib/activerecord-multi-tenant/model_extensions.rb 96.51% <100.00%> (ø)
lib/activerecord-multi-tenant/multi_tenant.rb 88.15% <100.00%> (+0.48%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gurkanindibay
Copy link
Contributor

Great solution @msasaki666 . The solution is like a charm.
Thank you to add the unit test to cover the cases.
In order to see if this solves #105, a test case is needed that covers this case.
Could you add a test to cover #105.

Thanks in advance for your great effort to make activerecord-multi-tenant better

Copy link
Contributor

@gurkanindibay gurkanindibay left a comment

Choose a reason for hiding this comment

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

Thanks

@msasaki666
Copy link
Contributor Author

Great solution @msasaki666 . The solution is like a charm. Thank you to add the unit test to cover the cases. In order to see if this solves #105, a test case is needed that covers this case. Could you add a test to cover #105.

Thanks in advance for your great effort to make activerecord-multi-tenant better

Sure.
I'll add it later.

@msasaki666
Copy link
Contributor Author

msasaki666 commented Jun 15, 2023

Great solution @msasaki666 . The solution is like a charm. Thank you to add the unit test to cover the cases. In order to see if this solves #105, a test case is needed that covers this case. Could you add a test to cover #105.

Thanks in advance for your great effort to make activerecord-multi-tenant better

@gurkanindibay
I have added it.

@gurkanindibay
Copy link
Contributor

gurkanindibay commented Jun 15, 2023

Thanks @msasaki666
Could you add a case as below as defined in #105

module MyModule
  class Tenant < ApplicationRecord; end

  class Foo < ApplicationRecord
    multi_tenant :tenant
  end
end

@msasaki666
Copy link
Contributor Author

msasaki666 commented Jun 16, 2023

@gurkanindibay
Sure.
I have updated it.
I think all of tenant classes are subclasses of ActiveRecord::Base.
So, I also modify other tests.
I can't use ApplicationRecord class in tests, so I use ActiveRecord::Base class instead.

@gurkanindibay gurkanindibay merged commit bed29b1 into citusdata:master Jun 16, 2023
41 checks passed
@gurkanindibay
Copy link
Contributor

Thanks @msasaki666 now code is in master branch. Thank you for all of your efforts

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.

None yet

3 participants