Skip to content

🤦 Fixing issues around Oauth and SlackApiErrors#527

Merged
RodneyU215 merged 1 commit intomasterfrom
RU_fixing_oauth_and_response_data
Oct 8, 2019
Merged

🤦 Fixing issues around Oauth and SlackApiErrors#527
RodneyU215 merged 1 commit intomasterfrom
RU_fixing_oauth_and_response_data

Conversation

@RodneyU215
Copy link
Contributor

@RodneyU215 RodneyU215 commented Oct 5, 2019

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 requested a review from seratch October 5, 2019 04:26
@RodneyU215 RodneyU215 self-assigned this Oct 5, 2019
@codecov
Copy link

codecov bot commented Oct 5, 2019 •
edited
Loading

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

RodneyU215 commented Oct 5, 2019

@seratch can you take a look at these fixes?

seratch
seratch approved these changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@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

RodneyU215 commented Oct 8, 2019

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
Merged
2 tasks
@gmkohler gmkohler mentioned this pull request Nov 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@seratch seratch seratch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RodneyU215 RodneyU215

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 2x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RodneyU215 @tyndyll @seratch