Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(discussion): Notifications are splitting original post from replies #6894

Closed
ewinslow opened this issue Jun 2, 2014 · 14 comments

Comments

Projects
None yet
4 participants
@ewinslow
Copy link
Member

commented Jun 2, 2014

The notifications for the original post is New discussion topic: <title>. The notifications for the replies are New reply to discussion topic: <title>.

In my email client this forces them to be split into separate conversations, with the initial topic post in one thread and then all the replies in a more recent thread, so I end up reading all the replies before I read the initial post. Very strange experience.

Instead, I think we should be making the subject just [Elgg Community] <title> for both topic and reply notifications. That way they'll end up in the same conversation thread since they have the same subject.

New discussion topic: is also just not very useful boilerplate. Prefixing subject with [<forum name>] seems to be pretty common practice for other forums' notifications, though.

@ewinslow ewinslow added bug labels Jun 2, 2014

@Srokap

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2014

Also it might be good occasion to introduce In-Reply-To and References SMTP headers that should also prevent splitting topics.

See: https://tools.ietf.org/html/rfc5322#section-3.6.4

@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

At least Outlook (web client) doesn't recognize a message as a reply even when the title is exactly the same. So we need some other way to inform the client about it.

@ewinslow

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2014

I figured it wouldn't be 100%... I'll research how to do this...

@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

It seems adding Re: to the subject translation tricks Outlook to think it as a reply. It's however not really the job of the translations to do this.

@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

I suppose the most correct way would be to register a 'email', 'system' hook handler (from groups plugin), which would add both a RE: to the title the headers @Srokap mentioned. To do this we would need to somehow pass the ElggDiscussionReply into the hook so it could check whether it's dealing with a discussion reply or not.

A couple of problems:

  1. Currently _elgg_send_email_notification() is not passing $params to elgg_send_email()even though it could. (trivial to fix!)
  2. Even if it were doing this, elgg_send_email() doesn't pass those params to the 'email', 'system' hook
  3. It would be possible to change the behavior, but does the notification object (ElggDiscussionReply) really belong all the way into that hook?
@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

An easy alternate solution: Let's register to the 'prepare', 'notification:create:object:discussion_reply' hook on a low priority, so it can add the RE: into the subject after it has been translated. That way translators do not have to worry about it.

(This may not work on all clients either, because AFAIK the Re: is just meant to be a human readable representation of the actual In-Reply-To header.)

@ewinslow

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2014

It seems outlook also uses a custom "Thread-Index" header to that messages.
Not sure how accurate that so is since everything I'm finding on this is
from years ago, but just something else to take into account.

On Wed, Jun 18, 2014, 5:51 AM, Juho Jaakkola notifications@github.com
wrote:

An easy alternate solution: Let's register to the 'prepare',
'notification:create:object:discussion_reply' hook on a low priority, so
it can add the RE: into the title after it has been translated. That
way translators do not have to worry about it.

(This may not work on all clients either, because AFAIK the Re: is just
meant to be a human readable representation of the actual In-Reply-To
header.)


Reply to this email directly or view it on GitHub
#6894 (comment).

@ewinslow

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2014

I think adding the RE a little earlier is warranted. I agree that it seems wrong for notification objects to be sent all the way down to elgg_send_email.

@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

I think adding the RE a little earlier is warranted.

A little earlier? So you're saying that you consider #6894 (comment) the best solution?

@ewinslow

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2014

Correct

@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

Ok, I'm fine with adding it to the PR.

Srokap added a commit to Srokap/Elgg that referenced this issue Jun 22, 2014

feature(discussions): Added email SMTP headers for better thread grou…
…ping.

Additionally changed message subjects for group discussions to make Gmail happy.

Fixes Elgg#6894

Srokap added a commit to Srokap/Elgg that referenced this issue Jun 22, 2014

feature(discussions): Added email SMTP headers for better thread grou…
…ping.

Additionally changed message subjects for group discussions to make Gmail happy.

Fixes Elgg#6894
@Srokap

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2014

I missed this discussion, but it should address most concerns: #6965

I'm using just title for subject as it seemed to be chosen approach in another discussion.

Srokap added a commit to Srokap/Elgg that referenced this issue Jul 9, 2014

feature(discussions): Added email SMTP headers for better thread grou…
…ping.

Additionally changed message subjects for group discussions to make Gmail happy.

Fixes Elgg#6894

Srokap added a commit to Srokap/Elgg that referenced this issue Jul 9, 2014

feature(discussions): Added email SMTP headers for better thread grou…
…ping.

Additionally changed message subjects for group discussions to make Gmail happy.

Fixes Elgg#6894

Srokap added a commit to Srokap/Elgg that referenced this issue Jul 10, 2014

feature(discussions): Added email SMTP headers for better thread grou…
…ping.

Additionally changed message subjects for group discussions to make Gmail happy.

Fixes Elgg#6894

Srokap added a commit to Srokap/Elgg that referenced this issue Jul 10, 2014

feature(discussions): Added email SMTP headers for better thread grou…
…ping.

Additionally changed message subjects for group discussions to make Gmail happy.

Fixes Elgg#6894

@Srokap Srokap closed this in #6965 Jul 10, 2014

@RiggingDojoAdmin

This comment has been minimized.

Copy link

commented Nov 19, 2014

I am so glad this is getting looked at, will it be rolled in to 1.9x officially?

@juho-jaakkola

This comment has been minimized.

Copy link
Member

commented Nov 19, 2014

This is already included in 1.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.