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

🤦 Fixing issues around Oauth and SlackApiErrors #527

Merged
merged 1 commit into from Oct 8, 2019

Conversation

RodneyU215
Copy link
Contributor

Summary

Fixing #481 by properly passing oauth params with aiohttp.BasicAuth.

Fixing #436 by returning the entire SlackResponse in SlackApiError's.

Requirements (place an x in each [ ])

@RodneyU215 RodneyU215 added Priority: High bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 2x labels Oct 5, 2019
@RodneyU215 RodneyU215 self-assigned this Oct 5, 2019
@codecov
Copy link

codecov bot commented Oct 5, 2019

Codecov Report

Merging #527 into master will decrease coverage by 0.02%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
- Coverage   68.98%   68.96%   -0.03%     
==========================================
  Files          15       15              
  Lines        1709     1711       +2     
  Branches       96       97       +1     
==========================================
+ Hits         1179     1180       +1     
  Misses        507      507              
- Partials       23       24       +1
Impacted Files Coverage Δ
slack/errors.py 100% <ø> (ø) ⬆️
slack/web/client.py 33.61% <0%> (ø) ⬆️
slack/web/slack_response.py 100% <100%> (+2.22%) ⬆️
slack/web/base_client.py 79.61% <33.33%> (-1.58%) ⬇️

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 70883cb...436eb96. Read the comment docs.

@RodneyU215
Copy link
Contributor Author

@seratch can you take a look at these fixes?

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.

Both look good to me. 👍

@tyndyll
Copy link

tyndyll commented Oct 7, 2019

Will this be merged soon? This blocks all OAuth access via the SDK

@RodneyU215 RodneyU215 merged commit 612c68d into master Oct 8, 2019
@RodneyU215
Copy link
Contributor Author

Thanks @seratch!

@tyndyll I'll work on getting a patch release out today for this.

@RodneyU215 RodneyU215 deleted the RU_fixing_oauth_and_response_data branch October 8, 2019 12:53
@RodneyU215 RodneyU215 mentioned this pull request Oct 8, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 2x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants