If a patron does not exist, scripts from the members module should handle it gracefully instead of crashing.
Created attachment 65834 [details] [review] Bug 19080: Handle non-existing patrons gratefully This is a recurrent bug we have over the last years. When a script is called with non-existent borrowernumber it will crashes. We need to handle this gracefully instead of letting the script crashes. On bug 18403 a new subroutine is added to the codebase (output_and_exit_if_error) to handle this kind of errors correctly. Since it is not pushed yet, I propose to just redirect to a script that handle it correctly (circulation.pl) instead of adding this message to all these scripts. Test plan: Hit different scripts from the members module and pass a non-existent borrowernumber. You must be redirected to circulation.pl with a friendly message.
Created attachment 65854 [details] [review] Bug 19080: Handle non-existing patrons gratefully This is a recurrent bug we have over the last years. When a script is called with non-existent borrowernumber it will crashes. We need to handle this gracefully instead of letting the script crashes. On bug 18403 a new subroutine is added to the codebase (output_and_exit_if_error) to handle this kind of errors correctly. Since it is not pushed yet, I propose to just redirect to a script that handle it correctly (circulation.pl) instead of adding this message to all these scripts. Test plan: Hit different scripts from the members module and pass a non-existent borrowernumber. You must be redirected to circulation.pl with a friendly message.
I had problems with several of the modified scripts: members/deletemem.pl - Loads the logged-in patron's account into circulation instead of showing the message. members/member-flags.pl & members/member-password.pl - Load circ with empty borrowernumber parameter and no message members/memberentry.pl - Loads "modify patron" form with no data. No redirect. members/update-child.pl - "An Error Occurred" 500 Internal Server Error
Created attachment 66036 [details] [review] Bug 19080: Fix member vs borrowernumber in delmember
(In reply to Owen Leonard from comment #3) > I had problems with several of the modified scripts: > > members/deletemem.pl - Loads the logged-in patron's account into circulation > instead of showing the message. Indeed, wrong variable. Fixed! > members/member-flags.pl & members/member-password.pl - Load circ with empty > borrowernumber parameter and no message > > members/memberentry.pl - Loads "modify patron" form with no data. No > redirect. > > members/update-child.pl - "An Error Occurred" 500 Internal Server Error I think you got that because called these scripts only with the borrowernumber parameter. Most of them need a "op" variable to behave correctly. I guess you get the same behaviour with an existing borrowernumber.
Created attachment 66190 [details] [review] [SIGNED-OFF] Bug 19080: Handle non-existing patrons gratefully This is a recurrent bug we have over the last years. When a script is called with non-existent borrowernumber it will crashes. We need to handle this gracefully instead of letting the script crashes. On bug 18403 a new subroutine is added to the codebase (output_and_exit_if_error) to handle this kind of errors correctly. Since it is not pushed yet, I propose to just redirect to a script that handle it correctly (circulation.pl) instead of adding this message to all these scripts. Test plan: Hit different scripts from the members module and pass a non-existent borrowernumber. You must be redirected to circulation.pl with a friendly message. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66191 [details] [review] [SIGNED-OFF] Bug 19080: Fix member vs borrowernumber in delmember Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
QA comment: 1) $ perlcritic members/routing-lists.pl Variable declared in conditional statement at line 51, column 1. Declare variables outside of the condition. (Severity: 5) This error is not detected by koha-qa, I'm not sure why. 2) If borrowernumber is 0, empty or not defined, the error message is not displayed in circ/circulation.pl Failing QA for 1
Created attachment 66383 [details] [review] Bug 19080: Fix perlcritic in routing-lists.pl
(In reply to Julian Maurice from comment #8) > QA comment: > > 1) $ perlcritic members/routing-lists.pl > Variable declared in conditional statement at line 51, column 1. Declare > variables outside of the condition. (Severity: 5) > > This error is not detected by koha-qa, I'm not sure why. Fixed, thanks! (not sure koha-qa does not catch it) > 2) If borrowernumber is 0, empty or not defined, the error message is not > displayed in circ/circulation.pl Yes it's the existing behaviour, I do not think we should change it here (if we really want to change it)
Created attachment 66384 [details] [review] Bug 19080: Handle non-existing patrons gratefully This is a recurrent bug we have over the last years. When a script is called with non-existent borrowernumber it will crashes. We need to handle this gracefully instead of letting the script crashes. On bug 18403 a new subroutine is added to the codebase (output_and_exit_if_error) to handle this kind of errors correctly. Since it is not pushed yet, I propose to just redirect to a script that handle it correctly (circulation.pl) instead of adding this message to all these scripts. Test plan: Hit different scripts from the members module and pass a non-existent borrowernumber. You must be redirected to circulation.pl with a friendly message. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 66385 [details] [review] Bug 19080: Fix member vs borrowernumber in delmember Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 66386 [details] [review] Bug 19080: Fix perlcritic in routing-lists.pl Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Pushed to master for 17.11, thanks to everybody involved!
Depends a lot on Bug 17829, I'm not backporting to 17.05.x