Bug 11184

Summary: Fix wrong attribute type cloning
Product: Koha Reporter: Srdjan Jankovic <srdjan>
Component: PatronsAssignee: 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
When building attributes list for the patron maintenance list, cloning is performed in suboptimal way that generates warnings.
Comment 1 Srdjan Jankovic 2013-11-01 08:27:38 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-11-01 08:28:31 UTC
Galen, can I ask you to scrutinise this one please.
Comment 3 Jonathan Druart 2014-01-15 16:31:49 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-01-19 11:06:40 UTC
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 };
Comment 5 Mark Tompsett 2014-03-24 04:31:08 UTC
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.
Comment 6 Srdjan Jankovic 2014-03-26 23:42:38 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-03-27 10:23:57 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2014-03-31 10:06:03 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2014-03-31 10:09:09 UTC
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.
Comment 10 Galen Charlton 2014-04-09 22:47:12 UTC
Pushed to master.  Thanks, Srdjan!
Comment 11 Galen Charlton 2014-04-09 22:54:35 UTC
*** Bug 11913 has been marked as a duplicate of this bug. ***
Comment 12 Fridolin Somers 2014-08-06 07:46:58 UTC
Pushed to 3.14.x, will be in 3.14.10
Comment 13 Kyle M Hall (khall) 2014-08-13 13:43:51 UTC
Picked for 3.12.15