Bug 34776 - Patron messaging preferences are lost when an error occurs during new account creation
Summary: Patron messaging preferences are lost when an error occurs during new account...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Emmi Takkinen
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-12 21:01 UTC by Aleisha Amohia
Modified: 2025-06-30 18:54 UTC (History)
7 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes creating a new patron - the messaging preferences are now remembered if there is an error when creating a new patron. Before this, if there was an error when creating a patron (for example, the wrong age for the patron category), the messaging preferences (either the default or changes made) were emptied and needed to be re-added.
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 34776: Restore patron messaging preferences if error occurs during new account creation (4.38 KB, patch)
2025-02-26 06:49 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34776: Restore patron messaging preferences if error occurs during new account creation (4.43 KB, patch)
2025-03-04 09:27 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34776: Restore patron messaging preferences if error occurs during new account creation (4.49 KB, patch)
2025-04-01 09:21 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 34776: Unit tests (1.54 KB, patch)
2025-04-23 15:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Remove unneeded if statement (1.28 KB, patch)
2025-04-24 11:31 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34776: Add unit tests for sub restore_form_values (2.70 KB, patch)
2025-04-24 11:31 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34776: Add unit tests for sub restore_form_values (3.85 KB, patch)
2025-06-13 05:33 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34776: Add unit tests for sub restore_form_value (3.86 KB, patch)
2025-06-13 06:09 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34776: Restore patron messaging preferences if error occurs during new account creation (4.53 KB, patch)
2025-06-13 09:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 34776: Unit tests (1.55 KB, patch)
2025-06-13 09:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 34776: Remove unneeded if statement (1.32 KB, patch)
2025-06-13 09:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 34776: Add unit tests for sub restore_form_values (3.91 KB, patch)
2025-06-13 09:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 34776: Restore patron messaging preferences if error occurs during new account creation (4.63 KB, patch)
2025-06-27 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Unit tests (1.64 KB, patch)
2025-06-27 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Remove unneeded if statement (1.42 KB, patch)
2025-06-27 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Add unit tests for sub restore_form_values (4.00 KB, patch)
2025-06-27 07:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Restore patron messaging preferences if error occurs during new account creation (4.63 KB, patch)
2025-06-27 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Unit tests (1.64 KB, patch)
2025-06-27 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Remove unneeded if statement (1.42 KB, patch)
2025-06-27 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34776: Add unit tests for sub restore_form_values (4.00 KB, patch)
2025-06-27 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2023-09-12 21:01:36 UTC
To reproduce:

1. Create a new account but cause an error that will keep the account from saving (enter the wrong age for a category or give the patron a username that's already being used)
2. Set some patron messaging preferences
3. Try to save the account and wait for the "The following fields are wrong. Please fix them." message
4. Note that the patron messaging preference selections are gone and you need to re-select them

Note, this depends on the EnhancedMessagingPreferences system preference to be enabled

This is similar to Bug 26558
Comment 1 Emmi Takkinen 2024-03-20 07:34:01 UTC
This also happens when patron is duplicate (see bug 25228). It seems that when error occurs, default messaging preferences for patron category are used.
Comment 2 Emmi Takkinen 2024-03-20 10:04:48 UTC
I ran into another problem while looking into this, see bug 36368.
Comment 3 Emmi Takkinen 2025-02-26 06:49:11 UTC
Created attachment 178702 [details] [review]
Bug 34776: Restore patron messaging preferences if error occurs during new account creation

If error occurs while creating a new patron messaging
preferences are either lost or reseted as default
fro patron category. This patch adds new method
restore_form_values to restore already added messaging
preferences.

To test:
1. Enable syspref EnhancedMessagingPreferences.
2. Create a new patron but cause an error to prevent it from
saving (e.g. add wrong age).
3. Add messaging preferences for patron (if there are default
preferences, add also new ones or remove defaults).
4. Attempt to save.
=> Note that messaging preferences were lost and you need to
readd them.
5. Apply this patch.
6. Repeat steps from 2 to 4.
=> Note that messaging preferences which you added are kept
even if error occurs.

Sponsored-by: Koha-Suomi Oy
Comment 4 Emmi Takkinen 2025-03-04 09:27:26 UTC
Created attachment 178896 [details] [review]
Bug 34776: Restore patron messaging preferences if error occurs during new account creation

If error occurs while creating a new patron messaging
preferences are either lost or reseted as default
fro patron category. This patch adds new method
restore_form_values to restore already added messaging
preferences.

To test:
1. Enable syspref EnhancedMessagingPreferences.
2. Create a new patron but cause an error to prevent it from
saving (e.g. add wrong age).
3. Add messaging preferences for patron (if there are default
preferences, add also new ones or remove defaults).
4. Attempt to save.
=> Note that messaging preferences were lost and you need to
readd them.
5. Apply this patch.
6. Repeat steps from 2 to 4.
=> Note that messaging preferences which you added are kept
even if error occurs.

Sponsored-by: Koha-Suomi Oy
Comment 5 Jesse Maseto 2025-04-01 09:21:06 UTC
Created attachment 180100 [details] [review]
Bug 34776: Restore patron messaging preferences if error occurs during new account creation

If error occurs while creating a new patron messaging
preferences are either lost or reseted as default
fro patron category. This patch adds new method
restore_form_values to restore already added messaging
preferences.

To test:
1. Enable syspref EnhancedMessagingPreferences.
2. Create a new patron but cause an error to prevent it from
saving (e.g. add wrong age).
3. Add messaging preferences for patron (if there are default
preferences, add also new ones or remove defaults).
4. Attempt to save.
=> Note that messaging preferences were lost and you need to
readd them.
5. Apply this patch.
6. Repeat steps from 2 to 4.
=> Note that messaging preferences which you added are kept
even if error occurs.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 6 Marcel de Rooy 2025-04-18 09:57:17 UTC
Couldnt finish here, but a few questions:

This if seems unneeded :)
        if ( $op eq 'add_form' ) {
            C4::Form::MessagingPreferences::restore_form_values( $input, $template );
        } else {
            C4::Form::MessagingPreferences::restore_form_values( $input, $template );
        }

+        my @transport_types = $input->multi_param($message_attribute_id);
+        foreach my $transport_type (@transport_types) {
+            $option->{ 'transports_' . $transport_type } = 1;
+        }
What is done here exactly? Not all values are transport types, right ?

A new routine in a module requires a unit test. Could you add one?
Comment 7 Emmi Takkinen 2025-04-22 10:05:58 UTC
(In reply to Marcel de Rooy from comment #6)
> Couldnt finish here, but a few questions:
> 
> This if seems unneeded :)
>         if ( $op eq 'add_form' ) {
>             C4::Form::MessagingPreferences::restore_form_values( $input,
> $template );
>         } else {
>             C4::Form::MessagingPreferences::restore_form_values( $input,
> $template );
>         }
Hmm, I probably assumed that if an error occurs during creating a new patron, op is still add_form. But that doesn't seem to be the case, it changes as cud-insert. So this is indeed unneeded. 

> +        my @transport_types = $input->multi_param($message_attribute_id);
> +        foreach my $transport_type (@transport_types) {
> +            $option->{ 'transports_' . $transport_type } = 1;
> +        }
> What is done here exactly? Not all values are transport types, right ?
While looping through all the message preferences it first finds the transport type which is selected in the form for the message preferences (by using its id). These are send from the form to back-end as key-value pairs like this: 4: 'email'. Or if sms messages are enabled 4: ['sms', 'email']. Then it loops through the found transport types. It takes the preference ($option variable), finds 'transports_[transport_type]' key and changes its value as 1.    

> A new routine in a module requires a unit test. Could you add one?
Will do. This might take a while though since we don't have any tests for this module yet.
Comment 8 Emmi Takkinen 2025-04-23 06:11:40 UTC
I'm now a bit lost here. These methods in C4::Form::MessagingPreferences take template inputs as parameters and I think they should be located in the memberentry.pl like method patron_attributes_form. But since OPAC needs the same functionalities they have been added to separate module under C4 (search bug 3222: new module to handle messaging preferences form from git history). So we can't really tests these like normal methods as far as I can tell. Is there another way to test these?
Comment 9 Marcel de Rooy 2025-04-23 14:29:11 UTC
(In reply to Emmi Takkinen from comment #8)
> I'm now a bit lost here. These methods in C4::Form::MessagingPreferences
> take template inputs as parameters and I think they should be located in the
> memberentry.pl like method patron_attributes_form. But since OPAC needs the
> same functionalities they have been added to separate module under C4
> (search bug 3222: new module to handle messaging preferences form from git
> history). So we can't really tests these like normal methods as far as I can
> tell. Is there another way to test these?

Taking a look.
Comment 10 Marcel de Rooy 2025-04-23 14:33:22 UTC
The only thing we have now, is:
#!/usr/bin/perl
#
# This Koha test module is a stub!
# Add more tests here!!!
use strict;
use warnings;
use Test::NoWarnings;
use Test::More tests => 2;
BEGIN {
    use_ok('C4::Form::MessagingPreferences');
}

So I cant be harshly insisting for tests here. But let me think a little bit.
Comment 11 Marcel de Rooy 2025-04-23 15:08:56 UTC
Created attachment 181389 [details] [review]
Bug 34776: Unit tests

Just a small beginning..
Comment 12 Marcel de Rooy 2025-04-23 15:09:41 UTC
Would you manage to add something related to what you are doing in restore_form_values in this test file attached?
Comment 13 Emmi Takkinen 2025-04-24 11:31:12 UTC
Created attachment 181433 [details] [review]
Bug 34776: Remove unneeded if statement

We don't need to check if $op is add_form after
error, since this value always changes.

Sponsored-by: Koha-Suomi Oy
Comment 14 Emmi Takkinen 2025-04-24 11:31:34 UTC
Created attachment 181434 [details] [review]
Bug 34776: Add unit tests for sub restore_form_values

Test that input values don't change when send to the
sub restore_form_values.

To test prove t/Form_MessagingPreferences.t

Sponsored-by: Koha-Suomi Oy
Comment 15 Emmi Takkinen 2025-04-24 11:33:52 UTC
I added a very simple test that checks if the input values remain the same after they are handled with the restore_form_values.
Comment 16 Marcel de Rooy 2025-04-24 11:59:40 UTC
Nice. I am still seeing something wrong in the test. Hang on.
Comment 17 Marcel de Rooy 2025-04-24 12:32:10 UTC
[1]
Instead of 
+    my $set_form_values_vars = $vars;

we should do something like:
+    my $set_form_values_vars = { %$vars };
+    $vars = {};

Otherwise we are just comparing the same.

[2]
If you change [1], you will get an error on digest. There is something wrong. Maybe related to [3].

[3]
The test does
+    C4::Members::Messaging::SetMessagingPreference(
+        {
+            borrowernumber          => $patron->id,
So changing prefs for a specific borrower.

The POD of restore_form_values says:
    C4::Form::MessagingPreferences::restore_form_values({ borrowernumber => 51 }, $template, $input);
Restores patron message preferences if error occurs while creating a patron.

But the code does not look at a borrower:
    my ( $input, $template ) = @_;
    my $messaging_options = C4::Members::Messaging::GetMessagingOptions();
We do not even pass a patron.
Comment 18 Emmi Takkinen 2025-06-12 11:32:21 UTC
(In reply to Marcel de Rooy from comment #17)
> [1]
> Instead of 
> +    my $set_form_values_vars = $vars;
> 
> we should do something like:
> +    my $set_form_values_vars = { %$vars };
> +    $vars = {};
> 
> Otherwise we are just comparing the same.
> 
> [2]
> If you change [1], you will get an error on digest. There is something
> wrong. Maybe related to [3].
> 
> [3]
> The test does
> +    C4::Members::Messaging::SetMessagingPreference(
> +        {
> +            borrowernumber          => $patron->id,
> So changing prefs for a specific borrower.
> 
> The POD of restore_form_values says:
>     C4::Form::MessagingPreferences::restore_form_values({ borrowernumber =>
> 51 }, $template, $input);
> Restores patron message preferences if error occurs while creating a patron.
> 
> But the code does not look at a borrower:
>     my ( $input, $template ) = @_;
>     my $messaging_options = C4::Members::Messaging::GetMessagingOptions();
> We do not even pass a patron.

POD is just wrong, restore_form_values receives only $input and $template params. In tests I tried to replicate the situation where we add patron with new message preferences and then we fetch them with set_form_values, hence use of SetMessagingPreference. But... now I have no idea how I thought this should work :D Back to the drawing board.
Comment 19 Emmi Takkinen 2025-06-13 05:33:26 UTC
Created attachment 183221 [details] [review]
Bug 34776: Add unit tests for sub restore_form_values

Test that input values don't change when send to the
sub restore_form_values.

To test prove t/Form_MessagingPreferences.t

Sponsored-by: Koha-Suomi Oy
Comment 20 Emmi Takkinen 2025-06-13 05:36:17 UTC
Fixed tests and POD. I added a new test to cover situation where messaging preferences are default preferences for patron category. Other still tests what happens when preferences have been stored to database via SetMessagingPreference and then set in template via set_form_values.
Comment 21 Emmi Takkinen 2025-06-13 06:07:45 UTC
Forgot to run qa-tool, test file isn't tidy.
Comment 22 Emmi Takkinen 2025-06-13 06:09:55 UTC
Created attachment 183222 [details] [review]
Bug 34776: Add unit tests for sub restore_form_value

Test that input values don't change when send to the
sub restore_form_values.

To test prove t/Form_MessagingPreferences.t

Sponsored-by: Koha-Suomi Oy
Comment 23 David Nind 2025-06-13 09:24:29 UTC
Created attachment 183236 [details] [review]
Bug 34776: Restore patron messaging preferences if error occurs during new account creation

If error occurs while creating a new patron messaging
preferences are either lost or reseted as default
fro patron category. This patch adds new method
restore_form_values to restore already added messaging
preferences.

To test:
1. Enable syspref EnhancedMessagingPreferences.
2. Create a new patron but cause an error to prevent it from
saving (e.g. add wrong age).
3. Add messaging preferences for patron (if there are default
preferences, add also new ones or remove defaults).
4. Attempt to save.
=> Note that messaging preferences were lost and you need to
readd them.
5. Apply this patch.
6. Repeat steps from 2 to 4.
=> Note that messaging preferences which you added are kept
even if error occurs.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2025-06-13 09:24:32 UTC
Created attachment 183237 [details] [review]
Bug 34776: Unit tests

Just a small beginning..

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2025-06-13 09:24:36 UTC
Created attachment 183238 [details] [review]
Bug 34776: Remove unneeded if statement

We don't need to check if $op is add_form after
error, since this value always changes.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2025-06-13 09:24:40 UTC
Created attachment 183239 [details] [review]
Bug 34776: Add unit tests for sub restore_form_values

Test that input values don't change when send to the
sub restore_form_values.

To test prove t/Form_MessagingPreferences.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2025-06-13 09:36:53 UTC
Testing notes (using KTD):
1. Add a 'Kid' patron.
2. Complete all the mandatory fields.
3. Change the date of birthday so that they are over 25 years old.
4. Add some messaging preferences.
5. Attempt to save.
Comment 28 Marcel de Rooy 2025-06-27 07:01:55 UTC
Created attachment 183573 [details] [review]
Bug 34776: Restore patron messaging preferences if error occurs during new account creation

If error occurs while creating a new patron messaging
preferences are either lost or reseted as default
fro patron category. This patch adds new method
restore_form_values to restore already added messaging
preferences.

To test:
1. Enable syspref EnhancedMessagingPreferences.
2. Create a new patron but cause an error to prevent it from
saving (e.g. add wrong age).
3. Add messaging preferences for patron (if there are default
preferences, add also new ones or remove defaults).
4. Attempt to save.
=> Note that messaging preferences were lost and you need to
readd them.
5. Apply this patch.
6. Repeat steps from 2 to 4.
=> Note that messaging preferences which you added are kept
even if error occurs.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2025-06-27 07:01:57 UTC
Created attachment 183574 [details] [review]
Bug 34776: Unit tests

Just a small beginning..

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2025-06-27 07:01:59 UTC
Created attachment 183575 [details] [review]
Bug 34776: Remove unneeded if statement

We don't need to check if $op is add_form after
error, since this value always changes.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2025-06-27 07:02:01 UTC
Created attachment 183576 [details] [review]
Bug 34776: Add unit tests for sub restore_form_values

Test that input values don't change when send to the
sub restore_form_values.

To test prove t/Form_MessagingPreferences.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2025-06-27 07:03:45 UTC
Created attachment 183577 [details] [review]
Bug 34776: Restore patron messaging preferences if error occurs during new account creation

If error occurs while creating a new patron messaging
preferences are either lost or reseted as default
fro patron category. This patch adds new method
restore_form_values to restore already added messaging
preferences.

To test:
1. Enable syspref EnhancedMessagingPreferences.
2. Create a new patron but cause an error to prevent it from
saving (e.g. add wrong age).
3. Add messaging preferences for patron (if there are default
preferences, add also new ones or remove defaults).
4. Attempt to save.
=> Note that messaging preferences were lost and you need to
readd them.
5. Apply this patch.
6. Repeat steps from 2 to 4.
=> Note that messaging preferences which you added are kept
even if error occurs.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2025-06-27 07:03:47 UTC
Created attachment 183578 [details] [review]
Bug 34776: Unit tests

Just a small beginning..

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2025-06-27 07:03:49 UTC
Created attachment 183579 [details] [review]
Bug 34776: Remove unneeded if statement

We don't need to check if $op is add_form after
error, since this value always changes.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2025-06-27 07:03:51 UTC
Created attachment 183580 [details] [review]
Bug 34776: Add unit tests for sub restore_form_values

Test that input values don't change when send to the
sub restore_form_values.

To test prove t/Form_MessagingPreferences.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2025-06-27 07:05:38 UTC
Thx for your perseverance, Emmi!
Comment 37 Lucas Gass (lukeg) 2025-06-30 14:32:12 UTC
Nice work everyone!

Pushed to main for 25.11