From 96d5354cd88cf222b9542b7a4d7f0f053df1ade0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 30 Jan 2021 14:00:10 +0100
Subject: [PATCH] Improve performance sorting SDG records

We noticed there was a performance issue while browsing the SDG
Management section and when one of our tests started failing sometimes
because the request to the relations#index controller took too long.

The issue proved to be `SDG::Target#<=>`. This method calls `.goal` for
each target, meaning we were generating 169 database queries when
sorting all targets.

So we're comparing codes directly to minimize the number of database
queries and improve performance. Requests to the relations index take
now less than third of the time they used to take.
---
 app/models/concerns/sdg/related.rb | 17 +++++++++++++++++
 app/models/sdg/goal.rb             |  9 ---------
 app/models/sdg/local_target.rb     | 19 -------------------
 app/models/sdg/target.rb           | 25 -------------------------
 4 files changed, 17 insertions(+), 53 deletions(-)

diff --git a/app/models/concerns/sdg/related.rb b/app/models/concerns/sdg/related.rb
index 1820b3ae3..baaa2a2e0 100644
--- a/app/models/concerns/sdg/related.rb
+++ b/app/models/concerns/sdg/related.rb
@@ -1,5 +1,6 @@
 module SDG::Related
   extend ActiveSupport::Concern
+  include Comparable
 
   RELATABLE_TYPES = %w[
     Budget::Investment
@@ -23,4 +24,20 @@ module SDG::Related
   def relatables
     relations.map(&:relatable)
   end
+
+  def <=>(goal_or_target)
+    if goal_or_target.class.ancestors.include?(SDG::Related)
+      subcodes <=> goal_or_target.subcodes
+    end
+  end
+
+  def subcodes
+    code.to_s.split(".").map do |subcode|
+      if subcode.to_i.positive?
+        subcode.to_i
+      else
+        subcode.to_i(36) * 1000
+      end
+    end
+  end
 end
diff --git a/app/models/sdg/goal.rb b/app/models/sdg/goal.rb
index 3e0b87007..167dc2bf4 100644
--- a/app/models/sdg/goal.rb
+++ b/app/models/sdg/goal.rb
@@ -1,5 +1,4 @@
 class SDG::Goal < ApplicationRecord
-  include Comparable
   include SDG::Related
 
   validates :code, presence: true, uniqueness: true, inclusion: { in: 1..17 }
@@ -19,14 +18,6 @@ class SDG::Goal < ApplicationRecord
     I18n.t("sdg.goals.goal_#{code}.description")
   end
 
-  def <=>(goal_or_target)
-    if goal_or_target.class == self.class
-      code <=> goal_or_target.code
-    elsif goal_or_target.respond_to?(:goal)
-      [self, -1] <=> [goal_or_target.goal, 1]
-    end
-  end
-
   def self.[](code)
     find_by!(code: code)
   end
diff --git a/app/models/sdg/local_target.rb b/app/models/sdg/local_target.rb
index 6e07d4895..9a73c5fc2 100644
--- a/app/models/sdg/local_target.rb
+++ b/app/models/sdg/local_target.rb
@@ -1,5 +1,4 @@
 class SDG::LocalTarget < ApplicationRecord
-  include Comparable
   include SDG::Related
 
   translates :title, touch: true
@@ -23,26 +22,8 @@ class SDG::LocalTarget < ApplicationRecord
     find_by!(code: code)
   end
 
-  def <=>(goal_or_target)
-    if goal_or_target.class == self.class
-      [target, numeric_subcode] <=> [goal_or_target.target, goal_or_target.numeric_subcode]
-    elsif [target.class, goal.class].include?(goal_or_target.class)
-      -1 * (goal_or_target <=> self)
-    end
-  end
-
-  protected
-
-    def numeric_subcode
-      subcode.to_i
-    end
-
   private
 
-    def subcode
-      code.split(".").last
-    end
-
     def set_related_goal
       self.goal ||= target&.goal
     end
diff --git a/app/models/sdg/target.rb b/app/models/sdg/target.rb
index e20536e1b..57ad343df 100644
--- a/app/models/sdg/target.rb
+++ b/app/models/sdg/target.rb
@@ -1,5 +1,4 @@
 class SDG::Target < ApplicationRecord
-  include Comparable
   include SDG::Related
 
   validates :code, presence: true, uniqueness: true
@@ -12,37 +11,13 @@ class SDG::Target < ApplicationRecord
     I18n.t("sdg.goals.goal_#{goal.code}.targets.target_#{code_key}.title")
   end
 
-  def <=>(goal_or_target)
-    if goal_or_target.class == self.class
-      [goal.code, numeric_subcode] <=> [goal_or_target.goal.code, goal_or_target.numeric_subcode]
-    elsif goal_or_target.class == goal.class
-      -1 * (goal_or_target <=> self)
-    elsif goal_or_target.class.name == "SDG::LocalTarget"
-      [self, -1] <=> [goal_or_target.target, 1]
-    end
-  end
-
   def self.[](code)
     find_by!(code: code)
   end
 
-  protected
-
-    def numeric_subcode
-      if subcode.to_i > 0
-        subcode.to_i
-      else
-        subcode.to_i(36) * 1000
-      end
-    end
-
   private
 
     def code_key
       code.gsub(".", "_").upcase
     end
-
-    def subcode
-      code.split(".").last
-    end
 end