-
Notifications
You must be signed in to change notification settings - Fork 6
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 webhooks in automation rules #455
Conversation
app/models/automation/webhook.rb
Outdated
def standard_webhook_data(thing) | ||
if thing.instance_of?(::MessageThread) | ||
{ | ||
message_thread_id: thing.id | ||
} | ||
elsif thing.instance_of?(::Message) | ||
{ | ||
message_id: thing.id, | ||
message_thread_id: thing.thread.id | ||
} | ||
else | ||
throw StandardError.new "Unsupported webhook thing object: '#{thing.class.name}'" | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tuto chces double dispatch pripadne polymorfiu na urovni response az vo view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zatiaľ je všetko iba Message, takže som tieto podmienky vyhodil preč
@celuchmarek tu este dve veci mozno, co by sme rovno mohli skusit ako test toho ci to funguje dobre:
|
@jsuchal updatol som podľa review a podľa dohody. Idem ešte nejaký test spraviť. |
app/models/automation/webhook.rb
Outdated
|
||
def fire!(message, event, timestamp, downloader: Faraday) | ||
data = { | ||
type: "#{message.class.name}.#{event}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu by som mozno pridal underscore
nech drzime konvencie rails/ruby
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
podľa toho štandardu má byť bodka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to chapem, ale nazov triedy je camelcase a vsetko ostatne je underscore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jo, už rozumiem. Dal som .underscore
app/components/settings/rules/automation_rules_table_row_component.html.erb
Show resolved
Hide resolved
EventBus.subscribe_job :message_thread_created, Automation::MessageThreadCreatedJob | ||
EventBus.subscribe_job :message_created, Automation::MessageCreatedJob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toto som si teraz vsimol, potencialne sa moze stat problem ked v queue budu joby ktore nevie odrazu kod spracovat (lebo uz nema ako)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jo, nasadíme tak, aby tam neboli.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zvykne sa to robit na 2 fazy, naprv nasadis nove, potom ked dobehnu joby, tak nasadis fix co vyhodi joby.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, vrátil som
app/lib/event_bus.rb
Outdated
EventBus.subscribe_job :message_thread_created, Automation::MessageThreadCreatedJob | ||
EventBus.subscribe_job :message_created, Automation::MessageCreatedJob | ||
[:message_thread_created, :message_created, :message_draft_submitted].each do |event| | ||
EventBus.subscribe_job event, Automation::EventTriggeredJob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ked uz robime toto, tak to nazvime asi podla toho co to aj robi Automation::ApplyRulesForEventJob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
premenoval som
@celuchmarek swagger nejaky a/alebo pouzivatelsku prirucku netreba upravit? |
@jsuchal pridal som do swaggeru. Chceme aj do tej PDF dokumentácie? |
queue_as :automation | ||
retry_on StandardError, wait: :polynomially_longer, attempts: 10 | ||
|
||
def perform(webhook, thing, event, timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu len poznamka bokom, ze toto je primitive obsession a vlastne by v evente mal byt ten thing aj timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ale chapem, ze nechces vyrabat serializer/deserializer kvoli tomu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Este kukni malu vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dokumentaciu hodme do JIRA nech sa nezabudne na to.
Testy dorobím, naprjv ale chcem vedieť, či je to dobre takto. V podstate som do Automation rules pridal ďalšie triggery z EventBus (message_thread_changed, message_thread_created, message_updated).
Tie webhooky vedia byť prázdny POST na zadanú URL alebo obsahujú body podľa standard-webhooks. T.j. napr:
Autorizáciu webhookov som zatiaľ tiež neriešil, ale IMHO to zatiaľ netreba. Ale je to %lahké dorobiť - do nejakého headera pridáme podpis, prípadne jwt.