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

Users/amit909sin/issue 195 #219

Merged

Conversation

Amit909Singh
Copy link
Contributor

Merging changes from master and fixing tenant scoping for delete_all

gurkanindibay and others added 17 commits June 2, 2023 16:51
* Wrap ActiveRecord::Base with ActiveSupport.on_load

* Test inspect method filters senstive column values

* Remove set secret_token and secret_key_base in spec_helper.rb

* Comment reason of filter_parameters in spec_helper.rb
* Adds Changelog entries for 2.3.0

* Updates version into 2.3.0 in version file

* Adds minimum coverage as 80 to be able to change version
* fix logic inside the tenant_klass_defined method

* add tests for tenant_klass_defined?

* fix failed tests

* rubocop -a

* remove unnecessary word

* add a test case for the following
citusdata#105

* test a more appropriate class

* add multi_tenant

* Fixes rubocop warnings
---------

Co-authored-by: Gürkan İndibay <gindibay@microsoft.com>
Bumps [certifi](https://github.com/certifi/python-certifi) from 2023.5.7 to 2023.7.22.
- [Commits](certifi/python-certifi@2023.05.07...2023.07.22)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Add rails 7.1.0.beta1

* Support Rails7.1

The `table_name` method was defined in `Arel::Nodes::Table` and `Arel::Nodes::TableAlias` to get the table name, but `Arel::Nodes::Table#table_name` was removed  rails/rails#46864.
Therefore, it is no longer possible to simply call `#table_name` to get `table_name`, so a `TableNode.table_name` has been added to get table_name from node.
Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.0.2 to 2.0.6.
- [Release notes](https://github.com/urllib3/urllib3/releases)
- [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst)
- [Commits](urllib3/urllib3@2.0.2...2.0.6)

---
updated-dependencies:
- dependency-name: urllib3
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.0.6 to 2.0.7.
- [Release notes](https://github.com/urllib3/urllib3/releases)
- [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst)
- [Commits](urllib3/urllib3@2.0.6...2.0.7)

---
updated-dependencies:
- dependency-name: urllib3
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Amit909Singh
Copy link
Contributor Author

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

@Amit909Singh 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="Microsoft"

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (issue_195@30accb5). Click here to learn what that means.

Additional details and impacted files
@@             Coverage Diff              @@
##             issue_195     #219   +/-   ##
============================================
  Coverage             ?   84.04%           
============================================
  Files                ?       16           
  Lines                ?      746           
  Branches             ?        0           
============================================
  Hits                 ?      627           
  Misses               ?      119           
  Partials             ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Amit909Singh
Copy link
Contributor Author

@serprex One of the checks is failing Rubocop on the file model_extension.rb. I didn't touch this file. Should I fix it, in order to get the checks, pass successfully. Also, I am currently pushing my changes to issue 195 branch. Please let me know who will help to get that branch merge to master?

@serprex
Copy link
Collaborator

serprex commented Dec 1, 2023

Yes, please fix. @gurkanindibay would merge; I no longer work at Microsoft

@@ -1,4 +1,5 @@
# activerecord-multi-tenant [![Build Status](https://github.com/citusdata/activerecord-multi-tenant/actions/workflows/active-record-multi-tenant-tests.yml/badge.svg)](https://github.com/citusdata/activerecord-multi-tenant/actions/workflows/active-record-multi-tenant-tests.yml) [![codecov](https://codecov.io/gh/citusdata/activerecord-multi-tenant/branch/master/graph/badge.svg?token=rw0TsEk4Ld)](https://codecov.io/gh/citusdata/activerecord-multi-tenant) [ ![Gems Version](https://img.shields.io/gem/v/activerecord-multi-tenant.svg)](https://rubygems.org/gems/activerecord-multi-tenant)[ ![Gem Download Count](https://img.shields.io/gem/dt/activerecord-multi-tenant.svg)](https://rubygems.org/gems/activerecord-multi-tenant) [![Documentation Status](https://readthedocs.org/projects/activerecord-multi-tenant/badge/?version=latest)](https://activerecord-multi-tenant.readthedocs.io/en/latest/?badge=latest)
# activerecord-multi-tenant
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there are multiple empty lines here?

execute "ALTER TABLE #{table_name} ADD PRIMARY KEY(\"#{options[:partition_key]}\", id)"
end
ret
module MultiTenant
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't understand why this change is required. Could you clarify?

@gurkanindibay
Copy link
Contributor

@Amit909Singh Great work! I appreciate your contribution. However, I would like additional clarification in the code to better grasp the rationale behind each modification. It seems you've made some valuable style and code enhancements, although they may not be directly tied to the issue at hand. While this is positive, I find it challenging to comprehend each specific change in this pull request. Providing more details will enable me to thoroughly review the issue. Thank you once again for your excellent effort.

@Amit909Singh
Copy link
Contributor Author

@Amit909Singh Great work! I appreciate your contribution. However, I would like additional clarification in the code to better grasp the rationale behind each modification. It seems you've made some valuable style and code enhancements, although they may not be directly tied to the issue at hand. While this is positive, I find it challenging to comprehend each specific change in this pull request. Providing more details will enable me to thoroughly review the issue. Thank you once again for your excellent effort.

Hi @gurkanindibay, I found your draft PR to address delete_all issue. I fork from your branch, but it was couple of months old and merge latest changes from the master. My changes are under my commits. Main changes are in delete_operations.rb
query_rewriter_spec.rb.
This PR is to push the changes to your branch.

@gurkanindibay gurkanindibay merged commit fc7ad0d into citusdata:issue_195 Dec 3, 2023
77 checks passed
gurkanindibay pushed a commit that referenced this pull request Dec 3, 2023
* Fix the tenant scoping for delete_all
gurkanindibay added a commit that referenced this pull request Dec 4, 2023
… incorrect query (#200)

* Adds initial implementation but still failing

* Users/amit909sin/issue 195 (#219)

* Fix the tenant scoping for delete_all

---------

Co-authored-by: amit909singh <amit909singh@gmail.com>
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

8 participants