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

feat: add isEnterpriseInstall to Context #1511

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

rockingskier
Copy link
Contributor

Summary

#1510 - Add isEnterpriseInstall to aid in processing events.

Requirements (place an x in each [ ])

@CLAassistant
Copy link

CLAassistant commented Jul 1, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rockingskier rockingskier force-pushed the feat/is-enterprise-install branch from a5866ee to b81d900 Compare July 1, 2022 22:08
@rockingskier
Copy link
Contributor Author

I've just seen that events have access to body.authorizations[0].is_enterprise_install and actions have access to body.is_enterprise_install. However, given that Bolt has already parsed this information I think it'd be nice to have it available on context :)

@seratch seratch self-assigned this Jul 2, 2022
@seratch seratch added enhancement M-T: A feature request for new functionality semver:minor labels Jul 2, 2022
@seratch seratch added this to the 3.12.0 milestone Jul 2, 2022
@seratch seratch self-requested a review July 2, 2022 02:37
src/App.ts Show resolved Hide resolved
src/App.ts Show resolved Hide resolved
@seratch seratch requested a review from filmaj July 2, 2022 02:37
@rockingskier rockingskier force-pushed the feat/is-enterprise-install branch from b81d900 to 845ff64 Compare July 2, 2022 07:34
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Looks good to me! Wow I had no idea of how complex the installation scenarios could be (i.e. individual workspace installs that exist inside an enterprise org). Thank you for this change 🙇

@filmaj
Copy link
Contributor

filmaj commented Jul 5, 2022

@rockingskier looks like some of the tests are failing, I believe because some unrelated tests are using empty mock objects for the context property, but due to this PR, context now requires the isEnterpriseInstall property in it. Do you think you can update these so that the tests pass?

@rockingskier rockingskier force-pushed the feat/is-enterprise-install branch from 845ff64 to f2bd0d0 Compare July 5, 2022 17:07
@rockingskier
Copy link
Contributor Author

Thanks, @filmaj. That was embarrassingly obvious :D
All fixed up and working locally now.

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #1511 (f2bd0d0) into main (7eec438) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1511   +/-   ##
=======================================
  Coverage   82.00%   82.00%           
=======================================
  Files          18       18           
  Lines        1495     1495           
  Branches      435      435           
=======================================
  Hits         1226     1226           
  Misses        172      172           
  Partials       97       97           
Impacted Files Coverage Δ
src/App.ts 83.87% <ø> (ø)
src/types/middleware.ts 100.00% <ø> (ø)

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 7eec438...f2bd0d0. Read the comment docs.

@filmaj filmaj merged commit 9bb3cb0 into slackapi:main Jul 5, 2022
@filmaj
Copy link
Contributor

filmaj commented Jul 5, 2022

Thank you very much @rockingskier ! Much appreciated 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants