Skip to content

Support slash commands in issues / MR description & comments

Rémy Coutable requested to merge 4273-slash-commands into master

What does this MR do?

It brings support for slash commands (I think "slash commands" is a good name for that, inspired by https://api.slack.com/slash-commands).

The UI does not update accordingly when leaving a comment with commands in it. This will require further frontend work but I think we can already ship this first iteration.

For the moment actions are interpreted in a hash of changes.

  • For new issuable, this hash of changes is then merged to the params used to create issuable in their respective creation service.
  • For new notes, the hash is passed to the noteable update service so that it updates the noteable first, and then tries to save the note. The good thing about this workflow is that since commands are extracted from the original note, no note is created in the case the note consisted of commands only.

For instance if I leave the following note:

/milestone %9.10
/label ~"Pick into Stable"

These commands will actually trigger an update of the noteable, and no actual note will be created (of course the issuable update will create new system notes).

I think this is the right approach as a first step, then we can build on this, by returning "special value" in the hash to trigger advanced actions. For instance, we could add /todo & /done actions that would translate in { todo: :mark } or { todo: :done } that could easily trigger a todo_service.mark_todo(note.noteable, current_user) / todo_service.mark_todos_as_done(note.noteable, current_user).

There's no need to make our models bigger with a new concern (as this was originally implemented) since:

  • all the creation/update logic is handled in services
  • advanced workflow like this one shouldn't be part of the model

Are there points in the code the reviewer needs to double check?

One important thing I wanted was to be sure that we don't bypass the services to create/update issuable so I don't think this MR can create security issues.

Why was this MR needed?

Because this is awesome and will allow many people to automate more. For instance, as a release manager, when I'm preparing a patch release, the workflow is the following:

  1. pick a commit into the stable branch
  2. leave a comment to say that it's been picked
  3. remove the ~"Pick into Stable" label

Now, I will be able to have only two steps:

  1. pick a commit into the stable branch
  2. leave a comment to say that it's been picked with the /remove_label ~"Pick into Stable" label command, and voila!

Also, since reply by email is now enabled on .com, you will be able to actually update an issue or MR from your email client!

What are the relevant issue numbers?

Closes #4273 (closed).

Screenshots

Slash commands when creating an issue

slash-commands-2

Slash commands in a note

slash-commands-1

Does this MR meet the acceptance criteria?

Todo:

  • Initial slash commands
  • /todo: Mark issuable as todo
  • /done: Mark todo as done for the issuable
  • /subscribe: Subscribe to the issuable without leaving a message
  • /unsubscribe: Unsubscribe from the issuable (useful for release managers that leave many messages but don't want to be subscribed!)
  • /due_date: Set a due date for the issue (not for the MR since it's not supported)
  • /clear_due_date: Set a due date for the issue (not for the MR since it's not supported)
  • We should also bring autocompletion for slash commands themselves once you type /!
  • Improve autocompletion by adding a description of the command
  • Check that the feature works well with the reply-by-email feature
  • The right thing happens when the reply contained nothing but commands (no comment is created, no error email is sent out)

To be done in EE:

  • /weight: Set a weight for the issuable => gitlab-org/gitlab-ee#852

To be done in a new MR (mote complex):

  • /merge: Merge the MR => gitlab-org/gitlab-ce#20741

Feature improvement:

  • Ensure that slash commands are not detected inside code blocks (see !3954 (merged))
  • Support /title <New title>
  • Improve CHANGELOG
  • Add a /cc command which would be a no-op but would appear in the auto-complete to not confuse people
  • Don't return actions that would have no effect (no /remove_milestone if the noteable has no milestone, or no /milestone if the project has no milestone, etc.)
  • Use a contextual description for commands: "Close this issue" or "Close this merge request" based on context?

Backend code improvement:

  • Improve the attributes.delete(:remove_label_ids) mess in IssuableBaseService
  • Improve the wording of the “You entered only commands” message
  • Extract #execute_slash_commands! to a new Notes::SlashCommandsService class
  • Extract the body of the command_params.reduce block to its own method
  • Use :subscription_event instead of manually trigger subscription!
  • Create a new :todo_event param that would work similarly to :state_event / :subscription_event?
  • Let the commands have their own block to find out if the noteable supports that command
  • Don’t noteable_updated_at != noteable.updated_at, return true if there’s any command
  • Use TodoFinder instead of current_user.todos.pending.find_by(target: note.noteable)
  • Create a new Notes::SlashCommandsService.UPDATE_SERVICES, as a hash from class name to service.
  • Get rid of a useless begin/end
  • Improve the command descriptions
  • Document that each command should be on a separate line
  • Let the user know that we support ChronicDuration style?
  • Make Extractor a class instead of a Struct
  • Ensure we don’t detect commands with no space between the command and the first argument
  • /remove_milestone -> /clear_milestone and set /remove_milestone as an alias
  • Ensure unauthorized user cannot perform unauthorized actions. Could be done in IssuableBaseService#filter_params.
  • If the user doesn't have permissions to perform an action, we should not even show that action in autocomplete

Frontend code:

  • Remove useless auto complete template

  • Ensure we add the reference prefix only for commands that need one

  • Insert a new line before inserting a slash command

  • Use !== instead of != in JS (don't use it for most of presence checks, i.e. use!= null and not !== null)

  • Do not use <b>, font-weight is more appropriate No need for <b> after all

  • Use _.template

  • Use bind instead of (function(_this){})(this) We don't even need bind

  • CHANGELOG entry added

  • Documentation created/updated

  • Tests

    • Added for this feature/bug
    • All builds are passing

  • Conform by the style guides

  • Branch has no merge conflicts with master (if you do - rebase it please)

  • Squashed related commits together

Merge request reports