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
from

Conversation

Projects
None yet
5 participants
@georgyangelov
Collaborator

georgyangelov commented Sep 23, 2014

PR към #140

Очаквам коментари :)

@georgyangelov georgyangelov force-pushed the georgyangelov:email-notifications branch from 60a5d1b to 20a284f Sep 23, 2014

@coveralls

This comment has been minimized.

coveralls commented Sep 23, 2014

Coverage Status

Coverage decreased (-4.08%) when pulling 20a284f on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 23, 2014

Coverage Status

Coverage increased (+0.0%) when pulling 20a284f on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

@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')

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

This comment has been minimized.

@georgyangelov

georgyangelov Sep 23, 2014

Collaborator

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

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

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

This comment has been minimized.

@mitio

mitio Sep 23, 2014

Collaborator

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

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

@mitio Like, read the link, bro.

This comment has been minimized.

@georgyangelov

georgyangelov Sep 24, 2014

Collaborator

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

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

@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')

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

Ditto.

if challenge.hidden_changed? and not challenge.hidden and not challenge.checked
ChallengeMailer.new_challenge challenge
end
end

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

create(:user, challenge_notification: true),
]
users.each do |user|

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

This comment has been minimized.

@georgyangelov

georgyangelov Sep 23, 2014

Collaborator

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

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

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

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

email: 'john@doe.com'
challenge.stub name: 'Challenge name',
closes_at: time

This comment has been minimized.

@skanev

skanev Sep 23, 2014

Owner

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

user.stub({
  first_name: 'John',
  email: 'john@doe.com',
})

This comment has been minimized.

@georgyangelov

georgyangelov Sep 23, 2014

Collaborator

Окей :)

@skanev

This comment has been minimized.

Owner

skanev commented Sep 23, 2014

Много яко, но има повторение.

  • Mailer-ите са почти еднакви. Observer-ите също.
  • Тестовете плачат за shared_examples_for

Отделно:

Не мога да напипам кога пращаш mail-и. Правиш някаква хакерия с hidden_changed?, която не ми се струва добра идея, понеже:

  1. Някой може да пусне предизвикателство без да иска и да го върне обратно, резултиращо в два мейла; и
  2. една идея по-магическо, отколкото трябва.

Щях да ти предложа да пазиш състояние във всяко такова дали студентите са били известени, но вместо това ти предлагам да сложиш един бутон "Изпрати известие за ново предизвикателство" за админите и да разкараш тези observer-и.

@georgyangelov

This comment has been minimized.

Collaborator

georgyangelov commented Sep 23, 2014

Благодаря за коментарите. :)

За повторението доста се чудих какво да направя, понеже не съм особено запознат с Rails и не ми е много ясно кое къде е добре да се сложи. Общ mailer, който да се използва за повече от един модел ОК ли е? Повечето повторения идват от факта, че самите модели challenge и task са доста подобни.
Не знаех за shared_examples_for.

Това с бутона е добро, ще го преработя утре.

@skanev

This comment has been minimized.

Owner

skanev commented Sep 23, 2014

Общ мейлър е ОК. Търсиш някакъв протокол (интерфейс) на който Challenge и Task да отговорят и правиш всичко да работи по него.

Мога да ти помогна с дизайна, но няма да е тази седмица. Евентуално следващата. Отвъд това, напълно ОК съм да ревюирам каквото изпратиш :)

georgyangelov added some commits Sep 24, 2014

@coveralls

This comment has been minimized.

coveralls commented Sep 24, 2014

Coverage Status

Coverage increased (+0.0%) when pulling 2f4a97c on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 24, 2014

Coverage Status

Coverage increased (+0.0%) when pulling 9183dfa on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

@georgyangelov

This comment has been minimized.

Collaborator

georgyangelov commented Sep 24, 2014

Преработих нещата:

  • Използвам формата на датата от config/locales/bg.yml. Тук в тестовете използвам I18n.l, което зависи от конфигурацията и малко ме притеснява. Има ли по-добър начин или така става?
  • Махнах observer-ите и сложих бутони.
  • Промених тестовете на mailer-ите и използвам shared_examples_for.

Не съм напълно убеден, че общ mailer е много добра идея в конкретния случай. Кодът не е много, а с изключение на challenge и task няма да може работи в този вид за друг модел. Освен това, ако трябва да се параметризира или да се изнесат на друго място специфичните неща, няма ли да се обезсмисли идеята на mailer-ите?

def new_challenge
challenge = Challenge.find params[:id]
ChallengeMailer.new_challenge challenge unless challenge.hidden

This comment has been minimized.

@mitio

mitio Sep 24, 2014

Collaborator

challenges.hidden е булева колонка, което означава, че можеш да я достъпваш и като предикат през модела, което аз бих предпочел в този случай: ... unless challenge.hidden?.

This comment has been minimized.

@georgyangelov

georgyangelov Sep 25, 2014

Collaborator

Nice. :)

def new_task
task = Task.find params[:id]
TaskMailer.new_task task unless task.hidden

This comment has been minimized.

@mitio

mitio Sep 24, 2014

Collaborator

Тук същото като за challenge.hidden?.

task = Task.find params[:id]
TaskMailer.new_task task unless task.hidden
redirect_to task_path(task), notice: 'Мейлите са поставени в опашката'

This comment has been minimized.

@mitio

mitio Sep 24, 2014

Collaborator

Не ти ли звучи странно "поставени на опашката"? Не е ли по-добре "наредени на опашката"? Или, "Засилихме писмата" 😎

Също така, думата е "имейли".

This comment has been minimized.

@georgyangelov

georgyangelov Sep 25, 2014

Collaborator

"Поставени на опашката" го видях от challenge_checks_controller 😊
Не се бях замислял дали думата я има в речника :)

This comment has been minimized.

@skanev

skanev Sep 25, 2014

Owner

Също така, думата е "имейли".

🔪 🔪 🔪

@coveralls

This comment has been minimized.

coveralls commented Sep 25, 2014

Coverage Status

Coverage increased (+0.0%) when pulling fdbcdea on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

Move some tests out of the shared ones for the mailers
The shared examples in their previous form could not be used for
any other mailer, even though there may be other mailers that send
emails to multiple users. Also, the parametrization of the shared
examples makes it harder to follow the expectations of the tests.

Now, only the examples for the `#new_challenge` and `#new_task` methods
are shared. This will make the shared test usable for other mailers.
The other option is to make another mailer whose job is to retrieve
all users from the database and call another mailer, but the method
itself is small and making it more generic will be almost like
recreating the functionality of `User.where.each`.

The expectations in the separate mailer spec files are similar, but
for other similar mailers they may be different. For example an
announcement mailer would not contain the title of the announcement
in the body text (only in the subject) and would not use
`announcement_url(announcement)` to generate its URL.
@coveralls

This comment has been minimized.

coveralls commented Sep 27, 2014

Coverage Status

Coverage increased (+0.0%) when pulling 259bd04 on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

@georgyangelov

This comment has been minimized.

Collaborator

georgyangelov commented Sep 27, 2014

Направих някои промени по тестовете - изкарах тези за #new_#{model}_for_user от shared_examples. Така ми се струва по-преизползваемо и по-просто. Аргументация има в съобщението на commit-a. :)

Имейлите за новини мисля да ги базирам на този бранч. Да пусна ли друг PR за тях или да изчакам да ревюирате този, и тогава?

ПП. Не бързам, просто се чудех дали не е добре да изчакам с другия PR. :)

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

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

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

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

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

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

This comment has been minimized.

@georgyangelov

georgyangelov Nov 9, 2014

Collaborator

Упс :)

@@ -9,6 +9,13 @@
= admin_only do
- unless ChallengeCheckWorker.queued? @challenge.id
= link_to 'Пусни тестовете', challenge_check_path(@challenge), method: :create, class: :action
= admin_only do
- unless @challenge.hidden

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

@challenge.hidden? (т.е. сложи въпросителна на hidden).

@@ -9,6 +9,13 @@
= admin_only do
- unless ChallengeCheckWorker.queued? @challenge.id
= link_to 'Пусни тестовете', challenge_check_path(@challenge), method: :create, class: :action
= admin_only do
- unless @challenge.hidden
= link_to 'Изпрати мейли',

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

Предлагам "Извести студентите по имейл". Каквото и да е, думата "мейли" не е окей. Трябва да е "имейли", тъй като това е официалното изписване.

This comment has been minimized.

@georgyangelov

georgyangelov Nov 9, 2014

Collaborator

Не съм го видял след предния ти коментар, извинявай.

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

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

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

@@ -8,6 +8,13 @@
= link_to 'Предай решение', task_my_solution_path(@task), class: :action
- if @current_user_solution
= link_to 'Коментари към моето решение', @current_user_solution, class: :action
= admin_only do
- unless @task.hidden

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

Същото като по-горе.

@@ -78,6 +78,8 @@ bg:
site: Сайт
about: Разкажете ни за себе си
comment_notification: Искам да получавам писма за коментари по решенията ми
task_notification: Искам да получавам писма за нови задачи
challenge_notification: Искам да получавам писма за нови предизвикателства

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

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

default from: Language.email_sender, reply_to: Language.email
def new_challenge(challenge)
User.where(challenge_notification: true).each do |user|

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

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

This comment has been minimized.

@mitio

mitio Nov 9, 2014

Collaborator

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

This comment has been minimized.

@georgyangelov

georgyangelov Nov 9, 2014

Collaborator

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

@mitio

This comment has been minimized.

Collaborator

mitio commented Nov 9, 2014

@stormbreakerbg, относно последния ти коментар, мисля, че това е добър пример за спекулативен дизайн – правиш си дизайна така, че да адресира евентуално бъдещо разширение (announcements mailer). Но това, колкото и сигурно да изглежда, е все още в неясен бъдещ момент.

В коментара на commit-съобщението, за който споменаваш, се говори за разлики в announcements mailer-а – че няма да съдържат заглавието на новината в body-текста (защо?) и че няма да ползват announcements_path. След 431633d последното вече не е необходимо.

Та, ето я реалността. Промените, които си направил, са на база на спекулация за евентуални бъдещи необходимости, които вече не съществуват.

Относно повтарянето на нещата – наистина са доста близки. Не мислиш ли, че ще е по-добре да обединим повторенията в "homework" и да кръстим нещата EmailNotifications.new_homework..., new_homework_for_user и прочее, като запазим разликите в текстовете и двете отделни опции? За целта ще трябва да параметризираме homework_type (:task, :challenge) и translated_homework_type (примерно) на "нова задача"/"ново предизвикателство". Май ще са само това разликите. Дай мнение когато можеш.

@georgyangelov

This comment has been minimized.

Collaborator

georgyangelov commented Nov 9, 2014

@mitio, честно казано, ако се бях сетил за EmailNotifications#new_homework_for_user нямаше да го направя така. :)

И все пак, обединението в homework силно се възползва от това, че имейлите са много близки по съдържание, което в този случай е напълно оправдано, но според мен по принцип е доста близо до границата с "DRY на всяка цена".

На мейлърите гледам като на конфигурация. Да, ще се случи два да имат подобен формат или да работят по подобен начин - например и двете да пращат до всички потребители, които са се абонирали, или в даден момент ще изглеждат по подобен начин. Но обединението на announcements и homeworks ще стане "насила". Споменавам го тук, но наистина тази част от коментара принадлежи на PR-а за имейлите за новините.

Наистина, ако го нямаше този общ знаменател (homework), според мен не би било оправдано да се опитваме "насила" да премахнем малко дублиране, което е само на ниво код. Да, методите new_task и new_challenge са много подобни (new_homework_for_user и new_announcement_for_user като по-добър пример), но дали си заслужава да усложняваме нещата, като параметризираме почти всичко в тези два реда код?

За новините - смятам, че няма нужда да съдържат заглавието и в body-то, защото така се доближаваме по-добре до семантиката на имейлите. Лично аз бих предпочел по-скоро да нагодим кода към имейла, отколкото обратното.

Да обобщя - харесва ми идеята за new_homework. Последният ми commit целеше да елиминира напълно "механичното" премахване на дублирания в тестовете (големият параметризиран shared_examples_for), което симулираше copy/paste на тестове, с нещо по-добре вписващо се логически. new_homework ще направи това добре за задачите и предизвикателствата.

Донякъде съм съгласен със забележката за спекулативния дизайн. Може би в случая не се получи добре, но целта ми беше и да махна големия shared_examples_for. Не съм напълно съгласен, че не трябва да мислим и за разширението, и различните use case-ове, които могат да произлязат от написването на един код. Все пак възможността за по-лесно разширение е едно от нещата, което определя добрия код. Това е, разбира се, при условие, че не се вдига сложността за сметка на нещо, което ще се появи в бъдещето. Разделянето на тестовете и позволяването на различаващ се формат на имейлите според мен не вдига сложността и не изисква много усилие за написване. Отново, ако се бях сетил за логическото групиране в new_homework нямаше да подходя така. :)

@mitio

This comment has been minimized.

Collaborator

mitio commented Nov 9, 2014

Макар че ми е трудно да го рационализирам, в този конкретен случай аз бих обединил функционалността за известие на задачи и предизвикателства под знаменател "homework", бих параметризирал само типовете и превода на нещото (а преводът даже може да се извлича автоматично от locale файла на база на типа).

Това за новините бих го оставил отделно.

Иначе съм съгласен, че е малко на границата на насилствения DRY. В крайна сметка, нямам силни възражения да остане и така. Бих оставил на теб да прецениш.

Като горигираш нещата, остави един коментар да погледна пак и да го merge-нем :)

@georgyangelov

This comment has been minimized.

Collaborator

georgyangelov commented Nov 9, 2014

Да, със сигурност тези двете ще ги обединя. homework е добър общ знаменател за задача и предизвикателство. За насилствения DRY говорех за новините и малко "по принцип" (ако не беше измислил homework), не конкретно за homework. :)

Ще ги коригирам. Няма да е днес, обаче.

@georgyangelov

This comment has been minimized.

Collaborator

georgyangelov commented Nov 10, 2014

@mitio оправих част от нещата. При обединяването на мейлърите възникна следният проблем, който не знам как най-добре да реша - не е достатъчно просто да сменим думата "задача" с "предизвикателство", защото са в различен род:

Нова задача <> Ново предизвикателство
Публикувано е ново предизвикателство <> Публикувана е нова задача

Дали двете да ги сложа в locale файла или само заглавието на имейла, а за другото да са отделни view-тата за съдържанието на имейла?

@coveralls

This comment has been minimized.

coveralls commented Nov 10, 2014

Coverage Status

Coverage increased (+0.0%) when pulling 1329330 on stormbreakerbg:email-notifications into 7d36321 on skanev:master.

@s2gatev

This comment has been minimized.

Collaborator

s2gatev commented Mar 28, 2015

Как стоят нещата тук?

@mitio

This comment has been minimized.

Collaborator

mitio commented Mar 29, 2015

Все още го искаме, но не е прегледан. Би трябвало да не е много трудно да се merge-не.

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