Attachment 62774 Details for Bug 15165 – [SIGNED-OFF] Bug 15165: Set amount parameter required, define its data type and confirm amount > 0

[patch] [SIGNED-OFF] Bug 15165: Set amount parameter required, define its data type and confirm amount > 0

SIGNED-OFF-Bug-15165-Set-amount-parameter-required.patch (text/plain), 9.87 KB, created by Josef Moravec on 2017-04-27 11:54:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josef Moravec

Created: 2017-04-27 11:54:20 UTC

Size: 9.87 KB

patch

obsolete

>From f51e6c720899e24652fabf48b1fef0645b77f668 Mon Sep 17 00:00:00 2001
>From: Lari Taskula <lari.taskula@jns.fi>
>Date: Fri, 3 Feb 2017 14:54:42 +0200
>Subject: [PATCH] [SIGNED-OFF] Bug 15165: Set amount parameter required, define
> its data type and confirm amount > 0
>
>This patch enforces "amount" to be required when paying towards accountline(s).
>It also specifies the data type of amount, so that nonsense amounts like "strings"
>cannot go through. Also, defines minimum amount as 0 and exclusiveMinimum true so
>that the amount must be greater than 0 (and not equal).
>
>This patch also wraps the operations in try/catch as now recommended. Possible
>enhancements to Koha::Account->pay could also be done so that we would catch
>Koha::Exceptions in the case of invalid input. However, this is probably outside
>the scope of this bug and would require more testing on other components as well.
>After all, we can handle the input nicely with Swagger alone.
>
>To test:
>1. prove t/db_dependent/api/v1/accountlines.t
>2. prove t/db_dependent/api/v1/patrons.t
>
>Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
>---
> Koha/REST/V1/Accountline.pm            | 57 ++++++++++++++++++++--------------
> Koha/REST/V1/Patron.pm                 | 53 ++++++++++++++++++-------------
> api/v1/swagger/paths/accountlines.json | 21 +++++++++----
> api/v1/swagger/paths/patrons.json      | 11 +++++--
> t/db_dependent/api/v1/accountlines.t   | 25 +++++++++++++--
> 5 files changed, 110 insertions(+), 57 deletions(-)
>
>diff --git a/Koha/REST/V1/Accountline.pm b/Koha/REST/V1/Accountline.pm
>index 57f83dd..f65a13b 100644
>--- a/Koha/REST/V1/Accountline.pm
>+++ b/Koha/REST/V1/Accountline.pm
>@@ -25,6 +25,8 @@ use C4::Auth qw( haspermission );
> use Koha::Account::Lines;
> use Koha::Account;
> 
>+use Try::Tiny;
>+
> sub list {
>     my ($c, $args, $cb) = @_;
> 
>@@ -55,33 +57,40 @@ sub edit {
> sub pay {
>     my ($c, $args, $cb) = @_;
> 
>-    my $accountline = Koha::Account::Lines->find($args->{accountlines_id});
>-    unless ($accountline) {
>-        return $c->$cb({error => "Accountline not found"}, 404);
>-    }
>-
>-    my $body = $c->req->json;
>-    my $amount = $body->{amount};
>-    my $note = $body->{note} || '';
>-
>-    if ($amount && !looks_like_number($amount)) {
>-        return $c->$cb({error => "Invalid amount"}, 400);
>-    }
>+    return try {
>+        my $accountline = Koha::Account::Lines->find($args->{accountlines_id});
>+        unless ($accountline) {
>+            return $c->$cb({error => "Accountline not found"}, 404);
>+        }
> 
>-    Koha::Account->new(
>-        {
>-            patron_id => $accountline->borrowernumber,
>+        my $body = $c->req->json;
>+        my $amount = $body->{amount};
>+        my $note = $body->{note} || '';
>+
>+        Koha::Account->new(
>+            {
>+                patron_id => $accountline->borrowernumber,
>+            }
>+          )->pay(
>+            {
>+                lines  => [$accountline],
>+                amount => $amount,
>+                note => $note,
>+            }
>+          );
>+
>+        $accountline = Koha::Account::Lines->find($args->{accountlines_id});
>+        return $c->$cb($accountline->unblessed(), 200);
>+    } catch {
>+        if ($_->isa('DBIx::Class::Exception')) {
>+            return $c->$cb({ error => $_->msg }, 500);
>         }
>-      )->pay(
>-        {
>-            lines  => [$accountline],
>-            amount => $amount,
>-            note => $note,
>+        else {
>+            return $c->$cb({
>+                error => 'Something went wrong, check the logs.'
>+            }, 500);
>         }
>-      );
>-
>-    $accountline = Koha::Account::Lines->find($args->{accountlines_id});
>-    return $c->$cb($accountline->unblessed(), 200);
>+    };
> }
> 
> 
>diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm
>index 3fbb05f..f229d98 100644
>--- a/Koha/REST/V1/Patron.pm
>+++ b/Koha/REST/V1/Patron.pm
>@@ -25,6 +25,8 @@ use C4::Auth qw( haspermission );
> use Koha::Patrons;
> use Koha::Account;
> 
>+use Try::Tiny;
>+
> sub list {
>     my ($c, $args, $cb) = @_;
> 
>@@ -51,31 +53,38 @@ sub get {
> sub pay {
>     my ($c, $args, $cb) = @_;
> 
>-    my $patron = Koha::Patrons->find($args->{borrowernumber});
>-    unless ($patron) {
>-        return $c->$cb({error => "Patron not found"}, 404);
>-    }
>-
>-    my $body = $c->req->json;
>-    my $amount = $body->{amount};
>-    my $note = $body->{note} || '';
>-
>-    unless ($amount && looks_like_number($amount)) {
>-        return $c->$cb({error => "Invalid amount"}, 400);
>-    }
>+    return try {
>+        my $patron = Koha::Patrons->find($args->{borrowernumber});
>+        unless ($patron) {
>+            return $c->$cb({error => "Patron not found"}, 404);
>+        }
> 
>-    Koha::Account->new(
>-        {
>-            patron_id => $args->{borrowernumber},
>+        my $body = $c->req->json;
>+        my $amount = $body->{amount};
>+        my $note = $body->{note} || '';
>+
>+        Koha::Account->new(
>+            {
>+                patron_id => $args->{borrowernumber},
>+            }
>+          )->pay(
>+            {
>+                amount => $amount,
>+                note => $note,
>+            }
>+          );
>+
>+        return $c->$cb('', 204);
>+    } catch {
>+        if ($_->isa('DBIx::Class::Exception')) {
>+            return $c->$cb({ error => $_->msg }, 500);
>         }
>-      )->pay(
>-        {
>-            amount => $amount,
>-            note => $note,
>+        else {
>+            return $c->$cb({
>+                error => 'Something went wrong, check the logs.'
>+            }, 500);
>         }
>-      );
>-
>-    return $c->$cb('', 204);
>+    };
> }
> 
> 1;
>diff --git a/api/v1/swagger/paths/accountlines.json b/api/v1/swagger/paths/accountlines.json
>index 1d262e0..f520c12 100644
>--- a/api/v1/swagger/paths/accountlines.json
>+++ b/api/v1/swagger/paths/accountlines.json
>@@ -55,13 +55,16 @@
>             "type": "object",
>             "properties": {
>               "amount": {
>-                "description": "Amount"
>+                "description": "Amount",
>+                "type": ["number", "null"]
>               },
>               "amountoutstanding": {
>-                "description": "Amount outstanding"
>+                "description": "Amount outstanding",
>+                "type": ["number", "null"]
>               },
>               "note": {
>-                "description": "Accountline note"
>+                "description": "Accountline note",
>+                "type": ["string", "null"]
>               }
>             }
>           }
>@@ -108,17 +111,23 @@
>         {
>           "name": "body",
>           "in": "body",
>+          "required": true,
>           "description": "A JSON object containing fields to modify",
>           "schema": {
>             "type": "object",
>             "properties": {
>               "amount": {
>-                "description": "Amount to pay"
>+                "description": "Amount to pay. If not given, the whole accountline will be paid.",
>+                "type": "number",
>+                "exclusiveMinimum": true,
>+                "minimum": 0
>               },
>               "note": {
>-                "description": "Payment note"
>+                "description": "Payment note",
>+                "type": "string"
>               }
>-            }
>+            },
>+            "required": ["amount"]
>           }
>         }
>       ],
>diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json
>index 5b3253a..695c016 100644
>--- a/api/v1/swagger/paths/patrons.json
>+++ b/api/v1/swagger/paths/patrons.json
>@@ -88,12 +88,17 @@
>             "type": "object",
>             "properties": {
>               "amount": {
>-                "description": "Amount to pay"
>+                "description": "Amount to pay",
>+                "type": "number",
>+                "exclusiveMinimum": true,
>+                "minimum": 0
>               },
>               "note": {
>-                "description": "Payment note"
>+                "description": "Payment note",
>+                "type": ["string", "null"]
>               }
>-            }
>+            },
>+            "required": ["amount"]
>           }
>         }
>       ],
>diff --git a/t/db_dependent/api/v1/accountlines.t b/t/db_dependent/api/v1/accountlines.t
>index a91b160..79ef067 100644
>--- a/t/db_dependent/api/v1/accountlines.t
>+++ b/t/db_dependent/api/v1/accountlines.t
>@@ -18,7 +18,7 @@
> use Modern::Perl;
> 
> 
>-use Test::More tests => 37;
>+use Test::More tests => 43;
> use Test::Mojo;
> use t::lib::TestBuilder;
> 
>@@ -160,7 +160,7 @@ is($accountline_edited->{amountoutstanding}, '-19.000000');
> 
> 
> # Payment tests
>-$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/4562765765/payment");
>+$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/4562765765/payment" => json => { amount => 1000 });
> $tx->req->cookies({name => 'CGISESSID', value => $session->id});
> $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
> $t->request_ok($tx)
>@@ -171,6 +171,27 @@ $tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accou
> $tx->req->cookies({name => 'CGISESSID', value => $session->id});
> $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
> $t->request_ok($tx)
>+  ->status_is(400);
>+
>+$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accountlines_id}/payment"
>+                       => json => { amount => 0 });
>+$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>+$tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+$t->request_ok($tx)
>+  ->status_is(400);
>+
>+$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accountlines_id}/payment"
>+                       => json => { amount => "no" });
>+$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>+$tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+$t->request_ok($tx)
>+  ->status_is(400);
>+
>+$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accountlines_id}/payment"
>+                       => json => { amount => 20 });
>+$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>+$tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+$t->request_ok($tx)
>   ->status_is(200);
> #$t->content_is('toto');
> 
>-- 
>2.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 15165: 44804 | 44805 | 44806 | 45259 | 50189 | 50190 | 50191 | 50790 | 52681 | 52682 | 52683 | 55033 | 55034 | 55035 | 56694 | 56695 | 56696 | 57843 | 57844 | 57845 | 57846 | 59239 | 59240 | 59241 | 59242 | 59932 | 59933 | 59934 | 60180 | 62769 | 62770 | 62771 | 62772 | 62773 | 62774 | 62775