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

Support multiple superclasses #98

Merged
merged 7 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,12 @@ gemspec

gem "activerecord", "~> #{ENV['RAILS_VERSION']}" if ENV["RAILS_VERSION"]
gem "activesupport", "~> #{ENV['RAILS_VERSION']}" if ENV["RAILS_VERSION"]

gem "bundler", "~> 2", :group => :development
gem "gc_ruboconfig", "~> 3.6.0", :group => :development
gem "rspec", "~> 3.9", :group => :development
gem "rspec-github", "~> 2.4.0", :group => :development
gem "yard", "~> 0.9.20", :group => :development

# For integration testing
gem "sqlite3", "~> 1.6.1", :group => :development
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,15 @@ Lint/DefineDeletionStrategy:
ModelSuperclass: Acme::Record
```

If your models use multiple superclasses, you can specify a list of superclasses in your `.rubocop.yml`. Note that you will have to specify `ApplicationRecord` explicitly in this list should you want to lint all models which inherit from `ApplicationRecord`.
```yml
Lint/DefineDeletionStrategy:
ModelSuperclass:
- Acme::Record
- UmbrellaCorp::Record
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope UmbrellaCorp get a high risk rating XD


```

## License & Contributing

* Anony is available as open source under the terms of the [MIT License](http://opensource.org/licenses/MIT).
Expand Down
9 changes: 0 additions & 9 deletions anony.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,6 @@ Gem::Specification.new do |spec|

spec.required_ruby_version = ">= 2.6"

spec.add_development_dependency "bundler", "~> 2"
spec.add_development_dependency "gc_ruboconfig", "~> 3.6.0"
spec.add_development_dependency "rspec", "~> 3.9"
spec.add_development_dependency "rspec-github", "~> 2.4.0"
spec.add_development_dependency "yard", "~> 0.9.20"

# For integration testing
spec.add_development_dependency "sqlite3", "~> 1.6.1"
Tabby marked this conversation as resolved.
Show resolved Hide resolved

spec.add_dependency "activerecord", ">= 5.2", "< 8"
spec.add_dependency "activesupport", ">= 5.2", "< 8"
spec.metadata["rubygems_mfa_required"] = "true"
Expand Down
4 changes: 2 additions & 2 deletions lib/anony/anonymisable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

require "active_support/core_ext/module/delegation"

require_relative "./not_anonymisable_exception"
require_relative "./strategies/overwrite"
require_relative "not_anonymisable_exception"
require_relative "strategies/overwrite"
require_relative "model_config"

module Anony
Expand Down
7 changes: 3 additions & 4 deletions lib/anony/cops/define_deletion_strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,16 @@ def on_class(node)
end

def model?(node)
return unless (superclass = node.children[1])

superclass.const_name == model_superclass_name
superclass = node.children[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider looking for all leaf nodes that include ApplicationRecord or ActiveRecord::Base in their hierarchy instead of just looking at immediate children?

Copy link
Contributor

Choose a reason for hiding this comment

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

So unfortunately it's not this straightforward because this is not a Ruby object, it's an AST node. As far as we could see this only has the context of the current file, so you'd have to do a bunch of finnagling to be able to look at the class hierarchy outside of that context.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 , happy to have another look into some ways around this if blocking though :~)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's probably fine. I guess we could potentially add a spec in payments-service to ensure all classes that inherit from ApplicationRecord and have children of their own are included in the ModelSuperclass list, but I think as @benk-gc says there's a limit to what we can check from a cop without a lot of extra work.

Maybe something that can be looked into another time. "Good enough" is better than not improving things because they're not yet perfect after all :)

model_superclass_name.include? superclass&.const_name
end

def class_name(node)
node.children[0].const_name
end

def model_superclass_name
cop_config["ModelSuperclass"] || "ApplicationRecord"
Array.wrap(cop_config["ModelSuperclass"] || "ApplicationRecord")
end
end
end
Expand Down
8 changes: 4 additions & 4 deletions lib/anony/model_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

require "active_support/core_ext/module/delegation"

require_relative "./strategies/destroy"
require_relative "./strategies/overwrite"
require_relative "./selectors"
require_relative "strategies/destroy"
require_relative "strategies/overwrite"
require_relative "selectors"

module Anony
class ModelConfig
Expand Down Expand Up @@ -108,7 +108,7 @@ def selectors(&block)
end

def selector_for?(subject)
return nil if @selectors_config.nil?
return false if @selectors_config.nil?

@selectors_config.selectors[subject].present?
end
Expand Down
2 changes: 1 addition & 1 deletion lib/anony/selectors.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

require_relative "./selector_not_found_exception"
require_relative "selector_not_found_exception"

module Anony
class Selectors
Expand Down
2 changes: 1 addition & 1 deletion lib/anony/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module Anony
VERSION = "1.1.0"
VERSION = "1.2.0"
end
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,48 @@ class Employee < Acme::Record
it_behaves_like "an offense"
end
end

context "when it uses multiple super classes" do
subject(:offenses) { cop.offenses }

let(:cop_config) { { "ModelSuperclass" => ["Acme::Record", "Another::Record"] } }

context "when models defines anonymisation rules" do
let(:source) do
<<~RUBY
class Employee < Acme::Record
anonymise do
destroy
end
end

class Boss < Another::Record
anonymise do
destroy
end
end
RUBY
end

it { expect(cop.offenses).to be_empty }
end

context "when models are missing anonymisation rules" do
let(:source) do
<<~RUBY
class Employee < Another::Record
end

class Boss < Another::Record
end
RUBY
end

it { expect(offenses.count).to eq(2) }

it "has the correct name" do
expect(offenses.first.cop_name).to eq(cop.name)
end
end
end
end