From a56e1bf3cf1c409d0fbbcc1d5f7f05f7992007cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Apr 2024 05:07:18 +0200 Subject: [PATCH 1/7] Simplify strategy to insert records in tests Since Rails 7.0, the `insert` method automatically generates timestamps. --- spec/factory_bot/strategy/insert.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/spec/factory_bot/strategy/insert.rb b/spec/factory_bot/strategy/insert.rb index 5c7468a25..9de345d1c 100644 --- a/spec/factory_bot/strategy/insert.rb +++ b/spec/factory_bot/strategy/insert.rb @@ -11,11 +11,7 @@ module FactoryBot build_class = evaluation.instance_variable_get(:@attribute_assigner) .instance_variable_get(:@build_class) - timestamps = { created_at: Time.current, updated_at: Time.current }.select do |attribute, _| - build_class.has_attribute?(attribute) - end - - build_class.insert!(timestamps.merge(@strategy.result(evaluation))) + build_class.insert!(@strategy.result(evaluation)) end end From 9841a9b03aa18745ef6879f4329480e9c07abd6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Apr 2024 05:18:01 +0200 Subject: [PATCH 2/7] Use in_order_of to sort translations by fallback This method was introduced in Rails 7.0, and thanks to it we can simplify the code that gets the translations in order. We tried to use this method to simplify the `Randomizable` concern as well. However, we found out that, when ordering tens of thousands of records, the query could take several minutes, so we aren't using it in this case. Using it for translation fallbacks is OK, since there's a good chance we're never going to have tens of thousands of available locales. Note that automated security tools reported a false positive related to SQL Injection due to the way we used `LEFT JOIN`, so now we get one less false positive in these reports. --- app/models/concerns/globalizable.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/globalizable.rb b/app/models/concerns/globalizable.rb index 5578c37db..0ab046c1b 100644 --- a/app/models/concerns/globalizable.rb +++ b/app/models/concerns/globalizable.rb @@ -102,17 +102,11 @@ module Globalizable translations_foreign_key = reflect_on_association(:translations).foreign_key fallbacks = Globalize.fallbacks(Globalize.locale) - fallbacks_with_order = fallbacks.map.with_index do |locale, order| - "('#{locale}', #{order})" - end.join(", ") - translations_ids = translation_class .select("DISTINCT ON (#{translations_foreign_key}) id") .where(locale: fallbacks) - .joins("LEFT JOIN (VALUES #{fallbacks_with_order}) " \ - "AS locales(name, ordering) " \ - "ON locale = locales.name") - .order(translations_foreign_key, "locales.ordering") + .order(translations_foreign_key) + .in_order_of(:locale, fallbacks) with_translations(fallbacks).where("#{translations_table_name}.id": translations_ids) end From 38ad65605ec946d3dc8267720b215acdb03aa14f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Apr 2024 18:51:52 +0200 Subject: [PATCH 3/7] Use `excluding` instead of `where.not(id:` This method was added in Rails 7.0 and makes the code slihgtly more readable. The downside is that it generates two queries instead of one, so it might generate some confusion when debugging SQL queries. Its impact on performance is probably negligible. --- app/controllers/proposals_controller.rb | 2 +- app/lib/acts_as_paranoid_aliases.rb | 2 +- app/models/banner.rb | 2 +- app/models/budget.rb | 2 +- app/models/budget/investment.rb | 4 ++-- app/models/concerns/verification.rb | 2 +- app/models/dashboard/administrator_task.rb | 2 +- app/models/poll.rb | 2 +- app/models/poll/question/option.rb | 2 +- app/models/proposal.rb | 10 +++------- app/models/proposal_notification.rb | 2 +- app/models/user.rb | 2 +- db/dev_seeds/flags.rb | 6 +++--- db/dev_seeds/votes.rb | 2 +- 14 files changed, 19 insertions(+), 23 deletions(-) diff --git a/app/controllers/proposals_controller.rb b/app/controllers/proposals_controller.rb index d22836066..7a27f7d3f 100644 --- a/app/controllers/proposals_controller.rb +++ b/app/controllers/proposals_controller.rb @@ -164,7 +164,7 @@ class ProposalsController < ApplicationController .sort_by_confidence_score .limit(Setting["featured_proposals_number"]) if @featured_proposals.present? - @resources = @resources.where.not(id: @featured_proposals) + @resources = @resources.excluding(@featured_proposals) end end end diff --git a/app/lib/acts_as_paranoid_aliases.rb b/app/lib/acts_as_paranoid_aliases.rb index 76afbbdf0..f8a44f766 100644 --- a/app/lib/acts_as_paranoid_aliases.rb +++ b/app/lib/acts_as_paranoid_aliases.rb @@ -43,7 +43,7 @@ module ActsAsParanoidAliases end def without_confirmed_hide - where(confirmed_hide_at: nil) + excluding(with_confirmed_hide) end def with_hidden diff --git a/app/models/banner.rb b/app/models/banner.rb index ea3a9b513..8d0513a21 100644 --- a/app/models/banner.rb +++ b/app/models/banner.rb @@ -20,7 +20,7 @@ class Banner < ApplicationRecord has_many :web_sections, through: :sections scope :with_active, -> { where(post_started_at: ..Date.current, post_ended_at: Date.current..) } - scope :with_inactive, -> { where.not(id: with_active) } + scope :with_inactive, -> { excluding(with_active) } scope :in_section, ->(section_name) do joins(:web_sections, :sections).where("web_sections.name ilike ?", section_name) end diff --git a/app/models/budget.rb b/app/models/budget.rb index 319f02d4b..894519282 100644 --- a/app/models/budget.rb +++ b/app/models/budget.rb @@ -47,7 +47,7 @@ class Budget < ApplicationRecord accepts_nested_attributes_for :phases scope :published, -> { where(published: true) } - scope :drafting, -> { where.not(id: published) } + scope :drafting, -> { excluding(published) } scope :informing, -> { where(phase: "informing") } scope :accepting, -> { where(phase: "accepting") } scope :reviewing, -> { where(phase: "reviewing") } diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 53b8c196c..b6b2ae6be 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -73,7 +73,7 @@ class Budget scope :valuation_open, -> { where(valuation_finished: false) } scope :with_admin, -> { where.not(administrator_id: nil) } - scope :without_admin, -> { where(administrator_id: nil) } + scope :without_admin, -> { excluding(with_admin) } scope :without_valuator_group, -> { where(valuator_group_assignments_count: 0) } scope :without_valuator, -> { without_valuator_group.where(valuator_assignments_count: 0) } scope :under_valuation, -> { valuation_open.valuating.with_admin } @@ -87,7 +87,7 @@ class Budget scope :valuation_finished_feasible, -> { where(valuation_finished: true, feasibility: "feasible") } scope :feasible, -> { where(feasibility: "feasible") } scope :unfeasible, -> { where(feasibility: "unfeasible") } - scope :not_unfeasible, -> { where.not(feasibility: "unfeasible") } + scope :not_unfeasible, -> { excluding(unfeasible) } scope :undecided, -> { where(feasibility: "undecided") } scope :with_supports, -> { where(cached_votes_up: 1..) } diff --git a/app/models/concerns/verification.rb b/app/models/concerns/verification.rb index 298c44b4d..e72ba0ab8 100644 --- a/app/models/concerns/verification.rb +++ b/app/models/concerns/verification.rb @@ -3,7 +3,7 @@ module Verification included do scope :residence_verified, -> { where.not(residence_verified_at: nil) } - scope :residence_unverified, -> { where(residence_verified_at: nil) } + scope :residence_unverified, -> { excluding(residence_verified) } scope :residence_and_phone_verified, -> { residence_verified.where.not(confirmed_phone: nil) } scope :residence_or_phone_unverified, -> { residence_unverified.or(where(confirmed_phone: nil)) } scope :phone_not_fully_confirmed, -> { where(unconfirmed_phone: nil).or(where(confirmed_phone: nil)) } diff --git a/app/models/dashboard/administrator_task.rb b/app/models/dashboard/administrator_task.rb index e0091d0a4..d0682c99d 100644 --- a/app/models/dashboard/administrator_task.rb +++ b/app/models/dashboard/administrator_task.rb @@ -6,8 +6,8 @@ class Dashboard::AdministratorTask < ApplicationRecord default_scope { order(created_at: :asc) } - scope :pending, -> { where(executed_at: nil) } scope :done, -> { where.not(executed_at: nil) } + scope :pending, -> { excluding(done) } def title "#{source.proposal.title} #{source.action.title}" diff --git a/app/models/poll.rb b/app/models/poll.rb index 18c78a77d..8f5da2b3e 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -80,7 +80,7 @@ class Poll < ApplicationRecord where("? < ends_at and ? >= starts_at", poll.starts_at.beginning_of_day, poll.ends_at.end_of_day) - .where.not(id: poll.id) + .excluding(poll) .where(related: poll.related) end diff --git a/app/models/poll/question/option.rb b/app/models/poll/question/option.rb index c191c5923..299e764bb 100644 --- a/app/models/poll/question/option.rb +++ b/app/models/poll/question/option.rb @@ -20,7 +20,7 @@ class Poll::Question::Option < ApplicationRecord validates_translation :title, presence: true validates :given_order, presence: true, uniqueness: { scope: :question_id } - scope :with_content, -> { where.not(id: without_content) } + scope :with_content, -> { excluding(without_content) } scope :without_content, -> do where(description: "") .where.missing(:images) diff --git a/app/models/proposal.rb b/app/models/proposal.rb index 634077fe0..6cdcc5979 100644 --- a/app/models/proposal.rb +++ b/app/models/proposal.rb @@ -79,14 +79,14 @@ class Proposal < ApplicationRecord scope :not_archived, -> { where(created_at: Setting.archived_proposals_date_limit..) } scope :last_week, -> { where(created_at: 7.days.ago..) } scope :retired, -> { where.not(retired_at: nil) } - scope :not_retired, -> { where(retired_at: nil) } + scope :not_retired, -> { excluding(retired) } scope :successful, -> { where(cached_votes_up: Proposal.votes_needed_for_success..) } scope :unsuccessful, -> { where(cached_votes_up: ...Proposal.votes_needed_for_success) } scope :public_for_api, -> { all } scope :selected, -> { where(selected: true) } scope :not_selected, -> { where(selected: false) } scope :published, -> { where.not(published_at: nil) } - scope :draft, -> { where(published_at: nil) } + scope :draft, -> { excluding(published) } scope :not_supported_by_user, ->(user) { where.not(id: user.find_voted_items(votable_type: "Proposal")) } scope :created_by, ->(author) { where(author: author) } @@ -108,15 +108,11 @@ class Proposal < ApplicationRecord tagged_with(user.interests, any: true) .where.not(author_id: user.id) .unsuccessful - .not_followed_by_user(user) + .excluding(followed_by_user(user)) .not_archived .not_supported_by_user(user) end - def self.not_followed_by_user(user) - where.not(id: followed_by_user(user).ids) - end - def to_param "#{id}-#{title}".parameterize end diff --git a/app/models/proposal_notification.rb b/app/models/proposal_notification.rb index 5579fd728..7cad3c705 100644 --- a/app/models/proposal_notification.rb +++ b/app/models/proposal_notification.rb @@ -16,7 +16,7 @@ class ProposalNotification < ApplicationRecord scope :sort_by_moderated, -> { reorder(moderated: :desc) } scope :moderated, -> { where(moderated: true) } - scope :not_moderated, -> { where(moderated: false) } + scope :not_moderated, -> { excluding(moderated) } scope :pending_review, -> { moderated.where(ignored_at: nil) } scope :ignored, -> { moderated.where.not(ignored_at: nil) } diff --git a/app/models/user.rb b/app/models/user.rb index 07c05dd05..8ba6d0f4c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -104,8 +104,8 @@ class User < ApplicationRecord where(document_type: document_type, document_number: document_number) end scope :email_digest, -> { where(email_digest: true) } - scope :active, -> { where(erased_at: nil) } scope :erased, -> { where.not(erased_at: nil) } + scope :active, -> { excluding(erased) } scope :public_for_api, -> { all } scope :by_authors, ->(author_ids) { where(id: author_ids) } scope :by_comments, ->(commentables) do diff --git a/db/dev_seeds/flags.rb b/db/dev_seeds/flags.rb index 582bd088f..dc5db86ab 100644 --- a/db/dev_seeds/flags.rb +++ b/db/dev_seeds/flags.rb @@ -1,19 +1,19 @@ section "Flagging Debates & Comments" do 40.times do debate = Debate.sample - flagger = User.where.not(id: debate.author_id).sample + flagger = User.excluding(debate.author).sample Flag.flag(flagger, debate) end 40.times do comment = Comment.sample - flagger = User.where.not(id: comment.user_id).sample + flagger = User.excluding(comment.user).sample Flag.flag(flagger, comment) end 40.times do proposal = Proposal.sample - flagger = User.where.not(id: proposal.author_id).sample + flagger = User.excluding(proposal.author).sample Flag.flag(flagger, proposal) end end diff --git a/db/dev_seeds/votes.rb b/db/dev_seeds/votes.rb index eb7711136..a91b6d315 100644 --- a/db/dev_seeds/votes.rb +++ b/db/dev_seeds/votes.rb @@ -1,5 +1,5 @@ section "Voting Debates, Proposals & Comments" do - not_org_users = User.where.not(id: User.organizations) + not_org_users = User.excluding(User.organizations) 100.times do voter = not_org_users.level_two_or_three_verified.sample vote = [true, false].sample From 1b1815194125a5bfa5e825f51a911729f37008c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Apr 2024 19:05:34 +0200 Subject: [PATCH 4/7] Use minimum and maximum in enumerables While the `minimum` and `maximum` methods have been available for a long time for ActiveRecord relations, Rails 7.0 has added these methods for enumerables as well. This means that the `start_date` and `end_date` methods in the ShiftsHelper can use `minimum` and `maximum` no matter whether they receive an ActiveRecord relation or an array of polls (I think the latter never happens, though, but I'm not 100% sure). --- app/helpers/shifts_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/shifts_helper.rb b/app/helpers/shifts_helper.rb index 01552a730..34a64e2a2 100644 --- a/app/helpers/shifts_helper.rb +++ b/app/helpers/shifts_helper.rb @@ -24,12 +24,12 @@ module ShiftsHelper end def start_date(polls) - start_date = polls.map(&:starts_at).min.to_date + start_date = polls.minimum(:starts_at).to_date [start_date, Date.current].max end def end_date(polls) - polls.map(&:ends_at).max.to_date + polls.maximum(:ends_at).to_date end def officer_select_options(officers) From f8f6844ec3da9530fd88096ca8ba5fd2a3c4fd42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Apr 2024 21:05:50 +0200 Subject: [PATCH 5/7] Simplify common HTML attributes using tag.attributes The `tag.attributes` method was introduced in Rails 7.0. --- app/components/layout/common_html_attributes_component.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/components/layout/common_html_attributes_component.rb b/app/components/layout/common_html_attributes_component.rb index 63f35e0f6..c79881ba6 100644 --- a/app/components/layout/common_html_attributes_component.rb +++ b/app/components/layout/common_html_attributes_component.rb @@ -4,18 +4,18 @@ class Layout::CommonHtmlAttributesComponent < ApplicationComponent private def attributes - sanitize([dir, lang, html_class].compact.join(" ")) + tag.attributes(dir: dir, lang: lang, class: html_class) end def dir - 'dir="rtl"' if rtl? + "rtl" if rtl? end def lang - "lang=\"#{I18n.locale}\"" + I18n.locale end def html_class - "class=\"tenant-#{Tenant.current_schema}\"" if Rails.application.config.multitenancy + "tenant-#{Tenant.current_schema}" if Rails.application.config.multitenancy end end From da86254fe549b994d1a22d0b5490795623c2626d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 6 Apr 2024 17:48:23 +0200 Subject: [PATCH 6/7] Use comparison validation to validate dates The `validates_comparison_of` method was added in Rails 7.0. We aren't changing the `date_range` validation in polls yet because it's a bit complex; we'll do it in the next commit. --- app/models/legislation/process.rb | 48 +++++++++++++++++++------------ app/models/poll.rb | 29 ++++++++++--------- 2 files changed, 44 insertions(+), 33 deletions(-) diff --git a/app/models/legislation/process.rb b/app/models/legislation/process.rb index a57f53564..f59ddf377 100644 --- a/app/models/legislation/process.rb +++ b/app/models/legislation/process.rb @@ -54,7 +54,35 @@ class Legislation::Process < ApplicationRecord validates :"#{phase_name}_end_date", presence: true, if: enabled_attribute end - validate :valid_date_ranges + validates :end_date, + comparison: { + greater_than_or_equal_to: :start_date, + message: :invalid_date_range + }, + allow_blank: true, + if: -> { start_date } + validates :debate_end_date, + comparison: { + greater_than_or_equal_to: :debate_start_date, + message: :invalid_date_range + }, + allow_blank: true, + if: -> { debate_start_date } + validates :draft_end_date, + comparison: { + greater_than_or_equal_to: :draft_start_date, + message: :invalid_date_range + }, + allow_blank: true, + if: -> { draft_start_date } + validates :allegations_end_date, + comparison: { + greater_than_or_equal_to: :allegations_start_date, + message: :invalid_date_range + }, + allow_blank: true, + if: -> { allegations_start_date } + validates :background_color, format: { allow_blank: true, with: ->(*) { CSS_HEX_COLOR }} validates :font_color, format: { allow_blank: true, with: ->(*) { CSS_HEX_COLOR }} @@ -140,22 +168,4 @@ class Legislation::Process < ApplicationRecord def self.search(terms) pg_search(terms) end - - private - - def valid_date_ranges - if end_date && start_date && end_date < start_date - errors.add(:end_date, :invalid_date_range) - end - if debate_end_date && debate_start_date && debate_end_date < debate_start_date - errors.add(:debate_end_date, :invalid_date_range) - end - if draft_end_date && draft_start_date && draft_end_date < draft_start_date - errors.add(:draft_end_date, :invalid_date_range) - end - if allegations_end_date && allegations_start_date && - allegations_end_date < allegations_start_date - errors.add(:allegations_end_date, :invalid_date_range) - end - end end diff --git a/app/models/poll.rb b/app/models/poll.rb index 8f5da2b3e..5fda6f884 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -34,9 +34,22 @@ class Poll < ApplicationRecord validates_translation :name, presence: true validate :date_range - validate :start_date_is_not_past_date, on: :create + validates :starts_at, + comparison: { + greater_than_or_equal_to: ->(*) { Time.current }, + message: ->(*) { I18n.t("errors.messages.past_date") } + }, + allow_blank: true, + on: :create + validates :ends_at, + comparison: { + greater_than_or_equal_to: ->(*) { Time.current }, + message: ->(*) { I18n.t("errors.messages.past_date") } + }, + on: :update, + if: -> { will_save_change_to_ends_at? } + validate :start_date_change, on: :update - validate :end_date_is_not_past_date, on: :update validate :end_date_change, on: :update validate :only_one_active, unless: :public? @@ -168,12 +181,6 @@ class Poll < ApplicationRecord end end - def start_date_is_not_past_date - if starts_at.present? && starts_at < Time.current - errors.add(:starts_at, I18n.t("errors.messages.past_date")) - end - end - def start_date_change if will_save_change_to_starts_at? if starts_at_in_database < Time.current @@ -184,12 +191,6 @@ class Poll < ApplicationRecord end end - def end_date_is_not_past_date - if will_save_change_to_ends_at? && ends_at < Time.current - errors.add(:ends_at, I18n.t("errors.messages.past_date")) - end - end - def end_date_change if will_save_change_to_ends_at? && ends_at_in_database < Time.current errors.add(:ends_at, I18n.t("errors.messages.cannot_change_date.poll_ended")) From 00c97ad5879720ac7dcc983b91b8dc562b593ea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 12 Apr 2024 22:47:56 +0200 Subject: [PATCH 7/7] Split polls date range validation It was a bit strange to leave the end date blank and have a message associated with the start date, so we're using presence validations instead. For the range validation, we're using the comparison validator included in Rails 7.0. --- app/models/poll.rb | 18 +++++++++++------- spec/models/poll/poll_spec.rb | 4 +++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index 5fda6f884..8b6ec8624 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -33,7 +33,17 @@ class Poll < ApplicationRecord belongs_to :budget validates_translation :name, presence: true - validate :date_range + + validates :starts_at, presence: true + validates :ends_at, presence: true + + validates :starts_at, + comparison: { + less_than_or_equal_to: :ends_at, + message: ->(*) { I18n.t("errors.messages.invalid_date_range") } + }, + allow_blank: true, + if: -> { ends_at } validates :starts_at, comparison: { greater_than_or_equal_to: ->(*) { Time.current }, @@ -175,12 +185,6 @@ class Poll < ApplicationRecord Poll::Voter.where(poll: self, user: user, origin: "web").exists? end - def date_range - if starts_at.blank? || ends_at.blank? || starts_at > ends_at - errors.add(:starts_at, I18n.t("errors.messages.invalid_date_range")) - end - end - def start_date_change if will_save_change_to_starts_at? if starts_at_in_database < Time.current diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index 992c7daa7..d88a1277a 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -24,12 +24,14 @@ describe Poll do poll.starts_at = nil expect(poll).not_to be_valid - expect(poll.errors[:starts_at]).to eq ["Invalid date range"] + expect(poll.errors[:starts_at]).to eq ["can't be blank"] end it "is not valid without an end date" do poll.ends_at = nil + expect(poll).not_to be_valid + expect(poll.errors[:ends_at]).to eq ["can't be blank"] end it "is not valid without a proper start/end date range" do