Refactor the Ci::Minutes::Additional pack service

In order to support the creation of multiple packs in one
request, this will refactor the CreateService to support
the passing of an array of new packs

Changelog: changed
EE: true

ee/app/services/ci/minutes/additional_packs/create_service.rb

@@ -4,54 +4,53 @@ module Ci
   module Minutes
     module AdditionalPacks
       class CreateService < ::Ci::Minutes::AdditionalPacks::BaseService
-        def initialize(current_user, namespace, params = {})
+        def initialize(current_user, namespace, packs = [])
           @current_user = current_user
           @namespace = namespace
-          @purchase_xid = params[:purchase_xid]
-          @expires_at = params[:expires_at]
-          @number_of_minutes = params[:number_of_minutes]
+          @packs = packs
         end
 
         def execute
           authorize_current_user!
 
-          if additional_pack.persisted? || save_additional_pack
-            reset_ci_minutes!
+          Ci::Minutes::AdditionalPack.transaction do
+            @additional_packs = packs.collect { |pack| find_or_create_pack!(pack) }
 
+            reset_ci_minutes!
             successful_response
-          else
-            error_response
           end
+        rescue ActiveRecord::RecordInvalid
+          error_response
         end
 
         private
 
-        attr_reader :current_user, :namespace, :purchase_xid, :expires_at, :number_of_minutes
+        attr_reader :current_user, :namespace, :packs, :additional_packs
 
         # rubocop: disable CodeReuse/ActiveRecord
-        def additional_pack
-          @additional_pack ||= Ci::Minutes::AdditionalPack.find_or_initialize_by(
+        def find_or_create_pack!(pack)
+          additional_pack = Ci::Minutes::AdditionalPack.find_or_initialize_by(
             namespace: namespace,
-            purchase_xid: purchase_xid
+            purchase_xid: pack[:purchase_xid]
           )
-        end
-        # rubocop: enable CodeReuse/ActiveRecord
 
-        def save_additional_pack
-          additional_pack.assign_attributes(
-            expires_at: expires_at,
-            number_of_minutes: number_of_minutes
+          return additional_pack if additional_pack.persisted?
+
+          additional_pack.update!(
+            expires_at: pack[:expires_at],
+            number_of_minutes: pack[:number_of_minutes]
           )
 
-          additional_pack.save
+          additional_pack
         end
+        # rubocop: enable CodeReuse/ActiveRecord
 
         def successful_response
-          success({ additional_pack: additional_pack })
+          success({ additional_packs: additional_packs })
         end
 
         def error_response
-          error('Unable to save additional pack')
+          error('Unable to save additional packs')
         end
 
         def reset_ci_minutes!

ee/lib/api/ci/minutes.rb

@@ -13,18 +13,20 @@ module API
         end
         params do
           requires :id, type: String, desc: 'The ID of a namespace'
-          requires :number_of_minutes, type: Integer, desc: 'Number of additional minutes purchased'
-          requires :expires_at, type: Date, desc: 'The expiry date for the purchase'
-          requires :purchase_xid, type: String, desc: 'Purchase ID for the additional minutes'
+          requires :packs, type: Array, desc: 'An array of additional purchased minutes packs' do
+            requires :number_of_minutes, type: Integer, desc: 'Number of additional minutes purchased'
+            requires :expires_at, type: Date, desc: 'The expiry date for the purchase'
+            requires :purchase_xid, type: String, desc: 'Purchase ID for the additional minutes'
+          end
         end
         post ':id/minutes' do
           namespace = find_namespace(params[:id])
           not_found!('Namespace') unless namespace
 
-          result = ::Ci::Minutes::AdditionalPacks::CreateService.new(current_user, namespace, params).execute
+          result = ::Ci::Minutes::AdditionalPacks::CreateService.new(current_user, namespace, params[:packs]).execute
 
           if result[:status] == :success
-            present result[:additional_pack], with: ::EE::API::Entities::Ci::Minutes::AdditionalPack
+            present result[:additional_packs], with: ::EE::API::Entities::Ci::Minutes::AdditionalPack
           else
             bad_request!(result[:message])
           end

ee/spec/requests/api/ci/minutes_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe API::Ci::Minutes do
     let_it_be(:user) { create(:user) }
 
     let(:namespace_id) { namespace.id }
-    let(:payload) do
+    let(:minutes_pack) do
       {
         number_of_minutes: 10_000,
         expires_at: (Date.current + 1.year).to_s,
@@ -24,6 +24,8 @@ RSpec.describe API::Ci::Minutes do
       }
     end
 
+    let(:payload) { { packs: [minutes_pack] } }
+
     subject(:post_minutes) { post api("/namespaces/#{namespace_id}/minutes", user), params: payload }
 
     context 'with insufficient access' do
@@ -47,31 +49,59 @@ RSpec.describe API::Ci::Minutes do
         it 'creates a new additional pack', :aggregate_failures do
           expect { post_minutes }.to change(Ci::Minutes::AdditionalPack, :count).by(1)
 
+          response_pack = json_response.first
+
           expect(response).to have_gitlab_http_status(:success)
-          expect(json_response['number_of_minutes']).to eq payload[:number_of_minutes]
-          expect(json_response['expires_at']).to eq payload[:expires_at]
-          expect(json_response['purchase_xid']).to eq payload[:purchase_xid]
-          expect(json_response['namespace_id']).to eq namespace_id
+          expect(response_pack['number_of_minutes']).to eq minutes_pack[:number_of_minutes]
+          expect(response_pack['expires_at']).to eq minutes_pack[:expires_at]
+          expect(response_pack['purchase_xid']).to eq minutes_pack[:purchase_xid]
+          expect(response_pack['namespace_id']).to eq namespace_id
         end
       end
 
       context 'when the additional pack already exists' do
         before do
-          create(:ci_minutes_additional_pack, purchase_xid: payload[:purchase_xid], namespace: namespace, number_of_minutes: 20_000)
+          create(:ci_minutes_additional_pack, purchase_xid: minutes_pack[:purchase_xid], namespace: namespace, number_of_minutes: 20_000)
         end
 
         it 'does not create a new additional pack and does not update the existing pack' do
           expect { post_minutes }.not_to change(Ci::Minutes::AdditionalPack, :count)
 
           expect(response).to have_gitlab_http_status(:success)
-          expect(json_response['number_of_minutes']).not_to eq 10_0000
+          expect(json_response.first['number_of_minutes']).not_to eq 10_0000
+        end
+      end
+
+      context 'when submitting multiple packs' do
+        context 'when duplicate packs' do
+          let(:payload) { { packs: [minutes_pack, minutes_pack] }}
+
+          it 'creates only one new pack' do
+            expect { post_minutes }.to change(Ci::Minutes::AdditionalPack, :count).by(1)
+          end
+        end
+
+        context 'when the packs are unique' do
+          let(:payload) do
+            {
+              packs: [
+                { number_of_minutes: 1_000, expires_at: (Date.current + 1.year).to_s, purchase_xid: SecureRandom.hex(16) },
+                { number_of_minutes: 1_000, expires_at: (Date.current + 1.year).to_s, purchase_xid: SecureRandom.hex(16) },
+                { number_of_minutes: 1_000, expires_at: (Date.current + 1.year).to_s, purchase_xid: SecureRandom.hex(16) }
+              ]
+            }
+          end
+
+          it 'creates all the packs' do
+            expect { post_minutes }.to change(Ci::Minutes::AdditionalPack, :count).by(3)
+          end
         end
       end
 
       context 'when the additional pack cannot be saved' do
         it 'returns an error' do
           allow_next_instance_of(Ci::Minutes::AdditionalPack) do |instance|
-            allow(instance).to receive(:save).and_return false
+            allow(instance).to receive(:update!).and_raise(ActiveRecord::RecordInvalid)
           end
 
           post_minutes

ee/spec/services/ci/minutes/additional_packs/create_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Ci::Minutes::AdditionalPacks::CreateService do
     let_it_be(:admin) { build(:user, :admin) }
     let_it_be(:non_admin) { build(:user) }
 
-    let(:params) { {} }
+    let(:params) { [] }
 
     subject(:result) { described_class.new(user, namespace, params).execute }
 
@@ -28,40 +28,42 @@ RSpec.describe Ci::Minutes::AdditionalPacks::CreateService do
 
       context 'when a record exists' do
         let(:params) do
-          {
-            expires_at: Date.today + 1.year,
-            purchase_xid: existing_pack.purchase_xid,
-            number_of_minutes: 10_000
-          }
+          [
+            { purchase_xid: existing_pack.purchase_xid, expires_at: Date.today + 1.year, number_of_minutes: 10_000 },
+            { purchase_xid: SecureRandom.hex(16), expires_at: Date.today + 1.year, number_of_minutes: 1_000 }
+          ]
         end
 
         it 'returns success' do
           expect(result[:status]).to eq :success
         end
 
-        it 'returns the existing record' do
-          expect(result[:additional_pack]).to eq existing_pack
+        it 'returns the existing and newly created records' do
+          expect(result[:additional_packs].size).to eq 2
+          expect(result[:additional_packs].first).to eq existing_pack
+          expect(result[:additional_packs].last[:purchase_xid]).to eq params.last[:purchase_xid]
         end
       end
 
       context 'when no record exists' do
         let(:params) do
-          {
-            expires_at: Date.today + 1.year,
-            purchase_xid: 'new-purchase-xid',
-            number_of_minutes: 10_000
-          }
+          [
+            { purchase_xid: SecureRandom.hex(16), expires_at: Date.today + 1.year, number_of_minutes: 1_000 },
+            { purchase_xid: SecureRandom.hex(16), expires_at: Date.today + 1.year, number_of_minutes: 2_000 },
+            { purchase_xid: SecureRandom.hex(16), expires_at: Date.today + 1.year, number_of_minutes: 3_000 }
+          ]
         end
 
-        it 'creates a new record', :aggregate_failures do
-          expect { result }.to change(Ci::Minutes::AdditionalPack, :count).by(1)
+        it 'creates new records', :aggregate_failures do
+          expect { result }.to change(Ci::Minutes::AdditionalPack, :count).by(3)
 
-          pack = result[:additional_pack]
-
-          expect(pack).to be_persisted
-          expect(pack.expires_at).to eq params[:expires_at]
-          expect(pack.purchase_xid).to eq params[:purchase_xid]
-          expect(pack.number_of_minutes).to eq params[:number_of_minutes]
+          result[:additional_packs].each_with_index do |pack, index|
+            expect(pack).to be_persisted
+            expect(pack.expires_at).to eq params[index][:expires_at]
+            expect(pack.purchase_xid).to eq params[index][:purchase_xid]
+            expect(pack.number_of_minutes).to eq params[index][:number_of_minutes]
+            expect(pack.namespace).to eq namespace
+          end
         end
 
         it 'kicks off reset ci minutes service' do
@@ -74,14 +76,15 @@ RSpec.describe Ci::Minutes::AdditionalPacks::CreateService do
           expect(result[:status]).to eq :success
         end
 
-        context 'with invalid params' do
-          let(:params) { { purchase_xid: 'missing-minutes' } }
+        context 'with invalid params', :aggregate_failures do
+          let(:params) { super().push({ purchase_xid: 'missing-minutes' }) }
 
           it 'returns an error' do
             response = result
 
             expect(response[:status]).to eq :error
-            expect(response[:message]).to eq 'Unable to save additional pack'
+            expect(response[:message]).to eq 'Unable to save additional packs'
+            expect(Ci::Minutes::AdditionalPack.count).to eq 0
           end
         end
       end