↩️ Reply to messages #2000

nickvergessen · 2019-07-15T11:23:23Z

API changes
Return the original message as parent, so it can be rendered easily
Add UI to reply to a message
Adjust UI to render the parent message

PR might be delayed if the UI is not ready for 17.

For the UI maybe @jenniferpiperek and @jancborchardt have some comments? :)
From my PoV:

There should be a reply button (or hidden in a … menu) on each message (future stuff may come too, like delete, etc), on mobile apps swiping to left (telegram) or right (whatsapp, viber) should be a shortcut to additional long press
When starting to reply, a hint or the message itself is displayed on the new comment form, so you see what you are replying to (check the chat apps from above)
The parent message is put as a one-liner in the top of the original message (trimmed), in a perfect world (maybe in Vue.JS) clicking the parent would jump to that message (like in any chat app)

To reply on this PR apply the following patch (219 being the comment id you want to reply to)

diff --git a/js/views/chatview.js b/js/views/chatview.js
index 16788817..0c0916d3 100644
--- a/js/views/chatview.js
+++ b/js/views/chatview.js
@@ -842,7 +842,8 @@
                        message = this._commentBodyHTML2Plain($commentField);
                        var data = {
                                token: this.collection.token,
-                               message: message
+                               message: message,
+                               replyTo: 219
                        };
 
                        if (!OC.getCurrentUser().uid) {

Fix #1136

nickvergessen · 2019-07-24T09:17:23Z

Nested integration test fails due to nextcloud/server#16523

nickvergessen · 2019-07-24T10:17:20Z

Rebased to fix integration tests since the server PR is merged

tests/integration/features/bootstrap/FeatureContext.php

danxuliu · 2019-07-25T09:27:35Z

tests/integration/features/bootstrap/FeatureContext.php

 				'room' => self::$tokenToIdentifier[$message['token']],
-				'actorType' => (string) $message['actorType'],


I know that back then I added the casts for some good reason, although now it seems to work without them... Maybe when they were added we did not use yet the type hinting and now it ensures that they are strings? I do not know. Anyway, it seems to work, so 👍

lib/Chat/Notifier.php

lib/Chat/ChatManager.php

lib/Chat/Notifier.php

nickvergessen · 2019-07-25T12:19:32Z

Rebased after the merge of #1214 due to conflicts in ChatManager.

lib/Controller/ChatController.php

danxuliu

I have made some cleanup of the integration tests and added some more. When doing that I found that replies to commands failed because

TypeError: Argument 2 passed to OCA\Spreed\Chat\ChatManager::getParentComment() must be of the type integer, string given, called in /nextcloud/apps/spreed/lib/Controller/ChatController.php on line 295 at /nextcloud/apps/spreed/lib/Chat/ChatManager.php#199

Turns out that getParentId() returns a string according to the documentation; I have not checked why it was a string when commands where involved but an int otherwise, I just changed the code to use the casted int version instead of getting the value again from getParentId(), although maybe it would have been better to change the type of the parameters in ChatManager::getParentComment() (note that inside getParentComment() the id is in fact casted to a string).

Anyway, the integration test when replying to a private command fails, because I assumed that in that case the reply would not be visible to other participants. Right now it is visible, so either the code is fixed or, if it is the expected behaviour, the test is fixed.

nickvergessen · 2019-07-25T19:13:06Z

It's unrelated to commands but actually is because of the change you did which I commented in 9414448#discussion_r307385860

nickvergessen · 2019-07-25T20:10:40Z

Fixed the error.

The problem with replying to commands is: they can currently only be identified by looking at the actor type. Rendering only tells if you can see the message. So I would suggest to simply not allow replying to commands and system messages (for now).

danxuliu · 2019-07-26T07:40:16Z

So I would suggest to simply not allow replying to commands and system messages (for now).

Perfect 👍

I removed again the empty lines added in the integration tests, as the idea was to group the tests by similar features (replies to messages, replies to replies, replies from users not in the room) using the empty lines to separate the groups; if we have the same empty lines between all tests then there are no groups :-)

danxuliu

The unit tests need to be fixed up too in 8d3420d

danxuliu · 2019-07-26T08:22:22Z

lib/Chat/Parser/Command.php

@@ -48,6 +48,7 @@ public function parseMessage(Message $message): void {
 			return;
 		}

+		$message->setMessageType('command');


The message type is set based on the verb and the verb for commands is set to command when they are executed. Therefore, could that be removed or is it needed in some case?

right, I added this without checking.

I guess we need to expose it to the API as well, so the mobile apps can check if replying is allowed as well.

I guess we need to expose it to the API as well, so the mobile apps can check if replying is allowed as well.

Makes sense 👍

danxuliu · 2019-07-26T08:24:50Z

lib/Controller/ChatController.php

+
+			$parentMessage = $this->messageParser->createMessage($this->room, $this->participant, $parent, $this->l);
+			$this->messageParser->parseMessage($parentMessage);
+			if ($parentMessage->getMessageType() === 'system' || $parentMessage->getMessageType() === 'command') {


I guess that we are not adding any other special type of message anytime soon, but maybe explicitly limit replies only to pure messages for now by checking if the type is not comment instead of checking if it is system or command?

¯\_(ツ)_/¯

the question is if a follow up type would be more likely to be repliable or not. If it is something like stickers or so, it could be repliable. If it is reall bots instead of commands, maybe not. but who knows.

nickvergessen · 2019-07-26T09:26:20Z

Fixed the remaining unit test and added some more.
Then did all the fixup, ready for final review

danxuliu

I have amended again the integration tests to use nested message names (like Message 1-1) as that makes easier to read the results.

Besides that I realized that shared file messages are system messages marked as regular messages, so as it is a rather special case I added an explicit integration test for them. And turns out that something is broken when replying to shared file messages :-P

It seems that after replying the original shared file message is replaced with the reply or something like that, as can be seen with this specially tailored (and thus not included in the tests) integration test:

  Scenario: trigger bug when replying to shared file message
    Given user "participant1" creates room "group room"
      | roomType | 2 |
      | invite   | attendees1 |
    And user "participant1" shares "welcome.txt" with room "group room"
    And user "participant1" sends message "Message 2" to room "group room" with 201
    And user "participant1" sees the following messages in room "group room" with 200
      | room       | actorType | actorId      | actorDisplayName         | message   | messageParameters | parentMessage |
      | group room | users     | participant1 | participant1-displayname | Message 2 | []                |               |
      | group room | users     | participant1 | participant1-displayname | {file}    | "IGNORE"          |               |
    And user "participant2" sees the following messages in room "group room" with 200
      | room       | actorType | actorId      | actorDisplayName         | message   | messageParameters | parentMessage |
      | group room | users     | participant1 | participant1-displayname | Message 2 | []                |               |
      | group room | users     | participant1 | participant1-displayname | {file}    | "IGNORE"          |               |
    When user "participant1" sends reply "Message X-1" on message "{file}" to room "group room" with 201
    Then user "participant1" sees the following messages in room "group room" with 200
      | room       | actorType | actorId      | actorDisplayName         | message     | messageParameters | parentMessage |
      | group room | users     | participant1 | participant1-displayname | Message X-1 | []                | file_shared   |
      | group room | users     | participant1 | participant1-displayname | Message 2   | []                |               |
    And user "participant2" sees the following messages in room "group room" with 200
      | room       | actorType | actorId      | actorDisplayName         | message     | messageParameters | parentMessage |
      | group room | users     | participant1 | participant1-displayname | Message X-1 | []                | file_shared   |
      | group room | users     | participant1 | participant1-displayname | Message 2   | []                |               |
      | group room | users     | participant1 | participant1-displayname | {file}      | "IGNORE"          |               |

Then user "participant1" sees the following messages in room "group room" with 200 passes, but the checked messages are wrong, because the shared file message is not included; And user "participant2" sees the following messages in room "group room" with 200 fails, but the checked messages are right, because the shared file message is included.

danxuliu · 2019-07-29T08:35:00Z

lib/Model/Message.php

@@ -52,6 +52,9 @@ class Message {
 	/** @var string */
 	protected $message = '';

+	/** @var string */
+	protected $info = '';


Given how it is used and why it was introduced, maybe it would be better if called $rawMessage or $unparsedMessage or something like that instead?

yeah, for the current usecases that would work. I though about using "extra" or "info" to be prepared for future stuff, like stickers and others (depending on how we do them).

picked raw now

danxuliu

Looks good 👍

…ints Signed-off-by: Joas Schilling <[email protected]>

Signed-off-by: Joas Schilling <[email protected]>

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

… database Signed-off-by: Joas Schilling <[email protected]>

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen · 2019-07-30T07:37:28Z

Fixups done and final rebase

nickvergessen added 2. developing design enhancement feature: chat 💬 Chat and system messages labels Jul 15, 2019

nickvergessen added this to the 💚 Next Major milestone Jul 15, 2019

nickvergessen mentioned this pull request Jul 24, 2019

Nested recursion breaking max nested level for parent comment calculation nextcloud/server#16523

Merged

nickvergessen force-pushed the feature/1136/reply-to-messages branch from 5ed2d7f to 0d1ac7a Compare July 24, 2019 09:16

nickvergessen force-pushed the feature/1136/reply-to-messages branch from 0d1ac7a to fc0137e Compare July 24, 2019 10:16

nickvergessen added 3. to review and removed 2. developing labels Jul 24, 2019

nickvergessen requested review from danxuliu and Ivansss July 24, 2019 10:17

danxuliu reviewed Jul 25, 2019

View reviewed changes

nickvergessen force-pushed the feature/1136/reply-to-messages branch from 4070763 to 43b629c Compare July 25, 2019 12:19

nickvergessen commented Jul 25, 2019

View reviewed changes

lib/Controller/ChatController.php Outdated Show resolved Hide resolved

danxuliu requested changes Jul 25, 2019

View reviewed changes

nickvergessen requested a review from danxuliu July 25, 2019 20:36

danxuliu requested changes Jul 26, 2019

View reviewed changes

danxuliu reviewed Jul 26, 2019

View reviewed changes

nickvergessen force-pushed the feature/1136/reply-to-messages branch from bf96d8c to 4310ef9 Compare July 26, 2019 09:25

nickvergessen requested a review from danxuliu July 26, 2019 09:25

danxuliu force-pushed the feature/1136/reply-to-messages branch from 4310ef9 to bc2bf8c Compare July 26, 2019 11:35

danxuliu requested changes Jul 26, 2019

View reviewed changes

danxuliu reviewed Jul 29, 2019

View reviewed changes

danxuliu approved these changes Jul 30, 2019

View reviewed changes

nickvergessen and others added 9 commits July 30, 2019 09:33

Allow to reply to messages and return the parent on the message endpo…

a1b17d2

…ints Signed-off-by: Joas Schilling <[email protected]>

Add a mention-alike notification for replies

e3e3e8e

Signed-off-by: Joas Schilling <[email protected]>

Add integration tests for replies

c145140

Signed-off-by: Joas Schilling <[email protected]>

Add a little hack for replying

573f07e

Signed-off-by: Joas Schilling <[email protected]>

Prevent replying to commands and system messages

b14e63d

Signed-off-by: Joas Schilling <[email protected]>

Add integration test for replies to shared file messages

1005aaa

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

Make sure we don't change the comments object as that will update the…

25aae9f

… database Signed-off-by: Joas Schilling <[email protected]>

Expose the message type as well

3a0503b

Signed-off-by: Joas Schilling <[email protected]>

Add docs for replying

597d8e5

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen force-pushed the feature/1136/reply-to-messages branch from 33987f8 to 597d8e5 Compare July 30, 2019 07:37

nickvergessen added 4. to release and removed 3. to review labels Jul 30, 2019

nickvergessen merged commit efe879c into master Jul 30, 2019

delete-merged-branch bot deleted the feature/1136/reply-to-messages branch July 30, 2019 12:29

szaimen mentioned this pull request Nov 11, 2019

Thread/"Respond to single post" option #1469

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

↩️ Reply to messages #2000

↩️ Reply to messages #2000

nickvergessen commented Jul 15, 2019 •

edited

Loading

nickvergessen commented Jul 24, 2019

nickvergessen commented Jul 24, 2019

danxuliu Jul 25, 2019

nickvergessen commented Jul 25, 2019

danxuliu left a comment

nickvergessen commented Jul 25, 2019

nickvergessen commented Jul 25, 2019

danxuliu commented Jul 26, 2019

danxuliu left a comment

danxuliu Jul 26, 2019

nickvergessen Jul 26, 2019

danxuliu Jul 26, 2019

danxuliu Jul 26, 2019

nickvergessen Jul 26, 2019

nickvergessen Jul 26, 2019

nickvergessen commented Jul 26, 2019

danxuliu left a comment

danxuliu Jul 29, 2019

nickvergessen Jul 29, 2019

nickvergessen Jul 29, 2019

danxuliu left a comment

nickvergessen commented Jul 30, 2019

		'room' => self::$tokenToIdentifier[$message['token']],
		'actorType' => (string) $message['actorType'],

↩️ Reply to messages #2000

↩️ Reply to messages #2000

Conversation

nickvergessen commented Jul 15, 2019 • edited Loading

nickvergessen commented Jul 24, 2019

nickvergessen commented Jul 24, 2019

Choose a reason for hiding this comment

nickvergessen commented Jul 25, 2019

danxuliu left a comment

Choose a reason for hiding this comment

nickvergessen commented Jul 25, 2019

nickvergessen commented Jul 25, 2019

danxuliu commented Jul 26, 2019

danxuliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Jul 26, 2019

danxuliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danxuliu left a comment

Choose a reason for hiding this comment

nickvergessen commented Jul 30, 2019

nickvergessen commented Jul 15, 2019 •

edited

Loading