Conditionally destroy a ressource

lib/api/access_requests.rb

@@ -67,13 +67,12 @@ module API
         end
         delete ":id/access_requests/:user_id" do
           source = find_source(source_type, params[:id])
-          member = source.public_send(:requesters).find_by!(user_id: params[:user_id])
+          member = source.requesters.find_by!(user_id: params[:user_id])
 
-          check_unmodified_since(member.updated_at)
-
-          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

lib/api/award_emoji.rb

@@ -85,12 +85,10 @@ module API
           end
           delete "#{endpoint}/:award_id" do
             award = awardable.award_emoji.find(params[:award_id])
-            check_unmodified_since(award.updated_at)
 
             unauthorized! unless award.user == current_user || current_user.admin?
 
-            status 204
-            award.destroy
+            destroy_conditionally!(award)
           end
         end
       end

lib/api/boards.rb

@@ -122,14 +122,13 @@ module API
         end
         delete "/lists/:list_id" do
           authorize!(:admin_list, user_project)
-
           list = board_lists.find(params[:list_id])
-          check_unmodified_since(list.updated_at)
 
-          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

lib/api/broadcast_messages.rb

@@ -90,10 +90,8 @@ module API
       end
       delete ':id' do
         message = find_message
-        check_unmodified_since(message.updated_at)
 
-        status 204
-        message.destroy
+        destroy_conditionally!(message)
       end
     end
   end

lib/api/deploy_keys.rb

@@ -125,10 +125,7 @@ module API
         key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
         not_found!('Deploy Key') unless key
 
-        check_unmodified_since(key.updated_at)
-
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
     end
   end

lib/api/environments.rb

@@ -78,10 +78,8 @@ module API
         authorize! :update_environment, user_project
 
         environment = user_project.environments.find(params[:environment_id])
-        check_unmodified_since(environment.updated_at)
 
-        status 204
-        environment.destroy
+        destroy_conditionally!(environment)
       end
 
       desc 'Stops an existing environment' do

lib/api/groups.rb

@@ -117,11 +117,10 @@ module API
       delete ":id" do
         group = find_group!(params[:id])
         authorize! :admin_group, group
-        
-        check_unmodified_since(group.updated_at)
 
-        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

lib/api/helpers.rb

@@ -11,14 +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']) if headers.key?('If-Unmodified-Since') rescue nil
+    def check_unmodified_since!(last_modified)
+      if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil
 
-      if if_unmodified_since && if_unmodified_since < last_modified
+      if if_unmodified_since && 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)
 

lib/api/issues.rb

@@ -230,10 +230,8 @@ module API
         not_found!('Issue') unless issue
 
         authorize!(:destroy_issue, issue)
-        check_unmodified_since(issue.updated_at)
 
-        status 204
-        issue.destroy
+        destroy_conditionally!(issue)
       end
 
       desc 'List merge requests closing issue'  do

lib/api/labels.rb

@@ -56,10 +56,7 @@ module API
         label = user_project.labels.find_by(title: params[:name])
         not_found!('Label') unless label
 
-        check_unmodified_since(label.updated_at)
-
-        status 204
-        label.destroy
+        destroy_conditionally!(label)
       end
 
       desc 'Update an existing label. At least one optional parameter is required.' do

lib/api/members.rb

@@ -93,12 +93,11 @@ module API
         end
         delete ":id/members/:user_id" do
           source = find_source(source_type, params[:id])
-          # Ensure that memeber exists
           member = source.members.find_by!(user_id: params[:user_id])
-          check_unmodified_since(member.updated_at)
 
-          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

lib/api/merge_requests.rb

@@ -164,10 +164,8 @@ module API
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
         authorize!(:destroy_merge_request, merge_request)
-        check_unmodified_since(merge_request.updated_at)
 
-        status 204
-        merge_request.destroy
+        destroy_conditionally!(merge_request)
       end
 
       params do

lib/api/notes.rb

@@ -131,10 +131,10 @@ module API
           note = user_project.notes.find(params[:note_id])
 
           authorize! :admin_note, note
-          check_unmodified_since(note.updated_at)
 
-          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

lib/api/project_hooks.rb

@@ -96,10 +96,7 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_project.hooks.find(params.delete(:hook_id))
 
-        check_unmodified_since(hook.updated_at)
-
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end

lib/api/project_snippets.rb

@@ -116,10 +116,8 @@ module API
         not_found!('Snippet') unless snippet
 
         authorize! :admin_project_snippet, snippet
-        check_unmodified_since(snippet.updated_at)
 
-        status 204
-        snippet.destroy
+        destroy_conditionally!(snippet)
       end
 
       desc 'Get a raw project snippet'

lib/api/projects.rb

@@ -334,9 +334,10 @@ module API
       desc 'Remove a project'
       delete ":id" do
         authorize! :remove_project, user_project
-        check_unmodified_since(user_project.updated_at)
 
-        ::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

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

lib/api/runners.rb

@@ -79,10 +79,8 @@ module API
         runner = get_runner(params[:id])
 
         authenticate_delete_runner!(runner)
-        check_unmodified_since(runner.updated_at)
 
-        status 204
-        runner.destroy!
+        destroy_conditionally!(runner)
       end
     end
 
@@ -137,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
 

lib/api/services.rb

@@ -655,8 +655,8 @@ module API
       end
       delete ":id/services/:service_slug" do
         service = user_project.find_or_initialize_service(params[:service_slug].underscore)
-        # Todo, not sure
-        check_unmodified_since(service.updated_at)
+        # Todo: Check if this done the right way
+        check_unmodified_since!(service.updated_at)
 
         attrs = service_attributes(service).inject({}) do |hash, key|
           hash.merge!(key => nil)

lib/api/snippets.rb

@@ -122,10 +122,8 @@ module API
         return not_found!('Snippet') unless snippet
 
         authorize! :destroy_personal_snippet, snippet
-        check_unmodified_since(snippet.updated_at)
 
-        status 204
-        snippet.destroy
+        destroy_conditionally!(snippet)
       end
 
       desc 'Get a raw snippet' do

lib/api/system_hooks.rb

@@ -66,10 +66,7 @@ module API
         hook = SystemHook.find_by(id: params[:id])
         not_found!('System hook') unless hook
 
-        check_unmodified_since(hook.updated_at)
-
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end

lib/api/triggers.rb

@@ -140,10 +140,7 @@ module API
         trigger = user_project.triggers.find(params.delete(:trigger_id))
         return not_found!('Trigger') unless trigger
 
-        check_unmodified_since(trigger.updated_at)
-
-        status 204
-        trigger.destroy
+        destroy_conditionally!(trigger)
       end
     end
   end

lib/api/users.rb

@@ -230,13 +230,7 @@ module API
         key = user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-<<<<<<< HEAD
-        status 204
-=======
-        check_unmodified_since(key.updated_at)
-
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc 'Add an email address to a specified user. Available only for admins.' do
@@ -292,14 +286,11 @@ module API
         email = user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-<<<<<<< HEAD
-        Emails::DestroyService.new(user, email: email.email).execute
-=======
-        check_unmodified_since(email.updated_at)
+        destroy_conditionally!(email) do |email|
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
 
-        email.destroy
         user.update_secondary_emails!
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -315,14 +306,9 @@ module API
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-<<<<<<< HEAD
-        status 204
-        user.delete_async(deleted_by: current_user, params: params)
-=======
-        check_unmodified_since(user.updated_at)
-
-        ::Users::DestroyService.new(current_user).execute(user)
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
+        destroy_conditionally!(user) do
+          user.delete_async(deleted_by: current_user, params: params)
+        end
       end
 
       desc 'Block a user. Available only for admins.'
@@ -500,10 +486,7 @@ module API
         key = current_user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-        check_unmodified_since(key.updated_at)
-
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc "Get the currently authenticated user's email addresses" do
@@ -554,9 +537,11 @@ module API
         email = current_user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-<<<<<<< HEAD
-        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'
@@ -572,12 +557,6 @@ module API
           .reorder(last_activity_on: :asc)
 
         present paginate(activities), with: Entities::UserActivity
-=======
-        check_unmodified_since(email.updated_at)
-
-        email.destroy
-        current_user.update_secondary_emails!
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
     end
   end

spec/requests/api/tags_spec.rb

@@ -283,7 +283,7 @@ describe API::Tags do
 
         it_behaves_like '404 response' do
           let(:request) { delete api(route, current_user) }
-          let(:message) { 'No such tag' }
+          let(:message) { '404 Tag Not Found' }
         end
       end
 
@@ -394,7 +394,7 @@ describe API::Tags do
 
         it_behaves_like '404 response' do
           let(:request) { put api(route, current_user), description: new_description }
-          let(:message) { 'Tag does not exist' }
+          let(:message) { '404 Tag Not Found' }
         end
       end