From 9837b1ab74cb71f21100432885a313626bf2c0c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 28 Apr 2020 15:32:22 +0200 Subject: [PATCH 1/9] Remove tasks to upgrade to version 1.1 These tasks are not needed for new installations, and in existing installations they've already been executed when upgrading to version 1.1. One of them also raises a warning in Rails 5.2: DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "MIN(id) as id". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql() --- config/deploy.rb | 51 +--------------- lib/tasks/budgets.rake | 11 ---- lib/tasks/consul.rake | 10 +-- lib/tasks/local_census_records.rake | 13 ---- lib/tasks/migrations.rake | 20 ------ lib/tasks/secrets.rake | 29 --------- spec/lib/tasks/budgets_spec.rb | 35 ----------- spec/lib/tasks/local_census_records_spec.rb | 36 ----------- spec/lib/tasks/migrations_spec.rb | 68 --------------------- 9 files changed, 2 insertions(+), 271 deletions(-) delete mode 100644 lib/tasks/local_census_records.rake delete mode 100644 lib/tasks/migrations.rake delete mode 100644 lib/tasks/secrets.rake delete mode 100644 spec/lib/tasks/budgets_spec.rb delete mode 100644 spec/lib/tasks/local_census_records_spec.rb delete mode 100644 spec/lib/tasks/migrations_spec.rb diff --git a/config/deploy.rb b/config/deploy.rb index f73c21832..8c53126e2 100644 --- a/config/deploy.rb +++ b/config/deploy.rb @@ -48,11 +48,9 @@ namespace :deploy do after :updating, "rvm1:install:rvm" after :updating, "rvm1:install:ruby" after :updating, "install_bundler_gem" - before "deploy:migrate", "remove_local_census_records_duplicates" after "deploy:migrate", "add_new_settings" - before :publishing, "smtp_ssl_and_delay_jobs_secrets" after :publishing, "setup_puma" after :published, "deploy:restart" @@ -76,16 +74,6 @@ task :install_bundler_gem do end end -task :remove_local_census_records_duplicates do - on roles(:db) do - within release_path do - with rails_env: fetch(:rails_env) do - execute :rake, "local_census_records:remove_duplicates" - end - end - end -end - task :refresh_sitemap do on roles(:app) do within release_path do @@ -116,49 +104,12 @@ task :execute_release_tasks do end end -desc "Create pid and socket folders needed by puma and convert unicorn sockets into symbolic links \ - to the puma socket, so legacy nginx configurations pointing to the unicorn socket keep working" +desc "Create pid and socket folders needed by puma" task :setup_puma do on roles(:app) do with rails_env: fetch(:rails_env) do execute "mkdir -p #{shared_path}/tmp/sockets; true" execute "mkdir -p #{shared_path}/tmp/pids; true" - - if test("[ -e #{shared_path}/tmp/sockets/unicorn.sock ]") - execute "ln -sf #{shared_path}/tmp/sockets/puma.sock #{shared_path}/tmp/sockets/unicorn.sock; true" - end - - if test("[ -e #{shared_path}/sockets/unicorn.sock ]") - execute "ln -sf #{shared_path}/tmp/sockets/puma.sock #{shared_path}/sockets/unicorn.sock; true" - end - end - end -end - -task :smtp_ssl_and_delay_jobs_secrets do - on roles(:app) do - if test("[ -d #{current_path} ]") - within current_path do - with rails_env: fetch(:rails_env) do - tasks_file_path = "lib/tasks/secrets.rake" - shared_secrets_path = "#{shared_path}/config/secrets.yml" - - unless test("[ -e #{current_path}/#{tasks_file_path} ]") - begin - unless test("[ -w #{shared_secrets_path} ]") - execute "sudo chown `whoami` #{shared_secrets_path}" - execute "chmod u+w #{shared_secrets_path}" - end - - execute "cp #{release_path}/#{tasks_file_path} #{current_path}/#{tasks_file_path}" - - execute :rake, "secrets:smtp_ssl_and_delay_jobs" - ensure - execute "rm #{current_path}/#{tasks_file_path}" - end - end - end - end end end end diff --git a/lib/tasks/budgets.rake b/lib/tasks/budgets.rake index 4efe30ee5..172944420 100644 --- a/lib/tasks/budgets.rake +++ b/lib/tasks/budgets.rake @@ -10,15 +10,4 @@ namespace :budgets do Budget.last.email_unselected end end - - desc "Update investments original_heading_id with current heading_id" - task set_original_heading_id: :environment do - ApplicationLogger.new.info "Setting original_heading_id to investments" - Budget::Investment.find_each do |investment| - unless investment.original_heading_id.present? - investment.update_column(:original_heading_id, investment.heading_id) - end - print "." - end - end end diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 4116a8fd0..b831c59f4 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -1,13 +1,5 @@ namespace :consul do desc "Runs tasks needed to upgrade to the latest version" task execute_release_tasks: ["settings:rename_setting_keys", - "settings:add_new_settings", - "execute_release_1.1.0_tasks"] - - desc "Runs tasks needed to upgrade from 1.0.0 to 1.1.0" - task "execute_release_1.1.0_tasks": [ - "budgets:set_original_heading_id", - "migrations:valuation_taggings", - "migrations:budget_admins_and_valuators" - ] + "settings:add_new_settings"] end diff --git a/lib/tasks/local_census_records.rake b/lib/tasks/local_census_records.rake deleted file mode 100644 index 64ad7d383..000000000 --- a/lib/tasks/local_census_records.rake +++ /dev/null @@ -1,13 +0,0 @@ -namespace :local_census_records do - desc "Remove duplicated records from database" - task remove_duplicates: :environment do - ids = LocalCensusRecord.group(:document_type, :document_number).pluck("MIN(id) as id") - duplicates = LocalCensusRecord.count - ids.size - - if duplicates > 0 - ApplicationLogger.new.info "Removing local census records duplicates" - LocalCensusRecord.where("id NOT IN (?)", ids).destroy_all - ApplicationLogger.new.info "Removed #{duplicates} records." - end - end -end diff --git a/lib/tasks/migrations.rake b/lib/tasks/migrations.rake deleted file mode 100644 index f545b620e..000000000 --- a/lib/tasks/migrations.rake +++ /dev/null @@ -1,20 +0,0 @@ -namespace :migrations do - desc "Migrates context of valuation taggings" - task valuation_taggings: :environment do - ApplicationLogger.new.info "Updating valuation taggings context" - Tagging.where(context: "valuation").update_all(context: "valuation_tags") - end - - desc "Migrates budget staff" - task budget_admins_and_valuators: :environment do - ApplicationLogger.new.info "Updating budget administrators and valuators" - Budget.find_each do |budget| - investments = budget.investments.with_hidden - - budget.update!( - administrator_ids: investments.where.not(administrator: nil).distinct.pluck(:administrator_id), - valuator_ids: Budget::ValuatorAssignment.where(investment: investments).distinct.pluck(:valuator_id) - ) - end - end -end diff --git a/lib/tasks/secrets.rake b/lib/tasks/secrets.rake deleted file mode 100644 index 77c1d7344..000000000 --- a/lib/tasks/secrets.rake +++ /dev/null @@ -1,29 +0,0 @@ -namespace :secrets do - desc "Add SMTP, SSL and delay jobs settings to secrets.yml" - task smtp_ssl_and_delay_jobs: :environment do - current_settings = { - "mailer_delivery_method" => ActionMailer::Base.delivery_method, - "smtp_settings" => ActionMailer::Base.smtp_settings, - "force_ssl" => Rails.application.config.force_ssl, - "delay_jobs" => Delayed::Worker.delay_jobs - } - - settings_to_add = current_settings.select do |name, _| - Rails.application.secrets[name].nil? - end - - exit if settings_to_add.empty? - - secrets = Rails.application.config.paths["config/secrets"].first - stream = Psych.parse_stream(File.read(secrets)) - nodes = stream.children.first.children.first - - environment_index = nodes.children.index do |child| - child.is_a?(Psych::Nodes::Scalar) && child.value == Rails.env - end - - nodes.children[environment_index + 1].children.push(*Psych.parse(settings_to_add.to_yaml).children.first.children) - - File.open(secrets, "w") { |file| file.write stream.to_yaml } - end -end diff --git a/spec/lib/tasks/budgets_spec.rb b/spec/lib/tasks/budgets_spec.rb deleted file mode 100644 index f1cd161a6..000000000 --- a/spec/lib/tasks/budgets_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require "rails_helper" - -describe Budget do - let(:run_rake_task) do - Rake::Task["budgets:set_original_heading_id"].reenable - Rake.application.invoke_task("budgets:set_original_heading_id") - end - - it "sets attribute original_heading_id for existing investments" do - heading = create(:budget_heading) - investment = create(:budget_investment, heading: heading) - investment.update!(original_heading_id: nil) - - expect(investment.original_heading_id).to equal(nil) - - run_rake_task - investment.reload - - expect(investment.original_heading_id).to equal(heading.id) - end - - it "does not overwrite original_heading_id when already present" do - original_heading = create(:budget_heading) - new_heading = create(:budget_heading) - investment = create(:budget_investment, heading: original_heading) - investment.update!(heading: new_heading) - - expect(investment.original_heading_id).to eq original_heading.id - - run_rake_task - investment.reload - - expect(investment.original_heading_id).to eq original_heading.id - end -end diff --git a/spec/lib/tasks/local_census_records_spec.rb b/spec/lib/tasks/local_census_records_spec.rb deleted file mode 100644 index cb0e3c00c..000000000 --- a/spec/lib/tasks/local_census_records_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -require "rails_helper" -require Rails.root.join("db", "migrate", "20190530082138_add_unique_index_to_local_census_records") - -describe "LocalCensusRecord tasks" do - let(:run_rake_task) do - Rake::Task["local_census_records:remove_duplicates"].reenable - Rake.application.invoke_task("local_census_records:remove_duplicates") - end - - describe "#remove_duplicates" do - around do |example| - ActiveRecord::Migration.suppress_messages do - example.run - end - end - - before { AddUniqueIndexToLocalCensusRecords.new.down } - after { AddUniqueIndexToLocalCensusRecords.new.up } - - it "Remove all duplicates keeping older records" do - record1 = create(:local_census_record, document_type: "1", document_number: "#DOCUMENT_NUMBER") - record2 = create(:local_census_record, document_type: "2", document_number: "#DOCUMENT_NUMBER") - dup_record1 = build(:local_census_record, document_type: "1", document_number: "#DOCUMENT_NUMBER") - dup_record1.save!(validate: false) - dup_record2 = build(:local_census_record, document_type: "2", document_number: "#DOCUMENT_NUMBER") - dup_record2.save!(validate: false) - record3 = create(:local_census_record, document_type: "3", document_number: "#DOCUMENT_NUMBER") - - expect(LocalCensusRecord.count).to eq(5) - - run_rake_task - - expect(LocalCensusRecord.all).to match_array([record1, record2, record3]) - end - end -end diff --git a/spec/lib/tasks/migrations_spec.rb b/spec/lib/tasks/migrations_spec.rb deleted file mode 100644 index 6e375e50e..000000000 --- a/spec/lib/tasks/migrations_spec.rb +++ /dev/null @@ -1,68 +0,0 @@ -require "rails_helper" - -describe "Migration tasks" do - describe "valuation_taggins" do - let(:run_rake_task) do - Rake::Task["migrations:valuation_taggings"].reenable - Rake.application.invoke_task("migrations:valuation_taggings") - end - - it "updates taggings" do - valuation_tagging = create(:tagging, context: "valuation") - another_valuation_tagging = create(:tagging, context: "valuation") - valuation_tags_tagging = create(:tagging, context: "valuation_tags") - tags_tagging = create(:tagging) - - run_rake_task - - expect(valuation_tagging.reload.context).to eq "valuation_tags" - expect(another_valuation_tagging.reload.context).to eq "valuation_tags" - expect(valuation_tags_tagging.reload.context).to eq "valuation_tags" - expect(tags_tagging.reload.context).to eq "tags" - end - end - - describe "budget_admins_and_valuators" do - let(:run_rake_task) do - Rake::Task["migrations:budget_admins_and_valuators"].reenable - Rake.application.invoke_task("migrations:budget_admins_and_valuators") - end - - let(:old_budget) { create(:budget) } - let(:current_budget) { create(:budget) } - - it "assigns administrators from existing investments" do - harold = create(:administrator) - john = create(:administrator) - root = create(:administrator) - - create(:budget_investment, budget: old_budget, administrator: john) - create(:budget_investment, budget: old_budget, administrator: harold) - create(:budget_investment, budget: old_budget, administrator: nil) - - create(:budget_investment, budget: current_budget, administrator: root) - - run_rake_task - - expect(old_budget.administrators).to match_array [john, harold] - expect(current_budget.administrators).to match_array [root] - end - - it "assigns valuators from existing investments" do - tyrion = create(:valuator) - cersei = create(:valuator) - jaime = create(:valuator) - - create(:budget_investment, budget: old_budget, valuators: [cersei]) - create(:budget_investment, budget: old_budget, valuators: [jaime, cersei]) - create(:budget_investment, budget: old_budget, valuators: []) - - create(:budget_investment, budget: current_budget, valuators: [tyrion, jaime]) - - run_rake_task - - expect(old_budget.valuators).to match_array [cersei, jaime] - expect(current_budget.valuators).to match_array [tyrion, jaime] - end - end -end From 6fd9a286d781a37a8c52997f3b37759039057b01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 May 2020 19:13:54 +0200 Subject: [PATCH 2/9] Don't access the database in `after_initialize` Rails 5.2 crashes in the `db:create` task because it tries to run the `after_initialize` block before the database is created. The easiest way to solve it is to move the code out of the initializer and calculate the API type definitions on demand. Note results are still cached using a class instance variable (not to be confused with a class variable), and so once definitions are obtained, they will remain constant until the application is restarted, even in the development environment. --- app/controllers/graphql_controller.rb | 2 +- config/application.rb | 1 - config/initializers/graphql_api.rb | 11 ----------- lib/graph_ql/api_types_creator.rb | 6 +++++- spec/lib/graph_ql/query_type_creator_spec.rb | 14 ++++++++------ spec/lib/graphql_spec.rb | 2 +- 6 files changed, 15 insertions(+), 21 deletions(-) delete mode 100644 config/initializers/graphql_api.rb diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index d8233a685..5b1c27d16 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -29,7 +29,7 @@ class GraphqlController < ApplicationController private def consul_schema - api_types = GraphQL::ApiTypesCreator.create(API_TYPE_DEFINITIONS) + api_types = GraphQL::ApiTypesCreator.create query_type = GraphQL::QueryTypeCreator.create(api_types) GraphQL::Schema.define do diff --git a/config/application.rb b/config/application.rb index bb5a7580b..a44e143ca 100644 --- a/config/application.rb +++ b/config/application.rb @@ -60,7 +60,6 @@ module Consul config.after_initialize do Globalize.set_fallbacks_to_all_available_locales - GraphQLApi::Loader.setup end config.assets.paths << Rails.root.join("app", "assets", "fonts") diff --git a/config/initializers/graphql_api.rb b/config/initializers/graphql_api.rb deleted file mode 100644 index cf7b4bc03..000000000 --- a/config/initializers/graphql_api.rb +++ /dev/null @@ -1,11 +0,0 @@ -module GraphQLApi - class Loader - def self.setup - if ActiveRecord::Base.connection.tables.any? - api_config = YAML.load_file("./config/api.yml") - GraphqlController.const_set "API_TYPE_DEFINITIONS", - GraphQL::ApiTypesCreator.parse_api_config_file(api_config) - end - end - end -end diff --git a/lib/graph_ql/api_types_creator.rb b/lib/graph_ql/api_types_creator.rb index 823e430a8..3fbb3d704 100644 --- a/lib/graph_ql/api_types_creator.rb +++ b/lib/graph_ql/api_types_creator.rb @@ -10,7 +10,7 @@ module GraphQL string: GraphQL::STRING_TYPE }.freeze - def self.create(api_types_definitions) + def self.create created_types = {} api_types_definitions.each do |model, info| create_type(model, info[:fields], created_types) @@ -18,6 +18,10 @@ module GraphQL created_types end + def self.api_types_definitions + @api_types_definitions ||= parse_api_config_file(YAML.load_file(Rails.root.join("config/api.yml"))) + end + def self.type_kind(type) if SCALAR_TYPES[type] :scalar diff --git a/spec/lib/graph_ql/query_type_creator_spec.rb b/spec/lib/graph_ql/query_type_creator_spec.rb index 7a56666d0..36d671c5c 100644 --- a/spec/lib/graph_ql/query_type_creator_spec.rb +++ b/spec/lib/graph_ql/query_type_creator_spec.rb @@ -1,13 +1,15 @@ require "rails_helper" describe GraphQL::QueryTypeCreator do - let(:api_type_definitions) do - { - ProposalNotification => { fields: { title: :string }}, - Proposal => { fields: { id: :integer, title: :string }} - } + before do + allow(GraphQL::ApiTypesCreator).to receive(:api_types_definitions).and_return( + { + ProposalNotification => { fields: { title: :string }}, + Proposal => { fields: { id: :integer, title: :string }} + } + ) end - let(:api_types) { GraphQL::ApiTypesCreator.create(api_type_definitions) } + let(:api_types) { GraphQL::ApiTypesCreator.create } describe "::create" do let(:query_type) { GraphQL::QueryTypeCreator.create(api_types) } diff --git a/spec/lib/graphql_spec.rb b/spec/lib/graphql_spec.rb index 1a2efcce8..0675c0a66 100644 --- a/spec/lib/graphql_spec.rb +++ b/spec/lib/graphql_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -api_types = GraphQL::ApiTypesCreator.create(GraphqlController::API_TYPE_DEFINITIONS) +api_types = GraphQL::ApiTypesCreator.create query_type = GraphQL::QueryTypeCreator.create(api_types) ConsulSchema = GraphQL::Schema.define do query query_type From d7d421b88f1a1cf1ee5c8d5347691f27cd84a8b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 May 2020 23:12:06 +0200 Subject: [PATCH 3/9] Rename columns with a slash in their names These columns were causing Rails 5.2 to throw a warning when ordering by them, as if they weren't valid column names: DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): :"budget/investments_count". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). This change also makes their names consistent with the rest of our tables and columns. --- app/models/tag_cloud.rb | 2 +- config/initializers/acts_as_taggable_on.rb | 2 +- ...519120717_rename_columns_with_slash_characters.rb | 9 +++++++++ db/schema.rb | 12 ++++++------ 4 files changed, 17 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20200519120717_rename_columns_with_slash_characters.rb diff --git a/app/models/tag_cloud.rb b/app/models/tag_cloud.rb index f76275558..2008685b3 100644 --- a/app/models/tag_cloud.rb +++ b/app/models/tag_cloud.rb @@ -31,6 +31,6 @@ class TagCloud end def table_name - resource_model.to_s.downcase.pluralize.gsub("::", "/") + resource_model.to_s.tableize.tr("/", "_") end end diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb index f1cff9c31..df1453908 100644 --- a/config/initializers/acts_as_taggable_on.rb +++ b/config/initializers/acts_as_taggable_on.rb @@ -79,7 +79,7 @@ module ActsAsTaggableOn private def custom_counter_field_name_for(taggable_type) - "#{taggable_type.underscore.pluralize}_count" + "#{taggable_type.tableize.tr("/", "_")}_count" end end end diff --git a/db/migrate/20200519120717_rename_columns_with_slash_characters.rb b/db/migrate/20200519120717_rename_columns_with_slash_characters.rb new file mode 100644 index 000000000..0884c421a --- /dev/null +++ b/db/migrate/20200519120717_rename_columns_with_slash_characters.rb @@ -0,0 +1,9 @@ +class RenameColumnsWithSlashCharacters < ActiveRecord::Migration[5.1] + def change + change_table :tags do |t| + t.rename :"budget/investments_count", :budget_investments_count + t.rename :"legislation/proposals_count", :legislation_proposals_count + t.rename :"legislation/processes_count", :legislation_processes_count + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 57012aa3c..573774563 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20191108173350) do +ActiveRecord::Schema.define(version: 20200519120717) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1407,12 +1407,12 @@ ActiveRecord::Schema.define(version: 20191108173350) do t.integer "debates_count", default: 0 t.integer "proposals_count", default: 0 t.string "kind" - t.integer "budget/investments_count", default: 0 - t.integer "legislation/proposals_count", default: 0 - t.integer "legislation/processes_count", default: 0 + t.integer "budget_investments_count", default: 0 + t.integer "legislation_proposals_count", default: 0 + t.integer "legislation_processes_count", default: 0 t.index ["debates_count"], name: "index_tags_on_debates_count" - t.index ["legislation/processes_count"], name: "index_tags_on_legislation/processes_count" - t.index ["legislation/proposals_count"], name: "index_tags_on_legislation/proposals_count" + t.index ["legislation_processes_count"], name: "index_tags_on_legislation_processes_count" + t.index ["legislation_proposals_count"], name: "index_tags_on_legislation_proposals_count" t.index ["name"], name: "index_tags_on_name", unique: true t.index ["proposals_count"], name: "index_tags_on_proposals_count" end From f6351819fa3542bc253562cf928a9656d25a9bb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 19 May 2020 14:41:12 +0200 Subject: [PATCH 4/9] Simplify SQL using DISTINCT Using `pluck("DISTINCT")` was raising a warning in Rails 5.2: DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "DISTINCT taggings.tag_id". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). Since there was only one other use of distinct, I've decided to change both of them in the same commit, even if the second one wasn't raising a warning. --- config/initializers/acts_as_taggable_on.rb | 2 +- lib/user_segments.rb | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb index df1453908..940f411b2 100644 --- a/config/initializers/acts_as_taggable_on.rb +++ b/config/initializers/acts_as_taggable_on.rb @@ -37,7 +37,7 @@ module ActsAsTaggableOn scope :public_for_api, -> do where("(tags.kind IS NULL or tags.kind = ?) and tags.id in (?)", "category", - Tagging.public_for_api.pluck("DISTINCT taggings.tag_id")) + Tagging.public_for_api.distinct.pluck("taggings.tag_id")) end include PgSearch diff --git a/lib/user_segments.rb b/lib/user_segments.rb index ae6d345be..7927b8bfd 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -46,10 +46,8 @@ class UserSegments def self.not_supported_on_current_budget author_ids( User.where( - "id NOT IN (SELECT DISTINCT(voter_id) FROM votes"\ - " WHERE votable_type = ? AND votes.votable_id IN (?))", - "Budget::Investment", - current_budget_investments.pluck(:id) + "id NOT IN (?)", + Vote.select(:voter_id).where(votable: current_budget_investments).distinct ) ) end From f427c757ba15e3a2efdd39ca67d95d4147cddcda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 19 May 2020 15:50:42 +0200 Subject: [PATCH 5/9] Use hash conditions instead of SQL's `IN` This is what we're doing in most places. --- app/models/budget/investment.rb | 4 ++-- app/models/community.rb | 4 +--- app/models/user.rb | 6 ++++-- spec/models/community_spec.rb | 2 ++ 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index f12bcee70..fc4d31ff7 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -167,7 +167,7 @@ class Budget ids += results.where(selected: true).pluck(:id) if params[:advanced_filters].include?("selected") ids += results.undecided.pluck(:id) if params[:advanced_filters].include?("undecided") ids += results.unfeasible.pluck(:id) if params[:advanced_filters].include?("unfeasible") - results = results.where("budget_investments.id IN (?)", ids) if ids.any? + results = results.where(id: ids) if ids.any? results end @@ -194,7 +194,7 @@ class Budget ids += Investment.where(heading_id: hid).order(confidence_score: :desc).limit(max_per_heading).pluck(:id) end - results.where("budget_investments.id IN (?)", ids) + results.where(id: ids) end def self.search_by_title_or_id(title_or_id) diff --git a/app/models/community.rb b/app/models/community.rb index 3a40b682c..48a6029d3 100644 --- a/app/models/community.rb +++ b/app/models/community.rb @@ -45,9 +45,7 @@ class Community < ApplicationRecord private def users_who_commented - topics_ids = topics.pluck(:id) - query = "comments.commentable_id IN (?)and comments.commentable_type = 'Topic'" - User.by_comments(query, topics_ids) + User.by_comments(topics) end def users_who_topics_author diff --git a/app/models/user.rb b/app/models/user.rb index 4bf778d91..ac530d3a1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -108,8 +108,10 @@ class User < ApplicationRecord scope :active, -> { where(erased_at: nil) } scope :erased, -> { where.not(erased_at: nil) } scope :public_for_api, -> { all } - scope :by_comments, ->(query, topics_ids) { joins(:comments).where(query, topics_ids).distinct } - scope :by_authors, ->(author_ids) { where("users.id IN (?)", author_ids) } + scope :by_authors, ->(author_ids) { where(id: author_ids) } + scope :by_comments, ->(commentables) do + joins(:comments).where("comments.commentable": commentables).distinct + end scope :by_username_email_or_document_number, ->(search_string) do string = "%#{search_string}%" where("username ILIKE ? OR email ILIKE ? OR document_number ILIKE ?", string, string, string) diff --git a/spec/models/community_spec.rb b/spec/models/community_spec.rb index 7354bc717..072e0cb36 100644 --- a/spec/models/community_spec.rb +++ b/spec/models/community_spec.rb @@ -13,11 +13,13 @@ RSpec.describe Community, type: :model do community = proposal.community user1 = create(:user) user2 = create(:user) + user3 = create(:user) topic1 = create(:topic, community: community, author: user1) create(:comment, commentable: topic1, author: user1) create(:comment, commentable: topic1, author: user2) create(:topic, community: community, author: user2) + create(:comment, commentable: proposal, author: user3) expect(community.participants).to match_array [user1, user2, proposal.author] end From 1b34c061bb36502e38c30718973b84a9adef4ea7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 19 May 2020 16:03:32 +0200 Subject: [PATCH 6/9] Use `where.not` instead of `where(NOT IN)` This way we simplify the code a bit and reduce our usage of raw SQL. --- app/controllers/proposals_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- db/dev_seeds/votes.rb | 2 +- lib/user_segments.rb | 7 +++---- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/controllers/proposals_controller.rb b/app/controllers/proposals_controller.rb index b09ce01a2..e16691abf 100644 --- a/app/controllers/proposals_controller.rb +++ b/app/controllers/proposals_controller.rb @@ -158,7 +158,7 @@ class ProposalsController < ApplicationController .sort_by_confidence_score.limit(Setting["featured_proposals_number"]) if @featured_proposals.present? set_featured_proposal_votes(@featured_proposals) - @resources = @resources.where("proposals.id NOT IN (?)", @featured_proposals.map(&:id)) + @resources = @resources.where.not(id: @featured_proposals) end end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fdec3f4bb..a50d55cf6 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -96,7 +96,7 @@ class UsersController < ApplicationController disabled_commentables << "Debate" unless Setting["process.debates"] disabled_commentables << "Budget::Investment" unless Setting["process.budgets"] if disabled_commentables.present? - all_user_comments.where("commentable_type NOT IN (?)", disabled_commentables) + all_user_comments.where.not(commentable_type: disabled_commentables) else all_user_comments end diff --git a/db/dev_seeds/votes.rb b/db/dev_seeds/votes.rb index 357d2de59..749df9be8 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(["users.id NOT IN(?)", User.organizations.pluck(:id)]) + not_org_users = User.where.not(id: User.organizations) 100.times do voter = not_org_users.level_two_or_three_verified.all.sample vote = [true, false].sample diff --git a/lib/user_segments.rb b/lib/user_segments.rb index 7927b8bfd..efa0d51a4 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -45,10 +45,9 @@ class UserSegments def self.not_supported_on_current_budget author_ids( - User.where( - "id NOT IN (?)", - Vote.select(:voter_id).where(votable: current_budget_investments).distinct - ) + User.where.not( + id: Vote.select(:voter_id).where(votable: current_budget_investments).distinct + ) ) end From 057679248f4353080c23bac3dcc431745afc90da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 19 May 2020 16:18:56 +0200 Subject: [PATCH 7/9] Use `be_not_found` instead of `be_missing` We were getting a deprecation message in Rails 5.2: The missing? predicate is deprecated and will be removed in Rails 6.0. Please use not_found? as provided by Rack::Response::Helpers --- spec/controllers/pages_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/pages_controller_spec.rb b/spec/controllers/pages_controller_spec.rb index ed46a77b0..c8447055e 100644 --- a/spec/controllers/pages_controller_spec.rb +++ b/spec/controllers/pages_controller_spec.rb @@ -38,12 +38,12 @@ describe PagesController do describe "Not found pages" do it "returns a 404 message" do get :show, params: { id: "nonExistentPage" } - expect(response).to be_missing + expect(response).to be_not_found end it "returns a 404 message for a JavaScript request" do get :show, params: { id: "nonExistentJavaScript.js" } - expect(response).to be_missing + expect(response).to be_not_found end end end From 17f442c7237878e46c708f1ee80ddfef112b7d35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 21 May 2020 03:04:51 +0200 Subject: [PATCH 8/9] Extract method to get a few random records In Ruby 5.2, we get a warning when using the "RANDOM()" function: DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "RANDOM()". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). This warning doesn't make much sense, though, since RANDOM() is a common function which is not dangerous at all. However, since the warning is annoying, we'll probably have to find a way to deal with it. So I'm extracting all our RANDOM() usages into a method. This way we'll only have to change one method to avoid this warning. I've chosen `sample` because it's similar to Ruby's Array#sample, and because `order_by_random` would be confusing if we consider we already have a method called `sort_by_random`. --- app/models/application_record.rb | 8 ++++++++ db/dev_seeds/budgets.rb | 2 +- db/dev_seeds/flags.rb | 6 +++--- db/dev_seeds/hiddings.rb | 12 ++++++------ db/dev_seeds/notifications.rb | 4 ++-- db/dev_seeds/polls.rb | 2 +- 6 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 10a4cba84..4c7aa15dc 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -1,3 +1,11 @@ class ApplicationRecord < ActiveRecord::Base self.abstract_class = true + + def self.sample(count = 1) + if count == 1 + reorder("RANDOM()").first + else + reorder("RANDOM()").limit(count) + end + end end diff --git a/db/dev_seeds/budgets.rb b/db/dev_seeds/budgets.rb index a1e4738f2..e8a1269bd 100644 --- a/db/dev_seeds/budgets.rb +++ b/db/dev_seeds/budgets.rb @@ -147,7 +147,7 @@ end section "Marking investments as visible to valuators" do (1..50).to_a.sample.times do - Budget::Investment.reorder("RANDOM()").first.update(visible_to_valuators: true) + Budget::Investment.sample.update(visible_to_valuators: true) end end diff --git a/db/dev_seeds/flags.rb b/db/dev_seeds/flags.rb index c57cac524..ebddb901a 100644 --- a/db/dev_seeds/flags.rb +++ b/db/dev_seeds/flags.rb @@ -19,7 +19,7 @@ section "Flagging Debates & Comments" do end section "Ignoring flags in Debates, comments & proposals" do - Debate.flagged.reorder("RANDOM()").limit(10).each(&:ignore_flag) - Comment.flagged.reorder("RANDOM()").limit(30).each(&:ignore_flag) - Proposal.flagged.reorder("RANDOM()").limit(10).each(&:ignore_flag) + Debate.flagged.sample(10).each(&:ignore_flag) + Comment.flagged.sample(30).each(&:ignore_flag) + Proposal.flagged.sample(10).each(&:ignore_flag) end diff --git a/db/dev_seeds/hiddings.rb b/db/dev_seeds/hiddings.rb index ed1d938d2..d91f5a886 100644 --- a/db/dev_seeds/hiddings.rb +++ b/db/dev_seeds/hiddings.rb @@ -1,11 +1,11 @@ section "Hiding debates, comments & proposals" do - Comment.with_hidden.flagged.reorder("RANDOM()").limit(30).each(&:hide) - Debate.with_hidden.flagged.reorder("RANDOM()").limit(5).each(&:hide) - Proposal.with_hidden.flagged.reorder("RANDOM()").limit(10).each(&:hide) + Comment.with_hidden.flagged.sample(30).each(&:hide) + Debate.with_hidden.flagged.sample(5).each(&:hide) + Proposal.with_hidden.flagged.sample(10).each(&:hide) end section "Confirming hiding in debates, comments & proposals" do - Comment.only_hidden.flagged.reorder("RANDOM()").limit(10).each(&:confirm_hide) - Debate.only_hidden.flagged.reorder("RANDOM()").limit(5).each(&:confirm_hide) - Proposal.only_hidden.flagged.reorder("RANDOM()").limit(5).each(&:confirm_hide) + Comment.only_hidden.flagged.sample(10).each(&:confirm_hide) + Debate.only_hidden.flagged.sample(5).each(&:confirm_hide) + Proposal.only_hidden.flagged.sample(5).each(&:confirm_hide) end diff --git a/db/dev_seeds/notifications.rb b/db/dev_seeds/notifications.rb index 5452c7f79..237d42215 100644 --- a/db/dev_seeds/notifications.rb +++ b/db/dev_seeds/notifications.rb @@ -4,10 +4,10 @@ section "Creating comment notifications" do title: Faker::Lorem.sentence(3).truncate(60), description: "

#{Faker::Lorem.paragraphs.join("

")}

", tag_list: Tag.all.sample(3).join(","), - geozone: Geozone.reorder("RANDOM()").first, + geozone: Geozone.sample, terms_of_service: "1") - comment = Comment.create!(user: User.reorder("RANDOM()").first, + comment = Comment.create!(user: User.sample, body: Faker::Lorem.sentence, commentable: debate) diff --git a/db/dev_seeds/polls.rb b/db/dev_seeds/polls.rb index e26c99d7d..9813865dc 100644 --- a/db/dev_seeds/polls.rb +++ b/db/dev_seeds/polls.rb @@ -13,7 +13,7 @@ section "Creating polls" do starts_at: 5.days.ago, ends_at: 5.days.from_now, geozone_restricted: true, - geozones: Geozone.reorder("RANDOM()").limit(3)) + geozones: Geozone.sample(3)) Poll.create!(name: I18n.t("seeds.polls.recounting_poll"), slug: I18n.t("seeds.polls.recounting_poll").parameterize, From 9318c4f1e9367626196fe4866b3a6e9d77b95bc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 May 2020 14:26:53 +0200 Subject: [PATCH 9/9] Bump pg_search from 2.0.1 to 2.3.0 Using pg_search 2.0.1 with Rails 5.2 results in deprecation warnings: DEPRECATION WARNING: Dangerous query method (method whose arguments used as raw SQL) called with non-attribute argument(s): "pg_search_978c2f8941354cf552831b.rank DESC, \"tags\".\"id\" ASC". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). We're not upgrading to the latest pg_search because it only supports ActiveRecord >= 5.2. --- Gemfile | 2 +- Gemfile.lock | 5 ++--- app/models/concerns/searchable.rb | 2 +- config/initializers/acts_as_taggable_on.rb | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index 6b90aebc9..7fab9992d 100644 --- a/Gemfile +++ b/Gemfile @@ -40,7 +40,7 @@ gem "omniauth-twitter", "~> 1.4.0" gem "paperclip", "~> 5.2.1" gem "paranoia", "~> 2.4.2" gem "pg", "~> 0.21.0" -gem "pg_search", "~> 2.0.1" +gem "pg_search", "~> 2.3.0" gem "puma", "~> 4.3.5" gem "recipient_interceptor", "~> 0.2.0" gem "redcarpet", "~> 3.4.0" diff --git a/Gemfile.lock b/Gemfile.lock index ee8adbee6..20329b10a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -396,10 +396,9 @@ GEM parser (2.7.1.3) ast (~> 2.4.0) pg (0.21.0) - pg_search (2.0.1) + pg_search (2.3.0) activerecord (>= 4.2) activesupport (>= 4.2) - arel (>= 6) public_suffix (4.0.1) puma (4.3.5) nio4r (~> 2.0) @@ -665,7 +664,7 @@ DEPENDENCIES paperclip (~> 5.2.1) paranoia (~> 2.4.2) pg (~> 0.21.0) - pg_search (~> 2.0.1) + pg_search (~> 2.3.0) puma (~> 4.3.5) rails (= 5.1.7) rails-assets-leaflet! diff --git a/app/models/concerns/searchable.rb b/app/models/concerns/searchable.rb index f6e5a8c21..3958288a6 100644 --- a/app/models/concerns/searchable.rb +++ b/app/models/concerns/searchable.rb @@ -2,7 +2,7 @@ module Searchable extend ActiveSupport::Concern included do - include PgSearch + include PgSearch::Model include SearchCache pg_search_scope :pg_search, ->(query) do diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb index 940f411b2..16d3d6845 100644 --- a/config/initializers/acts_as_taggable_on.rb +++ b/config/initializers/acts_as_taggable_on.rb @@ -40,7 +40,7 @@ module ActsAsTaggableOn Tagging.public_for_api.distinct.pluck("taggings.tag_id")) end - include PgSearch + include PgSearch::Model pg_search_scope :pg_search, against: :name, using: {