From 281155dde58f60d1ab10ac6b99b00b87620b8d80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Nov 2018 14:24:34 +0100 Subject: [PATCH 1/3] Remove duplicate spec --- spec/models/milestone_spec.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 35f175aea..7a1817810 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -40,11 +40,6 @@ describe Milestone do milestone.status_id = nil expect(milestone).to be_valid end - - it "is valid without description if status is present" do - milestone.description = nil - expect(milestone).to be_valid - end end describe "#description_or_status_present?" do From 6974b7d03af8cb48554d7029d2d0d1f82bc98e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Nov 2018 14:26:30 +0100 Subject: [PATCH 2/3] Remove redundant specs The same cases are tested in the previous `describe` block. --- spec/models/milestone_spec.rb | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 7a1817810..92cf96dd4 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -42,28 +42,6 @@ describe Milestone do end end - describe "#description_or_status_present?" do - let(:milestone) { build(:milestone) } - - it "is not valid when status is removed and there's no description" do - milestone.update(description: nil) - expect(milestone.update(status_id: nil)).to be false - end - - it "is not valid when description is removed and there's no status" do - milestone.update(status_id: nil) - expect(milestone.update(description: nil)).to be false - end - - it "is valid when description is removed and there is a status" do - expect(milestone.update(description: nil)).to be true - end - - it "is valid when status is removed and there is a description" do - expect(milestone.update(status_id: nil)).to be true - end - end - describe ".published" do it "uses the application's time zone date", :with_different_time_zone do published_in_local_time_zone = create(:milestone, From dca95d608f898c3ef95c98af4a66fc65ec5bf723 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Nov 2018 14:37:59 +0100 Subject: [PATCH 3/3] 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