Bug 17896 - BakerTaylorEnabled is not plack safe in the OPAC
Summary: BakerTaylorEnabled is not plack safe in the OPAC
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords: Academy
: 24704 (view as bug list)
Depends on:
Blocks: 18798 23482
  Show dependency treegraph
 
Reported: 2017-01-13 00:56 UTC by Aleisha Amohia
Modified: 2020-03-13 01:22 UTC (History)
6 users (show)

See Also:
Change sponsored?: 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.04


Attachments
Bug 17896: Rearranging code in opac-readingrecord.pl so BakerTaylorEnabled works (1.83 KB, patch)
2017-01-13 00:57 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module (3.75 KB, patch)
2017-06-13 21:55 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module (3.77 KB, patch)
2017-08-20 22:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 17896: load BakerTaylor module with use (4.79 KB, patch)
2020-02-21 13:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17896: load BakerTaylor module with use (4.86 KB, patch)
2020-02-21 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17896: Remove duplicated use statements (2.89 KB, patch)
2020-02-21 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2017-01-13 00:56:06 UTC
This error:
Undefined subroutine &CGI::Compile::ROOT::home_vagrant_kohaclone_opac_opac_2dreadingrecord_2epl::image_url called at /home/vagrant/kohaclone/opac/opac-readingrecord.pl line 125.
is triggered when I enable BakerTaylorEnabled and look at my reading history on the OPAC because subroutines it needs aren't imported until after they are called.
Comment 1 Aleisha Amohia 2017-01-13 00:57:52 UTC
Created attachment 58891 [details] [review]
Bug 17896: Rearranging code in opac-readingrecord.pl so BakerTaylorEnabled works

To test:
1) Enable BakerTaylorEnabled and attempt to view reading history in OPAC
2) Notice error
3) Apply patch and refresh page
4) Notice error is gone and page works

Sponsored-by: Catalyst IT
Comment 2 Jonathan Druart 2017-05-30 14:35:00 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2017-05-30 14:36:29 UTC
Aleisha,

I agree that there is something wrong, but the test plan is not enough.

There are several things wrong:
- import must be done at the beginning of the module
- import must not be dependent of syspref in begin blocks

What's happening is that Plack runs all BEGIN blocks when it starts, and that is the only time they are evaluated.
That means that if you start plack with the pref BakerTaylorEnabled disabled, the require+import will not be done.
Then if you enable the pref (and not restart plack of course) and access the opac-readingrec view, it will explode.
Comment 4 Aleisha Amohia 2017-06-13 21:55:56 UTC
Created attachment 64271 [details] [review]
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module

Also fix template to use Koha.Preference instead of passing it from the
pl file
Comment 5 Tomás Cohen Arazi 2017-08-07 15:44:47 UTC
Second patch doesn't apply.
Comment 6 Aleisha Amohia 2017-08-20 22:09:05 UTC
Created attachment 66251 [details] [review]
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module

Also fix template to use Koha.Preference instead of passing it from the
pl file
Comment 7 Aleisha Amohia 2017-08-20 22:09:41 UTC
patches apply without conflict - ready to test
Comment 8 M. Tompsett 2017-08-26 01:55:44 UTC
Comment on attachment 66251 [details] [review]
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module

Review of attachment 66251 [details] [review]:
-----------------------------------------------------------------

There is some JacketImages, AmazonCoverImages, etc. clean up that needs to be done that is beyond the scope of this bug. But deleting the JacketImages from the parameters was a partial start.
Comment 9 Jonathan Druart 2017-09-26 18:26:37 UTC
(In reply to M. Tompsett from comment #8)
> There is some JacketImages, AmazonCoverImages, etc. clean up that needs to
> be done that is beyond the scope of this bug. But deleting the JacketImages
> from the parameters was a partial start.

Same for BakerTaylorEnabled: opac-readingrecord.tt includes opac-bottom.inc which uses it.
Comment 10 Aleisha Amohia 2020-02-09 22:01:03 UTC
Can no longer reproduce this bug in master.
Comment 11 Jonathan Druart 2020-02-11 09:54:49 UTC
(In reply to Aleisha Amohia from comment #10)
> Can no longer reproduce this bug in master.

This is still valid.

1. hit opac-readingrecord.pl
2. Turn the pref BakerTaylorEnabled on
3. hit again opac-readingrecord.pl
=> Kaboom!
Comment 12 Katrin Fischer 2020-02-11 10:02:52 UTC
> 3. hit again opac-readingrecord.pl
> => Kaboom!

I like your dramatic ending of the test plan :)
Comment 13 Nick Clemens 2020-02-21 13:09:57 UTC
Created attachment 99373 [details] [review]
Bug 17896: load BakerTaylor module with use

We are incinsistent here, Amazon and Syndetics module are always loaeded in some places
BakerTaylor is conditional everywhere, and causes issues under plack

For simplicity sake I think we should just load this (small) module where it might be needed

To test:
1 - Disable Baker and Taylor images
2 - Restart plack
3 - Visit opac-readingrecord, opac-detail, opac-search, opac-shelves, opac-user
    Log in to opac
    Virew your reading history
    Make/view a list
    Search the catalog
    Look at an individual title
4 - Enable BakerTaylorEnabled
    If you don't have Baker and Taylor credentials, simply fudge them with bad data and enable
5 - Repeat steps above, in the word of Joubu "Kaboom"
6 - Apply patch
7 - Repeat 1-4
8 - You shoudl be able to load all the pages after enabling the pref
Comment 14 Nick Clemens 2020-02-21 13:10:18 UTC
*** Bug 24704 has been marked as a duplicate of this bug. ***
Comment 15 Jonathan Druart 2020-02-21 13:36:44 UTC
Created attachment 99377 [details] [review]
Bug 17896: load BakerTaylor module with use

We are incinsistent here, Amazon and Syndetics module are always loaeded in some places
BakerTaylor is conditional everywhere, and causes issues under plack

For simplicity sake I think we should just load this (small) module where it might be needed

To test:
1 - Disable Baker and Taylor images
2 - Restart plack
3 - Visit opac-readingrecord, opac-detail, opac-search, opac-shelves, opac-user
    Log in to opac
    Virew your reading history
    Make/view a list
    Search the catalog
    Look at an individual title
4 - Enable BakerTaylorEnabled
    If you don't have Baker and Taylor credentials, simply fudge them with bad data and enable
5 - Repeat steps above, in the word of Joubu "Kaboom"
6 - Apply patch
7 - Repeat 1-4
8 - You shoudl be able to load all the pages after enabling the pref

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2020-02-21 13:36:47 UTC
Created attachment 99378 [details] [review]
Bug 17896: Remove duplicated use statements

and remove uneeded '&'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2020-02-21 13:38:24 UTC
Did not test but regression tested.
Passing QA, thanks Nick.
Comment 18 Martin Renvoize 2020-02-21 15:45:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 19 Joy Nelson 2020-03-13 01:22:10 UTC
Pushed to 19.11.x for 19.11.04