From b7c427f3031a01a0303d06e5582e1b7f88ee770e Mon Sep 17 00:00:00 2001 From: Oleksandr Chebotarov Date: Wed, 18 Dec 2024 17:24:58 +0200 Subject: [PATCH 1/4] Remove aggregate_failures --- .../api/v1/credit_notes_controller_spec.rb | 206 ++++++++---------- 1 file changed, 92 insertions(+), 114 deletions(-) diff --git a/spec/requests/api/v1/credit_notes_controller_spec.rb b/spec/requests/api/v1/credit_notes_controller_spec.rb index fc0d2e77867..78d39b79660 100644 --- a/spec/requests/api/v1/credit_notes_controller_spec.rb +++ b/spec/requests/api/v1/credit_notes_controller_spec.rb @@ -31,41 +31,39 @@ it 'returns a credit note' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - - expect(json[:credit_note]).to include( - lago_id: credit_note.id, - sequential_id: credit_note.sequential_id, - number: credit_note.number, - lago_invoice_id: invoice.id, - invoice_number: invoice.number, - credit_status: credit_note.credit_status, - reason: credit_note.reason, - currency: credit_note.currency, - total_amount_cents: credit_note.total_amount_cents, - credit_amount_cents: credit_note.credit_amount_cents, - balance_amount_cents: credit_note.balance_amount_cents, - created_at: credit_note.created_at.iso8601, - updated_at: credit_note.updated_at.iso8601, - applied_taxes: [] - ) + expect(response).to have_http_status(:success) + + expect(json[:credit_note]).to include( + lago_id: credit_note.id, + sequential_id: credit_note.sequential_id, + number: credit_note.number, + lago_invoice_id: invoice.id, + invoice_number: invoice.number, + credit_status: credit_note.credit_status, + reason: credit_note.reason, + currency: credit_note.currency, + total_amount_cents: credit_note.total_amount_cents, + credit_amount_cents: credit_note.credit_amount_cents, + balance_amount_cents: credit_note.balance_amount_cents, + created_at: credit_note.created_at.iso8601, + updated_at: credit_note.updated_at.iso8601, + applied_taxes: [] + ) - expect(json[:credit_note][:items].count).to eq(2) + expect(json[:credit_note][:items].count).to eq(2) - item = credit_note_items.first - expect(json[:credit_note][:items][0]).to include( - lago_id: item.id, - amount_cents: item.amount_cents, - amount_currency: item.amount_currency - ) + item = credit_note_items.first + expect(json[:credit_note][:items][0]).to include( + lago_id: item.id, + amount_cents: item.amount_cents, + amount_currency: item.amount_currency + ) - expect(json[:credit_note][:items][0][:fee][:item]).to include( - type: item.fee.fee_type, - code: item.fee.item_code, - name: item.fee.item_name - ) - end + expect(json[:credit_note][:items][0][:fee][:item]).to include( + type: item.fee.fee_type, + code: item.fee.item_code, + name: item.fee.item_name + ) end context 'when credit note does not exists' do @@ -115,12 +113,9 @@ it 'updates the credit note' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - - expect(json[:credit_note][:lago_id]).to eq(credit_note.id) - expect(json[:credit_note][:refund_status]).to eq('succeeded') - end + expect(response).to have_http_status(:success) + expect(json[:credit_note][:lago_id]).to eq(credit_note.id) + expect(json[:credit_note][:refund_status]).to eq('succeeded') end end @@ -155,10 +150,8 @@ it 'enqueues a job to generate the PDF' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - expect(CreditNotes::GeneratePdfJob).to have_been_enqueued - end + expect(response).to have_http_status(:success) + expect(CreditNotes::GeneratePdfJob).to have_been_enqueued end context 'when a file is attached to the credit note' do @@ -167,10 +160,8 @@ it 'returns the credit note object' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - expect(json[:credit_note]).to be_present - end + expect(response).to have_http_status(:success) + expect(json[:credit_note]).to be_present end end @@ -226,12 +217,10 @@ it 'returns a list of credit_notes' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - expect(json[:credit_notes].count).to eq(2) - expect(json[:credit_notes].first[:items]).to be_empty - expect(json[:credit_notes].map { |i| i[:lago_id] }).to match_array credit_note_ids - end + expect(response).to have_http_status(:success) + expect(json[:credit_notes].count).to eq(2) + expect(json[:credit_notes].first[:items]).to be_empty + expect(json[:credit_notes].map { |i| i[:lago_id] }).to match_array credit_note_ids end context 'with pagination' do @@ -240,18 +229,16 @@ it 'returns the metadata' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - expect(json[:credit_notes].count).to eq(1) - - expect(json[:meta]).to include( - current_page: 1, - next_page: 2, - prev_page: nil, - total_pages: 2, - total_count: 2 - ) - end + expect(response).to have_http_status(:success) + expect(json[:credit_notes].count).to eq(1) + + expect(json[:meta]).to include( + current_page: 1, + next_page: 2, + prev_page: nil, + total_pages: 2, + total_count: 2 + ) end end @@ -261,12 +248,10 @@ it 'returns credit notes of the customer' do subject - aggregate_failures do - expect(response).to have_http_status(:success) + expect(response).to have_http_status(:success) - expect(json[:credit_notes].count).to eq(1) - expect(json[:credit_notes].first[:lago_id]).to eq(credit_note.id) - end + expect(json[:credit_notes].count).to eq(1) + expect(json[:credit_notes].first[:lago_id]).to eq(credit_note.id) end end end @@ -307,32 +292,30 @@ it 'creates a credit note' do subject - aggregate_failures do - expect(response).to have_http_status(:success) + expect(response).to have_http_status(:success) - expect(json[:credit_note]).to include( - credit_status: 'available', - refund_status: 'pending', - reason: 'duplicated_charge', - description: 'Duplicated charge', - currency: 'EUR', - total_amount_cents: 15, - credit_amount_cents: 10, - balance_amount_cents: 10, - refund_amount_cents: 5, - applied_taxes: [] - ) + expect(json[:credit_note]).to include( + credit_status: 'available', + refund_status: 'pending', + reason: 'duplicated_charge', + description: 'Duplicated charge', + currency: 'EUR', + total_amount_cents: 15, + credit_amount_cents: 10, + balance_amount_cents: 10, + refund_amount_cents: 5, + applied_taxes: [] + ) - expect(json[:credit_note][:items][0][:lago_id]).to be_present - expect(json[:credit_note][:items][0][:amount_cents]).to eq(10) - expect(json[:credit_note][:items][0][:amount_currency]).to eq('EUR') - expect(json[:credit_note][:items][0][:fee][:lago_id]).to eq(fee1.id) + expect(json[:credit_note][:items][0][:lago_id]).to be_present + expect(json[:credit_note][:items][0][:amount_cents]).to eq(10) + expect(json[:credit_note][:items][0][:amount_currency]).to eq('EUR') + expect(json[:credit_note][:items][0][:fee][:lago_id]).to eq(fee1.id) - expect(json[:credit_note][:items][1][:lago_id]).to be_present - expect(json[:credit_note][:items][1][:amount_cents]).to eq(5) - expect(json[:credit_note][:items][1][:amount_currency]).to eq('EUR') - expect(json[:credit_note][:items][1][:fee][:lago_id]).to eq(fee2.id) - end + expect(json[:credit_note][:items][1][:lago_id]).to be_present + expect(json[:credit_note][:items][1][:amount_cents]).to eq(5) + expect(json[:credit_note][:items][1][:amount_currency]).to eq('EUR') + expect(json[:credit_note][:items][1][:fee][:lago_id]).to eq(fee2.id) end context 'when invoice is not found' do @@ -355,13 +338,10 @@ it 'voids the credit note' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - - expect(json[:credit_note][:lago_id]).to eq(credit_note.id) - expect(json[:credit_note][:credit_status]).to eq('voided') - expect(json[:credit_note][:balance_amount_cents]).to eq(0) - end + expect(response).to have_http_status(:success) + expect(json[:credit_note][:lago_id]).to eq(credit_note.id) + expect(json[:credit_note][:credit_status]).to eq('voided') + expect(json[:credit_note][:balance_amount_cents]).to eq(0) end context 'when credit note does not exist' do @@ -409,21 +389,19 @@ it 'returns the computed amounts for credit note creation' do subject - aggregate_failures do - expect(response).to have_http_status(:success) - - estimated_credit_note = json[:estimated_credit_note] - expect(estimated_credit_note[:lago_invoice_id]).to eq(invoice.id) - expect(estimated_credit_note[:invoice_number]).to eq(invoice.number) - expect(estimated_credit_note[:currency]).to eq('EUR') - expect(estimated_credit_note[:taxes_amount_cents]).to eq(0) - expect(estimated_credit_note[:sub_total_excluding_taxes_amount_cents]).to eq(100) - expect(estimated_credit_note[:max_creditable_amount_cents]).to eq(100) - expect(estimated_credit_note[:max_refundable_amount_cents]).to eq(100) - expect(estimated_credit_note[:coupons_adjustment_amount_cents]).to eq(0) - expect(estimated_credit_note[:items].first[:amount_cents]).to eq(50) - expect(estimated_credit_note[:applied_taxes]).to be_blank - end + expect(response).to have_http_status(:success) + + estimated_credit_note = json[:estimated_credit_note] + expect(estimated_credit_note[:lago_invoice_id]).to eq(invoice.id) + expect(estimated_credit_note[:invoice_number]).to eq(invoice.number) + expect(estimated_credit_note[:currency]).to eq('EUR') + expect(estimated_credit_note[:taxes_amount_cents]).to eq(0) + expect(estimated_credit_note[:sub_total_excluding_taxes_amount_cents]).to eq(100) + expect(estimated_credit_note[:max_creditable_amount_cents]).to eq(100) + expect(estimated_credit_note[:max_refundable_amount_cents]).to eq(100) + expect(estimated_credit_note[:coupons_adjustment_amount_cents]).to eq(0) + expect(estimated_credit_note[:items].first[:amount_cents]).to eq(50) + expect(estimated_credit_note[:applied_taxes]).to be_blank end context 'with invalid invoice' do From 109f59c78d22a76185449d7052258cfdaa27f045 Mon Sep 17 00:00:00 2001 From: Oleksandr Chebotarov Date: Thu, 19 Dec 2024 11:26:08 +0200 Subject: [PATCH 2/4] Add new filter options to v1::credit_notes#index --- .../api/v1/credit_notes_controller.rb | 28 ++- .../api/v1/credit_notes_controller_spec.rb | 195 ++++++++++++++++-- 2 files changed, 201 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/credit_notes_controller.rb b/app/controllers/api/v1/credit_notes_controller.rb index d5ad6c59587..16f91876199 100644 --- a/app/controllers/api/v1/credit_notes_controller.rb +++ b/app/controllers/api/v1/credit_notes_controller.rb @@ -99,8 +99,18 @@ def index page: params[:page], limit: params[:per_page] || PER_PAGE }, + search_term: params[:search_term], filters: { - customer_external_id: params[:external_customer_id] + currency: params[:currency], + customer_external_id: params[:external_customer_id], + reason: select_valid_reasons(params[:reason]), + credit_status: select_valid_credit_statuses(params[:credit_status]), + refund_status: select_valid_refund_statuses(params[:refund_status]), + invoice_number: params[:invoice_number], + issuing_date_from: (Date.strptime(params[:issuing_date_from]) if valid_date?(params[:issuing_date_from])), + issuing_date_to: (Date.strptime(params[:issuing_date_to]) if valid_date?(params[:issuing_date_to])), + amount_from: params[:amount_from], + amount_to: params[:amount_to] } ) @@ -158,6 +168,22 @@ def update_params params.require(:credit_note).permit(:refund_status) end + def select_valid_credit_statuses(credit_statuses) + Array(credit_statuses) + .select { |credit_status| CreditNote.credit_statuses.key?(credit_status) } + .presence + end + + def select_valid_refund_statuses(refund_statuses) + Array(refund_statuses) + .select { |refund_status| CreditNote.refund_statuses.key?(refund_status) } + .presence + end + + def select_valid_reasons(reasons) + Array(reasons).select { |reason| CreditNote.reasons.key?(reason) }.presence + end + def estimate_params @estimate_params ||= params.require(:credit_note) .permit( diff --git a/spec/requests/api/v1/credit_notes_controller_spec.rb b/spec/requests/api/v1/credit_notes_controller_spec.rb index 78d39b79660..2ca331c5141 100644 --- a/spec/requests/api/v1/credit_notes_controller_spec.rb +++ b/spec/requests/api/v1/credit_notes_controller_spec.rb @@ -197,34 +197,35 @@ describe 'GET /api/v1/credit_notes' do subject { get_with_token(organization, '/api/v1/credit_notes', params) } - let(:second_customer) { create(:customer, organization:) } - let(:second_invoice) { create(:invoice, customer: second_customer, organization:) } - let(:params) { {} } + let(:organization) { customer.organization } + let(:customer) { create(:customer) } - let(:another_customer_credit_note) do - create(:credit_note, invoice: second_invoice, customer: second_invoice.customer) - end + context 'with no params' do + let(:params) { {} } + let(:invoices) { create_pair(:invoice, organization:, customer:) } - let!(:credit_note_ids) do - [ - credit_note, - another_customer_credit_note - ].pluck(:id) - end + let!(:credit_notes) do + invoices.map { |invoice| create(:credit_note, invoice:, customer:) } + end - include_examples 'requires API permission', 'credit_note', 'read' + include_examples 'requires API permission', 'credit_note', 'read' - it 'returns a list of credit_notes' do - subject + it 'returns a list of credit notes' do + subject - expect(response).to have_http_status(:success) - expect(json[:credit_notes].count).to eq(2) - expect(json[:credit_notes].first[:items]).to be_empty - expect(json[:credit_notes].map { |i| i[:lago_id] }).to match_array credit_note_ids + expect(response).to have_http_status(:success) + expect(json[:credit_notes].first[:items]).to be_empty + expect(json[:credit_notes].pluck(:lago_id)).to match_array credit_notes.pluck(:id) + end end context 'with pagination' do let(:params) { {page: 1, per_page: 1} } + let(:invoices) { create_pair(:invoice, organization:, customer:) } + + before do + invoices.map { |invoice| create(:credit_note, invoice:, customer:) } + end it 'returns the metadata' do subject @@ -244,14 +245,166 @@ context 'with external_customer_id filter' do let(:params) { {external_customer_id: customer.external_id} } + let!(:credit_note) { create(:credit_note, customer:) } + + before do + another_customer = create(:customer, organization:) + create(:credit_note, customer: another_customer) + end it 'returns credit notes of the customer' do subject expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to contain_exactly credit_note.id + end + end - expect(json[:credit_notes].count).to eq(1) - expect(json[:credit_notes].first[:lago_id]).to eq(credit_note.id) + context 'with reason filter' do + let(:params) { {reason: matching_reasons} } + let(:matching_reasons) { CreditNote::REASON.sample(2) } + + let!(:matching_credit_notes) do + matching_reasons.map { |reason| create(:credit_note, reason:, customer:) } + end + + before do + create( + :credit_note, + reason: CreditNote::REASON.excluding(matching_reasons).sample, + customer: + ) + end + + it 'returns credit notes with matching reasons' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to match_array matching_credit_notes.pluck(:id) + end + end + + context 'with credit status filter' do + let(:params) { {credit_status: matching_credit_statuses} } + let(:matching_credit_statuses) { CreditNote::CREDIT_STATUS.sample(2) } + + let!(:matching_credit_notes) do + matching_credit_statuses.map do |credit_status| + create(:credit_note, credit_status:, customer:) + end + end + + before do + create( + :credit_note, + credit_status: CreditNote::CREDIT_STATUS.excluding(matching_credit_statuses).sample, + customer: + ) + end + + it 'returns credit notes with matching credit statuses' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to match_array matching_credit_notes.pluck(:id) + end + end + + context 'with refund status filter' do + let(:params) { {refund_status: matching_refund_statuses} } + let(:matching_refund_statuses) { CreditNote::REFUND_STATUS.sample(2) } + + let!(:matching_credit_notes) do + matching_refund_statuses.map do |refund_status| + create(:credit_note, refund_status:, customer:) + end + end + + before do + create( + :credit_note, + refund_status: CreditNote::REFUND_STATUS.excluding(matching_refund_statuses).sample, + customer: + ) + end + + it 'returns credit notes with matching refund statuses' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to match_array matching_credit_notes.pluck(:id) + end + end + + context 'with invoice number filter' do + let(:params) { {invoice_number: matching_credit_note.invoice.number} } + let!(:matching_credit_note) { create(:credit_note, customer:) } + + before { create(:credit_note, customer:) } + + it 'returns credit notes with matching invoice number' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to contain_exactly matching_credit_note.id + end + end + + context 'with issuing date filters' do + let(:params) do + { + issuing_date_from: credit_notes.second.issuing_date, + issuing_date_to: credit_notes.fourth.issuing_date + } + end + + let!(:credit_notes) do + (1..5).to_a.map do |i| + create(:credit_note, issuing_date: i.days.ago, customer:) + end.reverse # from oldest to newest + end + + it 'returns credit notes that were issued between provided dates' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to match_array credit_notes[1..3].pluck(:id) + end + end + + context 'with amount filters' do + let(:params) do + { + amount_from: credit_notes.second.total_amount_cents, + amount_to: credit_notes.fourth.total_amount_cents + } + end + + let!(:credit_notes) do + (1..5).to_a.map do |i| + create(:credit_note, total_amount_cents: i.succ * 1_000, customer:) + end # from smallest to biggest + end + + it 'returns credit notes with total cents amount in provided range' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to match_array credit_notes[1..3].pluck(:id) + end + end + + context 'with search term' do + let(:params) { {search_term: matching_credit_note.invoice.number} } + let!(:matching_credit_note) { create(:credit_note, customer:) } + + before { create(:credit_note, customer:) } + + it 'returns credit notes matching the search terms' do + subject + + expect(response).to have_http_status(:success) + expect(json[:credit_notes].pluck(:lago_id)).to contain_exactly matching_credit_note.id end end end From a4dec30cfaa5acd396afd1422411d2d4e51bea67 Mon Sep 17 00:00:00 2001 From: Oleksandr Chebotarov Date: Thu, 19 Dec 2024 11:36:34 +0200 Subject: [PATCH 3/4] Cleanup data setup for CreditNoteQuery tests --- app/queries/credit_notes_query.rb | 6 +++--- spec/queries/credit_notes_query_spec.rb | 8 ++------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/app/queries/credit_notes_query.rb b/app/queries/credit_notes_query.rb index 5b44416ad64..1e7c0f3f025 100644 --- a/app/queries/credit_notes_query.rb +++ b/app/queries/credit_notes_query.rb @@ -6,15 +6,15 @@ def call credit_notes = paginate(credit_notes) credit_notes = apply_consistent_ordering(credit_notes) - credit_notes = with_currency(credit_notes) if filters.currency + credit_notes = with_currency(credit_notes) if filters.currency.present? credit_notes = with_customer_external_id(credit_notes) if filters.customer_external_id credit_notes = with_customer_id(credit_notes) if filters.customer_id.present? credit_notes = with_reason(credit_notes) if filters.reason.present? credit_notes = with_credit_status(credit_notes) if filters.credit_status.present? credit_notes = with_refund_status(credit_notes) if filters.refund_status.present? - credit_notes = with_invoice_number(credit_notes) unless filters.invoice_number.nil? + credit_notes = with_invoice_number(credit_notes) if filters.invoice_number.present? credit_notes = with_issuing_date_range(credit_notes) if filters.issuing_date_from || filters.issuing_date_to - credit_notes = with_amount_range(credit_notes) if filters.amount_from || filters.amount_to + credit_notes = with_amount_range(credit_notes) if filters.amount_from.present? || filters.amount_to.present? result.credit_notes = credit_notes result diff --git a/spec/queries/credit_notes_query_spec.rb b/spec/queries/credit_notes_query_spec.rb index fbc61ee3430..de057c02330 100644 --- a/spec/queries/credit_notes_query_spec.rb +++ b/spec/queries/credit_notes_query_spec.rb @@ -7,9 +7,8 @@ described_class.call(organization:, search_term:, pagination:, filters:) end - let(:membership) { create(:membership) } - let(:organization) { membership.organization } - let(:customer) { create(:customer, organization:) } + let(:organization) { customer.organization } + let(:customer) { create(:customer) } let(:pagination) { nil } let(:search_term) { nil } @@ -307,7 +306,6 @@ let(:customer) do create( :customer, - organization:, name: "Rick Sanchez", firstname: "Rick Ramon", lastname: "Sanchez Spencer" @@ -344,7 +342,6 @@ let(:customer) do create( :customer, - organization:, name: "Rick Sanchez", firstname: "Rick Ramon", lastname: "Sanchez Spencer" @@ -383,7 +380,6 @@ let(:customer) do create( :customer, - organization:, name: "Rick Sanchez", firstname: "Rick Ramon", lastname: "Sanchez Spencer" From c5868471660b724dfefcb55b4f51d2a2a6dd69f1 Mon Sep 17 00:00:00 2001 From: Oleksandr Chebotarov Date: Fri, 20 Dec 2024 14:58:50 +0200 Subject: [PATCH 4/4] Remove enum checks logic from controller to query object --- .../api/v1/credit_notes_controller.rb | 22 +----- app/queries/credit_notes_query.rb | 27 +++++-- spec/queries/credit_notes_query_spec.rb | 75 ++++++++++++++----- 3 files changed, 81 insertions(+), 43 deletions(-) diff --git a/app/controllers/api/v1/credit_notes_controller.rb b/app/controllers/api/v1/credit_notes_controller.rb index 16f91876199..ae053331f07 100644 --- a/app/controllers/api/v1/credit_notes_controller.rb +++ b/app/controllers/api/v1/credit_notes_controller.rb @@ -103,9 +103,9 @@ def index filters: { currency: params[:currency], customer_external_id: params[:external_customer_id], - reason: select_valid_reasons(params[:reason]), - credit_status: select_valid_credit_statuses(params[:credit_status]), - refund_status: select_valid_refund_statuses(params[:refund_status]), + reason: params[:reason], + credit_status: params[:credit_status], + refund_status: params[:refund_status], invoice_number: params[:invoice_number], issuing_date_from: (Date.strptime(params[:issuing_date_from]) if valid_date?(params[:issuing_date_from])), issuing_date_to: (Date.strptime(params[:issuing_date_to]) if valid_date?(params[:issuing_date_to])), @@ -168,22 +168,6 @@ def update_params params.require(:credit_note).permit(:refund_status) end - def select_valid_credit_statuses(credit_statuses) - Array(credit_statuses) - .select { |credit_status| CreditNote.credit_statuses.key?(credit_status) } - .presence - end - - def select_valid_refund_statuses(refund_statuses) - Array(refund_statuses) - .select { |refund_status| CreditNote.refund_statuses.key?(refund_status) } - .presence - end - - def select_valid_reasons(reasons) - Array(reasons).select { |reason| CreditNote.reasons.key?(reason) }.presence - end - def estimate_params @estimate_params ||= params.require(:credit_note) .permit( diff --git a/app/queries/credit_notes_query.rb b/app/queries/credit_notes_query.rb index 1e7c0f3f025..81ce400ac88 100644 --- a/app/queries/credit_notes_query.rb +++ b/app/queries/credit_notes_query.rb @@ -9,9 +9,9 @@ def call credit_notes = with_currency(credit_notes) if filters.currency.present? credit_notes = with_customer_external_id(credit_notes) if filters.customer_external_id credit_notes = with_customer_id(credit_notes) if filters.customer_id.present? - credit_notes = with_reason(credit_notes) if filters.reason.present? - credit_notes = with_credit_status(credit_notes) if filters.credit_status.present? - credit_notes = with_refund_status(credit_notes) if filters.refund_status.present? + credit_notes = with_reason(credit_notes) if valid_reasons.present? + credit_notes = with_credit_status(credit_notes) if valid_credit_statuses.present? + credit_notes = with_refund_status(credit_notes) if valid_refund_statuses.present? credit_notes = with_invoice_number(credit_notes) if filters.invoice_number.present? credit_notes = with_issuing_date_range(credit_notes) if filters.issuing_date_from || filters.issuing_date_to credit_notes = with_amount_range(credit_notes) if filters.amount_from.present? || filters.amount_to.present? @@ -66,15 +66,15 @@ def with_customer_id(scope) end def with_reason(scope) - scope.where(reason: filters.reason) + scope.where(reason: valid_reasons) end def with_credit_status(scope) - scope.where(credit_status: filters.credit_status) + scope.where(credit_status: valid_credit_statuses) end def with_refund_status(scope) - scope.where(refund_status: filters.refund_status) + scope.where(refund_status: valid_refund_statuses) end def with_invoice_number(scope) @@ -100,4 +100,19 @@ def issuing_date_from def issuing_date_to @issuing_date_to ||= parse_datetime_filter(:issuing_date_to) end + + def valid_credit_statuses + @valid_credit_statuses ||= Array(filters.credit_status) + .select { |credit_status| CreditNote.credit_statuses.key?(credit_status) } + end + + def valid_refund_statuses + @valid_refund_statuses ||= Array(filters.refund_status) + .select { |refund_status| CreditNote.refund_statuses.key?(refund_status) } + end + + def valid_reasons + @valid_reasons ||= Array(filters.reason) + .select { |reason| CreditNote.reasons.key?(reason) } + end end diff --git a/spec/queries/credit_notes_query_spec.rb b/spec/queries/credit_notes_query_spec.rb index de057c02330..655c91978dd 100644 --- a/spec/queries/credit_notes_query_spec.rb +++ b/spec/queries/credit_notes_query_spec.rb @@ -84,15 +84,13 @@ end context "when reason filter applied" do - let(:filters) { {reason: matching_reasons} } - let(:matching_reasons) { CreditNote::REASON.sample(2) } let!(:matching_credit_notes) do matching_reasons.map { |reason| create(:credit_note, reason:, customer:) } end - before do + let!(:non_matching_credit_note) do create( :credit_note, reason: CreditNote::REASON.excluding(matching_reasons).sample, @@ -100,15 +98,28 @@ ) end - it "returns credit notes with matching reasons" do - expect(result).to be_success - expect(result.credit_notes.pluck(:id)).to match_array matching_credit_notes.pluck(:id) + context "with valid options" do + let(:filters) { {reason: matching_reasons} } + + it "returns credit notes with matching reasons" do + expect(result).to be_success + expect(result.credit_notes.pluck(:id)).to match_array matching_credit_notes.pluck(:id) + end + end + + context "with invalid options" do + let(:filters) { {reason: "invalid-reason"} } + + it "returns all credit notes" do + expect(result).to be_success + + expect(result.credit_notes.pluck(:id)) + .to contain_exactly(*matching_credit_notes.pluck(:id), non_matching_credit_note.id) + end end end context "when credit status filter applied" do - let(:filters) { {credit_status: matching_credit_statuses} } - let(:matching_credit_statuses) { CreditNote::CREDIT_STATUS.sample(2) } let!(:matching_credit_notes) do @@ -117,7 +128,7 @@ end end - before do + let!(:non_matching_credit_note) do create( :credit_note, credit_status: CreditNote::CREDIT_STATUS.excluding(matching_credit_statuses).sample, @@ -125,15 +136,28 @@ ) end - it "returns credit notes with matching credit statuses" do - expect(result).to be_success - expect(result.credit_notes.pluck(:id)).to match_array matching_credit_notes.pluck(:id) + context "with valid options" do + let(:filters) { {credit_status: matching_credit_statuses} } + + it "returns credit notes with matching credit statuses" do + expect(result).to be_success + expect(result.credit_notes.pluck(:id)).to match_array matching_credit_notes.pluck(:id) + end + end + + context "with invalid options" do + let(:filters) { {credit_status: "invalid-credit-status"} } + + it "returns all credit notes" do + expect(result).to be_success + + expect(result.credit_notes.pluck(:id)) + .to contain_exactly(*matching_credit_notes.pluck(:id), non_matching_credit_note.id) + end end end context "when refund status filter applied" do - let(:filters) { {refund_status: matching_refund_statuses} } - let(:matching_refund_statuses) { CreditNote::REFUND_STATUS.sample(2) } let!(:matching_credit_notes) do @@ -142,7 +166,7 @@ end end - before do + let!(:non_matching_credit_note) do create( :credit_note, refund_status: CreditNote::REFUND_STATUS.excluding(matching_refund_statuses).sample, @@ -150,9 +174,24 @@ ) end - it "returns credit notes with matching refund statuses" do - expect(result).to be_success - expect(result.credit_notes.pluck(:id)).to match_array matching_credit_notes.pluck(:id) + context "with valid options" do + let(:filters) { {refund_status: matching_refund_statuses} } + + it "returns credit notes with matching refund statuses" do + expect(result).to be_success + expect(result.credit_notes.pluck(:id)).to match_array matching_credit_notes.pluck(:id) + end + end + + context "with invalid options" do + let(:filters) { {refund_status: "invalid-refund-status"} } + + it "returns all credit notes" do + expect(result).to be_success + + expect(result.credit_notes.pluck(:id)) + .to contain_exactly(*matching_credit_notes.pluck(:id), non_matching_credit_note.id) + end end end