Bug 36368

Summary: Cannot save new patron after error
Product: Koha Reporter: Emmi Takkinen <emmi.takkinen>
Component: PatronsAssignee: Emmi Takkinen <emmi.takkinen>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, kyle.m.hall, Laura.escamilla, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Circulation function:
Bug Depends on: 34478    
Bug Blocks:    
Attachments: Bug 36368: Declare template param "op" after variable $op is set
Works as described! Thank you for the patch.
Bug 36368: Declare template param "op" after variable $op is set
Bug 36368: Declare template param "op" after variable $op is set
Bug 36368: (QA follow-up) Remove unneeded line

Description Emmi Takkinen 2024-03-20 08:45:26 UTC
If an error occurs while adding new patron, after fixing the error and hitting save, patron entry page reloads to "Modify patron" section and error "Patron not found. Return to search" is displayed. But no patron is saved.

To reproduce:
1. Add new patron and cause an error (wrong age etc.).
2. Attempt to save patron, error message is displayed.
3. Fix your errors and attempt to save again.
=> Error message "Patron not found. Return to search" is displayed and new patron is not saved to database.
Comment 1 Emmi Takkinen 2024-03-20 10:43:58 UTC
It seems that this stems from change made somewhere in bug 34478. But I'm unsure where from there.
Comment 2 Emmi Takkinen 2024-03-20 13:26:30 UTC
We declare param "op" too early in memberentry.pl line 166. This line

>$template->param( "op" => $op );
was like this before bug 34478:

>$template->param( "add" => 1 ) if ( $op eq 'add' );

Add this point, $op is still "cud-insert". So when error occurs, instead of displaying add form, we display modify form. I assume this is easily fixed just by moving line 166 further in code.
Comment 3 Emmi Takkinen 2024-03-20 13:36:09 UTC
Created attachment 163526 [details] [review]
Bug 36368: Declare template param "op" after variable $op is set

If an error occurs while adding new patron, after fixing the error and
hitting save, patron entry page reloads to "Modify patron" section
and error "Patron not found. Return to search" is displayed. But no
patron is saved.

This happens because we declare template param op too early in the
code and it receives value "cud-insert" instead of "add_form" as it should.

To test:
1. Add new patron and cause an error (wrong age etc.).
2. Attempt to save patron, error message is displayed.
3. Fix your errors and attempt to save again.
=> Error message "Patron not found. Return to search" is displayed and new
patron is not saved to database.
4. Apply this patch.
5. Repeat steps from 1 to 3.
=> Saving patron should now work.
=> To be save, test if modifying patron also works as it should.

Sponsored-by: Koha-Suomi Oy
Comment 4 Emmi Takkinen 2024-03-21 06:35:39 UTC
Setting bugs importance as major since this is quite nasty bug. Feel free to change it.
Comment 5 Katrin Fischer 2024-03-21 06:36:28 UTC
(In reply to Emmi Takkinen from comment #4)
> Setting bugs importance as major since this is quite nasty bug. Feel free to
> change it.

I just did the same on another CSRF related bug, thanks for catching this one - certainly not an enh.
Comment 6 Laura Escamilla 2024-03-21 14:44:33 UTC Comment hidden (obsolete)
Comment 7 Laura Escamilla 2024-03-21 14:45:54 UTC
Created attachment 163625 [details] [review]
Bug 36368: Declare template param "op" after variable $op is set

If an error occurs while adding new patron, after fixing the error and
hitting save, patron entry page reloads to "Modify patron" section
and error "Patron not found. Return to search" is displayed. But no
patron is saved.

This happens because we declare template param op too early in the
code and it receives value "cud-insert" instead of "add_form" as it should.

To test:
1. Add new patron and cause an error (wrong age etc.).
2. Attempt to save patron, error message is displayed.
3. Fix your errors and attempt to save again.
=> Error message "Patron not found. Return to search" is displayed and new
patron is not saved to database.
4. Apply this patch.
5. Repeat steps from 1 to 3.
=> Saving patron should now work.
=> To be save, test if modifying patron also works as it should.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 8 Marcel de Rooy 2024-03-22 08:30:53 UTC
    $op = 'add_form'  if ( $op eq 'cud-insert' );
    $op = 'edit_form' if ( $op eq 'cud-save' );
    %data=%newdata;
    $template->param( updtype => ($op eq 'add' ?'I':'M'));  # used to check for $op eq "cud-insert"... but we just changed $op!

Looks like we are not done here yet. See above.
We are looking for 'add' but I see add_form ?

FQA
Comment 9 Emmi Takkinen 2024-03-25 06:42:39 UTC
(In reply to Marcel de Rooy from comment #8)
>     $op = 'add_form'  if ( $op eq 'cud-insert' );
>     $op = 'edit_form' if ( $op eq 'cud-save' );
>     %data=%newdata;
>     $template->param( updtype => ($op eq 'add' ?'I':'M'));  # used to check
> for $op eq "cud-insert"... but we just changed $op!
> 
> Looks like we are not done here yet. See above.
> We are looking for 'add' but I see add_form ?
> 
> FQA

So it seems. But I'm wondering do we really need that line? Starting from line 628 we have two if statements. First is used to check if $op equals "add_form" and if that's true updtype is "I". And in line 636 we check if $op equals "edit_form" and set updtype as "M". So why do we set updtype param already in that $template->param( updtype => ($op eq 'add' ?'I':'M'));?
Comment 10 Marcel de Rooy 2024-03-29 08:27:03 UTC
Created attachment 164111 [details] [review]
Bug 36368: Declare template param "op" after variable $op is set

If an error occurs while adding new patron, after fixing the error and
hitting save, patron entry page reloads to "Modify patron" section
and error "Patron not found. Return to search" is displayed. But no
patron is saved.

This happens because we declare template param op too early in the
code and it receives value "cud-insert" instead of "add_form" as it should.

To test:
1. Add new patron and cause an error (wrong age etc.).
2. Attempt to save patron, error message is displayed.
3. Fix your errors and attempt to save again.
=> Error message "Patron not found. Return to search" is displayed and new
patron is not saved to database.
4. Apply this patch.
5. Repeat steps from 1 to 3.
=> Saving patron should now work.
=> To be save, test if modifying patron also works as it should.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2024-03-29 08:27:05 UTC
Created attachment 164112 [details] [review]
Bug 36368: (QA follow-up) Remove unneeded line

As Emmy stated on comment9, this line is indeed unneeded (and wrong).
A successful cud-insert/cud-save does not come here.
And it is set for add_form, edit_form and duplicate later on.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Katrin Fischer 2024-04-02 16:00:58 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 Fridolin Somers 2024-04-17 15:14:46 UTC
Depends on Bug 34478 not in 23.11.x