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

Adds view_closed support #276

Merged
merged 14 commits into from
Oct 7, 2019
42 changes: 21 additions & 21 deletions src/App.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -432,25 +432,24 @@ describe('App', () => {
team: {},
view: {
callback_id: 'view_callback_id',
}
},
},
respond: noop,
ack: noop,
},
{
body: {
type: 'view_closed',
channel: {},
user: {},
team: {},
view: {
callback_id: 'view_callback_id',
},
},
respond: noop,
ack: noop,
},
// TODO: https://github.com/slackapi/bolt/issues/263
Copy link
Member

Choose a reason for hiding this comment

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

nit: we also can verify if the condition works by having the following lines in the test suite.

// add a new routing definition
app.view({callback_id: 'view_callback_id', type: 'view_closed'}, ({ }) => { viewFn(); })

// modify assert.equal(viewFn.callCount, 1); as below:
assert.equal(viewFn.callCount, 2);

// {
// body: {
// type: 'view_closed',
// channel: {},
// user: {},
// team: {},
// view: {
// callback_id: 'view_callback_id',
// }
// },
// respond: noop,
// ack: noop,
// },
];
}

Expand All @@ -467,11 +466,12 @@ describe('App', () => {
// Act
const app = new App({ receiver: fakeReceiver, authorize: sinon.fake.resolves(dummyAuthorizationResult) });
app.use((_args) => { ackFn(); });
app.action('block_action_id', ({ }) => { actionFn(); })
app.action({ callback_id: 'message_action_callback_id' }, ({ }) => { actionFn(); })
app.action({ callback_id: 'interactive_message_callback_id' }, ({ }) => { actionFn(); })
app.action({ callback_id: 'dialog_submission_callback_id' }, ({ }) => { actionFn(); })
app.view('view_callback_id', ({ }) => { viewFn(); })
app.action('block_action_id', ({ }) => { actionFn(); });
app.action({ callback_id: 'message_action_callback_id' }, ({ }) => { actionFn(); });
app.action({ callback_id: 'interactive_message_callback_id' }, ({ }) => { actionFn(); });
app.action({ callback_id: 'dialog_submission_callback_id' }, ({ }) => { actionFn(); });
app.view('view_callback_id', ({ }) => { viewFn(); });
app.view({ callback_id: 'view_callback_id', type: 'view_closed' }, ({ }) => { viewFn(); });
Comment on lines +473 to +474
Copy link

@rtrembecky rtrembecky Nov 4, 2021

Choose a reason for hiding this comment

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

hi, I was looking for a way to listen to view closed actions and I stumbled upon this PR.
can you please explain why this doesn't trigger multiple listeners? why is the view_closed action not handled by app.view('view_callback_id', ...) as well?

Choose a reason for hiding this comment

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

I wanted to write a listener to handle both the submission and the close action in the same function and I expected to write app.view('view_callback_id', ...) so it catches both 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@rtrembecky

can you please explain why this doesn't trigger multiple listeners?

No, both listeners are triggered here. The following assertion assert.equal(viewFn.callCount, 2); verifies if viewFn is called twice.

Thus, as long as you set only callback_id to an app.view listener, the listener should work as you expect.

Choose a reason for hiding this comment

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

thanks for the quick reply. however, that's not what I experience. I have the following listeners:

this.boltApp.view(FEEDBACK_NOTE_CALLBACK_ID, ({body, ack}) => this.handleFeedbackNoteCallback(body, ack, 'first'))
this.boltApp.view({callback_id: FEEDBACK_NOTE_CALLBACK_ID, type: 'view_closed'}, ({body, ack}) => this.handleFeedbackNoteCallback(body, ack, 'second'))

I'm logging the "first" and "second" strings. When closing a modal, I just tested only the second function is called.
I'm on "@slack/bolt": "^3.8.1",. Maybe this is a regression or something 🤷‍♂️ But I don't have time to create an issue or investigate further, as I have the solution that works. Thanks for your time.

app.options('external_select_action_id', ({ }) => { optionsFn(); });
app.options({ callback_id: 'dialog_suggestion_callback_id' }, ({ }) => { optionsFn(); });

Expand All @@ -481,7 +481,7 @@ describe('App', () => {

// Assert
assert.equal(actionFn.callCount, 4);
assert.equal(viewFn.callCount, 1);
assert.equal(viewFn.callCount, 2);
assert.equal(optionsFn.callCount, 2);
assert.equal(ackFn.callCount, dummyReceiverEvents.length);
assert(fakeErrorHandler.notCalled);
Expand Down
55 changes: 46 additions & 9 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ import {
onlyEvents,
matchEventType,
matchMessage,
onlyViewSubmits,
matchCallbackId,
onlyViewActions,
} from './middleware/builtin';
import { processMiddleware } from './middleware/process';
import { ConversationStore, conversationContext, MemoryStore } from './conversation-store';
Expand All @@ -33,6 +32,7 @@ import {
OptionsSource,
BlockAction,
InteractiveMessage,
SlackViewAction,
Receiver,
ReceiverEvent,
} from './types';
Expand Down Expand Up @@ -92,6 +92,11 @@ export interface ActionConstraints {
callback_id?: string | RegExp;
}

export interface ViewConstraints {
callback_id?: string | RegExp;
type?: 'view_closed' | 'view_submission';
}

export interface ErrorHandler {
(error: CodedError): void;
}
Expand Down Expand Up @@ -318,9 +323,39 @@ export default class App {
);
}

public view(callbackId: string | RegExp, ...listeners: Middleware<SlackViewMiddlewareArgs>[]): void {
public view<ViewActionType extends SlackViewAction = SlackViewAction>(
callbackId: string | RegExp,
...listeners: Middleware<SlackViewMiddlewareArgs<ViewActionType>>[]
): void;
public view<ViewActionType extends SlackViewAction = SlackViewAction>(
constraints: ViewConstraints,
...listeners: Middleware<SlackViewMiddlewareArgs<ViewActionType>>[]
): void;
public view<ViewActionType extends SlackViewAction = SlackViewAction>(
callbackIdOrConstraints: string | RegExp | ViewConstraints,
...listeners: Middleware<SlackViewMiddlewareArgs<ViewActionType>>[]): void {
const constraints: ViewConstraints =
(typeof callbackIdOrConstraints === 'string' || util.types.isRegExp(callbackIdOrConstraints)) ?
{ callback_id: callbackIdOrConstraints, type: 'view_submission' } : callbackIdOrConstraints;
// Fail early if the constraints contain invalid keys
const unknownConstraintKeys = Object.keys(constraints)
.filter(k => (k !== 'callback_id' && k !== 'type'));
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
if (unknownConstraintKeys.length > 0) {
this.logger.error(
`View listener cannot be attached using unknown constraint keys: ${unknownConstraintKeys.join(', ')}`,
);
return;
}

if (constraints.type !== undefined && !validViewTypes.includes(constraints.type)) {
this.logger.error(
`View listener cannot be attached using unknown view event type: ${constraints.type}`,
);
return;
}

this.listeners.push(
[onlyViewSubmits, matchCallbackId(callbackId), ...listeners] as Middleware<AnyMiddlewareArgs>[],
[onlyViewActions, matchConstraints(constraints), ...listeners] as Middleware<AnyMiddlewareArgs>[],
);
}

Expand Down Expand Up @@ -384,7 +419,7 @@ export default class App {
payload:
(type === IncomingEventType.Event) ?
(bodyArg as SlackEventMiddlewareArgs['body']).event :
(type === IncomingEventType.ViewSubmitAction) ?
(type === IncomingEventType.ViewAction) ?
(bodyArg as SlackViewMiddlewareArgs['body']).view :
(type === IncomingEventType.Action &&
isBlockActionOrInteractiveMessageBody(bodyArg as SlackActionMiddlewareArgs['body'])) ?
Expand Down Expand Up @@ -412,7 +447,7 @@ export default class App {
} else if (type === IncomingEventType.Options) {
const optionListenerArgs = listenerArgs as SlackOptionsMiddlewareArgs<OptionsSource>;
optionListenerArgs.options = optionListenerArgs.payload;
} else if (type === IncomingEventType.ViewSubmitAction) {
} else if (type === IncomingEventType.ViewAction) {
const viewListenerArgs = listenerArgs as SlackViewMiddlewareArgs;
viewListenerArgs.view = viewListenerArgs.payload;
}
Expand Down Expand Up @@ -476,6 +511,8 @@ export default class App {
const tokenUsage = 'Apps used in one workspace should be initialized with a token. Apps used in many workspaces ' +
'should be initialized with a authorize.';

const validViewTypes = ['view_closed', 'view_submission'];

/**
* Helper which builds the data structure the authorize hook uses to provide tokens for the context.
*/
Expand All @@ -491,11 +528,11 @@ function buildSource(
const source: AuthorizeSourceData = {
teamId:
((type === IncomingEventType.Event || type === IncomingEventType.Command) ? (body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).team_id as string :
(type === IncomingEventType.Action || type === IncomingEventType.Options || type === IncomingEventType.ViewSubmitAction) ? (body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).team.id as string :
(type === IncomingEventType.Action || type === IncomingEventType.Options || type === IncomingEventType.ViewAction) ? (body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).team.id as string :
assertNever(type)),
enterpriseId:
((type === IncomingEventType.Event || type === IncomingEventType.Command) ? (body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).enterprise_id as string :
(type === IncomingEventType.Action || type === IncomingEventType.Options || type === IncomingEventType.ViewSubmitAction) ? (body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).team.enterprise_id as string :
(type === IncomingEventType.Action || type === IncomingEventType.Options || type === IncomingEventType.ViewAction) ? (body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).team.enterprise_id as string :
undefined),
userId:
((type === IncomingEventType.Event) ?
Expand All @@ -504,7 +541,7 @@ function buildSource(
((body as SlackEventMiddlewareArgs['body']).event.channel !== undefined && (body as SlackEventMiddlewareArgs['body']).event.channel.creator !== undefined) ? (body as SlackEventMiddlewareArgs['body']).event.channel.creator as string :
((body as SlackEventMiddlewareArgs['body']).event.subteam !== undefined && (body as SlackEventMiddlewareArgs['body']).event.subteam.created_by !== undefined) ? (body as SlackEventMiddlewareArgs['body']).event.subteam.created_by as string :
undefined) :
(type === IncomingEventType.Action || type === IncomingEventType.Options || type === IncomingEventType.ViewSubmitAction) ? (body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).user.id as string :
(type === IncomingEventType.Action || type === IncomingEventType.Options || type === IncomingEventType.ViewAction) ? (body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).user.id as string :
(type === IncomingEventType.Command) ? (body as SlackCommandMiddlewareArgs['body']).user_id as string :
undefined),
conversationId: channelId,
Expand Down
2 changes: 1 addition & 1 deletion src/ExpressReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ function parseRequestBody(
// Parse this body anyway
return JSON.parse(stringBody);
} catch (e) {
logger.error(`Failed to parse body as JSON data for content-type: ${contentType}`)
logger.error(`Failed to parse body as JSON data for content-type: ${contentType}`);
throw e;
}
}
Expand Down
30 changes: 30 additions & 0 deletions src/helpers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,21 @@ describe('getTypeAndConversation()', () => {
});
});

describe('view types', () => {
// Arrange
const dummyViewBodies = createFakeViews();

dummyViewBodies.forEach((viewBody) => {
it(`should find Action type for ${viewBody.type}`, () => {
// Act
const typeAndConversation = getTypeAndConversation(viewBody);

// Assert
assert(typeAndConversation.type === IncomingEventType.ViewAction);
});
});
});

describe('invalid events', () => {
// Arrange
const fakeEventBody = {
Expand Down Expand Up @@ -150,3 +165,18 @@ function createFakeOptions(conversationId: string): any[] {
},
];
}

function createFakeViews(): any[] {
return [
// Body for a view_submission event
{
type: 'view_submission',
view: { id: 'V123' },
},
// Body for a view_closed event
{
type: 'view_closed',
view: { id: 'V456' },
},
];
}
6 changes: 3 additions & 3 deletions src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export enum IncomingEventType {
Action,
Command,
Options,
ViewSubmitAction,
ViewAction,
}

/**
Expand Down Expand Up @@ -54,9 +54,9 @@ export function getTypeAndConversation(body: any): { type?: IncomingEventType, c
conversationId: actionBody.channel !== undefined ? actionBody.channel.id : undefined,
};
}
if (body.type === 'view_submission') {
if (body.type === 'view_submission' || body.type === 'view_closed') {
return {
type: IncomingEventType.ViewSubmitAction,
type: IncomingEventType.ViewAction,
};
}
return {};
Expand Down
Loading