Description
Aleisha Amohia
2023-09-12 21:01:36 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. I ran into another problem while looking into this, see bug 36368. 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 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 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> 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? (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. 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? (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. 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. Created attachment 181389 [details] [review] Bug 34776: Unit tests Just a small beginning.. Would you manage to add something related to what you are doing in restore_form_values in this test file attached? 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 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 I added a very simple test that checks if the input values remain the same after they are handled with the restore_form_values. Nice. I am still seeing something wrong in the test. Hang on. [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. (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. 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 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. Forgot to run qa-tool, test file isn't tidy. 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 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> Created attachment 183237 [details] [review] Bug 34776: Unit tests Just a small beginning.. Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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. 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> 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> 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> 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> 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> 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> 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> 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> Thx for your perseverance, Emmi! Nice work everyone! Pushed to main for 25.11 |