Add email notifications for tasks and challenges#148
Add email notifications for tasks and challenges#148georgyangelov wants to merge 9 commits intoskanev:masterfrom
Conversation
georgyangelov
force-pushed
the
email-notifications
branch
from
60a5d1b to
20a284f
Compare
There was a problem hiding this comment.
Тоя формат не може ли да отиде в рейлската конфигурация? Отделно, нямаме ли някъде остандартен формат на дати?
There was a problem hiding this comment.
Не намерих глобален формат. Което не значи, че няма...
There was a problem hiding this comment.
Друг проект е било. Играта ти е нещо в config/initializers.
There was a problem hiding this comment.
Има вградени формати на дати в Рейлс. Не помя точните имена, но имаше
:short, :long и подобни, ако не се лъжа. И май се подаваха като аргумент на
:to_s.
There was a problem hiding this comment.
@mitio не ми харесаха форматите, затова реших да си го форматирам ръчно :)
EDIT: Имало е default формат в locales
|
Много яко, но има повторение.
Отделно: Не мога да напипам кога пращаш mail-и. Правиш някаква хакерия с
Щях да ти предложа да пазиш състояние във всяко такова дали студентите са били известени, но вместо това ти предлагам да сложиш един бутон "Изпрати известие за ново предизвикателство" за админите и да разкараш тези observer-и. |
|
Благодаря за коментарите. :) За повторението доста се чудих какво да направя, понеже не съм особено запознат с Rails и не ми е много ясно кое къде е добре да се сложи. Общ mailer, който да се използва за повече от един модел ОК ли е? Повечето повторения идват от факта, че самите модели challenge и task са доста подобни. Това с бутона е добро, ще го преработя утре. |
|
Общ мейлър е ОК. Търсиш някакъв протокол (интерфейс) на който Мога да ти помогна с дизайна, но няма да е тази седмица. Евентуално следващата. Отвъд това, напълно ОК съм да ревюирам каквото изпратиш :) |
Created EmailNotificationsController#new_challenge and #new_task
In views/challenges/show and views/tasks/show
|
Преработих нещата:
Не съм напълно убеден, че общ mailer е много добра идея в конкретния случай. Кодът не е много, а с изключение на challenge и task няма да може работи в този вид за друг модел. Освен това, ако трябва да се параметризира или да се изнесат на друго място специфичните неща, няма ли да се обезсмисли идеята на mailer-ите? |
There was a problem hiding this comment.
challenges.hidden е булева колонка, което означава, че можеш да я достъпваш и като предикат през модела, което аз бих предпочел в този случай: ... unless challenge.hidden?.
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.
|
Направих някои промени по тестовете - изкарах тези за Имейлите за новини мисля да ги базирам на този бранч. Да пусна ли друг PR за тях или да изчакам да ревюирате този, и тогава? ПП. Не бързам, просто се чудех дали не е добре да изчакам с другия PR. :) |
There was a problem hiding this comment.
Това мисля, че ще се събере на един ред. Мисля, че беше коментирано, че тази подредба не се тачи в този codebase :)
5 participants
PR към #140
Очаквам коментари :)