Merge branch 'master' into 36917-branch-tooltip

* master: (28 commits)
  Add changelog entry
  max-width for lazy-loaded images (this was removed in the original MR through merge resolution most probably)
  Backports EE 2756 logic to CE.
  Make rubocop happy
  Make profile settings dropdown consistent
  Fixes the margin of the top buttons of the pipeline page
  Add changelog
  Respect the default visibility level when creating a group
  Further break with_repo_branch_commit into parts
  Make sure inspect doesn't generate crazy string
  It doesn't seem that rubocop is complaining for me
  Move GPG signed commits docs to new location
  Refactor
  Add a spec when ressource is not modified
  Move the logic to a concern
  Add changelog and doc
  Add tests for the unmodified header
  Update remaining endpoints
  Use commit date for branches and tags
  Conditionally destroy a ressource
  ...

29 files changed, 127 insertions, 84 deletions

diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
index cdacf9839e5..4fa9b2b2494 100644
--- a/lib/api/access_requests.rb
+++ b/lib/api/access_requests.rb
@@ -67,10 +67,12 @@ module API
         end
         delete ":id/access_requests/:user_id" do
           source = find_source(source_type, params[:id])
+          member = source.requesters.find_by!(user_id: params[:user_id])
 
-          status 204
-          ::Members::DestroyService.new(source, current_user, params)
-            .execute(:requesters)
+          destroy_conditionally!(member) do
+            ::Members::DestroyService.new(source, current_user, params)
+              .execute(:requesters)
+          end
         end
       end
     end
diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb
index 5a028fc9d0b..8e3851640da 100644
--- a/lib/api/award_emoji.rb
+++ b/lib/api/award_emoji.rb
@@ -88,8 +88,7 @@ module API
 
             unauthorized! unless award.user == current_user || current_user.admin?
 
-            status 204
-            award.destroy
+            destroy_conditionally!(award)
           end
         end
       end
diff --git a/lib/api/boards.rb b/lib/api/boards.rb
index 5a2d7a681e3..0d11c5fc971 100644
--- a/lib/api/boards.rb
+++ b/lib/api/boards.rb
@@ -122,13 +122,13 @@ module API
         end
         delete "/lists/:list_id" do
           authorize!(:admin_list, user_project)
-
           list = board_lists.find(params[:list_id])
 
-          service = ::Boards::Lists::DestroyService.new(user_project, current_user)
-
-          unless service.execute(list)
-            render_api_error!({ error: 'List could not be deleted!' }, 400)
+          destroy_conditionally!(list) do |list|
+            service = ::Boards::Lists::DestroyService.new(user_project, current_user)
+            unless service.execute(list)
+              render_api_error!({ error: 'List could not be deleted!' }, 400)
+            end
           end
         end
       end
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index d3dbf941298..b87f7cdbad1 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -125,11 +125,18 @@ module API
       delete ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_push_project
 
-        result = DeleteBranchService.new(user_project, current_user)
-                 .execute(params[:branch])
+        branch = user_project.repository.find_branch(params[:branch])
+        not_found!('Branch') unless branch
+
+        commit = user_project.repository.commit(branch.dereferenced_target)
+
+        destroy_conditionally!(commit, last_update_field: :authored_date) do
+          result = DeleteBranchService.new(user_project, current_user)
+                    .execute(params[:branch])
 
-        if result[:status] != :success
-          render_api_error!(result[:message], result[:return_code])
+          if result[:status] != :success
+            render_api_error!(result[:message], result[:return_code])
+          end
         end
       end
 
diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb
index 9980aec4752..0b45621ce7b 100644
--- a/lib/api/broadcast_messages.rb
+++ b/lib/api/broadcast_messages.rb
@@ -91,8 +91,7 @@ module API
       delete ':id' do
         message = find_message
 
-        status 204
-        message.destroy
+        destroy_conditionally!(message)
       end
     end
   end
diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb
index 42e7c1486b0..f405c341398 100644
--- a/lib/api/deploy_keys.rb
+++ b/lib/api/deploy_keys.rb
@@ -125,8 +125,7 @@ module API
         key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
         not_found!('Deploy Key') unless key
 
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
     end
   end
diff --git a/lib/api/environments.rb b/lib/api/environments.rb
index c774a5c6685..e33269f9483 100644
--- a/lib/api/environments.rb
+++ b/lib/api/environments.rb
@@ -79,8 +79,7 @@ module API
 
         environment = user_project.environments.find(params[:environment_id])
 
-        status 204
-        environment.destroy
+        destroy_conditionally!(environment)
       end
 
       desc 'Stops an existing environment' do
diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb
index f64da4ab77b..25152f30998 100644
--- a/lib/api/group_variables.rb
+++ b/lib/api/group_variables.rb
@@ -88,8 +88,7 @@ module API
         variable = user_group.variables.find_by(key: params[:key])
         not_found!('GroupVariable') unless variable
 
-        status 204
-        variable.destroy
+        destroy_conditionally!(variable)
       end
     end
   end
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index e56427304a6..8c494a54329 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -7,7 +7,11 @@ module API
     helpers do
       params :optional_params_ce do
         optional :description, type: String, desc: 'The description of the group'
-        optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the group'
+        optional :visibility, type: String,
+                              values: Gitlab::VisibilityLevel.string_values,
+                              default: Gitlab::VisibilityLevel.string_level(
+                                Gitlab::CurrentSettings.current_application_settings.default_group_visibility),
+                              desc: 'The visibility of the group'
         optional :lfs_enabled, type: Boolean, desc: 'Enable/disable LFS for the projects in this group'
         optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access'
         optional :share_with_group_lock, type: Boolean, desc: 'Prevent sharing a project with another group within this group'
@@ -118,8 +122,9 @@ module API
         group = find_group!(params[:id])
         authorize! :admin_group, group
 
-        status 204
-        ::Groups::DestroyService.new(group, current_user).execute
+        destroy_conditionally!(group) do |group|
+          ::Groups::DestroyService.new(group, current_user).execute
+        end
       end
 
       desc 'Get a list of projects in this group.' do
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index b56fd2388b3..84980864151 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -11,6 +11,25 @@ module API
       declared(params, options).to_h.symbolize_keys
     end
 
+    def check_unmodified_since!(last_modified)
+      if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil
+
+      if if_unmodified_since && last_modified && last_modified > if_unmodified_since
+        render_api_error!('412 Precondition Failed', 412)
+      end
+    end
+
+    def destroy_conditionally!(resource, last_update_field: :updated_at)
+      check_unmodified_since!(resource.public_send(last_update_field))
+
+      status 204
+      if block_given?
+        yield resource
+      else
+        resource.destroy
+      end
+    end
+
     def current_user
       return @current_user if defined?(@current_user)
 
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 4cec1145f3a..6503629e2a2 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -230,8 +230,8 @@ module API
         not_found!('Issue') unless issue
 
         authorize!(:destroy_issue, issue)
-        status 204
-        issue.destroy
+
+        destroy_conditionally!(issue)
       end
 
       desc 'List merge requests closing issue'  do
diff --git a/lib/api/labels.rb b/lib/api/labels.rb
index 4520c98d951..c0cf618ee8d 100644
--- a/lib/api/labels.rb
+++ b/lib/api/labels.rb
@@ -56,8 +56,7 @@ module API
         label = user_project.labels.find_by(title: params[:name])
         not_found!('Label') unless label
 
-        status 204
-        label.destroy
+        destroy_conditionally!(label)
       end
 
       desc 'Update an existing label. At least one optional parameter is required.' do
diff --git a/lib/api/members.rb b/lib/api/members.rb
index bb970b7cd54..a5d3d7f25a0 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -93,11 +93,11 @@ module API
         end
         delete ":id/members/:user_id" do
           source = find_source(source_type, params[:id])
-          # Ensure that memeber exists
-          source.members.find_by!(user_id: params[:user_id])
+          member = source.members.find_by!(user_id: params[:user_id])
 
-          status 204
-          ::Members::DestroyService.new(source, current_user, declared_params).execute
+          destroy_conditionally!(member) do
+            ::Members::DestroyService.new(source, current_user, declared_params).execute
+          end
         end
       end
     end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 8810d4e441d..969c6064662 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -164,8 +164,8 @@ module API
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
         authorize!(:destroy_merge_request, merge_request)
-        status 204
-        merge_request.destroy
+
+        destroy_conditionally!(merge_request)
       end
 
       params do
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 4e4e473994b..e116448c15b 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -129,10 +129,12 @@ module API
         end
         delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
           note = user_project.notes.find(params[:note_id])
+
           authorize! :admin_note, note
 
-          status 204
-          ::Notes::DestroyService.new(user_project, current_user).execute(note)
+          destroy_conditionally!(note) do |note|
+            ::Notes::DestroyService.new(user_project, current_user).execute(note)
+          end
         end
       end
     end
diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb
index dbeaf9e17ef..e3123ef4e2d 100644
--- a/lib/api/pipeline_schedules.rb
+++ b/lib/api/pipeline_schedules.rb
@@ -117,8 +117,7 @@ module API
         not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :admin_pipeline_schedule, pipeline_schedule
 
-        status :accepted
-        present pipeline_schedule.destroy, with: Entities::PipelineScheduleDetails
+        destroy_conditionally!(pipeline_schedule)
       end
     end
 
diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb
index 649dd891f56..5b457bbe639 100644
--- a/lib/api/project_hooks.rb
+++ b/lib/api/project_hooks.rb
@@ -96,8 +96,7 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_project.hooks.find(params.delete(:hook_id))
 
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end
diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb
index f3d905b0068..704e8c6718d 100644
--- a/lib/api/project_snippets.rb
+++ b/lib/api/project_snippets.rb
@@ -116,8 +116,8 @@ module API
         not_found!('Snippet') unless snippet
 
         authorize! :admin_project_snippet, snippet
-        status 204
-        snippet.destroy
+
+        destroy_conditionally!(snippet)
       end
 
       desc 'Get a raw project snippet'
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 15c3832b032..78d900984ac 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -1,7 +1,6 @@
 require_dependency 'declarative_policy'
 
 module API
-  # Projects API
   class Projects < Grape::API
     include PaginationParams
 
@@ -334,7 +333,10 @@ module API
       desc 'Remove a project'
       delete ":id" do
         authorize! :remove_project, user_project
-        ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
+
+        destroy_conditionally!(user_project) do
+          ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
+        end
 
         accepted!
       end
@@ -363,8 +365,7 @@ module API
         authorize! :remove_fork_project, user_project
 
         if user_project.forked?
-          status 204
-          user_project.forked_project_link.destroy
+          destroy_conditionally!(user_project.forked_project_link)
         else
           not_modified!
         end
@@ -408,8 +409,7 @@ module API
         link = user_project.project_group_links.find_by(group_id: params[:group_id])
         not_found!('Group Link') unless link
 
-        status 204
-        link.destroy
+        destroy_conditionally!(link)
       end
 
       desc 'Upload a file'
diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb
index dccf4fa27a7..15fcb9e8e27 100644
--- a/lib/api/protected_branches.rb
+++ b/lib/api/protected_branches.rb
@@ -76,9 +76,7 @@ module API
       delete ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         protected_branch = user_project.protected_branches.find_by!(name: params[:name])
 
-        protected_branch.destroy
-
-        status 204
+        destroy_conditionally!(protected_branch)
       end
     end
   end
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 1a7ded31c91..11999354594 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -45,8 +45,10 @@ module API
       end
       delete '/' do
         authenticate_runner!
-        status 204
-        Ci::Runner.find_by_token(params[:token]).destroy
+
+        runner = Ci::Runner.find_by_token(params[:token])
+
+        destroy_conditionally!(runner)
       end
 
       desc 'Validates authentication credentials' do
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 31f940fe96b..68c2120cc15 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -77,10 +77,10 @@ module API
       end
       delete ':id' do
         runner = get_runner(params[:id])
+
         authenticate_delete_runner!(runner)
 
-        status 204
-        runner.destroy!
+        destroy_conditionally!(runner)
       end
     end
 
@@ -135,8 +135,7 @@ module API
         runner = runner_project.runner
         forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1
 
-        status 204
-        runner_project.destroy
+        destroy_conditionally!(runner_project)
       end
     end
 
diff --git a/lib/api/services.rb b/lib/api/services.rb
index 843c05ae32e..ff9ddd44439 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -656,12 +656,14 @@ module API
       delete ":id/services/:service_slug" do
         service = user_project.find_or_initialize_service(params[:service_slug].underscore)
 
-        attrs = service_attributes(service).inject({}) do |hash, key|
-          hash.merge!(key => nil)
-        end
+        destroy_conditionally!(service) do
+          attrs = service_attributes(service).inject({}) do |hash, key|
+            hash.merge!(key => nil)
+          end
 
-        unless service.update_attributes(attrs.merge(active: false))
-          render_api_error!('400 Bad Request', 400)
+          unless service.update_attributes(attrs.merge(active: false))
+            render_api_error!('400 Bad Request', 400)
+          end
         end
       end
 
diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb
index 35ece56c65c..00eb7c60f16 100644
--- a/lib/api/snippets.rb
+++ b/lib/api/snippets.rb
@@ -123,8 +123,7 @@ module API
 
         authorize! :destroy_personal_snippet, snippet
 
-        status 204
-        snippet.destroy
+        destroy_conditionally!(snippet)
       end
 
       desc 'Get a raw snippet' do
diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb
index c0179037440..6b6a03e3300 100644
--- a/lib/api/system_hooks.rb
+++ b/lib/api/system_hooks.rb
@@ -66,8 +66,7 @@ module API
         hook = SystemHook.find_by(id: params[:id])
         not_found!('System hook') unless hook
 
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end
diff --git a/lib/api/tags.rb b/lib/api/tags.rb
index 1333747cced..81b17935b81 100644
--- a/lib/api/tags.rb
+++ b/lib/api/tags.rb
@@ -65,11 +65,18 @@ module API
       delete ':id/repository/tags/:tag_name', requirements: TAG_ENDPOINT_REQUIREMENTS do
         authorize_push_project
 
-        result = ::Tags::DestroyService.new(user_project, current_user)
-          .execute(params[:tag_name])
+        tag = user_project.repository.find_tag(params[:tag_name])
+        not_found!('Tag') unless tag
+
+        commit = user_project.repository.commit(tag.dereferenced_target)
+
+        destroy_conditionally!(commit, last_update_field: :authored_date) do
+          result = ::Tags::DestroyService.new(user_project, current_user)
+                    .execute(params[:tag_name])
 
-        if result[:status] != :success
-          render_api_error!(result[:message], result[:return_code])
+          if result[:status] != :success
+            render_api_error!(result[:message], result[:return_code])
+          end
         end
       end
 
diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb
index edfdb63d183..c9fee7e5193 100644
--- a/lib/api/triggers.rb
+++ b/lib/api/triggers.rb
@@ -140,8 +140,7 @@ module API
         trigger = user_project.triggers.find(params.delete(:trigger_id))
         return not_found!('Trigger') unless trigger
 
-        status 204
-        trigger.destroy
+        destroy_conditionally!(trigger)
       end
     end
   end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index e2019d6d512..96f47bb618a 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -230,8 +230,7 @@ module API
         key = user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc 'Add an email address to a specified user. Available only for admins.' do
@@ -287,7 +286,11 @@ module API
         email = user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-        Emails::DestroyService.new(user, email: email.email).execute
+        destroy_conditionally!(email) do |email|
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
+
+        user.update_secondary_emails!
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -299,11 +302,13 @@ module API
       end
       delete ":id" do
         authenticated_as_admin!
+
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        status 204
-        user.delete_async(deleted_by: current_user, params: params)
+        destroy_conditionally!(user) do
+          user.delete_async(deleted_by: current_user, params: params)
+        end
       end
 
       desc 'Block a user. Available only for admins.'
@@ -403,8 +408,11 @@ module API
             requires :impersonation_token_id, type: Integer, desc: 'The ID of the impersonation token'
           end
           delete ':impersonation_token_id' do
-            status 204
-            find_impersonation_token.revoke!
+            token = find_impersonation_token
+
+            destroy_conditionally!(token) do
+              token.revoke!
+            end
           end
         end
       end
@@ -481,8 +489,7 @@ module API
         key = current_user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc "Get the currently authenticated user's email addresses" do
@@ -533,8 +540,11 @@ module API
         email = current_user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-        status 204
-        Emails::DestroyService.new(current_user, email: email.email).execute
+        destroy_conditionally!(email) do |email|
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
+
+        current_user.update_secondary_emails!
       end
 
       desc 'Get a list of user activities'
diff --git a/lib/api/variables.rb b/lib/api/variables.rb
index 7c0fdd3d1be..da71787abab 100644
--- a/lib/api/variables.rb
+++ b/lib/api/variables.rb
@@ -88,6 +88,7 @@ module API
         variable = user_project.variables.find_by(key: params[:key])
         not_found!('Variable') unless variable
 
+        # Variables don't have any timestamp. Therfore, destroy unconditionally.
         status 204
         variable.destroy
       end