Created attachment 59593 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, SelfCheckTimeout, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin, Sponsored-by: Catalyst IT
Let's start with the good: authbylogin -- cleaned up. SCOUserJS, SCOUserCSS -- both fixed. AllowSelfCheckReturns -- remnant left behind, but variable is used, so that's okay. Then the weird: display_patron_image -- something isn't quite right. It is only set if there is an image to display, whereas the preference change makes no such distinction. cardnumber is currently only set if there is an image, you could expand the condition. Strangely, cardnumber isn't even used in the template currently. The image logic is using borrower number. Something is wrong. Recommendation: Perhaps a prequel bug is in order to deal with this patron image bug in self-checkout? And lastly the reason I'm Failing QA. $selfchecktimeout -- Remnant left behind, which deals with system preference not being set. Logic in sco-main.tt line 343 does not deal with system preference not being set. Also line 419 in sco-main.tt was unchanged. Recommendation: leave the logic for this as was OR put the logic into the TT file.
Created attachment 59961 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT
(In reply to M. Tompsett from comment #2) > Then the weird: > display_patron_image -- something isn't quite right. > It is only set if there is an image to display, whereas the preference > change makes no such distinction. cardnumber is currently only set if there > is an image, you could expand the condition. Strangely, cardnumber isn't > even used in the template currently. The image logic is using borrower > number. Something is wrong. Recommendation: Perhaps a prequel bug is in > order to deal with this patron image bug in self-checkout? I fixed this in this patch just by passing a parameter to the template that says an image exists. > And lastly the reason I'm Failing QA. > $selfchecktimeout -- Remnant left behind, which deals with system preference > not being set. > Logic in sco-main.tt line 343 does not deal with system preference not being > set. > Also line 419 in sco-main.tt was unchanged. > Recommendation: leave the logic for this as was OR put the logic into the TT > file. Decided to leave the logic for this as was. Ready for signoff
Created attachment 62993 [details] [review] [SIGNED OFF] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=12601
This patch will not apply on top of bug 7550 (Passed QA), please rebase it on top.
Comment on attachment 62993 [details] [review] [SIGNED OFF] Bug 12691: Use Koha.Preference in Self-Checkout Review of attachment 62993 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt @@ +202,4 @@ > </div> > </div> <!-- / #newcheckout --> > </div> <!-- / .span12/12 --> > + [% IF ( Koha.Preference('ShowPatronImageInWebBasedSelfCheck') && image_exists ) %] This image_exists and display_patron_image logic doesn't quite match up, and is where the application logic fails. The old logic would still generate the span2 if the image doesn't exist. This logic will not. ::: opac/sco/sco-main.pl @@ +292,4 @@ > ); > if (C4::Context->preference('ShowPatronImageInWebBasedSelfCheck')) { > my $patron_image = Koha::Patron::Images->find($borrower->{borrowernumber}); > + $template->param( image_exists => 1 ) if $patron_image; While the master code has changed, I see no reason for this change, except perhaps to optimize URL calls. But if that were the case, then an ELSE in the template file would be better, so as to generate an empty span2.
Created attachment 63903 [details] [review] Bug 12691: [FOLLOW-UP] Follow-up patch This patch fixes merge conflicts and fixes the problems in Comment 7
(In reply to Aleisha Amohia from comment #8) > Created attachment 63903 [details] [review] [review] > Bug 12691: [FOLLOW-UP] Follow-up patch > > This patch fixes merge conflicts and fixes the problems in Comment 7 Sorry, follow-up patch does not apply on current master: fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt).
(In reply to Marc Véron from comment #9) > (In reply to Aleisha Amohia from comment #8) > > Created attachment 63903 [details] [review] [review] [review] > > Bug 12691: [FOLLOW-UP] Follow-up patch > > > > This patch fixes merge conflicts and fixes the problems in Comment 7 > > Sorry, follow-up patch does not apply on current master: > > fatal: sha1 information is lacking or useless > (koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt). Sorry, i mistaked, it is the first patch that does not apply.
Created attachment 65527 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=12601
Created attachment 65528 [details] [review] Bug 12691: [FOLLOW-UP] Follow-up patch This patch fixes merge conflicts and fixes the problems in Comment 7
Created attachment 65529 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=12601
There we go, merge conflicts fixed.
Can not test ATM because of Bug 19048
Tested on top of Bug 19048 Template process failed: file error - parse error - (...)/home/marc/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt line 205: unexpected end of directive [% IF ( Koha.Preference('ShowPatronImageInWebBasedSelfCheck') %]
Created attachment 66252 [details] [review] Bug 12691: [FOLLOW-UP] Missing bracket Patch adds bracket to template file (Comment 16)
Created attachment 66263 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66264 [details] [review] Bug 12691: [FOLLOW-UP] Follow-up patch This patch fixes merge conflicts and fixes the problems in Comment 7 QA tools complain about missing bracket, will be fixed in next followup Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66265 [details] [review] Bug 12691: [FOLLOW-UP] Missing bracket Patch adds bracket to template file (Comment 16) Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66756 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 66757 [details] [review] Bug 12691: [FOLLOW-UP] Follow-up patch This patch fixes merge conflicts and fixes the problems in Comment 7 QA tools complain about missing bracket, will be fixed in next followup Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 66758 [details] [review] Bug 12691: [FOLLOW-UP] Missing bracket Patch adds bracket to template file (Comment 16) Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
What are we doing here? If it is just replacing syspref fetch with Koha.Preference, the following is wrong: - [% IF ( display_patron_image ) %] + [% IF ( Koha.Preference('ShowPatronImageInWebBasedSelfCheck') ) %] display_patron_image is set when $borrower is set AND the pref is set. We had too many regressions in this area, I will not take any risks. If we are doing something else, it is not written in the commit messages. Please clarify.
(In reply to Jonathan Druart from comment #24) > What are we doing here? > > If it is just replacing syspref fetch with Koha.Preference, the following is > wrong: > > - [% IF ( display_patron_image ) %] > + [% IF ( > Koha.Preference('ShowPatronImageInWebBasedSelfCheck') ) %] > > display_patron_image is set when $borrower is set AND the pref is set. > We had too many regressions in this area, I will not take any risks. > > If we are doing something else, it is not written in the commit messages. > > Please clarify. Oh I see what you mean, I think I've made the logic a little too confusing. I'll take out the [% IF ( Koha.Preference('ShowPatronImageInWebBasedSelfCheck') ) %] line in the template and leave the logic for that as it was.
Created attachment 66906 [details] [review] Bug 12691: [FOLLOW-UP] Fixing some logic
Created attachment 66926 [details] [review] Bug 12691: [FOLLOW-UP] Fixing some logic Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66953 [details] [review] Bug 12691: Use Koha.Preference in Self-Checkout For calls to SCOUserJS, SCOUserCSS, OPACUserCSS, AllowSelfCheckReturns, OpacFavicon, ShowPatronImageInWebBasedSelfCheck, SelfCheckoutByLogin Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Bug 12691: [FOLLOW-UP] Follow-up patch This patch fixes merge conflicts and fixes the problems in Comment 7 QA tools complain about missing bracket, will be fixed in next followup Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Bug 12691: [FOLLOW-UP] Missing bracket Patch adds bracket to template file (Comment 16) Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Bug 12691: [FOLLOW-UP] Fixing some logic Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Patches have been squashed for readability and 1 removal occurrence of display_patron_image has been reintroduced.
Pushed to master for 17.11, thanks to everybody involved!
Enhancement not pushed to 17.05.x