Opened 9 years ago

Closed 8 years ago

#405 closed defect (fixed)

Email notifications for new comments

Reported by: Christopher Allan Webber Owned by: Derek
Priority: major Milestone: 0.3.1
Component: programming Keywords:
Cc: joar+gmg-bugs@… Parent Tickets:

Description

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! ;)

Subtickets

Change History (10)

comment:1 Changed 9 years ago by 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.

comment:3 Changed 9 years ago by joar

Cc: joar+gmg-bugs@… added
  • 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!

comment:4 Changed 9 years ago by Christopher Allan Webber

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.

comment:5 Changed 9 years ago by 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

comment:6 Changed 9 years ago by Derek

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

comment:7 Changed 9 years ago by 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

comment:8 Changed 8 years ago by Christopher Allan Webber

Milestone: 0.3.00.3.1

comment:9 Changed 8 years ago by Christopher Allan Webber

Component: component1programming

comment:10 Changed 8 years ago by joar

Resolution: fixed
Status: newclosed

Merged into master, Thank you Derek!

Last edited 8 years ago by joar (previous) (diff)
Note: See TracTickets for help on using tickets.