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

Jump to bottom

Add org app support to bolt #521

Merged
merged 13 commits into from
Nov 25, 2020
Merged

Add org app support to bolt #521

merged 13 commits into from
Nov 25, 2020

Conversation

stevengill
Copy link
Member

@stevengill stevengill commented Jun 18, 2020 •
edited
Loading

Summary

Added Org App support

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Jun 19, 2020 •
edited
Loading

Codecov Report

Merging #521 (136f28d) into main (ca732ea) will decrease coverage by 4.74%.
The diff coverage is 35.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #521      +/-   ##
==========================================
- Coverage   84.41%   79.67%   -4.75%     
==========================================
  Files           8        8              
  Lines         693      738      +45     
  Branches      206      238      +32     
==========================================
+ Hits          585      588       +3     
- Misses         71      107      +36     
- Partials       37       43       +6
Impacted Files Coverage Δ
src/App.ts 75.26% <35.44%> (-12.76%) ⬇️

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 99b9c32...8a14745. Read the comment docs.

stevengill
stevengill commented Jun 19, 2020
View reviewed changes
src/App.ts Outdated Show resolved Hide resolved
src/types/actions/block-action.ts
@@ -237,6 +237,14 @@ export interface BlockAction<ElementAction extends BasicElementAction = BlockEle

// this appears in the block_suggestions schema, but we're not sure when its present or what its type would be
app_unfurl?: any;

// exists for enterprise installs
is_enterprise_install?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed that button gets is_enterprise_install & enterprise: {id: '', name: ''}. Not sure if we should confirm the payloads for the other block elements in an enterprise (&| org install) world.

src/types/actions/dialog-action.ts Show resolved Hide resolved
src/types/actions/interactive-message.ts Show resolved Hide resolved
src/types/options/index.ts Show resolved Hide resolved
src/types/view/index.ts Show resolved Hide resolved
src/types/view/index.ts Show resolved Hide resolved
@clavin clavin changed the base branch from master to main July 8, 2020 03:09
stevengill
stevengill commented Jul 15, 2020
View reviewed changes
src/App.ts Outdated Show resolved Hide resolved
@stevengill stevengill self-assigned this Jul 28, 2020
@stevengill stevengill force-pushed the feat-org-apps branch 2 times, most recently from 7231bf4 to 91163fe Compare August 4, 2020 22:13
@stevengill stevengill added enhancement M-T: A feature request for new functionality semver:minor labels Aug 5, 2020
@stevengill stevengill marked this pull request as draft October 2, 2020 22:55
@misscoded misscoded added the draft A pull request that is currently in active development and not yet ready for review label Oct 5, 2020
stevengill
stevengill commented Nov 14, 2020
View reviewed changes
src/App.ts Show resolved Hide resolved
src/types/actions/block-action.ts
@@ -209,7 +209,7 @@ export interface BlockAction<ElementAction extends BasicElementAction = BlockEle
domain: string;
enterprise_id?: string; // undocumented
enterprise_name?: string; // undocumented
Copy link
Member Author

Choose a reason for hiding this comment

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

Verify if enterprise_id & enterpise_name are still arriving in this team object

@stevengill stevengill marked this pull request as ready for review November 24, 2020 01:09
seratch
seratch approved these changes Nov 24, 2020
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks. I did not detect anything to change so far.

src/App.ts Outdated
}
}
if (pool !== undefined) {
// Question: should teamId from source or authResult be passed in via clientOptionsCopy
Copy link
Member

Choose a reason for hiding this comment

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

In the case of enterprise_install, yes, I think so. We can set the default team_id to the client instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to update this to use the teamID from authorizeResult first if it exists, if not use the teamId from source

src/App.ts Outdated Show resolved Hide resolved
src/App.ts
source = {
teamId:
type === IncomingEventType.Event || type === IncomingEventType.Command
? ((body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).team_id as string)
Copy link
Member

Choose a reason for hiding this comment

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

just in case, we need to update the logic for SlackEventMiddlewareArgs - #687

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! I've gone ahead and added that fix right into this PR as I'm planning on releasing this today/tomorrow.

@stevengill stevengill merged commit 4664328 into main Nov 25, 2020
@stevengill stevengill deleted the feat-org-apps branch November 25, 2020 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

@misscoded misscoded Awaiting requested review from misscoded

@aoberoi aoberoi Awaiting requested review from aoberoi

@mwbrooks mwbrooks Awaiting requested review from mwbrooks

Assignees

@stevengill stevengill

Labels
draft A pull request that is currently in active development and not yet ready for review enhancement M-T: A feature request for new functionality semver:minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stevengill @seratch @misscoded