Add backend for conversation descriptions #4587

danxuliu · 2020-11-13T03:03:19Z

Requires #4656
Probably should be merged after #4496 (there will be conflicts, but it should be easier to fix them here)

Fixes (the backend part of) #3432

Only moderators and guest moderators of group and public conversations can set their description.

Setting and getting the description is available only in version 3 of the API.

The description is returned along the rest of the information of a conversation. However, as the description is not typically needed in the conversation list, it could be potentially "large" and the conversation list is fetched frequently only a hash of the description is returned in that case. The exception is when the description is empty, in which case an empty value is returned, instead of the value of sha1('').

~~If only a single conversation is fetched the actual description is always returned.~~ Due to design reasons the description is now limited to just 250 characters. Moreover, the OCS endpoint is gzip compressed, so sending the raw description instead of the hash when fetching the conversation list does not reduce the size too much, but introduces an inconsistency between the data of a single conversation and all conversations, which requires a more complex logic in the clients (and specifically in the WebUI it would require quite some changes). Due to all this, for simplicity the raw description is also returned when getting the conversation list.

Also for simplicity the owner of a password request conversation can currently set the description, even if there is probably no point on doing that. Preventing it would be easy, though, just something similar to 630e2f4

When the description is changed the external signaling server is notified and the new description is sent; the description, like the other properties, is sent whenever the conversation is modified, even if the description itself was not modified, but (if I recall correctly) this is needed for the external signaling server to track the changes.

Pending:

Return description too in SIP bridge requests? @nickvergessen -> Not needed
Handle error setting a description too long in the database? - The field is now declared as TEXT without a limit, so in practice the description set will never be too long
How to check that the database supports multibyte strings to use strlen instead of mb_strlen if it does not? Or remove the guard from the controller and just handle if there is an error when setting the description in the database? Or do both (for example, always guard with mb_strlen, but also handle the error)?
Add integration tests for video verification and file rooms
Add support for conversation description to occ commands
Send system message when the description is changed
Add documentation
Something else? Not sure if I am missing something

nickvergessen · 2020-11-13T06:31:00Z

I don't think the SIP bridge needs the description. If you put a link in there e.g. for a shared presentation, people would get it read out or what?🙈

danxuliu · 2020-11-13T08:51:53Z

I don't think the SIP bridge needs the description. If you put a link in there e.g. for a shared presentation, people would get it read out or what?see_no_evil

I have no idea, that is why I asked :-P But ok, no description for SIP bridge then ;-)

docs/conversation.md

nickvergessen · 2020-11-27T07:51:06Z

lib/Chat/Parser/SystemMessage.php

 				$parsedMessage = $this->l->t('An administrator renamed the conversation from "%1$s" to "%2$s"', [$parameters['oldName'], $parameters['newName']]);
 			}
+		} elseif ($message === 'description_set') {
+			$parsedMessage = $this->l->t('{actor} set the description to "%1$s"', [$parameters['newDescription']]);


I would not put the description into the system message.
It can be potentially long and also contain " and then look broken.

Whatsapp and others just write "{actor} updated the description" alike messages.

And we can then use this for create, update and remove at the same time.

@ma12-co checked other platforms and asked for the description to be shown in the message.

I think it should be shown, given that:

It's likely to be very important information for the conversation participants;

It's s going to be relatively concise since we're capping the max length to 500 chars, so it's not going to be disruptive;

lib/Chat/SystemMessage/Listener.php

lib/Controller/RoomController.php

lib/Migration/Version11000Date20201011082810.php

lib/Room.php

nickvergessen · 2020-11-27T08:02:58Z

lib/Room.php

+			$properties = array_merge($properties, [
+				'description' => $this->getDescription(),
+			]);


This is not changing anything on the signaling?

What do you mean?

This is getPropertiesForSignaling but the data is not used in signaling, so we don't need to expose it?

The new description will be relayed by the signaling server to the clients in the roomlist->update message. Currently we are not using the updated properties from the message, though, but I think it is better to send it for consistency with the rest of the update messages.

tests/integration/features/command/create.feature

lib/Migration/Version11000Date20201011082810.php

danxuliu · 2020-12-04T01:48:58Z

Squashed and rebased. It should be ready for a final review :-)

The only missing thing would be the How to check that the database supports multibyte strings to use strlen instead of mb_strlen if it does not?. However, given that the database is now a text field with no length set I guess that is no longer an issue, as even if the string contains multibyte characters but the database does not support them this would not cause the description to be too long to be stored in the database.

nickvergessen · 2020-12-08T08:35:59Z

The only missing thing would be the How to check that the database supports multibyte strings to use strlen instead of mb_strlen if it does not?. However, given that the database is now a text field with no length set I guess that is no longer an issue, as even if the string contains multibyte characters but the database does not support them this would not cause the description to be too long to be stored in the database.

mb is always better to use, as it prevents cutting inside a utf8 char

nickvergessen · 2020-12-08T11:56:27Z

Conflicting files
tests/integration/features/bootstrap/FeatureContext.php

I will rebase it, happened with #4702

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Only moderators and guest moderators of group and public conversations can set their description. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

As the description is not typically needed in the conversation list, it could be potentially "large" and the conversation list is fetched frequently only a hash of the description is returned in that case. The exception is when the description is empty, in which case an empty value is returned, instead of the value of "sha1('')". If only a single conversation is fetched the actual description is always returned. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

As the description length is limited and the response for the conversation list endpoint is gzip compressed the hash does not provide too much of a reduction, but introduces a more complex logic in the clients. Due to all this, for simplicity now the raw description is also returned when getting the conversation list. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

nickvergessen · 2020-12-08T12:01:25Z

Simply dropped "Guard against using undefined 'participants' key" which did exactly the same as 6206ca7 just a few lines further up

danxuliu added 2. developing enhancement feature: api 🛠️ OCS API for conversations, chats and participants labels Nov 13, 2020

danxuliu added this to the 💚 Next Major (21) milestone Nov 13, 2020

danxuliu mentioned this pull request Nov 13, 2020

Conversation description - front-end #4546

Merged

danxuliu force-pushed the add-backend-for-conversation-descriptions branch from b344c27 to 44c59e9 Compare November 26, 2020 10:11