Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Feb 28, 2022

@nickvergessen nickvergessen added this to the Nextcloud 24 milestone Feb 28, 2022
@nickvergessen nickvergessen self-assigned this Feb 28, 2022
@nickvergessen nickvergessen changed the title Techdebt/noid/group queries on pushing Group DB queries on pushing multiple notifications until we flush() Feb 28, 2022
@nickvergessen nickvergessen force-pushed the techdebt/noid/group-queries-on-pushing branch from 39d9452 to 42d64b0 Compare February 28, 2022 14:49
@nickvergessen nickvergessen marked this pull request as ready for review February 28, 2022 14:50
@nickvergessen nickvergessen force-pushed the techdebt/noid/group-queries-on-pushing branch from 42d64b0 to 86c3eb3 Compare March 1, 2022 09:03
@nickvergessen
Copy link
Member Author

Rebased after merge of #1157

@nickvergessen
Copy link
Member Author

Please review @vitormattos @danxuliu so this goes in for 24

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.

Other than the comments, code looks good (but I have not tested it).

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, from my limited knowledge, works 👍

Note that I have not tested the scenario of having some device or status already loaded and then having to load some more when flushing the payloads (as I did not know how to trigger that :-) ); I just tested starting a call in Talk, which caused all the devices and statuses to be loaded in flushPayloads.

@nickvergessen nickvergessen force-pushed the techdebt/noid/group-queries-on-pushing branch from 54b1d27 to 7024894 Compare March 14, 2022 12:46
@nickvergessen
Copy link
Member Author

Note that I have not tested the scenario of having some device or status already loaded and then having to load some more when flushing the payloads (as I did not know how to trigger that :-) )

It is currently not possible without hardcoding something. There is no API call which sends a notification to 1 user and afterwards to a second set of users

Copy link

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

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

Do we have integration tests covering this?

@nickvergessen nickvergessen merged commit a9d7b15 into master Mar 15, 2022
@nickvergessen nickvergessen deleted the techdebt/noid/group-queries-on-pushing branch March 15, 2022 17:02
@nickvergessen
Copy link
Member Author

Not really. The problem is there is no default API using this. We would need to add one.
But basically the Talk team tests it with every call on mobile apps as the call screen notification is it.

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