eduardoboucas / staticman

💪 User-generated content for Git-powered websites
https://staticman.net
MIT License
2.42k stars 540 forks source link

Email notification upon replies #42

Open TWiStErRob opened 7 years ago

TWiStErRob commented 7 years ago

Usually commenting solutions allow for users to be notified via email about replies to their comments, or in case of a flat commenting stream when anyone posts a new comment. Is it possible to do this with staticman?

binarymist commented 6 years ago

I think my problem was that I added the mailgun API key without the "key-" prefix. Once I added the correct API key as shown in the mailgun domain information and ran it through https://api.staticman.net/v2/encrypt/ along with my [mailgun-prefix].[my.domain], then submitted a test comment with the options[subscribe] checkbox of the submit form checked, I could then see a mailing list entry turn up in my mailgun account.

At this point notifications were working, but there were still errors in the mailgun Logs tab:

No MX for [mailgun-prefix].[my.domain]

At this point I added the two MX records to my DNS provider that mailgun recommends: name: mailgun value: mxa.mailgun.org name: mailgun value: mxb.mailgun.org

and tested every scenario I could think of. All working at this stage.

nakoo commented 6 years ago

@binarymist what do you mean [mailgun-prefix] of the domain?

binarymist commented 6 years ago

what do you mean [mailgun-prefix] of the domain?

The prefix that mailgun recommend their customers use. I think it can be anything, or possibly nothing (by the look of some of the other participants in this thread), but needs to be specified as part of the domain in the RSA'd domain string in the staticman.yml, and in your mailgun account.

binarymist commented 6 years ago

I've just used this functionality for a subscription to any new blog post. Great for capturing your audience and keeping them up to date directly to their inbox https://github.com/gcushen/hugo-academic/issues/427

wes-brooks commented 6 years ago

I've found two things, either of which can prevent the sending of reply notifications:

  1. If moderation is on, notifications won't be sent. This is due to lib/Staticman.js, lines 498-507. moderation==true, we go into the code that writes a Pull Request but if moderation==false we go into the logic that checks whether anyone has subscribed to hear about responses to the parent comment.
  2. Both subscriptions and replies are registered to whatever options[parent] was relevant to the comment-leaver, but notifications only fire when these match. This means that only "sibling" comments can notify each other (comments with the same parent.) I think the expected behavior is to see notifications based on replies to your own comment, but the current code will instead notify when someone replies to the same comment you replied to.

I think the way to address (2) is to change the part of lib/Staticman.js line 490 that is like:

 subscriptions.set(options.parent, this.fields[options.subscribe])

to be something like

 subscriptions.set(options._id, this.fields[options.subscribe])

instead (I don't know when the _id variable is assigned, though.) Then, the commenter can be notified when people reply to them.

Edited to add:

I think another common source of confusion comes from the fact that comments with parent set to nil cannot subscribe to reply notifications. This is because parent is part of the string that gets hashed to create a unique comment notification thread. The options[parent] property is set in HTML (usually by a Jekyll variable) but some of the projects linked in the README don't populate options[parent] for top-level comments. When I caught that, I started populating top-level parents with the {{ post.slug }} but I still had to be careful because the for loops that copy out the comments seem to want options[parent] to resolve to an integer. As far as I can tell, there is no reason that has to be, but you'll often see code like:

{% assign p       = comment._parent %}
{% assign parent  = p | to_integer %}

in the for loop. So far I have been able to replace that second line by

{% assign parent  = p %}

with no ill effect.

nakoo commented 6 years ago

@wrbrooks Thanks. I finally can get the notification mail. What I only changed is moderation: true to false. Mailgun Logs show me the success messages. But still, I find the way using notification without moderation to false.

wes-brooks commented 6 years ago

After some further investigation, I figured out how to get notifications to send when moderation is enabled: you have to add a webhook to your repository, pointed at

https://api.staticman.net/v1/webhook

Note that this is true even if you're using the v2 API for comments.

This fix is kind of obvious in retrospect (notifications only go out when the comment is accepted, but how was staticman going to know that the comment had been accepted?), but the documentation only mentions the webhook API in reference to automagically deleting the staticman_ branches after they are merged.

I think there should be a clarification in the documentation section about reply notifications to say that when moderation is enabled, sending notifications requires that you add the staticman webhook to your website's repository.

nakoo commented 6 years ago

@wrbrooks Perfect. I finally made it. Thanks for goodness.

binarymist commented 6 years ago

The main problem I'm (and my commenters) seeing is the multiple Staticman notifications for every comment: #182

chmac commented 6 years ago

@eduardoboucas Haven't read this issue in detail yet, but I could put some time into this if you like. I've just started thinking that I really want reply notifications and saw this and #35. Will wait for the go ahead, and if you're happy, put some effort into this. I'm offline next week, and only online a few days the following week, so might be 3-6 weeks before I get to it...

samarulmeu commented 6 years ago

I finally managed to get notification working with mailgun (in fact only adding the emails to a mailing list). Most of the trouble and stress was because I was trying to use a domain in the EU region. There is a difference in the API base URL: https://api.eu.mailgun.net/v3/ vs https://api.mailgun.net/v3/. Is there a way to specify the EU region?

gabeluci commented 5 years ago

I managed to setup notifications per-thread (i.e. people will only be notified if someone replied to a thread they started or replied to, rather than any thread on the post), but only because I'm running my own instance of Staticman and I can change the code.

The key was setting the parent property to the auto-generated id if the parent is empty. I did this in the processEntry function of /lib/Staticman.js:

Staticman.prototype.processEntry = function (fields, options) {
  if (!options.parent) {
    options.parent = this.uid
  }
  ...

That may not be the best place to do it, but it works. Maybe that's something that can be worked into the master code, even as a config option. I'm sure others could benefit.

Then in my site code, I leave options[parent] blank for a top-level comment, and I set it to the _id of the parent if it's a reply. I completely got rid of the replying_to property and instead just check if _parent == _id to determine if it's a top-level comment or a reply.

Feel free to look at my code. It is based on the good work of @mmistakes.

The only thing outstanding that I might fix is the wording of the initial email to the original commenter. The email is valuable when moderation is on since it basically tells the person that their comment was approved. But I'd rather it actually say that than say "Someone replied to a comment you subscribed to".

But that's for another day.

binarymist commented 5 years ago

But no PR submitted @gabeluci?

gabeluci commented 5 years ago

No, no PR. I don't know the code base well enough to say that's the best place in the code to do it, or if it will break how others are using Staticman. @eduardoboucas may decide it's better to make it a configurable option, if anything.

binarymist commented 5 years ago

Understood @gabeluci. Looking at the commits vs issues and PR's, it's starting to look like staticman is no longer being maintained. Is this correct @eduardoboucas? If so, have you considered passing the battern?

purpleidea commented 5 years ago

Looking at the commits vs issues and PR's, it's starting to look like staticman is no longer being maintained.

I'm sure @eduardoboucas is busy with other things as well, but send a patch, do a fork, or even host an alternative mail server if you'd like! At least from my POV it's pretty feature complete. The biggest trouble I have is integration with gohugo.

binarymist commented 5 years ago

Oh odd. What exactly are you struggling with there @purpleidea, I have it integrated (https://binarymist.io/blog/2018/02/24/hugo-with-staticman-commenting-and-subscriptions/)

purpleidea commented 5 years ago

@binarymist

I gave up after getting annoyed with the available functions in hugo, and not finding clear docs on how to write one. Code is here:

https://github.com/purpleidea/purpleidea.com

Patches welcome! :)

eduardoboucas commented 5 years ago

Looking at the commits vs issues and PR's, it's starting to look like staticman is no longer being maintained. Is this correct @eduardoboucas? If so, have you considered passing the battern?

The time I've been able to dedicate to the project hasn't been enough, sure. But what exactly do you mean by passing the batten? The repository is open, anyone can contribute with code. I'm more than happy to give write access to people that wish to become regular maintainers (as I've done in the past).

Most issues have to do with new feature requests, most of which I don't want to commit to precisely because I don't have enough time to implement and maintain them. Most PRs are from people wanting to add their site to the README (which, looking back, wasn't the best of ideas).

Maybe what I need to do is go through the list of issues/PRs, close what needs to be closed, merge what needs to be merged, and then start a GitHub Project to give some visibility on what needs to be addressed and in which priority. That should give users (and contributors) an indication on what is being worked on and what we need help with.

Thanks for the patience.

gabeluci commented 5 years ago

I ended up writing a whole article about how I setup my site to use Staticman. More relevant to this thread is the section about comment threading.

binarymist commented 5 years ago

What I mean @eduardoboucas is that it seems that @gabeluci has an answer for this issue in code, and it'd be great to see some colaboration between you two so that the work could be included in Staticman. Respect to both parties involved.

binarymist commented 5 years ago

@purpleidea That's why I provided the available functions and clear docs for you, if they're less than clear please leave a comment on the blog post and I'll do my best to help.

gabeluci commented 5 years ago

I don't think my way is the only way to do it. I think you could do per-thread notifications on the public API as-is. You would have to:

  1. For top-level comments, generate a unique identifier in JavaScript (this might help) and use that for the value of parent. That value would be used for replies too. That solves the issue of subscribing the top-level commenter.
  2. Add a second field called something like isTopLevel that is true for top-level comments and false for replies. This would be used solely for displaying the comments, since the first loop only looks at top-level comments.
chmac commented 5 years ago

I'm also a committer and have deploy privileges. I'd be open to putting some time into getting this live. Is anyone ready to submit a PR? I'm ready to review it, loop in @eduardoboucas, and if all's well, deploy.

@samarulmeu It would probably be easier to open a new issue about the switch to the EU region. There's a few things getting mixed together here.

gabeluci commented 5 years ago

@chmac Are you talking about setting parent to the generated id? I could attempt a PR for that, but it depends on if it should be a configurable option. Doesn't hurt to make it an option I guess.

I did submit an unrelated PR for something else related to notifications.

chmac commented 5 years ago

@gabeluci Aha, I see your PR. To be honest, I'm not 100% on top of the status of this issue. I'll try to schedule some time to dig into it in the next few weeks. I also now see there's quite a few open PRs which could be reviewed...

gabeluci commented 5 years ago

Well my PR doesn't really have anything to do with this issue, except being related to notifications.