Merge branch 'vij-ci-minute-pack-array' into 'master'

Refactor the Ci::Minutes::Additional pack service and API

See merge request gitlab-org/gitlab!67513

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 :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)
-
-          pack = result[:additional_pack]
+        it 'creates new records', :aggregate_failures do
+          expect { result }.to change(Ci::Minutes::AdditionalPack, :count).by(3)
 
+          result[:additional_packs].each_with_index do |pack, index|
             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]
+            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