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

Add email notifications for tasks and challenges #148

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
19 changes: 19 additions & 0 deletions app/mailers/challenge_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class ChallengeMailer < ActionMailer::Base
default from: Language.email_sender, reply_to: Language.email

def new_challenge(challenge)
User.where(challenge_notification: true).each do |user|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Това ще изпрати имейл и на админите. Трябва да го променим.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

На второ четене, не мисля, че е проблем. Аз бих искал да получавам имейл, за да съм сигурен, че нещата работят. А и всеки admin може да си го изключи при нужда. Така е добре.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, и аз с тази идея не го лимитирах.

ChallengeMailer.delay.new_challenge_for_user(challenge, user)
end
end

def new_challenge_for_user(challenge, user)
@user_name = user.first_name
@challenge_name = challenge.name
@challenge_url = challenge_url(challenge)
@challenge_end_date = challenge.closes_at.strftime('%d.%m.%Y %H:%M')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тоя формат не може ли да отиде в рейлската конфигурация? Отделно, нямаме ли някъде остандартен формат на дати?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не намерих глобален формат. Което не значи, че няма...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Друг проект е било. Играта ти е нещо в config/initializers.

Малко вдъхновение

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Има вградени формати на дати в Рейлс. Не помя точните имена, но имаше
:short, :long и подобни, ако не се лъжа. И май се подаваха като аргумент на
:to_s.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitio Like, read the link, bro.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitio не ми харесаха форматите, затова реших да си го форматирам ръчно :)

EDIT: Имало е default формат в locales


mail to: user.email,
subject: "Ново предизвикателство - #{challenge.name}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Това мисля, че ще се събере на един ред. Мисля, че беше коментирано, че тази подредба не се тачи в този codebase :)

end
end
19 changes: 19 additions & 0 deletions app/mailers/task_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class TaskMailer < ActionMailer::Base
default from: Language.email_sender, reply_to: Language.email

def new_task(task)
User.where(task_notification: true).each do |user|
TaskMailer.delay.new_task_for_user(task, user)
end
end

def new_task_for_user(task, user)
@user_name = user.first_name
@task_name = task.name
@task_url = task_url(task)
@task_end_date = task.closes_at.strftime('%d.%m.%Y %H:%M')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.


mail to: user.email,
subject: "Нова задача - #{task.name}"
end
end
11 changes: 11 additions & 0 deletions app/models/challenge_observer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class ChallengeObserver < ActiveRecord::Observer
def after_create(challenge)
ChallengeMailer.new_challenge challenge unless challenge.hidden or challenge.checked
end

def after_update(challenge)
if challenge.hidden_changed? and not challenge.hidden and not challenge.checked
ChallengeMailer.new_challenge challenge
end
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Този observer знае много неща за предизвикателствата. Не може ли да се вдигне всичко в един challenge.pending_notification? и тук само да се прави ChallengeMailer.new_changellenge if challenge.pending_notification? или нещо подобно?

end
11 changes: 11 additions & 0 deletions app/models/task_observer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class TaskObserver < ActiveRecord::Observer
def after_create(task)
TaskMailer.new_task task unless task.hidden or task.checked
end

def after_update(task)
if task.hidden_changed? and not task.hidden and not task.checked
TaskMailer.new_task task
end
end
end
5 changes: 5 additions & 0 deletions app/views/challenge_mailer/new_challenge_for_user.text.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Здравей, <%= @user_name %>!

Публикувано е ново предизвикателство - <%= @challenge_name %>. Срокът за предаване на решения е до <%= @challenge_end_date %>.

Можеш да прочетеш условието на следният адрес: <%= @challenge_url %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Може да прочетеш условието тук: ..." (не трябва да има пълен член и така го избягваме изцяло).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Упс :)

2 changes: 2 additions & 0 deletions app/views/profiles/edit.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
%fieldset
%legend Известия по поща
= form.input :comment_notification
= form.input :task_notification
= form.input :challenge_notification

%fieldset
%legend Промяна на парола
Expand Down
5 changes: 5 additions & 0 deletions app/views/task_mailer/new_task_for_user.text.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Здравей, <%= @user_name %>!

Публикувана е нова задача - <%= @task_name %>. Срокът за предаване на решения е до <%= @task_end_date %>.

Можеш да прочетеш условието на следният адрес: <%= @task_url %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тук както за предизвикателствата.

2 changes: 1 addition & 1 deletion config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class Application < Rails::Application

# Activate observers that should always be running.
# config.active_record.observers = :cacher, :garbage_collector, :forum_observer
config.active_record.observers = :comment_observer
config.active_record.observers = :comment_observer, :challenge_observer, :task_observer

# Set Time.zone default to the specified zone and make Active Record auto-convert to this zone.
# Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC.
Expand Down
2 changes: 2 additions & 0 deletions config/locales/bg.yml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ bg:
site: Сайт
about: Разкажете ни за себе си
comment_notification: Искам да получавам писма за коментари по решенията ми
task_notification: Искам да получавам писма за нови задачи
challenge_notification: Искам да получавам писма за нови предизвикателства
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"писма" → "имейл"?

sign_up:
full_name: Три имена
faculty_number: Факултетен номер
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddChallengeAndTaskNotificationsToUser < ActiveRecord::Migration
def change
add_column :users, :challenge_notification, :boolean, default: true, null: false
add_column :users, :task_notification, :boolean, default: true, null: false
end
end
62 changes: 62 additions & 0 deletions spec/mailers/challenge_mailer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
require 'spec_helper'

describe ChallengeMailer do
let(:challenge) { double 'challenge' }

describe '#new_challenge' do
let(:mailer_delay) { double 'ChallengeMailer.delay' }

before do
ChallengeMailer.stub(:delay) { mailer_delay }
end

it 'sends multiple emails' do
users = [
create(:user, challenge_notification: true),
create(:user, challenge_notification: true),
]

users.each do |user|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Без цикли в тестовете. first = create ...; second = create ...; mailer.should_receive...with(challenge, first) и т.н.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Добре. Каква е причината за това?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прави теста по-лесен за разбиране. В този случай дори ще е по-кратък :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Далеч не е същото, но:

http://xunitpatterns.com/Conditional%20Test%20Logic.html

Ако прочетеш това и разбереш защо if-овете правят тестовете по-зле, ще разбереш и защо предпочитам (в този случай) праволинеен код пред цикли.

mailer_delay.should_receive(:new_challenge_for_user).with(challenge, user)
end

ChallengeMailer.new_challenge challenge
end

it 'does not send emails to unsubscribed users' do
create(:user, challenge_notification: false)
users = [
create(:user, challenge_notification: true),
create(:user, challenge_notification: true),
]

users.each do |user|
mailer_delay.should_receive(:new_challenge_for_user).with(challenge, user)
end

ChallengeMailer.new_challenge challenge
end
end

describe '#new_challenge_for_user' do
subject { ChallengeMailer.new_challenge_for_user(challenge, user) }

let(:user) { double 'user' }
let(:time) { Time.now }

before do
user.stub first_name: 'John',
email: '[email protected]'

challenge.stub name: 'Challenge name',
closes_at: time
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Това форматиране не ми харесва :)

user.stub({
  first_name: 'John',
  email: '[email protected]',
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Окей :)

end

it { should have_subject 'Ново предизвикателство - Challenge name' }
it { should deliver_to '[email protected]' }
it { should have_body_text 'John' }
it { should have_body_text 'Challenge name' }
it { should have_body_text challenge_url(challenge) }
it { should have_body_text time.strftime('%d.%m.%Y %H:%M') }
end
end
62 changes: 62 additions & 0 deletions spec/mailers/task_mailer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
require 'spec_helper'

describe TaskMailer do
let(:task) { double 'task' }

describe '#new_task' do
let(:mailer_delay) { double 'TaskMailer.delay' }

before do
TaskMailer.stub(:delay) { mailer_delay }
end

it 'sends multiple emails' do
users = [
create(:user, task_notification: true),
create(:user, task_notification: true),
]

users.each do |user|
mailer_delay.should_receive(:new_task_for_user).with(task, user)
end

TaskMailer.new_task task
end

it 'does not send emails to unsubscribed users' do
create(:user, task_notification: false)
users = [
create(:user, task_notification: true),
create(:user, task_notification: true),
]

users.each do |user|
mailer_delay.should_receive(:new_task_for_user).with(task, user)
end

TaskMailer.new_task task
end
end

describe '#new_task_for_user' do
subject { TaskMailer.new_task_for_user(task, user) }

let(:user) { double 'user' }
let(:time) { Time.now }

before do
user.stub first_name: 'John',
email: '[email protected]'

task.stub name: 'Task name',
closes_at: time
end

it { should have_subject 'Нова задача - Task name' }
it { should deliver_to '[email protected]' }
it { should have_body_text 'John' }
it { should have_body_text 'Task name' }
it { should have_body_text task_url(task) }
it { should have_body_text time.strftime('%d.%m.%Y %H:%M') }
end
end
53 changes: 53 additions & 0 deletions spec/models/challenge_observer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
require 'spec_helper'

describe ChallengeObserver do
describe '#after_create' do
it "notifies users via email" do
challenge = build :challenge, hidden: false, checked: false
ChallengeMailer.should_receive(:new_challenge).with(challenge)
challenge.save!
end

it "does not notify users if the challenge is hidden" do
challenge = build :challenge, hidden: true, checked: false
ChallengeMailer.should_not_receive(:new_challenge)
challenge.save!
end

it "does not notify users if the challenge is checked" do
challenge = build :challenge, hidden: false, checked: true
ChallengeMailer.should_not_receive(:new_challenge)
challenge.save!
end
end

describe '#after_update' do
it "notifies users if #hidden changes from true to false" do
challenge = create :challenge, hidden: true
challenge.hidden = false

ChallengeMailer.should_receive(:new_challenge).with(challenge)

challenge.save!
end

it "does not notify users if the hidden attribute does not change" do
challenge = create :challenge, hidden: false
challenge.name = 'test'

ChallengeMailer.should_not_receive(:new_challenge)

challenge.save!
end

it "does not notify users if the challenge is checked" do
challenge = create :challenge, hidden: true
challenge.hidden = false
challenge.checked = true

ChallengeMailer.should_not_receive(:new_challenge)

challenge.save!
end
end
end
53 changes: 53 additions & 0 deletions spec/models/task_observer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
require 'spec_helper'

describe TaskObserver do
describe '#after_create' do
it "notifies users via email" do
task = build :task, hidden: false, checked: false
TaskMailer.should_receive(:new_task).with(task)
task.save!
end

it "does not notify users if the task is hidden" do
task = build :task, hidden: true, checked: false
TaskMailer.should_not_receive(:new_task)
task.save!
end

it "does not notify users if the task is checked" do
task = build :task, hidden: false, checked: true
TaskMailer.should_not_receive(:new_task)
task.save!
end
end

describe '#after_update' do
it "notifies users if #hidden changes from true to false" do
task = create :task, hidden: true, checked: false
task.hidden = false

TaskMailer.should_receive(:new_task).with(task)

task.save!
end

it "does not notify users if the hidden attribute does not change" do
task = create :task, hidden: false, checked: false
task.name = 'test'

TaskMailer.should_not_receive(:new_task)

task.save!
end

it "does not notify users if the task is checked" do
task = create :task, hidden: true, checked: false
task.hidden = false
task.checked = true

TaskMailer.should_not_receive(:new_task)

task.save!
end
end
end