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

Commenting on an error when there is only one user raises an exception #455

Merged
merged 2 commits into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
@alvarobp
Contributor

alvarobp commented Apr 8, 2013

As described in issue #452.

ArgumentError: At least one recipient (To, Cc or Bcc) is required to send a message

It looks like this was introduced in commit: 310b4fc

When the comment author is the only notification_recipient, there are no recipients for the email.

This pull request fixes that by putting the logic of when to send an email notification in the Comment#emailable? method (like the one in App). A comment is "emailable" when its app is emailable and has recipients other than the comment author.

Comment emailable logic to decide when to send notifications
Test coverage for comment observer

[Fixes #452]
@vegantech

This comment has been minimized.

Show comment
Hide comment
@vegantech

vegantech Apr 8, 2013

Contributor

Just to keep things DRY there should be an additional change:

app/mailers/mailer.rb line 38 should be changed from:

    recipients = @app.notification_recipients - [comment.user.email]

to

   recipients = comment.notification_recipients
Contributor

vegantech commented Apr 8, 2013

Just to keep things DRY there should be an additional change:

app/mailers/mailer.rb line 38 should be changed from:

    recipients = @app.notification_recipients - [comment.user.email]

to

   recipients = comment.notification_recipients
@alvarobp

This comment has been minimized.

Show comment
Hide comment
@alvarobp

alvarobp Apr 9, 2013

Contributor

You're right. That was one of the two reasons I added that method to Comment.

I changed it and I added a few basic tests cases for the comment_notification in mailer_spec. One of the cases checks that the recipients are delegated to Comment#notification_recipients.

Contributor

alvarobp commented Apr 9, 2013

You're right. That was one of the two reasons I added that method to Comment.

I changed it and I added a few basic tests cases for the comment_notification in mailer_spec. One of the cases checks that the recipients are delegated to Comment#notification_recipients.

lest added a commit that referenced this pull request Apr 9, 2013

Merge pull request #455 from alvarobp/comment-mail-with-one-user-comp…
…lete

Commenting on an error when there is only one user raises an exception

@lest lest merged commit 0972b7c into errbit:master Apr 9, 2013

1 check passed

default The Travis build passed
Details
@lest

This comment has been minimized.

Show comment
Hide comment
@lest

lest Apr 9, 2013

Member

Thanks ❤️

Member

lest commented Apr 9, 2013

Thanks ❤️

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