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.
This commit is contained in:
Javi Martín
2021-06-23 04:21:44 +02:00
parent b318a8b8d2
commit 8f20ee1a33
3 changed files with 31 additions and 10 deletions

View File

@@ -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

View File

@@ -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)

View File

@@ -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) }