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

Reply notification #50

Merged
merged 3 commits into from Jul 26, 2018

Conversation

Projects
None yet
@tsileo
Contributor

tsileo commented Dec 27, 2013

Hi,

I love Isso, but the only thing that prevent me to drop Disqus for Isso is the ability for commenters to receive an email notification of reply.

I get a working version with this feature (I use it in production on my blog: http://thomassileo.com), it still need some improvement, I just open the pull request to open the discussion.

What do you think about this feature ?

I added a field in the comment table, is this something that will prevent you to merge this feature ?

Let me know what you think about this,

Thanks!

@posativ

This comment has been minimized.

Show comment
Hide comment
@posativ

posativ Dec 27, 2013

Owner

Hey,

I already had this feature request in my mind, I'm glad you already got a working state. I made a few annotations to the source code, but in general the PR looks good to me.

I added a field in the comment table, is this something that will prevent you to merge this feature ?

Sure, I built a simple schema migration in isso/db/__init__.py where you can add a new column.

Owner

posativ commented Dec 27, 2013

Hey,

I already had this feature request in my mind, I'm glad you already got a working state. I made a few annotations to the source code, but in general the PR looks good to me.

I added a field in the comment table, is this something that will prevent you to merge this feature ?

Sure, I built a simple schema migration in isso/db/__init__.py where you can add a new column.

@@ -20,7 +20,7 @@ class Comments:
"""
fields = ['tid', 'id', 'parent', 'created', 'modified', 'mode', 'remote_addr',
'text', 'author', 'email', 'website', 'likes', 'dislikes', 'voters']
'text', 'author', 'email', 'website', 'likes', 'dislikes', 'voters', 'notification']

This comment has been minimized.

@posativ

posativ Dec 27, 2013

Owner

I'm not sure about the name "notification", better the plural "notifications" or the verb "notify" to apply for (multiple) notifications.

@posativ

posativ Dec 27, 2013

Owner

I'm not sure about the name "notification", better the plural "notifications" or the verb "notify" to apply for (multiple) notifications.

rv.write("Link to comment: %s\n" % (local("origin") + thread["uri"] + "#isso-%i" % comment["id"]))
rv.write("\n")
rv.write("IP address: %s\n" % comment["remote_addr"])
rv.write("Link to comment: %s\n" % (local("origin") + thread["uri"] + "#isso-%i" % comment["id"]))

This comment has been minimized.

@posativ

posativ Dec 27, 2013

Owner

A link to the comment in the reply notification would be useful.

@posativ

posativ Dec 27, 2013

Owner

A link to the comment in the reply notification would be useful.

body = self.format(thread, comment, admin=True)
self.sendmail(thread["title"], body, thread, comment)

This comment has been minimized.

@posativ

posativ Dec 27, 2013

Owner

Just a thing for later: the email value is not actually verified to contain a valid email address. Hence it should be checked what kind of errors can appear when an invalid email address is used (e.g. the while loop for the uWSGI implementation).

@posativ

posativ Dec 27, 2013

Owner

Just a thing for later: the email value is not actually verified to contain a valid email address. Hence it should be checked what kind of errors can appear when an invalid email address is used (e.g. the while loop for the uWSGI implementation).

@@ -214,5 +214,11 @@ a {
}
}
}
> .notification-section {
display: none;

This comment has been minimized.

@posativ

posativ Dec 27, 2013

Owner

Invisible by default? Why?

@posativ

posativ Dec 27, 2013

Owner

Invisible by default? Why?

@tsileo

This comment has been minimized.

Show comment
Hide comment
@tsileo

tsileo Dec 29, 2013

Contributor

Awesome!

I'll make the modifications (notify instead of notification, and update the mail body with the reply link) and dig into the schema migration.

I made the checkbox invisible by default so it only appear once the email is filled, and if email is left empty it never appear. Do you think it's better to always display it ?

Contributor

tsileo commented Dec 29, 2013

Awesome!

I'll make the modifications (notify instead of notification, and update the mail body with the reply link) and dig into the schema migration.

I made the checkbox invisible by default so it only appear once the email is filled, and if email is left empty it never appear. Do you think it's better to always display it ?

@posativ

This comment has been minimized.

Show comment
Hide comment
@posativ

posativ Dec 29, 2013

Owner

I actually meant that a change in the schema is not a reason to not merge ;) SQLite3 can add new columns without any migration code.

I made the checkbox invisible by default so it only appear once the email is filled, and if email is left empty it never appear. Do you think it's better to always display it ?

Oh, I've overseen this. I thought the checkbox is not visible at all. It is a good solution then :)

Owner

posativ commented Dec 29, 2013

I actually meant that a change in the schema is not a reason to not merge ;) SQLite3 can add new columns without any migration code.

I made the checkbox invisible by default so it only appear once the email is filled, and if email is left empty it never appear. Do you think it's better to always display it ?

Oh, I've overseen this. I thought the checkbox is not visible at all. It is a good solution then :)

@noqqe

This comment has been minimized.

Show comment
Hide comment
@noqqe

noqqe Feb 16, 2014

Contributor

Any news on this?

Would love to have that.

Contributor

noqqe commented Feb 16, 2014

Any news on this?

Would love to have that.

@tsileo

This comment has been minimized.

Show comment
Hide comment
@tsileo

tsileo Feb 16, 2014

Contributor

I will update my pull request today, stay tuned!

Contributor

tsileo commented Feb 16, 2014

I will update my pull request today, stay tuned!

@noqqe

This comment has been minimized.

Show comment
Hide comment
@noqqe

noqqe Mar 22, 2014

Contributor

Any news? :P

Contributor

noqqe commented Mar 22, 2014

Any news? :P

@posativ

This comment has been minimized.

Show comment
Hide comment
@posativ

posativ Mar 22, 2014

Owner

I've rebased your PR to the current master but did not finished yet. We've discusses this feature in IRC yesterday and it is more complex than initially thought.

When a user subscribes to further comments, the email might be a fake and is owned by someone else. Hence, there also needs to be an unsubscribe feature as well. Also it is useful to store the user's prefered locale and send localized emails instead of english-only.

Owner

posativ commented Mar 22, 2014

I've rebased your PR to the current master but did not finished yet. We've discusses this feature in IRC yesterday and it is more complex than initially thought.

When a user subscribes to further comments, the email might be a fake and is owned by someone else. Hence, there also needs to be an unsubscribe feature as well. Also it is useful to store the user's prefered locale and send localized emails instead of english-only.

@dertuxmalwieder

This comment has been minimized.

Show comment
Hide comment
@dertuxmalwieder

dertuxmalwieder Nov 25, 2014

Opt-out is not needed, double opt-in would be enough according to the laws. Actually, the user would just have to confirm his mail address by a unique token (or something) before it's added to the database (per article, of course).

Opt-out is not needed, double opt-in would be enough according to the laws. Actually, the user would just have to confirm his mail address by a unique token (or something) before it's added to the database (per article, of course).

@noqqe

This comment has been minimized.

Show comment
Hide comment
@noqqe

noqqe Feb 4, 2015

Contributor

any news?

Contributor

noqqe commented Feb 4, 2015

any news?

@jacmoe

This comment has been minimized.

Show comment
Hide comment
@jacmoe

jacmoe Mar 9, 2016

It would be really great to have this feature because I have several places where I would like Isso to replace Disqus..

But my sites are not really something that my commenters are following like hawks, so they would probably never notice that they got a reply, let alone even remember that they visited my sites :P

This feature would be the one that actually made me ditch Disqus completely.

jacmoe commented Mar 9, 2016

It would be really great to have this feature because I have several places where I would like Isso to replace Disqus..

But my sites are not really something that my commenters are following like hawks, so they would probably never notice that they got a reply, let alone even remember that they visited my sites :P

This feature would be the one that actually made me ditch Disqus completely.

@dertuxmalwieder

This comment has been minimized.

Show comment
Hide comment
@dertuxmalwieder

dertuxmalwieder Mar 9, 2016

AFAIR other self-hosted comment systems have it, so you could already ditch Disqus...

AFAIR other self-hosted comment systems have it, so you could already ditch Disqus...

@jacmoe

This comment has been minimized.

Show comment
Hide comment
@jacmoe

jacmoe Mar 9, 2016

AFAIR other self-hosted comment systems have it, so you could already ditch Disqus...

So, could I .. let me see: what other self-hosted systems are there that are as good as Isso?
I have found none.

jacmoe commented Mar 9, 2016

AFAIR other self-hosted comment systems have it, so you could already ditch Disqus...

So, could I .. let me see: what other self-hosted systems are there that are as good as Isso?
I have found none.

@dertuxmalwieder

This comment has been minimized.

Show comment
Hide comment
@dertuxmalwieder

dertuxmalwieder Mar 9, 2016

Last time I checked, HashOver (2.0, not "final" yet) seemed to be pretty decent.

Last time I checked, HashOver (2.0, not "final" yet) seemed to be pretty decent.

@jacmoe

This comment has been minimized.

Show comment
Hide comment
@jacmoe

jacmoe Mar 9, 2016

Hashover has a license that I don't agree with, and it seems a bit clunky. Especially the interface, which is important to me. But, yes: that is pretty close :)

I don't think it has Disqus import, though.

jacmoe commented Mar 9, 2016

Hashover has a license that I don't agree with, and it seems a bit clunky. Especially the interface, which is important to me. But, yes: that is pretty close :)

I don't think it has Disqus import, though.

@valvin1

This comment has been minimized.

Show comment
Hide comment

valvin1 commented Sep 19, 2016

+1

@tofof

This comment has been minimized.

Show comment
Hide comment
@tofof

tofof Feb 1, 2017

Not clear why this isn't more urgently being integrated.

tofof commented Feb 1, 2017

Not clear why this isn't more urgently being integrated.

@dkbast

This comment has been minimized.

Show comment
Hide comment
@dkbast

dkbast Feb 15, 2017

Is there anything we could do to help the process? This feature is currently the only thing holding me back from making the transition to isso.

dkbast commented Feb 15, 2017

Is there anything we could do to help the process? This feature is currently the only thing holding me back from making the transition to isso.

@blatinier

This comment has been minimized.

Show comment
Hide comment
@blatinier

blatinier Feb 16, 2017

Collaborator

Just to answer @dertuxmalwieder on his message on optout not being needed (yes it's more than 2 years ago :D) → optout links in emails are a legal obligation in France. I think France is not the only country requesting such a link. This PR should implement both double opt-in & optout to be completly ok I think.

Collaborator

blatinier commented Feb 16, 2017

Just to answer @dertuxmalwieder on his message on optout not being needed (yes it's more than 2 years ago :D) → optout links in emails are a legal obligation in France. I think France is not the only country requesting such a link. This PR should implement both double opt-in & optout to be completly ok I think.

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Feb 25, 2017

Looking forward to this feature. While I can enable email notifications when people comment on my blog, I'd like people to optionally start/stop notifications when people reply to their comments.

Looking forward to this feature. While I can enable email notifications when people comment on my blog, I'd like people to optionally start/stop notifications when people reply to their comments.

@farmdawgnation

This comment has been minimized.

Show comment
Hide comment
@farmdawgnation

farmdawgnation Feb 25, 2017

What's the status of this? This has been open for awhile and still unmerged. I'm looking at Isso pretty seriously, but this is something that's missing imo.

What's the status of this? This has been open for awhile and still unmerged. I'm looking at Isso pretty seriously, but this is something that's missing imo.

@posativ

This comment has been minimized.

Show comment
Hide comment
@posativ

posativ Feb 28, 2017

Owner

There is no opt-out, which is a hard requirement before I can consider merging.

Owner

posativ commented Feb 28, 2017

There is no opt-out, which is a hard requirement before I can consider merging.

@zhangnew

This comment has been minimized.

Show comment
Hide comment
@zhangnew

zhangnew Mar 29, 2017

I think add a checkbox let user choose send email for his comment be replied or not is a good idea.

I think add a checkbox let user choose send email for his comment be replied or not is a good idea.

@goxofy

This comment has been minimized.

Show comment
Hide comment
@goxofy

goxofy Aug 25, 2017

Maybe someone cloud resolve the conficts with this branch?

goxofy commented Aug 25, 2017

Maybe someone cloud resolve the conficts with this branch?

@rawsh

This comment has been minimized.

Show comment
Hide comment
@rawsh

rawsh Jan 22, 2018

Bump. This is a super feature

rawsh commented Jan 22, 2018

Bump. This is a super feature

@pellenilsson pellenilsson referenced this pull request Feb 25, 2018

Closed

SMTP thread collision #395

@pellenilsson

This comment has been minimized.

Show comment
Hide comment
@pellenilsson

pellenilsson Feb 25, 2018

Contributor

I merged this to current master and implemented opt-out using a link in every mail sent. And I made some small additional improvements:

  • Fix for a thread problem that popped up, see #395.
  • Add the new column to database if needed (by simply adding the column in a try statement, maybe there is a nicer way).
  • Include a link to the comment itself in the email (as was already done for the admin emails).
Contributor

pellenilsson commented Feb 25, 2018

I merged this to current master and implemented opt-out using a link in every mail sent. And I made some small additional improvements:

  • Fix for a thread problem that popped up, see #395.
  • Add the new column to database if needed (by simply adding the column in a try statement, maybe there is a nicer way).
  • Include a link to the comment itself in the email (as was already done for the admin emails).
@pellenilsson

This comment has been minimized.

Show comment
Hide comment
@pellenilsson

pellenilsson Feb 25, 2018

Contributor

There is one thing in this implementation that I'm not happy about, though. If you check the "Subscribe to email notification of replies" checkbox when making a reply, you will never get any notifications, since you can't really reply to a reply. I can see a number of solutions:

  1. Don't show the checkbox at all when writing replies.
  2. Checking the checkbox for a reply could mean that author gets notifications for all replies to the same parent comment that he/she replied to.
  3. Even though the Reply buttons shown after each reply currently has the exact same effect as clicking Reply on the parent comment, it could still be arranged so that notifications are sent based on exactly which Reply button you pressed.

Any preferences on this?

Contributor

pellenilsson commented Feb 25, 2018

There is one thing in this implementation that I'm not happy about, though. If you check the "Subscribe to email notification of replies" checkbox when making a reply, you will never get any notifications, since you can't really reply to a reply. I can see a number of solutions:

  1. Don't show the checkbox at all when writing replies.
  2. Checking the checkbox for a reply could mean that author gets notifications for all replies to the same parent comment that he/she replied to.
  3. Even though the Reply buttons shown after each reply currently has the exact same effect as clicking Reply on the parent comment, it could still be arranged so that notifications are sent based on exactly which Reply button you pressed.

Any preferences on this?

@rawsh

This comment has been minimized.

Show comment
Hide comment
@rawsh

rawsh Feb 25, 2018

I feel like it should act the same as the parent comment. Thanks for update, testing now

rawsh commented Feb 25, 2018

I feel like it should act the same as the parent comment. Thanks for update, testing now

@pellenilsson

This comment has been minimized.

Show comment
Hide comment
@pellenilsson

pellenilsson Apr 22, 2018

Contributor

I agree with @rawsh that solution 2 is the most intuitive one. I updated my branch with this behavior.

Contributor

pellenilsson commented Apr 22, 2018

I agree with @rawsh that solution 2 is the most intuitive one. I updated my branch with this behavior.

@FiveYellowMice

This comment has been minimized.

Show comment
Hide comment
@FiveYellowMice

FiveYellowMice Apr 22, 2018

I think solution 3 provides the best user experience, though it doesn't seem it can be implemented cleanly with the current reply model.

I think solution 3 provides the best user experience, though it doesn't seem it can be implemented cleanly with the current reply model.

@vincentbernat

This comment has been minimized.

Show comment
Hide comment
@vincentbernat

vincentbernat Apr 25, 2018

Contributor

@pellenilsson Maybe you should open another PR to give more visibility to your branch (with a small summary of the remaining issues so people don't have to refer to this PR).

Contributor

vincentbernat commented Apr 25, 2018

@pellenilsson Maybe you should open another PR to give more visibility to your branch (with a small summary of the remaining issues so people don't have to refer to this PR).

@blatinier

This comment has been minimized.

Show comment
Hide comment
@blatinier

blatinier May 1, 2018

Collaborator

@pellenilsson I would very much appreciate your contribution for my own instance and thus consider merging it soon. This current PR is a bit too much outdated and requires some updates on the latest discussions.

Collaborator

blatinier commented May 1, 2018

@pellenilsson I would very much appreciate your contribution for my own instance and thus consider merging it soon. This current PR is a bit too much outdated and requires some updates on the latest discussions.

@pellenilsson

This comment has been minimized.

Show comment
Hide comment
@pellenilsson

pellenilsson May 2, 2018

Contributor

@vincentbernat @blatinier Working on it! I merged with master again (after the Gravatar and Preview additions), but there are some details left to fix. Will create a new PR after that.

Contributor

pellenilsson commented May 2, 2018

@vincentbernat @blatinier Working on it! I merged with master again (after the Gravatar and Preview additions), but there are some details left to fix. Will create a new PR after that.

@blatinier

This comment has been minimized.

Show comment
Hide comment
@blatinier

blatinier May 2, 2018

Collaborator

That's great news! Thanks for your work in advance

Collaborator

blatinier commented May 2, 2018

That's great news! Thanks for your work in advance

@pellenilsson pellenilsson referenced this pull request May 13, 2018

Merged

Reply notification #443

@blatinier blatinier merged commit a322cf6 into posativ:master Jul 26, 2018

1 check passed

default The Travis CI build passed
Details
@jalcine

This comment has been minimized.

Show comment
Hide comment
@jalcine

jalcine Jul 26, 2018

Oh this is awesome!

jalcine commented Jul 26, 2018

Oh this is awesome!

@dertuxmalwieder

This comment has been minimized.

Show comment
Hide comment

It is!

@rawsh

This comment has been minimized.

Show comment
Hide comment

rawsh commented Jul 26, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment