Skip to content
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

Should not attempt to send subscription notifications when email not present #650

Open
comp615 opened this issue Mar 30, 2015 · 3 comments
Milestone

Comments

@comp615
Copy link

comp615 commented Mar 30, 2015

Right now when a user creates a post the system will attempt to send notifications to all previous posters (subscribers):
https://github.com/radar/forem/blob/rails4/app/models/forem/post.rb#L106

There is a bug here, however, in that at no point during the process does the system actually check if the user it's attempting to notify has an email (it's entirely plausible that they might not). This line should check to see if they have an email address that is valid before attempting to send:
https://github.com/radar/forem/blob/rails4/app/mailers/forem/subscription_mailer.rb#L10

@radar
Copy link
Collaborator

radar commented Mar 30, 2015

Hi @comp615,

It sure sounds like you know what the problem is and how to fix it. Could you please submit a PR that fixes this? Personally I would not put the check in the SubscriptionMailer, but on the method that calls the mailer.

@comp615
Copy link
Author

comp615 commented Mar 30, 2015

Yeah that was my plan, this is sort of a marker for me as well :-P I'll
take a stab at it when I have a chance

On Mon, Mar 30, 2015 at 1:33 PM, Ryan Bigg notifications@github.com wrote:

Hi @comp615 https://github.com/comp615,

It sure sounds like you know what the problem is and how to fix it. Could
you please submit a PR that fixes this? Personally I would not put the
check in the SubscriptionMailer, but on the method that calls the mailer.


Reply to this email directly or view it on GitHub
#650 (comment).

@krainboltgreene
Copy link

@comp615 Any progress on this?

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

No branches or pull requests

3 participants