From 97e826f2a4434e62b9f172bfded8bbf00e7f5e4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Oct 2019 16:29:46 +0200 Subject: [PATCH] Don't use `update_attribute` This method is ambiguous. Sometimes we use it to set invalid data in tests (which can usually be done with `update_column`), and other times we use it instead of `update!`. I'm removing it because, even if sometimes it could make sense to use it, it's too similar to `update_attributes` (which is an alias for `update` and runs validations), making it confusing. However, there's one case where we're still using it: in the ActsAsParanoidAliases module, we need to invoke the callbacks, which `update_column` skips, but tests related to translations fail if we use `update!`. The reason for this is the tests check what happens if we restore a record without restoring its translations. But that will make the record invalid, since there's a validation rule checking it has at least one translation. I'm not blacklisting any other method which skips validations because we know they skip validations and use them anyway (hopefully with care). --- .rubocop.yml | 3 --- .rubocop_basic.yml | 6 ++++++ app/controllers/debates_controller.rb | 4 ++-- app/models/poll/question/answer.rb | 2 +- spec/controllers/comments_controller_spec.rb | 2 +- spec/controllers/legislation/annotations_controller_spec.rb | 2 +- spec/controllers/legislation/answers_controller_spec.rb | 2 +- spec/features/admin/admin_notifications_spec.rb | 6 +++--- spec/features/admin/budget_investments_spec.rb | 2 +- spec/features/admin/emails/emails_download_spec.rb | 2 +- spec/features/admin/emails/newsletters_spec.rb | 6 +++--- spec/features/legislation/questions_spec.rb | 2 +- spec/lib/email_digests_spec.rb | 2 +- spec/models/concerns/globalizable.rb | 6 +++--- spec/models/proposal_spec.rb | 2 +- 15 files changed, 26 insertions(+), 23 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 1f4d68f62..4928a817d 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -18,9 +18,6 @@ Performance/StartWith: Rails/HasManyOrHasOneDependent: Enabled: true -Rails/SkipsModelValidations: - Enabled: true - Security/JSONLoad: Enabled: true diff --git a/.rubocop_basic.yml b/.rubocop_basic.yml index 364ae1b84..6ae2b10af 100644 --- a/.rubocop_basic.yml +++ b/.rubocop_basic.yml @@ -248,6 +248,12 @@ Rails/SaveBang: Enabled: true Severity: refactor +Rails/SkipsModelValidations: + Enabled: true + Blacklist: + - update_attribute + Exclude: lib/acts_as_paranoid_aliases.rb + Rails/TimeZone: Enabled: true diff --git a/app/controllers/debates_controller.rb b/app/controllers/debates_controller.rb index 67ff502d4..ee2103ee2 100644 --- a/app/controllers/debates_controller.rb +++ b/app/controllers/debates_controller.rb @@ -36,12 +36,12 @@ class DebatesController < ApplicationController end def unmark_featured - @debate.update_attribute(:featured_at, nil) + @debate.update!(featured_at: nil) redirect_to request.query_parameters.merge(action: :index) end def mark_featured - @debate.update_attribute(:featured_at, Time.current) + @debate.update!(featured_at: Time.current) redirect_to request.query_parameters.merge(action: :index) end diff --git a/app/models/poll/question/answer.rb b/app/models/poll/question/answer.rb index cb431b9b2..91320743c 100644 --- a/app/models/poll/question/answer.rb +++ b/app/models/poll/question/answer.rb @@ -21,7 +21,7 @@ class Poll::Question::Answer < ApplicationRecord def self.order_answers(ordered_array) ordered_array.each_with_index do |answer_id, order| - find(answer_id).update_attribute(:given_order, (order + 1)) + find(answer_id).update_column(:given_order, (order + 1)) end end diff --git a/spec/controllers/comments_controller_spec.rb b/spec/controllers/comments_controller_spec.rb index 9ec1af021..6f9cfc9b2 100644 --- a/spec/controllers/comments_controller_spec.rb +++ b/spec/controllers/comments_controller_spec.rb @@ -27,7 +27,7 @@ describe CommentsController do it "does not create a comment if the comments are closed" do sign_in user - legal_process.update_attribute(:debate_end_date, Date.current - 1.day) + legal_process.update!(debate_end_date: Date.current - 1.day) expect do post :create, xhr: true, diff --git a/spec/controllers/legislation/annotations_controller_spec.rb b/spec/controllers/legislation/annotations_controller_spec.rb index b309d7910..1768b4a3d 100644 --- a/spec/controllers/legislation/annotations_controller_spec.rb +++ b/spec/controllers/legislation/annotations_controller_spec.rb @@ -81,7 +81,7 @@ describe Legislation::AnnotationsController do it "does not create an annotation if the process allegations phase is not open" do sign_in user - legal_process.update_attribute(:allegations_end_date, Date.current - 1.day) + legal_process.update!(allegations_end_date: Date.current - 1.day) expect do post :create, xhr: true, diff --git a/spec/controllers/legislation/answers_controller_spec.rb b/spec/controllers/legislation/answers_controller_spec.rb index b45b97e6f..e05731801 100644 --- a/spec/controllers/legislation/answers_controller_spec.rb +++ b/spec/controllers/legislation/answers_controller_spec.rb @@ -41,7 +41,7 @@ describe Legislation::AnswersController do it "does not create an answer if the process debate phase is not open" do sign_in user - legal_process.update_attribute(:debate_end_date, Date.current - 1.day) + legal_process.update!(debate_end_date: Date.current - 1.day) expect do post :create, xhr: true, diff --git a/spec/features/admin/admin_notifications_spec.rb b/spec/features/admin/admin_notifications_spec.rb index 1829e2d60..6e8f69c9e 100644 --- a/spec/features/admin/admin_notifications_spec.rb +++ b/spec/features/admin/admin_notifications_spec.rb @@ -23,7 +23,7 @@ describe "Admin Notifications" do scenario "Notification with invalid segment recipient" do invalid_notification = create(:admin_notification) - invalid_notification.update_attribute(:segment_recipient, "invalid_segment") + invalid_notification.update_column(:segment_recipient, "invalid_segment") visit admin_admin_notification_path(invalid_notification) @@ -56,7 +56,7 @@ describe "Admin Notifications" do scenario "Notifications with invalid segment recipient" do invalid_notification = create(:admin_notification) - invalid_notification.update_attribute(:segment_recipient, "invalid_segment") + invalid_notification.update_column(:segment_recipient, "invalid_segment") visit admin_admin_notifications_path @@ -209,7 +209,7 @@ describe "Admin Notifications" do scenario "Admin notification with invalid segment recipient cannot be sent", :js do invalid_notification = create(:admin_notification) - invalid_notification.update_attribute(:segment_recipient, "invalid_segment") + invalid_notification.update_column(:segment_recipient, "invalid_segment") visit admin_admin_notification_path(invalid_notification) expect(page).not_to have_link("Send") diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index 93d55e92a..3bb12abdb 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -1832,7 +1832,7 @@ describe "Admin budget investments" do end scenario "Select an investment when some columns are not displayed", :js do - investment.update_attribute(:title, "Don't display me, please!") + investment.update!(title: "Don't display me, please!") visit admin_budget_budget_investments_path(budget) within("#js-columns-selector") { find("strong", text: "Columns").click } diff --git a/spec/features/admin/emails/emails_download_spec.rb b/spec/features/admin/emails/emails_download_spec.rb index 382107199..9c29a0742 100644 --- a/spec/features/admin/emails/emails_download_spec.rb +++ b/spec/features/admin/emails/emails_download_spec.rb @@ -19,7 +19,7 @@ describe "Admin download user emails" do admin_without_email = create(:user, newsletter: true, email: "no_email@consul.dev") create(:administrator, user: admin_without_email) - admin_without_email.update_attribute(:email, nil) + admin_without_email.update_column(:email, nil) end scenario "returns the selected users segment csv file" do diff --git a/spec/features/admin/emails/newsletters_spec.rb b/spec/features/admin/emails/newsletters_spec.rb index a1acb8ef4..d78b3c0fe 100644 --- a/spec/features/admin/emails/newsletters_spec.rb +++ b/spec/features/admin/emails/newsletters_spec.rb @@ -23,7 +23,7 @@ describe "Admin newsletter emails" do scenario "Invalid newsletter" do invalid_newsletter = create(:newsletter) - invalid_newsletter.update_attribute(:segment_recipient, "invalid_segment") + invalid_newsletter.update_column(:segment_recipient, "invalid_segment") visit admin_newsletter_path(invalid_newsletter) @@ -50,7 +50,7 @@ describe "Admin newsletter emails" do scenario "Invalid newsletter" do invalid_newsletter = create(:newsletter) - invalid_newsletter.update_attribute(:segment_recipient, "invalid_segment") + invalid_newsletter.update_column(:segment_recipient, "invalid_segment") visit admin_newsletters_path @@ -136,7 +136,7 @@ describe "Admin newsletter emails" do scenario "Invalid newsletter cannot be sent", :js do invalid_newsletter = create(:newsletter) - invalid_newsletter.update_attribute(:segment_recipient, "invalid_segment") + invalid_newsletter.update_column(:segment_recipient, "invalid_segment") visit admin_newsletter_path(invalid_newsletter) expect(page).not_to have_link("Send") diff --git a/spec/features/legislation/questions_spec.rb b/spec/features/legislation/questions_spec.rb index 32d416c77..10deb9a95 100644 --- a/spec/features/legislation/questions_spec.rb +++ b/spec/features/legislation/questions_spec.rb @@ -94,7 +94,7 @@ describe "Legislation" do end scenario "cannot answer question when phase not open" do - process.update_attribute(:debate_end_date, Date.current - 1.day) + process.update!(debate_end_date: Date.current - 1.day) question = process.questions.first create(:legislation_question_option, question: question, value: "Yes") create(:legislation_question_option, question: question, value: "No") diff --git a/spec/lib/email_digests_spec.rb b/spec/lib/email_digests_spec.rb index ab96fc0d6..d079691c9 100644 --- a/spec/lib/email_digests_spec.rb +++ b/spec/lib/email_digests_spec.rb @@ -133,7 +133,7 @@ describe EmailDigest do it "returns false if email does not exist" do user = create(:user) - user.update_attribute(:email, nil) + user.email = nil email_digest = EmailDigest.new(user) expect(email_digest.valid_email?).to be(false) diff --git a/spec/models/concerns/globalizable.rb b/spec/models/concerns/globalizable.rb index fc4c01e61..c72de6499 100644 --- a/spec/models/concerns/globalizable.rb +++ b/spec/models/concerns/globalizable.rb @@ -13,11 +13,11 @@ shared_examples_for "globalizable" do |factory_name| let(:attribute) { required_fields.sample || fields.sample } before do - record.update_attribute(attribute, "In English") + record.update!(attribute => "In English") I18n.with_locale(:es) do record.update!(required_fields.map { |field| [field, "En español"] }.to_h) - record.update_attribute(attribute, "En español") + record.update!(attribute => "En español") end record.reload @@ -156,7 +156,7 @@ shared_examples_for "globalizable" do |factory_name| before do I18n.with_locale(:de) do record.update!(required_fields.map { |field| [field, "Deutsch"] }.to_h) - record.update_attribute(attribute, "Deutsch") + record.update!(attribute => "Deutsch") end end diff --git a/spec/models/proposal_spec.rb b/spec/models/proposal_spec.rb index e75660a53..3cc07c438 100644 --- a/spec/models/proposal_spec.rb +++ b/spec/models/proposal_spec.rb @@ -253,7 +253,7 @@ describe Proposal do proposal = create(:proposal) tag_list = ["tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7"] - proposal.update_attribute(:tag_list, tag_list) + proposal.update!(tag_list: tag_list) expect(proposal.update_cached_votes).to eq(true) end