search

citusdata / activerecord-multi-tenant

Rails/ActiveRecord support for distributed multi-tenant databases like Postgres+Citus
MIT License
718 stars 99 forks source link

Increase test coverage for delete_all when supplying an id instead of an object #232

Open reidmorrison opened 7 months ago

reidmorrison commented 7 months ago

This PR reproduces an issue where MultiTenant.with(account) has different behavior from MultiTenant.with(account.id) when calling delete_all or update_all.

Is it expected for the behavior to differ, or should we go ahead and remove the following 2 lines in order to make the behavior consistent for delete_all and update_all? https://github.com/citusdata/activerecord-multi-tenant/blob/master/lib/activerecord-multi-tenant/relation_extension.rb#L8 And https://github.com/citusdata/activerecord-multi-tenant/blob/master/lib/activerecord-multi-tenant/relation_extension.rb#L20

reidmorrison commented 7 months ago

@microsoft-github-policy-service agree company="Salesloft"

reidmorrison commented 4 months ago

@reidmorrison please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Salesloft"

reidmorrison commented 5 days ago

The tests are now passing locally after these changes made in the PR were merged into master: https://github.com/citusdata/activerecord-multi-tenant/commit/20d09a83db160e40db8e526becf66f59ad65f9a7 Please consider merging this pull request which now only adds tests, since they now include test coverage for the scenario that was fixed.