Skip to content

Conversation

@paul-griffith
Copy link
Contributor

Summary

Fixes a bug in #400; the base Attachment class that BlockAttachment inherits from will populate the fields attribute with an empty list, which then gets sent in the JSON to Slack. That makes the JSON invalid, meaning the BlockAttachment class as it is now cannot be used without manually adjusting the JSON/dict returned.

This PR updates the class, as well as adds a test that would have caught this.

Requirements (place an x in each [ ])

@paul-griffith
Copy link
Contributor Author

I started using 2.1.0 internally and immediately ran into this 🤦‍♂️

@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #473 into master will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage   69.58%   69.94%   +0.35%     
==========================================
  Files          15       15              
  Lines        1654     1657       +3     
  Branches       91       91              
==========================================
+ Hits         1151     1159       +8     
+ Misses        481      476       -5     
  Partials       22       22
Impacted Files Coverage Δ
slack/web/classes/attachments.py 98.8% <100%> (+6.21%) ⬆️

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 df5351f...decad0e. Read the comment docs.

@RodneyU215
Copy link
Contributor

Thanks for your diligence Paul!

@RodneyU215 RodneyU215 added Priority: High bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 2x labels Aug 29, 2019
@RodneyU215 RodneyU215 merged commit aabf2d0 into slackapi:master Aug 29, 2019
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.

2 participants