Email notifications for new comments

[Christopher Allan Webber]

It would be really great if when someone comments on your media entry that you get notified. I know I'd love to know if someone loved my video!

Several parts to this, I think:

• User option to "email notifications of comments"
  • Add to the user interface in the user profile page
  • Add option to the model in Mongo and SQL models (if SQL hasn't landed completely by then!)
  • Add migration to Mongo (or SQL if it's landed before this ;))
• Actually email notify users! See the registration code which has examples of this.
• Possibly turn on and off this option altogether in the config file? See: ​http://wiki.mediagoblin.org/Configure_MediaGoblin (not sure if this is bloat or not ;))
• Unit tests for this would be nice... but then again unit tests of comments at all would be nice. Not a requirement for merge, sadly! ;)

[Derek]

This is done, tests pass. You should check the design though. Looks ugly. :) I will put the link to the branch as soon as I am done wrangling with my machine.

[Derek]

​git://gitorious.org/~derek-moore/mediagoblin/derek-moore-mediagoblin.git bug405_email_notifications_for_comments

[joar]

• You currently get a notification for commenting on your own post.
• In send_media_comment you're merging together URLs with format. use urlgen's qualified kwarg instead.

  comment_url = request.urlgen(
              'mediagoblin.user_pages.media_home.view_comment',
              comment=comment._id,
              user=media.get_uploader.username,
              media=media.slug_or_id,
              qualified=True) + '#comment'
  

• Make the email subject translatable: from mediagoblin.tools.translate import pass_to_ugettext as _ and wrap the string with _(...).

Other than that, it's good!

[Christopher Allan Webber]

• First of all, a new migration got merged in the meanwhile. I pushed a local version of your branch. You should probably merge with it. ​https://gitorious.org/~cwebber/mediagoblin/cwebbers-mediagoblin/commits/derek-moore-bug405_email_notifications_for_comments

git remote add cwebber git://gitorious.org/~cwebber/mediagoblin/cwebbers-mediagoblin.git
git fetch cwebber
git merge cwebber/derek-moore-bug405_email_notifications_for_comments

• Next, I think that ignoring the errors of the form is the wrong way to go. Instead, we should remove the Required() on old password and new password and only operate on them if they're both filled in (and matching), I think. So, also move the thing you're checking into the form.validate() section.

• Instead of checking the request.POST, get the value out of the form, which should handle the conversion to boolean for you. Like so:

ipdb> form.wants_comment_notification.data
False

(we really ought to be using form.field.data in more places of our
code, to be honest)

This is in the right direction though! Happy to answer further
questions on irc.

[Christopher Allan Webber]

Btw, you might run into some issues where doing that merge makes your database, er, mixed up since the order of things changed. I'd just move it back to before either migration, like so:

$ ./bin/gmg shell
>>> db.app_metadata.update({'_id': 'mediagoblin'}, {'$set': {'current_migration': 10}})
>>> exit()
$ ./bin/gmg migrate

[Derek]

Got it, thanks to both of you for taking time to review this!! I will fix this and reply when done.

[Derek]

Ok, incorporated suggestions from joar and cwebber, thanks again, guys:

• You currently get a notification for commenting on your own post.

Done... I actually thought I had already handled this user_pages/views.py (see line 163)

• In send_media_comment you're merging together URLs with format. use urlgen's qualified kwarg instead.

Done

• Make the email subject translatable

Done

• Next, I think that ignoring the errors of the form is the wrong way to go. Instead, we should remove the Required() on old password and new password and only operate on them if they're both filled in (and matching), I think.

Done, everything is validated before it is saved. Checks done to see if the old password and new password fields have been filled in or not (signaling the user's intention to change their password).

• Instead of checking the request.POST, get the value out of the form, which should handle the conversion to boolean for you

Done, thanks for the tip.

As always, let me know if I need to revisit, the comments are always appreciated. Thanks again!

See:

​https://gitorious.org/~derek-moore/mediagoblin/derek-moore-mediagoblin/commits/bug405_email_notifications_for_comments

[joar]

Merged into master