Description
Galen Charlton
2010-11-14 03:23:41 UTC
This bug is mentioned in: Bug 5404 Followup http://lists.koha-community.org/pipermail/koha-patches/2010-November/013024.html $ git grep subfield_is_koha_internal_p C4/Acquisition.pm:use C4::Koha qw( subfield_is_koha_internal_p ); C4/Acquisition.pm: next if ( subfield_is_koha_internal_p($subfield) ); C4/Items.pm: next if ( subfield_is_koha_internal_p($subfield) ); C4/Koha.pm: &subfield_is_koha_internal_p C4/Koha.pm:sub subfield_is_koha_internal_p { authorities/authorities-home.pl:use C4::Koha; # XXX subfield_is_koha_internal_p authorities/authorities.pl:use C4::Koha; # XXX subfield_is_koha_internal_p cataloguing/addbiblio.pl:use C4::Koha; # XXX subfield_is_koha_internal_p cataloguing/addbiblio.pl:use C4::Branch; # XXX subfield_is_koha_internal_p cataloguing/additem.pl:use C4::Koha; # XXX subfield_is_koha_internal_p cataloguing/additem.pl:use C4::Branch; # XXX subfield_is_koha_internal_p cataloguing/additem.pl: next if subfield_is_koha_internal_p($subfieldtag); cataloguing/additem.pl: next if subfield_is_koha_internal_p($subtag); labels/label-item-search.pl:use C4::Koha qw(GetItemTypes); # XXX subfield_is_koha_internal_p opac/opac-authorities-home.pl:use C4::Koha; # XXX subfield_is_koha_internal_p reports/guided_reports.pl:use C4::Branch; # XXX subfield_is_koha_internal_p tools/batchMod.pl:use C4::Koha; # XXX subfield_is_koha_internal_p tools/batchMod.pl:use C4::Branch; # XXX subfield_is_koha_internal_p tools/batchMod.pl: next if subfield_is_koha_internal_p($subfield); Created attachment 47968 [details] [review] Bug 5404: C4::Koha - remove subfield_is_koha_internal_p The commit b5ecefd485a75d54a5fa26fff5a0cc890541e2c3 Date: Mon Feb 3 18:46:00 2003 +0000 had a funny description: Added function to check if a MARC subfield name is "koha-internal" (instead of checking it for 'lib' and 'tag' everywhere); temporarily added to Koha.pm "Temporarily", since 2003, everything is relative, isn't it? :) The thing is that GetMarcStructure returns hash like field_200 => { subfield_a => { %attributes_of_subfield_a }, %attributes_of_field_200 } The attributes for field_200 can be 'repeatable', 'mandatory', 'tag', 'lib'. We don't want to loop on these values when looping on subfields. Since there are just { k => v } with v is a scalar (string), it's easier to test if we are processing a subfield testing the reference. At some places, we don't need to test that, we are looping on values from MARC::Field->subfields which are always valid subfields. Test plan: 1/ Edit items using the batch item mod tool 2/ display and edit items via the cataloguing module. You should not see any changes between before and after the patch applied. Tech notes: We need to check what we are processing when we loop on 'subfields' from GetMarcStructure, not from MARC::Field->subfields. Created attachment 48099 [details] [review] Bug 5404: C4::Koha - remove subfield_is_koha_internal_p The commit b5ecefd485a75d54a5fa26fff5a0cc890541e2c3 Date: Mon Feb 3 18:46:00 2003 +0000 had a funny description: Added function to check if a MARC subfield name is "koha-internal" (instead of checking it for 'lib' and 'tag' everywhere); temporarily added to Koha.pm "Temporarily", since 2003, everything is relative, isn't it? :) The thing is that GetMarcStructure returns hash like field_200 => { subfield_a => { %attributes_of_subfield_a }, %attributes_of_field_200 } The attributes for field_200 can be 'repeatable', 'mandatory', 'tag', 'lib'. We don't want to loop on these values when looping on subfields. Since there are just { k => v } with v is a scalar (string), it's easier to test if we are processing a subfield testing the reference. At some places, we don't need to test that, we are looping on values from MARC::Field->subfields which are always valid subfields. Test plan: 1/ Edit items using the batch item mod tool 2/ display and edit items via the cataloguing module. You should not see any changes between before and after the patch applied. Tech notes: We need to check what we are processing when we loop on 'subfields' from GetMarcStructure, not from MARC::Field->subfields. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> QA Comment: I am not sure if this really is a better solution. I also doubt about the remark in comment 1 "Apparently older versions of Koha considered a MARC subfield in the frameworks that had a code longer than one character to be reserved for internal use". To me it just seems to be a trick to skip the internal fields lib, tab, mandatory and repeatable (all length>1). In terms of design it would have been easier to put all subfields a level deeper instead of on the same level as lib, etc. I understand about not changing the design of the structure here. But now we move the test length<>1 from a subroutine into the code and check if it is a ref or not. I probably would like to still have the test in the subroutine (with all benefits of a subroutine); another name would be fine. If someone wants to add some other property to the structure and suppose that is an arrayref or a hashref or an object, your test would fail. I would propose to test (in the sub): [a] skip if the field name matches lib|tab|mandatory|repeatable or [b] proceed if the field is a ref called MARC::Field (instead of testing if ref is true, we test ref eq ..) Obviously, I agree with removing the call for a loop on only real subfields. Changing status (In reply to Marcel de Rooy from comment #5) > or [b] proceed if the field is a ref called MARC::Field (instead of testing > if ref is true, we test ref eq ..) Oops. That is not true. (In reply to Marcel de Rooy from comment #5) > QA Comment: > I am not sure if this really is a better solution. > I also doubt about the remark in comment 1 "Apparently older versions of > Koha considered a MARC subfield in the frameworks that had a code longer > than one character to be reserved for internal use". To me it just seems to > be a trick to skip the internal fields lib, tab, mandatory and repeatable > (all length>1). > > In terms of design it would have been easier to put all subfields a level > deeper instead of on the same level as lib, etc. > > I understand about not changing the design of the structure here. Yes, that would be a next step. > But now we move the test length<>1 from a subroutine into the code and check > if it is a ref or not. > I probably would like to still have the test in the subroutine (with all > benefits of a subroutine); another name would be fine. Not sure it's needed, as the test is very simple. Where would you move this subroutine? > If someone wants to add some other property to the structure and suppose > that is an arrayref or a hashref or an object, your test would fail. Yes but it works for the moment :) The structure has not changed for years. > I would propose to test (in the sub): > [a] skip if the field name matches lib|tab|mandatory|repeatable Imo, that would make the test less strong and too specific. (In reply to Jonathan Druart from comment #7) > Not sure it's needed, as the test is very simple. > Where would you move this subroutine? If we would ever have to change this very simple test, it would also be very easy to overlook a simple test. I would keep it close to GetMarcStructure. > Yes but it works for the moment :) The structure has not changed for years. True > Imo, that would make the test less strong and too specific. OK So, I prefer to move the ref-test to a subroutine in Biblio.pm IsMarcStructureInternal ? or something ? That would be the only place to document this test. Created attachment 48436 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub Same test plan as previous patch Josef: Could you please add your signoff on the follow-up (if possible..) ? This patch is on my QA list for Friday. I just tried the patch and I believe there is typo in C4/Acquisition.pm on line 2999: next if IsMarcStructureInternal(tagslib->{$tag}{$subfield}); there is missing $ before variable "tagslib" Beside that the patch is working, so I will be happy to add sign-off when this will be fixed. Could you do that Jonathan? Thanks Created attachment 48450 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub (In reply to Josef Moravec from comment #11) > Could you do that Jonathan? Done! Sorry about that, I have caught the error with qa tools but forgotten to amend the patch. Created attachment 48478 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Everything looks good, I'm signing off. Thanks Jonathan Created attachment 48673 [details] [review] Bug 5404: C4::Koha - remove subfield_is_koha_internal_p The commit b5ecefd485a75d54a5fa26fff5a0cc890541e2c3 Date: Mon Feb 3 18:46:00 2003 +0000 had a funny description: Added function to check if a MARC subfield name is "koha-internal" (instead of checking it for 'lib' and 'tag' everywhere); temporarily added to Koha.pm "Temporarily", since 2003, everything is relative, isn't it? :) The thing is that GetMarcStructure returns hash like field_200 => { subfield_a => { %attributes_of_subfield_a }, %attributes_of_field_200 } The attributes for field_200 can be 'repeatable', 'mandatory', 'tag', 'lib'. We don't want to loop on these values when looping on subfields. Since there are just { k => v } with v is a scalar (string), it's easier to test if we are processing a subfield testing the reference. At some places, we don't need to test that, we are looping on values from MARC::Field->subfields which are always valid subfields. Test plan: 1/ Edit items using the batch item mod tool 2/ display and edit items via the cataloguing module. You should not see any changes between before and after the patch applied. Tech notes: We need to check what we are processing when we loop on 'subfields' from GetMarcStructure, not from MARC::Field->subfields. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 48674 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 48675 [details] [review] Bug 5404: [QA Follow-up] Add test descriptions Adding descriptions for changes in t/db_dependent/Biblio.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #18) > Created attachment 48675 [details] [review] [review] > Bug 5404: [QA Follow-up] Add test descriptions > > Adding descriptions for changes in t/db_dependent/Biblio.t. Oops forgot that, thanks for the follow-up! Pushed to Master - Should be in the May 2016 release. Thanks! |