diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 1c0e9d7211801321dd95f412ef3f68dab28b51da..6861a04ffc6ca5c3b1f89413e25cd77d049c0c0c 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -67,9 +67,7 @@ end return unless helper.ci? return if helper.mr_labels.include?(DATABASE_APPROVED_LABEL) -db_paths_to_review = helper.changes_by_category[:database] - -if helper.mr_labels.include?('database') || db_paths_to_review.any? +if helper.mr_labels.include?('database') || database.changes.any? message 'This merge request adds or changes files that require a ' \ 'review from the [Database team](https://gitlab.com/groups/gl-database/-/group_members).' diff --git a/spec/tooling/danger/database_spec.rb b/spec/tooling/danger/database_spec.rb index ddcfa279dc3cf9959af8ef74e2b75d57c8a262cf..a342014cf6bfc7e418028b39eb9b7eb91bd3a8ed 100644 --- a/spec/tooling/danger/database_spec.rb +++ b/spec/tooling/danger/database_spec.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require 'rspec-parameterized' require 'gitlab-dangerfiles' require 'danger' require 'danger/plugins/internal/helper' @@ -41,11 +42,98 @@ let(:cutoff) { Date.parse('2022-10-01') - 21 } - subject(:database) { fake_danger.new } + subject(:database) { fake_danger.new(helper: fake_helper) } describe '#find_migration_files_before' do it 'returns migrations that are before the cutoff' do expect(database.find_migration_files_before(migration_files, cutoff).length).to eq(8) end end + + describe '#changes' do + using RSpec::Parameterized::TableSyntax + + where do + { + 'with database changes to a migration file' => { + modified_files: %w[ + db/migrate/20230720114001_test_migration.rb + db/schema_migrations/20230720114001 + db/structure.sql + app/models/test.rb + ], + changed_lines: [], + changes_by_category: { + database: %w[ + db/migrate/20230720114001_test_migration.rb + db/schema_migrations/20230720114001 + db/structure.sql + ] + }, + impacted_files: %w[ + db/migrate/20230720114001_test_migration.rb + db/schema_migrations/20230720114001 + db/structure.sql + ] + }, + 'with non-database changes' => { + modified_files: %w[ + app/models/test.rb + ], + changed_lines: %w[ + +# Comment explaining scope :blah + ], + changes_by_category: { + database: [] + }, + impacted_files: [] + }, + 'with database changes in a doc' => { + modified_files: %w[doc/development/database/test.md], + changed_lines: [ + '+scope :blah, ->() { where(hidden: false) }' + ], + changes_by_category: { + database: [] + }, + impacted_files: [] + }, + 'with database changes in a model' => { + modified_files: %w[app/models/test.rb], + changed_lines: [ + '+# Comment explaining scope :blah', + '+scope :blah, ->() { where(hidden: false) }' + ], + changes_by_category: { + database: [] + }, + impacted_files: %w[app/models/test.rb] + }, + 'with database changes in a concern' => { + modified_files: %w[app/models/concerns/test.rb], + changed_lines: [ + '- .where(hidden: false)', + '+ .where(hidden: true)' + ], + changes_by_category: { + database: [] + }, + impacted_files: %w[app/models/concerns/test.rb] + } + } + end + + with_them do + before do + allow(fake_helper).to receive(:modified_files).and_return(modified_files) + allow(fake_helper).to receive(:all_changed_files).and_return(modified_files) + allow(fake_helper).to receive(:changed_lines).and_return(changed_lines) + allow(fake_helper).to receive(:changes_by_category).and_return(changes_by_category) + end + + it 'returns database changes' do + expect(database.changes).to match impacted_files + end + end + end end diff --git a/tooling/danger/database.rb b/tooling/danger/database.rb index 4cfac7c4af44a04a1475d97557881bc792934247..71b3ed1a1a5d0f39a8187299e20551d2a106c886 100644 --- a/tooling/danger/database.rb +++ b/tooling/danger/database.rb @@ -5,6 +5,8 @@ module Danger module Database TIMESTAMP_MATCHER = /(?<timestamp>\d{14})/ MIGRATION_MATCHER = %r{\A(ee/)?db/(geo/)?(post_)?migrate/} + MODEL_PATHS = %r{\A(ee/)?app/models/} + MODEL_CHANGES = %r{^[^#\n]*?(?:scope :|where\(|joins\()} def find_migration_files_before(file_names, cutoff) migrations = file_names.select { |f| f.match?(MIGRATION_MATCHER) } @@ -15,6 +17,20 @@ def find_migration_files_before(file_names, cutoff) timestamp < cutoff end end + + def changes + changed_database_paths + changed_model_paths + end + + def changed_database_paths + helper.changes_by_category[:database] + end + + def changed_model_paths + helper.all_changed_files.grep(MODEL_PATHS).select do |file| + helper.changed_lines(file).any? { |change| change =~ MODEL_CHANGES } + end + end end end end