feat: presence enabled accepted as a valid to connect - #2071
feat: presence enabled accepted as a valid to connect#2071filipecabaco wants to merge 2 commits into
Conversation
CRAP Score Report |
PragTob
left a comment
There was a problem hiding this comment.
👋 Hi I have no idea what I'm doing. But I felt like reviewing to get a feel for the code. Hence I'm also not approving, as I don't feel like I know enough to approve yet.
All looks good to me, but this is the first time I'm taking a look at the code base 😅
You know me, but still: Hi I'm Tobi, I tend to leave a lot of comments with the intent to learn from each other. It's perfectly normal for me to leave 5+ comments but approve the PR because it's minor stuff that shouldn't stop delivery or just genuine question, interest or praise.
Here, I think the Map.values() one deserves a look as I hate flaky tests. The others are questions/praise/highly optional.
| {:rc_fastlane, fastlane_pid, serializer, topic, log_level, tenant_id, replayed_message_ids, presence_read?, | ||
| broadcast_read?} |
There was a problem hiding this comment.
maybe the creation/read speed for tuples outweighs this here, but otherwise feels like a struct may be beneficial (would cause less code changes on an update, as we don't need to reiterate all fields all the time)
| end | ||
|
|
||
| assert Agent.get(TestSerializer, & &1) == 0 | ||
| refute_receive _any |
There was a problem hiding this comment.
we could consider switching it to refute_received or adjusting the timeout, depending on timeout this might wait 100ms which we may or may not want :)
edgurgel
left a comment
There was a problem hiding this comment.
What happens if the user gains access to read broadcasts on a new refreshed token?
|
@edgurgel added the logic to be processed during a refresh token |
395f10d to
165ab1c
Compare
165ab1c to
b89ce97
Compare
What kind of change does this PR introduce?
presence enabled accepted as a valid to connect