Bug 24458

Summary: Search results don't use Koha::Filter::MARC::ViewPolicy
Product: Koha Reporter: David Cook <dcook>
Component: SearchingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, eb, fridolin.somers, jonathan.druart, katrin.fischer, lucy.vaux-harvey, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24403
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.12
Bug Depends on: 15870, 25416    
Bug Blocks: 34847, 25457, 25538, 25686, 25826    
Attachments: Bug 24458: Regression tests
Bug 24458: Use the ViewPolicy filter on search results
Bug 24458: Regression tests
Bug 24458: Use the ViewPolicy filter on search results
Bug 24458: Regression tests
Bug 24458: Use the ViewPolicy filter on search results

Description David Cook 2020-01-20 06:11:35 UTC
With https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15870, we started enforcing the bibliographic framework visibility in the OPAC. 

However, we're not using this for search results, which means that hidden fields could come through very visibly.
Comment 1 Tomás Cohen Arazi 2020-05-11 12:12:36 UTC
*** Bug 17527 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2020-05-11 13:22:15 UTC
Created attachment 104696 [details] [review]
Bug 24458: Regression tests

This patch introduces tests for searchResults. The idea is to set a
subfield as hidden in OPAC and another on the staff interface, and see
if searchResults honours this on the XSLTResultsRecord contents.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Search.t
=> FAIL: This is not really implemented

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=25416
Comment 3 Tomás Cohen Arazi 2020-05-11 13:22:20 UTC
Created attachment 104697 [details] [review]
Bug 24458: Use the ViewPolicy filter on search results

This patch implements the use of the ViewPolicy record processor filter
inside C4::Search::searchResults. The idea is that the $record_processor
is instantiated once and reused inside the loop. This leaves options for
further optimizations I will do on a follow-up bug.

The filter is applied to the MARC data before it is passed to the XSLT
processor.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Search.t
=> FAIL: This is not implemented, tests fail
3. Apply this patch
4. Repeat 2
5. Feel the joy in your body from a long standing bug being solved
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=25416
Comment 4 Victor Grousset/tuxayo 2020-05-11 16:38:58 UTC
Created attachment 104707 [details] [review]
Bug 24458: Regression tests

This patch introduces tests for searchResults. The idea is to set a
subfield as hidden in OPAC and another on the staff interface, and see
if searchResults honours this on the XSLTResultsRecord contents.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Search.t
=> FAIL: This is not really implemented

https://bugs.koha-community.org/show_bug.cgi?id=25416

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Victor Grousset/tuxayo 2020-05-11 16:39:02 UTC
Created attachment 104708 [details] [review]
Bug 24458: Use the ViewPolicy filter on search results

This patch implements the use of the ViewPolicy record processor filter
inside C4::Search::searchResults. The idea is that the $record_processor
is instantiated once and reused inside the loop. This leaves options for
further optimizations I will do on a follow-up bug.

The filter is applied to the MARC data before it is passed to the XSLT
processor.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Search.t
=> FAIL: This is not implemented, tests fail
3. Apply this patch
4. Repeat 2
5. Feel the joy in your body from a long standing bug being solved
6. Sign off :-D

https://bugs.koha-community.org/show_bug.cgi?id=25416

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2020-05-11 16:41:43 UTC
> 5. Feel the joy in your body from a long standing bug being solved

And also the joy Test Driven Development in the context of a signoff :D


I'll take a look at bug 25416, because I might I got the testing order wrong ^^"
Comment 7 Tomás Cohen Arazi 2020-05-11 16:44:33 UTC
(In reply to Victor Grousset/tuxayo from comment #6)
> > 5. Feel the joy in your body from a long standing bug being solved
> 
> And also the joy Test Driven Development in the context of a signoff :D
> 
> 
> I'll take a look at bug 25416, because I might I got the testing order wrong
> ^^"

The order was because I wrote the tests for the other one first.
Comment 8 David Cook 2020-05-11 23:27:30 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> 5. Feel the joy in your body from a long standing bug being solved

<3
Comment 9 Nick Clemens 2020-05-12 12:33:44 UTC
Created attachment 104765 [details] [review]
Bug 24458: Regression tests

This patch introduces tests for searchResults. The idea is to set a
subfield as hidden in OPAC and another on the staff interface, and see
if searchResults honours this on the XSLTResultsRecord contents.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Search.t
=> FAIL: This is not really implemented

https://bugs.koha-community.org/show_bug.cgi?id=25416

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2020-05-12 12:33:47 UTC
Created attachment 104766 [details] [review]
Bug 24458: Use the ViewPolicy filter on search results

This patch implements the use of the ViewPolicy record processor filter
inside C4::Search::searchResults. The idea is that the $record_processor
is instantiated once and reused inside the loop. This leaves options for
further optimizations I will do on a follow-up bug.

The filter is applied to the MARC data before it is passed to the XSLT
processor.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Search.t
=> FAIL: This is not implemented, tests fail
3. Apply this patch
4. Repeat 2
5. Feel the joy in your body from a long standing bug being solved
6. Sign off :-D

https://bugs.koha-community.org/show_bug.cgi?id=25416

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Martin Renvoize 2020-05-15 08:58:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-05-19 21:56:19 UTC
missing dependencies - not backported to 19.11.x
Comment 13 Joy Nelson 2020-05-20 21:02:30 UTC
thanks to tomas' help on 25416 i was able to push this to 19.11.x.  Will be in 19.11.06
Comment 14 Tomás Cohen Arazi 2020-06-06 00:34:40 UTC
*** Bug 25124 has been marked as a duplicate of this bug. ***
Comment 15 Victor Grousset/tuxayo 2020-06-08 21:24:22 UTC
Considering to backport to 19.05.X
Why is bug 25416 a dependency?

Trying to backport this bug without bug 25416 is the cause a few conflicts. But they can be reliably solved(independent changes).
And t/db_dependent/Search.t still passes.

But that's just a quick check.
Comment 16 Tomás Cohen Arazi 2020-06-08 23:03:20 UTC
It was because I wrote one before the other, and the tests initially depended on on setup of the mocked things.
Comment 17 David Cook 2020-06-09 00:14:10 UTC
I'm glad to see this change go in, but it appears that it caused Bug 25686, because the visibility for the 999$c was set to hide. 

I don't think that it's a bug or regression, but rather we should let people know somehow that they may need to update their MARC bibliographic frameworks, since they won't be automatically updated.

I suppose that the release notes would've been the place for that, but perhaps it's too late for that?

I think Koha should probably automatically update the "Default Framework" on upgrades. The first time we do it could be problematic, since people's changes would disappear (although we could back them up in an "Old Default Framework"), but I figure we need to have some strategy for delivering framework updates automatically.
Comment 18 Jonathan Druart 2020-06-09 09:07:19 UTC
David, What about adding a check in search_for_data_inconsistencies.pl?
Comment 19 Victor Grousset/tuxayo 2020-06-09 15:45:59 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> It was because I wrote one before the other, and the tests initially
> depended on on setup of the mocked things.

Ok, so I'll backport only this patchset.
Since it's for a major bug, the conflicts are reliable to solve and IIUC there is no strong dependency that could cause undetected regressions.
Comment 20 Victor Grousset/tuxayo 2020-06-09 17:59:53 UTC
Backported to 19.05.x branch for 19.05.12
Comment 21 David Cook 2020-06-09 23:20:50 UTC
(In reply to Jonathan Druart from comment #18)
> David, What about adding a check in search_for_data_inconsistencies.pl?

Yeah maybe. I wasn't familiar with search_for_data_inconsistencies.pl, so I just took a look. (It actually looks like it would benefit from Bug 25661.)

I suppose /cgi-bin/koha/admin/checkmarc.pl would be the best place for a user initiated check. 

Maybe a Koha::Check::* or Koha::Validate::* module that could be used in both places. 

Alternatively, I suppose we could make ViewPolicy skip critical MARC fields...
Comment 22 Tomás Cohen Arazi 2020-06-09 23:51:49 UTC
(In reply to David Cook from comment #17)
> I'm glad to see this change go in, but it appears that it caused Bug 25686,
> because the visibility for the 999$c was set to hide. 
> 
> I don't think that it's a bug or regression, but rather we should let people
> know somehow that they may need to update their MARC bibliographic
> frameworks, since they won't be automatically updated.
> 
> I suppose that the release notes would've been the place for that, but
> perhaps it's too late for that?
> 
> I think Koha should probably automatically update the "Default Framework" on
> upgrades. The first time we do it could be problematic, since people's
> changes would disappear (although we could back them up in an "Old Default
> Framework"), but I figure we need to have some strategy for delivering
> framework updates automatically.

I'm surprised by this, as I recall some bug excluding 999 from this, or maybe it was a comment on the tests because now I cannot find it in bugzilla.
Comment 23 David Cook 2020-06-10 03:13:27 UTC
(In reply to Tomás Cohen Arazi from comment #22)
> I'm surprised by this, as I recall some bug excluding 999 from this, or
> maybe it was a comment on the tests because now I cannot find it in bugzilla.

I see a comment in ./Koha/Filter/MARC/ViewPolicy.pm about how hiding the LDR causes "the MARCXML->MARC::Record->MARCXML transformation blows up"?

I imagine the fix would be to have a hash containing critical fields based on the MARC schema (I don't recall if Unimarc uses 999$c as well or not), and then just consult that in _filter_field()?
Comment 24 David Cook 2020-07-23 03:48:12 UTC
(In reply to Tomás Cohen Arazi from comment #22)
> I'm surprised by this, as I recall some bug excluding 999 from this, or
> maybe it was a comment on the tests because now I cannot find it in bugzilla.

I think maybe Bug 19261