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

Improve OAuth documentation #1315

Merged
merged 9 commits into from
Feb 15, 2022
Merged

Improve OAuth documentation #1315

merged 9 commits into from
Feb 15, 2022

Conversation

horeaporutiu
Copy link
Contributor

@horeaporutiu horeaporutiu commented Feb 14, 2022

Fixes #1306

Added details about when storeInstallation and fetchInstallation execute in Bolt

  • Added detail about the install path: if your app was hosted at www.example.com, you would be able to install your app at www.example.com/slack/install
  • Added details about OAuth process: first you must click on add to slack, then allow, then open Slack, and only then will fetch/storeInstallation will execute
  • Added sample installation object (useful for folks working on DB schemas before the fetch/storeInstallation runs
  • Added sample installQuery object

Summary

Describe the goal of this PR. Mention any related Issue numbers.

Requirements (place an x in each [ ])

Added details about when storeInstallation and fetchInstallation execute in Bolt
* Added detail about the install path: if your app was hosted at `www.example.com`, you would be able to install your app at `www.example.com/slack/install`
* Added details about OAuth process: first you must click on add to slack, then allow, then open Slack, and only then will fetch/storeInstallation will execute
* Added sample `installation` object (useful for folks working on DB schemas before the fetch/storeInstallation runs
* Added sample `installQuery` object
@srajiang srajiang added the docs M-T: Documentation work only label Feb 14, 2022
@srajiang srajiang added this to the 3.10.0 milestone Feb 14, 2022
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #1315 (354e4ea) into main (446622c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1315   +/-   ##
=======================================
  Coverage   72.88%   72.88%           
=======================================
  Files          17       17           
  Lines        1479     1479           
  Branches      442      442           
=======================================
  Hits         1078     1078           
  Misses        311      311           
  Partials       90       90           

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 446622c...354e4ea. Read the comment docs.

@srajiang srajiang changed the title fixes #1306 Improve OAuth documentation Feb 14, 2022
@srajiang
Copy link
Member

Thanks @horeaporutiu! FYI for some reason Github won't link the PR to close unless you add Fix #[issuenumber] in your PR description body vs in your PR header. I've gone ahead and updated it so that it's properly linked. Taking a look now!

Copy link
Member

@srajiang srajiang left a comment

Choose a reason for hiding this comment

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

I want to doublecheck when fetchInstallation vs. storeInstallation runs before 👍

I've added some suggestions for the time being on spacing / verbiage! Thanks again for this.

docs/_basic/authenticating_oauth.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Jiang <srajiang@gmail.com>
@horeaporutiu
Copy link
Contributor Author

Thanks for the suggestions @srajiang !! I've committed those. Yes, once quick double check would be amazing. Thanks!

@horeaporutiu
Copy link
Contributor Author

Btw - I've just added one last sentence in there about the Redirect URI / opening slack. I think this sentence is critical for understanding the full process.

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 for improving this! Left a comment

docs/_basic/authenticating_oauth.md Outdated Show resolved Hide resolved
add installerOptions.directInstall:true details

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
@seratch
Copy link
Member

seratch commented Feb 15, 2022

LGTM! We can merge this PR tomorrow in Pacific Time (if there is no further feedback) 👍

@horeaporutiu
Copy link
Contributor Author

@seratch I added some sample code showing installOptions:true. We can remove it if you think it is too verbose. Let me know :)

stateSecret: 'my-state-secret',
scopes: ['chat:write'],
installerOptions: {
directInstall: true,
Copy link
Member

Choose a reason for hiding this comment

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

@horeaporutiu Nice! but this option is optional while the rest are required. Can you add some code comment indicating that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seratch Comment has been added! Thanks for the suggestion!

Copy link
Member

@srajiang srajiang left a comment

Choose a reason for hiding this comment

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

Alrighty, thanks for patience as I reviewed the InstallProvider. Looks like fetchInstallation and storeInstallation are run anytime authorize() method is run, and that would happen when the receiver processes events from Slack, so I have added a suggestion to make that clearer. Feel free to reword if you think the meaning can be clearer.

docs/_basic/authenticating_oauth.md Outdated Show resolved Hide resolved
adding suggestions from Sarah

Co-authored-by: Sarah Jiang <srajiang@gmail.com>
@srajiang srajiang self-requested a review February 15, 2022 18:45
@srajiang srajiang merged commit 66ec23c into main Feb 15, 2022
@srajiang srajiang deleted the imporove-installationStore-docs branch February 15, 2022 18:57
@yamashush yamashush mentioned this pull request Dec 24, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve installationStore OAuth documentation
3 participants