Opened 6 years ago

Closed 6 years ago

#1827 closed bug (fixed)

mass mail tool breaks HTML from words

Reported by: pablobd Owned by: shevek
Priority: blocker Milestone: 1.3
Component: unknown Keywords:
Cc: shevek, sanderr, planetcruiser, jsfan, laurentS, pablobd

Description

If i edit the newsletter from the massmail tool ie. http://www.bewelcome.org/bw/admin/adminmassmails.php?action=edit&IdBroadCast=33 it will break some of the html code, and add stuff, it should just keep it the way it is entered during translation using words ie. http://www.bewelcome.org/bw/admin/adminwords.php?idword=53113

Change History (33)

comment:1 Changed 6 years ago by crumbking

The problem is that the admin words tool save the linebreaks as </ br> in the database.

If we call the text in the admin massmail tool these linebreaks will be added...

As a fast fix remove all line breaks in the admin words tool in all languages and replace links with - and so on with a tinyurl?

Pablo could you gimme rights for the most important languages in the translation tool I could help you and check the code.

comment:2 Changed 6 years ago by pablobd

I just gave you rights for "all"

comment:3 Changed 6 years ago by pablobd

  • Priority changed from major to blocker

comment:4 Changed 6 years ago by crumbking

  • Cc shevek sanderr planetcruiser added

shevek, sanderr, planetcruiser

Could you help here?

We might could add a string replace in the massmail tool.

comment:5 follow-up: Changed 6 years ago by planetcruiser

why was this never an issue before? since when has behaviour changed? can we work around this without coding for now?

comment:6 Changed 6 years ago by shevek

When adding a massmail the content of the edit field is processed with wwinlang from bw\lib\lang.php. This uses nl2br and leads to a pretty ugly display at least.

And after each update the number of <br> is growing. I gather that the mail send in the end will look rather ugly.

wwinlang is mainly used in old code. The only place I found in new code is to check for spam keywords (and I'm not even sure that code is used). So maybe we just fix it on alpha and use that to update the post texts and use www to send it?

comment:7 in reply to: ↑ 5 Changed 6 years ago by globetrotter_tt

Replying to planetcruiser:

why was this never an issue before? since when has behaviour changed? can we work around this without coding for now?

I think it was always an issue. Have you never read the bw newsletter? :P

comment:8 Changed 6 years ago by crumbking

An work around would be to remove all linbreaks in admin word. As far as I tested this locally if there are no linebreaks the nl2br have nothing to do ;-)

So a non coding work around would be:

  • wait till the translation are finished
  • go into every translation and remove all linebreaks
  • fix some of the broken links ("Sonderzeichen" like "-" ->maybe add a tinyurls?)

But as this is a lot of work better we fix it as this won't be the last newsletter ;-)

comment:9 Changed 6 years ago by shevek

Might be related with #1848. (Another unfortunate use of nl2br...)

Last edited 6 years ago by shevek (previous) (diff)

comment:10 follow-up: Changed 6 years ago by pablobd

Our current NL is ready to go, I already erased all the line breaks from all translations, but the code keep breaking the URLs in links when they include a space. example

http://downloads.bewelcome.org/BV%20Finances%202011-2012.pdf

turns to

http://downloads.bewelcome.org/BV 0.000000inances2012.pdf

Last edited 6 years ago by pablobd (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 6 years ago by planetcruiser

  • Cc jsfan added

Replying to pablobd:

Our current NL is ready to go, I already erased all the line breaks from all translations, but the code keep breaking the URLs in links when they include a space. example

http://downloads.bewelcome.org/BV%20Finances%202011-2012.pdf

turns to

http://downloads.bewelcome.org/BV 0.000000inances2012.pdf

ok, that's bad. can we work around this for now by using short links (for example bewelco.me - i would set up a link shortener)?

but then we would need to make sure this ticket makes it to 1.3. jsfan, shevek: could we include this please? if you agree, please change the milestone field.

comment:12 Changed 6 years ago by shevek

  • Milestone changed from unassigned to 1.3
  • Owner set to shevek
  • Status changed from new to assigned

comment:13 Changed 6 years ago by shevek

Milestone set and I start working on it.

comment:14 Changed 6 years ago by shevek

Okay, found out why it happens. %20F is a valid code for formatting with sprintf. Therefore it is exchanged with 0.000000.

To get the newsletter out: Instead of %20 use %%20 in the links.

The general fix will be a wee bit more complicated.

Last edited 6 years ago by shevek (previous) (diff)

comment:15 Changed 6 years ago by shevek

Fixed the automatic replace of bewlines to <br> and the breaking links by avoiding the call to wwinlang (as contents where English only anyway).

(Commit: https://gitorious.org/bewelcome/rox/commit/555c99de227aff41298f5c95fc6873e568b3e4b3)

comment:16 Changed 6 years ago by shevek

Note: The massmail form says that the first %s is replaced by the user name. Which means that it is likely that sprintf is called on sending. Therefore links still need proper escaping against this! (Links that contain any %xx needs to be changed to %%xx).

This can be avoided by starting every newsletter with "Hi %s, I hope you're well :-)" (if the replacement really happens otherwise that looks rather stupid ;-))

comment:17 Changed 6 years ago by jsfan

Deployed on alpha.

comment:18 Changed 6 years ago by jsfan

Could someone with the appropriate permissions please test this on alpha?

comment:19 Changed 6 years ago by pablobd

I just tested, I added line breaks in the code and they appear in the final e-mail as <br> tags the double %% works fine and the %s with user name works too

comment:20 follow-up: Changed 6 years ago by shevek

Still waiting for a reply by jsfan or planetcruiser which function does the scheduling.

But seems that that function calls nl2br and sprintf. Someone else is rewriting the mailbot at the moment, so I'd relay that info to him.

But for the next newsletter at least we don't have multiplying <br> anymore.

comment:21 in reply to: ↑ 20 Changed 6 years ago by planetcruiser

Replying to shevek:

Still waiting for a reply by jsfan or planetcruiser which function does the scheduling.

sorry, it's difficult to keep up with all the activity at bw at the moment. ;)

yes, there is a cronjob that calls mailbot every 5 minutes. i assume it also takes care of newsletter emails.

# Run message delivery (mailbot) every 5 minutes
*/5 * * * * root /usr/local/rox/script/cron_mailbot.sh www.bewelcome.org

cron_mailbot.sh:

#!/bin/bash
cd /var/www/$1
env SERVER_NAME=$1
php htdocs/bw/mailbot.php

comment:22 Changed 6 years ago by shevek

Thanks, planetcruiser. And of course mailbot uses wwinlang to get the translated version of the newsletter.

comment:23 Changed 6 years ago by shevek

Updated htdocs\bw\lib\lang.php to offer a getBroadCastElement function that returns the translated version from the DB if necessary and replaces the placeholder %username% with the username of the current user. (Backwards compatibility retained by replacing %s with the username and %% with % just in case).

Updated htdocs/bw/mailbot.php to use this function.

Updated both htdocs/bw/admin/adminmassmails.php and htdocs/bw/layout/adminmassmails.php to use this function. Changed the labels of the edit fields for a broadcast to inform the user about %username%.

(Commit: https://gitorious.org/bewelcome/rox/commit/704c0be24f2a955d177f4729a0f69db999ccc291)

Could someone with the proper rights test this please as soon as it is on alpha?

comment:24 Changed 6 years ago by laurentS

  • Cc laurentS added

comment:25 Changed 6 years ago by planetcruiser

from the dev list by shevek:

I needed to change mailbot.php to get this fixed completely. Well, that one is called using a cron job which of course pull mailbot.php from www. So unfortunately not testable at all.

we will need to improvise here. could you get mailbot running locally with fakemail? if that test runs ok, i think we can use the patch on www. it's just minor string manipulation, right?

comment:26 Changed 6 years ago by shevek

I tested by echoing the strings that would be put into the mails. But even to get these I needed to hack as the only guy I expected to have the rights in the DB (admin) didn't have the rights to trigger massmail.

Honestly I don't expect trouble there. But one never knows. I try fakemail.

comment:27 Changed 6 years ago by shevek

Okay, fakemail didn't work. I setup hMailServer as suggested here: http://trac.bewelcome.org/wiki/GetStartedWithTheCode

Figured that I left the debug output in the code and that I didn't comment in the call to bw_mail again.

Commit: https://gitorious.org/bewelcome/rox/commit/ac15d4b8dccb53607d449470364253b17f1ef112

comment:28 Changed 6 years ago by jsfan

Deployed on alpha.

comment:29 Changed 6 years ago by globetrotter_tt

  • Cc pablobd added

How to test this? Should Pablo send out a test newsletter to some selected people?

comment:30 Changed 6 years ago by shevek

With difficulties... The problem is that the cron job calls the old code.

So test locally and if you find a problem shout ;-)

comment:31 Changed 6 years ago by crumbking

We might do not find people to test this locally. I can't as you know ;-)

Could someone write me a howto setup an SMTP server for email features on ubuntu?

comment:32 Changed 6 years ago by laurentS

I am testing mailbot using the two setups described here:

  • this one first to test completely locally
  • the second for relaying mail to the outside world via gmail smtp servers, as you seem to use their service as well

They both worked right away for me (on ubuntu 12.04), so they should be good :)

comment:33 Changed 6 years ago by jsfan

  • Resolution set to fixed
  • Status changed from assigned to closed

Tested locally and worked fine. Closing.

Note: See TracTickets for help on using tickets.