Attachment 56668 Details for Bug 16330 – Bug 16330: (follow-up) Move validation logic from controller to Koha::Patron

[patch] Bug 16330: (follow-up) Move validation logic from controller to Koha::Patron

Bug-16330-follow-up-Move-validation-logic-from-con.patch (text/plain), 16.21 KB, created by Lari Taskula on 2016-10-19 14:52:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2016-10-19 14:52:26 UTC

Size: 16.21 KB

patch

obsolete

>From 3539aa2b3ebb314141874b459ec5564261a444b8 Mon Sep 17 00:00:00 2001
>From: Lari Taskula <lari.taskula@jns.fi>
>Date: Tue, 18 Oct 2016 20:07:42 +0300
>Subject: [PATCH] Bug 16330: (follow-up) Move validation logic from controller
> to Koha::Patron
>
>This patch moves validation logic from controller into Koha::Patron object and
>takes advantage of Koha::Exceptions for throwing proper exceptions and catching
>them inside controller in order to return correct error messages.
>
>To test:
>1. Apply this patch
>2. Run tests perl t/db_dependent/api/v1/patrons.t
>3. Follow the original test plan and observe it still applying
>---
> Koha/Exceptions.pm              |   4 ++
> Koha/Exceptions/Category.pm     |  17 +++++
> Koha/Exceptions/Library.pm      |  17 +++++
> Koha/Exceptions/Patron.pm       |  17 +++++
> Koha/Patron.pm                  |  59 +++++++++++++++++
> Koha/REST/V1/Patron.pm          | 136 +++++++++++++++-------------------------
> t/db_dependent/Patrons.t        |  30 ++++++++-
> t/db_dependent/api/v1/patrons.t |   6 +-
> 8 files changed, 197 insertions(+), 89 deletions(-)
> create mode 100644 Koha/Exceptions/Category.pm
> create mode 100644 Koha/Exceptions/Library.pm
> create mode 100644 Koha/Exceptions/Patron.pm
>
>diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm
>index dd8647b..fb671fd 100644
>--- a/Koha/Exceptions.pm
>+++ b/Koha/Exceptions.pm
>@@ -21,6 +21,10 @@ use Exception::Class (
>         isa => 'Koha::Exceptions::Exception',
>         description => 'A required parameter is missing'
>     },
>+    'Koha::Exceptions::UnblessedReference' => {
>+        isa => 'Koha::Exceptions::Exception',
>+        description => 'Calling unblessed reference'
>+    },
>     # Virtualshelves exceptions
>     'Koha::Exceptions::Virtualshelves::DuplicateObject' => {
>         isa => 'Koha::Exceptions::DuplicateObject',
>diff --git a/Koha/Exceptions/Category.pm b/Koha/Exceptions/Category.pm
>new file mode 100644
>index 0000000..3487faa
>--- /dev/null
>+++ b/Koha/Exceptions/Category.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Category;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+
>+    'Koha::Exceptions::Category' => {
>+        description => 'Something went wrong!',
>+    },
>+    'Koha::Exceptions::Category::CategorycodeNotFound' => {
>+        isa => 'Koha::Exceptions::Category',
>+        description => "Category does not exist",
>+        fields => ["categorycode"],
>+    },
>+);
>+
>+1;
>diff --git a/Koha/Exceptions/Library.pm b/Koha/Exceptions/Library.pm
>new file mode 100644
>index 0000000..1aea6ad
>--- /dev/null
>+++ b/Koha/Exceptions/Library.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Library;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+
>+    'Koha::Exceptions::Library' => {
>+        description => 'Something went wrong!',
>+    },
>+    'Koha::Exceptions::Library::BranchcodeNotFound' => {
>+        isa => 'Koha::Exceptions::Library',
>+        description => "Library does not exist",
>+        fields => ["branchcode"],
>+    },
>+);
>+
>+1;
>diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm
>new file mode 100644
>index 0000000..c409b4e
>--- /dev/null
>+++ b/Koha/Exceptions/Patron.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Patron;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+
>+    'Koha::Exceptions::Patron' => {
>+        description => 'Something went wrong!',
>+    },
>+    'Koha::Exceptions::Patron::DuplicateObject' => {
>+        isa => 'Koha::Exceptions::Patron',
>+        description => "Patron cardnumber and userid must be unique",
>+        fields => ["conflict"],
>+    },
>+);
>+
>+1;
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index b5449a3..adf59e6 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -2,6 +2,7 @@ package Koha::Patron;
> 
> # Copyright ByWater Solutions 2014
> # Copyright PTFS Europe 2016
>+# Copyright Koha-Suomi Oy 2016
> #
> # This file is part of Koha.
> #
>@@ -26,7 +27,12 @@ use C4::Context;
> use C4::Log;
> use Koha::Database;
> use Koha::DateUtils;
>+use Koha::Exceptions;
>+use Koha::Exceptions::Category;
>+use Koha::Exceptions::Library;
>+use Koha::Exceptions::Patron;
> use Koha::Issues;
>+use Koha::Libraries;
> use Koha::OldIssues;
> use Koha::Patron::Categories;
> use Koha::Patron::Images;
>@@ -267,6 +273,59 @@ sub track_login {
>     $self->lastseen( dt_from_string() )->store;
> }
> 
>+=head3 validate
>+
>+my $patron = $patron->new($body)->validate->store;
>+   $patron = $patron->set($body)->validate->store;
>+
>+Validates Koha::Patron object and throws Koha::Exceptions if validation issues
>+are found.
>+
>+Throws Koha::Exceptions::UnblessedReference if validate() is called
>+       on an unblessed reference.
>+Throws Koha::Exceptions::Patron::DuplicateObject if trying to add a duplicate.
>+Throws Koha::Exceptions::Library::BranchcodeNotFound if branchcode is not found.
>+Throws Koha::Exceptions::Category::CategorycodeNotFound if categorycode is not
>+       found.
>+
>+=cut
>+
>+sub validate {
>+    my ($self) = @_;
>+
>+    Koha::Exceptions::UnblessedReference->throw(
>+        error => 'validate() must be called on a blessed Koha::Patron object'
>+    ) unless $self && ref($self) eq "Koha::Patron";
>+
>+    # patron cardnumber and/or userid unique?
>+    if ($self->cardnumber || $self->userid) {
>+        my $patron = Koha::Patrons->find({cardnumber => $self->cardnumber, userid => $self->userid});
>+        if ($patron && (!$self->in_storage || $self->in_storage
>+            && $self->borrowernumber ne $patron->borrowernumber)) {
>+            Koha::Exceptions::Patron::DuplicateObject->throw(
>+                error => "Patron cardnumber and userid must be unique",
>+                conflict => { cardnumber => $patron->cardnumber, userid => $patron->userid }
>+            );
>+        }
>+    }
>+
>+    my $branch = Koha::Libraries->find({branchcode => $self->branchcode });
>+    unless ($branch) {
>+        Koha::Exceptions::Library::BranchcodeNotFound->throw(
>+            error => "Library does not exist",
>+            branchcode => $self->branchcode,
>+        );
>+    }
>+    my $category = Koha::Patron::Categories->find({ categorycode => $self->categorycode });
>+    unless ($category) {
>+        Koha::Exceptions::Category::CategorycodeNotFound->throw(
>+            error => "Patron category does not exist",
>+            categorycode => $self->categorycode,
>+        );
>+    }
>+    return $self;
>+}
>+
> =head3 type
> 
> =cut
>diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm
>index 1e2f4f8..67755ac 100644
>--- a/Koha/REST/V1/Patron.pm
>+++ b/Koha/REST/V1/Patron.pm
>@@ -23,6 +23,9 @@ use Koha::Patrons;
> use Koha::Patron::Categories;
> use Koha::Libraries;
> 
>+use Scalar::Util qw(blessed);
>+use Try::Tiny;
>+
> sub list {
>     my ($c, $args, $cb) = @_;
> 
>@@ -55,104 +58,69 @@ sub get {
> sub add {
>     my ($c, $args, $cb) = @_;
> 
>-    my $body = $c->req->json;
>+    try {
>+        my $body = $c->req->json;
> 
>-    # patron cardnumber and/or userid unique?
>-    if ($body->{cardnumber} || $body->{userid}) {
>-        my $patron = Koha::Patrons->find({cardnumber => $body->{cardnumber}, userid => $body->{userid} });
>-        if ($patron) {
>-            return $c->$cb({
>-                error => "Patron cardnumber and userid must be unique",
>-                conflict => { cardnumber => $patron->cardnumber, userid => $patron->userid }
>-            }, 409);
>-        }
>-    }
>+        if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given
> 
>-    my $branch = Koha::Libraries->find({branchcode => $body->{branchcode} });
>-    unless ($branch) {
>-        return $c->$cb({error => "Library with branchcode \"" . $body->{branchcode} . "\" does not exist"}, 404);
>+        my $patron = Koha::Patron->new($body)->validate->store;
>+        return $c->$cb($patron->unblessed, 201);
>     }
>-    my $category = Koha::Patron::Categories->find({ categorycode => $body->{categorycode} });
>-    unless ($category) {
>-        return $c->$cb({error => "Patron category \"" . $body->{categorycode} . "\" does not exist"}, 404);
>-    }
>-    # All OK - save new patron
>-
>-    if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given
>-
>-    my $patron = eval {
>-        Koha::Patron->new($body)->store;
>+    catch {
>+        unless (blessed $_ && $_->can('rethrow')) {
>+            return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+        if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) {
>+            return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) {
>+            return $c->$cb({ error => "Library with branchcode \"".$_->branchcode."\" does not exist" }, 404);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) {
>+            return $c->$cb({error => "Patron category \"".$_->categorycode."\" does not exist"}, 404);
>+        }
>+        else {
>+            return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500);
>+        }
>     };
>-
>-    unless ($patron) {
>-        return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500);
>-    }
>-
>-    return $c->$cb($patron->unblessed, 201);
> }
> 
> sub edit {
>     my ($c, $args, $cb) = @_;
> 
>-    my $patron = Koha::Patrons->find($args->{borrowernumber});
>-    unless ($patron) {
>-        return $c->$cb({error => "Patron not found"}, 404);
>-    }
>+    my $patron;
>+    try {
>+        $patron = Koha::Patrons->find($args->{borrowernumber});
>+        my $body = $c->req->json;
> 
>-    my $body = $c->req->json;
>-
>-    # Can we change userid and/or cardnumber? in that case check that they are altered first
>-    if ($body->{cardnumber} || $body->{userid}) {
>-        if ( ($body->{cardnumber} && $body->{cardnumber} ne $patron->cardnumber) || ($body->{userid} && $body->{userid} ne $patron->userid) ) {
>-            my $conflictingPatron = Koha::Patrons->find({cardnumber => $body->{cardnumber}, userid => $body->{userid} });
>-            if ($conflictingPatron) {
>-                return $c->$cb({
>-                    error => "Patron cardnumber and userid must be unique",
>-                    conflict => { cardnumber => $conflictingPatron->cardnumber, userid => $conflictingPatron->userid }
>-                }, 409);
>-            }
>-        }
>-    }
>+        if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given
> 
>-    if ($body->{branchcode}) {
>-        my $branch = Koha::Libraries->find({branchcode => $body->{branchcode} });
>-        unless ($branch) {
>-            return $c->$cb({error => "Library with branchcode \"" . $body->{branchcode} . "\" does not exist"}, 404);
>-        }
>-    }
>+        die unless $patron->set($body)->validate;
> 
>-    if ($body->{categorycode}) {
>-        my $category = Koha::Patron::Categories->find({ categorycode => $body->{categorycode} });
>-        unless ($category) {
>-            return $c->$cb({error => "Patron category \"" . $body->{categorycode} . "\" does not exist"}, 404);
>-        }
>+        return $c->$cb({}, 204) unless $patron->is_changed; # No Content = No changes made
>+        $patron->store;
>+        return $c->$cb($patron->unblessed, 200);
>     }
>-    # ALL OK - Update patron
>-    # Perhaps limit/validate what should be updated here? flags, et.al.
>-    if ($body->{password}) { $body->{password} = hash_password($body->{password}) }; # bcrypt password if given
>-
>-    my $updatedpatron = eval {
>-        $patron->set($body);
>-    };
>-
>-    if ($updatedpatron) {
>-        if ($updatedpatron->is_changed) {
>-
>-            my $res = eval {
>-                $updatedpatron->store;
>-            };
>-
>-            unless ($res) {
>-                return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500);
>-            }
>-            return $c->$cb($res->unblessed, 200);
>-
>-        } else {
>-            return $c->$cb({}, 204); # No Content = No changes made
>+    catch {
>+        unless ($patron) {
>+            return $c->$cb({error => "Patron not found"}, 404);
>         }
>-    } else {
>-        return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500);
>-    }
>+        unless (blessed $_ && $_->can('rethrow')) {
>+            return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+        if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) {
>+            return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) {
>+            return $c->$cb({ error => "Library with branchcode \"".$_->branchcode."\" does not exist" }, 404);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) {
>+            return $c->$cb({error => "Patron category \"".$_->categorycode."\" does not exist"}, 404);
>+        }
>+        else {
>+            return $c->$cb({error => "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+    };
> }
> 
> sub delete {
>diff --git a/t/db_dependent/Patrons.t b/t/db_dependent/Patrons.t
>index 24707df..0dddbbd 100755
>--- a/t/db_dependent/Patrons.t
>+++ b/t/db_dependent/Patrons.t
>@@ -17,8 +17,10 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 17;
>+use Test::More tests => 22;
> use Test::Warn;
>+use Test::Fatal;
>+use t::lib::TestBuilder;
> 
> use C4::Context;
> use Koha::Database;
>@@ -36,6 +38,7 @@ $dbh->{RaiseError} = 1;
> $dbh->do("DELETE FROM issues");
> $dbh->do("DELETE FROM borrowers");
> 
>+my $builder = t::lib::TestBuilder->new();
> my $categorycode =
>   Koha::Database->new()->schema()->resultset('Category')->first()
>   ->categorycode();
>@@ -46,7 +49,7 @@ my $b1 = Koha::Patron->new(
>     {
>         surname      => 'Test 1',
>         branchcode   => $branchcode,
>-        categorycode => $categorycode
>+        categorycode => $categorycode,
>     }
> );
> $b1->store();
>@@ -98,6 +101,29 @@ is( $b->surname(), 'Test 3', "Next returns third patron" );
> $b = $patrons->next();
> is( $b, undef, "Next returns undef" );
> 
>+is($b1->validate, $b1, "Patron b1 validates");
>+isa_ok(exception { $b1->set({ categorycode => "nonexistent" })->validate },
>+   "Koha::Exceptions::Category::CategorycodeNotFound",
>+   "Invalid categorycode");
>+isa_ok(exception { $b1->set({ branchcode => "nonexistent" })->validate },
>+   "Koha::Exceptions::Library::BranchcodeNotFound",
>+   "Invalid branchcode");
>+
>+# test validation for duplicate cardnumber & userid
>+my $patron = $builder->build({
>+    source => 'Borrower',
>+    value => {
>+        branchcode   => $branchcode,
>+        categorycode => $categorycode,
>+        flags        => 0,
>+    }
>+});
>+my $duplicate = $patron;
>+isa_ok(exception { Koha::Patron->new($duplicate)->validate },
>+    "Koha::Exceptions::Patron::DuplicateObject",
>+    "Duplicate patron");
>+
>+
> # Test Reset and iteration in concert
> $patrons->reset();
> foreach my $b ( $patrons->as_list() ) {
>diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t
>index f03f36d..f1d8f84 100644
>--- a/t/db_dependent/api/v1/patrons.t
>+++ b/t/db_dependent/api/v1/patrons.t
>@@ -186,7 +186,7 @@ $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron);
> $tx->req->cookies({name => 'CGISESSID', value => $session->id});
> $t->request_ok($tx)
>   ->status_is(500)
>-  ->json_is('/error' => "Something went wrong, check Koha logs for details");
>+  ->json_is('/error' => "Something went wrong, check Koha logs for details.");
> 
> delete $newpatron->{ falseproperty };
> $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron);
>@@ -213,7 +213,7 @@ $t->request_ok($tx)
>   ->status_is(404)
>   ->json_has('/error', 'Fails when trying to PUT nonexistent patron');
> 
>-$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => {categorycode => "nonexistent"});
>+$tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber } => json => {branchcode => $branchcode, categorycode => "nonexistent"});
> $tx->req->cookies({name => 'CGISESSID', value => $session->id});
> $t->request_ok($tx)
>   ->status_is(404)
>@@ -230,7 +230,7 @@ $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $newpatron->{ borrowernumber
> $tx->req->cookies({name => 'CGISESSID', value => $session->id});
> $t->request_ok($tx)
>   ->status_is(500)
>-  ->json_is('/error' => "Something went wrong, check Koha logs for details");
>+  ->json_is('/error' => "Something went wrong, check Koha logs for details.");
> delete $newpatron->{ falseproperty };
> 
> $newpatron->{ cardnumber } = $patron-> { cardnumber };
>-- 
>1.9.1

Actions: View | Diff | Splinter Review

Attachments on bug 16330: 50803 | 50889 | 51066 | 51131 | 51132 | 53216 | 53217 | 53820 | 54942 | 55147 | 56666 | 56667 | 56668 | 63646 | 63647 | 63649 | 63650 | 69546 | 69547 | 69548 | 69549 | 69550 | 69551 | 69752 | 69762 | 69763 | 69764 | 69765 | 69766 | 69767 | 69768 | 69787 | 70034 | 70035 | 70036 | 70037 | 70038 | 71667 | 71668 | 71669 | 71670 | 71671 | 71920 | 71921 | 71922 | 71923 | 71924 | 71925 | 71933 | 71934 | 71935 | 71936 | 71937 | 71938 | 73434