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
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.
Edited the bug Summary to reflect that this needs fixing in more than one place.
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.
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.
(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.
(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.
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.
Good Catch, Andreas
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>
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>
Pushed to master for 16.11, thanks Mark!