From f3cceec838cef52946027eb055c1edf37808a493 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 8 Dec 2017 09:17:28 -0300 Subject: [PATCH] Bug 19784: Unit tests for /api/v1/patrons This patch adapts the existing endpoint's tests so they expect: - 'patron_id' for 'borrowernumber' - 'library_id' for 'branchcode' - 'category_id' for 'categorycode' In the process, I tried to make the tests more robust, by creating random data that gets deleted to make sure our tests cases can't have false positives. Independent subtests are wrapped inside transactions to avoid eventual interference. To test: - Apply the patch - Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => FAIL: The api doesn't implement the expected behaviour and attributes for endpoint responses --- t/db_dependent/api/v1/patrons.t | 274 ++++++++++++++++++++++------------------ 1 file changed, 149 insertions(+), 125 deletions(-) diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index b52066d153..469893d2be 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -40,104 +40,101 @@ subtest 'list() tests' => sub { plan tests => 2; $schema->storage->txn_begin; - unauthorized_access_tests('GET', undef, undef); + $schema->storage->txn_rollback; subtest 'librarian access tests' => sub { plan tests => 8; - my ($borrowernumber, $sessionid) = create_user_and_session({ - authorized => 1 }); - my $patron = Koha::Patrons->find($borrowernumber); - Koha::Patrons->search({ - borrowernumber => { '!=' => $borrowernumber}, - cardnumber => { LIKE => $patron->cardnumber . "%" } - })->delete; - Koha::Patrons->search({ - borrowernumber => { '!=' => $borrowernumber}, - address2 => { LIKE => $patron->address2 . "%" } - })->delete; + $schema->storage->txn_begin; + + Koha::Patrons->search->delete; + + my ( $patron_id, $session_id ) = create_user_and_session({ authorized => 1 }); + my $patron = Koha::Patrons->find($patron_id); my $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); + $tx->req->env({ REMOTE_ADDR => '127.0.0.1' }); $t->request_ok($tx) ->status_is(200); - $tx = $t->ua->build_tx(GET => '/api/v1/patrons?cardnumber='. - $patron->cardnumber); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $tx = $t->ua->build_tx(GET => '/api/v1/patrons?cardnumber=' . $patron->cardnumber); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); + $tx->req->env({ REMOTE_ADDR => '127.0.0.1' }); $t->request_ok($tx) ->status_is(200) ->json_is('/0/cardnumber' => $patron->cardnumber); $tx = $t->ua->build_tx(GET => '/api/v1/patrons?address2='. $patron->address2); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); + $tx->req->env({ REMOTE_ADDR => '127.0.0.1' }); $t->request_ok($tx) ->status_is(200) ->json_is('/0/address2' => $patron->address2); - }; - $schema->storage->txn_rollback; + $schema->storage->txn_rollback; + }; }; subtest 'get() tests' => sub { plan tests => 3; $schema->storage->txn_begin; - unauthorized_access_tests('GET', -1, undef); + $schema->storage->txn_rollback; subtest 'access own object tests' => sub { plan tests => 4; - my ($patronid, $patronsessionid) = create_user_and_session({ - authorized => 0 }); + $schema->storage->txn_begin; + + my ( $patron_id, $session_id ) = create_user_and_session({ authorized => 0 }); # Access patron's own data even though they have no borrowers flag - my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$patronid"); - $tx->req->cookies({name => 'CGISESSID', value => $patronsessionid}); - $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $patron_id); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); + $tx->req->env({ REMOTE_ADDR => '127.0.0.1' }); $t->request_ok($tx) ->status_is(200); - my $guarantee = $builder->build({ - source => 'Borrower', - value => { - guarantorid => $patronid, + my $guarantee = $builder->build_object({ + class => 'Koha::Patrons', + value => { + guarantorid => $patron_id, } }); # Access guarantee's data even though guarantor has no borrowers flag - my $guaranteenumber = $guarantee->{borrowernumber}; - $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$guaranteenumber"); - $tx->req->cookies({name => 'CGISESSID', value => $patronsessionid}); - $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); + $tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $guarantee->id ); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); + $tx->req->env({ REMOTE_ADDR => '127.0.0.1' }); $t->request_ok($tx) ->status_is(200); + + $schema->storage->txn_rollback; }; subtest 'librarian access tests' => sub { - plan tests => 5; + plan tests => 6; - my ($patron_id) = create_user_and_session({ - authorized => 0 }); - my $patron = Koha::Patrons->find($patron_id); - my ($borrowernumber, $sessionid) = create_user_and_session({ - authorized => 1 }); - my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$patron_id"); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $schema->storage->txn_begin; + + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + my ( undef, $session_id ) = create_user_and_session({ authorized => 1 }); + + my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $patron->id); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(200) - ->json_is('/borrowernumber' => $patron_id) - ->json_is('/surname' => $patron->surname) - ->json_is('/lost' => Mojo::JSON->false ); - }; + ->json_is('/patron_id' => $patron->id) + ->json_is('/category_id' => $patron->categorycode ) + ->json_is('/surname' => $patron->surname) + ->json_is('/lost' => Mojo::JSON->false ); - $schema->storage->txn_rollback; + $schema->storage->txn_rollback; + }; }; subtest 'add() tests' => sub { @@ -145,120 +142,147 @@ subtest 'add() tests' => sub { $schema->storage->txn_begin; - my $categorycode = $builder->build({ source => 'Category' })->{categorycode}; - my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; - my $newpatron = { - address => 'Street', - branchcode => $branchcode, - cardnumber => $branchcode.$categorycode, - categorycode => $categorycode, - city => 'Joenzoo', - surname => "TestUser", - userid => $branchcode.$categorycode, - }; + my $patron = Koha::REST::V1::Patrons::_to_api( + $builder->build_object( { class => 'Koha::Patrons' } )->TO_JSON ); - unauthorized_access_tests('POST', undef, $newpatron); + unauthorized_access_tests('POST', undef, $patron); + + $schema->storage->txn_rollback; subtest 'librarian access tests' => sub { plan tests => 18; - my ($borrowernumber, $sessionid) = create_user_and_session({ - authorized => 1 }); + $schema->storage->txn_begin; + + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + my $newpatron = Koha::REST::V1::Patrons::_to_api( $patron->TO_JSON ); + delete $newpatron->{patron_id}; + + # Create a library just to make sure its ID doesn't exist on the DB + my $library_to_delete = $builder->build_object({ class => 'Koha::Libraries' }); + my $deleted_library_id = $library_to_delete->id; + # Delete library + $library_to_delete->delete; - $newpatron->{branchcode} = "nonexistent"; # Test invalid branchcode + my ( undef, $session_id ) = create_user_and_session({ authorized => 1 }); + + $newpatron->{library_id} = $deleted_library_id; my $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron ); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(400) - ->json_is('/error' => "Given branchcode does not exist"); - $newpatron->{branchcode} = $branchcode; + ->json_is('/error' => "Given library_id does not exist"); + $newpatron->{library_id} = $patron->branchcode; + + # Create a library just to make sure its ID doesn't exist on the DB + my $category_to_delete = $builder->build_object({ class => 'Koha::Patron::Categories' }); + my $deleted_category_id = $category_to_delete->id; + # Delete library + $category_to_delete->delete; - $newpatron->{categorycode} = "nonexistent"; # Test invalid patron category + $newpatron->{category_id} = $deleted_category_id; # Test invalid patron category $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(400) - ->json_is('/error' => "Given categorycode does not exist"); - $newpatron->{categorycode} = $categorycode; + ->json_is('/error' => "Given category_id does not exist"); + $newpatron->{category_id} = $patron->categorycode; $newpatron->{falseproperty} = "Non existent property"; $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(400); delete $newpatron->{falseproperty}; + my $patron_to_delete = $builder->build_object({ class => 'Koha::Patrons' }); + $newpatron = Koha::REST::V1::Patrons::_to_api($patron_to_delete->TO_JSON); + delete $newpatron->{patron_id}; + $patron_to_delete->delete; + $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(201, 'Patron created successfully') - ->json_has('/borrowernumber', 'got a borrowernumber') - ->json_is('/cardnumber', $newpatron->{ cardnumber }) - ->json_is('/surname' => $newpatron->{ surname }) - ->json_is('/firstname' => $newpatron->{ firstname }); - $newpatron->{borrowernumber} = $tx->res->json->{borrowernumber}; + ->json_has('/patron_id', 'got a patron_id') + ->json_is( '/cardnumber' => $newpatron->{ cardnumber }) + ->json_is( '/surname' => $newpatron->{ surname }) + ->json_is( '/firstname' => $newpatron->{ firstname }); $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(409) - ->json_has('/error', 'Fails when trying to POST duplicate'. - ' cardnumber or userid') + ->json_has('/error', 'Fails when trying to POST duplicate cardnumber or userid') ->json_has('/conflict', { userid => $newpatron->{ userid }, cardnumber => $newpatron->{ cardnumber } } ); - }; - $schema->storage->txn_rollback; + $schema->storage->txn_rollback; + }; }; subtest 'update() tests' => sub { plan tests => 2; $schema->storage->txn_begin; - unauthorized_access_tests('PUT', 123, {email => 'nobody@example.com'}); + $schema->storage->txn_rollback; subtest 'librarian access tests' => sub { plan tests => 20; + $schema->storage->txn_begin; + t::lib::Mocks::mock_preference('minPasswordLength', 1); - my ($borrowernumber, $sessionid) = create_user_and_session({ authorized => 1 }); - my ($borrowernumber2, undef) = create_user_and_session({ authorized => 0 }); + my ( $patron_id_1, $session_id ) = create_user_and_session({ authorized => 1 }); + my ( $patron_id_2, undef ) = create_user_and_session({ authorized => 0 }); - my $patron_1 = Koha::Patrons->find($borrowernumber); - my $patron_2 = Koha::Patrons->find($borrowernumber2); - my $newpatron = $patron_2->TO_JSON; + my $patron_1 = Koha::Patrons->find($patron_id_1); + my $patron_2 = Koha::Patrons->find($patron_id_2); + my $newpatron = Koha::REST::V1::Patrons::_to_api($patron_2->TO_JSON); # borrowernumber should not be passed in the request body for PUT delete $newpatron->{borrowernumber}; my $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/-1" => json => $newpatron ); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({name => 'CGISESSID', value => $session_id}); $t->request_ok($tx) ->status_is(404) ->json_has('/error', 'Fails when trying to PUT nonexistent patron'); - $newpatron->{categorycode} = 'nonexistent'; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + # Create a library just to make sure its ID doesn't exist on the DB + my $category_to_delete = $builder->build_object({ class => 'Koha::Patron::Categories' }); + my $deleted_category_id = $category_to_delete->id; + # Delete library + $category_to_delete->delete; + + $newpatron->{category_id} = $deleted_category_id; + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$patron_id_2" => json => $newpatron ); + $tx->req->cookies({name => 'CGISESSID', value => $session_id}); $t->request_ok($tx) ->status_is(400) - ->json_is('/error' => "Given categorycode does not exist"); - $newpatron->{categorycode} = $patron_2->categorycode; - - $newpatron->{branchcode} = 'nonexistent'; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + ->json_is('/error' => "Given category_id does not exist"); + $newpatron->{category_id} = $patron_2->categorycode; + + # Create a library just to make sure its ID doesn't exist on the DB + my $library_to_delete = $builder->build_object({ class => 'Koha::Libraries' }); + my $deleted_library_id = $library_to_delete->id; + # Delete library + $library_to_delete->delete; + + $newpatron->{library_id} = $deleted_library_id; + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$patron_id_2" => json => $newpatron ); + $tx->req->cookies({name => 'CGISESSID', value => $session_id}); $t->request_ok($tx) ->status_is(400) - ->json_is('/error' => "Given branchcode does not exist"); - $newpatron->{branchcode} = $patron_2->branchcode; + ->json_is('/error' => "Given library_id does not exist"); + $newpatron->{library_id} = $patron_2->branchcode; $newpatron->{falseproperty} = "Non existent property"; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$patron_id_2" => json => $newpatron ); + $tx->req->cookies({name => 'CGISESSID', value => $session_id}); $t->request_ok($tx) ->status_is(400) ->json_is('/errors/0/message' => @@ -269,8 +293,8 @@ subtest 'update() tests' => sub { $newpatron->{cardnumber} = $patron_1->cardnumber; $newpatron->{userid} = $patron_1->userid; - $tx = $t->ua->build_tx( PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); - $tx->req->cookies({ name => 'CGISESSID', value => $sessionid }); + $tx = $t->ua->build_tx( PUT => "/api/v1/patrons/$patron_id_2" => json => $newpatron ); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx)->status_is(409) ->json_has( '/error' => "Fails when trying to update to an existing cardnumber or userid") ->json_is( '/conflict', @@ -280,58 +304,58 @@ subtest 'update() tests' => sub { } ); - $newpatron->{ cardnumber } = $borrowernumber.$borrowernumber2; - $newpatron->{ userid } = "user".$borrowernumber.$borrowernumber2; - $newpatron->{ surname } = "user".$borrowernumber.$borrowernumber2; + $newpatron->{ cardnumber } = $patron_id_1.$patron_id_2; + $newpatron->{ userid } = "user".$patron_id_1.$patron_id_2; + $newpatron->{ surname } = "user".$patron_id_1.$patron_id_2; $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $patron_2->id => json => $newpatron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({name => 'CGISESSID', value => $session_id}); $t->request_ok($tx) ->status_is(200, 'Patron updated successfully') ->json_has($newpatron); is(Koha::Patrons->find( $patron_2->id )->cardnumber, $newpatron->{ cardnumber }, 'Patron is really updated!'); - }; - $schema->storage->txn_rollback; + $schema->storage->txn_rollback; + }; }; subtest 'delete() tests' => sub { plan tests => 2; $schema->storage->txn_begin; - unauthorized_access_tests('DELETE', 123, undef); + $schema->storage->txn_rollback; subtest 'librarian access test' => sub { plan tests => 4; - my ($borrowernumber, $sessionid) = create_user_and_session({ - authorized => 1 }); - my ($borrowernumber2, $sessionid2) = create_user_and_session({ - authorized => 0 }); + $schema->storage->txn_begin; + + my ( undef, $session_id ) = create_user_and_session({ authorized => 1 }); + my ( $patron_id, undef ) = create_user_and_session({ authorized => 0 }); my $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/-1"); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(404, 'Patron not found'); - $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/$borrowernumber2"); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/$patron_id"); + $tx->req->cookies({ name => 'CGISESSID', value => $session_id }); $t->request_ok($tx) ->status_is(200, 'Patron deleted successfully'); - }; - $schema->storage->txn_rollback; + $schema->storage->txn_rollback; + }; }; # Centralized tests for 401s and 403s assuming the endpoint requires # borrowers flag for access sub unauthorized_access_tests { - my ($verb, $patronid, $json) = @_; + my ($verb, $patron_id, $json) = @_; my $endpoint = '/api/v1/patrons'; - $endpoint .= ($patronid) ? "/$patronid" : ''; + $endpoint .= ($patron_id) ? "/$patron_id" : ''; subtest 'unauthorized access tests' => sub { plan tests => 5; @@ -340,11 +364,11 @@ sub unauthorized_access_tests { $t->request_ok($tx) ->status_is(401); - my ($borrowernumber, $sessionid) = create_user_and_session({ + my ($borrowernumber, $session_id) = create_user_and_session({ authorized => 0 }); $tx = $t->ua->build_tx($verb => $endpoint => json => $json); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); + $tx->req->cookies({name => 'CGISESSID', value => $session_id}); $t->request_ok($tx) ->status_is(403) ->json_is('/required_permissions', {"borrowers" => "1"}); -- 2.15.1