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

[NEW] Add message action to copy message to input as reply #12626

Merged

Conversation

mrsimpson
Copy link
Collaborator

@mrsimpson mrsimpson commented Nov 14, 2018

Motivation

As a developer of a bot, I want to suggest a response to the user. Instead of just making the user to send a fully predefined message, the user shall adjust the message (think of it as a template for the reply).
This reduces typing overhead, reduces errors during the conversation and makes sure the user follows some expected structure

Implementation

There already was a message-action to send a message by clicking a button living in an attachment.
This enhancement allows to create an action button (either from an integration, an API or a bot) which copies a predefined message as template into the message input (browser only!)

Consumption

The button is part of an attachment's actions and can be created anywhere there is a message being created.
Sample for an outgoing integration

class Script{
  
    prepare_outgoing_request({ request }) {

      return {
        message: {
          text: "Would you be so kind to let me know what you did today?",
          attachments[{
            actions: [
              {
                type: "button",
                msg_in_chat_window: true,
                msg_processing_type: "respondWithMessage",
                text: "Respond easily",
                msg: "**What I worked on today**\n- \n\n**What I am planning to do tomorrow**\n- "
              }
            ]
            }]
        }
      }
    }
}

How it looks like

Sample: A Daily-Bot is gathering input from users:
2018-11-15 08 08 01

Open issues

Mobile adaption - @rafaelks since this is a client side action, the mobile apps would have to implement this copy-to-input as well.

@rafaelks
Copy link
Contributor

@mrsimpson That looks pretty nice! Just curious, did you follow some pattern from other kind of bots in order to implement this? We definitely want this on the mobile! Any way to customize the button colors or something?

@mrsimpson
Copy link
Collaborator Author

@rafaelks the general "architecture" of the message actions is not invented by myself.
The package that brought the ability to provide actionable buttons in attachments was #11473 .
To be honest, I don't like it much, we had something on our fork which I preferred, but this PR made it into the core.
I didn't find any API of e. g. a bot framework, which matches the structure, only documentation in a wiki of a fork . @ear-dev or @bizzbyster seem to be driving this and may be able to answer. They obviously also have some rendering of the actions in their Android-App-Fork.

This PR only adds the ability to provide a reply-with-message-capability - and tries to be as compatible to the existing stuff merged with #11473 as possible.

@rafaelks
Copy link
Contributor

@mrsimpson Got it, that makes perfect sense, thanks for explaining that. I can see @bizzbyster have a good documentation on their Wiki about this new properties and how they work, what do you guys thinking about adding the parts that Rocket.Chat core supports in our own docs already, so we keep it updated and will be easier to other clients to support the same functionalities? I know that our Android app already supports some of the actions, was a contribution from @bizzbyster team.

@bizzbyster
Copy link

@mrsimpson

We need to move the documentation that is on WideChat into Rocket.chat. For now, you may want to look at:

  1. our test plan document https://github.com/WideChat/Rocket.Chat.Android/wiki/Phase-1--Rich-Messaging-Acceptance-Test.
  2. and also we have automated acceptance tests that we should add this to https://github.com/WideChat/bot-acceptance-tests/blob/master/richmessagebot-automated-tests/tests.js. We have tested against bots running on multiple bot platforms -- see https://github.com/WideChat/*-richmessage-testbot.

Hope that helps...

@timkinnane @Sing-Li Any thoughts to move rich message documentation and acceptance tests from WideChat to main repo?

Lastly, the UI above for this feature is kind of rough in my opinion. Do we want to include the asterisk character in the user interface as per this?
image Can we perhaps get better UI support so that we can supply the same functionality but without resorting to things like asterisks as separators? @rafaelks I'd think especially the mobile clients would have other standard UI components that would work well for this. Perhaps @thiagosanchz has an opinion?

@mrsimpson
Copy link
Collaborator Author

@bizzbyster the sample with the asterisks is noch meant to be structured.
It is plain more markdown which - in this sample - creates a template for a formatted message.
The template‘s value can be defined by the one providing the action

Sent with GitHawk

@wreiske
Copy link
Contributor

wreiske commented Nov 20, 2018

@bizzbyster made a comment about better UI support without resorting to things like asterisks. Maybe a look at https://api.slack.com/dialogs will give some ideas?
image

@mrsimpson
Copy link
Collaborator Author

@wreiske I am quite sure I understood @bizzbyster 's comment about the better UI-support - and I agree: There should be better support for more controls like input- or comboboxes. There also are other features which need to be provided in order to make message-based rich interactions possible.

But this is not the intention of this PR. The feature is a bit different: The purpose is to provide a button which creates a template for a message. Of course, this could be seen as an alternative implementation of a form, but it's so limited it's in fact a different feature. The sample I chose with the asterisks was only to illustrate there's an option to supply markdown in the template.

So: Yes, there should be other issues / PRs to provide real rich messaging.

@timkinnane
Copy link
Contributor

Hi @mrsimpson - I see the usefulness of this interaction, but I don't think it should be merged the way it's currently implemented.

The schema is already quite busy and adding attributes that are modifiers of other attributes makes it very complex.

i.e. Currently msg_processing_type changes the behaviour of msg_in_chat_window, which also means msg_in_chat_window is required to be true for msg_processing_type to work.

Neither attribute is very semantic, so it's just a high cognitive load for developers, and I think will make it harder to write simple docs, tests and encourage adoption.

I actually think the msg_in_chat_window isn't a very good attribute as is. "Chat window" sounds a bit obscure to me. Here's the docs from @bizzbyster's team...

msg_in_chat_window: (Required with msg) Indicates where the msg is to be sent. If true then it will go into the chat window. If false then it will be sent back to the bot but not appear in the chat window. (Boolean)

I think it would be better to rewrite the naming and usage of that attribute instead of adding another one. So instead of a boolean, it has three modes:

  1. Submit the message but don't display it in message stream.
  2. Submit the message and display it in the message stream.
  3. Don't submit the message, put it in the dialog for editing.

Then in future, there could be other options, without needing another refactor.

I would suggest something like msg_submit_mode ... or something? With hide, show (default), edit options.

@bizzbyster What do you think? Is your work on the schema too far along for that?

Keeping in mind, most of the attributes were made to match the Slack schema, but I think msg_in_chat_window was just made up for a hypothetical use case in Rocket.Chat and for that a boolean is too limiting and exactly why new uses require new attributes, which will ruin the simplicity of the schema over time. There's also the implementation problems with it already noted in #11994.

@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Nov 24, 2018

@timkinnane I couldn't agree more! Thanks for the feedback, I had the same doubts when implementing that piece of code. It's hacked.
But this is what a PR is for: To find the best solution ;)

I believe the action that is taken when clicking on a button is modeled very ... simply at the moment. This also makes it very hard to extend.

My intention was to be as compatible as possible since I don't know which parts of the design were taken (by @bizzbyster 's team) from another spec with which the actions should be compatible and which one is "invented here".

Therefore, I also added default behavior in order to be compatible with the current spec. Given the existing design, I didn't see another non-disruptive way (so that potential consumers of the API are not breaking).

IF we are free to change here, I'd do quite some changes. Before I go into the details, let me elaborate the model:

I understood that there are basically two levels of types:

  1. Where's the action processed
enum processingSide {
    client,
    server,
}
  1. Which is the directive to process it
enum clientSideProcessingDirective {
   postMessage, // creates a fixed message issued by the executer
   messageProposalWithTemplate, // fills in the message input with a given template,
   httpRequest, // triggers a http-request from within the client (browser, native app)
   openForm, // opens a dialog with a rich UI, as illustrated by @wreiske 's slack sample
}

enum serverSideProcessingDirective {
   callMethod, // triggers the execution of a defined method in the context of the executor
   httpRequest, // triggers a server side http-request
}

In order to keep this flexible, I'd propose a command-pattern, so that the consumer creates an object describing the action, e. g.

{
  processingSide: 'client',
  command: {
    directive: 'postMessage',
    msg: 'booo!'
  }
}

Then, handlers for each command could be provided on each platform, effectively executing the directive. This was extensible, led to maintainable code and could accommodate all the existing requirements.

However, this was extremely incompatible with the existing codebase.
@bizzbyster @rafaelks @timkinnane thoughts?

@mrsimpson
Copy link
Collaborator Author

@RocketChat/core Any ideas / concepts about how to properly provide rich message attachments? I'm more than willing to contribute also a more powerful solution - but only once I can be sure, that this is going to be supported by the core (also on mobile)

@ggazzo
Copy link
Member

ggazzo commented Feb 8, 2019

@rodrigok could you check the tests?

@sampaiodiego
Copy link
Member

@RocketChat/core Any ideas / concepts about how to properly provide rich message attachments? I'm more than willing to contribute also a more powerful solution - but only once I can be sure, that this is going to be supported by the core (also on mobile)

yes! I have recently discussed with @engelgabriel about this subject.. he thinks we should have an API like slack's blocks .. so we'll definitely give rich messages a look soon.

in the meanwhile I think we could have this feature merged in as we'll have a new schema for rich messages in the feature.

packages/rocketchat-ui/client/views/app/room.js Outdated Show resolved Hide resolved
@mrsimpson mrsimpson force-pushed the core/message-action-reply-suggestion branch from 75b548d to d8d8a28 Compare March 25, 2019 20:55
@sampaiodiego sampaiodiego added this to the 1.0.0 milestone Mar 26, 2019
d-gubert
d-gubert previously approved these changes Mar 27, 2019
Copy link
Member

@d-gubert d-gubert left a comment

Choose a reason for hiding this comment

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

LGTM

@d-gubert d-gubert dismissed sampaiodiego’s stale review March 27, 2019 18:59

Author applied requested changes

Copy link
Member

@d-gubert d-gubert left a comment

Choose a reason for hiding this comment

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

LGTM

@d-gubert d-gubert changed the title Add message action to copy message to input as reply [NEW] Add message action to copy message to input as reply Mar 27, 2019
@d-gubert d-gubert merged commit 7c2d680 into RocketChat:develop Mar 27, 2019
@mrsimpson
Copy link
Collaborator Author

@d-gubert i believe I introduced the timeout since the input was not always available to the jQuery-selector I hacked previously. The timeout was only there to be executed async.
However, this may be solved by the new mechanism as suggested by Diego.
Just wanted to explain

Sent with GitHawk

@d-gubert
Copy link
Member

Cool :)

We've tested it with some different input values and it worked without the timeout, so we've decided to remove it.

However, this may be solved by the new mechanism as suggested by Diego.

Indeed this might have been the case :)

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.

None yet

9 participants