Bug 17307 - Some edit buttons/links for patrons do not work
Summary: Some edit buttons/links for patrons do not work
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Andreas Roussos
QA Contact: Testopia
URL: /cgi-bin/koha/members/member.pl
Keywords:
Depends on: 16911
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-16 13:06 UTC by Andreas Roussos
Modified: 2017-12-07 22:16 UTC (History)
7 users (show)

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


Attachments
Bug 17307 - Some edit buttons/links for patrons do not work without a categorycode in the URL (4.96 KB, patch)
2016-09-16 16:35 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 17307: Smaller counter-patch (1.15 KB, patch)
2016-09-19 16:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17307: Smaller counter-patch (1.32 KB, patch)
2016-09-21 06:30 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 17307: Smaller counter-patch (1.39 KB, patch)
2016-09-22 22:25 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[PASSED QA] Bug 17307: Smaller counter-patch (1.46 KB, patch)
2016-09-25 17:07 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 Andreas Roussos 2016-09-16 13:06:58 UTC
Description of the problem:
The 'Edit' button in the patron search page doesn't work without a
category code in the URL.

[I've searched extensively and couldn't find this bug listed, so I'm
submitting it as new. Please note that it's different from:
Bug 4508 - can't edit patron attributes using little edit link, and
Bug 4427 - Can't edit patron additional attributes and identifiers.]

Steps to reproduce (in the current master 0254fd7):
Go to Home > Patrons, search for a patron, click on the 'Edit' button
(last column). You cannot edit the patron, and you get the following
Software error in the browser:
Can't call method "get_expiry_date" on an undefined value at
/home/aroussos/kohaclone/members/memberentry.pl line 375.

Notice how the URL is of the form:
cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=52

If you follow a different route and click on the 'Edit' button above
the patron name in the patron Details page it works fine because the
URL is like this (notice the added '&categorycode=J' at the end):
cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=52&categorycode=J
Comment 1 Andreas Roussos 2016-09-16 13:09:17 UTC
I'm no Perl expert but here's what I've found so far in my attempt
to debug this.

members/memberentry.pl in the current master (0254fd7):

372 if ( ($op eq 'modify' || $op eq 'insert' || $op eq 'save'|| $op eq 'duplicate') and ($step == 0 or $step == 3 )){
373     unless ($newdata{'dateexpiry'}){
374         my $patron_category = Koha::Patron::Categories->find( $newdata{categorycode} );
375         $newdata{'dateexpiry'} = $patron_category->get_expiry_date( $newdata{dateenrolled} );
376     }
377 }

The problem in line 375 is that the value of the $patron_category
variable is undefined, because at line 374 $newdata{categorycode}
comes from $input->param(), as per the comment at line 168. Since
no 'categorycode' value is passed in the URL, the call to
Koha::Patron::Categories->find() fails.

So, it seems that the easy fix is to add the categorycode in the URL.
It's trivial to fix so I'll provide a patch soon.
Comment 2 Andreas Roussos 2016-09-16 14:53:15 UTC
Edited the bug Summary to reflect that this needs fixing in more than one place.
Comment 3 Andreas Roussos 2016-09-16 16:35:23 UTC
Created attachment 55623 [details] [review]
Bug 17307 - Some edit buttons/links for patrons do not work without a categorycode in the URL

In the Staff client, the 'Edit' button in the patron search page doesn't
work. This is also true for one of the 'Edit' links in the patron Detail
page, and the 'Edit' link in the Norwegian national patron database search
page.

This reason behind this is a missing categorycode parameter in the URL,
and this patch fixes that.

Test plan:
0) [PREREQUISITES FOR STEP 3] Enable the Norwegian national patron database
   search by setting the following sysprefs:
       NorwegianPatronDBEnable                -> Enable
       NorwegianPatronDBEndpoint              -> test
       NorwegianPatronDBSearchNLAfterLocalHit -> Don't
       NorwegianPatronDBUsername              -> username
       NorwegianPatronDBPassword              -> password
1) Go to Home > Patrons, search for a patron, click on the 'Edit' button
   (last column). You cannot edit the patron, and you get the following
   Software error in your browser:
   Can't call method "get_expiry_date" on an undefined value at [...]
2) Go to the patron's Detail page and click on the 'Edit' link under the
   'Library use' heading on the right. You'll get the same error.
3) Manually navigate to <STAFF_URL>/cgi-bin/koha/members/nl-search.pl,
   then search for the card number of one of your patrons. Click 'Edit'
   under the 'Existing patrons' heading. You'll get the same error.
4) Apply the patch.
5) Repeat steps 1), 2), and 3). This time the 'Edit' button/links work.
Comment 4 Mark Tompsett 2016-09-19 16:40:41 UTC
Created attachment 55698 [details] [review]
Bug 17307: Smaller counter-patch

Given that the issue is trying to call a method on something that isn't
an object, because the category code doesn't exist, then why is it
even trying to change the expiry date? A simple postfix if fixes it.
Comment 5 Mark Tompsett 2016-09-19 16:49:39 UTC
(In reply to Andreas Roussos from comment #1)
> 373     unless ($newdata{'dateexpiry'}){
> 374         my $patron_category = Koha::Patron::Categories->find(
> $newdata{categorycode} );
> 375         $newdata{'dateexpiry'} = $patron_category->get_expiry_date(
> $newdata{dateenrolled} );
> 376     }

Let's look at this block of code.


> The problem in line 375 is that the value of the $patron_category
> variable is undefined, because at line 374 $newdata{categorycode}
> comes from $input->param(), as per the comment at line 168. Since
> no 'categorycode' value is passed in the URL, the call to
> Koha::Patron::Categories->find() fails.
> 
> So, it seems that the easy fix is to add the categorycode in the URL.
> It's trivial to fix so I'll provide a patch soon.

While this is all true, that's not what the block of code is trying to do. Patron's should have an expiry date for their account. The problem is not the lack of category code, the problem is the lack of expiry date. This code is trying to correct the expiry date based on the category code. However, given that we don't know what the category code is, the program shouldn't even be trying to set the dateexpiry at all. So, in the case that there is no category found, which is also possible by passing a BAD one, we should not set the dateexpiry. In short, set the dateexpiry to the function call *IF* there is a patron_category. Hence, my counter patch. Feel free to sign off on it and obsolete yours, Andreas, if you think it is better.

I don't think fixing all the templates to pass category code is the correct way to handle the problem.
Comment 6 Andreas Roussos 2016-09-21 06:24:36 UTC
(In reply to M. Tompsett from comment #5)
[snip]
> While this is all true, that's not what the block of code is trying to do.
> Patron's should have an expiry date for their account. The problem is not
> the lack of category code, the problem is the lack of expiry date. This code
> is trying to correct the expiry date based on the category code. However,
> given that we don't know what the category code is, the program shouldn't
> even be trying to set the dateexpiry at all. So, in the case that there is
> no category found, which is also possible by passing a BAD one, we should
> not set the dateexpiry. In short, set the dateexpiry to the function call
> *IF* there is a patron_category. Hence, my counter patch. Feel free to sign
> off on it and obsolete yours, Andreas, if you think it is better.
> 
> I don't think fixing all the templates to pass category code is the correct
> way to handle the problem.
I agree with your analysis above; the code shouldn't be trying to set an expiry
date if no patron_category can be found, so it makes more sense to fix this in
members/memberentry.pl rather than in the templates.

I've obsoleted my patch and will sign off on yours.
Comment 7 Andreas Roussos 2016-09-21 06:30:06 UTC
Created attachment 55732 [details] [review]
Bug 17307: Smaller counter-patch

Given that the issue is trying to call a method on something that isn't
an object, because the category code doesn't exist, then why is it
even trying to change the expiry date? A simple postfix if fixes it.

Signed-off-by: Andreas Roussos <arouss1980@gmail.com>
Applied counter-patch by M. Tompsett. All three patron 'Edit' links
mentioned in previous patch's test plan work fine.
Comment 8 Héctor Eduardo Castro Avalos 2016-09-22 19:33:48 UTC
Good Catch, Andreas
Comment 9 Benjamin Rokseth 2016-09-22 22:25:03 UTC
Created attachment 55760 [details] [review]
Bug 17307: Smaller counter-patch

Given that the issue is trying to call a method on something that isn't
an object, because the category code doesn't exist, then why is it
even trying to change the expiry date? A simple postfix if fixes it.

Applied counter-patch by M. Tompsett. All three patron 'Edit' links
mentioned in previous patch's test plan work fine.

Signed-off-by: Andreas Roussos <arouss1980@gmail.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 10 Katrin Fischer 2016-09-25 17:07:58 UTC
Created attachment 55805 [details] [review]
[PASSED QA] Bug 17307: Smaller counter-patch

Given that the issue is trying to call a method on something that isn't
an object, because the category code doesn't exist, then why is it
even trying to change the expiry date? A simple postfix if fixes it.

Applied counter-patch by M. Tompsett. All three patron 'Edit' links
mentioned in previous patch's test plan work fine.

Signed-off-by: Andreas Roussos <arouss1980@gmail.com>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 11 Kyle M Hall 2016-09-27 11:10:58 UTC
Pushed to master for 16.11, thanks Mark!