Bug 18647 - Internal server error on moremember.pl
Summary: Internal server error on moremember.pl
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18552 12461
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-21 15:03 UTC by Marc Véron
Modified: 2017-06-05 12:14 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18647: Internal server error on moremember.pl (1.58 KB, patch)
2017-05-21 15:07 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18647: Internal server error on moremember.pl (1.65 KB, patch)
2017-05-22 07:38 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18647: Resolve internal server error on category_type (1.50 KB, patch)
2017-05-23 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18647: Resolve internal server error on category_type (1.28 KB, patch)
2017-05-23 12:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18647: Resolve internal server error on category_type (1.32 KB, patch)
2017-05-23 16:07 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 Marc Véron 2017-05-21 15:03:13 UTC
To reproduce:
Go to the Details page of a member.
Result: Internal server error
Plack error log: 
Template process failed: undef error - The method category_code is not covered by tests! at (...)/koha/C4/Templates.pm line 121.
Comment 1 Marc Véron 2017-05-21 15:07:12 UTC
Created attachment 63590 [details] [review]
Bug 18647: Internal server error on moremember.pl

To reproduce:
Go to the Details page of a member.
Result: Internal server error
Plack error log:
Template process failed: undef error - The method category_code
is not covered by tests! at (...)/koha/C4/Templates.pm line 121.

To test:
Apply patch
Try to reproduce - patron's detail page should now display without error
Comment 2 Josef Moravec 2017-05-22 07:38:14 UTC
Created attachment 63596 [details] [review]
[SIGNED-OFF] Bug 18647: Internal server error on moremember.pl

To reproduce:
Go to the Details page of a member.
Result: Internal server error
Plack error log:
Template process failed: undef error - The method category_code
is not covered by tests! at (...)/koha/C4/Templates.pm line 121.

To test:
Apply patch
Try to reproduce - patron's detail page should now display without error

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Jonathan Druart 2017-05-22 12:48:31 UTC
No, it is definitely not the way to fix it, the borrower variable comes from GetMember most of the time (from other script of members/)
Comment 4 Jonathan Druart 2017-05-22 12:52:14 UTC
This is caused by bug 12461. The borrower variable should NOT be a Koha::Patron object.
Everything is cleared on bug 17829, but we will need to wait for the next release to see it in.
Comment 5 Jonathan Druart 2017-05-22 13:15:46 UTC
I'd say the patch should be:

diff --git a/members/moremember.pl b/members/moremember.pl
index 0e18d2234f..0649dcfd76 100755
--- a/members/moremember.pl
+++ b/members/moremember.pl
@@ -343,7 +343,6 @@ my $translated_language = C4::Languages::language_get_description( $subtag, $sub

 $template->param(
     patron          => $patron,
-    borrower        => $patron, # TODO Still needed by includes,
     translated_language => $translated_language,
     detailview      => 1,
     borrowernumber  => $borrowernumber,


The borrower variable should not be a Koha::Patron, because the include file expects an hashref.

Marcel, do you know why you add the TODO comment? I do not find where it is used.
Comment 6 Marc Véron 2017-05-22 15:14:13 UTC
(In reply to Jonathan Druart from comment #3)
> No, it is definitely not the way to fix it, the borrower variable comes from
> GetMember most of the time (from other script of members/)

Sorry then to have tried to fix it. Leaving it to others to fix the issue.
Comment 7 Tomás Cohen Arazi 2017-05-22 17:04:02 UTC
(In reply to Marc Véron from comment #6)
> (In reply to Jonathan Druart from comment #3)
> > No, it is definitely not the way to fix it, the borrower variable comes from
> > GetMember most of the time (from other script of members/)
> 
> Sorry then to have tried to fix it. Leaving it to others to fix the issue.

Your patch clearly fixes the issue by making patron-title.inc aware of the fact that borrower is now a K::Patron object. The problem is that the rest of the code is written in terms of the hashref returned by GetMember and we could be missing some edge cases. So we better move it back to the previous behaviour, and let bug 17829 take care of the C4 > Koha shift.

We still need Marcel's comment on why he put the TODO, because we might as well miss some other edge case Marcel noticed.
Comment 8 Marcel de Rooy 2017-05-23 12:16:12 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2017-05-23 12:41:43 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2017-05-23 12:45:22 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2017-05-23 12:54:39 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Jonathan Druart from comment #5)
> > I'd say the patch should be:
> > 
> > diff --git a/members/moremember.pl b/members/moremember.pl

> No, you cannot remove the borrower template var yet.

Yes, we can remove it. Probably the same Plack problem..
Comment 12 Marcel de Rooy 2017-05-23 12:56:14 UTC
Created attachment 63651 [details] [review]
Bug 18647: Resolve internal server error on category_type

See bug 18552. When we resolved the housebound_role bug, the hash got
filled correctly again. And this revealed that the (second) call to
Koha::Patrons->find was not appropriate. It can be removed, as Jonathan
explained on the report.

Note: Commit 95429af6855e8ed37945b681dc466bf360deffb added this call, but
it was hidden until the template variable hash got fixed.

Test plan:
Restart Plack and go to patron details again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-05-23 12:58:04 UTC
From IRC:

Joubu	marcelr: 290 $template->param(%$data); 
[14:51]	Joubu	patron-title.inc need patron's info, there are either picked from borrower.$field, or $field
[14:51]	Joubu	in moremember we have $field, because of l.290
[14:51]	Joubu	we just need to reset the state to the one before the patron's club ft
[14:51]	Joubu	and so delete the line
Comment 14 Jonathan Druart 2017-05-23 16:07:21 UTC
Created attachment 63663 [details] [review]
Bug 18647: Resolve internal server error on category_type

See bug 18552. When we resolved the housebound_role bug, the hash got
filled correctly again. And this revealed that the (second) call to
Koha::Patrons->find was not appropriate. It can be removed, as Jonathan
explained on the report.

Note: Commit 95429af6855e8ed37945b681dc466bf360deffb added this call, but
it was hidden until the template variable hash got fixed.

Test plan:
Restart Plack and go to patron details again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Marcel de Rooy 2017-05-24 05:43:58 UTC
(In reply to Jonathan Druart from comment #14)
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Thanks. Changing status.
Comment 16 Jonathan Druart 2017-05-24 13:05:43 UTC
I was expecting someone else to QA it.
Comment 17 Marcel de Rooy 2017-05-24 13:16:43 UTC
(In reply to Jonathan Druart from comment #16)
> I was expecting someone else to QA it.

Well, let Kyle have a last look now..
Comment 18 Kyle M Hall 2017-05-24 14:16:54 UTC
Pushed to master for 17.05, thanks Marcel, Jonathan!
Comment 19 Katrin Fischer 2017-06-05 12:14:22 UTC
This patch has been pushed to 16.11.x and will be in 16.11.09.