Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jun 4, 2020

@nickvergessen nickvergessen requested a review from danxuliu July 9, 2020 09:37
@nickvergessen nickvergessen force-pushed the techdebt/noid/deferrable-notification-apps branch from 176195b to 429305b Compare July 9, 2020 09:37
@nickvergessen nickvergessen requested a review from rullzer July 9, 2020 09:38
@nickvergessen nickvergessen marked this pull request as ready for review July 9, 2020 09:38
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen force-pushed the techdebt/noid/deferrable-notification-apps branch from 57f203d to b8503d0 Compare July 9, 2020 14:23
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

Should the flushing be added to CheckHostedSignalingServer.php too?

I have noticed that when a conversation with several participants is created there is still one notification sent for each user, although the issue seems to be that an AddParticipantsEvent is emitted for each user instead of one containing all the users, so it is not related to these changes.

@nickvergessen
Copy link
Member Author

Should the flushing be added to CheckHostedSignalingServer.php too?

Since this is in a background job, I don't care too much.

@nickvergessen nickvergessen merged commit 51dc317 into master Jul 28, 2020
@nickvergessen nickvergessen deleted the techdebt/noid/deferrable-notification-apps branch July 28, 2020 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants