Add webhooks for PeerConnected and PeerDisconnected events #104
Conversation
Codecov Report
@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 85.30% 85.73% +0.42%
==========================================
Files 44 46 +2
Lines 796 834 +38
==========================================
+ Hits 679 715 +36
- Misses 117 119 +2
Continue to review full report in Codecov by Sentry.
|
mickel8
left a comment
mickel8
left a comment
There was a problem hiding this comment.
Nicely done! 🔥 I've just added a couple of comments mostly related to the code consistency
lib/jellyfish/room.ex
Outdated
| def registry_id(room_id), | ||
| do: {:via, Registry, {Jellyfish.RoomRegistry, room_id}} |
There was a problem hiding this comment.
Is there any reason why we moved this code here? In general I would recommend introducing as few changes as possible so if there is no reason for changing a place for this function I would keep the previous version
There was a problem hiding this comment.
During testing another approach with storing webhook_url in Registry, I found out that we create registry_id from hand in RoomService, which is error-prone because if we change how registry_id is made in Room we could forget to change it in RoomService
There was a problem hiding this comment.
Ah, now I see. Let's move this then to the top, before start function
| alias Phoenix.PubSub | ||
|
|
||
| @port 5907 | ||
| @webhook_port 2137 |
There was a problem hiding this comment.
Yeah yeah, I know this meme but maybe we should not mixin political memes in the code? I am not sure 🤔 Maybe use panuozzo pollo e pancetta price x2 e.g. 2929
lib/jellyfish_web/peer_socket.ex
Outdated
| end | ||
|
|
||
| @impl true | ||
| def terminate(_reason, state) do |
There was a problem hiding this comment.
I would get rid of this function at all as it won't be called in every case so sometimes we get this log and soimetimes not
There was a problem hiding this comment.
This function is required by behavior Phoenix.Socket.Transport, but if you want I can remove this log from it.
lib/jellyfish/room_service.ex
Outdated
| def find_room(room_id) do | ||
| case Registry.lookup(Jellyfish.RoomRegistry, room_id) do | ||
| [{room_pid, nil}] -> | ||
| [{room_pid, _webhook_url}] -> |
There was a problem hiding this comment.
I would change the _webhook_url to a different name as we never assign webhook to registry id.
Co-authored-by: Michał Śledź <[email protected]>
Co-authored-by: Michał Śledź <[email protected]>
Co-authored-by: Michał Śledź <[email protected]>
No description provided.