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

interactive-messages: message_action payload routed to viewSubmission handler #929

Closed
5 of 15 tasks
davidalpert opened this issue Dec 10, 2019 · 5 comments
Closed
5 of 15 tasks
Labels
enhancement M-T: A feature request for new functionality pkg:interactive-messages (deprecated) applies to `@slack/interactive-messages`

Comments

@davidalpert
Copy link

Description

While first working with the @slack/interactive-messages library and the new Slack App patterns I misunderstood the role of the callback_id. Now I understand callback_id as a way to uniquely identify which workflow step is sending a given callback to easily route the callbacks to the correct handlers. Originally, however, I thought that the callback_id was used to tie together several steps in a workflow so was using the same callback_id value for both message actions and view submissions.

Given that there are registration handlers for different types of callback payloads I was surprised when a handler I had registered through the viewSubmission(...) method started intercepting message_action payloads.

e.g.

  • the action(...) registration method implies a "type": "message_action" constraint;
  • the viewSubmission(...) registration method implies a "type": "view_submission" constraint;

but these implied constraints don't seem to get applied.

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.

Packages:

Select all that apply:

  • @slack/web-api
  • @slack/events-api
  • @slack/interactive-messages
  • @slack/rtm-api
  • @slack/webhooks
  • I don't know

Reproducible in:

package version:

  • @slack/interactive-messages@1.4.0

node version:

  • v10.16.0

OS version(s):

Steps to reproduce:

  1. create a slack app and configure a message interaction to send an order_pizza callback id

  2. create an api using @slack/interactive-messages@1.4.0 to handle callbacks from this slack app

  3. register a message_action handler using the action(...)

    slackInteractions.action({ type: 'message_action': callback_id: 'order_pizza' }, openPizzaOptionsModal);

    return a view payload to open a modal view with the view callback_id set to order_pizza (because it's part of the same workflow)

  4. initiate the action in the slack app

  5. verify that the "Pizza Options" modal dialog opens

  6. add a new handler before the action handler (because sometimes scanning folders for files reads and registers dialogHandlers.js before messageHandlers.js)

    slackInteractions.action('order_pizza', pizzaOrderSubmitted);
  7. initiate the action in the slack app

Expected result:

  • the openPizzaOptionsModal handler fires, opening the "Pizza Options" dialog
  • on submission of the "Pizza Options" dialog the pizzaOrderSubmitted handler fires

Actual result:

  • the pizzaOrderSubmitted handler fires on the initial message_action, throws an error because it expects the payload to match the view_submission schema
  • no dialog gets opened
  • the slack user gets no feedback about why clicking on the message action had no effect.
@stevengill stevengill added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:interactive-messages (deprecated) applies to `@slack/interactive-messages` and removed bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented labels Dec 10, 2019
@stevengill
Copy link
Member

Hey @davidalpert,

Interesting observation. I'm not sure this is really a bug. The documentation states that callback IDs should be unique. I do see how you made the assumptions you did though.

I also see your suggestion of using the registration method type to determine the correct handler, but that would also require us having to analyze the handlers to determine which one is correct. IMO it adds too much overhead.

Bolt did recently add type as a constraint option. We could looking at doing something similar here.

@davidalpert
Copy link
Author

thank you for considering it. now that I've learned a bit more about both the slack app callbacks and the @slack/interactive-messages package I don't think I will stumble over this anymore. I was simply surprised that a viewSubmission-registered handler would match a callback of type other than view_submission but I see your point.

I am comfortable closing this if you feel that is expected behavior.

@stevengill
Copy link
Member

Sounds good @davidalpert. I'll mark it as closed but feel free to reopen if you feel more discussion is needed for this.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 11, 2019

Hey folks, just wanted to add my 2c 👋

Slack doesn’t enforce unique callback_ids across different interactions, so its not 100% clear that @davidalpert‘s expected behavior is incorrect, nor that this package should require them to be unique.

Instead I think this issue highlights a shortcoming of our constraints matching system’s idea of “best” when finding the right handler. The way it works is that we look at each handler (in the order they are added) and evaluate whether there’s any reason to eliminate it from matching a certain payload. If there is, move onto the next handler. If there isn’t, that means this handler satisfies the constraints and we can invoke it. Notice how we’re not gathering all listeners which weren’t eliminated to do a second pass to determine which listener matches the most. That we be more expensive.

Instead, we provide you this guidance so we don’t have to make the performance tradeoff: register your most specific handlers first and your more generic handlers later. In the case you described above, you happen to know the pizzaOrderSubmitted handler is always happening as a modal submission, so let’s be more specific and avoid the ordering challenge all together.

slackInteractions.viewSubmission('order_pizza', pizzaOrderSubmitted);

Now, I’m going to read between the lines of your original message and assume you didn’t do this because you’re reading from a directory in while each file exports a handler and constraints, and you’re looking to use the most generic method possible to register those handlers.

If that’s the case, I think we can make a change to better suit your needs. We could add a new possible value for the type action constraint to match view submissions. Here’s what it might look like for you:

slackInteractions.action({ type: 'view_submission', callbackId: 'order_pizza' } , pizzaOrderSubmitted);

Would this change be something you’re interested in?

@aoberoi aoberoi reopened this Dec 11, 2019
@aoberoi aoberoi added the enhancement M-T: A feature request for new functionality label Jan 10, 2020
@misscoded
Copy link
Contributor

We are closing this issue as @slack/interactive-messages officially reached its EOL on May 31st, 2021. As of that date, development has fully stopped for this package and all remaining open issues and pull requests are being closed.

At this time, we recommend migrating to Bolt for JavaScript, a framework that offers all of the functionality available in this package (and more!). To help with that process, we've also provided some migration samples for those looking to convert their existing apps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality pkg:interactive-messages (deprecated) applies to `@slack/interactive-messages`
Projects
None yet
Development

No branches or pull requests

4 participants