Bug 17829 - Move GetMember to Koha::Patron
Summary: Move GetMember to Koha::Patron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 17554 17680 17737 17738
Blocks: 16846 18403 18938 18942 19048 19080 19276 19808 20003 20303 20348 20763 20998 21019
  Show dependency treegraph
 
Reported: 2016-12-29 16:20 UTC by Jonathan Druart
Modified: 2019-10-14 19:56 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17829: Move GetMember to Koha::Patron (250.53 KB, patch)
2017-07-06 19:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: followup for request.pl (8.77 KB, patch)
2017-07-06 19:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: Move GetMember to Koha::Patron (249.83 KB, patch)
2017-07-10 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17829: followup for request.pl (8.81 KB, patch)
2017-07-10 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17829: (followup) Fix small rebasing errors (6.19 KB, patch)
2017-07-10 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17829: Update CanUserManageBasket to fix tests (1.81 KB, patch)
2017-07-10 17:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: (follow-up) Move GetMember to Koha::Patron (2.17 KB, patch)
2017-07-11 18:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: Fix import patron (1.95 KB, patch)
2017-07-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: Execute rollingloans tests in a transaction (1010 bytes, patch)
2017-07-26 19:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19048: (bug 17829 follow-up) Fix regression in self checkout (2.27 KB, patch)
2017-08-07 13:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17829: (follow-up) Move GetMember to Koha::Patron - routing-lists.pl (895 bytes, patch)
2017-08-09 19:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: (follow-up) Fix import patron (1.10 KB, patch)
2017-08-22 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17829: (follow-up) Move GetMember to Koha::Patron (1009 bytes, patch)
2017-10-26 18:41 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-12-29 16:20:31 UTC

    
Comment 1 Jonathan Druart 2016-12-29 16:24:25 UTC
See https://github.com/joubu/Koha/commits/bug_17829
Comment 2 Jonathan Druart 2017-02-24 09:52:34 UTC
Remote branch has been rebased against master.
Comment 3 Jonathan Druart 2017-03-22 13:55:07 UTC
Remote branch has been rebased against master.

Asking for a signoff now.

Test plan:
- send notices
- place holds
- play with acquisition: create basket, orders, receive them, transfer them
- play with circulation: checkout/in
- play with patrons: add a discharge, delete a patron, edit password and permissions, create invoices and pay them
Comment 4 Jonathan Druart 2017-03-31 19:20:33 UTC
Remote branch has been rebased against master.
Comment 5 Jonathan Druart 2017-04-11 20:13:14 UTC
I have worked for several days on bug 18403, on top of these patches, and did not find any errors.
I am quite confident with these patches, even if the changes are important. Please do not let them wait too long.
Comment 6 Martin Renvoize 2017-04-28 15:40:37 UTC
Not sure how signoff works on a github branch.. but I've tested thoroughly and am happy to signoff.
Comment 7 Jonathan Druart 2017-06-02 15:51:26 UTC
Remote branch has been rebased.
Comment 8 Tomás Cohen Arazi 2017-06-23 18:45:42 UTC
(In reply to Jonathan Druart from comment #7)
> Remote branch has been rebased.

It doesn't apply due to updatedatabase.pl conflicts...
Comment 9 Tomás Cohen Arazi 2017-07-05 13:55:32 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Remote branch has been rebased.
> 
> It doesn't apply due to updatedatabase.pl conflicts...

I'm sorry, the updatedatabase.pl merge problem was not the problem (easy fix). The problem was in Circulation.pm:

diff --cc C4/Circulation.pm
index 7e65a80,789a85f..0000000
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@@ -1903,16 -1904,15 +1904,23 @@@ sub AddReturn 
          }
  
          if ($borrowernumber) {
++<<<<<<< HEAD
++=======
+             if ( ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) || $return_date ) {
+                 _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } );
+             }
+ 
++>>>>>>> dadae20... Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
              eval {
 -                MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
 +                my $issue_id = MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
                      $circControlBranch, $return_date, $borrower->{'privacy'} );
 +                $issue->{issue_id} = $issue_id;
              };
 -            if ( $@ ) {
 +            unless ( $@ ) {
 +                if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) {
 +                    _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } );
 +                }
 +            } else {
                  $messages->{'Wrongbranch'} = {
                      Wrongbranch => $branch,
                      Rightbranch => $message
Comment 10 Jonathan Druart 2017-07-05 16:34:07 UTC
Remote branch has been rebased.
Comment 11 Jonathan Druart 2017-07-06 19:16:16 UTC
Created attachment 64872 [details] [review]
Bug 17829: Move GetMember to Koha::Patron

GetMember returned a patron given a borrowernumber, cardnumber or
userid.
All of these 3 attributes are defined as a unique key at the DB level
and so we can use Koha::Patrons->find to replace this subroutine.
Additionaly GetMember set category_type and description.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Jonathan Druart 2017-07-06 19:16:21 UTC
Created attachment 64873 [details] [review]
Bug 17829: followup for request.pl

This script is really ugly and need to be rewritten completely to
separate the different action.
$patron is not always defined, we need to take it into account.
Note that this patch is mainly indentation changes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Tomás Cohen Arazi 2017-07-10 15:23:42 UTC
Created attachment 64971 [details] [review]
Bug 17829: Move GetMember to Koha::Patron

GetMember returned a patron given a borrowernumber, cardnumber or
userid.
All of these 3 attributes are defined as a unique key at the DB level
and so we can use Koha::Patrons->find to replace this subroutine.
Additionaly GetMember set category_type and description.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2017-07-10 15:23:52 UTC
Created attachment 64972 [details] [review]
Bug 17829: followup for request.pl

This script is really ugly and need to be rewritten completely to
separate the different action.
$patron is not always defined, we need to take it into account.
Note that this patch is mainly indentation changes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2017-07-10 15:23:58 UTC
Created attachment 64973 [details] [review]
Bug 17829: (followup) Fix small rebasing errors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2017-07-10 15:24:33 UTC
Excellent, Jonathan!
Comment 17 Jonathan Druart 2017-07-10 17:18:36 UTC
Created attachment 64976 [details] [review]
Bug 17829: Update CanUserManageBasket to fix tests
Comment 18 Jonathan Druart 2017-07-10 17:53:06 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 19 Jonathan Druart 2017-07-11 18:43:22 UTC
Created attachment 65002 [details] [review]
Bug 17829: (follow-up) Move GetMember to Koha::Patron

Prevent crash at the opac when no user is logged in
Comment 20 Jonathan Druart 2017-07-11 18:44:07 UTC
Last patch has been pushed to master.
Comment 21 Fridolin Somers 2017-07-18 11:54:11 UTC
Enhancement not pushed to 17.05.x
Comment 22 Jonathan Druart 2017-07-25 13:32:44 UTC
Created attachment 65213 [details] [review]
Bug 17829: Fix import patron

Can't call method "unblessed" on an undefined value at
/home/vagrant/kohaclone/tools/import_borrowers.pl line 230, <$fh> line 2.
Comment 23 Jonathan Druart 2017-07-25 13:33:39 UTC
Last patch has been pushed to master!
Comment 24 Jonathan Druart 2017-07-26 19:40:37 UTC
Created attachment 65286 [details] [review]
Bug 17829: Execute rollingloans tests in a transaction
Comment 25 Jonathan Druart 2017-07-26 19:40:57 UTC
Last patch has been pushed to master!
Comment 26 Colin Campbell 2017-07-27 15:49:40 UTC
Found that the initial patch breaks searching by name in staff client. 
Go to patron page enter name in search box (Enter patron card number or partial name) and search - nothing is ever returned. Resetting the system to the status before the initial patch restores ability to search by name.

First impression looking at the code, GetMember was handling doing a name search, almost as a side effect of the more specific 'number' searches and we've lost that functionality. This may have implications elsewhere
Comment 27 Jonathan Druart 2017-07-27 16:11:23 UTC
Hi Colin, I do not recreate the problem you describe. Could you give me more details please? Are you referring to the call from members/member.pl or another one?
Comment 28 Colin Campbell 2017-07-28 14:55:30 UTC
(In reply to Jonathan Druart from comment #27)
> Hi Colin, I do not recreate the problem you describe. Could you give me more
> details please? Are you referring to the call from members/member.pl or
> another one?

I've traced it through its when svc/members/search is called ultimately it gets logged in the error log

 search: Undefined subroutine &C4::Members::Attributes::SearchIdMatchingAttribute called at /home/cc/kohaclone/C4/Utils/DataTables/Members.pm line 127.: /home/cc/kohaclone/svc/members/search, referer: http://admin/cgi-bin/koha/members/member.pl

at line 127 of C4::Utils/DataTables/Members.pm we have

           my $matching_borrowernumbers = C4::Members::Attributes::SearchIdMatchingAttribute($searchmember);

which needs replacing with an equivalent
Comment 29 Fridolin Somers 2017-07-31 08:28:51 UTC
Enhancement not pushed to 17.05.x
Comment 30 Marc Véron 2017-08-07 13:39:12 UTC
Created attachment 65549 [details] [review]
Bug 19048: (bug 17829 follow-up) Fix regression in self checkout

$patronid is not necessarily set or does not match a valid cardnumber.
These cases must be taken into account to avoid the script to crash and
raise the following error:
Can't call method "unblessed" on an undefined value at
(...)/koha/opac/sco/sco-main.pl line 117

Test plan:
Hit sco/sco-main.pl and confirm that the error is gone with this patch
applied

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 31 Marc Véron 2017-08-07 13:40:09 UTC
Comment on attachment 65549 [details] [review]
Bug 19048: (bug 17829 follow-up) Fix regression in self checkout

Sorry, wrong Bug number...
Comment 32 Jonathan Druart 2017-08-09 19:52:48 UTC
Created attachment 65778 [details] [review]
Bug 17829: (follow-up) Move GetMember to Koha::Patron - routing-lists.pl
Comment 33 Jonathan Druart 2017-08-09 19:54:18 UTC
(In reply to Jonathan Druart from comment #32)
> Created attachment 65778 [details] [review] [review]
> Bug 17829: (follow-up) Move GetMember to Koha::Patron - routing-lists.pl

Patch pushed to master!
Comment 34 Jonathan Druart 2017-08-22 16:42:08 UTC
Created attachment 66360 [details] [review]
Bug 17829: (follow-up) Fix import patron
Comment 35 Jonathan Druart 2017-08-22 16:43:06 UTC
(In reply to Jonathan Druart from comment #34)
> Created attachment 66360 [details] [review] [review]
> Bug 17829: (follow-up) Fix import patron

Patch pushed to master!
Comment 36 Jonathan Druart 2017-10-26 18:41:33 UTC
Created attachment 68634 [details] [review]
Bug 17829: (follow-up) Move GetMember to Koha::Patron

Do not call method on $patron if there is no other reserves
Comment 37 Jonathan Druart 2017-10-26 18:59:34 UTC
(In reply to Jonathan Druart from comment #36)
> Created attachment 68634 [details] [review] [review]
> Bug 17829: (follow-up) Move GetMember to Koha::Patron
> 
> Do not call method on $patron if there is no other reserves

Patch pushed to master!