Bugzilla – Attachment 53657 Details for
Bug 16960
Patron::Modifications should be fixed
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 16960 - Patron::Modifications should be fixed
Bug-16960---PatronModifications-should-be-fixed.patch (text/plain), 19.60 KB, created by
Kyle M Hall (khall)
on 2016-07-22 16:52:38 UTC
(
hide
)
Description:
Bug 16960 - Patron::Modifications should be fixed
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2016-07-22 16:52:38 UTC
Size:
19.60 KB
patch
obsolete
>From ae7163fde93847c91ba25e2413e079675897337f Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >Date: Fri, 22 Jul 2016 16:50:50 +0000 >Subject: [PATCH] Bug 16960 - Patron::Modifications should be fixed > >The changes from opac-memberentry do not reach the table, since the >Patron::Modifications object does not work well. > >Test Plan: >1) Apply this patch >2) Create some patron modification requests >3) Ensure you can approve and deny modifications >4) Ensure patron self registration works >--- > Koha/Patron/Modification.pm | 36 ++++- > Koha/Patron/Modifications.pm | 218 ++------------------------- > mainpage.pl | 3 +- > members/members-update-do.pl | 10 +- > members/members-update.pl | 2 +- > opac/opac-memberentry.pl | 17 +-- > opac/opac-registration-verify.pl | 8 +- > t/db_dependent/Koha_borrower_modifications.t | 135 ++++++----------- > 8 files changed, 109 insertions(+), 320 deletions(-) > >diff --git a/Koha/Patron/Modification.pm b/Koha/Patron/Modification.pm >index a06aec9..8afbd5e 100644 >--- a/Koha/Patron/Modification.pm >+++ b/Koha/Patron/Modification.pm >@@ -27,14 +27,44 @@ use base qw(Koha::Object); > > =head1 NAME > >-Koha::Item - Koha Item object class >- >-=head1 API >+Koha::Patron::Modification - Class represents a requrest to modify or create a patron > > =head2 Class Methods > > =cut > >+=head2 approve >+ >+$m->approve(); >+ >+Commits the pending modifications to the borrower record and removes >+them from the modifications table. >+ >+=cut >+ >+sub approve { >+ my ($self) = @_; >+ >+ my $data = $self->unblessed(); >+ >+ delete $data->{timestamp}; >+ delete $data->{verification_token}; >+ >+ foreach my $key ( keys %$data ) { >+ delete $data->{$key} unless ( defined( $data->{$key} ) ); >+ } >+ >+ my $patron = Koha::Patrons->find( $self->borrowernumber ); >+ >+ return unless $patron; >+ >+ $patron->set($data); >+ >+ if ( $patron->store() ) { >+ return $self->delete(); >+ } >+} >+ > =head3 type > > =cut >diff --git a/Koha/Patron/Modifications.pm b/Koha/Patron/Modifications.pm >index 7e17289..088f7a0 100644 >--- a/Koha/Patron/Modifications.pm >+++ b/Koha/Patron/Modifications.pm >@@ -25,80 +25,20 @@ Koha::Patron::Modifications > use Modern::Perl; > > use C4::Context; >-use C4::Debug; > >-use base qw(Koha::Objects); >- >-=head2 AddModifications >- >-Koha::Patron::Modifications->AddModifications( $data ); >- >-Adds or updates modifications for a patron >- >-Requires either the key borrowernumber, or verification_token >-to be part of the passed in hash. >- >-=cut >- >-sub AddModifications { >- my ( $self, $data ) = @_; >- >- delete $data->{borrowernumber}; >- if( $self->{borrowernumber} ) { >- return if( not keys %$data ); >- $data->{borrowernumber} = $self->{borrowernumber}; >- $data->{verification_token} = ''; >- } >- elsif( $self->{verification_token} ) { >- $data->{verification_token} = $self->{verification_token}; >- $data->{borrowernumber} = 0; >- } >- else { >- return; >- } >+use Koha::Patron::Modification; > >- my $rs = Koha::Database->new()->schema->resultset('BorrowerModification'); >- return $rs->update_or_create($data, { key => 'primary' } ); >-} >- >-=head2 Verify >- >-$verified = Koha::Patron::Modifications->Verify( $verification_token ); >- >-Returns true if the passed in token is valid. >- >-=cut >- >-sub Verify { >- my ( $self, $verification_token ) = @_; >- >- $verification_token = >- ($verification_token) >- ? $verification_token >- : $self->{'verification_token'}; >- >- my $dbh = C4::Context->dbh; >- my $query = " >- SELECT COUNT(*) AS count >- FROM borrower_modifications >- WHERE verification_token = ? >- "; >- my $sth = $dbh->prepare($query); >- $sth->execute($verification_token); >- my $result = $sth->fetchrow_hashref(); >- >- return $result->{'count'}; >-} >+use base qw(Koha::Objects); > >-=head2 GetPendingModificationsCount >+=head2 pending_count > >-$count = Koha::Patron::Modifications->GetPendingModificationsCount(); >+$count = Koha::Patron::Modifications->pending_count(); > >-Returns the number of pending modifications for existing patron. >+Returns the number of pending modifications for existing patrons. > > =cut > >-sub GetPendingModificationsCount { >+sub pending_count { > my ( $self, $branchcode ) = @_; > > my $dbh = C4::Context->dbh; >@@ -119,18 +59,18 @@ sub GetPendingModificationsCount { > $sth->execute(@params); > my $result = $sth->fetchrow_hashref(); > >- return $result->{'count'}; >+ return $result->{count}; > } > >-=head2 GetPendingModifications >+=head2 pending > >-$arrayref = Koha::Patron::Modifications->GetPendingModifications(); >+$arrayref = Koha::Patron::Modifications->pending(); > > Returns an arrayref of hashrefs for all pending modifications for existing patrons. > > =cut > >-sub GetPendingModifications { >+sub pending { > my ( $self, $branchcode ) = @_; > > my $dbh = C4::Context->dbh; >@@ -162,144 +102,6 @@ sub GetPendingModifications { > return \@m; > } > >-=head2 ApproveModifications >- >-Koha::Patron::Modifications->ApproveModifications( $borrowernumber ); >- >-Commits the pending modifications to the borrower record and removes >-them from the modifications table. >- >-=cut >- >-sub ApproveModifications { >- my ( $self, $borrowernumber ) = @_; >- >- $borrowernumber = >- ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; >- >- return unless $borrowernumber; >- >- my $data = $self->GetModifications( { borrowernumber => $borrowernumber } ); >- delete $data->{timestamp}; >- delete $data->{verification_token}; >- >- my $rs = Koha::Database->new()->schema->resultset('Borrower')->search({ >- borrowernumber => $data->{borrowernumber}, >- }); >- if( $rs->update($data) ) { >- $self->DelModifications( { borrowernumber => $borrowernumber } ); >- } >-} >- >-=head2 DenyModifications >- >-Koha::Patron::Modifications->DenyModifications( $borrowernumber ); >- >-Removes the modifications from the table for the given patron, >-without committing the changes to the patron record. >- >-=cut >- >-sub DenyModifications { >- my ( $self, $borrowernumber ) = @_; >- >- $borrowernumber = >- ($borrowernumber) ? $borrowernumber : $self->{'borrowernumber'}; >- >- return unless $borrowernumber; >- >- return $self->DelModifications( { borrowernumber => $borrowernumber } ); >-} >- >-=head2 DelModifications >- >-Koha::Patron::Modifications->DelModifications({ >- [ borrowernumber => $borrowernumber ], >- [ verification_token => $verification_token ] >-}); >- >-Deletes the modifications for the given borrowernumber or verification token. >- >-=cut >- >-sub DelModifications { >- my ( $self, $params ) = @_; >- >- my ( $field, $value ); >- >- if ( $params->{'borrowernumber'} ) { >- $field = 'borrowernumber'; >- $value = $params->{'borrowernumber'}; >- } >- elsif ( $params->{'verification_token'} ) { >- $field = 'verification_token'; >- $value = $params->{'verification_token'}; >- } >- >- return unless $value; >- >- my $dbh = C4::Context->dbh; >- >- $field = $dbh->quote_identifier($field); >- >- my $query = " >- DELETE >- FROM borrower_modifications >- WHERE $field = ? >- "; >- >- my $sth = $dbh->prepare($query); >- return $sth->execute($value); >-} >- >-=head2 GetModifications >- >-$hashref = Koha::Patron::Modifications->GetModifications({ >- [ borrowernumber => $borrowernumber ], >- [ verification_token => $verification_token ] >-}); >- >-Gets the modifications for the given borrowernumber or verification token. >- >-=cut >- >-sub GetModifications { >- my ( $self, $params ) = @_; >- >- my ( $field, $value ); >- >- if ( defined( $params->{'borrowernumber'} ) ) { >- $field = 'borrowernumber'; >- $value = $params->{'borrowernumber'}; >- } >- elsif ( defined( $params->{'verification_token'} ) ) { >- $field = 'verification_token'; >- $value = $params->{'verification_token'}; >- } >- >- return unless $value; >- >- my $dbh = C4::Context->dbh; >- >- $field = $dbh->quote_identifier($field); >- >- my $query = " >- SELECT * >- FROM borrower_modifications >- WHERE $field = ? >- "; >- >- my $sth = $dbh->prepare($query); >- $sth->execute($value); >- my $data = $sth->fetchrow_hashref(); >- >- foreach my $key ( keys %$data ) { >- delete $data->{$key} unless ( defined( $data->{$key} ) ); >- } >- >- return $data; >-} >- > sub _type { > return 'BorrowerModification'; > } >diff --git a/mainpage.pl b/mainpage.pl >index d9e01b7..f4efc65 100755 >--- a/mainpage.pl >+++ b/mainpage.pl >@@ -65,8 +65,7 @@ my $branch = > my $pendingcomments = numberofreviews(0); > my $pendingtags = get_count_by_tag_status(0); > my $pendingsuggestions = CountSuggestion("ASKED"); >-my $pending_borrower_modifications = >- Koha::Patron::Modifications->GetPendingModificationsCount( $branch ); >+my $pending_borrower_modifications = Koha::Patron::Modifications->pending_count( $branch ); > my $pending_discharge_requests = Koha::Patron::Discharge::count({ pending => 1 }); > > $template->param( >diff --git a/members/members-update-do.pl b/members/members-update-do.pl >index f55b7ab..db4686a 100755 >--- a/members/members-update-do.pl >+++ b/members/members-update-do.pl >@@ -50,14 +50,14 @@ foreach my $param (@params) { > my $action = $query->param($param); > > if ( $action eq 'approve' ) { >- Koha::Patron::Modifications->ApproveModifications( $borrowernumber ); >+ my $m = Koha::Patron::Modifications->find( { borrowernumber => $borrowernumber } ); >+ $m->approve() if $m; > } > elsif ( $action eq 'deny' ) { >- Koha::Patron::Modifications->DenyModifications( $borrowernumber ); >- } >- elsif ( $action eq 'ignore' ) { >- >+ my $m = Koha::Patron::Modifications->find( { borrowernumber => $borrowernumber } ); >+ $m->delete() if $m; > } >+ # elsif ( $action eq 'ignore' ) { } > } > } > >diff --git a/members/members-update.pl b/members/members-update.pl >index 1441346..c97bcc5 100755 >--- a/members/members-update.pl >+++ b/members/members-update.pl >@@ -49,7 +49,7 @@ my $branch = > : undef; > > my $pending_modifications = >- Koha::Patron::Modifications->GetPendingModifications($branch); >+ Koha::Patron::Modifications->pending($branch); > > my $borrowers; > foreach my $pm (@$pending_modifications) { >diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl >index de157ef..541023c 100755 >--- a/opac/opac-memberentry.pl >+++ b/opac/opac-memberentry.pl >@@ -27,6 +27,7 @@ use C4::Output; > use C4::Members; > use C4::Form::MessagingPreferences; > use Koha::Patrons; >+use Koha::Patron::Modification; > use Koha::Patron::Modifications; > use C4::Branch qw(GetBranchesLoop); > use C4::Scrubber; >@@ -127,11 +128,11 @@ if ( $action eq 'create' ) { > $template->param( 'email' => $borrower{'email'} ); > > my $verification_token = md5_hex( \%borrower ); >- $borrower{'password'} = random_string(".........."); > >- Koha::Patron::Modifications->new( >- verification_token => $verification_token ) >- ->AddModifications(\%borrower); >+ $borrower{password} = random_string(".........."); >+ $borrower{verification_token} = $verification_token; >+ >+ Koha::Patron::Modification->new( \%borrower )->store(); > > #Send verification email > my $letter = C4::Letters::GetPreparedLetter( >@@ -214,12 +215,10 @@ elsif ( $action eq 'update' ) { > } > ); > >- my $m = >- Koha::Patron::Modifications->new( >- borrowernumber => $borrowernumber ); >+ $borrower_changes{borrowernumber} = $borrowernumber; >+ >+ my $m = Koha::Patron::Modification->new( \%borrower_changes )->store(); > >- $m->DelModifications; >- $m->AddModifications(\%borrower_changes); > $template->param( > borrower => GetMember( borrowernumber => $borrowernumber ), > ); >diff --git a/opac/opac-registration-verify.pl b/opac/opac-registration-verify.pl >index 5ceff26..1098af0 100755 >--- a/opac/opac-registration-verify.pl >+++ b/opac/opac-registration-verify.pl >@@ -34,10 +34,10 @@ unless ( C4::Context->preference('PatronSelfRegistration') ) { > } > > my $token = $cgi->param('token'); >-my $m = Koha::Patron::Modifications->new( verification_token => $token ); >+my $m = Koha::Patron::Modifications->find( { verification_token => $token } ); > > my ( $template, $borrowernumber, $cookie ); >-if ( $m->Verify() ) { >+if ( $m ) { > ( $template, $borrowernumber, $cookie ) = get_template_and_user( > { > template_name => "opac-registration-confirmation.tt", >@@ -50,13 +50,13 @@ if ( $m->Verify() ) { > $template->param( > OpacPasswordChange => C4::Context->preference('OpacPasswordChange') ); > >- my $borrower = Koha::Patron::Modifications->GetModifications({ verification_token => $token }); >+ my $borrower = $m->unblessed(); > > my $password; > ( $borrowernumber, $password ) = AddMember_Opac(%$borrower); > > if ($borrowernumber) { >- Koha::Patron::Modifications->DelModifications({ verification_token => $token }); >+ $m->delete(); > C4::Form::MessagingPreferences::handle_form_action($cgi, { borrowernumber => $borrowernumber }, $template, 1, C4::Context->preference('PatronSelfRegistrationDefaultCategory') ) if C4::Context->preference('EnhancedMessagingPreferences'); > > $template->param( password_cleartext => $password ); >diff --git a/t/db_dependent/Koha_borrower_modifications.t b/t/db_dependent/Koha_borrower_modifications.t >index f98dfeb..2a860cf 100755 >--- a/t/db_dependent/Koha_borrower_modifications.t >+++ b/t/db_dependent/Koha_borrower_modifications.t >@@ -1,133 +1,92 @@ > #!/usr/bin/perl > > use Modern::Perl; >-use Test::More tests => 14; >+use Test::More tests => 8; > > use C4::Context; > use t::lib::TestBuilder; > use C4::Members; > >-use Koha::Patron::Modifications; >+BEGIN { >+ use_ok('Koha::Patron::Modification'); >+ use_ok('Koha::Patron::Modifications'); >+} > >-my $dbh = C4::Context->dbh; >-$dbh->{RaiseError} = 1; >-$dbh->{AutoCommit} = 0; >+my $schema = Koha::Database->new->schema; >+$schema->storage->txn_begin; > >+my $dbh = C4::Context->dbh; > $dbh->do("DELETE FROM borrower_modifications"); > > ## Create new pending modification >-Koha::Patron::Modifications->new( verification_token => '1234567890' ) >- ->AddModifications( { surname => 'Hall', firstname => 'Kyle' } ); >+Koha::Patron::Modification->new( >+ { >+ verification_token => '1234567890', >+ surname => 'Hall', >+ firstname => 'Kyle' >+ } >+)->store(); > > ## Get the new pending modification >-my $borrower = Koha::Patron::Modifications->GetModifications( >- { verification_token => '1234567890' } ); >+my $borrower = >+ Koha::Patron::Modifications->find( { verification_token => '1234567890' } ); > > ## Verify we get the same data >-ok( $borrower->{'surname'} = 'Hall', >- 'Test AddModifications() and GetModifications()' ); >- >-## Check the Verify method >-ok( >- Koha::Patron::Modifications->Verify('1234567890'), >- 'Test that Verify() succeeds with a valid token' >-); >- >-## Delete the pending modification >-$borrower = Koha::Patron::Modifications->DelModifications( >- { verification_token => '1234567890' } ); >- >-## Verify it's no longer in the database >-$borrower = Koha::Patron::Modifications->GetModifications( >- { verification_token => '1234567890' } ); >-ok( !defined( $borrower->{'surname'} ), 'Test DelModifications()' ); >- >-## Check the Verify method >-ok( >- !Koha::Patron::Modifications->Verify('1234567890'), >- 'Test that Verify() method fails for a bad token' >-); >+is( $borrower->surname, 'Hall', 'Found modification has matching surname' ); > > ## Create new pending modification for a patron > my $builder = t::lib::TestBuilder->new; >-my $borr1 = $builder->build({ source => 'Borrower' })->{borrowernumber}; >-Koha::Patron::Modifications->new( borrowernumber => $borr1 ) >- ->AddModifications( { surname => 'Hall', firstname => 'Kyle' } ); >+my $borr1 = $builder->build( { source => 'Borrower' } )->{borrowernumber}; >+ >+my $m1 = Koha::Patron::Modification->new( >+ { >+ borrowernumber => $borr1, >+ surname => 'Hall', >+ firstname => 'Kyle' >+ } >+)->store(); > > ## Test the counter >-ok( Koha::Patron::Modifications->GetPendingModificationsCount() == 1, >- 'Test GetPendingModificationsCount()' ); >+is( Koha::Patron::Modifications->pending_count, >+ 1, 'Test pending_count()' ); > > ## Create new pending modification for another patron >-my $borr2 = $builder->build({ source => 'Borrower' })->{borrowernumber}; >-Koha::Patron::Modifications->new( borrowernumber => $borr2 ) >- ->AddModifications( { surname => 'Smith', firstname => 'Sandy' } ); >+my $borr2 = $builder->build( { source => 'Borrower' } )->{borrowernumber}; >+my $m2 = Koha::Patron::Modification->new( >+ { >+ borrowernumber => $borr2, >+ surname => 'Smith', >+ firstname => 'Sandy' >+ } >+)->store(); > > ## Test the counter >-ok( >- Koha::Patron::Modifications->GetPendingModificationsCount() == 2, >+is( >+ Koha::Patron::Modifications->pending_count(), 2, > 'Add a new pending modification and test GetPendingModificationsCount() again' > ); > > ## Check GetPendingModifications >-my $pendings = Koha::Patron::Modifications->GetPendingModifications(); >-my @firstnames_mod = sort ( $pendings->[0]->{firstname}, $pendings->[1]->{firstname} ); >-ok( $firstnames_mod[0] eq 'Kyle', 'Test GetPendingModifications()' ); >-ok( $firstnames_mod[1] eq 'Sandy', 'Test GetPendingModifications() again' ); >+my $pendings = Koha::Patron::Modifications->pending; >+my @firstnames_mod = >+ sort ( $pendings->[0]->{firstname}, $pendings->[1]->{firstname} ); >+ok( $firstnames_mod[0] eq 'Kyle', 'Test pending()' ); >+ok( $firstnames_mod[1] eq 'Sandy', 'Test pending() again' ); > > ## This should delete the row from the table >-Koha::Patron::Modifications->DenyModifications( $borr2 ); >- >-## Test the counter >-ok( Koha::Patron::Modifications->GetPendingModificationsCount() == 1, >- 'Test DenyModifications()' ); >+$m2->delete(); > > ## Save a copy of the borrowers original data > my $old_borrower = GetMember( borrowernumber => $borr1 ); > > ## Apply the modifications >-Koha::Patron::Modifications->ApproveModifications( $borr1 ); >- >-## Test the counter >-ok( >- Koha::Patron::Modifications->GetPendingModificationsCount() == 0, >- 'Test ApproveModifications() removes pending modification from db' >-); >+$m1->approve(); > > ## Get a copy of the borrowers current data > my $new_borrower = GetMember( borrowernumber => $borr1 ); > > ## Check to see that the approved modifications were saved > ok( $new_borrower->{'surname'} eq 'Hall', >- 'Test ApproveModifications() applys modification to borrower' ); >- >-## Now let's put it back the way it was >-Koha::Patron::Modifications->new( borrowernumber => $borr1 )->AddModifications( >- { >- surname => $old_borrower->{'surname'}, >- firstname => $old_borrower->{'firstname'} >- } >-); >- >-## Test the counter >-ok( Koha::Patron::Modifications->GetPendingModificationsCount() == 1, >- 'Test GetPendingModificationsCount()' ); >- >-## Apply the modifications >-Koha::Patron::Modifications->ApproveModifications( $borr1 ); >- >-## Test the counter >-ok( >- Koha::Patron::Modifications->GetPendingModificationsCount() == 0, >- 'Test ApproveModifications() removes pending modification from db, again' >-); >- >-$new_borrower = GetMember( borrowernumber => $borr1 ); >- >-## Test to verify the borrower has been updated with the original values >-ok( >- $new_borrower->{'surname'} eq $old_borrower->{'surname'}, >- 'Test ApproveModifications() applys modification to borrower, again' >-); >+ 'Test approve() applys modification to borrower' ); > > $dbh->rollback(); >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16960
:
53657
|
53920
|
54027
|
54079
|
54080
|
54157
|
54461
|
54628
|
54665
|
54666
|
54667
|
54668