From e1ea3082e5d250131e509b5102758bb7fe5ffe46 Mon Sep 17 00:00:00 2001
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 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.30.2