From 9c9573553418d95274d37551964ee8f9c30d229a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 18 Sep 2021 15:03:09 +0200 Subject: [PATCH 1/5] Remove invalid key in `before_destroy` The `only:` key does not apply to model callbacks. It was added in commit 1077e25b2, probably by accident. Using this key raises an error in Rails 6.0. --- app/models/poll/booth_assignment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/poll/booth_assignment.rb b/app/models/poll/booth_assignment.rb index e3529e06a..3af585e77 100644 --- a/app/models/poll/booth_assignment.rb +++ b/app/models/poll/booth_assignment.rb @@ -5,7 +5,7 @@ class Poll delegate :name, to: :booth - before_destroy :destroy_poll_shifts, only: :destroy + before_destroy :destroy_poll_shifts has_many :officer_assignments, dependent: :destroy has_many :officers, through: :officer_assignments From 94d4e1db1e52639ab07d7139a2283efc125c3605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 18 Sep 2021 15:45:46 +0200 Subject: [PATCH 2/5] Use `render template:` instead of `render file:` We did a similar change in commit 47925fbab, and we were getting a warning in Rails 6.0: DEPRECATION WARNING: render file: should be given the absolute path to a file Since using `render file:` would ignore views in custom folders, we're using `render template:` instead. --- app/views/proposals/created.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/proposals/created.html.erb b/app/views/proposals/created.html.erb index 6dba6ab68..1c9ed9fae 100644 --- a/app/views/proposals/created.html.erb +++ b/app/views/proposals/created.html.erb @@ -27,4 +27,4 @@ -<%= render file: "proposals/show.html.erb", locals: { preview: true } %> +<%= render template: "proposals/show", locals: { preview: true } %> From effd646e5462319f4a06eca5e229cffad06a46dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 00:09:18 +0200 Subject: [PATCH 3/5] Update the way we create form builders in tests We were getting a warning in Rails 6: DEPRECATION WARNING: ActionView::Base instances should be constructed with a lookup context, assignments, and a controller. --- spec/components/sdg/related_list_selector_component_spec.rb | 2 +- spec/lib/consul_form_builder_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/components/sdg/related_list_selector_component_spec.rb b/spec/components/sdg/related_list_selector_component_spec.rb index f06280fa0..07f5893b1 100644 --- a/spec/components/sdg/related_list_selector_component_spec.rb +++ b/spec/components/sdg/related_list_selector_component_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" describe SDG::RelatedListSelectorComponent do let(:debate) { create(:debate) } - let(:form) { ConsulFormBuilder.new(:debate, debate, ActionView::Base.new, {}) } + let(:form) { ConsulFormBuilder.new(:debate, debate, ApplicationController.new.view_context, {}) } let(:component) { SDG::RelatedListSelectorComponent.new(form) } before do diff --git a/spec/lib/consul_form_builder_spec.rb b/spec/lib/consul_form_builder_spec.rb index ed73b6b62..a61e984da 100644 --- a/spec/lib/consul_form_builder_spec.rb +++ b/spec/lib/consul_form_builder_spec.rb @@ -11,7 +11,7 @@ describe ConsulFormBuilder do stub_const("DummyModel::OPTIONS", %w[Good Bad Ugly].freeze) end - let(:builder) { ConsulFormBuilder.new(:dummy, DummyModel.new, ActionView::Base.new, {}) } + let(:builder) { ConsulFormBuilder.new(:dummy, DummyModel.new, ApplicationController.new.view_context, {}) } describe "hints" do it "does not generate hints by default" do From 2e863fdc51b72d4dcf2fb7b7b16118506ad22a73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 02:01:26 +0200 Subject: [PATCH 4/5] Fix OR condition in public taggings In SQL, conditions like: ``` tag_id IN (x) AND taggable_type='Debate' OR taggable_type='Proposal' ``` Don't work as intended; we need to write: ``` tag_id IN (x) AND (taggable_type='Debate' OR taggable_type='Proposal') ``` Due to this bug, we were returning taggings for proposals without intending to do so. Since the code was very hard to read, we're also simplifying it. --- config/initializers/acts_as_taggable_on.rb | 17 ++++++++--------- spec/models/tagging_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-) create mode 100644 spec/models/tagging_spec.rb diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb index 16d3d6845..2dac4e959 100644 --- a/config/initializers/acts_as_taggable_on.rb +++ b/config/initializers/acts_as_taggable_on.rb @@ -4,12 +4,10 @@ module ActsAsTaggableOn after_destroy :touch_taggable, :decrement_tag_custom_counter scope :public_for_api, -> do - where(%{taggings.tag_id in (?) and - (taggings.taggable_type = 'Debate' and taggings.taggable_id in (?)) or - (taggings.taggable_type = 'Proposal' and taggings.taggable_id in (?))}, - Tag.where("kind IS NULL or kind = ?", "category").pluck(:id), - Debate.public_for_api.pluck(:id), - Proposal.public_for_api.pluck(:id)) + where( + tag: Tag.where(kind: [nil, "category"]), + taggable: [Debate.public_for_api, Proposal.public_for_api] + ) end def touch_taggable @@ -35,9 +33,10 @@ module ActsAsTaggableOn include Graphqlable scope :public_for_api, -> do - where("(tags.kind IS NULL or tags.kind = ?) and tags.id in (?)", - "category", - Tagging.public_for_api.distinct.pluck("taggings.tag_id")) + where( + kind: [nil, "category"], + id: Tagging.public_for_api.distinct.pluck(:tag_id) + ) end include PgSearch::Model diff --git a/spec/models/tagging_spec.rb b/spec/models/tagging_spec.rb new file mode 100644 index 000000000..7fc7c0938 --- /dev/null +++ b/spec/models/tagging_spec.rb @@ -0,0 +1,21 @@ +require "rails_helper" + +describe Tagging do + describe ".public_for_api" do + it "returns taggings for debates and proposals" do + create(:tag, name: "Health", kind: nil) + debate = create(:debate, tag_list: "Health") + proposal = create(:proposal, tag_list: "Health") + + expect(Tagging.public_for_api.map(&:taggable)).to match_array [debate, proposal] + end + + it "does not return taggings for other tag kinds" do + create(:tag, name: "Health", kind: "custom") + create(:debate, tag_list: "Health") + create(:proposal, tag_list: "Health") + + expect(Tagging.public_for_api.map(&:taggable)).to be_empty + end + end +end From dba68c2c041cbef8b7c6a27ec06591eec352d63f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 Sep 2021 02:09:16 +0200 Subject: [PATCH 5/5] Fix deprecation warning in Tagging monkey patch We were getting a warning in Rails 6.0: DEPRECATION WARNING: Class level methods will no longer inherit scoping from `public_for_api` in Rails 6.1. To continue using the scoped relation, pass it into the block directly. To instead access the full set of models, as Rails 6.1 will, use `ActsAsTaggableOn::Tag.default_scoped`. --- config/initializers/acts_as_taggable_on.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb index 2dac4e959..d9ec982a4 100644 --- a/config/initializers/acts_as_taggable_on.rb +++ b/config/initializers/acts_as_taggable_on.rb @@ -5,7 +5,8 @@ module ActsAsTaggableOn scope :public_for_api, -> do where( - tag: Tag.where(kind: [nil, "category"]), + # TODO: remove default_scoped after upgrading to Rails 6.1 + tag: Tag.default_scoped.where(kind: [nil, "category"]), taggable: [Debate.public_for_api, Proposal.public_for_api] ) end