Bug 13448

Summary: runreport.pl html email striping
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: ReportsAssignee: Bugs List <koha-bugs>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kyle, liz, m.de.rooy, martin.renvoize, mtompset, vanoudt
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13447    
Bug Blocks:    
Attachments: Bug 13448: Make Striping gmail/hotmail compatible

Description Martin Renvoize 2014-12-12 13:07:13 UTC
As of Bug 10777 Galen added css to zebra stripe html emailed reports; unfortunately this was implemented in a way incompatible with both gmail and hotmail on-line email clients

This is reasonably easily solved by in-lining the styles as opposed to adding them to the page head.
Comment 1 Martin Renvoize 2014-12-12 13:12:49 UTC
Created attachment 34342 [details] [review]
Bug 13448: Make Striping gmail/hotmail compatible

This patch inlines the css to stripe a html output report
Comment 2 Mark Tompsett 2015-06-22 04:19:00 UTC
This isn't an enhancement. It is fixing a behaviour broken in particular contexts. Changed to 'minor' bug fix.
Comment 3 Mark Tompsett 2015-06-22 04:21:30 UTC
Comment on attachment 34342 [details] [review]
Bug 13448: Make Striping gmail/hotmail compatible

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

::: misc/cronjobs/runreport.pl
@@ +249,3 @@
>          while (my $line = $sth->fetchrow_arrayref) {
>              foreach (@$line) { defined($_) or $_ = ''; }    # catch undef values, replace w/ ''
> +            if ($count++ % 2 == 0) {

This is an ugly coding style!
Comment 4 Mark Tompsett 2015-06-22 04:46:01 UTC
Comment on attachment 34342 [details] [review]
Bug 13448: Make Striping gmail/hotmail compatible

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

::: misc/cronjobs/runreport.pl
@@ -270,4 @@
>          my $email = Koha::Email->new();
>          my %mail;
>          if ($format eq 'html') {
> -                $message = "<html><head><style>tr:nth-child(2n+1) { background-color: #ccc;}</style></head><body>$message</body></html>";

It makes sense to remove the <head>...</head>, but the <html><body>$message</body></html> should probably be left in tact. Otherwise, you get just <table>...</table> as your message.
Comment 5 Mark Tompsett 2015-06-22 04:47:30 UTC
Marking as failed qa because:
1) coding style which has multiple side effects in a single statement, but more importantly,
2) Only generating <table>...</table>, because of excessive cutting.
Comment 6 Jonathan Druart 2015-06-22 07:53:36 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 34342 [details] [review] [review]
> Bug 13448: Make Striping gmail/hotmail compatible
> 
> Review of attachment 34342 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/cronjobs/runreport.pl
> @@ +249,3 @@
> >          while (my $line = $sth->fetchrow_arrayref) {
> >              foreach (@$line) { defined($_) or $_ = ''; }    # catch undef values, replace w/ ''
> > +            if ($count++ % 2 == 0) {
> 
> This is an ugly coding style!

What's ugly? What do you suggest?
Comment 7 Mark Tompsett 2015-06-22 11:59:40 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to M. Tompsett from comment #3)
> > > +            if ($count++ % 2 == 0) {
> > 
> > This is an ugly coding style!
> 
> What's ugly? What do you suggest?

Incrementing should be external to the modulus comparison.
    $count++;
    if ($count%2==1) {
This is more readable, and less likely to be misunderstood.