From c2bc153314f14566abfdcbf92020cff41fc05a7e Mon Sep 17 00:00:00 2001
From: Tomasz Maczukin <tomasz@maczukin.pl>
Date: Fri, 23 Mar 2018 00:12:15 +0100
Subject: [PATCH] Refactorize ChronicDurationAttribute concern

---
 .../concerns/chronic_duration_attribute.rb    | 51 ++++++++-----------
 .../chronic_duration_attribute_spec.rb        | 24 +++++----
 2 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb
index ad8934f58d8..fa1eafb1d7a 100644
--- a/app/models/concerns/chronic_duration_attribute.rb
+++ b/app/models/concerns/chronic_duration_attribute.rb
@@ -2,49 +2,38 @@ module ChronicDurationAttribute
   extend ActiveSupport::Concern
 
   class_methods do
-    def chronic_duration_attr(virtual_attribute, source_attribute)
-      chronic_duration_attr_reader(virtual_attribute, source_attribute)
-      chronic_duration_attr_writer(virtual_attribute, source_attribute)
-    end
-
     def chronic_duration_attr_reader(virtual_attribute, source_attribute)
       define_method(virtual_attribute) do
-        value = self.send(source_attribute) # rubocop:disable GitlabSecurity/PublicSend
-
-        return '' if value.nil?
-
-        ChronicDuration.output(value, format: :short)
+        chronic_duration_attributes[virtual_attribute] || output_chronic_duration_attribute(source_attribute)
       end
     end
 
     def chronic_duration_attr_writer(virtual_attribute, source_attribute)
-      virtual_attribute_validator = "#{virtual_attribute}_validator".to_sym
-      validation_error = "#{virtual_attribute}_error".to_sym
-
-      validate virtual_attribute_validator
-      attr_accessor validation_error
+      chronic_duration_attr_reader(virtual_attribute, source_attribute)
 
       define_method("#{virtual_attribute}=") do |value|
+        chronic_duration_attributes[virtual_attribute] = value.presence || ''
+
         begin
-          self.send("#{validation_error}=", '') # rubocop:disable GitlabSecurity/PublicSend
-
-          new_value =
-            if value.blank?
-              nil
-            else
-              ChronicDuration.parse(value).to_i
-            end
-
-          self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend
-        rescue ChronicDuration::DurationParseError => ex
-          self.send("#{validation_error}=", ex.message) # rubocop:disable GitlabSecurity/PublicSend
+          new_value = ChronicDuration.parse(value).to_i if value.present?
+          assign_attributes(source_attribute => new_value)
+        rescue ChronicDuration::DurationParseError
+          # ignore error as it will be caught by validation
         end
       end
 
-      define_method(virtual_attribute_validator) do
-        error = self.send(validation_error) # rubocop:disable GitlabSecurity/PublicSend
-        self.send('errors').add(source_attribute, error) unless error.blank? # rubocop:disable GitlabSecurity/PublicSend
-      end
+      validates virtual_attribute, allow_nil: true, duration: true
     end
+
+    alias_method :chronic_duration_attr, :chronic_duration_attr_writer
+  end
+
+  def chronic_duration_attributes
+    @chronic_duration_attributes ||= {}
+  end
+
+  def output_chronic_duration_attribute(source_attribute)
+    value = attributes[source_attribute.to_s]
+    ChronicDuration.output(value, format: :short) if value
   end
 end
diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb
index fea3e752ab5..27c86e60e60 100644
--- a/spec/models/concerns/chronic_duration_attribute_spec.rb
+++ b/spec/models/concerns/chronic_duration_attribute_spec.rb
@@ -12,10 +12,10 @@ shared_examples 'ChronicDurationAttribute reader' do
   end
 
   context 'when value is set to nil' do
-    it 'outputs empty string' do
+    it 'outputs nil' do
       subject.send("#{source_field}=", nil)
 
-      expect(subject.send(virtual_field)).to be_empty
+      expect(subject.send(virtual_field)).to be_nil
     end
   end
 end
@@ -25,15 +25,15 @@ shared_examples 'ChronicDurationAttribute writer' do
     expect(subject.class).to be_public_method_defined("#{virtual_field}=")
   end
 
-  it 'parses chronic duration input' do
+  before do
     subject.send("#{virtual_field}=", '10m')
+  end
 
+  it 'parses chronic duration input' do
     expect(subject.send(source_field)).to eq(600)
   end
 
   it 'passes validation' do
-    subject.send("#{virtual_field}=", '10m')
-
     expect(subject.valid?).to be_truthy
   end
 
@@ -54,33 +54,34 @@ shared_examples 'ChronicDurationAttribute writer' do
       subject.send("#{virtual_field}=", '-10m')
 
       expect(subject.valid?).to be_falsey
+      expect(subject.errors&.messages).to include(virtual_field => ['is not a correct duration'])
     end
   end
 
   context 'when empty input is used' do
-    it 'writes nil' do
+    before do
       subject.send("#{virtual_field}=", '')
+    end
 
+    it 'writes nil' do
       expect(subject.send(source_field)).to be_nil
     end
 
     it 'passes validation' do
-      subject.send("#{virtual_field}=", '')
-
       expect(subject.valid?).to be_truthy
     end
   end
 
   context 'when nil input is used' do
-    it 'writes nil' do
+    before do
       subject.send("#{virtual_field}=", nil)
+    end
 
+    it 'writes nil' do
       expect(subject.send(source_field)).to be_nil
     end
 
     it 'passes validation' do
-      subject.send("#{virtual_field}=", nil)
-
       expect(subject.valid?).to be_truthy
     end
 
@@ -103,6 +104,7 @@ end
 describe 'ChronicDurationAttribute - reader' do
   let(:source_field) {:timeout}
   let(:virtual_field) {:timeout_human_readable}
+
   subject {Ci::BuildMetadata.new}
 
   it "doesn't contain dynamically created writer method" do