Bug 21083

Summary: Batch patron modification does not allow to modify repeatable patron attributes
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: ToolsAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, fridolin.somers, lucas, philip.orr, severine.queune
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00
Bug Depends on: 15367, 20443    
Bug Blocks: 34274, 34818    
Attachments: Bug 21083: Handle repeatable patron attributes in batch patron modification tool
Bug 21083: Remove all attributes
Bug 21083: Handle repeatable patron attributes in batch patron modification tool
Bug 21083: Remove all attributes
Bug 21083: Handle repeatable patron attributes in batch patron modification tool
Bug 21083: Remove all attributes
Bug 21083: Handle repeatable patron attributes in batch patron modification tool
Bug 21083: Remove all attributes
Bug 21083: (QA follow-up) Add a hint that attributes will replace all existing 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 :)
Comment 7 Jonathan Druart 2023-04-17 12:29:29 UTC
Created attachment 149749 [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 8 Jonathan Druart 2023-04-17 12:29:31 UTC
Created attachment 149750 [details] [review]
Bug 21083: Remove all attributes

not only the first one
Comment 9 Katrin Fischer 2023-04-23 21:28:55 UTC
Did you see bug 33156?
Comment 10 Jonathan Druart 2023-04-27 05:02:57 UTC
(In reply to Katrin Fischer from comment #9)
> Did you see bug 33156?

Why are you asking?
Comment 11 Katrin Fischer 2023-04-28 17:47:23 UTC
Because it fixes JavaScript error breaking functionality with the patron attributes on that page.
Comment 12 Lucas Gass 2023-04-28 18:22:19 UTC
If you are testing this bug it is probably good to know about this one which may effect your ability to test:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33638
Comment 13 Philip Orr 2023-06-27 07:55:18 UTC
Created attachment 152707 [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.

Signed-off-by: Philip Orr <philip.orr@lmscloud.de>
Comment 14 Philip Orr 2023-06-27 07:55:21 UTC
Created attachment 152708 [details] [review]
Bug 21083: Remove all attributes

not only the first one

Signed-off-by: Philip Orr <philip.orr@lmscloud.de>
Comment 15 Philip Orr 2023-06-27 07:56:10 UTC
First thing I tested was to create a new borrower attribute with the name "School class" with nothing checked (so not repeatable), also not linked to any authorized values.
It showed up in patron batch modification and worked correctly, the patrons I batch modified had the value I
specified ("3a") added to their attribute.

Then I tried setting the borrower attribute to repeatable. It still shows up in patron modification, so that's good.
I tried setting a new value for the same patrons ("4b"). The old value "3a" was overwritten by the new value "4b", so that worked correctly.
Then I manually gave each patron 3 separate values for the "School class" attribute, "3a", "4b" and "5c".
Then I went back to patron modification and used batch patron modification to give them a new value "6d".
This resulted in all of the previous values being deleted and instead the new value was saved.
So instead of having three separate "School class" attributes, they now only had one with the value "6d".
So that worked correctly as well.
I then tried giving the patrons multiple "School class" values in the same batch patron modification and that worked as expected too: the old value "6d" was deleted and the multiple new values "3a", "6y" and "12b" were created for each patron.

Then I thought it might be a good idea to test if I could change multiple different borrower attributes in the same
batch patron modification. Unfortunately that doesn't work very well, as soon as one the borrower attributes is connected to an authorized value (in this case our usual attribute "Show barcode" connected to the authorized value YES_NO) then in batch patron modification all the text entry fields for all borrower attributes are changed to authorized value drop down fields. So I couldn't edit "School class" and "Show barcode" at the same time in batch patron modification. I don't know if that's already an existing bug or if I should make a new bug for that.

Anyway besides that everything works as expected. Only the currently saved borrower attributes "3a", "6y" and "12b" are found in borrower_attributes:
SELECT * FROM borrower_attributes
id	borrowernumber	code	attribute
38	47				CLASS	3a
39	47				CLASS	6y
40	47				CLASS	12b
41	18				CLASS	3a
42	18				CLASS	6y
43	18				CLASS	12b
44	12				CLASS	3a
45	12				CLASS	6y
46	12				CLASS	12b
47	16				CLASS	3a
48	16				CLASS	6y
49	16				CLASS	12b
Comment 16 Nick Clemens 2023-07-13 16:50:21 UTC
Created attachment 153421 [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.

Signed-off-by: Philip Orr <philip.orr@lmscloud.de>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2023-07-13 16:50:23 UTC
Created attachment 153422 [details] [review]
Bug 21083: Remove all attributes

not only the first one

Signed-off-by: Philip Orr <philip.orr@lmscloud.de>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2023-07-13 16:50:26 UTC
Created attachment 153423 [details] [review]
Bug 21083: (QA follow-up) Add a hint that attributes will replace all existing attributes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2023-09-05 17:46:00 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Fridolin Somers 2023-09-08 06:56:23 UTC
Enhancement or bug ?
Needs backport right ?