From 8f20ee1a33a1a8751678239b6dae1824e1283a7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 23 Jun 2021 04:21:44 +0200
Subject: [PATCH] Move related content validation to the model

We were manually checking validation rules (like both relationable
objects are present, or they're both the same object) in the controller
and then using the `save!` method.

However, we usually use the `save` method (which checks all validations)
in a condition, and proceed depending on the result.

Now we're taking the same approach here. This means introducing a new
validation rule in the model to check whether both relationable objects
are the same, which is more robust than checking a condition in the
controller.
---
 .../related_contents_controller.rb            | 22 ++++++++++---------
 app/models/related_content.rb                 | 11 ++++++++++
 spec/models/related_content_spec.rb           |  8 +++++++
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/app/controllers/related_contents_controller.rb b/app/controllers/related_contents_controller.rb
index bbb391465..e6ebac6b0 100644
--- a/app/controllers/related_contents_controller.rb
+++ b/app/controllers/related_contents_controller.rb
@@ -4,18 +4,20 @@ class RelatedContentsController < ApplicationController
   respond_to :html, :js
 
   def create
-    if relationable_object && related_object
-      if relationable_object != related_object
-        RelatedContent.create!(parent_relationable: @relationable, child_relationable: @related, author: current_user)
+    related_content = RelatedContent.new(
+      parent_relationable: relationable_object,
+      child_relationable: related_object,
+      author: current_user
+    )
 
-        flash[:success] = t("related_content.success")
-      else
-        flash[:error] = t("related_content.error_itself")
-      end
+    if related_content.save
+      flash[:success] = t("related_content.success")
+    elsif related_content.same_parent_and_child?
+      flash[:error] = t("related_content.error_itself")
     else
       flash[:error] = t("related_content.error", url: Setting["url"])
     end
-    redirect_to polymorphic_path(@relationable)
+    redirect_to polymorphic_path(relationable_object)
   end
 
   def score_positive
@@ -40,7 +42,7 @@ class RelatedContentsController < ApplicationController
     end
 
     def relationable_object
-      @relationable = params[:relationable_klass].singularize.camelize.constantize.find_by(id: params[:relationable_id])
+      @relationable ||= params[:relationable_klass].singularize.camelize.constantize.find_by(id: params[:relationable_id])
     end
 
     def related_object
@@ -51,7 +53,7 @@ class RelatedContentsController < ApplicationController
                            .flatten.map { |i| i.to_s.singularize.camelize }.join("::")
         related_id = url.match(/\/(\d+)(?!.*\/\d)/)[1]
 
-        @related = related_klass.singularize.camelize.constantize.find_by(id: related_id)
+        related_klass.singularize.camelize.constantize.find_by(id: related_id)
       end
     rescue
       nil
diff --git a/app/models/related_content.rb b/app/models/related_content.rb
index 8a81413ed..c383bf376 100644
--- a/app/models/related_content.rb
+++ b/app/models/related_content.rb
@@ -16,6 +16,7 @@ class RelatedContent < ApplicationRecord
   validates :child_relationable_id, presence: true
   validates :child_relationable_type, presence: true
   validates :parent_relationable_id, uniqueness: { scope: [:parent_relationable_type, :child_relationable_id, :child_relationable_type] }
+  validate :different_parent_and_child
 
   after_create :create_opposite_related_content, unless: proc { opposite_related_content.present? }
   after_create :create_author_score
@@ -34,8 +35,18 @@ class RelatedContent < ApplicationRecord
     related_content_scores.exists?(user: user)
   end
 
+  def same_parent_and_child?
+    parent_relationable == child_relationable
+  end
+
   private
 
+    def different_parent_and_child
+      if same_parent_and_child?
+        errors.add(:parent_relationable, :itself)
+      end
+    end
+
     def create_opposite_related_content
       related_content = RelatedContent.create!(opposite_related_content: self, parent_relationable: child_relationable,
                                                child_relationable: parent_relationable, author: author)
diff --git a/spec/models/related_content_spec.rb b/spec/models/related_content_spec.rb
index 557ff3e53..75d540624 100644
--- a/spec/models/related_content_spec.rb
+++ b/spec/models/related_content_spec.rb
@@ -25,6 +25,14 @@ describe RelatedContent do
     expect(new_related_content).not_to be_valid
   end
 
+  it "does not allow relating a content to itself" do
+    related_content = build(:related_content,
+                            parent_relationable: parent_relationable,
+                            child_relationable: parent_relationable)
+
+    expect(related_content).not_to be_valid
+  end
+
   describe "create_opposite_related_content" do
     let(:parent_relationable) { create(:proposal) }
     let(:child_relationable) { create(:debate) }