From e91a704ab18ce498a6ada65a71baf6841c2a66e2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 16 Jul 2015 11:24:56 +0100 Subject: [PATCH] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares Signed-off-by: Alex Arnaud Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- C4/Installer/PerlDependencies.pm | 5 ++ C4/VirtualShelves.pm | 95 +------------------------------- C4/VirtualShelves/Page.pm | 6 +- Koha/Exceptions.pm | 34 ++++++++++++ Koha/Virtualshelf.pm | 46 +++++++++++++++- Koha/Virtualshelfshare.pm | 74 +++++++++++++++++++++++++ Koha/Virtualshelfshares.pm | 50 +++++++++++++++++ opac/opac-shareshelf.pl | 47 +++++++++++----- t/db_dependent/VirtualShelves.t | 74 ------------------------- t/db_dependent/Virtualshelves.t | 75 ++++++++++++++++++++++++- 10 files changed, 318 insertions(+), 188 deletions(-) create mode 100644 Koha/Exceptions.pm create mode 100644 Koha/Virtualshelfshare.pm create mode 100644 Koha/Virtualshelfshares.pm diff --git a/C4/Installer/PerlDependencies.pm b/C4/Installer/PerlDependencies.pm index 765151e901..e815cdac2a 100644 --- a/C4/Installer/PerlDependencies.pm +++ b/C4/Installer/PerlDependencies.pm @@ -99,6 +99,11 @@ our $PERL_DEPS = { 'required' => '1', 'min_ver' => '1.103' }, + 'Exception::Class' => { + 'usage' => 'Core', + 'required' => '1.39', + 'min_ver' => '1.39' + }, 'HTML::Scrubber' => { 'usage' => 'Core', 'required' => '1', diff --git a/C4/VirtualShelves.pm b/C4/VirtualShelves.pm index c0bc12d07c..68e9a4795d 100644 --- a/C4/VirtualShelves.pm +++ b/C4/VirtualShelves.pm @@ -45,7 +45,6 @@ BEGIN { &ShelfPossibleAction &DelFromShelf &GetBibliosShelves - &AddShare &AcceptShare &RemoveShare &IsSharedList ); @EXPORT_OK = qw( &GetAllShelves &ShelvesMax @@ -390,7 +389,7 @@ sub ShelfPossibleAction { } } elsif($action eq 'acceptshare') { - #the key for accepting is checked later in AcceptShare + #the key for accepting is checked later in Koha::Virtualshelf->share #you must not be the owner, list must be private if( $shelf->{category}==1 ) { return (0, 8) if $shelf->{owner}==$user; @@ -526,98 +525,6 @@ sub HandleDelBorrower { #Koha::Virtualshelf->new->delete too. } -=head2 AddShare - - AddShare($shelfnumber, $key); - -Adds a share request to the virtualshelves table. -Authorization must have been checked, and a key must be supplied. See script -opac-shareshelf.pl for an example. -This request is not yet confirmed. So it has no borrowernumber, it does have an -expiry date. - -=cut - -sub AddShare { - my ($shelfnumber, $key)= @_; - return if !$shelfnumber || !$key; - - my $dbh = C4::Context->dbh; - my $sql = "INSERT INTO virtualshelfshares (shelfnumber, invitekey, sharedate) VALUES (?, ?, NOW())"; - $dbh->do($sql, undef, ($shelfnumber, $key)); - return !$dbh->err; -} - -=head2 AcceptShare - - my $result= AcceptShare($shelfnumber, $key, $borrowernumber); - -Checks acceptation of a share request. -Key must be found for this shelf. Invitation must not have expired. -Returns true when accepted, false otherwise. - -=cut - -sub AcceptShare { - my ($shelfnumber, $key, $borrowernumber)= @_; - return if !$shelfnumber || !$key || !$borrowernumber; - - my $sql; - my $dbh = C4::Context->dbh; - $sql=" -UPDATE virtualshelfshares -SET invitekey=NULL, sharedate=NOW(), borrowernumber=? -WHERE shelfnumber=? AND invitekey=? AND (sharedate + INTERVAL ? DAY) >NOW() - "; - my $i= $dbh->do($sql, undef, ($borrowernumber, $shelfnumber, $key, SHARE_INVITATION_EXPIRY_DAYS)); - return if !defined($i) || !$i || $i eq '0E0'; #not found - return 1; -} - -=head2 IsSharedList - - my $bool= IsSharedList( $shelfnumber ); - -IsSharedList checks if a (private) list has shares. -Note that such a check would not be useful for public lists. A public list has -no shares, but is visible for anyone by nature.. -Used to determine the list type in the display of Your lists (all private). -Returns boolean value. - -=cut - -sub IsSharedList { - my ($shelfnumber) = @_; - my $dbh = C4::Context->dbh; - my $sql="SELECT id FROM virtualshelfshares WHERE shelfnumber=? AND borrowernumber IS NOT NULL"; - my $sth = $dbh->prepare($sql); - $sth->execute($shelfnumber); - my ($rv)= $sth->fetchrow_array; - return defined($rv); -} - -=head2 RemoveShare - - RemoveShare( $user, $shelfnumber ); - -RemoveShare removes a share for specific shelf and borrower. -Returns true if a record could be deleted. - -=cut - -sub RemoveShare { - my ($user, $shelfnumber)= @_; - my $dbh = C4::Context->dbh; - my $sql=" -DELETE FROM virtualshelfshares -WHERE borrowernumber=? AND shelfnumber=? - "; - my $n= $dbh->do($sql,undef,($user, $shelfnumber)); - return if !defined($n) || !$n || $n eq '0E0'; #nothing removed - return 1; -} - - sub GetShelfCount { my ($owner, $category) = @_; my @params; diff --git a/C4/VirtualShelves/Page.pm b/C4/VirtualShelves/Page.pm index 59d201cbb7..84f74a82df 100644 --- a/C4/VirtualShelves/Page.pm +++ b/C4/VirtualShelves/Page.pm @@ -389,7 +389,8 @@ sub shelfpage { } #remove a share if(/REMSHR/) { - RemoveShare($loggedinuser, $number); + my $shelf = Koha::Virtualshelves->find( $number ); + $shelf->delete_share( $loggedinuser ); delete $shelflist->{$number} if exists $shelflist->{$number}; $stay=0; next; @@ -461,7 +462,8 @@ sub shelfpage { $shelflist->{$element}->{ownername} = defined($member) ? $member->{firstname} . " " . $member->{surname} : ''; $numberCanManage++ if $canmanage; # possibly outmoded if ( $shelflist->{$element}->{'category'} eq '1' ) { - $shelflist->{$element}->{shares} = IsSharedList($element); + my $shelf = Koha::Virtualshelves->find( $element ); + $shelflist->{$element}->{shares} = $shelf->is_shared; push( @shelveslooppriv, $shelflist->{$element} ); } else { push( @shelvesloop, $shelflist->{$element} ); diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm new file mode 100644 index 0000000000..f6534e050a --- /dev/null +++ b/Koha/Exceptions.pm @@ -0,0 +1,34 @@ +package Koha::Exceptions; + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Exception' => { + description => 'Something went wrong!', + }, + + 'Koha::Exceptions::DuplicateObject' => { + isa => 'Koha::Exceptions::Exception', + description => 'Same object already exists', + }, + + 'Koha::Exceptions::Virtualshelves::DuplicateObject' => { + isa => 'Koha::Exceptions::DuplicateObject', + description => "poeut", + }, + 'Koha::Exceptions::Virtualshelves::InvalidInviteKey' => { + isa => 'Koha::Exceptions::Exception', + description => 'Invalid key on accepting the share', + }, + 'Koha::Exceptions::Virtualshelves::InvalidKeyOnSharing' => { + isa => 'Koha::Exceptions::Exception', + description=> 'Invalid key on sharing a shelf', + }, + 'Koha::Exceptions::Virtualshelves::ShareHasExpired' => { + isa => 'Koha::Exceptions::Exception', + description=> 'Cannot share this shelf, the share has expired', + } +); + +1; diff --git a/Koha/Virtualshelf.pm b/Koha/Virtualshelf.pm index a9dd8084cd..0a9ab73457 100644 --- a/Koha/Virtualshelf.pm +++ b/Koha/Virtualshelf.pm @@ -21,7 +21,9 @@ use Carp; use Koha::Database; use Koha::DateUtils qw( dt_from_string ); -use Koha::Exception::DuplicateObject; +use Koha::Exceptions; +use Koha::Virtualshelfshare; +use Koha::Virtualshelfshares; use base qw(Koha::Object); @@ -93,6 +95,48 @@ sub is_shelfname_valid { return $count ? 0 : 1; } +sub get_shares { + my ( $self ) = @_; + return $self->{_result}->virtualshelfshares; +} + +sub share { + my ( $self, $key ) = @_; + unless ( $key ) { + Koha::Exceptions::Virtualshelves::InvalidKeyOnSharing->throw; + } + Koha::Virtualshelfshare->new( + { + shelfnumber => $self->shelfnumber, + invitekey => $key, + sharedate => dt_from_string, + } + )->store; +} + +sub is_shared { + my ( $self ) = @_; + return $self->get_shares->search( + { + borrowernumber => { '!=' => undef }, + } + )->count; +} + +sub remove_share { + my ( $self, $borrowernumber ) = @_; + my $shelves = Koha::Virtualshelfshares->search( + { + shelfnumber => $self->shelfnumber, + borrowernumber => $borrowernumber, + } + ); + return 0 unless $shelves->count; + + # Only 1 share with 1 patron can exist + return $shelves->next->delete; +} + sub type { return 'Virtualshelve'; } diff --git a/Koha/Virtualshelfshare.pm b/Koha/Virtualshelfshare.pm new file mode 100644 index 0000000000..d1a7ffae59 --- /dev/null +++ b/Koha/Virtualshelfshare.pm @@ -0,0 +1,74 @@ +package Koha::Virtualshelfshare; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Carp; +use DateTime; +use DateTime::Duration; + +use Koha::Database; +use Koha::DateUtils; +use Koha::Exceptions; + +use base qw(Koha::Object); + +use constant SHARE_INVITATION_EXPIRY_DAYS => 14; #two weeks to accept + +=head1 NAME + +Koha::Virtualshelfshare - Koha Virtualshelfshare Object class + +=head1 API + +=head2 Class Methods + +=cut + +=head3 type + +=cut + +sub accept { + my ( $self, $invitekey, $borrowernumber ) = @_; + if ( $self->has_expired ) { + Koha::Exceptions::Virtualshelves::ShareHasExpired->throw; + } + if ( $self->invitekey ne $invitekey ) { + Koha::Exceptions::Virtualshelves::InvalidInviteKey->throw; + } + $self->invitekey(undef); + $self->sharedate(dt_from_string); + $self->borrowernumber($borrowernumber); + $self->store; +} + +sub has_expired { + my ($self) = @_; + my $dt_sharedate = dt_from_string( $self->sharedate, 'sql' ); + my $today = dt_from_string; + my $expiration_delay = DateTime::Duration->new( days => SHARE_INVITATION_EXPIRY_DAYS ); + my $has_expired = DateTime->compare( $today, $dt_sharedate->add_duration($expiration_delay) ); + # Note: has_expired = 0 if the share expires today + return $has_expired == 1 ? 1 : 0 +} + +sub type { + return 'Virtualshelfshare'; +} + +1; diff --git a/Koha/Virtualshelfshares.pm b/Koha/Virtualshelfshares.pm new file mode 100644 index 0000000000..e49e335139 --- /dev/null +++ b/Koha/Virtualshelfshares.pm @@ -0,0 +1,50 @@ +package Koha::Virtualshelfshares; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use Koha::Virtualshelf; + +use base qw(Koha::Objects); + +=head1 NAME + +Koha::Virtualshelfshares - Koha Virtualshelfshares Object class + +=head1 API + +=head2 Class Methods + +=cut + +=head3 type + +=cut + +sub type { + return 'Virtualshelfshare'; +} + +sub object_class { + return 'Koha::Virtualshelfshare'; +} + +1; diff --git a/opac/opac-shareshelf.pl b/opac/opac-shareshelf.pl index a27aa03cab..99133cd3bf 100755 --- a/opac/opac-shareshelf.pl +++ b/opac/opac-shareshelf.pl @@ -122,20 +122,35 @@ sub show_accept { #errorcode 5: should be private list #errorcode 8: should not be owner - my $dbkey = keytostring( stringtokey( $param->{key}, 0 ), 1 ); - if ( AcceptShare( $param->{shelfnumber}, $dbkey, $param->{loggedinuser} ) ) - { - notify_owner($param); - - #redirect to view of this shared list - print $param->{query}->redirect( - -uri => SHELVES_URL . $param->{shelfnumber}, - -cookie => $param->{cookie} - ); - exit; - } - else { + # We could have used ->find with the share id, but we don't want to change + # the url sent to the patron + my $shared_shelf = Koha::Virtualshelfshares->search( + { + shelfnumber => $param->{shelfnumber}, + }, + { + order_by => 'sharedate desc', + limit => 1, + } + ); + + if ( $shared_shelf ) { + $shared_shelf = $shared_shelf->next; + my $key = keytostring( stringtokey( $param->{key}, 0 ), 1 ); + my $is_accepted = eval { $shared_shelf->accept( $key, $param->{loggedinuser} ) }; + if ( $is_accepted ) { + notify_owner($param); + + #redirect to view of this shared list + print $param->{query}->redirect( + -uri => SHELVES_URL . $param->{shelfnumber}, + -cookie => $param->{cookie} + ); + exit; + } $param->{errcode} = 7; #not accepted (key not found or expired) + } else { + # This shelf is not shared } } @@ -200,7 +215,11 @@ sub send_invitekey { my @newkey = randomlist( KEYLENGTH, 64 ); #generate a new key #add a preliminary share record - if ( !AddShare( $param->{shelfnumber}, keytostring( \@newkey, 1 ) ) ) { + my $shelf = Koha::Virtualshelves->find( $param->{shelfnumber} ); + my $key = keytostring( \@newkey, 1 ); + my $is_shared = eval { $shelf->share( $key ); }; + # TODO Better error handling, catch the exceptions + if ( $@ or not $is_shared ) { push @{ $param->{fail_addr} }, $a; next; } diff --git a/t/db_dependent/VirtualShelves.t b/t/db_dependent/VirtualShelves.t index f4dc31d380..18f272bae8 100755 --- a/t/db_dependent/VirtualShelves.t +++ b/t/db_dependent/VirtualShelves.t @@ -122,80 +122,6 @@ for my $i (0..9){ $used{$key}++; } -#----------------------- TEST AddShare ----------------------------------------# - -#first count the number of shares in the table -my $sql_sharecount="select count(*) from virtualshelfshares"; -my $cnt1=$dbh->selectrow_array($sql_sharecount); - -#try to add a share without shelfnumber: should fail -AddShare(0, 'abcdefghij'); -my $cnt2=$dbh->selectrow_array($sql_sharecount); -is($cnt1,$cnt2, "Did not add an invalid share record"); - -#add another share: should be okay -#AddShare assumes that you tested if category==private (so we could actually -#be doing something illegal here :) -my $n=$shelves[0]->{number}; -if($n<0) { - ok(1, 'Skip AddShare for shelf -1'); -} -else { - AddShare($n, 'abcdefghij'); - my $cnt3=$dbh->selectrow_array($sql_sharecount); - is(1+$cnt2, $cnt3, "Added one new share record with invitekey"); -} - -#----------------------- TEST AcceptShare -------------------------------------# - -# test accepting a wrong key -my $testkey= 'keyisgone9'; -my $acctest="delete from virtualshelfshares where invitekey=?"; -$dbh->do($acctest,undef,($testkey)); #just be sure it does not exist -$acctest="select shelfnumber from virtualshelves"; -my ($accshelf)= $dbh->selectrow_array($acctest); -is(AcceptShare($accshelf,$testkey,$borrowernumbers[0]),undef,'Did not accept invalid key'); - -# test accepting a good key -if( AddShare($accshelf,$testkey) && $borrowernumbers[0] ) { - is(AcceptShare($accshelf, $testkey, $borrowernumbers[0]),1,'Accepted share'); -} -else { #cannot accept if addshared failed somehow - ok(1, 'Skipped second AcceptShare test'); -} - -#----------------------- TEST IsSharedList ------------------------------------# - -for my $i (0..9) { - my $sql="select count(*) from virtualshelfshares where shelfnumber=? and borrowernumber is not null"; - my $sh=$shelves[$i]->{number}; - my ($n)=$dbh->selectrow_array($sql,undef,($sh)); - is(IsSharedList($sh),$n? 1: '', "Checked IsSharedList for shelf $sh"); -} - -#----------------TEST Koha::Virtualshelf->delete & DelFromShelf functions------------------------# - -for my $i (0..9){ - my $shelfnumber = $shelves[$i]->{number}; - if($shelfnumber<0) { - ok(1, 'Skip Koha::Virtualshelf->delete for shelf -1'); - next; - } - my $status = Koha::Virtualshelves->find($shelfnumber)->delete; - is($status, 1, "deleted shelf $shelfnumber and its contents"); -} - -#----------------------- TEST RemoveShare -------------------------------------# - -my $remshr_test="select borrowernumber, shelfnumber from virtualshelfshares where borrowernumber is not null"; -my @remshr_shelf= $dbh->selectrow_array($remshr_test); -if(@remshr_shelf) { - is(RemoveShare(@remshr_shelf),1,'Removed a shelf share'); -} -else { - ok(1,'Skipped RemoveShare test'); -} - #----------------------- SOME SUBS --------------------------------------------# sub randomname { diff --git a/t/db_dependent/Virtualshelves.t b/t/db_dependent/Virtualshelves.t index 407011fb96..ff0bb1a31a 100644 --- a/t/db_dependent/Virtualshelves.t +++ b/t/db_dependent/Virtualshelves.t @@ -1,12 +1,12 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 1; +use Test::More tests => 2; use C4::Context; use Koha::DateUtils; -use Koha::Virtualshelf; use Koha::Virtualshelves; +use Koha::Virtualshelfshares; use t::lib::TestBuilder; @@ -14,6 +14,7 @@ my $dbh = C4::Context->dbh; $dbh->{AutoCommit} = 0; $dbh->do(q|DELETE FROM virtualshelves|); +$dbh->do(q|DELETE FROM virtualshelfshares|); my $builder = t::lib::TestBuilder->new; @@ -56,7 +57,7 @@ subtest 'CRUD' => sub { } )->store; }; - is( ref($@), 'Koha::Exception::DuplicateObject' ); + is( ref($@), 'Koha::Exceptions::Virtualshelves::DuplicateObject' ); $number_of_shelves = Koha::Virtualshelves->search->count; is( $number_of_shelves, 1, 'To be sure the number of shelves is still 1' ); @@ -78,3 +79,71 @@ subtest 'CRUD' => sub { $number_of_shelves = Koha::Virtualshelves->search->count; is( $number_of_shelves, 1, 'To be sure the shelf has been deleted' ); }; + +subtest 'Sharing' => sub { + plan tests => 18; + my $patron_wants_to_share = $builder->build({ + source => 'Borrower', + }); + my $share_with_me = $builder->build({ + source => 'Borrower', + }); + my $just_another_patron = $builder->build({ + source => 'Borrower', + }); + + my $number_of_shelves_shared = Koha::Virtualshelfshares->search->count; + is( $number_of_shelves_shared, 0, 'No shelves should exist' ); + + my $shelf_to_share = Koha::Virtualshelf->new({ + shelfname => "my first shelf", + owner => $patron_wants_to_share->{borrowernumber}, + category => 1, + } + )->store; + + my $shelf_not_to_share = Koha::Virtualshelf->new({ + shelfname => "my second shelf", + owner => $patron_wants_to_share->{borrowernumber}, + category => 1, + } + )->store; + + my $shared_shelf = eval { $shelf_to_share->share }; + is ( ref( $@ ), 'Koha::Exceptions::Virtualshelves::InvalidKeyOnSharing', 'Do not share if no key given' ); + $shared_shelf = eval { $shelf_to_share->share('this is a valid key') }; + is( ref( $shared_shelf ), 'Koha::Virtualshelfshare', 'On sharing, the method should return a valid Koha::Virtualshelfshare object' ); + + my $another_shared_shelf = eval { $shelf_to_share->share('this is another valid key') }; # Just to have 2 shares in DB + + $number_of_shelves_shared = Koha::Virtualshelfshares->search->count; + is( $number_of_shelves_shared, 2, '2 shares should have been inserted' ); + + my $is_accepted = eval { + $shared_shelf->accept( 'this is an invalid key', $share_with_me->{borrowernumber} ); + }; + is( $is_accepted, undef, 'The share should have not been accepted if the key is invalid' ); + is( ref( $@ ), 'Koha::Exceptions::Virtualshelves::InvalidInviteKey', 'accept with an invalid key should raise an exception' ); + + $is_accepted = $shared_shelf->accept( 'this is a valid key', $share_with_me->{borrowernumber} ); + ok( defined($is_accepted), 'The share should have been accepted if the key valid' ); + + is( $shelf_to_share->is_shared, 1 ); + is( $shelf_not_to_share->is_shared, 0 ); + + is( $shelf_to_share->is_shared_with( $patron_wants_to_share->{borrowernumber} ), 0 , "The shelf should not be shared with the owner" ); + is( $shelf_to_share->is_shared_with( $share_with_me->{borrowernumber} ), 1 , "The shelf should be shared with share_with_me" ); + is( $shelf_to_share->is_shared_with( $just_another_patron->{borrowernumber} ), 0, "The shelf should not be shared with just_another_patron" ); + + is( $shelf_to_share->remove_share( $just_another_patron->{borrowernumber} ), 0, 'No share should be removed if the share has not been done with this patron' ); + $number_of_shelves_shared = Koha::Virtualshelfshares->search->count; + is( $number_of_shelves_shared, 2, 'To be sure no shares have been removed' ); + + is( $shelf_not_to_share->remove_share( $share_with_me->{borrowernumber} ), 0, '0 share should have been removed if the shelf is not share' ); + $number_of_shelves_shared = Koha::Virtualshelfshares->search->count; + is( $number_of_shelves_shared, 2, 'To be sure no shares have been removed' ); + + is( $shelf_to_share->remove_share( $share_with_me->{borrowernumber} ), 1, '1 share should have been removed if the shelf was shared with this patron' ); + $number_of_shelves_shared = Koha::Virtualshelfshares->search->count; + is( $number_of_shelves_shared, 1, 'To be sure the share has been removed' ); +}; -- 2.39.5