v3(services): refactor bindings models save methods

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

Author: pivotal-marcela-campo

app/actions/service_credential_binding_app_create.rb

@@ -30,12 +30,10 @@ def precursor(service_instance, app: nil, name: nil, volume_mount_services_enabl
           credentials: {}
         }
 
-        ServiceBinding.new(**binding_details).tap do |b|
-          b.save_with_new_operation(
-            {
-              type: 'create',
-              state: 'in progress',
-            }
+        ServiceBinding.new.tap do |b|
+          b.save_with_attributes_and_new_operation(
+            binding_details,
+            CREATE_IN_PROGRESS_OPERATION
           )
         end
       rescue Sequel::ValidationFailed, Sequel::UniqueConstraintViolation => e

app/actions/service_credential_binding_key_create.rb

@@ -25,12 +25,10 @@ def precursor(service_instance, name)
           credentials: {}
         }
 
-        ServiceKey.new(**binding_details).tap do |b|
-          b.save_with_new_operation(
-            {
-              type: 'create',
-              state: 'in progress',
-            }
+        ServiceKey.new.tap do |b|
+          b.save_with_attributes_and_new_operation(
+            binding_details,
+            CREATE_IN_PROGRESS_OPERATION
           )
         end
       rescue Sequel::ValidationFailed => e

app/actions/service_route_binding_create.rb

@@ -16,19 +16,19 @@ def initialize(user_audit_info, audit_hash)
       def precursor(service_instance, route, message:)
         validate!(service_instance, route)
 
-        RouteBinding.db.transaction do
-          b = RouteBinding.new.save_with_new_operation(
-            {
-              service_instance: service_instance,
-              route: route,
-            },
-            {
-              type: 'create',
-              state: 'in progress',
-            }
-          )
-          MetadataUpdate.update(b, message)
-          b
+        binding_details = {
+          service_instance: service_instance,
+          route: route,
+        }
+
+        RouteBinding.new.tap do |b|
+          RouteBinding.db.transaction do
+            b.save_with_attributes_and_new_operation(
+              binding_details,
+              CREATE_IN_PROGRESS_OPERATION
+            )
+            MetadataUpdate.update(b, message)
+          end
         end
       end
 

app/actions/v3/service_binding_create.rb

@@ -10,6 +10,8 @@ class ServiceBindingCreate
       PollingFinished = PollingStatus.new(true, nil).freeze
       ContinuePolling = ->(retry_after) { PollingStatus.new(false, retry_after) }
 
+      CREATE_IN_PROGRESS_OPERATION = { type: 'create', state: 'in progress' }.freeze
+
       def bind(binding, parameters: {}, accepts_incomplete: false)
         client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
         details = client.bind(binding, arbitrary_parameters: parameters, accepts_incomplete: accepts_incomplete)

app/models/services/service_key.rb

@@ -99,11 +99,6 @@ def required_parameters
     end
 
     def save_with_attributes_and_new_operation(attributes, operation)
-      save_with_new_operation(operation, attributes: attributes)
-      self
-    end
-
-    def save_with_new_operation(last_operation, attributes: {})
       ServiceKey.db.transaction do
         self.lock!
         set(attributes.except(:parameters, :route_services_url, :endpoints))
@@ -116,9 +111,10 @@ def save_with_new_operation(last_operation, attributes: {})
         # it is important to create the service key operation with the service key
         # instead of doing self.service_key_operation = x
         # because mysql will deadlock when requests happen concurrently otherwise.
-        ServiceKeyOperation.create(last_operation.merge(service_key_id: self.id))
+        ServiceKeyOperation.create(operation.merge(service_key_id: self.id))
         self.service_key_operation(reload: true)
       end
+      self
     end
   end
 end

spec/unit/models/services/service_key_spec.rb

@@ -156,7 +156,7 @@ def new_model
       end
     end
 
-    describe '#save_with_new_operation' do
+    describe '#save_with_attributes_and_new_operation' do
       let(:service_instance) { ServiceInstance.make }
       let(:binding) {
         ServiceKey.new(
@@ -172,7 +172,7 @@ def new_model
           type: 'create',
           description: '10%'
         }
-        binding.save_with_new_operation(last_operation)
+        binding.save_with_attributes_and_new_operation({}, last_operation)
 
         expect(binding.last_operation.state).to eq 'in progress'
         expect(binding.last_operation.description).to eq '10%'
@@ -186,15 +186,15 @@ def new_model
         end
 
         it 'should rollback the binding' do
-          expect { binding.save_with_new_operation({ state: 'will fail' }) }.to raise_error(Sequel::DatabaseError)
+          expect { binding.save_with_attributes_and_new_operation({}, { state: 'will fail' }) }.to raise_error(Sequel::DatabaseError)
           expect(ServiceKey.where(guid: binding.guid).count).to eq(0)
         end
       end
 
       context 'when called twice' do
-        it 'does saves the second operation' do
-          binding.save_with_new_operation({ state: 'in progress', type: 'create', description: 'description' })
-          binding.save_with_new_operation({ state: 'in progress', type: 'delete' })
+        it 'does save the second operation' do
+          binding.save_with_attributes_and_new_operation({}, { state: 'in progress', type: 'create', description: 'description' })
+          binding.save_with_attributes_and_new_operation({}, { state: 'in progress', type: 'delete' })
 
           expect(binding.last_operation.state).to eq 'in progress'
           expect(binding.last_operation.type).to eq 'delete'
@@ -220,7 +220,7 @@ def new_model
         }
 
         it 'updates the attributes' do
-          binding.save_with_new_operation(last_operation, attributes: attributes)
+          binding.save_with_attributes_and_new_operation(attributes, last_operation)
           binding.reload
           expect(binding.last_operation.state).to eq 'in progress'
           expect(binding.last_operation.description).to eq '10%'
@@ -232,13 +232,15 @@ def new_model
 
         it 'only saves permitted attributes' do
           expect {
-            binding.save_with_new_operation(last_operation, attributes: attributes.merge(
-              parameters: {
-                foo: 'bar',
-                ding: 'dong'
-              },
-              endpoints: [{ host: 'mysqlhost', ports: ['3306'] }],
-            ))
+            binding.save_with_attributes_and_new_operation(attributes.merge(
+                                                             parameters: {
+                                                               foo: 'bar',
+                                                               ding: 'dong'
+                                                             },
+                                                             endpoints: [{ host: 'mysqlhost', ports: ['3306'] }],
+            ),
+              last_operation
+            )
           }.not_to raise_error
         end
       end