Bug 5404

Summary: C4::Koha::subfield_is_koha_internal_p no longer serves a purpose
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: brendan, jonathan.druart, josef.moravec, julian.maurice, katrin.fischer, m.de.rooy
Version: master   
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:
Bug Depends on:    
Bug Blocks: 15779    
Attachments: Bug 5404: C4::Koha - remove subfield_is_koha_internal_p
Bug 5404: C4::Koha - remove subfield_is_koha_internal_p
Bug 5404: Move the test to a new IsMarcStructureInternal sub
Bug 5404: Move the test to a new IsMarcStructureInternal sub
Bug 5404: Move the test to a new IsMarcStructureInternal sub
Bug 5404: C4::Koha - remove subfield_is_koha_internal_p
Bug 5404: Move the test to a new IsMarcStructureInternal sub
Bug 5404: [QA Follow-up] Add test descriptions

Description Galen Charlton 2010-11-14 03:23:41 UTC
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.  This no longer applies, so the routine and the last couple uses of it can be removed.
Comment 1 MJ Ray (software.coop) 2010-12-14 10:22:20 UTC
This bug is mentioned in:
Bug 5404 Followup http://lists.koha-community.org/pipermail/koha-patches/2010-November/013024.html
Comment 2 Katrin Fischer 2015-06-06 20:18:02 UTC
$ 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);
Comment 3 Jonathan Druart 2016-02-12 12:54:28 UTC Comment hidden (obsolete)
Comment 4 Josef Moravec 2016-02-16 09:46:03 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-02-19 08:58:47 UTC
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
Comment 6 Marcel de Rooy 2016-02-19 09:00:24 UTC
(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.
Comment 7 Jonathan Druart 2016-02-19 15:28:25 UTC
(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.
Comment 8 Marcel de Rooy 2016-02-22 10:36:18 UTC
(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.
Comment 9 Jonathan Druart 2016-02-29 10:06:01 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2016-02-29 11:31:30 UTC
Josef:
Could you please add your signoff on the follow-up (if possible..) ?
This patch is on my QA list for Friday.
Comment 11 Josef Moravec 2016-02-29 12:35:16 UTC
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
Comment 12 Jonathan Druart 2016-02-29 13:24:19 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-02-29 13:25:24 UTC
(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.
Comment 14 Josef Moravec 2016-02-29 21:21:05 UTC Comment hidden (obsolete)
Comment 15 Josef Moravec 2016-02-29 21:22:27 UTC
Everything looks good, I'm signing off. Thanks Jonathan
Comment 16 Marcel de Rooy 2016-03-04 11:00:32 UTC
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>
Comment 17 Marcel de Rooy 2016-03-04 11:00:37 UTC
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>
Comment 18 Marcel de Rooy 2016-03-04 11:00:41 UTC
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>
Comment 19 Jonathan Druart 2016-03-04 11:30:10 UTC
(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!
Comment 20 Brendan Gallagher 2016-03-07 17:35:29 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!