Summary: | Fix wrong attribute type cloning | ||
---|---|---|---|
Product: | Koha | Reporter: | Srdjan Jankovic <srdjan> |
Component: | Patrons | Assignee: | Srdjan Jankovic <srdjan> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | fridolin.somers, gmcharlt, kyle.m.hall, kyle, mtompset |
Version: | Main | ||
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: | ||
Circulation function: | |||
Attachments: |
bug_11184: correct attribute cloning for the maint screen
bug_11184: correct attribute cloning for the maint screen bug_11184: correct attribute cloning for the maint screen bug_11184: correct attribute cloning for the maint screen [PASSED QA] Bug 11184: correct attribute cloning for the maint screen [PASSED QA] Bug 11184: correct attribute cloning for the maint screen |
Description
Srdjan Jankovic
2013-11-01 08:22:53 UTC
Created attachment 22664 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Galen, can I ask you to scrutinise this one please. Created attachment 24412 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Srdjan and Jonathan, could you provide more information on the effect of this patch? I have tested the patron attributes on memberentry.pl and it seems to work, but it's a bit hard to tell what to look out for. Also, the warnings in my logs seem the same before and after applying the patch. Also, shouldn't this line be changed as well? 825 my $newentry = { map { $_ => $entry->{$_} } %$entry }; The fix: my $newentry = { %$entry }; takes the hash and generates a copy. This is what: my $newentry = { map { $_ => $entry->{$_} } %$entry }; is supposed to do, but it does it wrong. It could be fixed by making it: my $newentry = { map { $_ => $entry->{$_} } keys %$entry }; However, mapping and access functions to copy are much likely to be slower than just straight copying. Instead of copying { k1 => v1, k2 => v2, ..., kn => vn } The broken code generates { k1 => v1, v1 => undef, k2 => v2, v2 => undef, ... kn => vn, vn => undef } This can create some weird errors if the value of any of those keys is undef. This is also twice the size of the hash. And yes, this fix should be applied to the other line as well. Hope this clarifies, Katrin. Created attachment 26619 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Created attachment 26627 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26702 [details] [review] [PASSED QA] Bug 11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tested with different types of patron attributes: - repeatable - linked to an authorized value - free text Tested editing, adding, removing one of multiple, adding multiple, etc. No regressions found. Passes all tests and QA script. Created attachment 26703 [details] [review] [PASSED QA] Bug 11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tested with different types of patron attributes: - repeatable - linked to an authorized value - free text Tested editing, adding, removing one of multiple, adding multiple, etc. No regressions found. Passes all tests and QA script. Pushed to master. Thanks, Srdjan! *** Bug 11913 has been marked as a duplicate of this bug. *** Pushed to 3.14.x, will be in 3.14.10 Picked for 3.12.15 |