Bug 19080 - Handle non existing patrons
Summary: Handle non existing patrons
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17829
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-10 17:25 UTC by Jonathan Druart
Modified: 2017-09-12 14:19 UTC (History)
3 users (show)

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


Attachments
Bug 19080: Handle non-existing patrons gratefully (24.54 KB, patch)
2017-08-10 17:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19080: Handle non-existing patrons gratefully (23.79 KB, patch)
2017-08-10 19:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19080: Fix member vs borrowernumber in delmember (867 bytes, patch)
2017-08-15 18:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19080: Handle non-existing patrons gratefully (23.72 KB, patch)
2017-08-18 20:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19080: Fix member vs borrowernumber in delmember (929 bytes, patch)
2017-08-18 20:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19080: Fix perlcritic in routing-lists.pl (907 bytes, patch)
2017-08-23 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19080: Handle non-existing patrons gratefully (23.90 KB, patch)
2017-08-23 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19080: Fix member vs borrowernumber in delmember (982 bytes, patch)
2017-08-23 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19080: Fix perlcritic in routing-lists.pl (967 bytes, patch)
2017-08-23 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-08-10 17:25:46 UTC
If a patron does not exist, scripts from the members module should handle it gracefully instead of crashing.
Comment 1 Jonathan Druart 2017-08-10 17:50:04 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2017-08-10 19:57:55 UTC
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.
Comment 3 Owen Leonard 2017-08-11 15:08:49 UTC
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
Comment 4 Jonathan Druart 2017-08-15 18:09:39 UTC
Created attachment 66036 [details] [review]
Bug 19080: Fix member vs borrowernumber in delmember
Comment 5 Jonathan Druart 2017-08-15 18:11:29 UTC
(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.
Comment 6 Josef Moravec 2017-08-18 20:27:35 UTC
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>
Comment 7 Josef Moravec 2017-08-18 20:27:39 UTC
Created attachment 66191 [details] [review]
[SIGNED-OFF] Bug 19080: Fix member vs borrowernumber in delmember

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Julian Maurice 2017-08-23 14:00:21 UTC
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
Comment 9 Jonathan Druart 2017-08-23 14:43:30 UTC
Created attachment 66383 [details] [review]
Bug 19080: Fix perlcritic in routing-lists.pl
Comment 10 Jonathan Druart 2017-08-23 14:44:56 UTC
(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)
Comment 11 Julian Maurice 2017-08-23 14:50:47 UTC
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>
Comment 12 Julian Maurice 2017-08-23 14:50:51 UTC
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>
Comment 13 Julian Maurice 2017-08-23 14:50:54 UTC
Created attachment 66386 [details] [review]
Bug 19080: Fix perlcritic in routing-lists.pl

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 14 Jonathan Druart 2017-08-25 14:10:06 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 15 Fridolin SOMERS 2017-09-12 14:19:51 UTC
Depends a lot on Bug 17829, I'm not backporting to 17.05.x