Bug 17403

Summary: Internal Server Error while deleting patron
Product: Koha Reporter: Marc Véron <veron>
Component: PatronsAssignee: Marc Véron <veron>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P1 - high CC: brendan, f.demians, gmcharlt, julian.maurice, katrin.fischer, kyle.m.hall
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17411
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 17403: Internal Server Error while deleting patron
Bug 17403: Internal Server Error while deleting patron
Bug 17403: Internal Server Error while deleting patron
[PASSED QA] Bug 17403: Internal Server Error while deleting patron

Description Marc Véron 2016-10-05 13:42:05 UTC
To reproduce:

- Go to a patron's detail page
- Toolbar : More : Delete
- Confirm "Are you sure you want to delete..."

Result: Internal Server Error
Plack error log:
exited nonzero: 1 at /home/marc/koha/members/deletemem.pl
Comment 1 Marc Véron 2016-10-05 13:47:33 UTC
Created attachment 56055 [details] [review]
Bug 17403: Internal Server Error while deleting patron

This patch fixes a Internal Server Error while deleting patrons.

To reproduce:

- Go to a patron's detail page
- Toolbar : More : Delete
- Confirm "Are you sure you want to delete..."

Result: Internal Server Error
Plack error log:
exited nonzero: 1 at /home/marc/koha/members/deletemem.pl
Note: Patron is deleted

To test:

- Apply patch
- Re-start plack
- Try to reproduce steps above (with other patron)
Expected result:
No Internal Server Error, Redirect to Home > Patrons
Comment 2 Marc Véron 2016-10-05 14:40:36 UTC
Created attachment 56057 [details] [review]
Bug 17403: Internal Server Error while deleting patron

This patch fixes a Internal Server Error while deleting patrons.

To reproduce:

- Go to a patron's detail page
- Toolbar : More : Delete
- Confirm "Are you sure you want to delete..."

Result: Internal Server Error
Plack error log:
exited nonzero: 1 at /home/marc/koha/members/deletemem.pl
Note: Patron is deleted

Additional tests: Try to delete yourself, to delete a staff member
without having superlibrarian permission etc.

To test:

- Apply patch
- Re-start plack
- Try to reproduce steps above (with other patron)
Expected result:
No Internal Server Error, Redirect to Home > Patrons

Amended to include all occurences of 'exit 1'
Comment 3 Chris Cormack 2016-10-06 08:12:59 UTC
Created attachment 56067 [details] [review]
Bug 17403: Internal Server Error while deleting patron

This patch fixes a Internal Server Error while deleting patrons.

To reproduce:

- Go to a patron's detail page
- Toolbar : More : Delete
- Confirm "Are you sure you want to delete..."

Result: Internal Server Error
Plack error log:
exited nonzero: 1 at /home/marc/koha/members/deletemem.pl
Note: Patron is deleted

Additional tests: Try to delete yourself, to delete a staff member
without having superlibrarian permission etc.

To test:

- Apply patch
- Re-start plack
- Try to reproduce steps above (with other patron)
Expected result:
No Internal Server Error, Redirect to Home > Patrons

Amended to include all occurences of 'exit 1'

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Good catch Marc
Comment 4 Katrin Fischer 2016-10-10 10:07:06 UTC
I can't reproduce the problem under Plack - patron got deleted just fine. What am I missing?
Comment 5 Marc Véron 2016-10-10 14:17:19 UTC
(In reply to Katrin Fischer from comment #4)
> I can't reproduce the problem under Plack - patron got deleted just fine.
> What am I missing?

I tried again with scenario in initial comment and reproduced on current master.


Plack error log says:
exited nonzero: 1 /home/marc/members/deleteitem.pl

exit 1 signals that there was an error in the script, but at this place we simply want to exit (without signaling an error).

With patch applied deleting works fine.
Comment 6 David Cook 2016-10-10 23:11:09 UTC
Comment on attachment 56067 [details] [review]
Bug 17403: Internal Server Error while deleting patron

Review of attachment 56067 [details] [review]:
-----------------------------------------------------------------

::: members/deletemem.pl
@@ -98,4 @@
>      if ( !C4::Context->IsSuperLibrarian() && $bor->{'branchcode'}){
>          unless ($userenv->{branch} eq $bor->{'branchcode'}){
>              print $input->redirect("/cgi-bin/koha/members/moremember.pl?borrowernumber=$member&error=CANT_DELETE_OTHERLIBRARY");
> -            exit;

"exit" is the same as "exit 0", so this doesn't really matter, but I'm certainly in favour of a consistent approach. 

Why use "exit 0" over "exit" though? Just to be more explicit?
Comment 7 Marc Véron 2016-10-11 04:59:13 UTC
(In reply to David Cook from comment #6)
> Comment on attachment 56067 [details] [review] [review]
> Bug 17403: Internal Server Error while deleting patron
> 
> Review of attachment 56067 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: members/deletemem.pl
> @@ -98,4 @@
> >      if ( !C4::Context->IsSuperLibrarian() && $bor->{'branchcode'}){
> >          unless ($userenv->{branch} eq $bor->{'branchcode'}){
> >              print $input->redirect("/cgi-bin/koha/members/moremember.pl?borrowernumber=$member&error=CANT_DELETE_OTHERLIBRARY");
> > -            exit;
> 
> "exit" is the same as "exit 0", so this doesn't really matter, but I'm
> certainly in favour of a consistent approach. 
> 
> Why use "exit 0" over "exit" though? Just to be more explicit?

Yes. I prefer it because it clearly shows the intention.
BTW, it's used at many other places as well ( git grep 'exit 0' ).
Comment 8 Katrin Fischer 2016-10-11 12:59:57 UTC
Hah! ... my Plack was turned off, can reproduce the error now.
Comment 9 Katrin Fischer 2016-10-11 13:31:59 UTC
We checked the code base - we have both, exit and exit 0. At the moment, I don't consider this a blocker here. If someone wants to standardize on one writing, they can do so in a separate patch :)
Comment 10 Katrin Fischer 2016-10-11 13:40:52 UTC
Created attachment 56186 [details] [review]
[PASSED QA] Bug 17403: Internal Server Error while deleting patron

This patch fixes a Internal Server Error while deleting patrons.

To reproduce:

- Go to a patron's detail page
- Toolbar : More : Delete
- Confirm "Are you sure you want to delete..."

Result: Internal Server Error
Plack error log:
exited nonzero: 1 at /home/marc/koha/members/deletemem.pl
Note: Patron is deleted

Additional tests: Try to delete yourself, to delete a staff member
without having superlibrarian permission etc.

To test:

- Apply patch
- Re-start plack
- Try to reproduce steps above (with other patron)
Expected result:
No Internal Server Error, Redirect to Home > Patrons

Amended to include all occurences of 'exit 1'

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Good catch Marc

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 11 Brendan Gallagher 2016-10-12 07:24:17 UTC
Pushed to Master - Should be in the November 16.11 release.  Thanks!
Comment 12 Frédéric Demians 2016-10-22 11:39:38 UTC
Pushed in 16.05. Will be in 16.05.05.
Comment 13 Julian Maurice 2016-10-24 09:47:20 UTC
Pushed to 3.22.x, will be in 3.22.12