Bug 7933 - Fields in marc display not the same
Summary: Fields in marc display not the same
Status: RESOLVED DUPLICATE of bug 11592
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Kyle M Hall
URL: /cgi-bin/koha/opac-MARCdetail.pl?bibl...
Keywords: rel_3_16_candidate
Depends on:
Blocks:
 
Reported: 2012-04-09 17:09 UTC by Nicole C. Engard
Modified: 2014-10-29 19:07 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
labeled marc with no 090 (54.30 KB, image/png)
2012-04-09 17:09 UTC, Nicole C. Engard
Details
plain marc with 090 (43.60 KB, image/png)
2012-04-09 17:09 UTC, Nicole C. Engard
Details
Bug 7933 - Fields in marc display not the same (4.13 KB, patch)
2012-04-26 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Proposed patch (revisited) (22.36 KB, patch)
2012-11-27 16:50 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (rebased master) (22.34 KB, patch)
2013-03-18 11:11 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 7933: Fields in marc display not the same (22.26 KB, patch)
2013-06-13 08:35 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 7933: Unit test for C4::Biblio::RemoveHiddenSubfields (2.47 KB, patch)
2013-06-13 08:36 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7933: Fields in marc display not the same (22.38 KB, patch)
2013-09-08 15:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7933: Unit test for C4::Biblio::RemoveHiddenSubfields (2.57 KB, patch)
2013-09-08 15:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 7933: QA Followup - Remove unnecessary object creation (958 bytes, patch)
2013-09-12 13:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7933: Fields in marc display not the same (22.42 KB, patch)
2013-09-12 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7933: Unit test for C4::Biblio::RemoveHiddenSubfields (2.63 KB, patch)
2013-09-12 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7933: QA Followup - Remove unnecessary object creation (1013 bytes, patch)
2013-09-12 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-04-09 17:09:15 UTC
Created attachment 9045 [details]
labeled marc with no 090

If you click to view the marc display of a record in the OPAC it doesn't show all of your fields.

If you click 'view plain' you do see all the fields.  

If it's MARC view you should see all the filled in fields on both views, just with a slightly different view.

For example see the attached images - the 090 is there on view plain, but not on the labeled view.
Comment 1 Nicole C. Engard 2012-04-09 17:09:30 UTC
Created attachment 9046 [details]
plain marc with 090
Comment 2 Nicole C. Engard 2012-04-09 17:18:52 UTC
[13:13]  <jcamins> nengard: did you check your frameworks re: 7933?
[13:14]  <jcamins> Remember the "hidden" attribute only controls expanded MARC view and cataloging.
[13:14]  <nengard> my argument there is that it should control everything then
[13:14]  <nengard> it's confusing to the librarian
[13:14]  <jcamins> I agree it should.
[13:14]  <jcamins> But it doesn't.
[13:15]  <jcamins> So the bug isn't that 090 doesn't appear on the expanded MARC view. It's that the "hidden" attribute hasn't been taken out back and shot.
[13:16]  <sekjal> MARC definitions need to be separated from frameworks
[13:17]  <jcamins> That would be great.
[13:17]  <sekjal> hopefully structured in a way that would be as quick to load as possible
[13:18]  <sekjal> marc defs could be file-based; they don't really change all that much
Comment 3 Kyle M Hall 2012-04-25 15:42:32 UTC
Cannot reproduce the situation. It appears that in master, the labeled and plain views both display the same MARC fields ( whatever is defined in the record's current framework ).
Comment 4 Jared Camins-Esakov 2012-04-25 17:11:15 UTC
I just tested, and was able to reproduce the problem on latest master: http://screencast.com/t/BzcOItus
Comment 5 Kyle M Hall 2012-04-26 13:47:47 UTC Comment hidden (obsolete)
Comment 6 Ian Bays 2012-04-26 15:08:19 UTC
We narrowed it down to the tabs in the framework.  A number of our librarians request that commonly-used MARC tags appear on the tab 0 for editing.  We noticed a pattern that if a MARC tag is not allocated its "natural" tab then that tab will not show in the MARC [view labeled] display.
We could prove this by changing the tab for a subfield and making it appear and disappear.
There is a loop in opac-MARCdetail.pl which goes through the tabs:

# loop through each tab 0 through 9
for ( my $tabloop = 0 ; $tabloop <= 10 ; $tabloop++ ) {

(is this another bug as that loop goes through 10!).

Lower down is a condition:

next if ( ($sf_def->{tab}||'') ne $tabloop);

which if commented puts out all tags about 10 times.

Added another condition which refines the condition to allow the first character of the tag to match the tabloop and be allowed to display:

next if ( ($sf_def->{tab}||'') ne $tabloop && substr($fields[$x_i]->tag(),0,1) ne $tabloop);

This seems to do the trick.  I have no idea why opac-MARCdetail.pl would want to go through the display tabs when listing the MARC data for a bib record.  However until that is unravelled this should solve (at least) the problem identified here.
Comment 7 Jared Camins-Esakov 2012-04-27 02:47:41 UTC
I'm a little confused by the conversations going on in this bug. The issue reported in comment 6 seems to have to do with the labeled MARC view rather than the plain MARC view, and the proposed patch doesn't implement hidden in a maintainable way. In particular, it seems to take hidden as a boolean, and offers no method for specifying context, which is a rather major functional change (and step back, probably). I think a contextual filter is required, at least.
Comment 8 Ian Bays 2012-04-27 08:23:00 UTC
Comment 6 was in response to the initial (#1) bug.  The problems and (a) solution apply to code at version 3.06.02.003 Koha.  The code in comment 6 does not mention the hidden characteristic, but the existing code in opac-MARCdetail.pl does indeed do a "next" if hidden.  I'll leave it for others to debate the virtue of hiding MARC subfields in the two MARC display formats.  However the point I was trying to fix was that in labeled (should be labelled) MARC view it was not showing subfields if we had chosen to move the subfield to a different edit tab (eg move 245$a to tab 0 for convenience).
I am sure there are more elegant fixes than mine, but on basic testing the fix proposed works.  I could not see any reference in comment 6 to processing for hidden.
I see the time of Kyle's post (#5) is the same date as post #6 so if Kyle's post removes the problem I apologise for any confusion but I can only see reference to handling for "hidden" in other recent posts and no mention of tags being moved to different edit tabs failing to display.
Comment 9 Jared Camins-Esakov 2012-05-12 14:22:39 UTC
Marking this patch failed QA since it misapprehends the meaning of the "hidden" parameter.
Comment 10 Fridolin SOMERS 2012-11-26 14:44:52 UTC
Just created 9147 to hide in OPAC subfields in ISBD view like they are in MARC view.
It was trivial since framework informations where already used, but not "hidden".

Is this not a valid behavior ?

I have a request from a library that wants to *really* hide a note field at OPAC. This note contains important informations for librarian only. Are there no other libraries with this request ?
Comment 11 Fridolin SOMERS 2012-11-27 16:50:08 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-02-17 15:55:39 UTC
Hi, 
maybe I'm doing a wrong test or do not understand the purpose of the patch.

For a given record, MARC view don't show 999, plain view yes, and marcxml export yes too, and it's a hidden field. 
Are not supposed to be equal?

Also koha-qa complains a little:
* C4/Biblio.pm                                                       FAIL
        pod                         FAIL
                *** ERROR: Spurious text after =cut  in file C4/Biblio.pm
        forbidden patterns          OK
        valid                       OK
        critic                      OK
Comment 13 Fridolin SOMERS 2013-02-18 08:19:44 UTC
(In reply to comment #12)
> For a given record, MARC view don't show 999, plain view yes, and marcxml
> export yes too, and it's a hidden field. 
> Are not supposed to be equal?

Yes, this is the purpose of the patch.
Witch hidden value did you use ? Do you try with OPAC ?
Comment 14 Bernardo Gonzalez Kriegel 2013-02-18 15:08:44 UTC
My mistake, have wrong hidden values.
It works. I was using OPAC.
Comment 15 Bernardo Gonzalez Kriegel 2013-02-18 15:18:00 UTC
Now it works, I'm ready to sign.

But please, resubmit fixing pod error, or add a followup,
since it will not pass QA.
Comment 16 Owen Leonard 2013-03-14 13:45:31 UTC
(In reply to comment #15)
> But please, resubmit fixing pod error, or add a followup,
> since it will not pass QA.

I assume this means the status of this bug should be 'Failed QA'
Comment 17 Fridolin SOMERS 2013-03-18 11:11:23 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2013-04-05 09:44:12 UTC
Jared: I am wondering if we should look to this report as an enhancement rather than a mere bugfix at this point in time?
Comment 19 Jonathan Druart 2013-04-23 09:43:03 UTC
Unit tests are mandatory for new routines in C4 and Koha namespaces.
Marked as Failed QA.
Comment 20 Fridolin SOMERS 2013-06-12 16:05:35 UTC
(In reply to comment #19)
> Unit tests are mandatory for new routines in C4 and Koha namespaces.
Working on it
Comment 21 Fridolin SOMERS 2013-06-13 08:35:45 UTC Comment hidden (obsolete)
Comment 22 Fridolin SOMERS 2013-06-13 08:36:35 UTC Comment hidden (obsolete)
Comment 23 Bernardo Gonzalez Kriegel 2013-09-08 15:12:02 UTC Comment hidden (obsolete)
Comment 24 Bernardo Gonzalez Kriegel 2013-09-08 15:12:13 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-09-12 13:28:28 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-09-12 13:54:32 UTC
Created attachment 21039 [details] [review]
Bug 7933: Fields in marc display not the same

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. No koha-qa errors

Solved minor conflict on applying patch.

Test:
Edited framework and set a tag to hidden for OPAC
Correct behavior on both MARC views, exported record and cart

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2013-09-12 13:54:43 UTC
Created attachment 21040 [details] [review]
Bug 7933: Unit test for C4::Biblio::RemoveHiddenSubfields

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Run without errors. No koha-qa errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2013-09-12 13:54:55 UTC
Created attachment 21041 [details] [review]
Bug 7933: QA Followup - Remove unnecessary object creation

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Fridolin SOMERS 2014-01-16 17:04:31 UTC
Is there something blocking master integration ?
Comment 30 Galen Charlton 2014-05-05 22:46:28 UTC
I prefer the approach that Mark took in bug 11592 (the currently rebase issue aside) of creating a new GetFilteredOpacBiblio rather than changing GetMarcBiblio.
Comment 31 Fridolin SOMERS 2014-10-29 19:07:02 UTC
(In reply to Galen Charlton from comment #30)
> I prefer the approach that Mark took in bug 11592 (the currently rebase
> issue aside) of creating a new GetFilteredOpacBiblio rather than changing
> GetMarcBiblio.

Ok, bug 11592 is more recent also so I set this one has dublicate

*** This bug has been marked as a duplicate of bug 11592 ***