|
Description
Jonathan Druart
2018-07-18 13:34:39 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. 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.
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. Created attachment 133104 [details] [review] Bug 21083: Remove all attributes not only the first one 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 :) 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. Created attachment 149750 [details] [review] Bug 21083: Remove all attributes not only the first one (In reply to Katrin Fischer from comment #9) > Did you see bug 33156? Why are you asking? Because it fixes JavaScript error breaking functionality with the patron attributes on that page. 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 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> Created attachment 152708 [details] [review] Bug 21083: Remove all attributes not only the first one Signed-off-by: Philip Orr <philip.orr@lmscloud.de> 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
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> 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> 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> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement or bug ? Needs backport right ? Backport to 23.05 would be great. |