From a702f57adf2913541fec8905fdba839aa58eb720 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 1 Mar 2023 15:26:30 +0000 Subject: [PATCH] Bug 21043: Handle exceptions and switch to debit response We were cheating a bit here and expecting a 'debit' to be sent in but a 'line' to be returned. We should really be sending a debit and returning a debit.. so I've update the paths schema as such and we're now coercing the Koha::Account::Line object that's returned by Koha::Account->add_debit into a Koha::Account::Debit object. Longer term it would be nice to convert returns from the various Koha::Account methods to their correct Koha::Account:: objects as apposed to them all being the base ::Line I've also added some code to catch exceptions that can be thrown by Koha::Account->add_debit and added the appropriate 400 errors into the path specs again. Finally.. I added more unit tests to prove the above Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall Signed-off-by: Tomas Cohen Arazi --- Koha/REST/V1/Patrons/Account.pm | 32 +++++++++++++++--- api/v1/swagger/paths/patrons_account.yaml | 6 +++- t/db_dependent/api/v1/patrons_accounts.t | 41 +++++++++++++++++++---- 3 files changed, 67 insertions(+), 12 deletions(-) diff --git a/Koha/REST/V1/Patrons/Account.pm b/Koha/REST/V1/Patrons/Account.pm index ca2850752c..e2d18f5824 100644 --- a/Koha/REST/V1/Patrons/Account.pm +++ b/Koha/REST/V1/Patrons/Account.pm @@ -243,15 +243,18 @@ sub add_debit { Koha::Account::Debit->new_from_api( $c->validation->param('body') ) ->unblessed; - $data->{library_id} = $data->{branchcode}; - $data->{cash_register} = $data->{cash_register_id}; - $data->{item_id} = $data->{itemnumber}; - $data->{interface} = 'api' + $data->{library_id} = delete $data->{branchcode}; + $data->{type} = delete $data->{debit_type_code}; + $data->{cash_register} = delete $data->{register_id}; + $data->{item_id} = delete $data->{itemnumber}; + $data->{transaction_type} = delete $data->{payment_type}; + $data->{interface} = 'api' ; # Should this always be API, or should we allow the API consumer to choose? $data->{user_id} = $patron->borrowernumber ; # Should this be API user OR staff the API may be acting on behalf of? my $debit = $patron->account->add_debit($data); + $debit = Koha::Account::Debit->_new_from_dbic($debit->{_result}); $c->res->headers->location( $c->req->url->to_string . '/' . $debit->id ); @@ -263,6 +266,27 @@ sub add_debit { ); } catch { + if ( blessed $_ ) { + if ( $_->isa('Koha::Exceptions::Account::RegisterRequired') ) { + return $c->render( + status => 400, + openapi => { error => $_->description } + ); + } + elsif ( $_->isa('Koha::Exceptions::Account::AmountNotPositive') ) { + return $c->render( + status => 400, + openapi => { error => $_->description } + ); + } + elsif ( $_->isa('Koha::Exceptions::Account::UnrecognisedType') ) { + return $c->render( + status => 400, + openapi => { error => $_->description } + ); + } + } + $c->unhandled_exception($_); }; } diff --git a/api/v1/swagger/paths/patrons_account.yaml b/api/v1/swagger/paths/patrons_account.yaml index 9b05e11572..d67cfb55bb 100644 --- a/api/v1/swagger/paths/patrons_account.yaml +++ b/api/v1/swagger/paths/patrons_account.yaml @@ -197,7 +197,11 @@ "201": description: Debit added schema: - $ref: "../swagger.yaml#/definitions/account_line" + $ref: "../swagger.yaml#/definitions/debit" + "400": + description: Bad parameter + schema: + $ref: "../swagger.yaml#/definitions/error" "401": description: Authentication required schema: diff --git a/t/db_dependent/api/v1/patrons_accounts.t b/t/db_dependent/api/v1/patrons_accounts.t index 770e8a1af3..6217a0fad2 100755 --- a/t/db_dependent/api/v1/patrons_accounts.t +++ b/t/db_dependent/api/v1/patrons_accounts.t @@ -371,7 +371,7 @@ subtest 'list_debits() test' => sub { subtest 'add_debit() tests' => sub { - plan tests => 13; + plan tests => 18; $schema->storage->txn_begin; @@ -397,7 +397,7 @@ subtest 'add_debit() tests' => sub { my $debit = { amount => 100, description => "A description", - debit_type => "NEW_CARD", + type => "NEW_CARD", user_id => $patron->borrowernumber, interface => 'test', library_id => $library->id, @@ -406,16 +406,16 @@ subtest 'add_debit() tests' => sub { my $ret = $t->post_ok( "//$userid:$password@/api/v1/patrons/$patron_id/account/debits" => json => $debit )->status_is(201)->tx->res->json; - my $account_line = Koha::Account::Lines->find( $ret->{account_line_id} ); + my $account_line = Koha::Account::Debits->find( $ret->{account_line_id} ); is_deeply( $ret, $account_line->to_api, 'Line returned correctly' ); is( $account_line->branchcode, - $library->id, 'Library id is sored correctly' ); + $library->id, 'Library id is stored correctly' ); my $outstanding_debits = $account->outstanding_debits; - is( $outstanding_debits->count, 1 ); - is( $outstanding_debits->total_outstanding, 100 ); + is( $outstanding_debits->count, 1, "One outstanding debit added" ); + is( $outstanding_debits->total_outstanding, 100, "Outstanding debit is 100" ); my $credit_1 = $account->add_credit( { @@ -430,7 +430,7 @@ subtest 'add_debit() tests' => sub { } )->store()->apply( { debits => [$account_line] } ); - is( $account->outstanding_credits->total_outstanding, 0 ); + is( $account->outstanding_credits->total_outstanding, 0, "Credits all applied" ); is( $account->outstanding_debits->total_outstanding, 75, "Credits partially cancelled debit" ); @@ -443,5 +443,32 @@ subtest 'add_debit() tests' => sub { is( $account->outstanding_debits->total_outstanding, 175, "Debit added to total outstanding debits" ); + # Cash register handling and PAYOUTs + t::lib::Mocks::mock_preference( 'UseCashRegisters', 1 ); + my $payout = { + amount => 10, + description => "A description", + type => "PAYOUT", + payout_type => "CASH", + user_id => $patron->borrowernumber, + interface => 'test', + library_id => $library->id, + }; + + $t->post_ok( + "//$userid:$password@/api/v1/patrons/$patron_id/account/debits" => + json => $payout )->status_is(400) + ->json_is( '/error' => 'Account transaction requires a cash register' ); + + my $register = $builder->build_object( + { + class => 'Koha::Cash::Registers', + } + ); + $payout->{cash_register_id} = $register->id; + my $res = $t->post_ok( + "//$userid:$password@/api/v1/patrons/$patron_id/account/debits" => + json => $payout )->status_is(201)->tx->res->json; + $schema->storage->txn_rollback; }; -- 2.39.5