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

Notifications system for Evans #163

Open
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@skovachev

skovachev commented May 24, 2015

Adds support for simple notifications. This feature is implemented as part of the HackBulgaria Ruby on Rails course. In summary this pull request adds:

  • a notifications view page for all users, containing a list of all unread notifications
  • a notifications menu item that shows an unread counter
  • new notifications are posted when a new task or challenge is created, or when new reply is posted on a topic to which a user has contributed
  • users can view notifications from the list which takes them to the notification source and marks the notification as read

Full task description can be found here: Week 8, Evans task description

@coveralls

This comment has been minimized.

coveralls commented May 24, 2015

Coverage Status

Coverage decreased (-23.73%) to 76.13% when pulling e6df792 on skovachev:feature-notifications into 21d5ddf on skanev:master.

@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented May 24, 2015

@mitio, @skanev, can we ping you for a review?

class NotificationsController < ApplicationController
def index
# @notifications = Notification.unread_for_user current_user.id

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

✂️

def post_notification
generate_notifications_for User.all, title: "Новo предизвикателство: #{name}"
end

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

✂️

def post_notification
generate_notifications_for User.all, title: "Нова задача: #{name}"
end

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

✂️

Общо съобщения:
%strong= @notifications.count

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

✂️

@@ -1,10 +1,16 @@
class Reply < Post

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

✂️

extend self
def generate_notifications_for(users, title:)
users.each do |user|

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

Indentation is off. You may also wanna use #find_each for the bulk of it.

notification.title = title
notification.source = self
notification.user = user
notification.save

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

You may wanna #save! here.

belongs_to :source, polymorphic: true
class << self
def unread_for_user(id)

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

I think accepting an User object here makes for a less surprising API.

This comment has been minimized.

@skovachev

skovachev May 24, 2015

I would then have to reference the User object in the method. Is that acceptable? Feels kind of strange to me

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

You can say its just a duck, you don't depend on the user model per say, but something that responds to #id. You can see Rails itself using that on numerous of occasions.

def show
notification = Notification.find params[:id]
notification.is_read = true
notification.save

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

#save!

I also think you can put this logic into its own method. Say Notification#mark_as_read.

@@ -0,0 +1,13 @@
class NotificationsController < ApplicationController

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

✂️

@@ -41,4 +41,8 @@ def require_admin
def set_time_zone
Time.zone = 'Sofia'
end
def load_notifications

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

def set_notifications_for_current_user. See intention revealing methods.

@@ -0,0 +1,13 @@
module GeneratesNotifications

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

This can be named better.

users.each do |user|
notification = Notification.new
notification.title = title
notification.source = self

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

Is this the only reason why you have this a as a module? Why not pass it as a parameter and put the method on the Notification singleton class instead? One less module in the ancestor chains for the models, a bit more explicit API.

module GeneratesNotifications
extend self
def generate_notifications_for(users, title:)

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

def send_notifications_to does that fit the intention?

def self.up
create_table(:notifications) do |t|
t.string :title, :null => false

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

null: false, E.g. new hash syntax.

@coveralls

This comment has been minimized.

coveralls commented May 24, 2015

Coverage Status

Coverage decreased (-23.73%) to 76.13% when pulling 68c5d28 on skovachev:feature-notifications into 21d5ddf on skanev:master.

@coveralls

This comment has been minimized.

coveralls commented May 24, 2015

Coverage Status

Coverage decreased (-23.82%) to 76.03% when pulling 68c5d28 on skovachev:feature-notifications into 21d5ddf on skanev:master.

@@ -1,5 +1,7 @@
class Topic < Post
has_many :replies, -> { order 'created_at ASC' }
has_many :users, :through => :replies

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

New style hashes (through: :replies).

where(user_id: id, is_read: false)
end
def mark_as_read(notification)

This comment has been minimized.

@gsamokovarov

gsamokovarov May 24, 2015

Contributor

This can be an instance method. notification.mark_as_read

create_table(:notifications) do |t|
t.string :title, null: false
t.references :source, polymorphic: true
t.boolean :read, default: false

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

null: false?

t.string :title, null: false
t.references :source, polymorphic: true
t.boolean :read, default: false
t.references :user

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

null: false?

This comment has been minimized.

@gsamokovarov

gsamokovarov May 28, 2015

Contributor

👍

Сценарий: Потребителите виждат само непрочетени известия
Дадено че съм студент
Дадено че съществува известие "Прочетено известие"
Дадено че съществува непрочетено известие "Ново непрочетено известие"

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Дадено/И/И, не Дадено/Дадено/Дадено. Ditto за другите места.

describe NotificationsController do
describe "GET index" do
it "assigns unread user notifications to @notifications" do
controller.stub_chain(:current_user).and_return(:user)

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Има хелпър за това някъде.

end
describe "GET show" do
notification = FactoryGirl.build_stubbed(:notification)

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Do you even let bruh?

config.before(:each, type: :controller) do
allow(Notification).to receive(:unread_for_user).and_return([])
end

This comment has been minimized.

@skanev
@@ -44,5 +44,10 @@
config.include EmailSpec::Helpers, type: :mailer
config.include EmailSpec::Matchers, type: :mailer
config.include CustomPaths, type: :mailer
config.include Devise::TestHelpers, :type => :controller

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Also nope.

def mark_as_read
self.read = true
save!

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Erghm, update_attributes! read: true?

@@ -41,4 +41,8 @@ def require_admin
def set_time_zone
Time.zone = 'Sofia'
end
def set_notifications_for_current_user
@notifications = Notification.unread_for_user(current_user) if user_signed_in?

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Повтарям: такива глобални instance variable-и са гадни. Замени го с:

helper_method :notifications

def notifications
  @_notifications ||= Notification.unread_for_user(current_user) || []
end
describe "GET show" do
notification = FactoryGirl.build_stubbed(:notification)
it "marks notification as read and redirects to its source" do

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Тук не тестваш, че notification-а се маркира като прочетен.

Отделно, това са два теста

it "marks the notification as read"
it "redirects to the notification's source"
describe NotificationsController do
describe "GET index" do
it "assigns unread user notifications to @notifications" do

This comment has been minimized.

@skanev

skanev May 27, 2015

Owner

Тоя тест е куц, защото контролена не прави това, което пише в името на теста.

@skanev

This comment has been minimized.

Owner

skanev commented May 28, 2015

Ето още малко храна (за размисъл). Две неща са. За първото, как да е, но за второто се изненадвам, че аз трябваше да се сетя, понеже ми се струва, че @mitio и @gsamokovarov ползват evans много повече от мен.

Първото касае защо тия callback-ци не ми харесват.

Има две отделни неща, които кода в момента третира като едно:

  1. Събитие, което има нужда от известие.
  2. Създаването на запис в базата.

В момента пращаме известието при (2), а всъщност, то трябва да бъде пратено при (1). Изглежда, че съвпадат, но само изглежда така. Може би защото има малко видове известия все още, може би защото са изкодени само лесните, а може би и защото изпускаме някой детайл.

Представете си, че добавяме функционалност, позволяваща разговор от решението на задача да се превърне в тема. Админ решава, че дискусията е интересна за всички, и си казва "хайде да я преместим във форумите". За целта трябва да се изтрият коментарите по решението и да се направят Topic + Reply. Само дето, тоя after_create ще създаде нови известния за всеки пост. Трудно можеш да го предотвратиш с текущия дизайн. Може да добавиш original_task_id в topics, но пак е индиректно и трудно се обосновава, ако ти трябва само за това.

Второ, известия за предизвикателства.

Workflow-а ни не е такъв, какъвто предполагаш. Предизвикателството не е публикувано в момента на създаване. Обикновено го създаваме по-рано и го публикуваме чак покрай лекцията. Дори често имаме предизвикателства готови няколко дена по-рано – създадени но непубликувани.

Което пак опира до: известията не се пращат в правилното време понеже третираме създаването на запис и събитието, породило известие като едно нещо, а те са две отделни.

private
def post_notification
Notification.create_notifications_for topic, to: topic.participants, title: "Нов отговор в тема: #{topic_title}"

This comment has been minimized.

@skanev

skanev May 28, 2015

Owner

Като го се замисля, това няма ли да създаде известие за човека, който е пуснал отговора?

@skanev

This comment has been minimized.

Owner

skanev commented May 28, 2015

Яко, че си се помъчил с Cucumber. Не съм му голям фен и действително спира някои мързеливи хора да пишат тестове (гледам към теб, @mitio), така че бонус точки, че си му се хвърлил :)

Липсват ти няколко теста, обаче. Един, който проверява какви notification-и се създават за отговор на тема и друг, който проверява същото за предизвикателства. Знам, че краставицата ги покрива на end-to-end ниво, но трябва да има и unit тест :)

@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented May 28, 2015

@skanev и аз не се кефя на AR callbacks, това за което се боря са обекти (есплицитността), а не генеричната event система.

Като главният аргумент ми е време: ще ни трябва interface, за което ще се бием доста време докато го догодим. Ще има technicalities, с които ще трябва да се оправяме. Ще има и проблеми, които не мога да превидя сега и т.н. И аз обичам да ми е забавно и да експериментирам с интерфейси и идеи, но защо да го правим на чужд гръб?

Общо взето ще зе завъртим за нищо, IMO. Защо мислиш, че ще е по просто? Ние, предполагам, нямаме интерфейса, които ти имаш в главата си. Ако толкова държиш на тази система защо не го изкараш в др. PR, където scope-а му е само това, а не да сучем този? Тук може да решим проблема сега, там може да покажеш "вие сте тъпи, това беше мн. по-яко" и между временно да имаме feature-a готов.

@skanev

This comment has been minimized.

Owner

skanev commented May 28, 2015

Ох. Ощипано. Ще отговоря в разбъркан ред:

Тук може да решим проблема сега, там може да покажеш "вие сте тъпи, това беше мн. по-яко" и между временно да имаме feature-a готов.

Първо, искам да уточня две неща:

  1. Не "решаваме проблем". Добавяме feature, без който, честно казано, може. Не изопачавай фактите, това е слаба реторика.
  2. Не искам да показвам, че някой е тъп. Ако го тълкуваш така, вероятно си едновременно (1) много тънкообиден и (2) надценяваш колко време съм готов да инвестирам в обясняване на очевидни неща в свободното си време, за което никой не ми плаща. Chill.

Ся, ще адресирам някои от коментарите ти, които, често казано, намирам за абсолютна загуба на времето си – най-вече, защото веме съм им отговорил. Очаквам повече.

Като главният аргумент ми е време: ще ни трябва interface, за което ще се бием доста време докато го догодим ... Ние, предполагам, нямаме интерфейса, които ти имаш в главата си.

Bullshit. Интерфейса, който имам в главата си е изразен в gist по-горе. Имплементацията е очевидна. Имате го и няма нищо за догаждане.

"Времето" е аргумент, който ти предлагам и двамата да запазим за работодателите си. В момента сме създали добро learning opportunity по много начини. Ако ти нямаш времето да научиш нещо, защо въобще се занимаваш с преподаване? Ако @skovachev няма време да научи нещо, защо се занимава с pull request?

Допълнително, "ще направим нещо тъпо, щото нямаме време да го направим хубаво" е обидно за хората, чиито труд ползваш. В случая, това е основно моя труд, hence – тона на този отговор.

И аз обичам да ми е забавно и да експериментирам с интерфейси и идеи, но защо да го правим на чужд гръб?

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

В момента правя обратното – опитвам се да науча няколко човека на нещо. Ще спестя имена и неща, но честно казано, от коментарите му тук и от посоките в които бутам проекта можеш да научиш доста. И не само ти. Ся, може да не си съгласен с методите, може да не си съгласен и с това, което се опитвам да ти покажа, а може и въобще да не искаш да бъдеш научен в тоя контекст. Това си е твое решение. На твое място бих пробвал (1) техническата идея и бих излязъл с аргументи за/против или (2) бих се отказал от този PR. Със bullshit аргументи нищо смислено няма да стане, освена да си загубим времето допълнително.

Общо взето ще зе завъртим за нищо, IMO. Защо мислиш, че ще е по просто?

Обясних по-горе. Допускам, че не си го прочел. Мога да го copy/paste-на, мога да го изразя с други думи, а можеш и просто да идеш, да го прочетеш, да помислиш над него и да ми кажеш защо не e по-просто. Ще бъде полезно и за теб, и за мен.

Ако толкова държиш на тази система защо не го изкараш в др. PR, където scope-а му е само това, а не да сучем този?

Наглееш. Искаш да merge-неш лош код, в проект в който съм налял доста пот и сърце. Това е толкова неуважително, че губиш моралното право да се възмущаваш от тона ми в този PR. Just sayin'.

Also, нахално.


TL;DR: Може или всички да научим нещо техническо или да се правим на ощипани. Нямам интерес към второто, нямам интерес и към компромиси.

def create_notifications_for(source, to: nil, title: nil)
unless to.nil?
to.find_each do |user|
Notification.create do |notification|

This comment has been minimized.

@gsamokovarov

gsamokovarov May 28, 2015

Contributor

Notification.create! може да помогнем, като изгърми мощно, ако променим схемата. create просто ще ни върне невалиден notification, който не е записан в базата.

@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented May 28, 2015

Ех, не искам да звучиа ущипано или пък да щипя. Сега, така си се изразявам, нали, не съм се и опитвал да обиждам. Времето не мисля, че bullshit, защото всички знаем колко може да се протакат нещата тук и това e силно демотивиращо. Мога да relate-на до някъде, страничен проект е, но това не помага на мотивацията.

Ta, пак казвам, че не държа да го мържнем по този начин и моите пари са на малки обекчета като този:

class TopicReplyCreator < Struct.new(:topic, :author, :attributes)
  def self.call(*args)
    new(*args).call
  end

  def call
    topic.replies.build(attributes) do |reply|
      reply.user = author
      notifiy_participants if reply.save
    end
  end

  private

  def participants
    topic.participants - [author]
  end

  def notifiy_participants
    title = "Нов отговор на #{topic.title}"
    Notification.create_notifications for: topic, to: participants, title: title
  end
end

Те могат да се използват така:

class RepliesController < ApplicationController
  before_action :require_user, only: :create
  before_action :authorize,    only: %w(edit update)

  def create
    @topic = Topic.find params[:topic_id]
    @reply = TopicReplyCreator.call(@topic, current_user, params[:reply])

    if @reply.valid?
      redirect_to [@topic, @reply]
    else
      render :new
    end
  end

  # ...
end

Защо ми харесва това:

  • Прости руби обекти.
  • Лесно се тестват.
  • Ако имам по-сложна логика, мога да си я разбия в малки intention revealing private методи.
  • Мога да си направя създаването на модела и нотификацията в транзакция, ако потрябва.

Защо не ми харесва това:

  • Оverkill за малки операции.

Защо не ми харесва event bus-a:

  • По-трудно ще ги тествам.
  • По-трудно ще им намеря дефиницията (на класовете мога да скоча с помощта на ctags).

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

@skanev

This comment has been minimized.

Owner

skanev commented May 29, 2015

Йей! Ще може да си говорим технически.

Нека да наречем двете идеи "service layer"-а и "event bus"-а. И понеже вече съм стар и ме мързи да сменям layout във всяко изречение, нека ги наричаме още "слоя" и "автобуса" "рейса". Хем ще е по-забавно.

Тъй.

Първо, да уточним нещо. В момента нямаме належащ проблем, който искаме да решим. Също, това не е PR, който имплементира цял развит feature. Той единствено поставя основи. Създава notification-и за две неща:

  • Ново предизвикателство
  • Отговор на тема

Ако сме сериозни за този feature, имаме нужда от известия за още:

  • Нова задача.
  • Резултати от нова задача.
  • Коментар по задача (твоя, или такава, в която участваш).
  • Може би новина?

И прочее.

Както и да дизайнем сега, избираме какви основи полагаме. Няма нужда да правим супер мощната система за всичко, но е добре да съпоставим посоката в която движим дизайна, с посоката, в която вървем. Да минем на спицъсите.

Защо слоя ти харесва:

  • Прости руби обекти.

Рейса също са прости руби обекти. Дори DSL-а за event handler-и който съм набутал е достатъчно прост. Виждам как едното може да е "по-просто" но не виждам как разликата между двете може да е съществена.

Съответно, би трябвало и рейса да ти харесва, защото той има просто Ruby обекти.

  • Лесно се тестват.

Че защо рейса да се тества трудно? Контролер:

describe TopicController do
  describe 'POST create' do
    context 'when successful' do
      it 'triggers an event' do
        EventBus.should_receive(:trigger).with(:post_created, new_post)
        post :create
      end
    end
  end
end

Модел:

describe NotificationSubscriber do
  describe "(reply notifications)" do
    def notifications_for(user, source)
      Notification.where(:user_id: user.id, source_id: source.id, source_type: source.class.name)
    end

    it "notifies all participants in the topic for the new reply" do
      topic = create :topic
      previous_reply = create :reply, topic: topic
      new_reply = create :reply, topic: topic

      NotificationSubscriber.process(:post_created, reply)

      notifications_for(previous_reply.user, new_reply).exist?.should be true
      notifications_for(topic.user, new_reply).exist?.should be true
    end

    it "doesn't notify the reply's author" do
      topic = create :topic
      reply = create :reply, topic: reply

      NotificationSubscriber.process(:post_created, reply)

      notifications_for(reply.user, reply).exist?.should be false
    end
  end
end

Понеже това се тества просто, също би трябвало да ти харесва.

  • Ако имам по-сложна логика, мога да си я разбия в малки intention revealing private методи.

Тук защо да не можеш?

class NotificationsListener < EventBusSubscriber
  def participants_to_be_notified(reply)
    author = reply.user
    reply.topic.participants - [author]
  end

  on :reply_created do |reply|
    Notification.create_notifications for: topic,
                                      to: participants_to_be_notified(reply),
                                      title: "Нова отговор на #{topic.title}",
  end
end

Обърни внимание, че не съм съгласен, че това е достатъчно сложна логика, която има нужда да се изнесе в метод. Но имаш как да го направиш.

  • Мога да си направя създаването на модела и нотификацията в транзакция, ако потрябва.

Това не е аргумент, защото няма да потрябва. Говорихме си, че трябва да става в background-а, remember? Иначе:

class ChallengesController < ApplicationController
  def create
    @challenge = Challenge.new params[:challenge]

    ActiveRecord::Base.transaction do
      @challenge.save!
      EventBus.transmit :challenge_created, @challenge
    end

    redirect_to @challenge, notice: 'Предизвикателството е създадено успешно'
  rescue ActiveRecord::InvalidRecord
    render :new
  end
end

Пак можеш да го направиш. Има недостатъци, но: Няма. Да. Потрябва. YAGNI.

Защо не ти харесва рейса:

  • По-трудно ще ги тествам.

Вече уточнихме, че това не е вярно.

  • По-трудно ще им намеря дефиницията (на класовете мога да скоча с помощта на ctags).

IDE-то е наклонена плоскост. Това е несериозен аргумент. Какво става в редактора не трябва да участва в това какви дизайн решения вземаш. Особено в редактор, в който можеш да направиш ctags да прави каквото си поискаш и use case, който се решава с ack.

Сега, малко коментари от моя страна по твоята имплементация на слоя.

  • Както го направи, view/controller връзката стана по-скложна. Вместо @reply, имаме някакъв @creator. Да, ама формата вече е за @creator.reply.
  • call метода е рошав. Допълнително, повтаря един if който контролера вече прави.
  • Няма метод valid?. Вероятно имаш предвид @creator.reply.valid? или директно @creator.valid?. Хубаво, ама така викаш валидацията два пъти. Можеш да преместиш резултата от save в променлива и да питаш method object-а за него, но това е друга тема. При всички случаи е по-сложно.

Ето ти алтернатива, която според мен е по-добър слой:

class RepliesController < ApplicationController
  def create
    @topic = Topic.find params[:topic_id]
    @reply = @topic.replies.build params[:reply]
    @reply.user = current_user

    if @reply.save
      ReplyNotificationCreator.notify_about(@reply)
      redirect_to [@topic, @reply]
    else
      render :new
    end
  end
end

В крайна сметка, ако искаме малки обекти, нека да са малки обекти.

Обаче, не мисля, че добрия слой е хубава идея. Мисля, че добрия рейс е хубава идея. Защо? Практическите причини вече ги изброих. Ето и философската:

Ще вкараш нов pattern. Нямам нищо против новите pattern-и, но този: нито е широко приложим тук, нито решава проблема по-добре. Също, тегав е разбиране, тегав е и за репликиране. Моята идея за слой, твоята идея за слой и нечия друга идея за слой ще са три различни неща. Аз ще имам едни ценности, ти ще имаш други. За мен едно нещо ще е overkill, за теб ще е друго. Когато структурата на кода не показва кога един pattern е добра идея, този pattern е труден за обясняване.

Също, почваш да губиш симетрия. Защо имаш ReplyCreator, но не и ReplyEditor? Струва ли си да имам за да бъде симетрично? Overkill ли е? Все трудни въпроси. Когато няма достатъчно код наоколо стават по-трудни.

Рейса няма тоя проблем. Или дори да го има, е в много по-малка степен.

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


Нека да завършим на нетехническа нотка:

Времето не мисля, че bullshit, защото всички знаем колко може да се протакат нещата тук и това e силно демотивиращо. Мога да relate-на до някъде, страничен проект е, но това не помага на мотивацията.

Не ми пука за еднорозии и дъги. Аз се мотивирам от технологията. Чувствата и добрия тон са загуба на свободното ми време. Не ни се налага да правиш неща заедно – избираме да го правим.

Твърдя, че в evans има много, от което може да се научи. В този pull request също има много, което да се научи. Ако мотивацията ти е да "научиш" нещо, си попаднал на добро място. Както виждаш, отговарям на всякакви технически въпроси, и съм склонен да бистря всякакви идеи в детайли. Полезно е и за мен.

Ако мотивацията ти е, че ще се почувсташ оценен и ще ти стане готино от merge-нат PR, докато някой те залива с листа от рози докато същевременно ти отговаря културно, учтиво и подброно за всичко дребно, което не си си направил труда да помислиш – то, дошъл си на грешното място :)

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

Докосващо, но лицемерно. Да започнем от тук:

Оценявай труда на другите, които е напъно безвъзмезден към този проект.

Ще припомня две неща:

  • Не съм карал никого да използва еvans.
  • Не съм молил никого да пуска код.

Радвам се, ако се окаже полезен на някого, било то научавайки нещо от него или ползвайки го за свои цели. Затова е в GitHub и затова кода е отворен.

Обаче, ако ми трябва нещо, ще си го направя сам. Тоя pull request не ми трябва. Защо въобще е тук?

Може би му трябва на някой друг? Може би автора е искал да научи и да упражни нещо? Не трябва ли, аджеба, моя труд да се уважава, че съм прекарал немалко вечери да създам контекст, в който това може да се случи?

Щото ако има нещо неуважително, то е да ми казваш "аре да го merge-нем, щото ни мързи да инвестираме повече време". Ако те мързи, не се занимавай. Или, нали, има fork бутон. Whatever. Просто не се дръж всякаш ти дължа нещо. Обратното няма да го коментирам.

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

Чак пък да ме карат да се чувствам добре. Всъщност, издразнен съм, понеже вместо да бистрим интересния въпрос, трябва да се занимавам с "мотивация" и да обяснявам тия неща. Сигурно е шоу отстрани, но само ми губи времето, честно казано. Или, нали, освен ако няма смисъл:

Дали си струваха тези думи, си прецени сам.

Разбира се. Показват кои къде стои. Показват какво мисля за "оценявай труда на другите, който идва безвъздмездно". Показвам и какво мисля за "какво влияе на мотивацията". От там нататък, всеки си решава дали да продължи да engage-ва или не.

@s2gatev

This comment has been minimized.

Collaborator

s2gatev commented May 29, 2015

Харесва ми това, че изпращането на известието остава в контролера. Супер.

При слоя на @gsamokovarov не ми харесва, че записването на обекта става в слоя. Колко подобни слоеве можем да имаме за дадена операция? Според мен е повече от един (примерно мейл слой). При слоя на @skanev този проблем го няма.

Не ми харесва, че при слоя говорите за ReplyNotificationCreator.notify_about. Какво става когато добавим и другите типове известия? Защо не го обърнем и не ги организираме в един обект? NotificationCreator.notify_about_reply.

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

Относно рейса:

Плюс:

  • Произволен брой слушатели (известия, мейли, tweets, whatever...)
  • Не пипаме publisher-а, когато добавяме ново събитие. Примерно добавяме мейл subscriber, който се закача на същото събитие.
  • Нещата остават разкачени и изолирани. Нито контролера знае за subscriber-ите, нито те един за друг.

Минус:

  • Още една абстракция. В случая съм по-склонен да живея с нея, но това е по-скоро защото не виждам алтернатива. Обаче, определено мисля че броя на термините в този проект става доста голям.
@skanev

This comment has been minimized.

Owner

skanev commented May 29, 2015

@s2gatev 👍

Малко уточнение:

Минус:

  • Още една абстракция. В случая съм по-склонен да живея с нея, но това е по-скоро защото не виждам алтернатива. Обаче, определено мисля че броя на термините в този проект става доста голям.

Докато не съм несъгласен с теб, слоя страда от много подобен проблем. Вкарва още един pattern и то такъв, който не е много универсален и всеки го прави различно. Дори, бих казал, че рейса вкарва ясна абстракция, докато слоя вкарва неясна такава. Именно това поражда въпроси като "Ако направя ReplyCreator, трябва ли да направя ReplyEditor"?

@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented May 29, 2015

Нека да завършим на нетехническа нотка:

Окей, това може да се проточи. Нека спрем със глупостите и да караме технически. Аз се издразних и скочих и да съм те обидил, не е била това целта, нали. Спирам да бутам за времето, дай да го направим така, че всички да сме щастливи накрая. ✌️


Именно това поражда въпроси като "Ако направя ReplyCreator, трябва ли да направя ReplyEditor"?

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

Сега, малко коментари от моя страна по твоята имплементация на слоя.

Сега, това е скеч имплементация. #call връща Reply, точно за да не нося creator-а. Дали е рошав, дали да го преименоваме, дали да кръщаваме сервизите като глаголи са се решение, все предочитания. Не държа на "мойто", стига да е консистенто навсякъде. Вкрайна сметка това са technicalities и може да ги изгладим.

Друг минус с рейса, за мене е търсенето е имплицитността на subscriber-а. Когато тригърна нещо, на колко места трябва да погледна за да знам какво ще пусна? Да кажем, че това не е особен проблем за evans, тъй като не е мн. голям и ако търсим записали се само в EventBusSubscriber-и аз съм окей.


@skanev Как го виждаш това API малко по-специфично?

EventBusSubscriber

  • def self.on(even_name) - приема име на събитие и блок с произволен брой аргументи? Блока има достъп до instance методи?
  • def self.subclass - Тук ли се случва закачването към централният сторидж на събития? Класова променлива в EventBusSubscriber ли е сториджа (защото наследниците я споделят)? Такива неща.

Друг интерфейс има ли? Къде да оставим имплементацията и къде да живеят subscriber-ите? lib/event_bus{_,/}subscriber.rb, app/subscribers? Вариант ли е ползването на ActiveSupport::Notifications за разхвърлято на събития или си го правим изцяло ние?

EventBus

  • def self.trigger(event_name, *args). Това само възбужда събития? Грешките ги оставяме да гърмят, което според мен е разумно, или мислим стратегия за подтискане на грешки и се надяваме да изкараме всичко, което ще оживее? Искаме ли всичко или нищо e.g. транзакция, ако кажем че главно AR-неща ще минават през нас – един listener успява, др. фейлва; оставаме ли нещата така или чистим. Бърка ли ни АR като цяло?

Друг интерфейс има ли?

@skanev

This comment has been minimized.

Owner

skanev commented May 29, 2015

Много сложно го мислиш. Няма нужда от storage:

module EventBus
  extend self

  def subscribers
    @subscribers ||= []
  end

  def register(subscriber)
    subscribers << subscriber
  end

  def trigger(event_name, *args) do
    subscribers.each do |subscriber|
      subscriber.process event_name, *args
    end
  end
end

class EventSubscriber
  class << self
    def on(event_name, &block)
      define_method :"on_#{event_name}", &block
    end
  end

  def process(event_name, *args)
    handler = :"on_#{event_name}"

    send(handler, *args) if respond_to?(handler)
  end
end

Това е скеч след шест часа шофиране, така че сигурно има много грешки. Два коментара:

  1. Оставям въроса "как навързваме subscriber-и с bus"-а отворен. В dev и prod е нещо просто (всички наследници, например), но в test е добре да бъде opt-in – нищо, освен ако тоя тест не го поиска. Имам няколко идеи, не мисля, че има голямо значение на тоя етап.
  2. @mitio предложи вместо on :whatever do да има def on_whatever. ОК съм с това (ще разкара един метод от EventSubscriber), но предпочитам on за да има симетрия между event-а и handler-а: и в двата случая е просто :whatever. Дребен детайл е, но урока който научих от Ruby, е че дребните детайли за важни.

Друг интерфейс има ли? Къде да оставим имплементацията и къде да живеят subscriber-ите? lib/event_bus{_,/}subscriber.rb, app/subscribers?

Така ми изглежда супер. Само не знам дали да е "subscriber", "listener" или нещо друго.

Вариант ли е ползването на ActiveSupport::Notifications за разхвърлято на събития или си го правим изцяло ние?

Бих казал "по-скоро не", но не съм сигурен защо. Това горе ми се струва по-просто. А и ActiveSupport::Notifications в тоя случай ми опъва някаква струна накриво. Ако настояваш за него, ще помисля какъв точно ми е проблема.

Грешките ги оставяме да гърмят, което според мен е разумно, или мислим стратегия за подтискане на грешки и се надяваме да изкараме всичко, което ще оживее?

Не съм сигурен за кои грешки говориш. Можеш да объркаш име на събитие, но тогава просто няма handler ;)

Искаме ли всичко или нищо e.g. транзакция, ако кажем че главно AR-неща ще минават през нас – един listener успява, др. фейлва; оставаме ли нещата така или чистим. Бърка ли ни АR като цяло?

Бих казал "не". Ако ти трябва нещо с транзакция (пример би бил полезен), е хубаво транзакцията да е на едно място, а не разхвърляна из кода. Съответно, ако искам да направя X неща в транзакция, бих си направил обектче за тях, не бих ги разхвърлял из събития.

Припомням, че не виждам случай, където транзакциите са фактор. Не ми се струва, че reply-а и notification-ите трябва да са в транзакция, още повече защото notification-ите ми се струват подходящи за job :)

@skanev

This comment has been minimized.

Owner

skanev commented May 29, 2015

Друг минус с рейса, за мене е търсенето е имплицитността на subscriber-а. Когато тригърна нещо, на колко места трябва да погледна за да знам какво ще пусна?

:Ack 'on :whatever' app/subscribers
@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented May 31, 2015

Ако настояваш за него (ActiveSupport::Notifications), ще помисля какъв точно ми е проблема.

Не настоявам, мисля че е достатачно просто и без него.

Не съм сигурен за кои грешки говориш.

Когато handler хвърли exception, но за сега не мисля че има смисъл да правим кавкото и да е. Нека гърми да се вижда.

Супер тогава, имаме план :)

@skanev

This comment has been minimized.

Owner

skanev commented May 31, 2015

Когато handler хвърли exception, но за сега не мисля че има смисъл да правим кавкото и да е. Нека гърми да се вижда.

Indeed. Няма асинхронност, няма транзакции – да ходи в bugsnag-а :)

БТВ, щом имаме план, който се наеме да го имплементира, моля да разкара досегашните observer-и :)

@skovachev

This comment has been minimized.

skovachev commented Jun 1, 2015

За мен цялата дискусия определено ми беше (и е) изключително интересна и
полезна. Благодаря за което!
Ще обърна внимание на всички коментари и при първа възможност пускам ъпдейт.
On нд, 31.05.2015 г. at 20:44 Stefan Kanev notifications@github.com wrote:

Когато handler хвърли exception, но за сега не мисля че има смисъл да
правим кавкото и да е. Нека гърми да се вижда.

Indeed. Няма асинхронност, няма транзакции – да ходи в bugsnag-а :)

БТВ, щом имаме план, който се наеме да го имплементира, моля да разкара
досегашните observer-и :)


Reply to this email directly or view it on GitHub
#163 (comment).

@skanev

This comment has been minimized.

Owner

skanev commented Nov 23, 2015

@s2gatev

This comment has been minimized.

Collaborator

s2gatev commented Jan 21, 2016

Нещо против аз да довърша това?

@skovachev

This comment has been minimized.

skovachev commented Jan 21, 2016

@s2gatev Аз съм ок. Имайки предвид кога е последния път, когато имах време да седна да го пипна, изглежда е добра идея.

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