From dca95d608f898c3ef95c98af4a66fc65ec5bf723 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 28 Nov 2018 14:37:59 +0100
Subject: [PATCH] Display description validation error in milestones

We had a validation rule for milestones which made sure either the
status or the description was present. However, since the description is
now translatable, the validation error wasn't being displayed in the
admin form.

Moving the validation rule to the translation object fixes the problem.
However, the translation object needs to check an attribute in the
milestone object in order to know whether the description is required or
not. This is tricky because by default it loads the milestone object
from the database, meaning it doesn't work with new records and it
ignores params sent by the browser.

The solution is to manually assign the globalized model before
validating the object. It's a hack, but apparently Rails doesn't provide
a better way to handle this case [1].

[1] https://github.com/rails/rails/issues/32024
---
 app/models/milestone.rb                     | 12 +++++++-----
 app/models/milestone/translation.rb         |  3 +++
 spec/shared/features/admin_milestoneable.rb | 12 ++++++++++++
 3 files changed, 22 insertions(+), 5 deletions(-)
 create mode 100644 app/models/milestone/translation.rb

diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 1b4790040..5abd5cb4f 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -13,7 +13,9 @@ class Milestone < ActiveRecord::Base
 
   validates :milestoneable, presence: true
   validates :publication_date, presence: true
-  validate :description_or_status_present?
+
+  before_validation :assign_milestone_to_translations
+  validates_translation :description, presence: true, unless: -> { status_id.present? }
 
   scope :order_by_publication_date, -> { order(publication_date: :asc, created_at: :asc) }
   scope :published,                 -> { where("publication_date <= ?", Date.current) }
@@ -23,9 +25,9 @@ class Milestone < ActiveRecord::Base
     80
   end
 
-  def description_or_status_present?
-    unless description.present? || status_id.present?
-      errors.add(:description)
+  private
+
+    def assign_milestone_to_translations
+      translations.each { |translation| translation.globalized_model = self }
     end
-  end
 end
diff --git a/app/models/milestone/translation.rb b/app/models/milestone/translation.rb
new file mode 100644
index 000000000..e1b589d96
--- /dev/null
+++ b/app/models/milestone/translation.rb
@@ -0,0 +1,3 @@
+class Milestone::Translation < Globalize::ActiveRecord::Translation
+  delegate :status_id, to: :globalized_model
+end
diff --git a/spec/shared/features/admin_milestoneable.rb b/spec/shared/features/admin_milestoneable.rb
index 66b4d1301..17f43a012 100644
--- a/spec/shared/features/admin_milestoneable.rb
+++ b/spec/shared/features/admin_milestoneable.rb
@@ -68,6 +68,18 @@ shared_examples "admin_milestoneable" do |factory_name, path_name|
           expect(page).to have_content 'New description milestone'
         end
       end
+
+      scenario "Show validation errors with no description nor status" do
+        visit path
+        click_link "Create new milestone"
+
+        fill_in "Date", with: Date.current
+        click_button "Create milestone"
+
+        within "#new_milestone" do
+          expect(page).to have_content "can't be blank", count: 1
+        end
+      end
     end
 
     context "Edit" do