Description
Aleisha Amohia
2017-01-13 00:56:06 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 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 happen is that Plack run all BEGIN blocks and it starts, and that the only time they are evaluated. That means that if you start plack and your pref BakerTaylorEnabled is disabled, the require+import will not be done. If you then enable the pref (and not restart plack of course) and access the opac-readingrec view, it will explode. 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. 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 Second patch doesn't apply. 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 patches apply without conflict - ready to test 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. (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. Can no longer reproduce this bug in master. (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!
> 3. hit again opac-readingrecord.pl
> => Kaboom!
I like your dramatic ending of the test plan :)
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 *** Bug 24704 has been marked as a duplicate of this bug. *** 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> 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> Did not test but regression tested. Passing QA, thanks Nick. Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x for 19.11.04 |