Bug 6030 - Allow for html in letters in overdue notices
Summary: Allow for html in letters in overdue notices
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P3 critical (vote)
Assignee: Jonathan Druart
QA Contact: Mason James
URL:
Keywords:
: 5878 (view as bug list)
Depends on:
Blocks: 5992
  Show dependency treegraph
 
Reported: 2011-03-31 22:06 UTC by Chris Cormack
Modified: 2015-01-06 12:58 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (2.41 KB, patch)
2011-03-31 22:22 UTC, Chris Cormack
Details | Diff | Splinter Review
proposed patch (2.41 KB, patch)
2011-08-31 14:12 UTC, Paul Poulain
Details | Diff | Splinter Review
proposed patch follow-up (8.49 KB, patch)
2011-08-31 14:14 UTC, Paul Poulain
Details | Diff | Splinter Review
Alternative patch-set to send html emails (2.66 KB, application/x-zip-compressed)
2011-11-08 08:12 UTC, Nicholas van Oudtshoorn
Details
SQL required for the previous patch-set (117 bytes, application/octet-stream)
2011-11-08 08:39 UTC, Nicholas van Oudtshoorn
Details
Bug 6030 follow-up (8.66 KB, patch)
2012-09-10 16:11 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6030 followup : add a condition to use getlanguage in bath mode (764 bytes, patch)
2012-09-10 16:11 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6030 followup : Specify "biblio.title" as a column name to differentiate from borrowers.title (1.14 KB, patch)
2012-09-10 16:11 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6030 Allow html content in letters (2.43 KB, patch)
2012-09-10 16:22 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6030 followup : Specify "biblio.title" as a column name to differentiate from borrowers.title (1.25 KB, patch)
2012-09-14 14:28 UTC, Mason James
Details | Diff | Splinter Review
Bug 6030 Allow html content in letters (2.50 KB, patch)
2012-09-14 14:30 UTC, Mason James
Details | Diff | Splinter Review
Bug 6030 followup : add a condition to use getlanguage in bath mode (830 bytes, patch)
2012-09-14 14:35 UTC, Mason James
Details | Diff | Splinter Review
Bug 6030 follow-up (8.73 KB, patch)
2012-09-14 14:35 UTC, Mason James
Details | Diff | Splinter Review
SIGNED-OFF-Bug-6030-followup-Specify-biblio.title-as.patch (1.25 KB, patch)
2012-09-20 15:08 UTC, wajasu
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6030 Allow html content in letters (2.53 KB, patch)
2012-10-05 13:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6030 followup : add a condition to use getlanguage in bath mode (859 bytes, patch)
2012-10-05 13:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6030 follow-up (8.76 KB, patch)
2012-10-05 13:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6030 followup : Specify "biblio.title" as a column name to differentiate from borrowers.title (1.33 KB, patch)
2012-10-05 13:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6030 follow-up (2.23 KB, patch)
2012-10-11 13:13 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6030 Allow html content in letters (10.99 KB, patch)
2012-10-24 09:10 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6030 [3.8.x] Allow html content in letters (11.06 KB, patch)
2012-10-24 12:06 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6030 Allow html content in letters (11.05 KB, patch)
2012-12-14 14:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6030 Allow html content in letters (11.05 KB, patch)
2012-12-14 14:53 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-03-31 22:06:34 UTC

    
Comment 1 Chris Cormack 2011-03-31 22:22:14 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2011-07-06 15:59:38 UTC
I tested a few HTML tags and it appears to work, it should probably be tested again and get a second sign off.
Comment 3 Paul Poulain 2011-08-09 15:57:02 UTC
doing some tests, I get:
17:47 ~/koha.dev/koha-community (BZ6030 $%)$ misc/cronjobs/overdue_notices.pl --html=test-html
Global symbol "$columns_def_hashref" requires explicit package name at misc/cronjobs/overdue_notices.pl line 486.
Execution of misc/cronjobs/overdue_notices.pl aborted due to compilation errors.


Digging into biblibre/master, I see this script has more lines :

my $columns_def_hashref = C4::Reports::Guided::_get_column_defs();

foreach my $key ( keys %$columns_def_hashref ) {
    my $initkey = $key;
    $key =~ s/[^\.]*\.//;
    $columns_def_hashref->{$initkey}=NormalizeString($columns_def_hashref->{$initkey});
    $columns_def_hashref->{$initkey}=~s# #<br/>#;
    $columns_def_hashref->{$key} = $columns_def_hashref->{$initkey};
}

I think adding them will result in the script working, but without this, it can't work.

Nicole = what kind of tests did you do ?
Comment 4 Nicole C. Engard 2011-08-09 21:07:19 UTC
All my tests are purely cosmetic.  The email generated with HTML in it. I did not send the emails (cause I don't have email sending turned on) and I don't read the server logs or anything.  

Nicole
Comment 5 Paul Poulain 2011-08-31 14:12:52 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2011-08-31 14:14:47 UTC Comment hidden (obsolete)
Comment 7 Nicholas van Oudtshoorn 2011-11-08 08:12:45 UTC Comment hidden (obsolete)
Comment 8 Nicholas van Oudtshoorn 2011-11-08 08:39:24 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2012-01-06 15:14:34 UTC
Ok, I am confused here - perhaps someone can clarify: why is this failed qa and what is the relation to 5878 (that I spent a lot of time testing on)?
Comment 10 Paul Poulain 2012-01-09 19:46:30 UTC
(In reply to comment #9)
> Ok, I am confused here - perhaps someone can clarify: why is this failed qa and
> what is the relation to 5878 (that I spent a lot of time testing on)?

Reading the bug history, I can't explain why I've set failed QA. I feel I made the change without noticing it.

Switching back to need signoff.

Nicholas : could you confirm your alternative patch & SQL required are both an alternative option ? In this case i'll mark them as obsolete & continue with my initial patches
Comment 11 Nicholas van Oudtshoorn 2012-01-10 01:44:33 UTC
Hi Paul,

Yes - they're an alternative option. (A quick solution I threw together so that our library could send nice-looking email reminders out! - I'm more than happy for an official solution - this was just a "hey - this is how we've solved it" when I noticed that the problem was being worked on....)
Comment 12 Katrin Fischer 2012-01-15 20:06:29 UTC
Follow up does not apply:

[~/kohaclone (6030_htmloverdues)⚡]> git am -iu3 /tmp/proposed-patch-follow-up-jH3H5e.patch
Commit Body is:
--------------------------
Bug 6030 follow-up

Fixing problem detected previously:
* add column_def retrieval, to have header displaying the "explained" row name, not the table name. (the name is not expanded for CSV exports)
* removed some warnings.
* fixed some problems with csvfilename being defined/empty. Now, csvfilename is always defined, and set to '' by default. Some tests have been updated accordingly
* added the charset=UTF8 on mail headers.

This patch also adds some missing columns to columns.def
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6030 follow-up
fatal: sha1 information is lacking or useless (misc/cronjobs/overdue_notices.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 6030 follow-up
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort"
Comment 13 Katrin Fischer 2012-01-15 20:07:25 UTC
Paul, can you clear up the relationship to bug 5878? - Can this bug be closed as features will be implemented incrementally by other bugs/patches?
Comment 14 Paul Poulain 2012-01-18 11:23:22 UTC
*** Bug 5878 has been marked as a duplicate of this bug. ***
Comment 15 Sophie MEYNIEUX 2012-09-10 16:11:25 UTC Comment hidden (obsolete)
Comment 16 Sophie MEYNIEUX 2012-09-10 16:11:33 UTC Comment hidden (obsolete)
Comment 17 Sophie MEYNIEUX 2012-09-10 16:11:46 UTC Comment hidden (obsolete)
Comment 18 Sophie MEYNIEUX 2012-09-10 16:22:03 UTC Comment hidden (obsolete)
Comment 19 Sophie MEYNIEUX 2012-09-10 16:30:28 UTC
I have signed off the first 2 patches and added 2 small follow-up : 
  * the first one is need because when $query is not defined, themelanguage returns an error
  * the seconf one because in french we need to differentiate borrowers.title and biblio.title (it's not the same translation)

I don't know why the main path is the fourth file sended : in consequence, to test, you should apply 4th patch first and then 1, 2 and 3
Comment 20 Mason James 2012-09-14 14:28:52 UTC Comment hidden (obsolete)
Comment 21 Mason James 2012-09-14 14:30:39 UTC Comment hidden (obsolete)
Comment 22 Mason James 2012-09-14 14:35:33 UTC Comment hidden (obsolete)
Comment 23 Mason James 2012-09-14 14:35:58 UTC Comment hidden (obsolete)
Comment 24 Mason James 2012-09-14 14:39:26 UTC
> Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>

i rebased these 4 patches to apply to master
patches looks good, passing QA… nice work Sophie!

$ koha-qa.pl  -c 4

testing 4 commit(s) (applied to commit 0acebb8)
 * e34fa8f Bug 6030 followup : add a condition to use getlanguage in bath mo
      C4/Templates.pm

 * cb90e51 Bug 6030 follow-up
      koha-tmpl/intranet-tmpl/prog/en/columns.def
      misc/cronjobs/overdue_notices.pl

 * a346838 Bug 6030 followup : Specify "biblio.title" as a column name to di
      misc/cronjobs/overdue_notices.pl

 * 8ff05be Bug 6030 Allow html content in letters
      misc/cronjobs/overdue_notices.pl

* C4/Templates.pm                                                          OK
* misc/cronjobs/overdue_notices.pl                                         OK
Comment 25 Paul Poulain 2012-09-17 16:14:58 UTC
Running the script says:
DBD::mysql::st execute failed: Unknown column '$date' in 'field list' at ./overdue_notices.pl line 505.
DBD::mysql::st fetchrow_hashref failed: fetch() without execute() at ./overdue_notices.pl line 518.

Investigating a little, show that line 505 is running $sth2, that contains:
    my $sth2 = $dbh->prepare( <<'END_SQL' );
SELECT biblio.title as "biblio.title",  biblio.*, items.*, issues.*, biblioitems.itemtype, TO_DAYS($date)-TO_DAYS(date_due) AS days_overdue
  FROM issues,items,biblio, biblioitems
  WHERE items.itemnumber=issues.itemnumber
    AND biblio.biblionumber   = items.biblionumber
    AND biblio.biblionumber   = biblioitems.biblionumber
    AND issues.borrowernumber = ?
    AND TO_DAYS($date)-TO_DAYS(date_due) BETWEEN ? and ?
END_SQL

the line:
    AND TO_DAYS($date)-TO_DAYS(date_due) BETWEEN ? and ?

is heavily wrong !

The signoff was probably right when it was made, but time passed, things were updated, and now it's invalid !
Comment 26 wajasu 2012-09-20 15:08:43 UTC Comment hidden (obsolete)
Comment 27 wajasu 2012-09-20 17:40:35 UTC
The
Bug 6030 follow-up 
needs to be adjusted so the patch will apply.
Comment 28 Magnus Enger 2012-10-02 10:02:15 UTC
(In reply to comment #27)
> The
> Bug 6030 follow-up 
> needs to be adjusted so the patch will apply.

I'm not sure I understand which follow-up needs to be adjusted?
Comment 29 wajasu 2012-10-02 19:23:47 UTC
The third one, is the one I believe I had problems applying.
attachment 12235 [details] [review]
Comment 30 Magnus Enger 2012-10-04 13:12:41 UTC
As far as I can tell, attachments 12232, 12235 and 12397 have signoffs, but not 12234. Having a unified signoff on all of them would probably be good. 

I just did "git bz apply 6030" on current master and all patches applied, so I'm resetting to "Needs signoff", but I wont have time to do a signoff now myself, so please feel free... :-)
Comment 31 Kyle M Hall 2012-10-05 13:01:32 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2012-10-05 13:01:51 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2012-10-05 13:02:09 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2012-10-05 13:02:27 UTC Comment hidden (obsolete)
Comment 35 Mason James 2012-10-09 03:28:17 UTC
(In reply to comment #34)
> Created attachment 12701 [details] [review]
> [SIGNED-OFF] Bug 6030 followup : Specify "biblio.title" as a column name to
> differentiate from borrowers.title
> 
> Signed-off-by: wajasu <matted-34813@mypacks.net>
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>


4 patches look good, passing QA on this...

$ koha-qa.pl -c 4 

testing 4 commit(s) (applied to commit 37340e3)
 * 4403c59 Bug 6030 followup : Specify "biblio.title" as a column name to differentiate from borrowers.title
      misc/cronjobs/overdue_notices.pl

 * a9a93a7 Bug 6030 follow-up
      koha-tmpl/intranet-tmpl/prog/en/columns.def
      misc/cronjobs/overdue_notices.pl

 * 5fc93ff Bug 6030 followup : add a condition to use getlanguage in bath mode
      C4/Templates.pm

 * 437886a Bug 6030 Allow html content in letters
      misc/cronjobs/overdue_notices.pl

* misc/cronjobs/overdue_notices.pl                                         OK
* C4/Templates.pm                                                          OK
Comment 36 Paul Poulain 2012-10-11 13:13:10 UTC Comment hidden (obsolete)
Comment 37 Paul Poulain 2012-10-11 13:16:33 UTC
I don't think this patch is just an ENH, sor RMaint, your choice, but this patch should be OK in 3.8 imo

Examining possible side effect before pushing:
* The patch contains:
-                my $titles = "";
+                my $titles;
=> I think it's a bad idea. If you have the html parameter, title will be filled by
 if ($htmlfilename) {
     $titles = "<table id='itemscontent$borrowernumber'>";
but, if you don't have html, it will stay empty, and the line
                        $titles .= join("\t", @item_info) . "\n";
will issue an error

I've reintroduced the = '';

* the patch add:
 $letter->{'content-type'}="text/".($htmlfilename?"html":"plain")."; charset=UTF8";

=> having a content-type is always ++
and it's the correct one I think if we don't have html

* testing the script:
 - I get a bad
DBD::mysql::st execute failed: Unknown column '$date' in 'field list' at misc/cronjobs/overdue_notices.pl line 505.

I've attached a follow-up

2nd try = I can't get anything in the generated file.
Running the script 
misc/cronjobs/overdue_notices.pl --html=test-html -v

tells me that I've one patron late:
returns 1 rows at misc/cronjobs/overdue_notices.pl line 490.
borrower firstname, TEST (7852) has items triggering level 1. at misc/cronjobs/overdue_notices.pl line 496.
STH2 PARAMS: borrowernumber = 7852, mindays = 1, maxdays = 90 returns 1 rows at misc/cronjobs/overdue_notices.pl line 506.

but the generated HTML is empty:
html>
<head>
<style type='text/css'>
pre {page-break-after: always;}
pre {white-space: pre-wrap;}
pre {white-space: -moz-pre-wrap;}
pre {white-space: -o-pre-wrap;}
pre {word-wrap: break-work;}
</style>
</head>
<body>
</body>
</html>

The notice defined for this patron is ODUE, i've checked the "html" on tools/letters.pl

so... any idea why I can't make it work welcomed.

I also think a squashed patch (with an updated git patch comment) would be more than welcomed. Those 5 patches are looking poor !
Comment 38 Sophie MEYNIEUX 2012-10-24 09:10:41 UTC Comment hidden (obsolete)
Comment 39 Sophie MEYNIEUX 2012-10-24 12:06:39 UTC
Created attachment 13018 [details] [review]
Bug 6030 [3.8.x] Allow html content in letters

Squashed commit of the following:

commit 8368e5de723ec7607402be341975014c8b7b0ea9
Author: Henri-Damien LAURENT <henridamien.laurent@biblibre.com>
Date:   Tue Sep 21 14:41:07 2010 +0200

    Bug 6030 : overdue_notices :Removing warning

    Two extra warnings were issued when using html

commit 5984fc92772af68c51f7f5b5450fd5b27d5b6d37
Author: Henri-Damien LAURENT <henridamien.laurent@biblibre.com>
Date:   Tue Sep 21 12:27:29 2010 +0200

    Bug 6030 : Letters can be html in overdu_notices.pl

Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>

Bug 6030 follow-up

Fixing problem detected previously:
* add column_def retrieval, to have header displaying the "explained" row name, not the table name. (the name is not expanded for CSV exports)
* removed some warnings.
* fixed some problems with csvfilename being defined/empty. Now, csvfilename is always defined, and set to '' by default. Some tests have been updated accordingly
* added the charset=UTF8 on mail headers.

This patch also adds some missing columns to columns.def

Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>

 Bug 6030 followup : add a condition to use getlanguage in bath mode

Bug 6030 followup : Specify "biblio.title" as a column name to differentiate from borrowers.title

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Sophie MEYNIEUX 2012-10-24 12:12:37 UTC
I have squashed all the followup in one patch en send a specific pathc for 3.8.x
Each patch was signed-off individually, but maybe the global patch should be validated again.
Comment 41 Sophie MEYNIEUX 2012-10-30 09:56:53 UTC
I have changed the status of this ticket. This is not an enhancement, but a bugfix.
The feature to create html file exist in overdue_notices.pl but it does not work withour this patch.
Comment 42 Katrin Fischer 2012-10-30 10:06:38 UTC
Hi Sophie, 

can you provide a test plan for this? I am not sure what this patch includes as it seems to do a lot of things.
Comment 43 Katrin Fischer 2012-10-30 10:45:27 UTC
Hi Sophie, taking a look at your patch.

First thing I notice is that it also seems to touch the CSV export option. So this needs to be retested as well?

I am worried about using the guided reports .pref files for the items table as those are known to not be translatable with the scripts. 

Also wondering if we really want to output a table. I think this should be optional as it's a bigger change in behaviour. This seems to be the old output:

$return = "<pre>\n";
$return .= "$params->{'letter'}->{'content'}\n";
$return .= "\n</pre>\n";

I am also wondering about the syntaxes in the overdue letter to be used. Does it work with both <<items.content>> and the <item><fine></fine></item> syntax?

Hope I didn't misread.
Comment 44 Sophie MEYNIEUX 2012-11-23 13:58:43 UTC
Katrin, 

Concerning table headers, we have defined a french .pref file that we copy in french directory each time we execute a translation. I agree it's not so good, but we need to have french headers for our customers. Do you have any better suggestion ?

Else for the test plan, without the patch, if you use overdue_notices.pl with -html parameter, html file generated doesn't contain any HTML tags between <pre> and </pre>, so the .pdf generated misc/cronjobs/printoverdues.sh contains only plain text without layout.

If you apply the patch, then html file contains html tags and pdf file can then be generated with a nice layout. 

As far as csv parameter is concerned, I dedn't change anything from proposed patches that where signed off and that I squashed. But I asssume it should be better to test that nothing is broken.
Comment 45 Kyle M Hall 2012-12-14 14:53:07 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2012-12-14 14:53:35 UTC
Created attachment 14129 [details] [review]
[SIGNED-OFF] Bug 6030 Allow html content in letters

Squashed commit of the following:

commit 8368e5de723ec7607402be341975014c8b7b0ea9
Author: Henri-Damien LAURENT <henridamien.laurent@biblibre.com>
Date:   Tue Sep 21 14:41:07 2010 +0200

    Bug 6030 : overdue_notices :Removing warning

    Two extra warnings were issued when using html

commit 5984fc92772af68c51f7f5b5450fd5b27d5b6d37
Author: Henri-Damien LAURENT <henridamien.laurent@biblibre.com>
Date:   Tue Sep 21 12:27:29 2010 +0200

    Bug 6030 : Letters can be html in overdu_notices.pl

Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>

Bug 6030 follow-up

Fixing problem detected previously:
* add column_def retrieval, to have header displaying the "explained" row name, not the table name. (the name is not expanded for CSV exports)
* removed some warnings.
* fixed some problems with csvfilename being defined/empty. Now, csvfilename is always defined, and set to '' by default. Some tests have been updated accordingly
* added the charset=UTF8 on mail headers.

This patch also adds some missing columns to columns.def

Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>

 Bug 6030 followup : add a condition to use getlanguage in bath mode

Bug 6030 followup : Specify "biblio.title" as a column name to differentiate from borrowers.title

Conflicts:

	misc/cronjobs/overdue_notices.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Kyle M Hall 2012-12-14 14:54:36 UTC
I think this needs to move forward to QA for the patch for master. There is little sense in having this held back by the lack of an SO for the 3.8.x versioned patch.
Comment 48 Jared Camins-Esakov 2013-01-16 03:48:23 UTC
I am entirely perplexed about what this patch is supposed to do and why it is marked critical. Based on the information on this report this sounds like at most a "minor" bug.

Also, while I have this bully pulpit, could one of the original authors address Katrin's question about the CSV export changes?
Comment 49 Sophie MEYNIEUX 2013-02-14 13:59:14 UTC
Hi Jared, 

Without this patch, overdues notices are not correctly generated in html, so pdf file could not be produced.
It is critical for our clients because they used to print overdue letters, and they can't do it anymore when upgrading to 3.8.

There were several patches and follow-up provided. I only provide correction for the remaining problems and sqashed the already signed off patches.
So concerning Katrin question about csv, modifications where in those primitive patches. 

Tell me if I should begin again from scratch an propose a new patch only dealing with html. Our librairies really need this funcitonality.
Comment 50 Jared Camins-Esakov 2013-02-14 15:14:38 UTC
(In reply to comment #49)
> Without this patch, overdues notices are not correctly generated in html, so
> pdf file could not be produced.
> It is critical for our clients because they used to print overdue letters,
> and they can't do it anymore when upgrading to 3.8.

So this is a feature that used to work?

> There were several patches and follow-up provided. I only provide correction
> for the remaining problems and sqashed the already signed off patches.
> So concerning Katrin question about csv, modifications where in those
> primitive patches. 

I guess hdl is the original author, and therefore no longer available to comment? I looked at the code, and it looks like the CSV-related changes may have been simply to quiet warnings, in which case, since it is an old patch, I have no objection to it remaining in place, provided the CSV export is tested.
Comment 51 Derryn 2013-02-19 06:31:16 UTC
After I applied this patch on my 3.10.02 setup, my overdue notices go out as plain text, but still with the HTML markup applied.

After opening the email and changing:
Content-Type: text/plain; charset=UTF8
to
Content-Type: text/html; charset=UTF8

It displays perfectly!

Also, and I'm not sure if this is related, but overdue notices also seem not to respect the KohaAdminEmailAddress preference, instead using the default library email address.
Comment 52 Sophie MEYNIEUX 2013-03-01 14:47:13 UTC
Derrin, if you use -html option, you have to check html on you notice template in order your email get the right content type.

And KohaAdminEmailAddress is only used if Library email address is not set. This was the previous way to send overdues and this patch does not change that.
Comment 53 Derryn 2013-03-02 00:34:18 UTC
(In reply to comment #52)
> Derrin, if you use -html option, you have to check html on you notice
> template in order your email get the right content type.
> 
> And KohaAdminEmailAddress is only used if Library email address is not set.
> This was the previous way to send overdues and this patch does not change
> that.

This is without the -html option. I'm not outputting them to a file, which the -html option is for; I'm making them to email.
Comment 54 Katrin Fischer 2013-03-27 06:47:07 UTC
I will try to test this, but a step by step test plan would be a great help in making this bug move.
Comment 55 Marcel de Rooy 2013-03-28 13:10:35 UTC
Several conflicts in overdue_notices.pl
Comment 56 Eric Bégin 2013-10-11 19:54:04 UTC
Some email clients doesn't support UTF-8 encoding, should the charset be a system preference ? (emailCharset or emailEncoding)
Comment 57 mathieu saby 2013-10-12 13:49:07 UTC
I'm a big lost. Is this bug for 3.8.x or for master ?

Mathieu
Comment 58 Bob Birchall 2013-11-05 04:36:25 UTC
Where is this up to?  Its marked 'Does not apply'.  If it can be re-based, I'm up for testing it.
Comment 59 Sophie MEYNIEUX 2013-11-15 16:24:19 UTC
As this patch seemed difficult to sign, I have made several small patches, each dealing with one problem (list in "See also")

I will mark this patch as resolved when all those patches will be pushed
Comment 60 M. Tompsett 2014-04-09 04:26:35 UTC
Comment on attachment 13018 [details] [review]
Bug 6030 [3.8.x] Allow html content in letters

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

::: misc/cronjobs/overdue_notices.pl
@@ +344,4 @@
>  
>  our $csv;       # the Text::CSV_XS object
>  our $csv_fh;    # the filehandle to the CSV file.
> +if ( $csvfilename ) {

No, because the file name could be "0". Old code is better.

@@ +563,4 @@
>                              email          => $email,
>                              itemcount      => $itemcount,
>                              titles         => $titles,
> +                            outputformat   => $csvfilename ? 'csv' : defined $htmlfilename ? 'html' : '',

This nested mess should be made clearer elsewhere. And no, defined $csvfilename should be used.

@@ +591,4 @@
>                                  email          => $email,
>                                  itemcount      => $itemcount,
>                                  titles         => $titles,
> +                                outputformat   => $csvfilename ? 'csv' : defined $htmlfilename ? 'html' : '',

This nested mess should be made clearer elsewhere. And no, defined $csvfilename should be used.

@@ +602,4 @@
>      }
>  
>      if (@output_chunks) {
> +        if ( $csvfilename ) {

This nested mess should be made clearer elsewhere. And no, defined $csvfilename should be used.
Comment 61 Katrin Fischer 2015-01-06 12:24:40 UTC
Looking at critical bugs I noticed this one - does the problem still exist and can we get the patch back into the queue?
Comment 62 Sophie MEYNIEUX 2015-01-06 12:57:13 UTC
As I said in comment #59 (http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6030#c59), this bug was splitted in many small one.
It seems that all attached bugs are pushed except Bug 11173 (that won't apparently). So I think it could be closed.
Comment 63 Katrin Fischer 2015-01-06 12:58:57 UTC
Hi Sophie, thx for clarifying! I am closing this then.