Issue 35 - Add "New Comment" email notification

[jduan]

Hi Jordan,

This isn't a 'done' request. I think I've made some decent progress. Can you take a look at the code to see how well I'm doing?

There are a couple issues at this point:

1. I don't have tests yet.
2. Somehow 'app/views/comment_mailer/comment_made.html.erb' doesn't seem to recognize the @body var. I've spent a lot of time trying to figure it out with no luck. I might be doing something completely nonsense.

Anyway, please give me some feedback so I can head down the right direction.

Thanks, Jingjing

[jordanbyron]

Jingjing I gave you a lot of feedback and I think you are on the right path, we just need to address a few bumps along the way. Please ask @sandal and @semmons99 for help if you get stuck. If anything I said was unclear let me know and I'd be more than happy to clarify.

Thanks so much for working on this. I'm really excited to see if this helps increase engagement between users after they've solved the puzzles. :smile:

[jduan]

Hi Jordan,

Thanks for your review. I'll address the issues you raised. Sorry for the late reply. I've been busy working on personal project and assignment lately. I've made some progress on them and now I can dedicate some time to this project:)

Best, Jingjing

[jordanbyron]

Hey Jingjing,

Got you some more feedback. I think you should have enough to resume work. We're not too far from being done, but you've still got some work to do. Thanks for continuing to push forward :smile:

Jordan

[jduan]

Spent some time wrapping up my pp and assignments today. Will spend some time Friday night addressing these issues.

It's been a busy week for me at work. I'm hoping to get a lot done this weekend and pass the course...

[jduan]

Hi Jordan,

I've made some changes based on your feedback. Can you take a look again? The course is ending soon. If you can give me feedback soon, that would be great. I'm expecting to make more changes.

A few issues at this point:

1. No tests yet. I've manually testing along the way. Will try to add some tests while waiting for feedback from you.
2. I tried to move the smtp settings to an initializer. However, that made the code stop working. Delayed_jobs complains it can't send emails.
3. I somehow messed up my commit history after merging from the upstream. Please take a look at the last 7 commits which were changes I recently made. https://github.com/jduan/puzzlenode/commits/master

[jduan]

Jordan,

Can you give me some help on writing tests for this feature? I'm thinking of doing something like:

1. set up some testing data (users, puzzles, comments, etc)
2. call Comment#notify_others
3. find the puzzle for the comment and all the users that have solved the puzzle and indicated they want to receive notification
4. check if there are emails sent to each user

However, I can't seem to get the following test to pass. I wonder if it is related to delayed_jobs. It fails on the assert line (close to the end). Can you help?

require 'test_helper'

class CommentTest < ActiveSupport::TestCase
  def setup
    @harry = Factory(:user,
      :admin    => false,
      :name     => "Harry Hacker",
      :nickname => "hh4x0r",
      :email    => "harry@hackers.org",
      :notify_comment_made => true,
    )
    @sally = Factory(:user,
      :admin    => false,
      :name     => "Sally Solid",
      :nickname => "sallys",
      :email    => "sallys@solid.com",
      :notify_comment_made => true,
    )
  end

  test "Comment#notify_others should email the other users when a comment is made" do
    create_submission(Factory(:puzzle), @harry, true)
    create_submission(Factory(:puzzle), @sally, true)
    puzzle = Factory(:puzzle)
    comment = puzzle.comments.create(:body => "this is a comment",
                                     :user_id => @harry.id)
    comment.notify_others
    assert !ActionMailer::Base.deliveries.empty?
  end
end

[jordanbyron]

@jduan can you push your code so I can take a look at it? The only changes I see here are from 4 weeks ago :fearful:

[jduan]

Jordan,

Do you want me to push the code to your repository (the upstream/master)? Do you want me to straighten the commit history?

Best, Jingjing

On Mon, Jan 30, 2012 at 6:04 AM, Jordan Byron reply@reply.github.com wrote:

@jduan can you push your code so I can take a look at it? The only changes I see here are from 4 weeks ago :fearful:

---

Reply to this email directly or view it on GitHub: https://github.com/mendicant-university/puzzlenode/pull/67#issuecomment-3719891

[jordanbyron]

Nope, just push to your fork. Then all your commits will appear here.

[jduan]

I already did. It's here: https://github.com/jduan/puzzlenode/commits/master

I was working on the 'issue_35' branch and later on I switched to master.

Let me know if you still don't see it.

Thanks, Jingjing

On Mon, Jan 30, 2012 at 8:26 AM, Jordan Byron reply@reply.github.com wrote:

Nope, just push to your fork. Then all your commits will appear here.

---

Reply to this email directly or view it on GitHub: https://github.com/mendicant-university/puzzlenode/pull/67#issuecomment-3722357

[jordanbyron]

I was working on the 'issue_35' branch and later on I switched to master.

Oh well that is the problem. It's always a good idea to work on a feature branch. It pull your changes into your branch, just:

$ git checkout issue_35
$ git merge master
$ git push origin issue_35

Then your commits will show up here and I'll be able to comment on them.

[jduan]

I did what you said. Can you see it now?

[jordanbyron]

:tada: Yay I now see your commits. Thanks for figuring that out. It really helps both of us to work on a single pull request. That way I don't have to worry about missing anything :smile:

Sadly, I have a very busy day today and probably won't be able to review your new code until later tonight (22:00 UTC). I know @sandal has granted you an extension, but let's try our best to get this merged as soon as possible.

In the meantime, see if you can clean things up and get this code ready to be merged. Talk to you in a few hours.

[jduan]

Thanks. I'll work late tonight and do my best to wrap things up as early as possible.

On Mon, Jan 30, 2012 at 10:18 AM, Jordan Byron reply@reply.github.com wrote:

:tada: Yay I now see your commits. Thanks for figuring that out. It really helps both of us to work on a single pull request. That way I don't have to worry about missing anything :smile:

Sadly, I have a very busy day today and probably won't be able to review your new code until later tonight (22:00 UTC). I know @sandal has granted you an extension, but let's try our best to get this merged as soon as possible.

In the meantime, see if you can clean things up and get this code ready to be merged. Talk to you in a few hours.

---

Reply to this email directly or view it on GitHub: https://github.com/mendicant-university/puzzlenode/pull/67#issuecomment-3724450

[jordanbyron]

Jingjing I just hit you with another round of feedback. I believe your tests aren't working because you never called .deliver on the mail object.

There is still a bunch of feedback from before which you still haven't implemented. So please go through the diff, re-read my feedback, and make sure you don't miss anything. Good luck and let me know if you get stuck.

Jordan

[jduan]

Hi Jordan,

I've submitted a few changes. Can you take a look?

A few questions:

1. I created an initializer (config/initializers/action_mailer.rb) and put SMTP settings there. However, it doesn't seem to work. I had to add them back to (config/environments/development.rb) to make it work again. I guess the way I created initializers way wrong. Do you have any experience?
2. I think I've gone through all your feedback and fixed what you recommended. Let me know if I'm still missing anything.

Best, Jingjing

[jordanbyron]

I created an initializer (config/initializers/action_mailer.rb) and put SMTP settings there. However, it doesn't seem to work. I had to add them back to (config/environments/development.rb) to make it work again. I guess the way I created initializers way wrong. Do you have any experience?

Definitely take it out of config/environments/development.rb, and create an initializer called mail_settings.rb with the following code:

ActionMailer::Base.smtp_settings = {
  :address              => "",
  :port                 => 587,
  :domain               => "",
  :user_name            => "",
  :password             => "",
  :authentication       => "plain",
  :enable_starttls_auto => true
}

Make sure you add that file to the .gitignore and then also check in a copy of it with the .example extension.

[jordanbyron]

@jduan this is now very, very close to being complete. Make these last few changes and I'll be able to merge this in the morning. But now, I am tried and am going to bed. I'll check back in around 13:00 UTC

[jduan]

Jordan,

I've made the changes you suggested. Please take another look.

One issue left at this point:

1. test/unit/comment_test.rb fails. I'm expecting to see 2 emails in the queue but somehow only see 1.

I'll check again early tomorrow morning (UTC 14:00) to see if you have any other comments.

[jordanbyron]

:tada: :balloon: :clap:

We've done it! Congratulations Jingjing you really did an excellent job going through my feedback and getting this feature 95% of the way there. I took it the last 5% and merged it into master. Thanks for all your hard work!

Jordan :smile:

[jduan]

Thanks Jordan.

I was running behind and thanks so much for your help and support at the last minute. I owe you a bunch. Hope I can work with you on some other issues later.

Best, Jingjing

On Tue, Jan 31, 2012 at 6:40 AM, Jordan Byron reply@reply.github.com wrote:

:tada: :balloon: :clap:

We've done it! Congratulations Jingjing you really did an excellent job going through my feedback and getting this feature 95% of the way there. I took it the last 5% and merged it into master. Thanks for all your hard work!

Jordan :smile:

---

Reply to this email directly or view it on GitHub: https://github.com/mendicant-university/puzzlenode/pull/67#issuecomment-3740125