Bug 15870

Summary: Add Filter for MARC to respect visibility settings
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: Architecture, internals, and plumbingAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: brendan, dcook, hector.hecaxmmx, jonathan.druart, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11592
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16101
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15777, 15871    
Bug Blocks: 11592, 19261, 24458, 25019    
Attachments: Bug 15870: MARC Filter to exclude fields/subfields lacking visibility
Bug 15870: MARC Filter to exclude fields/subfields lacking visibility
[SIGNED-OFF]Bug 15870: MARC Filter to exclude fields/subfields lacking visibility
Bug 15870: (QA followup) Add tests
Bug 15870: MARC Filter to exclude fields/subfields lacking visibility
Bug 15870: (QA followup) Add tests
Bug 15870 - Follow-up of filter and tests
Bug 15870 - potential follow up to comment #8
[SIGNED-OFF]Bug 15870: MARC Filter to exclude fields/subfields lacking visibility
[SIGNED-OFF]Bug 15870: (QA followup) Add tests
[SIGNED-OFF]Bug 15870: Follow-up of filter and tests
[SIGNED-OFF]Bug 15870: potential follow up to comment #8
Bug 15870: Follow up to address comment #20
[SIGNED-OFF]Bug 15870: Follow up to address comment #20
Bug 15870: MARC Filter to exclude fields/subfields lacking visibility
Bug 15870: (QA followup) Add tests
Bug 15870: Follow-up of filter and tests
Bug 15870: potential follow up to comment #8
Bug 15870: Follow up to address comment #20

Description Mark Tompsett 2016-02-19 21:07:58 UTC
There needs to be a new filter for MARC::Records such that when a record is processed a cloned record with hidden fields and sub-fields excluded.

This will only affect MARC::Records, and as such will leave some visibility gaps still. This will only attempt to implement this filter on OPAC pages, though the filter should be usable for the staff client as well.
Comment 1 Mark Tompsett 2016-02-19 21:55:00 UTC
Correction, this will ONLY be the filter, and tests for the filter. OPAC stuff will go on bug 11592.
Comment 2 Mark Tompsett 2016-02-19 22:15:36 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2016-02-19 22:25:08 UTC Comment hidden (obsolete)
Comment 4 Héctor Eduardo Castro Avalos 2016-02-24 15:47:51 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2016-03-03 15:28:58 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2016-03-03 15:38:11 UTC
This bug was lacking functionality tests. As it is in my best interest to have this moving, I've gone through the process of writing unit tests for this piece of code.

Things that weren't tested:
- opac vs. intranet (interface option)
- test all the possible values.
- control fields and datafields traverse different code branches, and needed to be tested.

The tests I wrote cover most of what is needed. Please read the TODO in the commit message.

The tests fail.

Mark: you need to get rid of the clone. The RecordProcessor pipeline is designed so it works on the original object. Doing so will make several tests pass:

    foreach my $current_record (@records) {
-        my $result        = $current_record->clone();
+        my $result        = $current_record;


There are still problems. Hopefully problems in the tests. I couldn't spend more time on this.

Also, there is a weird construct you should get rid off, so you simplify the code and make it more readable:

+    else {
+        # visibility is a "level" (-7 to +7), default to 0
+        my $visibility = $marcsubfieldstructure->{$tag}->{q{@}}->{hidden};
+        $visibility //= 0;
+        my $hidden;
+        if ( $display->{$interface}->{$visibility} ) {
+            $hidden = 0;
+        }
+        else {
+            $hidden = 1;
+            $result->delete_fields($field);
+        }
+    }

could be just:

+    else {
+        # visibility is a "level" (-7 to +7), default to 0
+        my $visibility = $marcsubfieldstructure->{$tag}->{q{@}}->{hidden};
+        $visibility //= 0;
+        if ( ! $display->{$interface}->{$visibility} ) {
+            $result->delete_fields($field);
+        }
+    }

Looking forward to your comments and/or followups. Count on me to have this done ASAP.
Comment 7 Mark Tompsett 2016-03-03 23:53:30 UTC
Wow. Much more comprehensive tests.
I'll have to look into the clone issue.
I can't remember why I had hidden, except to perhaps clarify that a delete was a hide. I'll look more closely at your comments.
Though, I just noticed you missed a perldoc change in the package, because you renamed the test file. :)
Comment 8 Mark Tompsett 2016-03-04 20:00:57 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> Mark: you need to get rid of the clone. The RecordProcessor pipeline is
> designed so it works on the original object.

I'm torn on that.

%%%% SNIP %%%%
sub process {
    my $self = shift;
    my $record = shift || $self->record;

    return unless defined $record;

    my $newrecord = $record;

    foreach my $filterobj (@{$self->filters}) {
        next unless $filterobj;
        $newrecord = $filterobj->filter($newrecord);
    }

    return $newrecord;
}
%%%% SNIP %%%%

The clincher for me is that the $filterobj->filter call returns something. What is the point of returning something if we are modifying the original object? The return $newrecord is still necessary because of pre-call binding or passing the record(s) directly. However, the "$newrecord =" makes no sense if the original was not intended to stay intact, because it would have already been modified!
Comment 9 Mark Tompsett 2016-03-07 14:19:10 UTC
Progress. GetMarcStructure was pulling cached values. Still working on this.
Comment 10 Mark Tompsett 2016-03-07 15:56:43 UTC
From 3.10.x online help
hidden
allows you to select from 19 possible visibility conditions, 17 of which are implemented. They are the following:
-9 => Future use
-8 => Flag
-7 => OPAC !Intranet !Editor Collapsed
-6 => OPAC Intranet !Editor !Collapsed
-5 => OPAC Intranet !Editor Collapsed
-4 => OPAC !Intranet !Editor !Collapsed
-3 => OPAC !Intranet Editor Collapsed
-2 => OPAC !Intranet Editor !Collapsed
-1 => OPAC Intranet Editor Collapsed
0 => OPAC Intranet Editor !Collapsed
1 => !OPAC Intranet Editor Collapsed
2 => !OPAC !Intranet Editor !Collapsed
3 => !OPAC !Intranet Editor Collapsed
4 => !OPAC Intranet Editor !Collapsed
5 => !OPAC !Intranet !Editor Collapsed
6 => !OPAC Intranet !Editor !Collapsed
7 => !OPAC Intranet !Editor Collapsed
8 => !OPAC !Intranet !Editor !Collapsed
9 => Future use
( ! means 'not visible' or in the case of Collapsed 'not Collapsed')
This is pre-bug 9894. So cait was right!
Comment 11 Mark Tompsett 2016-03-08 06:19:00 UTC Comment hidden (obsolete)
Comment 12 Mark Tompsett 2016-03-08 06:19:10 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2016-03-08 06:19:23 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2016-03-08 06:19:29 UTC Comment hidden (obsolete)
Comment 15 Mark Tompsett 2016-03-08 06:26:16 UTC
TEST PLAN
---------
apply all but the last patch
prove -v t/db_dependent/Filter_MARC_ViewPolicy.t
prove -v t/RecordProcessor.t
run koha qa test tools
apply the last patch
prove -v t/db_dependent/Filter_MARC_ViewPolicy.t
prove -v t/RecordProcessor.t
run koha qa test tools

Decide if the last patch is what addresses comment #8.
Comment 16 Héctor Eduardo Castro Avalos 2016-03-08 19:46:20 UTC Comment hidden (obsolete)
Comment 17 Héctor Eduardo Castro Avalos 2016-03-08 19:46:29 UTC Comment hidden (obsolete)
Comment 18 Héctor Eduardo Castro Avalos 2016-03-08 19:46:38 UTC Comment hidden (obsolete)
Comment 19 Héctor Eduardo Castro Avalos 2016-03-08 19:46:46 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-03-11 10:06:10 UTC
Comment on attachment 48814 [details] [review]
[SIGNED-OFF]Bug 15870: Follow-up of filter and tests

Review of attachment 48814 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Filter_MARC_ViewPolicy.t
@@ +33,2 @@
>  use Koha::Database;
> +use English qw/-no_match_vars/;

You are not using regex, it's not needed.

@@ +33,4 @@
>  use Koha::Database;
> +use English qw/-no_match_vars/;
> +
> +$OUTPUT_AUTOFLUSH = 1;

This is not used later.

@@ +66,2 @@
>  
> +        $schema->storage->txn_begin();

I'd suggest to begin/rollback the transaction in the subtests.

@@ +69,5 @@
> +        my $update_sql =
> +            q{UPDATE marc_subfield_structure SET hidden=? }
> +          . q{WHERE tagfield='020' OR }
> +          . q{      tagfield='008';};
> +        my $sth = $dbh->prepare($update_sql);

prepare should be outside of the loop, otherwise there are no needs to prepare it :)

@@ +100,2 @@
>          # Data fields
> +        if ( any { $_ == $hidden_value } @{ $hidden->{$interface} } ) {

You could have used grep, but does not really matter.

@@ +152,5 @@
> +    my @fields      = (
> +        MARC::Field->new( '003', 'AR-CdUBM' ),
> +        MARC::Field->new( '008', '######suuuu####ag_||||__||||_0||_|_uuu|d' ),
> +        MARC::Field->new( '020', q{}, q{}, 'a' => $isbn ),
> +        MARC::Field->new( '245', q{}, q{}, 'a' => $title ),

Would be good to make it marcflavour independent, but not a requirement yet.
Comment 21 Jonathan Druart 2016-03-11 10:13:07 UTC
I am fine with modifying the record and not cloning it (better perfs), the caller will have to take care of that if it does want to keep the original record.
Comment 22 Mark Tompsett 2016-03-11 13:41:21 UTC Comment hidden (obsolete)
Comment 23 Héctor Eduardo Castro Avalos 2016-03-11 15:01:10 UTC Comment hidden (obsolete)
Comment 24 Mark Tompsett 2016-03-14 02:34:24 UTC
(In reply to Jonathan Druart from comment #21)
> I am fine with modifying the record and not cloning it (better perfs), the
> caller will have to take care of that if it does want to keep the original
> record.

BTW, the 4th patch removes the clone.
Comment 25 Tomás Cohen Arazi 2016-03-24 15:21:39 UTC
Created attachment 49534 [details] [review]
Bug 15870: MARC Filter to exclude fields/subfields lacking visibility

TEST PLAN
---------
0) Apply 15777 and 15871 as required.
1) Apply patch
2) prove -v t/db_dependpent/RecordProcessor_ViewPolicy.t
   -- all should pass.
3) koha qa test tools.
BONUS
4) perlcritic -1 t/db_dependpent/RecordProcessor_ViewPolicy.t
   -- using the koha qa test tools perlcriticrc, it should pass
      though I am excluding checking print return values.
      (see bug 15646)

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2016-03-24 15:21:50 UTC
Created attachment 49535 [details] [review]
Bug 15870: (QA followup) Add tests

This patch introduces functionality tests for the new Koha::Filter.
It iterates over all the possible 'hidden' values defined in the docs
for hiding/showing fields and subfields, and checks the filter does the job.

It tests the functionality for both opac and intranet interfaces and tries
to catch all scenarios. It adds control fields to the sample record (they
traverse a different branch in the code).

TODO: Add another subfield to 245, set it to be hidden, and make sure 245 $a is still
shown.

The original tests file has been renamed conveniently.

To test:
- Run
  $ prove t/db_dependent/Filter_MARC_ViewPolicy.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2016-03-24 15:22:03 UTC
Created attachment 49536 [details] [review]
Bug 15870: Follow-up of filter and tests

This patch:
- improves perlcritic messages in the filter and tests.
- changes should display logic to should hide logic
  to simplify filter.
- perltidies the scripts
- debugs the issues outstanding on the comprehensive tests
  provided in the second commit.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2016-03-24 15:22:26 UTC
Created attachment 49537 [details] [review]
Bug 15870: potential follow up to comment #8

This patch:
- makes the Koha::RecordProcessor code more clear
  by removing the unnecessary newrecord variable.
- revises the filter to be more clear about the
  expectation that operations are done directly on
  the record parameter.

TEST PLAN
---------
prove t/RecordProcessor.t
prove t/db_dependent/Filter_MARC_ViewPolicy.t
run koha qa test tools

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised. NO koha-qa errors
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2016-03-24 15:22:36 UTC
Created attachment 49538 [details] [review]
Bug 15870: Follow up to address comment #20

- $OUTPUT_AUTOFLUSH wasn't used to it was removed
- Use English was removed since $OUTPUT_AUTOFLUSH wasn't not needed
- transaction start and end were moved to subtests
- prepare was moved outside loop
- partial MARC agnosticism was added

TEST PLAN
---------
1) apply patch
2) prove t/db_dependent/Filter_MARC_ViewPolicy.t
3) run koha qa test tools

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Brendan Gallagher 2016-03-24 20:16:37 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 31 David Cook 2020-01-20 06:11:47 UTC
I just noticed that we're not using this functionality for OPAC search results, which means we're not fully respecting the bibliographic frameworks for the OPAC.

I've opened up https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24458 in regards to this.