ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

bump rubocop-rails 

I was able to replace one of our custom cops with a built in one
(just had to make it ignore older migrations).

Then I had to manually fix a couple of (important!) offenses

Change-Id: I000310bb6b065034384ba3a33ef5e37e22b9be5a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/315855
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
Build-Review: Jacob Burroughs <jburroughs@instructure.com>
Build-Review: Cody Cutrer <cody@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
This commit is contained in:
Cody Cutrer 2023-04-12 15:23:39 -06:00
parent 7c37aa5f64
commit 7144f0c66a
21 changed files with 52 additions and 251 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
.rubocop.yml
View File

@ -119,10 +119,18 @@ Rails/HasManyOrHasOneDependent:
Enabled: false # legacy code + most things we soft delete anyway
Rails/HelperInstanceVariable:
Enabled: false # legacy code
Rails/I18nLocaleTexts:
Exclude:
- spec/**/*.rb
- "**/spec_canvas/**/*.rb"
Rails/NegateInclude:
Enabled: false # exclude? isn't as intuitive as !include?
Rails/Present:
UnlessBlank: false # allow `unless foo.blank?`
Rails/RedundantPresenceValidationOnBelongsTo:
Enabled: false # we do _not_ have config.active_record.belongs_to_required_by_default set
# even though it's been a default since Rails 5.0, we don't have a
# config.load_defaults line
Rails/SkipsModelValidations:
Enabled: false # Canvas skips validations in many places for optimization reasons
Rails/WhereExists:
@ -275,11 +283,20 @@ Naming/VariableName:
Naming/VariableNumber:
Enabled: false
Rails/ActionControllerFlashBeforeRender:
Severity: convention
AutoCorrect: false
Rails/ActionOrder:
Severity: convention
AutoCorrect: false
Rails/ActiveRecordCallbacksOrder:
Severity: convention
AutoCorrect: false
Rails/ActiveRecordOverride:
Severity: convention
Rails/ActiveSupportOnLoad:
Severity: convention
AutoCorrect: false
Rails/ApplicationMailer:
Severity: convention
AutoCorrect: false
@ -293,6 +310,10 @@ Rails/CreateTableWithTimestamps:
Rails/Date:
Enabled: false
AutoCorrect: false
Rails/DeprecatedActiveModelErrorsMethods:
AutoCorrect: false
Rails/DuplicateAssociation:
AutoCorrect: false
Rails/EagerEvaluationLogMessage:
Severity: convention
AutoCorrect: false

2
Gemfile.d/rubocop.rb
View File

@ -33,7 +33,7 @@ group :test do
gem "rubocop", "1.49.0", require: false
gem "rubocop-canvas", require: false, path: "gems/rubocop-canvas"
gem "rubocop-performance", "1.17.1", require: false
gem "rubocop-rails", "2.12.4", require: false
gem "rubocop-rails", "2.19", require: false
gem "rubocop-rake", "0.6.0", require: false
gem "rubocop-rspec", "2.19.0", require: false
end

7
Gemfile.rails70.lock.partial
View File

@ -148,6 +148,7 @@ PATH
jira_ref_parser (= 1.0.1)
outrigger (~> 3.0)
rubocop (~> 1.19)
rubocop-rails (~> 2.19)
PATH
remote: gems
@ -979,10 +980,10 @@ GEM
rubocop-performance (1.17.1)
rubocop (>= 1.7.0, < 2.0)
rubocop-ast (>= 0.4.0)
rubocop-rails (2.12.4)
rubocop-rails (2.19.0)
activesupport (>= 4.2.0)
rack (>= 1.1)
rubocop (>= 1.7.0, < 2.0)
rubocop (>= 1.33.0, < 2.0)
rubocop-rake (0.6.0)
rubocop (~> 1.0)
rubocop-rspec (2.19.0)
@ -1360,7 +1361,7 @@ DEPENDENCIES
rubocop (= 1.49.0)
rubocop-canvas!
rubocop-performance (= 1.17.1)
rubocop-rails (= 2.12.4)
rubocop-rails (= 2.19)
rubocop-rake (= 0.6.0)
rubocop-rspec (= 2.19.0)
ruby-duration (= 3.2.3)

6
app/controllers/live_assessments/assessments_controller.rb
View File

@ -80,11 +80,13 @@ module LiveAssessments
@assessments = []
if params[:assessments].any? { |assessment_hash| assessment_hash.dig(:links, :outcome) }
return unless authorized_action(@context, @current_user, :manage_outcomes)
end
Assessment.transaction do
params[:assessments].each do |assessment_hash|
if (outcome_id = assessment_hash.dig(:links, :outcome))
return unless authorized_action(@context, @current_user, :manage_outcomes)
@outcome = @context.linked_learning_outcomes.where(id: outcome_id).first
reject! "outcome must be linked to the context" unless @outcome
end

2
app/models/assignment_override_student.rb
View File

@ -36,7 +36,7 @@ class AssignmentOverrideStudent < ActiveRecord::Base
validates :assignment_override, :user, presence: true
validates :user_id, uniqueness: { scope: [:assignment_id, :quiz_id],
conditions: -> { where.not(workflow_state: "deleted") },
message: "already belongs to an assignment override" }
message: -> { t("already belongs to an assignment override") } }
validate :assignment_override, if: :active? do |record|
if record.assignment_override && record.assignment_override.set_type != "ADHOC"

4
app/models/calendar_event.rb
View File

@ -421,7 +421,7 @@ class CalendarEvent < ActiveRecord::Base
e.updating_user = updating_user
e.destroy(false)
end
return true unless update_context_or_parent
next unless update_context_or_parent
if appointment_group
context.touch if context_type == "AppointmentGroup" # ensures end_at/start_at get updated
@ -433,8 +433,8 @@ class CalendarEvent < ActiveRecord::Base
parent_event.workflow_state = parent_event.locked? ? "active" : "deleted"
parent_event.save!
end
true
end
true
end
def time_zone_edited

2
app/models/context_external_tool.rb
View File

@ -41,7 +41,7 @@ class ContextExternalTool < ActiveRecord::Base
validates :config_url, presence: { if: ->(t) { t.config_type == "by_url" } }
validates :config_xml, presence: { if: ->(t) { t.config_type == "by_xml" } }
validates :domain, length: { maximum: 253, allow_blank: true }
validates :lti_version, inclusion: { in: %w[1.1 1.3], message: "%{value} is not a valid LTI version" }
validates :lti_version, inclusion: { in: %w[1.1 1.3], message: -> { t("%{value} is not a valid LTI version") } }
validate :url_or_domain_is_set
validate :validate_urls
attr_reader :config_type, :config_url, :config_xml

4
app/models/custom_gradebook_column.rb
View File

@ -27,11 +27,11 @@ class CustomGradebookColumn < ActiveRecord::Base
validates :title, presence: true
validates :teacher_notes, inclusion: { in: [true, false], message: "teacher_notes must be true or false" }
validates :teacher_notes, inclusion: { in: [true, false], message: -> { t("teacher_notes must be true or false") } }
validates :title, length: { maximum: maximum_string_length },
exclusion: {
in: GradebookImporter::GRADEBOOK_IMPORTER_RESERVED_NAMES,
message: "cannot use gradebook importer reserved names"
message: -> { t("cannot use gradebook importer reserved names") }
},
allow_nil: true

3
app/models/discussion_entry.rb
View File

@ -565,7 +565,8 @@ class DiscussionEntry < ActiveRecord::Base
lock!
old_rating = rating(current_user)
if new_rating == old_rating
return true
entry_participant = true
raise ActiveRecord::Rollback
end
entry_participant = update_or_create_participant(current_user: current_user, rating: new_rating).first

2
app/models/discussion_entry_participant.rb
View File

@ -30,7 +30,7 @@ class DiscussionEntryParticipant < ActiveRecord::Base
validate :prevent_creates
validates :report_type, inclusion: { in: %w[inappropriate offensive other],
message: "%{value} is not valid" }
message: -> { t("%{value} is not valid") } }
def prevent_creates
if new_record?

2
app/models/enrollment.rb
View File

@ -52,7 +52,7 @@ class Enrollment < ActiveRecord::Base
validates :limit_privileges_to_course_section, inclusion: { in: [true, false] }
validates :associated_user_id, inclusion: { in: [nil],
unless: ->(enrollment) { enrollment.type == "ObserverEnrollment" },
message: "only ObserverEnrollments may have an associated_user_id" }
message: -> { t("only ObserverEnrollments may have an associated_user_id") } }
validate :cant_observe_self, if: ->(enrollment) { enrollment.type == "ObserverEnrollment" }
validate :valid_role?

2
app/models/learning_outcome_group.rb
View File

@ -121,7 +121,7 @@ class LearningOutcomeGroup < ActiveRecord::Base
def sync_source_group
transaction do
return unless source_outcome_group
raise ActiveRecord::Rollback unless source_outcome_group
source_outcome_group.child_outcome_links.active.each do |link|
add_outcome(link.content, skip_touch: true)

2
app/models/media_track.rb
View File

@ -32,7 +32,7 @@ class MediaTrack < ActiveRecord::Base
RE_LOOKS_LIKE_TTML = /<tt\s+xml/i.freeze
validates :content, format: {
without: RE_LOOKS_LIKE_TTML,
message: "TTML tracks are not allowed because they are susceptible to xss attacks"
message: -> { t("TTML tracks are not allowed because they are susceptible to xss attacks") }
}
def add_attachment_id

2
app/models/originality_report.rb
View File

@ -35,7 +35,7 @@ class OriginalityReport < ActiveRecord::Base
validates :submission, presence: true
validates :workflow_state, inclusion: { in: ORDERED_VALID_WORKFLOW_STATES }
validates :originality_score, inclusion: { in: 0..100, message: "score must be between 0 and 100" }, allow_nil: true
validates :originality_score, inclusion: { in: 0..100, message: -> { t("score must be between 0 and 100") } }, allow_nil: true
alias_attribute :file_id, :attachment_id
alias_attribute :originality_report_file_id, :originality_report_attachment_id

2
app/models/outcome_calculation_method.rb
View File

@ -54,7 +54,7 @@ class OutcomeCalculationMethod < ApplicationRecord
if: lambda do |model|
CALCULATION_METHODS.include?(model.calculation_method)
end,
message: "invalid calculation_int for this calculation_method"
message: -> { t("invalid calculation_int for this calculation_method") }
}
after_save :clear_cached_methods

4
app/models/role.rb
View File

@ -64,8 +64,8 @@ class Role < ActiveRecord::Base
validates :name, :workflow_state, presence: true
validates :account_id, presence: { if: :belongs_to_account? }
validates :base_role_type, inclusion: { in: BASE_TYPES, message: "is invalid" }
validates :name, exclusion: { in: KNOWN_TYPES, unless: :built_in?, message: "is reserved" }
validates :base_role_type, inclusion: { in: BASE_TYPES, message: -> { t("is invalid") } }
validates :name, exclusion: { in: KNOWN_TYPES, unless: :built_in?, message: -> { t("is reserved") } }
validate :ensure_non_built_in_name
def role_for_root_account_id(target_root_account_id)

2
app/models/wiki.rb
View File

@ -181,7 +181,7 @@ class Wiki < ActiveRecord::Base
# otherwise we lose dirty changes
context.save! if context.changed?
context.lock!
return context.wiki if context.wiki_id
next context.wiki if context.wiki_id
# TODO: i18n
t :default_course_wiki_name, "%{course_name} Wiki", course_name: nil

8
gems/rubocop-canvas/lib/rubocop_canvas.rb
View File

@ -20,6 +20,7 @@
$LOAD_PATH << File.dirname(__FILE__)
require "rubocop"
require "rubocop-rails"
require "rubocop_canvas/version"
# helpers
@ -49,7 +50,6 @@ require "rubocop_canvas/cops/migration/change_column"
require "rubocop_canvas/cops/migration/rename_table"
require "rubocop_canvas/cops/migration/add_index"
require "rubocop_canvas/cops/migration/change_column_null"
require "rubocop_canvas/cops/migration/boolean_columns"
require "rubocop_canvas/cops/migration/data_fixup"
require "rubocop_canvas/cops/migration/predeploy"
require "rubocop_canvas/cops/migration/id_column"
@ -86,9 +86,9 @@ module RuboCop
AST::Node.include(Indifferent)
AST::SymbolNode.include(IndifferentSymbol)
if defined?(Cop::Style::ConcatArrayLiterals)
Cop::Style::ConcatArrayLiterals.prepend(Cop::Style::ConcatArrayLiteralsWithIgnoredReceivers)
end
Cop::Style::ConcatArrayLiterals.prepend(Cop::Style::ConcatArrayLiteralsWithIgnoredReceivers)
Cop::Rails::ThreeStateBooleanColumn.prepend(LegacyMigrations)
Cop::Rails::ThreeStateBooleanColumn.legacy_cutoff_date = "20220628134809"
end
end
end

60
gems/rubocop-canvas/lib/rubocop_canvas/cops/migration/boolean_columns.rb
View File

@ -1,60 +0,0 @@
# frozen_string_literal: true
#
# Copyright (C) 2021 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
module RuboCop
module Cop
module Migration
class BooleanColumns < Cop
include RuboCop::Canvas::CurrentDef
MSG = "Boolean columns should be NOT NULL and have a default value"
def_node_matcher :add_column?, <<~PATTERN
(send nil? :add_column _ _ (sym {:boolean | :bool}) ...)
PATTERN
def_node_matcher :t_boolean?, <<~PATTERN
(send lvar {:boolean | :bool} _ ...)
PATTERN
def_node_matcher :t_column?, <<~PATTERN
(send lvar :column _ (sym {:boolean | :bool}) ...)
PATTERN
def_node_search :default_value?, <<~PATTERN
(pair (sym :default) {true | false})
PATTERN
def_node_search :not_null?, <<~PATTERN
(pair (sym :null) false)
PATTERN
RESTRICT_ON_SEND = %i[add_column column boolean bool].freeze
def on_send(node)
return if @current_def == :down
if add_column?(node) || t_boolean?(node) || t_column?(node)
add_offense(node) unless default_value?(node) && not_null?(node)
end
end
end
end
end
end

1
gems/rubocop-canvas/rubocop-canvas.gemspec
View File

@ -16,6 +16,7 @@ Gem::Specification.new do |spec|
spec.add_dependency "jira_ref_parser", "1.0.1"
spec.add_dependency "outrigger", "~> 3.0"
spec.add_dependency "rubocop", "~> 1.19"
spec.add_dependency "rubocop-rails", "~> 2.19"
spec.add_development_dependency "bundler", "~> 2.2"
spec.add_development_dependency "byebug"

165
gems/rubocop-canvas/spec/rubocop/cop/migration/boolean_columns_spec.rb
View File

@ -1,165 +0,0 @@
# frozen_string_literal: true
#
# Copyright (C) 2021 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
describe RuboCop::Cop::Migration::BooleanColumns do
subject(:cop) { described_class.new }
context "create_table/change_table block with t.boolean" do
it "complains if a boolean column is nullable" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def up
create_table :widgets do |t|
t.boolean :purple, default: true
end
end
end
RUBY
expect(cop.offenses.size).to eq 1
expect(cop.messages.first).to include "Boolean columns should be NOT NULL"
expect(cop.offenses.first.severity.name).to eq(:convention)
end
it "complains if a boolean column has no default" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def up
create_table :widgets do |t|
t.boolean :purple, null: false, default: true
t.boolean :yellow, null: false
end
end
end
RUBY
expect(cop.offenses.size).to eq 1
expect(cop.messages.first).to include "have a default value"
expect(cop.offenses.first.severity.name).to eq(:convention)
end
it "doesn't complain if its requirements are met" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def up
create_table :widgets do |t|
t.boolean :purple, default: true, null: false
end
end
end
RUBY
expect(cop.offenses.size).to eq 0
end
end
context "create_table/change_table block with t.column" do
it "complains if a boolean column is nullable" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
change_table :widgets do |t|
t.column :purple, :boolean, default: true
end
end
end
RUBY
expect(cop.offenses.size).to eq 1
expect(cop.messages.first).to include "Boolean columns should be NOT NULL"
expect(cop.offenses.first.severity.name).to eq(:convention)
end
it "complains if a boolean column has no default" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
change_table :widgets do |t|
t.column :purple, :boolean, null: false, default: true
t.column :yellow, :boolean, null: false
end
end
end
RUBY
expect(cop.offenses.size).to eq 1
expect(cop.messages.first).to include "have a default value"
expect(cop.offenses.first.severity.name).to eq(:convention)
end
it "doesn't complain if its requirements are met" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
change_table :widgets do |t|
t.column :purple, :boolean, default: true, null: false
end
end
end
RUBY
expect(cop.offenses.size).to eq 0
end
end
context "add_column" do
it "complains if a boolean column is nullable" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
add_column :widgets, :purple, :boolean, default: true
end
end
RUBY
expect(cop.offenses.size).to eq 1
expect(cop.messages.first).to include "Boolean columns should be NOT NULL"
expect(cop.offenses.first.severity.name).to eq(:convention)
end
it "complains if a boolean column has no default" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
add_column :widgets, :purple, :boolean, null: false
end
end
RUBY
expect(cop.offenses.size).to eq 1
expect(cop.messages.first).to include "have a default value"
expect(cop.offenses.first.severity.name).to eq(:convention)
end
it "recognizes the :bool spelling" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
add_column :widgets, :purple, :bool
end
end
RUBY
expect(cop.offenses.size).to eq 1
end
it "doesn't complain if its requirements are met" do
inspect_source(<<~RUBY)
class TestMigration < ActiveRecord::Migration
def change
add_column :widgets, :purple, :boolean, null: false, default: true
add_column :widgets, :yellow, :boolean, default: false, null: false
end
end
RUBY
expect(cop.offenses.size).to eq 0
end
end
end