Merge branch 'fix-avatar-removal' into 'master'

Fix bug where avatar filenames were not actually deleted from the database during removal

This would result in a 404 error in certain views.

The `save` call was being rolled back due to an error in the validation step.
Relax the validation step so that this works.

Closes #1570

See merge request !620

7 files changed, 57 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index d9c8d38991d..ffe5169bfd2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -40,6 +40,7 @@ v 7.11.0 (unreleased)
   - Fix bug where commit data would not appear in some subdirectories (Stan Hu)
   - Unescape branch names in compare commit (Stan Hu)
   - Task lists are now usable in comments, and will show up in Markdown previews.
+  - Fix bug where avatar filenames were not actually deleted from the database during removal (Stan Hu)
   - Fix bug where Slack service channel was not saved in admin template settings. (Stan Hu)
   - Move snippets UI to fluid layout
   - Improve UI for sidebar. Increase separation between navigation and content
diff --git a/app/models/group.rb b/app/models/group.rb
index 1386a9eccc9..687458adac4 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -20,7 +20,7 @@ class Group < Namespace
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
   has_many :users, through: :group_members
 
-  validate :avatar_type, if: ->(user) { user.avatar_changed? }
+  validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   mount_uploader :avatar, AvatarUploader
diff --git a/app/models/project.rb b/app/models/project.rb
index bc77e7217ee..09d3ffd22fe 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -144,7 +144,7 @@ class Project < ActiveRecord::Base
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :avatar_type,
-    if: ->(project) { project.avatar && project.avatar_changed? }
+    if: ->(project) { project.avatar.present? && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   mount_uploader :avatar, AvatarUploader
diff --git a/app/models/user.rb b/app/models/user.rb
index d088d2d8630..4dd37e73564 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -148,7 +148,7 @@ class User < ActiveRecord::Base
 
   validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true
   validate :namespace_uniq, if: ->(user) { user.username_changed? }
-  validate :avatar_type, if: ->(user) { user.avatar_changed? }
+  validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :unique_email, if: ->(user) { user.email_changed? }
   validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
   validate :owns_public_email, if: ->(user) { user.public_email_changed? }
@@ -309,7 +309,7 @@ class User < ActiveRecord::Base
     if primary_email_record
       primary_email_record.destroy
       self.emails.create(email: self.email_was)
-      
+
       self.update_secondary_emails!
     end
   end
diff --git a/spec/controllers/groups/avatars_controller_spec.rb b/spec/controllers/groups/avatars_controller_spec.rb
new file mode 100644
index 00000000000..3dac134a731
--- /dev/null
+++ b/spec/controllers/groups/avatars_controller_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe Groups::AvatarsController do
+  let(:user)  { create(:user) }
+  let(:group) { create(:group, owner: user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+
+  before do
+    sign_in(user)
+  end
+
+  it 'destroy should remove avatar from DB' do
+    delete :destroy, group_id: group.path
+    @group = assigns(:group)
+    expect(@group.avatar.present?).to be_falsey
+    expect(@group).to be_valid
+  end
+end
diff --git a/spec/controllers/profiles/avatars_controller_spec.rb b/spec/controllers/profiles/avatars_controller_spec.rb
new file mode 100644
index 00000000000..ad5855df0a4
--- /dev/null
+++ b/spec/controllers/profiles/avatars_controller_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe Profiles::AvatarsController do
+  let(:user)    { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png")) }
+
+  before do
+    sign_in(user)
+    controller.instance_variable_set(:@user, user)
+  end
+
+  it 'destroy should remove avatar from DB' do
+    delete :destroy
+    @user = assigns(:user)
+    expect(@user.avatar.present?).to be_falsey
+    expect(@user).to be_valid
+  end
+end
diff --git a/spec/controllers/projects/avatars_controller_spec.rb b/spec/controllers/projects/avatars_controller_spec.rb
new file mode 100644
index 00000000000..e79b46a3504
--- /dev/null
+++ b/spec/controllers/projects/avatars_controller_spec.rb
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe Projects::AvatarsController do
+  let(:project) { create(:project, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+  let(:user)    { create(:user) }
+
+  before do
+    sign_in(user)
+    project.team << [user, :developer]
+    controller.instance_variable_set(:@project, project)
+  end
+
+  it 'destroy should remove avatar from DB' do
+    delete :destroy, namespace_id: project.namespace.id, project_id: project.id
+    expect(project.avatar.present?).to be_falsey
+    expect(project).to be_valid
+  end
+end