From 480ab6a9da643e62b3c3d77c0437cc058f4761ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 22:18:11 +0200 Subject: [PATCH 1/6] Use truncate_all instead of DatabaseCleaner Performance tests show both methods of truncating the database take about the same time, so we can remove one dependency and we don't lose anything in the process. --- Gemfile | 1 - Gemfile.lock | 7 ------- db/dev_seeds.rb | 5 +---- spec/spec_helper.rb | 2 +- 4 files changed, 2 insertions(+), 13 deletions(-) diff --git a/Gemfile b/Gemfile index e3537bce2..a9cb8fbbd 100644 --- a/Gemfile +++ b/Gemfile @@ -72,7 +72,6 @@ end group :development, :test do gem "bullet", "~> 6.1.4" gem "byebug", "~> 11.1.3" - gem "database_cleaner", "~> 2.0.1" gem "factory_bot_rails", "~> 6.2.0" gem "faker", "~> 2.18.0" gem "i18n-tasks", "~> 0.9.34" diff --git a/Gemfile.lock b/Gemfile.lock index ee0c8f765..8287adb66 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -168,12 +168,6 @@ GEM crass (1.0.6) daemons (1.4.0) dalli (2.7.11) - database_cleaner (2.0.1) - database_cleaner-active_record (~> 2.0.0) - database_cleaner-active_record (2.0.1) - activerecord (>= 5.a) - database_cleaner-core (~> 2.0.0) - database_cleaner-core (2.0.1) delayed_job (4.1.9) activesupport (>= 3.0, < 6.2) delayed_job_active_record (4.1.6) @@ -730,7 +724,6 @@ DEPENDENCIES coveralls (~> 0.8.23) daemons (~> 1.4.0) dalli (~> 2.7.11) - database_cleaner (~> 2.0.1) delayed_job_active_record (~> 4.1.6) devise (~> 4.8.0) devise-security (~> 0.16.0) diff --git a/db/dev_seeds.rb b/db/dev_seeds.rb index bdbacaff3..ebcfeacb5 100644 --- a/db/dev_seeds.rb +++ b/db/dev_seeds.rb @@ -1,7 +1,4 @@ -unless Rails.env.test? - require "database_cleaner" - DatabaseCleaner.clean_with :truncation -end +ActiveRecord::Tasks::DatabaseTasks.truncate_all unless Rails.env.test? @logger = Logger.new(STDOUT) @logger.formatter = proc do |_severity, _datetime, _progname, msg| msg unless @avoid_log diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 7e84626b4..59c87016a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -45,7 +45,7 @@ RSpec.configure do |config| example.run self.use_transactional_tests = true - DatabaseCleaner.clean_with(:truncation) + ActiveRecord::Tasks::DatabaseTasks.truncate_all Rails.application.load_seed end From ea3abd6317884253340a45c4ed38f2909a59297f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 22:57:06 +0200 Subject: [PATCH 2/6] Add and apply Rails/Pick rubocop rule The `pick` method was added in Rails 6.0. --- .rubocop.yml | 3 +++ app/models/setting.rb | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index baac68548..61ec40311 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -288,6 +288,9 @@ Rails/OutputSafety: Exclude: - app/helpers/text_with_links_helper.rb +Rails/Pick: + Enabled: true + Rails/PluckId: Enabled: true diff --git a/app/models/setting.rb b/app/models/setting.rb index 63e326e09..9fa1e7d68 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -31,7 +31,7 @@ class Setting < ApplicationRecord class << self def [](key) - where(key: key).pluck(:value).first.presence + where(key: key).pick(:value).presence end def []=(key, value) @@ -42,7 +42,7 @@ class Setting < ApplicationRecord def rename_key(from:, to:) if where(key: to).empty? - value = where(key: from).pluck(:value).first.presence + value = where(key: from).pick(:value).presence create!(key: to, value: value) end remove(from) From b59e2b31d3e9745a4275bdc20a4877265dce8550 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 23:18:43 +0200 Subject: [PATCH 3/6] Use unfreeze_time instead of travel_back This is consistent with `with_frozen_time`. --- spec/models/poll/stats_spec.rb | 2 +- spec/system/polls/voter_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/poll/stats_spec.rb b/spec/models/poll/stats_spec.rb index abe080fe8..27b7571a0 100644 --- a/spec/models/poll/stats_spec.rb +++ b/spec/models/poll/stats_spec.rb @@ -267,7 +267,7 @@ describe Poll::Stats do expect(stats.version).to eq "v#{time.to_i}" - travel_back + unfreeze_time travel_to 2.seconds.from_now do expect(stats.version).to eq "v#{time.to_i}" diff --git a/spec/system/polls/voter_spec.rb b/spec/system/polls/voter_spec.rb index cac36b494..3b9c6bc1d 100644 --- a/spec/system/polls/voter_spec.rb +++ b/spec/system/polls/voter_spec.rb @@ -176,7 +176,7 @@ describe "Voter" do expect(page).not_to have_link(answer_yes.title) end - travel_back + unfreeze_time click_link "Sign out" From c97e7852a400c7f78c2d13774aeddbe2057c7b25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 23:37:46 +0200 Subject: [PATCH 4/6] Use "excluding" instead of "reject" This method was already available as #without, but we didn't know about it. --- app/views/shared/_errors.html.erb | 2 +- spec/models/budget/investment_spec.rb | 8 ++++---- spec/system/admin/poll/shifts_spec.rb | 6 ++---- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/views/shared/_errors.html.erb b/app/views/shared/_errors.html.erb index fff7801f8..80f49048c 100644 --- a/app/views/shared/_errors.html.erb +++ b/app/views/shared/_errors.html.erb @@ -5,7 +5,7 @@ - <% errors_count = resource.errors.messages.reject { |attribute| attribute == :base }.count %> + <% errors_count = resource.errors.messages.excluding(:base).count %> <%= pluralize errors_count, t("form.error"), t("form.errors") %> diff --git a/spec/models/budget/investment_spec.rb b/spec/models/budget/investment_spec.rb index fa8bb73f8..a0d4e2ef4 100644 --- a/spec/models/budget/investment_spec.rb +++ b/spec/models/budget/investment_spec.rb @@ -160,7 +160,7 @@ describe Budget::Investment do end it "returns false in any other phase" do - Budget::Phase::PHASE_KINDS.reject { |phase| phase == "selecting" }.each do |phase| + Budget::Phase::PHASE_KINDS.excluding("selecting").each do |phase| budget = create(:budget, phase: phase) investment = create(:budget_investment, budget: budget) @@ -178,7 +178,7 @@ describe Budget::Investment do end it "returns false in any other phase" do - Budget::Phase::PHASE_KINDS.reject { |phase| phase == "valuating" }.each do |phase| + Budget::Phase::PHASE_KINDS.excluding("valuating").each do |phase| budget = create(:budget, phase: phase) investment = create(:budget_investment, budget: budget) @@ -203,7 +203,7 @@ describe Budget::Investment do end it "returns false in any other phase" do - Budget::Phase::PHASE_KINDS.reject { |phase| phase == "balloting" }.each do |phase| + Budget::Phase::PHASE_KINDS.excluding("balloting").each do |phase| budget = create(:budget, phase: phase) investment = create(:budget_investment, :selected, budget: budget) @@ -1198,7 +1198,7 @@ describe Budget::Investment do end it "returns false if budget is not balloting phase" do - Budget::Phase::PHASE_KINDS.reject { |phase| phase == "balloting" }.each do |phase| + Budget::Phase::PHASE_KINDS.excluding("balloting").each do |phase| budget.update!(phase: phase) investment = create(:budget_investment, heading: heading1) diff --git a/spec/system/admin/poll/shifts_spec.rb b/spec/system/admin/poll/shifts_spec.rb index 683906622..a5a475092 100644 --- a/spec/system/admin/poll/shifts_spec.rb +++ b/spec/system/admin/poll/shifts_spec.rb @@ -96,11 +96,9 @@ describe "Admin shifts", :admin do create(:poll_shift, :vote_collection_task, officer: officer, booth: booth, date: Date.current) create(:poll_shift, :recount_scrutiny_task, officer: officer, booth: booth, date: Time.zone.tomorrow) - vote_collection_dates = (Date.current..poll.ends_at.to_date).to_a - .reject { |date| date == Date.current } + vote_collection_dates = (Date.current..poll.ends_at.to_date).excluding(Date.current) .map { |date| I18n.l(date, format: :long) } - recount_scrutiny_dates = (poll.ends_at.to_date..poll.ends_at.to_date + 1.week).to_a - .reject { |date| date == Time.zone.tomorrow } + recount_scrutiny_dates = (poll.ends_at.to_date..poll.ends_at.to_date + 1.week).excluding(Time.zone.tomorrow) .map { |date| I18n.l(date, format: :long) } visit available_admin_booths_path From eca1714a26f54d82b78435f9d9ce9b8dd54372b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 26 Jul 2022 02:03:32 +0200 Subject: [PATCH 5/6] Use Rails native attachment validations They were introduced in Rails 6.0. --- app/models/concerns/attachable.rb | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index cc457f934..87bd92363 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -5,9 +5,8 @@ module Attachable has_one_attached :attachment attr_accessor :cached_attachment - validate :attachment_presence - validates :attachment, + presence: true, file_content_type: { allow: ->(record) { record.accepted_content_types }, if: -> { association_class && attachment.attached? }, @@ -63,12 +62,4 @@ module Attachable def file_path ActiveStorage::Blob.service.path_for(attachment.blob.key) end - - private - - def attachment_presence - unless attachment.attached? - errors.add(:attachment, I18n.t("errors.messages.blank")) - end - end end From 5b844bf231a1ae7a3882860acb54c6c31be75bec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 26 Jul 2022 02:34:17 +0200 Subject: [PATCH 6/6] Use index_with to simplify hash generation This method was introduced in Rails 6.0. It can be used to take an array and create a hash where the elements of the array are the indexes of the hash. --- config/initializers/globalize.rb | 4 ++-- db/dev_seeds/admin_notifications.rb | 24 ++++++++++++------------ db/dev_seeds/widgets.rb | 24 ++++++++++++------------ spec/models/concerns/globalizable.rb | 12 ++++++------ 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/config/initializers/globalize.rb b/config/initializers/globalize.rb index 7d93e9fda..80fd85ca8 100644 --- a/config/initializers/globalize.rb +++ b/config/initializers/globalize.rb @@ -17,7 +17,7 @@ module Globalize end def Globalize.set_fallbacks_to_all_available_locales - Globalize.fallbacks = I18n.available_locales.each_with_object({}) do |locale, fallbacks| - fallbacks[locale] = (I18n.fallbacks[locale] + I18n.available_locales).uniq + Globalize.fallbacks = I18n.available_locales.index_with do |locale| + (I18n.fallbacks[locale] + I18n.available_locales).uniq end end diff --git a/db/dev_seeds/admin_notifications.rb b/db/dev_seeds/admin_notifications.rb index fee6ebeb5..47e2a8702 100644 --- a/db/dev_seeds/admin_notifications.rb +++ b/db/dev_seeds/admin_notifications.rb @@ -1,33 +1,33 @@ section "Creating Admin Notifications & Templates" do AdminNotification.create!( random_locales_attributes( - %i[title body].map do |attribute| - [attribute, -> { I18n.t("seeds.admin_notifications.proposal.#{attribute}") }] - end.to_h + %i[title body].index_with do |attribute| + -> { I18n.t("seeds.admin_notifications.proposal.#{attribute}") } + end ).merge(link: "#{Setting["url"]}/proposals", segment_recipient: "administrators") ).deliver AdminNotification.create!( random_locales_attributes( - %i[title body].map do |attribute| - [attribute, -> { I18n.t("seeds.admin_notifications.help.#{attribute}") }] - end.to_h + %i[title body].index_with do |attribute| + -> { I18n.t("seeds.admin_notifications.help.#{attribute}") } + end ).merge(link: "https://crwd.in/consul", segment_recipient: "administrators") ).deliver AdminNotification.create!( random_locales_attributes( - %i[title body].map do |attribute| - [attribute, -> { I18n.t("seeds.admin_notifications.map.#{attribute}") }] - end.to_h + %i[title body].index_with do |attribute| + -> { I18n.t("seeds.admin_notifications.map.#{attribute}") } + end ).merge(segment_recipient: "administrators") ).deliver AdminNotification.create!( random_locales_attributes( - %i[title body].map do |attribute| - [attribute, -> { I18n.t("seeds.admin_notifications.budget.#{attribute}") }] - end.to_h + %i[title body].index_with do |attribute| + -> { I18n.t("seeds.admin_notifications.budget.#{attribute}") } + end ).merge(segment_recipient: "administrators", sent_at: nil) ) end diff --git a/db/dev_seeds/widgets.rb b/db/dev_seeds/widgets.rb index d82d655fe..d77940f7d 100644 --- a/db/dev_seeds/widgets.rb +++ b/db/dev_seeds/widgets.rb @@ -9,9 +9,9 @@ section "Creating header and cards for the homepage" do Widget::Card.create!( random_locales_attributes( - %i[title description link_text label].map do |attribute| - [attribute, -> { I18n.t("seeds.cards.header.#{attribute}") }] - end.to_h + %i[title description link_text label].index_with do |attribute| + -> { I18n.t("seeds.cards.header.#{attribute}") } + end ).merge( link_url: "http://consulproject.org/", header: true, @@ -21,9 +21,9 @@ section "Creating header and cards for the homepage" do Widget::Card.create!( random_locales_attributes( - %i[title description link_text label].map do |attribute| - [attribute, -> { I18n.t("seeds.cards.debate.#{attribute}") }] - end.to_h + %i[title description link_text label].index_with do |attribute| + -> { I18n.t("seeds.cards.debate.#{attribute}") } + end ).merge( link_url: "https://youtu.be/zU_0UN4VajY", header: false, @@ -33,9 +33,9 @@ section "Creating header and cards for the homepage" do Widget::Card.create!( random_locales_attributes( - %i[title description link_text label].map do |attribute| - [attribute, -> { I18n.t("seeds.cards.proposal.#{attribute}") }] - end.to_h + %i[title description link_text label].index_with do |attribute| + -> { I18n.t("seeds.cards.proposal.#{attribute}") } + end ).merge( link_url: "https://youtu.be/ZHqBpT4uCoM", header: false, @@ -45,9 +45,9 @@ section "Creating header and cards for the homepage" do Widget::Card.create!( random_locales_attributes( - %i[title description link_text label].map do |attribute| - [attribute, -> { I18n.t("seeds.cards.budget.#{attribute}") }] - end.to_h + %i[title description link_text label].index_with do |attribute| + -> { I18n.t("seeds.cards.budget.#{attribute}") } + end ).merge( link_url: "https://youtu.be/igQ8KGZdk9c", header: false, diff --git a/spec/models/concerns/globalizable.rb b/spec/models/concerns/globalizable.rb index 54135137c..0eca3b268 100644 --- a/spec/models/concerns/globalizable.rb +++ b/spec/models/concerns/globalizable.rb @@ -16,7 +16,7 @@ shared_examples_for "globalizable" do |factory_name| record.update!(attribute => "In English") I18n.with_locale(:es) do - record.update!(required_fields.map { |field| [field, "En español"] }.to_h) + record.update!(required_fields.index_with("En español")) record.update!(attribute => "En español") end @@ -26,7 +26,7 @@ shared_examples_for "globalizable" do |factory_name| describe "Add a translation" do it "Maintains existing translations" do record.update!(translations_attributes: [ - { locale: :fr }.merge(fields.map { |field| [field, "En Français"] }.to_h) + { locale: :fr }.merge(fields.index_with("En Français")) ]) record.reload @@ -37,7 +37,7 @@ shared_examples_for "globalizable" do |factory_name| it "Works with non-underscored locale name" do record.update!(translations_attributes: [ - { locale: :"pt-BR" }.merge(fields.map { |field| [field, "Português"] }.to_h) + { locale: :"pt-BR" }.merge(fields.index_with("Português")) ]) record.reload @@ -63,7 +63,7 @@ shared_examples_for "globalizable" do |factory_name| record.reload record.update!(translations_attributes: [ - { locale: :de }.merge(fields.map { |field| [field, "Deutsche Sprache"] }.to_h) + { locale: :de }.merge(fields.index_with("Deutsche Sprache")) ]) record.reload @@ -102,7 +102,7 @@ shared_examples_for "globalizable" do |factory_name| record.reload record.update!(translations_attributes: [ - { id: record.translations.first.id }.merge(fields.map { |field| [field, "Actualizado"] }.to_h) + { id: record.translations.first.id }.merge(fields.index_with("Actualizado")) ]) record.reload @@ -155,7 +155,7 @@ shared_examples_for "globalizable" do |factory_name| describe "Fallbacks" do before do I18n.with_locale(:de) do - record.update!(required_fields.map { |field| [field, "Deutsche Sprache"] }.to_h) + record.update!(required_fields.index_with("Deutsche Sprache")) record.update!(attribute => "Deutsche Sprache") end end