Skip to content
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

Remove notifications key from ocs response #1819

Merged
merged 6 commits into from
Aug 4, 2021

Conversation

refs
Copy link
Member

@refs refs commented Jun 23, 2021

No description provided.

@refs refs requested a review from labkode as a code owner June 23, 2021 13:59
@refs
Copy link
Member Author

refs commented Jun 23, 2021

/cc @pascalwengerter

@refs
Copy link
Member Author

refs commented Jun 23, 2021

Again, the semantics are up to the clients to understand what it means to have a notifications key; for which we've seen clients (at least web) assumes that if the key is present, do notifications. This logic might be wrong because it could potentially break clients, the issue is that I'm unaware of that. @butonic do you have more input here?

@refs
Copy link
Member Author

refs commented Jul 2, 2021

cc @pascalwengerter is this still relevant enough to bring it up on monday in an impro meeting?

@pascalwengerter
Copy link
Contributor

cc @pascalwengerter is this still relevant enough to bring it up on monday in an impro meeting?

Yes, please! Causes buggy behaviour in web & mobile clients, would be nice to have that streamlined and fixed :)

@pascalwengerter
Copy link
Contributor

@labkode can this get some attention soon? Reva incorrectly announcing capabilities causes errors in web and other clients

@labkode
Copy link
Member

labkode commented Jul 15, 2021

@pascalwengerter I was waiting for the confirmation about your meeting.
Do you need the current PR to be merged?

@pascalwengerter
Copy link
Contributor

@pascalwengerter I was waiting for the confirmation about your meeting.
Do you need the current PR to be merged?

Could you have a chat with @refs about it? We should get this merged to avoid unnecessary notifications requests in web & other clients due to Reva communicating its capabilities in an unexpected way 😉

@butonic
Copy link
Contributor

butonic commented Jul 29, 2021

@refs make this green again, then this can be merged! @pascalwengerter talked to @TheOneRing and the mobile teams: the notifications property should be omitted.

@butonic
Copy link
Contributor

butonic commented Jul 29, 2021

@labkode
Copy link
Member

labkode commented Aug 2, 2021

@refs one minor nit

@labkode labkode self-requested a review August 4, 2021 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants