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

Fix slack session when channel id is null #802

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

etrex
Copy link
Contributor

@etrex etrex commented Jun 15, 2020

issue

When user clicked a button on home tab, session key can't generate correctly.

spec

If the slack event is not related to any channel, then the session key set to the user id.

@etrex etrex changed the base branch from master to release/1.5 June 15, 2020 11:16
@etrex etrex requested a review from chentsulin June 15, 2020 11:17
@chentsulin chentsulin requested a review from tw0517tw June 15, 2020 11:35
promises.channelMembers = this._client.getAllConversationMembers(
channelId
);
if (channelId != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (channelId != null) {
if (channelId) {

@etrex etrex changed the title Fix slack session when channel id is null WIP:Fix slack session when channel id is null Jun 15, 2020
const rawEvent = this._getRawEventFromRequest(body) as any;

// FIXME: define types for every slack events
getChannelId(rawEvent: any): string | null {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what will this method return if it's app home event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2020

Codecov Report

Merging #802 into release/1.5 will increase coverage by 2.56%.
The diff coverage is 86.92%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/1.5     #802      +/-   ##
===============================================
+ Coverage        81.08%   83.65%   +2.56%     
===============================================
  Files              108      115       +7     
  Lines             4747     6068    +1321     
  Branches          1197     1491     +294     
===============================================
+ Hits              3849     5076    +1227     
- Misses             862      988     +126     
+ Partials            36        4      -32     
Impacted Files Coverage Δ
...s/bottender/src/cli/providers/messenger/profile.ts 83.05% <ø> (-3.56%) ⬇️
packages/bottender/src/console/ConsoleConnector.ts 100.00% <ø> (ø)
packages/bottender/src/console/ConsoleContext.ts 100.00% <ø> (ø)
packages/bottender/src/line/LineEvent.ts 96.64% <ø> (-1.11%) ⬇️
...ckages/bottender/src/messenger/MessengerContext.ts 83.59% <ø> (-1.48%) ⬇️
packages/bottender/src/slack/SlackContext.ts 92.20% <ø> (+6.00%) ⬆️
...ckages/bottender/src/telegram/TelegramConnector.ts 96.93% <ø> (+0.38%) ⬆️
...kages/bottender/src/test-utils/SimulatedContext.ts 87.50% <ø> (-12.50%) ⬇️
packages/bottender/src/types.ts 100.00% <ø> (ø)
packages/bottender/src/viber/ViberConnector.ts 91.56% <ø> (+5.60%) ⬆️
... and 117 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b251e4...658916c. Read the comment docs.

@etrex etrex changed the title WIP:Fix slack session when channel id is null Fix slack session when channel id is null Jun 16, 2020
@etrex etrex requested a review from tw0517tw June 16, 2020 04:57
@chentsulin chentsulin merged commit 773667b into release/1.5 Jun 16, 2020
@chentsulin chentsulin deleted the fix-slack-session-when-channel-id-is-null branch June 16, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants