Use static error message and don't give booleans

in validation. Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9219#note_23437431
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9219#note_23437868

2 files changed, 8 insertions, 9 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 6d5f02a1011..e865da0ce7a 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -306,17 +306,12 @@ class ApplicationSetting < ActiveRecord::Base
   end
 
   def check_default_artifacts_expire_in
-    return true unless default_artifacts_expire_in
-
-    if ChronicDuration.parse(default_artifacts_expire_in).nil?
-      errors.add(:default_artifacts_expire_in,
+    if default_artifacts_expire_in &&
+      ChronicDuration.parse(default_artifacts_expire_in).nil?
+      errors.add(:default_artifacts_expiration,
         "can't be 0. Leave it blank for no expiration")
-      false
-    else
-      true
     end
   rescue ChronicDuration::DurationParseError => e
-    errors.add(:default_artifacts_expire_in, ": #{e.message}")
-    false
+    errors.add(:default_artifacts_expiration, "is invalid")
   end
 end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 9629f858609..719eda9b5b3 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -34,12 +34,16 @@ describe ApplicationSetting, models: true do
         setting.update(default_artifacts_expire_in: 'a')
 
         expect(setting).to be_invalid
+        expect(setting.errors.messages)
+          .to have_key(:default_artifacts_expiration)
       end
 
       it 'does not allow 0' do
         setting.update(default_artifacts_expire_in: '0')
 
         expect(setting).to be_invalid
+        expect(setting.errors.messages)
+          .to have_key(:default_artifacts_expiration)
       end
 
       it 'sets the value if it is valid' do