JujaLabs / itevents

Resource to subscribe on it events
Apache License 2.0
7 stars 5 forks source link

Issue47 #94

Closed witjem closed 8 years ago

witjem commented 9 years ago

Issue 47

Об'єднані всі підтаски та дописаний шедулер

vaa25 commented 8 years ago
  1. У нас не все юзеры в бд являются подписчиками. Ошибка в бизнес-логике.
  2. в task-context.xml строка ну уж очень длинная
  3. название "myScheduler" подходит для домашнего проекта, но не для коммерческого (мнение)
  4. clearDateTimeInEventList -> clearEventDateInEvents (мнение)
  5. везде до сих пор был порядок BuilderUtil.buildЧтоКакое, а у вас buildКакоеЧто (builderJavaFilter->builderFilterJava например) (мнение)
  6. в shouldReturnFreeEvents нарушены отступы
  7. в MailBuilderUtilTest потерялись модификаторы доступа полей
  8. в присылаемом письме находится прямая ссылка на сайт события, поэтому мы не можем зарегистрировать переход по ссылке. это не правильно. мы должны фиксировать каждый переход. (пока что мнение)
  9. в DateTimeUtilTest лишние аннотации класса. не все методы DateTimeUtil тестируются, плюс те что тестируются нигде больше не используются, а значит нет необходимости делать их public. getNowDate() нужно оставить public, так как он используется в 49. (п.4)
  10. в shouldReturnDataWithAdditionalDays() лишнее разбитие строки (мнение)
  11. некоторые тесты работы с mail обращаются к интернету. может моки помогут?(п.3)

остальные недочеты вроде как пришли из мастера. да, и надо как-то с 49 состыковать, я свои аргументы выложил, ваших не видел.

witjem commented 8 years ago

@vaa25 Ти реально круто робиш рефактор ) Деякі вже виправив =)

Як завжди з деякими не згоден

  1. Це класна ідея для нового таску
  2. Клас протестований, з приводу необхідності методів, то в п.4 описані рекомендації.
  3. SendGridMailServiceTest не є юніт тестом.
witjem commented 8 years ago

Виправив 11й пункт