Bug 25008 - Koha::RecordProcessor->options doesn't refresh the filters
Summary: Koha::RecordProcessor->options doesn't refresh the filters
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-27 20:53 UTC by Tomás Cohen Arazi
Modified: 2020-05-13 22:07 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
Bug 25008: Regression tests (2.12 KB, patch)
2020-03-27 20:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25008: Tests for ->options (1.19 KB, patch)
2020-03-30 13:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25008: Overload Koha::RecordProcessor->options to update filters (1.35 KB, patch)
2020-03-30 13:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25008: Regression tests (2.20 KB, patch)
2020-03-30 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25008: Tests for ->options (1.26 KB, patch)
2020-03-30 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25008: Overload Koha::RecordProcessor->options to update filters (1.42 KB, patch)
2020-03-30 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25008: Regression tests (2.26 KB, patch)
2020-04-13 10:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25008: Tests for ->options (1.31 KB, patch)
2020-04-13 10:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25008: Overload Koha::RecordProcessor->options to update filters (1.48 KB, patch)
2020-04-13 10:53 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-03-27 20:53:16 UTC
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.
Comment 1 Tomás Cohen Arazi 2020-03-27 20:56:22 UTC
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!
Comment 2 Tomás Cohen Arazi 2020-03-27 21:38:00 UTC
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({
Comment 3 Jonathan Druart 2020-03-30 10:36:01 UTC
The easiest for now seems to make it RO and modify the callers, what do you think?
Comment 4 Tomás Cohen Arazi 2020-03-30 11:38:55 UTC
(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.
Comment 5 Tomás Cohen Arazi 2020-03-30 13:31:06 UTC
Created attachment 102082 [details] [review]
Bug 25008: Tests for ->options
Comment 6 Tomás Cohen Arazi 2020-03-30 13:31:10 UTC
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
Comment 7 Jonathan Druart 2020-03-30 14:26:03 UTC
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>
Comment 8 Jonathan Druart 2020-03-30 14:26:07 UTC
Created attachment 102087 [details] [review]
Bug 25008: Tests for ->options

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2020-03-30 14:26:10 UTC
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>
Comment 10 Katrin Fischer 2020-04-12 20:58:43 UTC
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?
Comment 11 Katrin Fischer 2020-04-12 21:33:10 UTC
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.
Comment 12 Tomás Cohen Arazi 2020-04-13 01:04:59 UTC
(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.
Comment 13 Katrin Fischer 2020-04-13 10:51:49 UTC
> > 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 :)
Comment 14 Katrin Fischer 2020-04-13 10:53:44 UTC
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>
Comment 15 Katrin Fischer 2020-04-13 10:53:47 UTC
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>
Comment 16 Katrin Fischer 2020-04-13 10:53:51 UTC
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>
Comment 17 Katrin Fischer 2020-04-13 11:53:15 UTC
(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
Comment 18 Martin Renvoize 2020-04-14 07:37:17 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 19 Joy Nelson 2020-05-05 21:09:00 UTC
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
Comment 20 Tomás Cohen Arazi 2020-05-05 21:18:01 UTC
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.
Comment 21 Joy Nelson 2020-05-05 23:13:09 UTC
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.
Comment 22 Tomás Cohen Arazi 2020-05-05 23:34:46 UTC
(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
Comment 23 Lucas Gass 2020-05-13 22:07:53 UTC
Same pod error for me but I backported regardless. 

Backported to 19.05.x for 19.05.11