Opened 4 years ago

Closed 4 years ago

#2230 closed bug (fixed)

No notification is sent on new comment

Reported by: Tsjoek Owned by: Tsjoek
Priority: major Milestone: 2.6
Component: BW Profile Keywords:
Cc: shevek

Description (last modified by Tsjoek)

due to a missing check no notification is currently sent when a new comment is created.

Use this opportunity as well to improve the comment notification itself: include commenttext, more actions, basic html.

Attachments (1)

diff2230.txt (1.7 KB) - added by shevek 4 years ago.
Possible update to members.ctrl.php and members.model.php

Download all attachments as: .zip

Change History (27)

comment:2 Changed 4 years ago by shevek

Just tested this and the comment notification didn't contain the text of the comment. What would be the right content of the word code?

comment:3 Changed 4 years ago by Tsjoek

Message_profile_comment

Hi %s,
<p>
%s has left you a %s comment. The comment says:<br>
<i> "%s"</i>
<p>
<a href="%s">Read more</a> about the comment.<br>
<a href="%s">Write or update</a> your comment about %s
<p>
Have a great time!<br>
The BeWelcome Team
<p>
PS. In case you have a problem with this comment
please <a href="%s">contact us</a>'

Message_profile_comment_update

Hi %s,
<p>
%s has updated the comment about you. This is now a %s comment and says:<br>
<i>"%s"</i>
<p>
<a href="%s">Read more</a> about the comment.<br>
<a href="%s">Write or update</a> your comment about %s
<p>
Have a great time!<br>
The BeWelcome Team
<p>
PS. In case you have a problem with this comment
please <a href="%s">contact us</a>
Last edited 4 years ago by shevek (previous) (diff)

comment:4 Changed 4 years ago by shevek

Should we add the word codes through a Phinx migration?

comment:5 follow-up: Changed 4 years ago by Tsjoek

I've been thinking about that, has certain advantages. But the disadvantage is that the constraints/settings for wordcodes are circumvented, which I think counts harder, so I'd rather not do that.

comment:6 follow-up: Changed 4 years ago by crumbking

While we are working on it. Comments are soooo important for a hospex site. I see my comment section. Need a lot people to reply with a comment. Just need a little reminder (some social pressure ;)

So what about an automated mail 14 days, 2 month and one year after the comment was written:


Dear crumbking,

before a while memberX had a great experience with you. He/ She found the time to find some great words for this experience:

CommentText

What about writing him/her some words about this great experience?

[BIG ACTION BUTTON] Write a comment for memberX! [BIG ACTION BUTTON]

[Small link] Stop these reminder [Small link] more .....

comment:7 Changed 4 years ago by crumbking

line 1372 $fromMember->Username

Isn't there double information (two times $fromMember->Username)?

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

Update to my comment 3:

The from username is used twice. So I suggest to use the notation %1$s to be able to reuse a parameter.

That would change the Message_profile_comment to

Hi %1$s,
<p>
%2$s has left you a %3$s comment. The comment says:<br />
<i> "%4$s"</i>
<p>
<a href="%5$s">Read more</a> about the comment.<br />
<a href="%6$s">Write or update</a> your comment about %2$s
<p>
Have a great time!<br />
The BeWelcome Team
<p>
PS. In case you have a problem with this comment
please <a href="%7$s">contact us</a>'.

And line 1372 in members.model.php should be deleted.

PS: The site is delivered as XHTML so we should use <br /> instead of <br>.

comment:9 in reply to: ↑ 6 Changed 4 years ago by shevek

@crumbking:

So what about an automated mail 14 days, 2 month and one year after the comment was written:

See suggestion 'Comment reminder': http://www.bewelcome.org/suggestions/124/rank

And the dicussion that was against any automated reminders: http://www.bewelcome.org/groups/1654/forum/s11352-Comment_reminder

comment:10 in reply to: ↑ 5 Changed 4 years ago by shevek

Replying to Tsjoek:

I've been thinking about that, has certain advantages. But the disadvantage is that the constraints/settings for wordcodes are circumvented, which I think counts harder, so I'd rather not do that.

The word codes need to be added by a dev anyway. Which constraints do you mean? The description?

comment:11 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by Tsjoek

Replying to shevek:

I suggest to use the notation %1$s to be able to reuse a parameter.

Fine with me, I always use the Rox options that are already used nearby, as I don't know which functionality goes with what functions/classes etc, due to lack of documentation.

PS: The site is delivered as XHTML so we should use <br /> instead of <br>.

oh gosh, that's so 2007. O wait ... ;-)

Which constraints do you mean? The description?

Yes, and having the archive/dnt/idmember/prio etc. correctly, AdminWord takes care of that.

comment:12 in reply to: ↑ 11 Changed 4 years ago by shevek

Replying to Tsjoek:

PS: The site is delivered as XHTML so we should use <br /> instead of <br>.

oh gosh, that's so 2007. O wait ... ;-)

Right, we need to update the site to something more modern soon.

Which constraints do you mean? The description?

Yes, and having the archive/dnt/idmember/prio etc. correctly, AdminWord takes care of that.

But having the word codes at hand with the intended content will make it easier for testers to see if the functionality is correct. To ensure correctness of the entries in the words table a migration review would be necessary of course.

A good example how to do it right in the database change How To should help with that as well.

Anyway that's off topic in Trac so lets move this to the dev list.

comment:13 Changed 4 years ago by shevek

  • Status changed from local_testing to needs_work

The message to the member is send as plain text. This should take care of the setting for the mail format of the member receiving the comment.

comment:14 Changed 4 years ago by Tsjoek

  • Description modified (diff)
  • Status changed from needs_work to local_testing

Update:

  • Message with %$1s style variables
  • Message in xhtml (although I'm unsure if the notifications are delivered in xhtml as well)
  • All wordcodes as new wordcodes using phinx
  • Archiving old wordcodes using phinx

https://www.gitorious.org/bewelcome/rox/commit/d0b75e5e9c50d858d830c1ba9ad89dde239767e5
https://www.gitorious.org/bewelcome/rox/commit/f6b5b2ede7212fdb583f5d53feef89eea14d94be

comment:15 follow-up: Changed 4 years ago by Tsjoek

  • serving the mails sent from member.entity.php depends on html-preference

It's worth mentioning that I don't have a mailserver running locally, so testing the way things get delivered is quite necessary.

comment:16 in reply to: ↑ 15 Changed 4 years ago by shevek

Tested a bit: The text of the comment is stripped of all linebreaks and HTML tags are escaped (<b> becomes &lt;b&gt;) which makes the comment hard to read in the mail.

I'd rather use the HTMLpurifier to ensure correctness of the entered comment (note that comments can contain HTML but only limited (see getBasicHtmlPurifier for details)).

But that means the <i> from the wordcodes has to go.

Added an attachment with a diff that could fix that.

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

Changed 4 years ago by shevek

Possible update to members.ctrl.php and members.model.php

comment:17 Changed 4 years ago by shevek

  • Status changed from local_testing to to_alpha

comment:18 Changed 4 years ago by shevek

  • Status changed from to_alpha to needs_work

comment:19 Changed 4 years ago by Tsjoek

That's not the whole story: The sending process of the mail goes through the member.entity, where a purifier is applied as well. Also some variables were not correctly passed.

I think I somehow lack a bit of knowledge about the mail proces and together with not running a mailserver I got a bit confused about what needs to be cleaned where.

I've pushed what I had, I guess it's unwanted as well to double purify. But feel free to edit if that is necessary with the new html mails.

https://www.gitorious.org/bewelcome/rox/commit/54a2a3042184b5069cda1ff1f75e330655d628d6

comment:20 follow-up: 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 crumbking

not sure this is the right ticket: after a vendor/bin/phinx migrate -c phinx.php I get:

PHP Fatal error: Class 'Rox\Tools\RoxMigration?' not found in /bw/migrations/20140720191131_ comment_notification_new_wordcodes.php on line 14

comment:23 Changed 4 years ago by shevek

@crumbking: Wrong ticket, should have been #2219.

comment:24 in reply to: ↑ 20 Changed 4 years ago by thorgal67

Replying to shevek:

Tested: Notification arrived in my mail box after 5 seconds.

comment:25 Changed 4 years ago by shevek

  • Status changed from alpha to testing

comment:26 Changed 4 years ago by crumbking

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

I habe send shevek a comment. Works

Note: See TracTickets for help on using tickets.