Bug 21083

Summary: Batch patron modification does not allow to modify repeatable patron attributes
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: ToolsAssignee: Jonathan Druart <jonathan.druart+koha>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: severine.queune
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15367, 20443    
Bug Blocks:    
Attachments: Bug 21083: Handle repeatable patron attributes in batch patron modification tool
Bug 21083: Remove all attributes

Description Jonathan Druart 2018-07-18 13:34:39 UTC
This has been a limitation of the script since it has been integrated in to Koha (see bug 12636).
I would like to see bug 20443 pushed and then could work on fixing this one.
Comment 1 Jonathan Druart 2020-05-21 13:12:26 UTC
It's not bug 12636 but bug 15367
Comment 2 Jonathan Druart 2020-05-21 14:17:37 UTC
Created attachment 105201 [details] [review]
Bug 21083: Handle repeatable patron attributes in batch patron modification tool

This patch adds the ability to set patron attributes marked as
repeatable in the batch patron modification tool.
Prior to this patch they were ignored.

Test plan:
You should try with several combinaisons and set patron attributes using
the batch patron modification tool.
Make sure there is no data lose and that the result is what you expect
Please detail in a comment what you tested.
Comment 3 Jonathan Druart 2020-05-22 07:45:33 UTC
I think the following changes should be applied to Koha::Patron->extended_attributes

diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 5436ccc448..cf8264a308 100644
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -1482,6 +1482,9 @@ sub extended_attributes {

                 # Insert the new ones
                 for my $attribute (@$attributes) {
+                    # FIXME It seems that we need this check here
+                    next if $attribute->category_code && $attribute->category_code ne $self->categorycode;
+
                     eval {
                         $self->_result->create_related('borrower_attributes', $attribute);
                     };


However it's not needed for this bug report.
Comment 4 Séverine Queune 2022-04-04 14:58:26 UTC
I not sure I totally understood the expected behaviour of this patch, but here as some feedback about my first quick tests.

If patrons have several entries for one attribute, the batch modification replace all the attributes by only the new value. I assume so but can you confirm it's the behaviour expected ?

I also noticed :

  - when I check the box to empty the field, I still have empty entries in database instead of having no entries at all, e.g; : 
select * from borrower_attributes
--> id 	 borrowernumber  code 	 attribute
--> 124  21              disc 	 Maths
--> 125  21              disc 	 
--> 126  21              disc 	 Maths
Shouldn't line 125 have been deleted ?

 -you can create several empty datas : to reproduce, select an attribute, checkbox the box, add the same attribute and check the box again. Is there a way to lock the use of an attribute that is selected 'to delete' ?

Expect that points, I've tests with both free input and authorized values linked attributes and it seems to work quite fine.

Failed though for the empty data issues in borrower_attributes table.
Comment 5 Jonathan Druart 2022-04-08 07:29:05 UTC
Created attachment 133104 [details] [review]
Bug 21083: Remove all attributes

not only the first one
Comment 6 Jonathan Druart 2022-04-08 07:30:11 UTC
Hi Séverine, I think this patch should fix the problems you found.


(In reply to Séverine Queune from comment #4)
> I not sure I totally understood the expected behaviour of this patch, but
> here as some feedback about my first quick tests.
> 
> If patrons have several entries for one attribute, the batch modification
> replace all the attributes by only the new value. I assume so but can you
> confirm it's the behaviour expected ?

I think so :)