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.
*** Bug 17527 has been marked as a duplicate of this bug. ***
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
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
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>
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>
> 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 ^^"
(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.
(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
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>
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>
Nice work everyone! Pushed to master for 20.05
missing dependencies - not backported to 19.11.x
thanks to tomas' help on 25416 i was able to push this to 19.11.x. Will be in 19.11.06
*** Bug 25124 has been marked as a duplicate of this bug. ***
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.
It was because I wrote one before the other, and the tests initially depended on on setup of the mocked things.
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.
David, What about adding a check in search_for_data_inconsistencies.pl?
(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.
Backported to 19.05.x branch for 19.05.12
(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...
(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.
(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()?
(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