Merge branch 'rs-avatar-validation' into 'master'

Only validate avatar cropping values when avatar is present AND changed

Closes #13905

See merge request !3018

2 files changed, 16 insertions, 2 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 2dfef208ee5..3098d49d58a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -169,7 +169,7 @@ class User < ActiveRecord::Base
   validates :avatar_crop_x, :avatar_crop_y, :avatar_crop_size,
     numericality: { only_integer: true },
     presence: true,
-    if: ->(user) { user.avatar? }
+    if: ->(user) { user.avatar? && user.avatar_changed? }
 
   before_validation :generate_password, on: :create
   before_validation :restricted_signup_domains, on: :create
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 7ad7aab2eec..412101ac9f9 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -176,7 +176,7 @@ describe User, models: true do
     end
 
     describe 'avatar' do
-      it 'only validates when avatar is present' do
+      it 'only validates when avatar is present and changed' do
         user = build(:user, :with_avatar)
 
         user.avatar_crop_x    = nil
@@ -184,6 +184,20 @@ describe User, models: true do
         user.avatar_crop_size = nil
 
         expect(user).not_to be_valid
+        expect(user.errors.keys).
+          to match_array %i(avatar_crop_x avatar_crop_y avatar_crop_size)
+      end
+
+      it 'does not validate when avatar has not changed' do
+        user = create(:user, :with_avatar)
+
+        expect { user.avatar_crop_x = nil }.not_to change(user, :valid?)
+      end
+
+      it 'does not validate when avatar is not present' do
+        user = create(:user)
+
+        expect { user.avatar_crop_y = nil }.not_to change(user, :valid?)
       end
     end
   end