Bug 12691 - Use Koha.Preference for calls to SCOUserJS, SCOUserCSS, OPACUserCSS, opacuserjs, etc in Self-Checkout
Summary: Use Koha.Preference for calls to SCOUserJS, SCOUserCSS, OPACUserCSS, opacuser...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 7550 19048
Blocks: 17381
  Show dependency treegraph
 
Reported: 2014-08-01 07:02 UTC by David Cook
Modified: 2017-10-10 09:55 UTC (History)
5 users (show)

See Also:
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:


Attachments
Bug 12691: Use Koha.Preference in Self-Checkout (9.90 KB, patch)
2017-01-27 03:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: Use Koha.Preference in Self-Checkout (9.63 KB, patch)
2017-02-06 23:31 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12691: Use Koha.Preference in Self-Checkout (9.75 KB, patch)
2017-05-02 21:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Follow-up patch (1.54 KB, patch)
2017-06-02 03:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: Use Koha.Preference in Self-Checkout (10.04 KB, patch)
2017-08-07 00:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Follow-up patch (1.50 KB, patch)
2017-08-07 00:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: Use Koha.Preference in Self-Checkout (9.24 KB, patch)
2017-08-07 00:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Missing bracket (1.23 KB, patch)
2017-08-20 22:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: Use Koha.Preference in Self-Checkout (9.29 KB, patch)
2017-08-21 08:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Follow-up patch (1.70 KB, patch)
2017-08-21 08:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Missing bracket (1.36 KB, patch)
2017-08-21 08:36 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12691: Use Koha.Preference in Self-Checkout (9.35 KB, patch)
2017-09-01 15:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Follow-up patch (1.75 KB, patch)
2017-09-01 15:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Missing bracket (1.42 KB, patch)
2017-09-01 15:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Fixing some logic (1.43 KB, patch)
2017-09-06 22:58 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12691: [FOLLOW-UP] Fixing some logic (1.56 KB, patch)
2017-09-07 10:21 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12691: Use Koha.Preference in Self-Checkout (9.74 KB, patch)
2017-09-07 17:05 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 David Cook 2014-08-01 07:02:17 UTC

    
Comment 1 Aleisha Amohia 2017-01-27 03:46:55 UTC
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
Comment 2 M. Tompsett 2017-02-03 18:45:05 UTC
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.
Comment 3 Aleisha Amohia 2017-02-06 23:31:53 UTC
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
Comment 4 Aleisha Amohia 2017-02-06 23:33:04 UTC
(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
Comment 5 Katrin Fischer 2017-05-02 21:23:39 UTC
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
Comment 6 Jonathan Druart 2017-05-02 22:25:16 UTC
This patch will not apply on top of bug 7550 (Passed QA), please rebase it on top.
Comment 7 M. Tompsett 2017-05-15 04:19:50 UTC
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.
Comment 8 Aleisha Amohia 2017-06-02 03:38:17 UTC
Created attachment 63903 [details] [review]
Bug 12691: [FOLLOW-UP] Follow-up patch

This patch fixes merge conflicts and fixes the problems in Comment 7
Comment 9 Marc Véron 2017-08-03 10:19:12 UTC
(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).
Comment 10 Marc Véron 2017-08-04 09:49:05 UTC
(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.
Comment 11 Aleisha Amohia 2017-08-07 00:09:08 UTC
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
Comment 12 Aleisha Amohia 2017-08-07 00:30:28 UTC
Created attachment 65528 [details] [review]
Bug 12691: [FOLLOW-UP] Follow-up patch

This patch fixes merge conflicts and fixes the problems in Comment 7
Comment 13 Aleisha Amohia 2017-08-07 00:30:36 UTC
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
Comment 14 Aleisha Amohia 2017-08-07 00:32:20 UTC
There we go, merge conflicts fixed.
Comment 15 Marc Véron 2017-08-07 09:41:53 UTC
Can not test ATM because of Bug 19048
Comment 16 Marc Véron 2017-08-07 15:14:44 UTC
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') %]
Comment 17 Aleisha Amohia 2017-08-20 22:12:50 UTC
Created attachment 66252 [details] [review]
Bug 12691: [FOLLOW-UP] Missing bracket

Patch adds bracket to template file (Comment 16)
Comment 18 Marc Véron 2017-08-21 08:34:03 UTC
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>
Comment 19 Marc Véron 2017-08-21 08:35:52 UTC
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>
Comment 20 Marc Véron 2017-08-21 08:36:45 UTC
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>
Comment 21 Nick Clemens 2017-09-01 15:43:34 UTC
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>
Comment 22 Nick Clemens 2017-09-01 15:43:38 UTC
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>
Comment 23 Nick Clemens 2017-09-01 15:43:41 UTC
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>
Comment 24 Jonathan Druart 2017-09-06 16:26:23 UTC
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.
Comment 25 Aleisha Amohia 2017-09-06 22:57:13 UTC
(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.
Comment 26 Aleisha Amohia 2017-09-06 22:58:03 UTC
Created attachment 66906 [details] [review]
Bug 12691: [FOLLOW-UP] Fixing some logic
Comment 27 Marc Véron 2017-09-07 10:21:14 UTC
Created attachment 66926 [details] [review]
Bug 12691: [FOLLOW-UP] Fixing some logic

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 28 Jonathan Druart 2017-09-07 17:05:18 UTC
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.
Comment 29 Jonathan Druart 2017-09-07 18:06:23 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 30 Fridolin SOMERS 2017-10-10 09:55:56 UTC
Enhancement not pushed to 17.05.x