Bug 17896

Summary: BakerTaylorEnabled is not plack safe in reading history at the OPAC
Product: Koha Reporter: Aleisha Amohia <aleisha>
Component: OPACAssignee: Aleisha Amohia <aleisha>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: jonathan.druart, josef.moravec, tomascohen
Version: unspecifiedKeywords: Academy
Hardware: All   
OS: All   
Whiteboard:
Change sponsored?: Sponsored Patch complexity: Small patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Bug Depends on:    
Bug Blocks: 18798    
Attachments: Bug 17896: Rearranging code in opac-readingrecord.pl so BakerTaylorEnabled works
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module
Bug 17896: [FOLLOW-UP] Make opac-readingrecord.pl use module

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.