Fix #977 Add a way to turn off web page rendering for "/slack/install" #1079
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1079 +/- ##
==========================================
+ Coverage 68.73% 70.90% +2.16%
==========================================
Files 13 13
Lines 1206 1220 +14
Branches 355 365 +10
==========================================
+ Hits 829 865 +36
+ Misses 304 284 -20
+ Partials 73 71 -2
Continue to review full report at Codecov.
|
src/receivers/ExpressReceiver.ts
Outdated
| @@ -41,6 +41,7 @@ interface InstallerOptions { | |||
| authVersion?: InstallProviderOptions['authVersion']; // default 'v2' | |||
| metadata?: InstallURLOptions['metadata']; | |||
| installPath?: string; | |||
| directInstallUrlEnabled?: boolean; // see https://api.slack.com/start/distributing/directory#direct_install | |||
There was a problem hiding this comment.
Do we prefer something simpler like directInstall: boolean?
There was a problem hiding this comment.
I like the shorter name but that's only my personal preference
There was a problem hiding this comment.
Got two votes 😄 Renamed.
src/receivers/ExpressReceiver.ts
Outdated
| @@ -41,6 +41,7 @@ interface InstallerOptions { | |||
| authVersion?: InstallProviderOptions['authVersion']; // default 'v2' | |||
| metadata?: InstallURLOptions['metadata']; | |||
| installPath?: string; | |||
| directInstallUrlEnabled?: boolean; // see https://api.slack.com/start/distributing/directory#direct_install | |||
There was a problem hiding this comment.
I like the shorter name but that's only my personal preference
|
I think this one is ready for merge but just in case, I'll wait for others' comments until tomorrow. |
Co-authored-by: Fil Maj <maj.fil@gmail.com>
seratch
force-pushed
the
issue-977-slack-install-flag
branch
from
3c82e94 to
516d5b7
Compare
| "author": "Slack Technologies, Inc.", | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "@slack/bolt": "^3.6.0" |
There was a problem hiding this comment.
Since the installer option would be available in 3.7.0 release, it's probably okay with the ^, but since you've pointed out already that the flag is available to 3.7.0 and on perhaps this should be ^3.7.0?
There was a problem hiding this comment.
As the flag is commented out now, the example app works even with v3.6.0. I am using npm link for verifying the behavior with the flag. Once we release v3.7, we can upgrade the minimum version here.
| ); | ||
| const HTTPReceiver = await importHTTPReceiver(overrides); | ||
|
|
||
| const metadata = 'this is bat country'; |
There was a problem hiding this comment.
This is not mine. I just copied and pasted the example in the document 😄
There was a problem hiding this comment.
Okay, I was thinking - wow what an obscure American cultural reference @seratch is making here....
Co-authored-by: Sarah Jiang <srajiang@gmail.com>
Summary
This pull request fixes #977. Refer to the issue for details.
Requirements (place an
xin each[ ])