It is possible to change the visibility in OPAC by affecting the value of hidden in marc_subfield_structure (Home->Administration->MARC frameworks->BKS Subfields -> ... -> Advanced Constraints -> Uncheck/Check OPAC). opac-MARCdetail.pl, for the most part, respects it, but if things normally displayed in opac-detail.pl are marked as not having visibility in OPAC (unchecked), they are still displayed.
Created attachment 24600 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. Lastly, opac-showmarc now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing.
Created attachment 24601 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. Lastly, opac-showmarc now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing.
Still to do: Add tests for new functions to C4/Biblio. :)
Created attachment 24611 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. Lastly, opac-showmarc now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing.
Created attachment 24629 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. Lastly, opac-showmarc now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing.
Created attachment 24654 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
Created attachment 24655 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
Comment on attachment 24655 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Review of attachment 24655 [details] [review]: ----------------------------------------------------------------- Will this also hide appropriate fields in opac-ISBDdetail that should be hidden? ::: C4/Biblio.pm @@ +1222,5 @@ > + > +C<$OpacHideMARC> is a ref to a hash which contains a series > +of key value pairs indicating if that field (key) is > +hidden (value == 1) or not (value == 0). > + How can this be used to get information about something that doesn't have a kohafield attached? e.g. I want to hide marc field 505 from the OPAC, but it doesn't have a corresponding Koha field. Would this still work? @@ +1230,5 @@ > + > +sub GetOpacHideMARC { > + my ( $frameworkcode ) = shift || ''; > + my $dbh = C4::Context->dbh; > + my $sth = $dbh->prepare("SELECT kohafield AS field,tagfield AS tag,hidden FROM marc_subfield_structure WHERE LENGTH(kohafield)>0 AND frameworkcode=? ORDER BY field,tagfield;"); I would suspect that doing WHERE kohafield <> '' might be a tiny bit faster than asking it to do a length calculation. Probably negligible though. I'd probably also fix the spacing as right now it looks like it groups wrongly in the columns that it's selecting, even though it doesn't. @@ +1238,5 @@ > + foreach my $fullfield (keys %{$data}) { > + my @tmpsplit = split(/\./,$fullfield); > + my $field = $tmpsplit[-1]; > + foreach my $tag (keys %{$data->{$fullfield}}) { > + if ($data->{$fullfield}->{$tag}->{'hidden'}>0) { !=0 is likely to be marginally faster (unless negatives are a thing you care about.) @@ +1270,5 @@ > + my $filtered_record = $record->clone; > + > + my ( $frameworkcode ) = shift || ''; > + > + my $marcsubfieldstructure = GetMarcStructure(0,$frameworkcode); Maybe allow the marcsubfieldstructure to be passed in instead of the framework code. This becomes important if this happens over and over, as it'll do a big bunch of database work each time, this makes things very slow when it could be cached outside and passed in. It should be easy enough to see if you have a scalar or a ref, and so whether you have a code or the structure. @@ +1274,5 @@ > + my $marcsubfieldstructure = GetMarcStructure(0,$frameworkcode); > + if ($marcsubfieldstructure->{'000'}->{'@'}->{hidden}>0) { > + # LDR field is excluded from $record->fields(). > + # if we hide it here, the MARCXML->MARC::Record->MARCXML transformation blows up. > + } This if doesn't actually do anything. @@ +1977,4 @@ > push @marcsubjects, { > MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop, > authoritylink => $authoritylink, > + } if $authoritylink || $#subfields_loop>=0; $#subfields_loop>=0 is a bit of an ugly construction. Best to use just @subfields_loop, it does the same thing and is easier to read. ::: opac/opac-detail.pl @@ +489,4 @@ > } > > my $dat = &GetBiblioData($biblionumber); > +my $OpacHideMARC = &GetOpacHideMARC($dat->{'frameworkcode'}); & is a perl4-ism, not required. @@ +654,5 @@ > +my ($st_tag,$st_subtag) = GetMarcFromKohaField('bibliosubtitle.subtitle',$dat->{'frameworkcode'}); > +my $subtitle; > +if ($st_tag && $st_subtag) { > + my @subtitles = $record->subfield($st_tag,$st_subtag); > + $subtitle = \@subtitles if scalar @subtitles; you don't need to say 'scalar' here. @@ +656,5 @@ > +if ($st_tag && $st_subtag) { > + my @subtitles = $record->subfield($st_tag,$st_subtag); > + $subtitle = \@subtitles if scalar @subtitles; > +} > +if ($subtitle && scalar @$subtitle) { nor here
> Will this also hide appropriate fields in opac-ISBDdetail that should be > hidden? I didn't even look at opac-ISBDdetail -- we don't use it. I suppose I should. Though, it has that syspref related to the output for it. I'll add a filter call. > > +C<$OpacHideMARC> is a ref to a hash which contains a series > > +of key value pairs indicating if that field (key) is > > +hidden (value == 1) or not (value == 0). > > + > > How can this be used to get information about something that doesn't have a > kohafield attached? e.g. I want to hide marc field 505 from the OPAC, but it > doesn't have a corresponding Koha field. Would this still work? OpacHideMARC is intended as a hack for the loop in opac-detail that creates template parameters based on koha field names. It was around like 699 in the master, 713 in the patch. This is how [% title %] and other parameters are created. This covers the half of the problem that doesn't use the MARC record directly. If you are trying to hide something that doesn't have a kohafield, you are looking at the other function: GetFilteredOpacBiblio. This takes the MARC record and strips out things marked to be hidden. > > + my $sth = $dbh->prepare("SELECT kohafield AS field,tagfield AS tag,hidden FROM marc_subfield_structure WHERE LENGTH(kohafield)>0 AND frameworkcode=? ORDER BY field,tagfield;"); > > I would suspect that doing WHERE kohafield <> '' might be a tiny bit faster > than asking it to do a length calculation. I didn't do testing, but LENGTH(NULL) = 0, right? Which way handles the NULL case better? -- Just checked >'' is used elsewhere and returns the same number on my data. > Probably negligible though. I'd > probably also fix the spacing as right now it looks like it groups wrongly > in the columns that it's selecting, even though it doesn't. I don't understand what you are trying to say. OH! field,tagfield lacks spacing. Will fix that to improve readability. > > + if ($data->{$fullfield}->{$tag}->{'hidden'}>0) { > > !=0 is likely to be marginally faster (unless negatives are a thing you care > about.) valid values for hidden range from -15 to +... anyways... <=0 OPAC visibility is checked. >0 OPAC visibility is unchecked. So, yes, care about negatives. > > @@ +1270,5 @@ > > + my $filtered_record = $record->clone; > > + > > + my ( $frameworkcode ) = shift || ''; > > + > > + my $marcsubfieldstructure = GetMarcStructure(0,$frameworkcode); > > Maybe allow the marcsubfieldstructure to be passed in instead of the > framework code. This becomes important if this happens over and over, as > it'll do a big bunch of database work each time, this makes things very slow > when it could be cached outside and passed in. > > It should be easy enough to see if you have a scalar or a ref, and so > whether you have a code or the structure. Hmm... GetMarcStructure is cached. Look in C4/Biblio.pm for "sub GetMarcStructure". You will see the $marc_structure_cache line just above that, and it being used at the top of the function. > @@ +1274,5 @@ > > + my $marcsubfieldstructure = GetMarcStructure(0,$frameworkcode); > > + if ($marcsubfieldstructure->{'000'}->{'@'}->{hidden}>0) { > > + # LDR field is excluded from $record->fields(). > > + # if we hide it here, the MARCXML->MARC::Record->MARCXML transformation blows up. > > + } > > This if doesn't actually do anything. It explains why I didn't hide the LDR record. As this function is called only once, I don't think it is a big deal, but I will comment it out so people don't get the idea to fix the remaining LDR field problem this way. > > @@ +1977,4 @@ > > push @marcsubjects, { > > MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop, > > authoritylink => $authoritylink, > > + } if $authoritylink || $#subfields_loop>=0; > > $#subfields_loop>=0 is a bit of an ugly construction. Best to use just > @subfields_loop, it does the same thing and is easier to read. Okay, I suppose I can try that. > > my $dat = &GetBiblioData($biblionumber); > > +my $OpacHideMARC = &GetOpacHideMARC($dat->{'frameworkcode'}); > > & is a perl4-ism, not required. Okay, I was confused a little by it used in some places and not in others. I'll remove my &. > > + $subtitle = \@subtitles if scalar @subtitles; > > you don't need to say 'scalar' here. > > +if ($subtitle && scalar @$subtitle) { > > nor here I'll test and remove, once confirmed.
(In reply to M. Tompsett from comment #9) > Hmm... GetMarcStructure is cached. Look in C4/Biblio.pm for "sub > GetMarcStructure". You will see the $marc_structure_cache line just above > that, and it being used at the top of the function. Ah, interesting. It's also cached in memcache which actually slows things down. This explains why.
Created attachment 24657 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
Created attachment 24658 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
Created attachment 24659 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
opac-search.pl is out of scope for now. opac-ISDBdetail.pl could be cleaned up more, but the focus of this was more consistency between opac-MARCdetail and opac-detail pages.
Created attachment 24660 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
Created attachment 24661 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl.
Created attachment 24662 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl.
Created attachment 24663 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework.
Created attachment 24664 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl.
Created attachment 24875 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
Created attachment 24876 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework. Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
(sorry, attached them the wrong way around due to expecting git bz to work like most git commands.)
Created attachment 24952 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24953 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Looking forward to this one going in. It would be a great help...
A few additional thoughts... 1) A follow-up should probably be added for a few other OPAC scripts like opac-basket.pl, opac-sendbasket.pl, opac-sendshelf.pl, etc., since these will all show the unfiltered MARC record. 2) It would be nice to see a future version of this that filters for both the OPAC and the staff client. Passing a "context" in and checking the 'hidden' values against a hashref of hashrefs might be a quick way of doing that: my $display = { opac => { 0 => 1, -1 => 1, -2 => 1, -3 => 1, -4 => 1, -5 => 1, -6 => 1, -7 => 1, }, intranet => { -6 => 1, -5 => 1, -1 => 1, 0 => 1, 1 => 1, 4 => 1, 6 => 1, 7 => 1, }, }; 3) I've added a few notes in Splinter Review for ways I think that the filter sub could be improved. Since this has already passed QA, I'll just leave them as notes. If it's too late to make changes, I'll probably send a patch with some of them after it's pushed in any case.
ARG! This no longer applies. And because the t/db_dependent/Biblio.t file has gotten more complicated that a straight list of tests, I'm not sure how to rebase.
Rebase need caused by bug 11912.
I think I have something working, but I need to get my unimarc VM updated and do a test on it first.
Created attachment 28002 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 28003 [details] [review] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Since bug 11912 forced Biblio.t to UNIMARC and MARC21 marcflavour the tests, I needed to tweak the test cases to properly handle UNIMARC vs. MARC21 testing. It would be good to see if someone that uses UNIMARC could signoff this bug. The test plan is the same as comment 31, but obviously 200$a is title under UNIMARC. My test cases ran as expected under UNIMARC and MARC21. Test both. :) Perhaps I should have removed the sign offs from the second patch?
(In reply to David Cook from comment #26) > A few additional thoughts... > > 1) > A follow-up should probably be added for a few other OPAC scripts like > opac-basket.pl, opac-sendbasket.pl, opac-sendshelf.pl, etc., since these > will all show the unfiltered MARC record. Ah, right. GetMarcBiblio. > 2) > It would be nice to see a future version of this that filters for both the > OPAC and the staff client. > > Passing a "context" in and checking the 'hidden' values against a hashref of > hashrefs might be a quick way of doing that... I am reconsidering these suggestions, since tweaking a lot of file is as much of a pain as testing the far reaching effects of modifying GetMarcBiblio.
And this is where converting the parameters of GetMarcBiblio to a hash reference would be useful, because optional parameters are a pain otherwise. *sigh*
Moving back to assigned, while I think and work on this a little more.
Created attachment 28477 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Next, opac-export and opac-showmarc were tweaked to use the new parameter calling convention to filter properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN ---------
Created attachment 28478 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Next, opac-export and opac-showmarc were tweaked to use the new parameter calling convention to filter properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility.
Created attachment 28479 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 28480 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display.
I think this is it.
Comment on attachment 28480 [details] [review] Bug 11592 - MARC Visibility settings not respected Review of attachment 28480 [details] [review]: ----------------------------------------------------------------- Overall, I really want to see this patch get into Koha. However, I think there is some unnecessary code, some potentially incorrect code, and some code that probably belongs in a different bug/patch. ::: C4/Biblio.pm @@ +1322,5 @@ > + my $display = ShouldDisplayOnInterface(); > + > + my $dbh = C4::Context->dbh; > + my $sth = $dbh->prepare("SELECT kohafield AS field, tagfield AS tag, hidden FROM marc_subfield_structure WHERE kohafield>'' AND frameworkcode=? ORDER BY field, tagfield;"); > + my $rv = $sth->execute($frameworkcode); Why is there a "my $rv" here? It shouldn't be needed. @@ +1331,5 @@ > + my @tmpsplit = split(/\./,$fullfield); > + my $field = $tmpsplit[-1]; > + foreach my $tag (keys %{$data->{$fullfield}}) { > + my $show = $display->{$interface}->{ $data->{$fullfield}->{$tag}->{'hidden'} }; > + if (!$show || $show==0) { $show should always be 1 or undefined, so I wouldn't think $show==0 would be necessary @@ +1334,5 @@ > + my $show = $display->{$interface}->{ $data->{$fullfield}->{$tag}->{'hidden'} }; > + if (!$show || $show==0) { > + $shouldhidemarc{ $field } = 1; > + } > + elsif ( !exists($shouldhidemarc{ $field }) ) { It shouldn't be necessary to add fields that aren't hidden to this hash. Plus, since the value is 0, you'll most likely get the same end result you would if you didn't have this block of code. @@ +1493,5 @@ > + # LDR field is excluded from $record->fields(). > + # if we hide it here, the MARCXML->MARC::Record->MARCXML > + # transformation blows up. > + #} > + foreach my $fields ($record->fields()) { Should probably use $field rather than $fields, as this incorrect pluralization gets confusing lower down. @@ +1495,5 @@ > + # transformation blows up. > + #} > + foreach my $fields ($record->fields()) { > + my $tag = $fields->tag(); > + my $hidden; This $hidden variable isn't really necessary. If it should be hidden, you should just hide/delete it upon detection. The code looks like it would work either way, but it would be easier to read with less needless code. @@ +1500,5 @@ > + if ($tag>=10) { > + foreach my $subpairs ($fields->subfields()) { > + my ($subtag,$value) = @$subpairs; > + my $visibility = $marcsubfieldstructure->{$tag}->{$subtag}->{hidden}; > + $visibility //= 0; Is there are a reason to have this extra line instead of "my $visibility = $marcsubfieldstructure->{$tag}->{$subtag}->{hidden} // 0"? @@ +1670,4 @@ > $oauthors .= "&rft.au=$au"; > } > } > + $title = $record->subfield( '245', 'a' ) // ''; Why is this in this patch? @@ +2122,4 @@ > push @marcsubjects, { > MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop, > authoritylink => $authoritylink, > + } if $authoritylink || @subfields_loop; Why is this in this patch? ::: C4/XSLT.pm @@ +93,5 @@ > + @new_subfields > + ) ); > + } > + else { > + $record->delete_fields($field); What is this doing here, and why is it in this patch? ::: opac/opac-detail.pl @@ +683,4 @@ > my $marcseriesarray = GetMarcSeries ($record,$marcflavour); > my $marcurlsarray = GetMarcUrls ($record,$marcflavour); > my $marchostsarray = GetMarcHosts($record,$marcflavour); > +my ($st_tag,$st_subtag) = GetMarcFromKohaField('bibliosubtitle.subtitle',$dat->{'frameworkcode'}); I think that this section is a mistake. What is bibliosubtitle.subtitle? Also, why would this be in this patch? ::: opac/opac-showmarc.pl @@ +55,4 @@ > > if ($view eq 'card' || $view eq 'html') { > my $xmlrecord= $importid? $record->as_xml(): GetXmlBiblio($biblionumber); > + if (!$importid && $view eq 'html') { I haven't looked at the context, but why not filter for all cases here?
(In reply to David Cook from comment #41) > ::: C4/Biblio.pm > > + my $rv = $sth->execute($frameworkcode); > > Why is there a "my $rv" here? It shouldn't be needed. It was debugging code that I forgot to strip. :) > > + if (!$show || $show==0) { > > $show should always be 1 or undefined, so I wouldn't think $show==0 > would be necessary. Actually, 1, 0, or undefined. Someone could put => 0 into the hash checked. I'll confirm if I can simplify the logic. > @@ +1334,5 @@ > > + my $show = $display->{$interface}->{ $data->{$fullfield}->{$tag}->{'hidden'} }; > > + if (!$show || $show==0) { > > + $shouldhidemarc{ $field } = 1; > > + } > > + elsif ( !exists($shouldhidemarc{ $field }) ) { > > It shouldn't be necessary to add fields that aren't hidden to this hash. > Plus, since the value is 0, you'll most likely get the same end result you > would if you didn't have this block of code. I don't like having undefined values. Those tend to generate annoying floody errors when some comparison works but was expecting a number, for example. > @@ +1493,5 @@ > > + # LDR field is excluded from $record->fields(). > > + # if we hide it here, the MARCXML->MARC::Record->MARCXML > > + # transformation blows up. > > + #} > > + foreach my $fields ($record->fields()) { > > Should probably use $field rather than $fields, as this incorrect > pluralization gets confusing lower down. Okay, that makes sense. > @@ +1495,5 @@ > > + # transformation blows up. > > + #} > > + foreach my $fields ($record->fields()) { > > + my $tag = $fields->tag(); > > + my $hidden; > > This $hidden variable isn't really necessary. If it should be hidden, you > should just hide/delete it upon detection. The code looks like it would work > either way, but it would be easier to read with less needless code. If you look at the hidden value setting that would be inside an if/else. I scoped it wider, so there was no need to have two lines of code. Fine, two lines of code it is. > > + my $visibility = $marcsubfieldstructure->{$tag}->{$subtag}->{hidden}; > > + $visibility //= 0; > > Is there are a reason to have this extra line instead of "my $visibility = > $marcsubfieldstructure->{$tag}->{$subtag}->{hidden} // 0"? Yes, because otherwise visibility is undef. It should be a valid number (-7 to +7) reflecting valid values set in the Advanced Constraints screen. > @@ +1670,4 @@ > > $oauthors .= "&rft.au=$au"; > > } > > } > > + $title = $record->subfield( '245', 'a' ) // ''; > > Why is this in this patch? This fixes a floody error I had during testing. I suppose it could be another patch. I'll confirm. > @@ +2122,4 @@ > > push @marcsubjects, { > > MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop, > > authoritylink => $authoritylink, > > + } if $authoritylink || @subfields_loop; > > Why is this in this patch? This properly hides the subjects. > ::: C4/XSLT.pm > @@ +93,5 @@ > > + @new_subfields > > + ) ); > > + } > > + else { > > + $record->delete_fields($field); > > What is this doing here, and why is it in this patch? If you try to delete the last subfield of a field, you need to delete the field. If I recall, deleting the last subfield fails. Fields/subfields are deleted (from memory) to hide them from being displayed. > ::: opac/opac-detail.pl > @@ +683,4 @@ > > my $marcseriesarray = GetMarcSeries ($record,$marcflavour); > > my $marcurlsarray = GetMarcUrls ($record,$marcflavour); > > my $marchostsarray = GetMarcHosts($record,$marcflavour); > > +my ($st_tag,$st_subtag) = GetMarcFromKohaField('bibliosubtitle.subtitle',$dat->{'frameworkcode'}); > > I think that this section is a mistake. GetRecordValue, which is what was there initially, does not grab values from marc_subfield_structure, but rather fieldmapping. This means the subtitle is not properly hidden. > What is bibliosubtitle.subtitle? Also, why would this be in this patch? The value in the marc_subfield_structure table. Subtitles are a mess in Koha. > ::: opac/opac-showmarc.pl > @@ +55,4 @@ > > > > if ($view eq 'card' || $view eq 'html') { > > my $xmlrecord= $importid? $record->as_xml(): GetXmlBiblio($biblionumber); > > + if (!$importid && $view eq 'html') { > > I haven't looked at the context, but why not filter for all cases here? I think this is related to the save as exports. This is what worked for me.
(In reply to David Cook from comment #41) > @@ +1334,5 @@ > > + my $show = $display->{$interface}->{ $data->{$fullfield}->{$tag}->{'hidden'} }; > > + if (!$show || $show==0) { > > + $shouldhidemarc{ $field } = 1; > > + } > > + elsif ( !exists($shouldhidemarc{ $field }) ) { > > It shouldn't be necessary to add fields that aren't hidden to this hash. > Plus, since the value is 0, you'll most likely get the same end result you > would if you didn't have this block of code. > SELECT tagfield,tagsubfield,kohafield FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield IN ('541','952') ORDER BY kohafield; items.stocknumber is a pain. This is why this code exists. If the first one is hidden (I didn't sort the keys, so it is "random") or visible, the other value doesn't affect it.
Created attachment 28724 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display.
I changed the title of be 'opac detail scripts', because there are still various OPAC pages this patch doesn't fix. However, these patches provide a way to correct those other problems. I'm hoping others will have time to fix them.
The second patch has conflicts in the 2 test files, if you fix them up I'll sign off
Created attachment 31356 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display.
Created attachment 32382 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32383 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display.
*** Bug 7933 has been marked as a duplicate of this bug. ***
The patches currently don't apply for me: Apply? [(y)es, (n)o, (i)nteractive] i Applying: Bug 11592 - MARC Visibility settings not respected fatal: sha1 information is lacking or useless (C4/Biblio.pm).
The removal of the prog theme broke this, specifically bug 13170. Bug 11912 and other related bugs which touched t/db_dependent/Biblio.t affected this as well. I'm working on a rebase.
(In reply to M. Tompsett from comment #52) > Bug 11912 and other related bugs which touched > t/db_dependent/Biblio.t affected this as well. > I'm working on a rebase. Oops. Bug 12570, because it was t/Biblio.t :)
Created attachment 33356 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 33357 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 33358 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display.
Created attachment 33689 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display.
Created attachment 35855 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> TOOK me awhile but it works
Created attachment 35856 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 35857 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 35858 [details] [review] Bug 11592 - MARC Visibility settings not respected The advanced constraints section while editing subfield structure for a particular MARC bibliographic framework allow the user to set visibility settings for OPAC and Intranet. These settings are not fully respected. This was first discovered while comparing the information displayed on the opac-MARCdetail and opac-detail pages. To this end, this patch will provide the ability to correct this problem across the board as desired, but will focus primarily on the OPAC interface. This was accomplished by adding 3 functions to C4/Biblio.pm: - ShouldDisplayOnInterface - ShouldHideMARC - GetFilteredBiblio And also by modifying GetMarcBiblio to filter or not based on parameter style and parameter values passed. ShouldDisplayOnInterface is a wrapper routine which just returns a hash of hashes which let us know if something is visible (1) or not (0 or undef) for a given interface (opac or intranet). This allows us to add/remove to the advance constraints section and then not have to update multiple routines to keep the filtering working properly, though if it is a drastic refactoring that would be necessary. ShouldHideMARC takes a field from marc_subfield_structure, and determines if it should be hidden. It does this for all the fields for a given framework code on a particular interface. If no framework code is given, the framework code used is the default one. If not interface is specified, OPAC is assumed. This routine returns a hash reference to the corresponding hash that was built. GetFilteredBiblio takes a MARC::Record, clones it, and then strips all the fields that should not be visible for a given framework code and interface. If no framework code is specied, it assumes the default one. If no interface is given, OPAC is assumed. The routine then returns the filtered record. GetMarcBiblio was modified to accept a hash style parameter. This allowed for filtering if the new convention was used, and no apparent change if the old convention is used. These functions allowed for filtering, but frequently there were problems with titles, subjects, etc. This required other tweaks. In C4/Biblio.pm, GetMarcSubjects was returning the subject, even it was supposed to be hidden. C4/XSLT.pm tried to replace a field with no field, rather than properly delete it. With the filtering being implemented based on the parameter style, it just required a minor tweak in opac-ISDBdetail to filter. The title needed to be purposefully not set if it was supposed to be hidden in opac-MARCdetail. However, opac-detail was interesting challenge. First, changing the parameter style did filtering, but the output still was leaking things which should be hidden. This is the main reason ShouldHideMARC was written, because there is an insidious little loop that runs through a set of keys and sets template parameters to values. Second, By skipping to the next key based on the hash ref returned by ShouldHideMARC, the template parameter is not set, and thus the value is properly hidden. Third, the subtitle was grabbed by GetRecordValue. This was refactored so filtering worked properly. Then opac-MARCdetail's 'view plain' was still showing details. This required converting the XML record to a MARC::Record, filtering it, and then converting it back. By tweaking opac-export and opac-showmarc to use the new parameter calling convention, records were filtered properly. And of course, with modifications and additions to C4/Biblio.pm, tests were required. t/Biblio.t has been modified to check the new function ShouldDisplayOnInterface. t/db_dependent/Biblio.t has also been modified. It confirms that both old and new calling conventions work. It also tests hiding the title field (200$a UNIMARC, 245$a MARC). It includes tests for ShouldHideMARC, GetFilteredBiblio, and GetMarcBiblio. TEST PLAN --------- 1) Back up your DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be mostly hidden as before until you... 13) Click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) perldoc C4::Biblio -- Look for ShouldHideMARC and confirm that the perldoc entries are okay. 17) prove -v t/Biblio.t -- 'Visibility values confirmed.' test should pass. 18) prove -v t/db_dependent/Biblio.t -- tests 26 through 41 should pass for UNIMARC and MARC21. It would be good to test this patch on both types of systems. 19) Refresh the opac-MARCdetail page. Title should hide now. 20) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 21) Click Normal view 22) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 23) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 24) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields do hide in 'MARC view', but not normal view. This patch does not deal with 952 fields. 25) Attempt to 'Save record' in a variety of types. -- Only unhidden fields should display. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Be care, prototype of GetMarcBiblio is modified here, bug 12252 did it too!
Comment on attachment 35858 [details] [review] Bug 11592 - MARC Visibility settings not respected Review of attachment 35858 [details] [review]: ----------------------------------------------------------------- ::: C4/Biblio.pm @@ +1369,5 @@ > =cut > > sub GetMarcBiblio { > + my @catch_parameters = @_; > + Passing a hashref would be better.
(In reply to Jonathan Druart from comment #62) > Be care, prototype of GetMarcBiblio is modified here, bug 12252 did it too! This change catches the old way and a hashref. Though, because a third parameter was added in 12252, this would require a more detailed look there. However, that would more likely come anyways if that is pushed to master first anyways, because this would need a rebase. Thanks for the heads up.
(In reply to M. Tompsett from comment #64) > (In reply to Jonathan Druart from comment #62) > > Be care, prototype of GetMarcBiblio is modified here, bug 12252 did it too! > > This change catches the old way and a hashref. Though, because a third > parameter was added in 12252, this would require a more detailed look there. > However, that would more likely come anyways if that is pushed to master > first anyways, because this would need a rebase. Thanks for the heads up. I would consider this bugfix if it was implemented as a Koha::RecordProcessor filter for MARC::Record objects, rather than just another hook to C4::Search. That way we could use it wherever we need it. BUT I have to say that I've been playing with an implementation that works with MARCMXL instead (using XSLT) and could help us simplicy results rendering and probably performance. I'm trying to finish the patches (the hackfest had me busy with other stuff) so I can attach them for peer review.
(In reply to Tomás Cohen Arazi from comment #65) > I would consider this bugfix if it was implemented as a > Koha::RecordProcessor filter for MARC::Record objects, rather than just > another hook to C4::Search. That way we could use it wherever we need it. This does NOT touch C4::Search at all. My initial worry is the whole inclusion loop making debugging a pain! But your idea of effectively moving all the stuff added to C4::Biblio into a filter module is good, if there isn't some magical loop which makes debugging a pain. > BUT I have to say that I've been playing with an implementation that works > with MARCMXL instead (using XSLT) and could help us simplicy results > rendering and probably performance. > > I'm trying to finish the patches (the hackfest had me busy with other stuff) > so I can attach them for peer review. I was going to attempt to go that way. My concern is if we have MARCXML filters and non-XML filters, we need to make sure they are identical. I suppose part of the unit tests to add into the continuous integration. :) This is why I was wondering if perhaps we would go completely MARCXML internally, as I vaguely recall some indexing limit which you hit for a MARC Record, but perhaps that is unrelated.
tcohen: could you please update your comments on bug 11592 as related to bug 12252
Some comments from IRC: [16:32] marcelr so i was curious about your position before starting qa [16:32] tcohen my decision about that kind of stuff is that we should definitely resurrect Koha::RecordProcessor [16:33] tcohen so, Mark should implement (if that's what he wants) a Koha::Filter for MARC records [16:33] marcelr which would mean: rewrite the patch set first [16:33] tcohen I wrote my own for MARCXML (in the form of XML::LibXML::Document objects, which is the result of parsing XML) [16:33] tcohen so, I already have an implementation [16:34] tcohen that "can be used" wherever we want [16:34] tcohen the problem with MArk's is that it is MARC::Record specific, and is hooked in the wrong place (IMHO) [16:34] tcohen it can be discussed [16:34] marcelr where is yours now? [16:35] tcohen https://github.com/tomascohen/koha/tree/xslt_filtering [16:35] tcohen I introduced a Koha::Filter::MARCXML::ViewPolicy filter class Returning to this later..
(In reply to M. de Rooy from comment #68) > Some comments from IRC: > [16:33] tcohen so, Mark should implement (if that's what he wants) a > Koha::Filter for MARC records Which is something I will get around to doing.
I increased the severity of this big should be increased, as it's a big security problem
What's next here? Is it ready to QA? Looking at the last comments I'd say no.
I have yet to implement a filter. This would be a huge rewrite. However, some of the things in this patch could be split off to tinier bugs.
Progress is being made on this bug. I have a filter. Now to break the mess into parts, and assemble in small testable pieces while tweaking other code to use it.
Created attachment 47993 [details] [review] Bug 11592 - Old Filtery stuff
Created attachment 47996 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 47997 [details] [review] Bug 11592 - Old Filtery stuff
Created attachment 48001 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment on attachment 47997 [details] [review] Bug 11592 - Old Filtery stuff Bug 15870 and bug 15777 contain the equivalent code.
Created attachment 49288 [details] [review] Add ShouldHideMARC to ViewPolicy filter
Created attachment 49289 [details] [review] Applying filtering to opac interface. Applying the filtering and then... Debugging opac/opac-detail.pl filtering Debugging opac/opac-ISBDdetail.pl more Debugging opac/opac-export.pl Tweak opac/opac-export.pl fix variable declarations, conditional assignments Debugging opac/opac-showmarc.pl
Created attachment 49290 [details] [review] Biblio tweak for MARCSUBJECT
Created attachment 49291 [details] [review] Tweak C4/XSLT to delete field when no subfields left.
Applying bug 15870 and then the last 5 patches above, should give something testable. However, as this is still kind of being worked on, I thought I'd just share this to get feedback on whether the ShouldHideMARC should be on bug 15870 (which has not passed QA), or whether it could be here. Problem outstanding XML filtering. Any word on a bugzilla report for that, Tomas? I recall you mentioning some sample test code somewhere, but I forget the URL.
Comment on attachment 35858 [details] [review] Bug 11592 - MARC Visibility settings not respected Except for some lack of MARCXML filtering, this should mostly be replaced by the other patches added here.
See the test plan in comment #61. Known failings: MARCXML stuff. I've toggled this out of "In Discussion" as I think it is best to get something into Master (MARC filtering) rather than nothing. I'll fully test later, and set to Needs Signoff after I'm satisfied.
(In reply to M. Tompsett from comment #79) > Created attachment 49288 [details] [review] [review] > Add ShouldHideMARC to ViewPolicy filter I think that function is a mistake! Should be using C4::Biblio::GetMarcStructure instead. C4::Biblio::GetMarcStructure is cached already, and using yours will only make performance worse.
(In reply to Tomás Cohen Arazi from comment #86) > I think that function [ShouldHideMARC] is a mistake! Should be using > C4::Biblio::GetMarcStructure instead. C4::Biblio::GetMarcStructure is cached > already, and using yours will only make performance worse. Granted, another SQL query is a performance hit. However, the result is not keyed on kohafield. I'll post a revision that uses GetMarcStructure, as per your suggestion.
Created attachment 49612 [details] [review] Bug 11592 - Add should_hide_marc method to filter Add should_hide_marc to ViewPolicy filter Add should_hide_marc tests to t/db_dependent/Filter_MARC_ViewPolicy.t
Created attachment 49613 [details] [review] Applying filtering to opac interface. Applying the filtering and then... Debugging opac/opac-detail.pl filtering Debugging opac/opac-ISBDdetail.pl more Debugging opac/opac-export.pl Tweak opac/opac-export.pl fix variable declarations, conditional assignments Debugging opac/opac-showmarc.pl
Created attachment 49614 [details] [review] Biblio tweak for MARCSUBJECT
Created attachment 49615 [details] [review] Tweak C4/XSLT to delete field when no subfields left.
See the test plan in comment #61. Known failings: MARCXML stuff. Known failings: LEADER fields. Desired extensions beyond the scope of this particular bug: Authorized Values. I still have to fully test. But this is provided so as to see whether it addresses comment #86 sufficiently.
This test plan supercedes comment #61. Just one tiny piece missing for 100% code change testing. Known failings: MARCXML stuff. Known failings: LEADER fields. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) Still trying to remember how I triggered the XSLT change.
Okay, that last patch requires bad data to trigger it as far as I can tell. I did, however, notice another problem in C4/XSLT, that I'll make a separate bug for that.
Comment on attachment 49615 [details] [review] Tweak C4/XSLT to delete field when no subfields left. Can only be triggered by bad data, as far as I can tell. This change is not required.
See the test plan in comment #93.
Testing, I see a huge change in the 'Normal view' but little to no change in either the MARC or ISBD view Export bibtex or DublinCore are both almost empty after patch. Tests pass. If I understood the test plan I think I should see nothing/next to nothing in the MARC/ISBD views but I still see most of the record.
(In reply to Nick Clemens from comment #97) > Testing, I see a huge change in the 'Normal view' but little to no change in > either the MARC or ISBD view How did you test? Did you do: UPDATE marc_subfield_structure set hidden=-8; Because if you only do one field at a time, obviously, only that field/subfield will disappear. > If I understood the test plan I think I should see nothing/next to nothing > in the MARC/ISBD views but I still see most of the record. Yes, exactly. Screen shots. Because it works fine for me. I'll post mine shortly.
Created attachment 50484 [details] Normal view after patch and db update
Created attachment 50485 [details] marc virew after db and patch
Created attachment 50486 [details] isbd after db and patch
I think some ISBD bug which may have gotten pushed recently caused a bit of fun here. The GetISBDView function needs massive reworking. I believe I have the MARCView working.
Created attachment 50489 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. Test plan to follow later.
Comment on attachment 50484 [details] Normal view after patch and db update confirmed.
Comment on attachment 50485 [details] marc virew after db and patch Confirmed and patched.
Comment on attachment 50486 [details] isbd after db and patch Confirmed and patched. This led to a GetISBDView refactor. It also led to the discovery of leakage in the carts and lists. Lists are mostly dealt with. Carts partially, but may not be noticeable.
Comment on attachment 50489 [details] [review] Bug 11592: MARCView and ISBD followup Review of attachment 50489 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-ISBDdetail.pl @@ +92,4 @@ > } > } > > +my $record_unfiltered = GetMarcBiblio($biblionumber,1); This is because GetMarcBiblio was previously called in C4::Biblio::GetISBDView with ,1 which includes embedded items. ::: opac/opac-MARCdetail.pl @@ +108,5 @@ > my ($bt_tag,$bt_subtag) = GetMarcFromKohaField('biblio.title',$itemtype); > $template->param( > bibliotitle => $biblio->{title}, > +) if $tagslib->{$bt_tag}->{$bt_subtag}->{hidden} <= 0 && # <=0 OPAC visible. > + $tagslib->{$bt_tag}->{$bt_subtag}->{hidden} > -8; # except -8; Forgot the -8 (flagged) case.
Created attachment 50542 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files.
Created attachment 50550 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 50551 [details] [review] Bug 11592 - Add should_hide_marc method to filter Add should_hide_marc to ViewPolicy filter Add should_hide_marc tests to t/db_dependent/Filter_MARC_ViewPolicy.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 50552 [details] [review] Applying filtering to opac interface. Applying the filtering and then... Debugging opac/opac-detail.pl filtering Debugging opac/opac-ISBDdetail.pl more Debugging opac/opac-export.pl Tweak opac/opac-export.pl fix variable declarations, conditional assignments Debugging opac/opac-showmarc.pl https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 50553 [details] [review] Biblio tweak for MARCSUBJECT https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 50554 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I am sorry, this patch no longer applies: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11592 - Updated License Text and use Modern::Perl Using index info to reconstruct a base tree... M C4/Biblio.pm M C4/XSLT.pm M opac/opac-detail.pl Falling back to patching base and 3-way merge... Auto-merging C4/XSLT.pm Auto-merging C4/Biblio.pm Applying: Bug 11592 - Add should_hide_marc method to filter Applying: Applying filtering to opac interface. Applying: Biblio tweak for MARCSUBJECT Applying: Bug 11592: MARCView and ISBD followup fatal: sha1 information is lacking or useless (C4/Biblio.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
The OPACISBD variable bug caused this. I'll work on a rebase.
Created attachment 51313 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 51314 [details] [review] Bug 11592 - Add should_hide_marc method to filter Add should_hide_marc to ViewPolicy filter Add should_hide_marc tests to t/db_dependent/Filter_MARC_ViewPolicy.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 51315 [details] [review] Applying filtering to opac interface. Applying the filtering and then... Debugging opac/opac-detail.pl filtering Debugging opac/opac-ISBDdetail.pl more Debugging opac/opac-export.pl Tweak opac/opac-export.pl fix variable declarations, conditional assignments Debugging opac/opac-showmarc.pl https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 51316 [details] [review] Biblio tweak for MARCSUBJECT https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 51317 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 51318 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 51326 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Mark, I'm sorry but could you please rebase this?
Created attachment 53440 [details] [review] Bug 11592 - Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 53441 [details] [review] Bug 11592 - Add should_hide_marc method to filter Add should_hide_marc to ViewPolicy filter Add should_hide_marc tests to t/db_dependent/Filter_MARC_ViewPolicy.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 53442 [details] [review] Applying filtering to opac interface. Applying the filtering and then... Debugging opac/opac-detail.pl filtering Debugging opac/opac-ISBDdetail.pl more Debugging opac/opac-export.pl Tweak opac/opac-export.pl fix variable declarations, conditional assignments Debugging opac/opac-showmarc.pl https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 53443 [details] [review] Biblio tweak for MARCSUBJECT https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 53444 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Hi Mark, looking at https://wiki.koha-community.org/wiki/Hidden_values: -8 is described as "flag" - which noone knows exactly what it means I think. For "Everything hidden everywhere" I would have picked 8 or 5. Can you explain your choice of -8?
(In reply to Katrin Fischer from comment #129) > Hi Mark, > looking at https://wiki.koha-community.org/wiki/Hidden_values: > -8 is described as "flag" - which noone knows exactly what it means I think. > For "Everything hidden everywhere" I would have picked 8 or 5. > Can you explain your choice of -8? Because even though it is <0 which is typically visible in OPAC, -8 is NOT visible in OPAC. There is logic which uses <0 in a lot of places and a -8 is bound to catch it if you are looking around and notice it isn't there.
(In reply to Katrin Fischer from comment #129) > Can you explain your choice of -8? opac-MARCDetail.pl on the last patch.
Will continue with this in the next days.
Created attachment 55040 [details] [review] Bug 11592: Updated License Text and use Modern::Perl Why not clean up the License Agreement stuff while the files are being changed? Used the current one found at: http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence Changed the strict and warning lines into just a Modern::Perl. Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 55041 [details] [review] Bug 11592: Add should_hide_marc method to filter Add should_hide_marc to ViewPolicy filter Add should_hide_marc tests to t/db_dependent/Filter_MARC_ViewPolicy.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 55042 [details] [review] Bug 11592: Applying filtering to opac interface. Applying the filtering and then... Debugging opac/opac-detail.pl filtering Debugging opac/opac-ISBDdetail.pl more Debugging opac/opac-export.pl Tweak opac/opac-export.pl fix variable declarations, conditional assignments Debugging opac/opac-showmarc.pl https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 55043 [details] [review] Bug 11592: Biblio tweak for MARCSUBJECT https://bugs.koha-community.org/show_bug.cgi?id=11592 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 55044 [details] [review] Bug 11592: MARCView and ISBD followup There are still some leaks, but it is not as a result of the filter, but rather a result of poorly written template files. Bug fixing template files is beyond the scope of this set of patches. TEST PLAN --------- 1) Backup your DB 2) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=-8; -- this should set EVERYTHING to hidden across the board. 3) In staff client, set OPACXSLTDetailsDisplay to blank 4) In OPAC, view any detail. -- Normal view may mostly leak values still. -- MARC view may leak values. -- ISBD view may leak values. 5) In staff client, set OPACXSLTDetailsDisplay to default 6) In OPAC, view any detail. -- same issues as step 4 -- 'View Plain' may leak too. 7) 'Save record' -> 'Dublin Core' 8) Apply this patch 9) run koha qa test tools -- should be fine 10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t -- should pass -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too 11) In OPAC, view any detail. -- Normal view: -- Material type comes from the LEADER field. -- Lists this is on will still display -- 'Tags from this library' will still display -- Item information in table will still display (THIS IS BEYOND SCOPE) -- MARC view: -- Record number is leaked (THIS IS BEYOND SCOPE) -- 'View plain' leaks LEADER field. -- ISBD view may leak field headings, but not values. (THIS IS BEYOND SCOPE) 12) In staff client, set OPACXSLTDetailsDisplay to blank 13) In OPAC, view any detail. -- same kind of output as step 10 14) 'Save record' -> BIBTEXT -- Should be next to nothing leaked. 15) 'Save record' -> Dublin Core -- Should be the same or less leaked between the two versions. -- (XML FILTERING IS BEYOND SCOPE) 16) In the staff client, go view the same record. -- it should be mostly hidden in ISBD View. 17) run the following SQL on your DB. > UPDATE marc_subfield_structure set hidden=1; -- this should set EVERYTHING to hidden in OPAC, but not the STAFF across the board. 18) Refresh the staff ISBD page -- values should reappear. 19) View the ISBD details in the OPAC -- values should still be hidden. 20) Check out the OPAC Cart and List -- while the intermediate pages may still leak the download links should leak very minimally. -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH THE INTRANET ISBD AND SOME CART/LIST STUFF WERE FIXED BECAUSE OF THE GetISBDView REFACTOR) Expectations: Before Patch - all the OPAC Detail pages will display things After Patch - all the OPAC Detail pages will display much less, and hopefully nothing (though there are known limits). the ISBD detail page in the Staff client will be filtered as well based on STAFF settings. The saving/exporting should generate nearly empty files. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 55045 [details] [review] Bug 11592: (QA followup) Simplify code Koha::RecordProcessor and the defined filters are supposed to bring us joy and happiness. Let's keep the code compact, simple and clean. This patch removes record cloning all over the place. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 55046 [details] [review] Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls This patch adds the frameworkcode option param, using each record's frameworkcode as expected by the filter. Otherwise the ViewPolicy filter falls back to the default framework. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment on attachment 55045 [details] [review] Bug 11592: (QA followup) Simplify code Review of attachment 55045 [details] [review]: ----------------------------------------------------------------- Just an eyeball. ::: catalogue/ISBDdetail.pl @@ -33,5 @@ > > =cut > > -use strict; > -#use warnings; FIXME - Bug 2505 I wouldn't change to Modern::Perl, unless all the Bug 2505 references are fixed like this. ::: opac/opac-export.pl @@ +35,5 @@ > $biblionumber = int($biblionumber); > my $error = q{}; > > +my $include_items = ($format =~ /bibtex/) ? 0 : 1; > +my $marc = GetMarcBiblio($biblionumber, $include_items) Oooo... nice simplification. ::: opac/opac-showmarc.pl @@ -58,4 @@ > > if ($view eq 'card' || $view eq 'html') { > - # FIXME: GetXmlBiblio needs filtering later. > - my $xml = $importid ? $record->as_xml(): GetXmlBiblio($biblionumber); GetXmlBiblio includes fields which aren't necessarily in GetMarcBiblio, if I recall correctly. I'm not sure about this.
Comment on attachment 55046 [details] [review] Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls Review of attachment 55046 [details] [review]: ----------------------------------------------------------------- Just a quick eyeball. ::: opac/opac-MARCdetail.pl @@ +73,5 @@ > +if ( ! $record ) { > + print $query->redirect("/cgi-bin/koha/errors/404.pl"); > + exit; > +} > + Okay. More logical location.
(In reply to M. Tompsett from comment #140) > Comment on attachment 55045 [details] [review] [review] > Bug 11592: (QA followup) Simplify code > > Review of attachment 55045 [details] [review] [review]: > ----------------------------------------------------------------- > > Just an eyeball. > > ::: catalogue/ISBDdetail.pl > @@ -33,5 @@ > > > > =cut > > > > -use strict; > > -#use warnings; FIXME - Bug 2505 > > I wouldn't change to Modern::Perl, unless all the Bug 2505 references are > fixed like this. I prefer to do it, and take responsability if it introduces a warning. Haven't seen one so far. > ::: opac/opac-showmarc.pl > @@ -58,4 @@ > > > > if ($view eq 'card' || $view eq 'html') { > > - # FIXME: GetXmlBiblio needs filtering later. > > - my $xml = $importid ? $record->as_xml(): GetXmlBiblio($biblionumber); > > GetXmlBiblio includes fields which aren't necessarily in GetMarcBiblio, if I > recall correctly. I'm not sure about this. The main difference is that GetMarcBiblio does some sanitization. I think it is safe. This is probably some reminder from the days when everything was done against biblioitems.marc instead of biblioitems.marcxml.
Mark, are you going to sign off on Tomas' follow-ups so this can move on?
(In reply to Katrin Fischer from comment #143) > Mark, are you going to sign off on Tomas' follow-ups so this can move on? Thanks for the reminder. So many distractions.
Created attachment 55184 [details] [review] Bug 11592: (QA followup) Simplify code Koha::RecordProcessor and the defined filters are supposed to bring us joy and happiness. Let's keep the code compact, simple and clean. This patch removes record cloning all over the place. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 55185 [details] [review] Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls This patch adds the frameworkcode option param, using each record's frameworkcode as expected by the filter. Otherwise the ViewPolicy filter falls back to the default framework. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Pushed to master for 16.11, thanks Mark, Tomas!
In UNIMARC XSLT trasformation on page opac-detail.pl fails with software error Unsupported UNIMARC character encoding [] for XML output for UNIMARC; 100$a -> at /usr/share/perl5/MARC/File/XML.pm line 568. Error occurs if subfield 100$a is made invisible in OPAC. Yet there is no problem with MARC-view and ISBD view Should this be a new bug?
(In reply to Oleg Vasilenko from comment #148) > Should this be a new bug? Hi Oleg, This bug report is marked as pushed to master and the error you got does not seem related. So yes, please open a new bug report or email the list if you have a specific problem. Your problem is usually caused by the fact to have the "marcflavour" system preference set to UNIMARC but you have MARC21 records in your DB.
Hi Jonathan, This the problem with UNIMARC 100$a is connected with this patch, because this problem started after this bug 11592 was pushed to master. Removing this patch solved problem until I found the reason why it happens. It appears, that after filtering (rows 92-100), record is passed to XSLT processing (rows 152-163), and it already lacks field 100$a (it was filtered before). SetUTF8Flag (row 147) does not add filtered 100$a. So right now turning 100$a OPAC visibility off leads to error when user accesses opac-detail.pl. There is no problems with opac-MARCdetail.pl and opac-ISBDdetail.pl — subfield is hidden correctly. So should I create a new bug? (In reply to Jonathan Druart from comment #149) > (In reply to Oleg Vasilenko from comment #148) > > Should this be a new bug? > > Hi Oleg, > This bug report is marked as pushed to master and the error you got does not > seem related. So yes, please open a new bug report or email the list if you > have a specific problem. > Your problem is usually caused by the fact to have the "marcflavour" system > preference set to UNIMARC but you have MARC21 records in your DB.
(In reply to Oleg Vasilenko from comment #150) > So should I create a new bug? Yes please, and link it ("Depends on") to this one.
(In reply to Oleg Vasilenko from comment #148) > Should this be a new bug? Yes, but I think it may be covered by 17527, which is currently in discussion, because people didn't test and go, "Oh, strangeness when we do something bad like everything set to hidden=-8 is perfectly okay." I'm going to tweak it to Needs Sign off, so we can move forward on this.