Bug 16960 - Patron::Modifications should be fixed
Summary: Patron::Modifications should be fixed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 16985 (view as bug list)
Depends on: 14757
Blocks: 16990 17080
  Show dependency treegraph
 
Reported: 2016-07-22 11:43 UTC by Marcel de Rooy
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16960 - Patron::Modifications should be fixed (19.60 KB, patch)
2016-07-22 16:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16960 - Patron::Modifications should be fixed (20.11 KB, patch)
2016-08-02 15:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16960: Update 1 missing occurrence of GetModifications (3.51 KB, patch)
2016-08-05 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16960 - Patron::Modifications should be fixed (20.16 KB, patch)
2016-08-06 21:14 UTC, Bob Birchall
Details | Diff | Splinter Review
Bug 16960: Update 1 missing occurrence of GetModifications (3.55 KB, patch)
2016-08-06 21:14 UTC, Bob Birchall
Details | Diff | Splinter Review
Bug 16960: Fix error on validating the registration (979 bytes, patch)
2016-08-08 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16960: Fix error on validating the registration (1.14 KB, patch)
2016-08-15 12:04 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16960: Delete previous patron modifications (1.44 KB, patch)
2016-08-19 12:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 16960 - Patron::Modifications should be fixed (20.22 KB, patch)
2016-08-21 20:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16960: Update 1 missing occurrence of GetModifications (3.63 KB, patch)
2016-08-21 20:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16960: Fix error on validating the registration (1.21 KB, patch)
2016-08-21 20:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16960: Delete previous patron modifications (1.51 KB, patch)
2016-08-21 20:25 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-07-22 11:43:33 UTC
The changes from opac-memberentry do not reach the table, since the Patron::Modifications object does not work well.

Look at this from opac-memberentry:
    Koha::Patron::Modifications->new( borrowernumber => $borrowernumber );
Passing borrowernumber here is useless, since it is an instance of Koha::Objects.
On the contrary, the following call
    $m->DelModifications;
should have the borrowernumber passed !

So what could we add a sub new to Patron::Modifications like:
+sub new {
+    my ( $class, $params ) = @_;
+    my $self = $class->SUPER::new();
+    $self->{borrowernumber} = $params->{borrowernumber};
+    return $self;
+}
And in opac-memberentry:
+            my $patronmod = Koha::Patron::Modifications->new({ borrowernumber => $borrowernumber });
+            $patronmod->DelModifications({ borrowernumber => $borrowernumber });
+            $patronmod->AddModifications(\%borrower_changes);


But this is a horrible hack. We should not use a Koha::Objects like that..
And what about its unit test:  t/db_dependent/Koha_borrower_modifications.t
Currently fails !
Comment 1 Marcel de Rooy 2016-07-22 11:47:32 UTC
Not to mention the random stuff interfering here too :)
Comment 2 Jonathan Druart 2016-07-22 12:04:17 UTC
Upgrading the severity.
I have completely missed that on QAing bug 14757, I focused on the notices code and did not see this part!
Comment 3 Kyle M Hall 2016-07-22 16:52:38 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2016-07-24 04:21:35 UTC
Hi Kyle,

Sorry, after applying the patch, I get an "Internal server error" on Home > Patrons (members-home.pl)

plack-error.log says:
Can't locate object method "GetPendingModificationsCount" via package "Koha::Patron::Modifications" at /home/marc/koha/members/members-home.pl line 91.

git grep GetPendingModifications

members/members-home.pl:  Koha::Patron::Modifications->GetPendingModificationsCount( $branch );
^ Leads to error. If I fix this line, members-home.pl displays as expected


t/db_dependent/Koha_borrower_modifications.t:'Add a new pending modification and test GetPendingModificationsCount() again'
t/db_dependent/Koha_borrower_modifications.t:## Check GetPendingModifications
marc@debian8vm:~/koha$ 
^ Inside comments, but I think it would be fine to change them for readability

Otherwise, sending an update request from OPAC and applying it from Staff client works OK.
Comment 5 Jonathan Druart 2016-08-01 10:42:37 UTC
*** Bug 16985 has been marked as a duplicate of this bug. ***
Comment 6 Katrin Fischer 2016-08-01 22:18:20 UTC
Upping severity - appears to have bad side effects.
Comment 7 Kyle M Hall 2016-08-02 15:54:42 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2016-08-02 15:55:04 UTC
(In reply to Marc Véron from comment #4)
> Hi Kyle,
> 
> Sorry, after applying the patch, I get an "Internal server error" on Home >
> Patrons (members-home.pl)
> 
> plack-error.log says:
> Can't locate object method "GetPendingModificationsCount" via package
> "Koha::Patron::Modifications" at /home/marc/koha/members/members-home.pl
> line 91.
> 
> git grep GetPendingModifications
> 
> members/members-home.pl: 
> Koha::Patron::Modifications->GetPendingModificationsCount( $branch );
> ^ Leads to error. If I fix this line, members-home.pl displays as expected
> 
> 
> t/db_dependent/Koha_borrower_modifications.t:'Add a new pending modification
> and test GetPendingModificationsCount() again'
> t/db_dependent/Koha_borrower_modifications.t:## Check GetPendingModifications
> marc@debian8vm:~/koha$ 
> ^ Inside comments, but I think it would be fine to change them for
> readability
> 
> Otherwise, sending an update request from OPAC and applying it from Staff
> client works OK.

This should now be fixed!
Comment 9 Marcel de Rooy 2016-08-04 06:45:50 UTC
You forgot one call:

circ/circulation.pl
    modifications             => Koha::Patron::Modifications->GetModifications({ borrowernumber => $borrowernumber }),

And two typos :)
requrest
applys
Comment 10 Jonathan Druart 2016-08-05 10:53:11 UTC Comment hidden (obsolete)
Comment 11 Bob Birchall 2016-08-06 19:30:51 UTC
(In reply to Kyle M Hall from comment #7)
> Created attachment 53920 [details] [review] [review]
> 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

I wasn't completely sure what the test plan was asking for.  This is what I did:
- logged in to OPAC and changed some personal details;
- logged in to staff client and accepted the changes; on searching for the patron the changes are present;
- enabled self registration and registered in OPAC as a new patron; logged in as that patron OK; checked in staff - all details present;
- logged in to OPAC again, made more changes to Personal Details; returned to Staff client and Denied those changes - on searching none of the data is amended.  OK.

I'll sign off on this.  I'm having an issue with git so but will get some help.
Comment 12 Bob Birchall 2016-08-06 21:14:29 UTC Comment hidden (obsolete)
Comment 13 Bob Birchall 2016-08-06 21:14:55 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2016-08-07 11:12:23 UTC
I think this improves the situation, but there is still a problem with patron self registration:

1) Ensure email verification is mandatory
2) Register a new patron from the OPAC
3) Copy the URL (token now is back!) from the message_queue entry 
4) Open the page, receive error:
DBIx::Class::Storage::DBI::_dbh_execute(): Column 'address' cannot be null at /home/katrin/kohaclone/C4/Members.pm line 696

I had filled out surname, firstname and email - the address fields were left empty, but were also not set to be mandatory.
Comment 15 Jonathan Druart 2016-08-08 12:47:20 UTC Comment hidden (obsolete)
Comment 16 Marc Véron 2016-08-15 12:04:33 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-08-19 11:46:49 UTC
DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '7874' for key 'PRIMARY' at /home/koha/src/Koha/Object.pm line 109
 at /home/koha/src/opac/opac-memberentry.pl line 428

To recreate:
At the OPAC, change some of your patron's details.
And change the same ones or others => boom
Comment 18 Jonathan Druart 2016-08-19 12:44:14 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2016-08-21 20:25:40 UTC
Created attachment 54665 [details] [review]
[PASSED QA] 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

Signed-off-by: Bob Birchall <bob@calyx.net.au>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 20 Katrin Fischer 2016-08-21 20:25:44 UTC
Created attachment 54666 [details] [review]
[PASSED QA] Bug 16960: Update 1 missing occurrence of GetModifications

Signed-off-by: Bob Birchall <bob@calyx.net.au>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 21 Katrin Fischer 2016-08-21 20:25:48 UTC
Created attachment 54667 [details] [review]
[PASSED QA] Bug 16960: Fix error on validating the registration

Followed steps in comment #14, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 22 Katrin Fischer 2016-08-21 20:25:52 UTC
Created attachment 54668 [details] [review]
[PASSED QA] Bug 16960: Delete previous patron modifications

If a patron edit his/her details a second time, we need to delete the
first ones to avoid a "duplicate entry for key PRIMARY" error.

Test plan:
Log in at the OPAC
Edit your details
Edit them again
=> Without this patch, Koha will crash
=> With the patch applied, everything should work as expected

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 23 Kyle M Hall 2016-08-22 11:45:55 UTC
Pushed to master for 16.11, thanks for the followups Jonathan!