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

Link Expanding generates extra unexpected message to be received by Bolt app #463

Closed
4 of 9 tasks
joffreyvillard opened this issue Apr 4, 2020 · 6 comments
Closed
4 of 9 tasks
Labels
auto-triage-stale bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented discussion M-T: An issue where more input is needed to reach a decision

Comments

@joffreyvillard
Copy link

joffreyvillard commented Apr 4, 2020

First of all, thanks for the great work with Bolt!
Details of the issue below.

Description

I have an app that listens to all messages it subscribed to (app.message(messageListener)). If it sends a message that includes a link (e.g. using the say function), it receives an extra message due to the Slackbot-LinkExpanding that updated the original message.

From my point of view, this is an unexpected behavior: this event with type message and subtype message_changed should be filtered out (as the echoed messages seem to be already), or be handled with app.event instead of app.message (but what would I do with the information that my own message has been updated by Slack itself due to Link Expanding?)

Of course, I can filter these events myself, but I feel this should be built-in into Bolt, as a very simple example (see below) does not behave as expected (from user and developer point of view).

The same thing happens if the user sends a message that includes a link.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

package version: 1.8.0

node version: 12.15.0

OS version(s): Ubuntu 18.04.4

Steps to reproduce:

  1. setup an app that subscribes to message.im
  2. listen to any message and reply with a message that includes a link that will be expanded:
app.message(async (args: SlackEventMiddlewareArgs<'message'>) => {
  const { say } = args
  say('Have you checked that <https://slack.com/|great tool>?')
})

Expected result:

As a user talking to that bot, I expect to receive a single answer for each message.

As a developer of an almost "hello world" example, I expect it to work in the most simple and obvious way.

Actual result:

The user receives the message twice. The second one is triggered by the update event when the link is expanded by Slackbot-LinkExpanding.

Attachments:

screenshot_double_message

@joffreyvillard
Copy link
Author

joffreyvillard commented Apr 4, 2020

EDIT: note below is not specific to Bolt, but relates to Slack message event not much consistent between all subtypes (https://api.slack.com/events/message), which is not the main point of this issue.


Original message:

I think this can definitely be considered as a bug, as the received args.message object (as seen by the listener) looks as follows, which is not consistent with the "standard" message, and hence cannot be considered of the same type.

args.message of a Slackbot-LinkExpanding "operation"

{
  type: 'message',
  subtype: 'message_changed',
  hidden: true,
  message: {
    bot_id: 'xxx',
    type: 'message',
    text: 'xxx'
    user: 'xxx',
    team: 'xxx',
    bot_profile: {
      id: 'xxx',
      deleted: false,
      name: 'xxx',
      updated: 1585226981,
      app_id: 'xxx',
      icons: [Object],
      team_id: 'xxx'
    },
    edited: { user: 'xxx', ts: '1586016596.000000' },
    attachments: [ [Object] ],
    ts: '1586016596.005200'
  },
  channel: 'xxx',
  previous_message: {...},
  event_ts: '1586016596.005300',
  ts: '1586016596.005300',
  channel_type: 'im'
}

Standard args.message:

{
  client_msg_id: 'xxx',
  type: 'message',
  text: 'hi',
  user: 'xxx',
  ts: '1586017553.007000',
  team: 'xxx',
  blocks: [ { type: 'rich_text', block_id: 'xxx', elements: [Array] } ],
  channel: 'xxx',
  event_ts: '1586017553.007000',
  channel_type: 'im'
}

@joffreyvillard
Copy link
Author

In fact, I guess the best way to handle any text message would be rather to use a pattern so that only "message event" with a text field would be passed to the listener:

app.message(/.+/, messageListener)

In the end, some additional notes in the Bolt documentation about "message events" could be enough, and the fact that adding a pattern as above filters out any "message event" that does not have a text field (in case you do not want to worry about all the subtypes).

@seratch
Copy link
Member

seratch commented Apr 6, 2020

Thank you very much for writing in. I will share this feedback with the server-side teams concerned. Regarding the discussion on the Bolt side, I will create a discussion labeled issue for it.

A possible workaround at this point is to test if there is a subtype in a message-typed payloads in your listeners. If a payload doesn't have its subtype, it means the message event is about a new message.

For reference: a similar issue related to TypeScript type definitions - #311

@joffreyvillard
Copy link
Author

Thanks for your reply and the follow-up issue!

I've implemented both workarounds for now (ignoring all message events that show a subtype and filtering only the ones with a text field using app.message(/.+/, messageListener))

In the short term, I think this way of handling any new message (app.message(/.+/, messageListener) ) could be added somewhere in the below paragraph of the Bolt documentation so that developers are not misleaded with the optional nature of the pattern parameter):

image

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented discussion M-T: An issue where more input is needed to reach a decision labels Apr 18, 2020
@github-actions
Copy link

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out.

@github-actions
Copy link

As this issue has been inactive for more than one month, we will be closing it. Thank you to all the participants! If you would like to raise a related issue, please create a new issue which includes your specific details and references this issue number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-triage-stale bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented discussion M-T: An issue where more input is needed to reach a decision
Projects
None yet
Development

No branches or pull requests

2 participants