When a borrower has a restriction applied, the Restrictions tab on the Details screen doesn't show the restriction is there even though there's a message at the top of the screen saying the account is restricted. This does appear correctly on the circulation page.
Confirmed for current master.
Hours of debugging later, one line of code messes up template params.
Created attachment 62216 [details] [review] Bug 18179: Patron Restrictions not showing on member detail page When a borrower has a restriction applied, the Restrictions tab on the Details screen (moremember.pl) doesn't show the restriction is there even though there's a message at the top of the screen saying the account is restricted. The restrictions appear correctly on the circulation page (circulation.pl). To test: - Apply patch - Verify that restrictions tab on moremember.pl behaves the same as on circulation.pl
BTW: Syspref DumpTemplateVarsIntranet finally helped to find out that template variables were messed up. housebound_role got the variable name of the next template variable as value and all following template variables were wrong.
Created attachment 62218 [details] [review] [SIGNED-OFF] Bug 18179: Patron Restrictions not showing on member detail page When a borrower has a restriction applied, the Restrictions tab on the Details screen (moremember.pl) doesn't show the restriction is there even though there's a message at the top of the screen saying the account is restricted. The restrictions appear correctly on the circulation page (circulation.pl). To test: - Apply patch - Verify that restrictions tab on moremember.pl behaves the same as on circulation.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Yes we have a big problem here, it may be needed to fix it globally. For instance: in members/memberentry.pl 755 $template->param( 756 housebound_role => Koha::Patron::HouseboundRoles->find($borrowernumber), 757 ); If someone add a param: 755 $template->param( 756 housebound_role => Koha::Patron::HouseboundRoles->find($borrowernumber), 757 foo => 1, 758 ); housebound_role will be set to 'foo' if the patron does not have a housebound role, and foo will not be defined in the template.
Stealing this one for a more complete fix.
Created attachment 62304 [details] [review] Bug 18179: Forbid list context calls for Koha::Objects->find Reading https://perlmaven.com/how-to-return-undef-from-a-function this sound like the more correct behaviour. Considering: $template->param( stuff => Koha::Stuffs->find( $id ), foo => 1, ); without this patch, if the $id does not represent any rows in the DB, stuff will be assigned to 'foo' and $foo will be undef in the template. That can lead to very bad side-effects. With this patch we make sure that it will never happen again. Test plan: prove t/db_dependent/Koha/Objects.t should return green
Created attachment 62305 [details] [review] Bug 18179: Update existing calls This patch updates the existing occurrences of ->find called in a list context. There are certainly others that are not easy to catch with git grep. Test plan: Confirm that the 4 modified scripts still works as expected. We need this one ASAP in master to make sure we will not get other side-effects of this kind and to catch possible uncaught occurrences before the release.
@RMaints, I would not backport the first patch, but the second one is safe ("Update existing calls").
Created attachment 62319 [details] [review] Bug 18179: Forbid list context calls for Koha::Objects->find Reading https://perlmaven.com/how-to-return-undef-from-a-function this sound like the more correct behaviour. Considering: $template->param( stuff => Koha::Stuffs->find( $id ), foo => 1, ); without this patch, if the $id does not represent any rows in the DB, stuff will be assigned to 'foo' and $foo will be undef in the template. That can lead to very bad side-effects. With this patch we make sure that it will never happen again. Test plan: prove t/db_dependent/Koha/Objects.t should return green Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62321 [details] [review] Bug 18179: Update existing calls This patch updates the existing occurrences of ->find called in a list context. There are certainly others that are not easy to catch with git grep. Test plan: Confirm that the 4 modified scripts still works as expected. We need this one ASAP in master to make sure we will not get other side-effects of this kind and to catch possible uncaught occurrences before the release. Tested scripts changed by this patch, they work as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62656 [details] [review] Bug 18179: Forbid list context calls for Koha::Objects->find Reading https://perlmaven.com/how-to-return-undef-from-a-function this sound like the more correct behaviour. Considering: $template->param( stuff => Koha::Stuffs->find( $id ), foo => 1, ); without this patch, if the $id does not represent any rows in the DB, stuff will be assigned to 'foo' and $foo will be undef in the template. That can lead to very bad side-effects. With this patch we make sure that it will never happen again. Test plan: prove t/db_dependent/Koha/Objects.t should return green Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62657 [details] [review] Bug 18179: Update existing calls This patch updates the existing occurrences of ->find called in a list context. There are certainly others that are not easy to catch with git grep. Test plan: Confirm that the 4 modified scripts still works as expected. We need this one ASAP in master to make sure we will not get other side-effects of this kind and to catch possible uncaught occurrences before the release. Tested scripts changed by this patch, they work as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
*** Bug 18492 has been marked as a duplicate of this bug. ***
Pushed to master for 17.05, thanks Jonathan!
This patch is causing errors with many new find calls that have not been updated by the patch set. I think it would be best if we waited until post-release to push it back into master. That way we can find all the errors without subjecting librarians to them ; )
This patch should not have been reverted: Bug 18179: Update existing calls It fixes the original issues. Where do you get errors?
Created attachment 62879 [details] [review] Bug 18179: Update 1 occurrence in booksellers.pl To test: 1 - Load http://localhost:8081/cgi-bin/koha/acqui/booksellers.pl?booksellerid=1 2 - Should get internal server erro 3 - Apply patch 4 - Reload 5 - Should not get error
Created attachment 62880 [details] [review] Bug 18179: Update 1 occurrence in moremember.pl Cannot use "->find" in list context at /home/vagrant/kohaclone/members/moremember.pl line 334. Test Plan: 1) Got to moremember.pl, note error 2) Apply this patch 3) Refresh page, no error!
*** Bug 18511 has been marked as a duplicate of this bug. ***
*** Bug 18510 has been marked as a duplicate of this bug. ***
This requires another QA.
Kyle, I think the last 3 patches should be pushed before the release. We could move the croak to its own bug report if you do not feel confident with it.
Note: 18361 also adds a subtest find.
Same include: [%- IF borrower.category.category_type == 'I' %] [%- IF category_type == 'I' %] I am having the feeling that it should be borrower.category_type when I am looking at GetMember. Note that this change in the last patch could be considered out of scope here.. Also wondering about this hardcoded category too. I do not see in the sample patron categories. Please explain.
Jonathan: If you move the first patch somewhere else and remove the category change, please update to Passed QA.
(In reply to Marcel de Rooy from comment #26) > Same include: > > [%- IF borrower.category.category_type == 'I' %] > > [%- IF category_type == 'I' %] > > I am having the feeling that it should be borrower.category_type when I am > looking at GetMember. Yes, right. I found 3 occurrences where borrower is passed to the template: members/pay.pl: borrower => $borrower, members/paycollect.pl: borrower => $borrower, members/routing-lists.pl: borrower => $borrower, $borrower comes from GetMember. And from 1 members/moremember.pl (Koha::Patron) So the change does not make sense. But it's a regression of bug 12461, should be fixed elsewhere. Please have a look at bug 17829 to get rid of all these issues!!
Created attachment 63106 [details] [review] Bug 18179: Update existing calls This patch updates the existing occurrences of ->find called in a list context. There are certainly others that are not easy to catch with git grep. Test plan: Confirm that the 4 modified scripts still works as expected. We need this one ASAP in master to make sure we will not get other side-effects of this kind and to catch possible uncaught occurrences before the release. Tested scripts changed by this patch, they work as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> bsoletes: 62657 - Bug 18179: Update existing calls
Created attachment 63107 [details] [review] Bug 18179: Update 1 occurrence in booksellers.pl To test: 1 - Load http://localhost:8081/cgi-bin/koha/acqui/booksellers.pl?booksellerid=1 2 - Should get internal server erro 3 - Apply patch 4 - Reload 5 - Should not get error Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 63108 [details] [review] Bug 18179: Update 1 occurrence in moremember.pl Cannot use "->find" in list context at /home/vagrant/kohaclone/members/moremember.pl line 334. Test Plan: 1) Got to moremember.pl, note error 2) Apply this patch 3) Refresh page, no error! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 63109 [details] [review] Bug 18179: Use borrower.category_type in patron-title.inc I found 3 occurrences where borrower is passed to the template: members/pay.pl: borrower => $borrower, members/paycollect.pl: borrower => $borrower, members/routing-lists.pl: borrower => $borrower, $borrower comes from GetMember. And from 1 members/moremember.pl (Koha::Patron) So the change does not make sense. But it's a regression of bug 12461, should be fixed elsewhere. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment on attachment 62656 [details] [review] Bug 18179: Forbid list context calls for Koha::Objects->find See bug 18539 for this patch
(In reply to Jonathan Druart from comment #10) > @RMaints, I would not backport the first patch, but the second one is safe > ("Update existing calls"). Tried to backport, but there is a conflict in pay.pl. Can you take a look please?
(In reply to Jonathan Druart from comment #31) > Created attachment 63108 [details] [review] [review] > Bug 18179: Update 1 occurrence in moremember.pl This patch is not longer needed, see bug 18647.
(In reply to Jonathan Druart from comment #32) > Created attachment 63109 [details] [review] [review] > Bug 18179: Use borrower.category_type in patron-title.inc Same as previous comment
Created attachment 63832 [details] [review] [16.11.x] Bug 18179: Update existing calls This patch updates the existing occurrences of ->find called in a list context. There are certainly others that are not easy to catch with git grep. Test plan: Confirm that the 4 modified scripts still works as expected. We need this one ASAP in master to make sure we will not get other side-effects of this kind and to catch possible uncaught occurrences before the release. Tested scripts changed by this patch, they work as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Only the first patch is needed for 16.11.x
Sorry, first patch listed here and first patch pushed do both not apply? It also looks like a part of the patches has been reverted from master and the status here is "Passed QA" - confusing!
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in v17.05.01
This patch has been pushed to 16.11.x and will be in 16.11.09. Picked the 16.11.x patch from here.
Template process failed: undef error - Cannot use "->find" in list context at /usr/share/koha/masterclone/Koha/Club.pm line 51. You will find a few with git grep "return.*->find"