On testing something in opac-showmarc.pl I noticed it wasn't applying the ViewPolicy filter! And it seems the way the record processor is used is to blame. I plan to submit a fix for opac-showmarc.pl but thought it was worth filing a bug against Koha::RecordProcessor.
Created attachment 102019 [details] [review] Bug 25008: Regression tests This patch highlights a behaviour of Koha::RecordProcessor that is unexpected: if you change the original options using ->options, the loaded filters don't pick the change. That's because the filter objects are loaded on ->new, and they are never updated. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/RecordProcessor.t => FAIL: Test prove ->options doesn't update the filters!
Problematic uses: $ git grep 'processor\->options' opac/opac-basket.pl: $record_processor->options({ opac/opac-downloadcart.pl: $record_processor->options({ opac/opac-downloadshelf.pl: $record_processor->options({ opac/opac-shelves.pl: $record_processor->options({
The easiest for now seems to make it RO and modify the callers, what do you think?
(In reply to Jonathan Druart from comment #3) > The easiest for now seems to make it RO and modify the callers, what do you > think? I agree. I filled this bug so I don't forget about the problem. We want to be able to update the options when processing lists of records (e.g. lists, carts, etc) so we don't generate new objects only to change (say) the items. In those, we should cache the objects on a per-framework basis.
Created attachment 102082 [details] [review] Bug 25008: Tests for ->options
Created attachment 102083 [details] [review] Bug 25008: Overload Koha::RecordProcessor->options to update filters This patch overloads the 'options' accessor generated by Class:Accessor. It does so the passed options are used to refresh the loaded filters. Tests are added for this overloaded method as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/RecordProcessor.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 102086 [details] [review] Bug 25008: Regression tests This patch highlights a behaviour of Koha::RecordProcessor that is unexpected: if you change the original options using ->options, the loaded filters don't pick the change. That's because the filter objects are loaded on ->new, and they are never updated. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/RecordProcessor.t => FAIL: Test prove ->options doesn't update the filters! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 102087 [details] [review] Bug 25008: Tests for ->options Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 102088 [details] [review] Bug 25008: Overload Koha::RecordProcessor->options to update filters This patch overloads the 'options' accessor generated by Class:Accessor. It does so the passed options are used to refresh the loaded filters. Tests are added for this overloaded method as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/RecordProcessor.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The patch and description are very technical - should we expect that this fixes the display issues on the names pages? And should this not be verified in testing?
I tested this by hiding 100$a and 245$c via the framework settings: Problematic = information still shows - Result list - Cart - normal and more detailed view - MARC download from cart/list - MARC download from detail page (245$c kept showing?) Couldn't test the emails sent form list and shelf. Probably an entirely different thing to discuss: - Use for a suggestoin (from detail page) - author is in the form - Place hold (probably using other methods of pulling the information) Good = information doesn't show - Detail page, normal view: hides - Detail page, plain and MARC (plain fixed by patch) - ISBD view - RIS export - ISBD export - Bibtex export - Dublin Core export - MARCXML export - List view (fixed by patch) I am willing to PQA on ths, but could you take a list at the problematic ones first? Result list especially.
(In reply to Katrin Fischer from comment #11) > I tested this by hiding 100$a and 245$c via the framework settings: > > Problematic = information still shows > - Result list > - Cart - normal and more detailed view > - MARC download from cart/list > - MARC download from detail page (245$c kept showing?) > Couldn't test the emails sent form list and shelf. > > Probably an entirely different thing to discuss: > - Use for a suggestoin (from detail page) - author is in the form > - Place hold (probably using other methods of pulling the information) > > Good = information doesn't show > - Detail page, normal view: hides > - Detail page, plain and MARC (plain fixed by patch) > - ISBD view > - RIS export > - ISBD export > - Bibtex export > - Dublin Core export > - MARCXML export > - List view (fixed by patch) > > I am willing to PQA on ths, but could you take a list at the problematic > ones first? Result list especially. Katrina, I was waiting for you to show up :-) Please file bugs for the problematic ones and sign them to me. I will happily deal with them. They are not related to this bug directly, they don't use RecordProcessor.
> > I am willing to PQA on ths, but could you take a list at the problematic > > ones first? Result list especially. > > Katrina, I was waiting for you to show up :-) Please file bugs for the > problematic ones and sign them to me. I will happily deal with them. They > are not related to this bug directly, they don't use RecordProcessor. Should have read 'take a look'(not list). It looks like the bugs mailing list is out of order at the moment, which means I might be missing things. Please email me directly if you need me to show up somewhere else :)
Created attachment 102806 [details] [review] Bug 25008: Regression tests This patch highlights a behaviour of Koha::RecordProcessor that is unexpected: if you change the original options using ->options, the loaded filters don't pick the change. That's because the filter objects are loaded on ->new, and they are never updated. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/RecordProcessor.t => FAIL: Test prove ->options doesn't update the filters! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102807 [details] [review] Bug 25008: Tests for ->options Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102808 [details] [review] Bug 25008: Overload Koha::RecordProcessor->options to update filters This patch overloads the 'options' accessor generated by Class:Accessor. It does so the passed options are used to refresh the loaded filters. Tests are added for this overloaded method as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/RecordProcessor.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Tomás Cohen Arazi from comment #12) > (In reply to Katrin Fischer from comment #11)> > Katrina, I was waiting for you to show up :-) Please file bugs for the > problematic ones and sign them to me. I will happily deal with them. They > are not related to this bug directly, they don't use RecordProcessor. Done: bug 25124, bug 25125, bug 25126
Nice work everyone! Pushed to master for 20.05
qa tests pass but i do get an error on pod (see below) I did backport to 19.11.x for 19.11.06 FAIL Koha/RecordProcessor.pm OK critic OK forbidden patterns OK git manipulation FAIL pod *** ERROR: Spurious text after =cut in file Koha/RecordProcessor.pm *** ERROR: Spurious text after =cut in file Koha/RecordProcessor.pm *** ERROR: Spurious text after =cut in file Koha/RecordProcessor.pm *** ERROR: Spurious text after =cut in file Koha/RecordProcessor.pm OK pod coverage OK spelling OK valid
Katrina wouldn't have allowed me to submit something like that. I guess it is because of the rebase. I can go fix it with a follow-up patch.
Entirely likely my fault! I would it if you would take a look at it. I thought I had rebased it well and when I went back to check nothing looked wrong to me. Thanks! joy (In reply to Tomás Cohen Arazi from comment #20) > Katrina wouldn't have allowed me to submit something like that. I guess it > is because of the rebase. I can go fix it with a follow-up patch.
(In reply to Joy Nelson from comment #21) > Entirely likely my fault! I would it if you would take a look at it. I > thought I had rebased it well and when I went back to check nothing looked > wrong to me. > > Thanks! > joy > > (In reply to Tomás Cohen Arazi from comment #20) > > Katrina wouldn't have allowed me to submit something like that. I guess it > > is because of the rebase. I can go fix it with a follow-up patch. It is a POD error so no rush. I will fix it first thing in the morning with Javier
Same pod error for me but I backported regardless. Backported to 19.05.x for 19.05.11