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.

Sign up for GitHub

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
Merged

Adds view_closed support #276

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
250fc78
Add view_closed functionality and consolidate matchConstraints
Oct 3, 2019
36f970d
Add getTypeAndConversation() tests for views
Oct 3, 2019
9c4a075
Merge branch 'master' into add-view-closed-support
Oct 3, 2019
cf03031
Merge branch 'master' into add-view-closed-support
Oct 4, 2019
6b3564d
Add unknownConstraints check
Oct 4, 2019
a28c65d
Merge branch 'add-view-closed-support' of github.com:shanedewael/bolt…
Oct 4, 2019
51e6811
add kaz test
Oct 4, 2019
ab6909f
Kaz test
Oct 7, 2019
c89ddbb
Add unknown view event type check
Oct 7, 2019
43f0ce0
view_closed cleanup
Oct 7, 2019
479cf3a
clarity in error message
Oct 7, 2019
b99bb8d
Cleanup doo dah
Oct 7, 2019
a7cd1d7
a wild console.log was stranded. i successfully rescued it and brough…
Oct 7, 2019
c4114be
Merge branch 'master' into add-view-closed-support
Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions src/App.spec.ts
View file
Open in desktop
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 •
edited
Loading

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?

Copy link

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.

Copy link

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
View file
Open in desktop
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 @@ -90,6 +90,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 @@ -312,9 +317,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 @@ -378,7 +413,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 @@ -406,7 +441,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 @@ -470,6 +505,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 @@ -485,11 +522,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 @@ -498,7 +535,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
4 changes: 2 additions & 2 deletions src/ExpressReceiver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
constructor({
signingSecret = '',
logger = new ConsoleLogger(),
endpoints = { events: '/slack/events' }
endpoints = { events: '/slack/events' },
}: ExpressReceiverOptions) {
super();

Expand Down Expand Up @@ -256,7 +256,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
View file
Open in desktop
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
View file
Open in desktop
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