-
Notifications
You must be signed in to change notification settings - Fork 435
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
fix(federation): Sync room properties on join #13072
Conversation
/backport to stable30 |
baff185
to
d168309
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not fully convinced on adding a specific method to update the database in a single query. While saving database queries is of course good, not many queries might be saved with that, so the standard RoomService
methods could be used instead. In the initial sync it may help, but after that it would only help if some cloud notification is missed, as right now it syncs more properties than what is explicitly propagated (for example, the call recording state), but those missing properties should be propagated anyway; otherwise a federated user already in the room would not receive the change until another federated user joins again.
Syncing all the properties in one go in general seems to have the same effect as using the standard RoomService
methods, but not sending the individual modification events could also miss some behaviours (like invalidating the reference cache), and there could be also missing behaviours on some specific methods (like resetting the participant permissions when the room permissions are modified). So I am not sure if the few saved database queries are worth the trouble of handling those things 🤔
Independently of that, CI does not seem to be happy.
my main intention was to avoid certain reactions of other components listening to the hooks |
Most of red CI is due to nextcloud/server#46991 |
73e4c25
to
c3c9112
Compare
Changed it to the RoomService methods now, made the SignalingListener pause on BeforeRoomSync and send a single signaling messages afterwards. |
|
c3c9112
to
16711be
Compare
16711be
to
fdb2755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Force pushed to fix the check against the last activity.
I have also added a fixup, but not applied it yet, to update the lobby. Before only the lobby state was checked, but not the timer. Therefore, if the lobby was previously set but only the timer changed when the data was synchronized again it was not updated. But given that there was a condition inside the external if that took into account the timer maybe I am missing something, so I prefered not to apply it without a review from @nickvergessen
Other than that and the comment, tested and works 👍
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
fdb2755
to
9dda21c
Compare
☑️ Resolves
Follow-up ideas
🛠️ API Checklist
🏁 Checklist
docs/
has been updated or is not required