Opened 6 years ago

Closed 4 years ago

#1853 closed improve feature (fixed)

upgrade mailbot to use MOD_mail

Reported by: laurentS Owned by: laurentS
Priority: major Milestone: 2.6
Component: BW Mail Keywords:
Cc: shevek

Description

Opening this ticket to keep track of my work on moving all email sending operations to use MOD_mail::sendEmail() so that all emails look the same, and future redesigns are easier.

Current strategy is to rewrite the mailbot with cleaner code, so it still runs as a cron job, but all emails are sent via the same (code) route.

Attachments (7)

msg_senderview01.png (22.5 KB) - added by Tsjoek 4 years ago.
msg_notification02.png (17.9 KB) - added by Tsjoek 4 years ago.
msg_notification03.png (13.8 KB) - added by Tsjoek 4 years ago.
msg_senderview03.png (19.7 KB) - added by Tsjoek 4 years ago.
msg_notification04.png (12.2 KB) - added by Tsjoek 4 years ago.
msg_notification01.png (14.2 KB) - added by Tsjoek 4 years ago.
You have received a message from member-1223.mbox (14.7 KB) - added by shevek 4 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 6 years ago by mahouni

If I understand correctly: you are replacing ./htdocs/bw/lib/mailer.php, which seems to be used for the newsletter at the moment, with tools/mailbot/mailbot.class.php?

Is

require_once "htdocs/bw/lib/init.php";

still needed then?

comment:2 Changed 6 years ago by laurentS

In an ideal world, I'd get rid of the require_once you mention, but I have no idea what "new" functions I should be using instead. If you can point me in the right direction (maybe there's a wiki page? I didn't find anything beyond a description of MOD_words), I can probably get rid of the call to init.php.

And I'm replacing /htdocs/bw/mailbot.php with the /tools/mailbot/mailbot.class.php. But the idea is that mailer.php isn't used anymore, so that all email is sent through MOD_mail::sendEmail.

comment:3 Changed 6 years ago by jsfan

  • Milestone changed from unassigned to 1.5

comment:4 Changed 6 years ago by jsfan

laurentS, do you have your patch available in a clone somewhere? I could probably take over if you cannot get this ready for 1.5.

comment:6 Changed 6 years ago by laurentS

@jsfan:

the latest code is in the mailbot-fixes branch. Very much appreciate if you can take this over.

Shevek mentioned a couple issues on the mailing list:

Forum notification don't work correctly:
- Even after the grace period no mails are sent.
- The plain text part contains HTML instead of the plain text.

I can't reproduce the first one, emailing works fine for me. The second I overlooked, but I don't think it requires much work to fix.

Also, you may want to revert the part of https://gitorious.org/~bamse207/bewelcome/bamse207s-rox/commit/c275dce1ad4d0a76c2ea72c76174cde0187aa10a on FunctionsTools?.php so that the old mailbot works, just in case anything goes wrong when going live. (I didn't alter the old mailbot at all beyond that single commented line)

I'll do my best to answer questions if you have any before I disappear. Good luck!

comment:7 Changed 6 years ago by jsfan

  • Milestone changed from 1.5 to unassigned

Removed from 1.5. Just not urgent enough.

comment:8 Changed 5 years ago by jsfan

  • Cc shevek added

comment:9 Changed 4 years ago by shevek

  • Milestone changed from unassigned to 2.6
  • Status changed from new to local_testing

Thanks to the work of LaurentS this was rather simple to put into place finally.

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

As a side effect the message text area is now a TinyMCE as well.

This includes a new preference to disable sending of HTML mails to members that dislike them.

You need to run the following SQL query to get that in place:


INSERT INTO 
	`preferences` 
	(`id`, `position`, `codeName`, `codeDescription`, `Description`, `created`, `DefaultValue`, `PossibleValues`, `EvalString`, `Status`) 
VALUES 
	(NULL, '51', 'PreferenceHtmlMails', 'PreferenceHtmlMailsDesc', 'This allows the member to choose the format of the messages send by bewelcome.org. Defaults to HTML', CURRENT_TIMESTAMP, 'Yes', 'Yes;No', '', 'Normal');
Last edited 4 years ago by shevek (previous) (diff)

comment:10 Changed 4 years ago by shevek

To call the new mailer class use

php tools/mailbot/mailbot.class.php

comment:11 Changed 4 years ago by Tsjoek

After the last step of signup I get a:

Fatal error: Class 'Swift_Message' not found in [...]\modules\mail\lib\mail.lib.php on line 71

Related to this ticket?

comment:12 Changed 4 years ago by shevek

Yes, but not a bug :-)

You need to do a composer update --no-dev as the Swift mailer is now fetched from composer.

comment:13 Changed 4 years ago by Tsjoek

Yup, that solves it. So actually it's related to #2226 ;-)

comment:14 Changed 4 years ago by Tsjoek

if "Disable the WYSIWYG editor" is set to "I want to use it", the text of a sent message remains empty in the db. I get the following message:

Notice: Undefined variable: html in [...]\build\messages\messages.model.php on line 526

comment:15 Changed 4 years ago by shevek

Fixed the undefined variable. Additionally fixed some wrong calls to send mail.

https://gitorious.org/bewelcome/rox/commit/656eb56e72e8fce686b85e00287af7fdd5dff54c

Still needs some work to ensure that all instances feed in a valid HTML text to sendmail.

comment:16 Changed 4 years ago by shevek

  • Status changed from local_testing to to_alpha

comment:17 Changed 4 years ago by shevek

  • Status changed from to_alpha to testing

comment:18 Changed 4 years ago by shevek

  • Status changed from testing to to_beta

comment:19 Changed 4 years ago by shevek

  • Status changed from to_beta to needs_work

comment:20 Changed 4 years ago by shevek

  • Status changed from needs_work to to_alpha

comment:21 Changed 4 years ago by shevek

  • Status changed from to_alpha to alpha

comment:22 Changed 4 years ago by shevek

test

comment:23 Changed 4 years ago by shevek

  • Status changed from alpha to testing

Changed 4 years ago by Tsjoek

Changed 4 years ago by Tsjoek

Changed 4 years ago by Tsjoek

Changed 4 years ago by Tsjoek

Changed 4 years ago by Tsjoek

comment:24 Changed 4 years ago by Tsjoek

  • Status changed from testing to needs_work

Replying to shevek:

As a side effect the message text area is now a TinyMCE as well.

I think the option to include images should be disabled (or at least for now). The layout of the produced mails is still very poor. I've received the mails in Thunderbird, of course the mailclient can also highly influence how things look.

First test (through alpha) Sender wants WYSIWYG, Receiver wants HTML

Confirmation for sender does not contain all formatting see screendump. Click on 'edit message' brings a few back, but underline and strikethrough are still missing. Mailnotification for receiver looks ugly in my setup (screendump):

  • 'all messages with this member' (2x) should be removed
  • Some layout is missing also here
  • The actions are missing
  • We used to have a yellow background (although I wasn't too thrilled about it)

Receiver view on the website is identical to senderview.

Second test Sender wants WYSIWYG, Receiver wants Plain Text

Senderconfirmation is equal to first test. Receivernotification, see screendump. Apart from some missing layout:

  • Bold gets capitalized (may be intentional?)
  • Italic gets underlined
  • Numlist gets bulleted
  • Internal links are not absolute
  • Disclaimer is missing

Third Test Sender wants Plain Text, Receiver wants Plain Text

Senderview has similar symptoms as above. On top of that, After clicking edit it is not the same anymore:

  • The numbered list has changed into asterisks
  • Both lists are indented which they weren't when I typed them.
  • The reference to the link has been added in brackets

Receivernotification has the same symptoms plus: external link is linkified and also linked in the footer. We should choose to do one or the other.

Fourth Test Sender wants Plain Text, Receiver wants HTML

No additional remarks

EDIT: Removed observation that was due to clientside preferences. Removed observation about numbered lists in html, they worked in test 1 as well. I don't seem to be able to remove attachments from trac, please ignore msg_notification04.png

Last edited 4 years ago by Tsjoek (previous) (diff)

Changed 4 years ago by Tsjoek

comment:25 Changed 4 years ago by shevek

Do you allow Thunderbird to download images from BW?

This is what I see

http://trac.bewelcome.org/raw-attachment/wiki/SandBox/shevek/bwmessage.png

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

comment:26 Changed 4 years ago by Tsjoek

I don't, so tried not to make remarks about that. But generally TB asks if it should download them and it didn't for these mails.

So the yellow color is replaced by this layout for all emails? Looks better, but probably there are more people on BW who don't download images by default. I know it's terribly hard to get emails nicely layouted on all configs.

comment:27 follow-up: Changed 4 years ago by shevek

Regarding receiver wants plain text that is obviously Thunberbird interfering, because the picture obviously isn't plain text.

The text only display for me looks like this:

http://trac.bewelcome.org/raw-attachment/wiki/SandBox/shevek/bwplaintext.png

(Note: The highlighted links are done as well by the mail program.)

Underlines and strikethrough aren't supported by MarkDown? so the tags around are just removed.

comment:28 in reply to: ↑ 27 Changed 4 years ago by Tsjoek

Replying to shevek:

Underlines and strikethrough aren't supported by MarkDown so the tags around are just removed.

Which is perfectly acceptable if the receiver wants a plain text notification. But then the bold and the italic should be ignored as well, because the capitals and underlining can be differently interpreted from what was originally intended.

The senderview, the re-edit and receiverview on the website shouldn't be affected by MarkDown. In other words: first save to DB, then MarkDown if applicable.

Note that both of the testexamples you have given so far are not messages, maybe a difference, maybe not.

comment:29 Changed 4 years ago by shevek

Removing bold and Italic doesn't make sense to me. Then I would either strip all HTML tags or not allow plain text message. I expect that people who select 'plain text' message also know what markdown is :-) Additionally the formatting is visible in BW. I check if that's really the case due to your comment.

Find attached a message with both HTML and text part.

comment:30 Changed 4 years ago by shevek

Fixed with: https://gitorious.org/bewelcome/rox/commit/6428c4ab919def8c1dc334534ebccc6b190dd33a

ol.li und ul.li where set to list-style-type: none and therefore the structure wasn't visible.

Removed the possibility to add an image from tiny_mce.

Introduced a new HTMLPurifier config so that messages can be handled differently if some wishes that.

comment:31 Changed 4 years ago by shevek

  • Status changed from needs_work to to_beta

comment:32 Changed 4 years ago by shevek

  • Status changed from to_beta to testing

Deployed to alpha and beta.

comment:33 Changed 4 years ago by beatnickgr

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

Closed, as crumbking requested. (not tested by me)

Note: See TracTickets for help on using tickets.