v3(services): refactor delete for service instance (cloudfoundry#1980)

We have moved to a pattern where a controller either uses an action to
do something, or schedules a job to do something, and the job uses an
action under the hood. The services instances delete endpoint was using
an action to schedule a job, which could be confusing as it's different
to other endpoints.

[#171728591](https://www.pivotaltracker.com/story/show/171728591)

Author: blgm

app/actions/v3/service_instance_delete.rb

@@ -18,15 +18,14 @@ def delete(service_instance)
 
         lock = DeleterLock.new(service_instance)
 
-        job = case service_instance
-              when ManagedServiceInstance
-                asynchronous_destroy(service_instance)
-              when UserProvidedServiceInstance
-                lock.lock!
-                synchronous_destroy(service_instance, lock)
-              end
-
-        job
+        case service_instance
+        when ManagedServiceInstance
+          return false
+        when UserProvidedServiceInstance
+          lock.lock!
+          synchronous_destroy(service_instance, lock)
+          return true
+        end
       end
 
       private
@@ -37,16 +36,6 @@ def synchronous_destroy(service_instance, lock)
         nil
       end
 
-      def asynchronous_destroy(service_instance)
-        delete_job = V3::DeleteServiceInstanceJob.new(
-          service_instance.guid,
-          service_event_repository.user_audit_info)
-
-        pollable_job = Jobs::Enqueuer.new(delete_job, queue: Jobs::Queues.generic).enqueue_pollable
-
-        pollable_job.guid
-      end
-
       def association_not_empty!
         raise AssociationNotEmptyError
       end

app/controllers/v3/service_instances_controller.rb

@@ -92,9 +92,7 @@ def create
   end
 
   def update
-    service_instance = ServiceInstance.first(guid: hashed_params[:guid])
-    resource_not_found!(:service_instance) unless service_instance && can_read_service_instance?(service_instance)
-    unauthorized! unless can_write_space?(service_instance.space)
+    service_instance = fetch_writable_service_instance(hashed_params[:guid])
 
     case service_instance
     when ManagedServiceInstance
@@ -105,24 +103,21 @@ def update
   end
 
   def destroy
-    service_instance = ServiceInstance.first(guid: hashed_params[:guid])
-    service_instance_not_found! unless service_instance && can_read_service_instance?(service_instance)
+    service_instance = fetch_writable_service_instance(hashed_params[:guid])
     purge = params['purge'] == 'true'
-
-    unauthorized! unless can_write_space?(service_instance.space)
-
     service_event_repository = VCAP::CloudController::Repositories::ServiceEventRepository.new(user_audit_info)
 
     if purge
       ServiceInstancePurge.new(service_event_repository).purge(service_instance)
       return [:no_content, nil]
     end
 
-    job_guid = V3::ServiceInstanceDelete.new(service_event_repository).delete(service_instance)
+    deleted = V3::ServiceInstanceDelete.new(service_event_repository).delete(service_instance)
 
-    if job_guid.blank?
+    if deleted
       head :no_content
     else
+      job_guid = enqueue_delete_job(service_instance)
       head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{job_guid}")
     end
   rescue V3::ServiceInstanceDelete::AssociationNotEmptyError
@@ -294,10 +289,6 @@ def update_managed(service_instance)
     raise CloudController::Errors::ApiError.new_from_details('AsyncServiceBindingOperationInProgress', e.service_binding.app.name, e.service_binding.service_instance.name)
   end
 
-  def admin?
-    permission_queryer.can_write_globally?
-  end
-
   def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found_spaces)
     unreadable_spaces = found_spaces.reject { |s| can_read_space?(s) }
     unwriteable_spaces = found_spaces.reject { |s| can_write_space?(s) || unreadable_spaces.include?(s) }
@@ -316,6 +307,33 @@ def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found
     end
   end
 
+  def build_create_message(params)
+    generic_message = ServiceInstanceCreateMessage.new(params)
+    unprocessable!(generic_message.errors.full_messages) unless generic_message.valid?
+
+    specific_message = if generic_message.type == 'managed'
+                         ServiceInstanceCreateManagedMessage.new(params)
+                       else
+                         ServiceInstanceCreateUserProvidedMessage.new(params)
+                       end
+
+    unprocessable!(specific_message.errors.full_messages) unless specific_message.valid?
+    specific_message
+  end
+
+  def fetch_writable_service_instance(guid)
+    service_instance = ServiceInstance.first(guid: guid)
+    service_instance_not_found! unless service_instance && can_read_service_instance?(service_instance)
+    unauthorized! unless can_write_space?(service_instance.space)
+    service_instance
+  end
+
+  def enqueue_delete_job(service_instance)
+    delete_job = V3::DeleteServiceInstanceJob.new(service_instance.guid, user_audit_info)
+    pollable_job = Jobs::Enqueuer.new(delete_job, queue: Jobs::Queues.generic).enqueue_pollable
+    pollable_job.guid
+  end
+
   def unreadable_error_message(service_instance_name, unreadable_space_guids)
     if unreadable_space_guids.any?
       unreadable_guid_list = unreadable_space_guids.map { |g| "'#{g}'" }.join(', ')
@@ -349,22 +367,8 @@ def can_write_space?(space)
     permission_queryer.can_write_to_space?(space.guid)
   end
 
-  def build_create_message(params)
-    generic_message = ServiceInstanceCreateMessage.new(params)
-    unprocessable!(generic_message.errors.full_messages) unless generic_message.valid?
-
-    specific_message = if generic_message.type == 'managed'
-                         ServiceInstanceCreateManagedMessage.new(params)
-                       else
-                         ServiceInstanceCreateUserProvidedMessage.new(params)
-                       end
-
-    unprocessable!(specific_message.errors.full_messages) unless specific_message.valid?
-    specific_message
-  end
-
-  def service_instance_not_found!
-    resource_not_found!(:service_instance)
+  def admin?
+    permission_queryer.can_write_globally?
   end
 
   def service_plan_valid?(service_plan, space)
@@ -373,6 +377,10 @@ def service_plan_valid?(service_plan, space)
       service_plan.visible_in_space?(space)
   end
 
+  def service_instance_not_found!
+    resource_not_found!(:service_instance)
+  end
+
   def unprocessable_space!
     unprocessable!('Invalid space. Ensure that the space exists and you have access to it.')
   end

spec/unit/actions/v3/service_instance_delete_spec.rb

@@ -55,20 +55,16 @@ module CloudController
               with(:delete, instance_of(UserProvidedServiceInstance))
           end
 
-          it 'returns nothing' do
-            expect(subject.delete(service_instance)).to be_nil
+          it 'returns true' do
+            expect(subject.delete(service_instance)).to be_truthy
           end
         end
 
         context 'managed service instances' do
           let!(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make }
 
-          it 'enqueues a job and returns the job guid' do
-            job_guid = subject.delete(service_instance)
-            job = VCAP::CloudController::PollableJobModel.last
-
-            expect(job.guid).to eq(job_guid)
-            expect(job.resource_guid).to eq(service_instance.guid)
+          it 'returns false' do
+            expect(subject.delete(service_instance)).to be_falsey
           end
         end