view_closed support (Block Kit in Modals)

[seratch]

Description

As of version 1.3.0, Bolt lacks view_closed action support. view_closed events can be sent to slack app servers when notify_on_close in views.

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

Reproducible in:

package version: 1.3.0
node version: any
OS version(s): any

Steps to reproduce:

1. Call views.open having notify_on_close: true
2. Close the view on Slack
3. Bolt doesn't handle the action request, saying [WARN] Could not determine the type of an incoming event. No listeners will be called.

Expected result:

app.view('callback-id' , fn) can handle view_closed action as well.

Actual result:

As above.

Attachments:

Payload example:

{
  "type": "view_closed",
  "team": {
    "id": "T12345678",
    "domain": "domain"
  },
  "user": {
    "id": "U12345678",
    "username": "username",
    "name": "username",
    "team_id": "T12345678"
  },
  "api_app_id": "A12345678",
  "token": "random value",
  "view": {
    "id": "V12345678",
    "team_id": "T12345678",
    "type": "modal",
    "title": {
      "type": "plain_text",
      "text": "Title",
      "emoji": true
    },
    "close": null,
    "submit": {
      "type": "plain_text",
      "text": "Submit",
      "emoji": true
    },
    "blocks": [
    ],
    "private_metadata": "",
    "callback_id": "modal-callback-id",
    "state": {
      "values": {
      }
    },
    "hash": "random",
    "clear_on_close": false,
    "notify_on_close": true,
    "root_view_id": "V12345678",
    "previous_view_id": null,
    "app_id": "A12345678",
    "external_id": "",
    "bot_id": "B12345678"
  },
  "is_cleared": false
}

[shaydewael]

Sorry this is so long-winded but....
I started implementing this today, but got pretty stuck over the decision of what method the view_closed event should be handled with. I wanted to lay out the options here in case any others had input:

Option 1: event()

For incoming requests that we label as type "Event", there is a pretty big upside: we automatically acknowledge the request before we pass it to any listeners or middleware. This makes it so the developer won't have to call ack() in their listeners. This makes sense because you'll never have to acknowledge the event with anything other than 200 OK (for context: incoming requests like view_submission require you to acknowledge with different options depending on how you want to handle the submission or if there are any errors).

However, this is breaking convention because up until now, we've only had the event() method handle Events API events. Events API events can be sent to a different endpoint than interactive component events, but this doesn't matter much for Bolt since it will decide which kind of event the incoming request is based on its own logic rather than relying on endpoints. There might be additional developer overhead here if someone (rationally) thinks that events() should only handle Events API events.

Another (pretty big) downside is if you had several view submissions, there isn't a built-in way to filter on callback_id so you'd have to handle logic for all view closed events within one listener. A somewhat-viable workaround that I can think of off the top of my head is to provide a built-in middleware that allows you to filter on callback_id (does this already exist?) which you could attach to your event() listener

Ex: app.event('view_closed', ({ view }) => { // some logic })

Option 2: views()

Another option is using the views() method and adding a constraints object (similar to the constraints for the actions() method). This has an upside where you're handling all requests related to views in one type of listener, which may make more intuitive sense to a developer. The biggest downside for this option is that we'd likely force the developer to acknowledge the incoming view_closed events for consistency's sake. I don't think that's a huge problem, but it's a minor inconvenience.

Ex: app.views({ type: 'view_cancelled', callback_id: 'YOUR-CALLBACK-ID'}, ({ view, ack }) => { ack(); // some logic }); or maybe app.views('YOUR-CALLBACK-ID', viewCancelOnly()...

Option 3ish: actions()

Less interested in this option. It'd be less intuitive as to where the developer should handle the view cancellation and it would still require them to acknowledge it. The pro is that we only have a constraints object on the actions() method (I don't know if that's a real pro though).

I don't have a really strong opinion. I lean a little bit more towards the event() method after typing it out, but I'm worried that route may cause a little bit of confusion. Would love other's input if they have them, or any pros/cons that I may have missed (cc @seratch)

[seratch]

@shanedewael
Thanks for the detailed explanation 🙏 I vote for the Option 2, and also let me share my preferable interface later in this comment.

Option 1

No need to call ack() in this case is indeed pretty neat and reasonable, but I'm afraid that the two downsides are not ignorable. In particular, keeping to offer app.event only for Events API must be easy-to-understand for everyone. I don't want to have exceptions which require some explanation in docs here.

Option 3

From the viewpoint of API consistencies, there is no reason to go with app.action for it.
#265 (comment)

Option2

First of all, for convenience, I believe we should keep the behavior when not specifying a payload type as before (= means handling only view_submission events).

app.view('YOUR-CALLBACK-ID', ({ view, ack }) => { 
    // handle only view_submission events here 
});
app.view({callback_id: 'YOUR-CALLBACK-ID'}, ({ view, ack }) => { 
    // handle only view_submission events here 
});

Regarding the ways to specify a payload type, I came up with two ways as below. I want to allow both, but going with either is also excellent.

// Option A - giving a full name of type
app.view({callback_id: 'YOUR-CALLBACK-ID', type: 'view_closed'}, ({ view, ack }) => { });
app.view({callback_id: 'YOUR-CALLBACK-ID', type: 'view_submission'}, ({ view, ack }) => { });

// Option B - skipping the redundant part - `view_`, and giving only the suffix
app.view({callback_id: 'YOUR-CALLBACK-ID', type: 'closed'}, ({ view, ack }) => { });
app.view({callback_id: 'YOUR-CALLBACK-ID', type: 'submission'}, ({ view, ack }) => { });

The following part may be a bit off-topic, but, with this approach, we may be able to come up with a similar interface for dialogs. Introducing a new name may be able to make the framework simpler.

Disclaimer: I haven't verified the feasibility of providing the APIs for both JS and TS yet (particularly, a bit worrying about dialog_suggestion events).

// Option A - giving a full name of type
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'dialog_submission'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'dialog_suggestion'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'dialog_cancellation'}, ({ dialog, ack }) => { });

// Option B - skipping the redundant part - `dialog_`, and giving only the suffix
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'submission'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'suggestion'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'cancellation'}, ({ dialog, ack }) => { });

However, this approach introduces more ways to do the same. If we go with this, we may deprecate existing action and options for dialogs. In that case, we should have a quite long time until removing the deprecated ones.

// submission
app.action({callback_id: 'YOUR-CALLBACK-ID'}, ({ action, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'submission'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'dialog_submission'}, ({ dialog, ack }) => { });

// suggestion
app.options({callback_id: 'YOUR-CALLBACK-ID'}, ({ options, ack }) => { }}, 
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'suggestion'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'dialog_suggestion'}, ({ dialog, ack }) => { });

// cancellation
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'cancellation'}, ({ dialog, ack }) => { });
app.dialog({callback_id: 'YOUR-CALLBACK-ID', type: 'dialog_cancellation'}, ({ dialog, ack }) => { });

Having multiple ways for a single purpose is not a small downside, but having app.dialog and app.view looks more consistent to me.

.... let's get back on track. Regarding `view_closed support, if you go with Option 2 in some way, I don't have any objections for it.

[aoberoi]

I'm glad we're all in agreement! I just wanted to add one more advantage to Option 2 that I'm excited about.

With this design, you can write listener middleware that deals with general cleanup that occurs whenever a modal is dismissed, whether its because it was submitted or because it was canceled. Example:

// This function cleans up some temporary state from a data store and puts it on the context
// Use case: A partially initialized object is put in the database but fully initializing it depends on gathering input from the modal
function cleanUpTemporaryState({ context, next }) { /* ... */ }

// Submission handler
app.view('create_order', cleanUpTemporaryState, ({ view }) => { /* ... */ });

// Cancel handler
app.view(
  { callback_id: 'create_order', type: 'view_closed' },
  cleanUpTemporaryState,
  ({ view }) => { /* ... */ }
);

[smoneta]

how this would work with python bolt? I trued this but does not seems to work..

@app.view_closed('my_callback_id'):
def handle_some_action(body):
    print(f'The modal have been closed:{body}')

[seratch]

@smoneta Can you post a new question issue in https://github.com/slackapi/bolt-python/issues with a bit more details such as your code?

[smoneta]

Thanks, slackapi/bolt-python#624