See https://github.com/joubu/Koha/commits/bug_17829
Remote branch has been rebased against master.
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
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.
Not sure how signoff works on a github branch.. but I've tested thoroughly and am happy to signoff.
Remote branch has been rebased.
(In reply to Jonathan Druart from comment #7) > Remote branch has been rebased. It doesn't apply due to updatedatabase.pl conflicts...
(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
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>
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>
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>
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>
Created attachment 64973 [details] [review] Bug 17829: (followup) Fix small rebasing errors Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Excellent, Jonathan!
Created attachment 64976 [details] [review] Bug 17829: Update CanUserManageBasket to fix tests
Pushed to master for 17.11, thanks to everybody involved!
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
Last patch has been pushed to master.
Enhancement not pushed to 17.05.x
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.
Last patch has been pushed to master!
Created attachment 65286 [details] [review] Bug 17829: Execute rollingloans tests in a transaction
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
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?
(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
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 on attachment 65549 [details] [review] Bug 19048: (bug 17829 follow-up) Fix regression in self checkout Sorry, wrong Bug number...
Created attachment 65778 [details] [review] Bug 17829: (follow-up) Move GetMember to Koha::Patron - routing-lists.pl
(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!
Created attachment 66360 [details] [review] Bug 17829: (follow-up) Fix import patron
(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!
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
(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!