From d3f72b100e81632e542d750bc76d16097a5069cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 20 Jan 2021 19:15:43 +0100 Subject: [PATCH 1/5] Allow target and local target comparison This way we'll be able to sort a mixed collection of targets and local targets. --- app/models/sdg/local_target.rb | 10 ++++++---- app/models/sdg/target.rb | 10 ++++++---- spec/models/sdg/local_target_spec.rb | 8 ++++++++ spec/models/sdg/target_spec.rb | 26 ++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/app/models/sdg/local_target.rb b/app/models/sdg/local_target.rb index 8443e2a40..a7328955c 100644 --- a/app/models/sdg/local_target.rb +++ b/app/models/sdg/local_target.rb @@ -17,10 +17,12 @@ class SDG::LocalTarget < ApplicationRecord belongs_to :target - def <=>(local_target) - return unless local_target.class == self.class - - [target, numeric_subcode] <=> [local_target.target, local_target.numeric_subcode] + def <=>(any_target) + if any_target.class == self.class + [target, numeric_subcode] <=> [any_target.target, any_target.numeric_subcode] + elsif any_target.class == target.class + -1 * (any_target <=> self) + end end protected diff --git a/app/models/sdg/target.rb b/app/models/sdg/target.rb index 6ff87d155..ee1e96a45 100644 --- a/app/models/sdg/target.rb +++ b/app/models/sdg/target.rb @@ -12,10 +12,12 @@ class SDG::Target < ApplicationRecord I18n.t("sdg.goals.goal_#{goal.code}.targets.target_#{code_key}.title") end - def <=>(target) - return unless target.class == self.class - - [goal.code, numeric_subcode] <=> [target.goal.code, target.numeric_subcode] + def <=>(any_target) + if any_target.class == self.class + [goal.code, numeric_subcode] <=> [any_target.goal.code, any_target.numeric_subcode] + elsif any_target.class.name == "SDG::LocalTarget" + [self, -1] <=> [any_target.target, 1] + end end def self.[](code) diff --git a/spec/models/sdg/local_target_spec.rb b/spec/models/sdg/local_target_spec.rb index 24c51c683..523baa396 100644 --- a/spec/models/sdg/local_target_spec.rb +++ b/spec/models/sdg/local_target_spec.rb @@ -73,5 +73,13 @@ describe SDG::LocalTarget do expect(local_target).to be > lesser_local_target expect(local_target).to be < greater_local_target end + + it "can be compared against global targets" do + lesser_target = build(:sdg_target, code: "10.A", goal: SDG::Goal[10]) + greater_target = build(:sdg_target, code: "11.1", goal: SDG::Goal[11]) + + expect(local_target).to be > lesser_target + expect(local_target).to be < greater_target + end end end diff --git a/spec/models/sdg/target_spec.rb b/spec/models/sdg/target_spec.rb index 3dda9cd8d..d227d9954 100644 --- a/spec/models/sdg/target_spec.rb +++ b/spec/models/sdg/target_spec.rb @@ -61,6 +61,32 @@ describe SDG::Target do expect(target).to be > lesser_target expect(target).to be < greater_target end + + context "comparing with a local target" do + it "compares using the goal first" do + lesser_local_target = build(:sdg_local_target, code: "2.1.1") + greater_local_target = build(:sdg_local_target, code: "11.1.2") + + expect(target).to be > lesser_local_target + expect(target).to be < greater_local_target + end + + it "compares using the target when the goal is the same" do + lesser_target = build(:sdg_target, code: "10.2", goal: goal) + greater_target = build(:sdg_target, code: "10.A", goal: goal) + lesser_local_target = build(:sdg_local_target, code: "10.2.25", target: lesser_target) + greater_local_target = build(:sdg_local_target, code: "10.A.1", target: greater_target) + + expect(target).to be > lesser_local_target + expect(target).to be < greater_local_target + end + + it "is smaller than a local target belonging to it" do + local_target = build(:sdg_local_target, target: target, code: "10.19.1") + + expect(target).to be < local_target + end + end end describe ".[]" do From 14e4c528e97e91081997d926183a81778be1a95d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 20 Jan 2021 19:41:50 +0100 Subject: [PATCH 2/5] Allow filtering by local target in SDG management --- .../sdg_management/relations/index_component.rb | 4 +++- app/models/concerns/sdg/relatable.rb | 6 +++++- spec/models/sdg/relatable_spec.rb | 13 +++++++++++++ spec/system/sdg_management/relations_spec.rb | 15 +++++++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/components/sdg_management/relations/index_component.rb b/app/components/sdg_management/relations/index_component.rb index 9c66e85e8..41eb4a26f 100644 --- a/app/components/sdg_management/relations/index_component.rb +++ b/app/components/sdg_management/relations/index_component.rb @@ -53,6 +53,8 @@ class SDGManagement::Relations::IndexComponent < ApplicationComponent end def target_options - options_from_collection_for_select(SDG::Target.all.sort, :code, :code, params[:target_code]) + targets = SDG::Target.all + SDG::LocalTarget.all + + options_from_collection_for_select(targets.sort, :code, :code, params[:target_code]) end end diff --git a/app/models/concerns/sdg/relatable.rb b/app/models/concerns/sdg/relatable.rb index 06a29c437..ef92696a7 100644 --- a/app/models/concerns/sdg/relatable.rb +++ b/app/models/concerns/sdg/relatable.rb @@ -20,7 +20,11 @@ module SDG::Relatable end def by_target(code) - by_sdg_related(SDG::Target, code) + if SDG::Target.find_by(code: code) + by_sdg_related(SDG::Target, code) + else + by_sdg_related(SDG::LocalTarget, code) + end end def by_sdg_related(sdg_class, code) diff --git a/spec/models/sdg/relatable_spec.rb b/spec/models/sdg/relatable_spec.rb index 965d1a394..3065f89b0 100644 --- a/spec/models/sdg/relatable_spec.rb +++ b/spec/models/sdg/relatable_spec.rb @@ -194,6 +194,19 @@ describe SDG::Relatable do expect(relatable.class.by_target(target.code)).to be_empty end + + it "returns records associated to a local target" do + relatable.sdg_local_targets = [local_target] + + expect(relatable.class.by_target(local_target.code)).to eq [relatable] + end + + it "does not return records not associated with that local target" do + create(:proposal) + create(:proposal, sdg_local_targets: [another_local_target]) + + expect(relatable.class.by_target(local_target.code)).to be_empty + end end describe ".pending_sdg_review" do diff --git a/spec/system/sdg_management/relations_spec.rb b/spec/system/sdg_management/relations_spec.rb index 83791a9a6..0fdea91a4 100644 --- a/spec/system/sdg_management/relations_spec.rb +++ b/spec/system/sdg_management/relations_spec.rb @@ -167,6 +167,21 @@ describe "SDG Relations", :js do expect(page).to have_css "li.is-active h2", exact_text: "Pending" end + scenario "local target filter" do + schools = create(:sdg_local_target, code: "4.1.1") + teachers = create(:sdg_local_target, code: "4.1.2") + + create(:debate, title: "Rebuild local schools", sdg_local_targets: [schools]) + create(:debate, title: "Hire teachers", sdg_local_targets: [teachers]) + + visit sdg_management_debates_path + select "4.1.1", from: "target_code" + click_button "Search" + + expect(page).to have_content "Rebuild local schools" + expect(page).not_to have_content "Hire teachers" + end + scenario "search within current tab" do visit sdg_management_proposals_path(filter: "pending_sdg_review") From 176839c905a05b8e9b42d83aa34d145281b89eb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 21 Jan 2021 16:45:24 +0100 Subject: [PATCH 3/5] Rename sdg_targets association We use `sdg_global_targets` because we will add a new `sdg_targets` method which will return both targets and local targets. --- app/models/concerns/sdg/relatable.rb | 26 +++++++++++++++++--------- spec/models/sdg/relatable_spec.rb | 10 +++++----- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/app/models/concerns/sdg/relatable.rb b/app/models/concerns/sdg/relatable.rb index ef92696a7..0c6908f2d 100644 --- a/app/models/concerns/sdg/relatable.rb +++ b/app/models/concerns/sdg/relatable.rb @@ -4,33 +4,41 @@ module SDG::Relatable included do has_many :sdg_relations, as: :relatable, dependent: :destroy, class_name: "SDG::Relation" - %w[SDG::Goal SDG::Target SDG::LocalTarget].each do |sdg_type| + %w[SDG::Goal SDG::LocalTarget].each do |sdg_type| has_many sdg_type.constantize.table_name.to_sym, through: :sdg_relations, source: :related_sdg, source_type: sdg_type end + has_many :sdg_global_targets, + through: :sdg_relations, + source: :related_sdg, + source_type: "SDG::Target" + alias_method :sdg_targets, :sdg_global_targets + alias_method :sdg_targets=, :sdg_global_targets= has_one :sdg_review, as: :relatable, dependent: :destroy, class_name: "SDG::Review" end class_methods do def by_goal(code) - by_sdg_related(SDG::Goal, code) + by_sdg_related(:sdg_goals, code) end def by_target(code) if SDG::Target.find_by(code: code) - by_sdg_related(SDG::Target, code) + by_sdg_related(:sdg_global_targets, code) else - by_sdg_related(SDG::LocalTarget, code) + by_sdg_related(:sdg_local_targets, code) end end - def by_sdg_related(sdg_class, code) + def by_sdg_related(association, code) return all if code.blank? - joins(sdg_class.table_name.to_sym).merge(sdg_class.where(code: code)) + sdg_class = reflect_on_association(association).options[:source_type].constantize + + joins(association).merge(sdg_class.where(code: code)) end def sdg_reviewed @@ -51,12 +59,12 @@ module SDG::Relatable end def sdg_target_list - sdg_targets.sort.map(&:code).join(", ") + sdg_global_targets.sort.map(&:code).join(", ") end def sdg_related_list sdg_goals.order(:code).map do |goal| - [goal, sdg_targets.where(goal: goal).sort] + [goal, sdg_global_targets.where(goal: goal).sort] end.flatten.map(&:code).join(", ") end @@ -66,7 +74,7 @@ module SDG::Relatable goals = goal_codes.map { |code| SDG::Goal[code] } transaction do - self.sdg_targets = targets + self.sdg_global_targets = targets self.sdg_goals = (targets.map(&:goal) + goals).uniq end end diff --git a/spec/models/sdg/relatable_spec.rb b/spec/models/sdg/relatable_spec.rb index 3065f89b0..31dad7a84 100644 --- a/spec/models/sdg/relatable_spec.rb +++ b/spec/models/sdg/relatable_spec.rb @@ -36,9 +36,9 @@ describe SDG::Relatable do end end - describe "#sdg_targets" do + describe "#sdg_global_targets" do it "can assign targets to a model" do - relatable.sdg_targets = [target, another_target] + relatable.sdg_global_targets = [target, another_target] expect(SDG::Relation.count).to be 2 expect(SDG::Relation.first.relatable).to eq relatable @@ -48,9 +48,9 @@ describe SDG::Relatable do end it "can obtain the list of targets" do - relatable.sdg_targets = [target, another_target] + relatable.sdg_global_targets = [target, another_target] - expect(relatable.reload.sdg_targets).to match_array [target, another_target] + expect(relatable.reload.sdg_global_targets).to match_array [target, another_target] end end @@ -190,7 +190,7 @@ describe SDG::Relatable do it "does not return records not associated with that target" do create(:proposal) - create(:proposal, sdg_targets: [another_target]) + create(:proposal, sdg_global_targets: [another_target]) expect(relatable.class.by_target(target.code)).to be_empty end From 39d68a177987e37778af486eec8557ed74640097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 20 Jan 2021 19:56:17 +0100 Subject: [PATCH 4/5] List local targets alongside targets --- app/models/concerns/sdg/relatable.rb | 7 +++++-- spec/models/sdg/relatable_spec.rb | 7 +++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/sdg/relatable.rb b/app/models/concerns/sdg/relatable.rb index 0c6908f2d..f596fcb31 100644 --- a/app/models/concerns/sdg/relatable.rb +++ b/app/models/concerns/sdg/relatable.rb @@ -14,7 +14,6 @@ module SDG::Relatable through: :sdg_relations, source: :related_sdg, source_type: "SDG::Target" - alias_method :sdg_targets, :sdg_global_targets alias_method :sdg_targets=, :sdg_global_targets= has_one :sdg_review, as: :relatable, dependent: :destroy, class_name: "SDG::Review" @@ -54,12 +53,16 @@ module SDG::Relatable sdg_relations.map(&:related_sdg) end + def sdg_targets + sdg_global_targets + sdg_local_targets + end + def sdg_goal_list sdg_goals.order(:code).map(&:code).join(", ") end def sdg_target_list - sdg_global_targets.sort.map(&:code).join(", ") + sdg_targets.sort.map(&:code).join(", ") end def sdg_related_list diff --git a/spec/models/sdg/relatable_spec.rb b/spec/models/sdg/relatable_spec.rb index 31dad7a84..2c3d2f4fd 100644 --- a/spec/models/sdg/relatable_spec.rb +++ b/spec/models/sdg/relatable_spec.rb @@ -60,6 +60,13 @@ describe SDG::Relatable do expect(relatable.sdg_target_list).to eq "1.2, 2.1, 2.2" end + + it "includes both targets and local targets in order" do + relatable.sdg_global_targets = [SDG::Target[2.2], SDG::Target[1.2], SDG::Target[2.1]] + relatable.sdg_local_targets = %w[1.1.1 2.1.3].map { |code| create(:sdg_local_target, code: code) } + + expect(relatable.sdg_target_list).to eq "1.1.1, 1.2, 2.1, 2.1.3, 2.2" + end end describe "#sdg_local_targets" do From b5ccae2f4007f6a211cad5bd52162a99445246b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 21 Jan 2021 18:05:07 +0100 Subject: [PATCH 5/5] Allow assigning both targets and local targets Particularly useful in tests, because writing `targets` is shorter than writing `global_targets` and `local_targets`. --- app/models/concerns/sdg/relatable.rb | 10 +++++++++- spec/models/sdg/relatable_spec.rb | 12 ++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/sdg/relatable.rb b/app/models/concerns/sdg/relatable.rb index f596fcb31..636feca73 100644 --- a/app/models/concerns/sdg/relatable.rb +++ b/app/models/concerns/sdg/relatable.rb @@ -14,7 +14,6 @@ module SDG::Relatable through: :sdg_relations, source: :related_sdg, source_type: "SDG::Target" - alias_method :sdg_targets=, :sdg_global_targets= has_one :sdg_review, as: :relatable, dependent: :destroy, class_name: "SDG::Review" end @@ -57,6 +56,15 @@ module SDG::Relatable sdg_global_targets + sdg_local_targets end + def sdg_targets=(targets) + global_targets, local_targets = targets.partition { |target| target.class.name == "SDG::Target" } + + transaction do + self.sdg_global_targets = global_targets + self.sdg_local_targets = local_targets + end + end + def sdg_goal_list sdg_goals.order(:code).map(&:code).join(", ") end diff --git a/spec/models/sdg/relatable_spec.rb b/spec/models/sdg/relatable_spec.rb index 2c3d2f4fd..4dd6aea51 100644 --- a/spec/models/sdg/relatable_spec.rb +++ b/spec/models/sdg/relatable_spec.rb @@ -69,6 +69,18 @@ describe SDG::Relatable do end end + describe "#sdg_targets=" do + it "assigns both targets and local targets" do + global_targets = [SDG::Target[2.2], SDG::Target[1.2]] + local_targets = %w[2.2.1 3.1.1].map { |code| create(:sdg_local_target, code: code) } + + relatable.sdg_targets = global_targets + local_targets + + expect(relatable.sdg_global_targets).to match_array global_targets + expect(relatable.sdg_local_targets).to match_array local_targets + end + end + describe "#sdg_local_targets" do it "can assign local targets to a model" do relatable.sdg_local_targets = [local_target, another_local_target]