Bug 15870

Correction, this will ONLY be the filter, and tests for the filter. OPAC stuff will go on bug 11592.

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.

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. :)

(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!

Progress. GetMarcStructure was pulling cached values. Still working on this.

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!

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 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.

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.

(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.

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>

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>

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>

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>

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>

Pushed to Master - Should be in the May 2016 release.  Thanks!

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.