Skip to content

[beta] Add support for Message Metadata and Subscribe in Slack API #1151

Merged
srajiang merged 4 commits intoslackapi:feat-hermesfrom
srajiang:msg-metadata-subscribe-in-slack
Dec 13, 2021
Merged

[beta] Add support for Message Metadata and Subscribe in Slack API #1151
srajiang merged 4 commits intoslackapi:feat-hermesfrom
srajiang:msg-metadata-subscribe-in-slack

Conversation

@srajiang
Copy link
Copy Markdown
Contributor

@srajiang srajiang commented Dec 11, 2021

Summary

These are changes intended for beta release - as these are not GA, I have not yet included method unit tests.

  • Adds Message Metadata field to chat.postMessage and chat.update methods
  • Adds apps.notifications.subscriptions.* methods
  • Adds a todo to implement tests once features are GA

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@srajiang srajiang added the enhancement M-T: A feature request for new functionality label Dec 11, 2021
@srajiang srajiang self-assigned this Dec 11, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 11, 2021 •
edited
Loading

Codecov Report

Merging #1151 (efe3e2b) into feat-hermes (aae7460) will decrease coverage by 0.13%.
The diff coverage is 33.33%.

❗ Current head efe3e2b differs from pull request most recent head 1225c9a. Consider uploading reports for the commit 1225c9a to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           feat-hermes    #1151      +/-   ##
===============================================
- Coverage        86.65%   86.52%   -0.14%     
===============================================
  Files              110      110              
  Lines            10760    10787      +27     
===============================================
+ Hits              9324     9333       +9     
- Misses            1436     1454      +18
Impacted Files Coverage Δ
slack_sdk/web/async_client.py 84.39% <33.33%> (-0.47%) ⬇️
slack_sdk/web/client.py 85.80% <33.33%> (-0.49%) ⬇️
slack_sdk/web/legacy_client.py 85.51% <33.33%> (-0.49%) ⬇️

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 aae7460...1225c9a. Read the comment docs.

filmaj
filmaj approved these changes Dec 13, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@filmaj filmaj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

Seems like test coverage is a bit low on this PR. Looks like the named methods like apps_notifications_subscriptions_create call into the api_call method but slightly modify the keyworded arguments (**kwargs) before calling into api_call. It seems to me like this modify-kwargs logic is what is not covered by unit tests.

Not sure how strict we are in adhering to the code coverage. Given the structure of this code, the unit tests would be very light to ensure coverage. Adding these tests would be more about chasing coverage than preventing regressions or assuring quality, but, green check marks are nice?

@srajiang
Copy link
Copy Markdown
Contributor Author

srajiang commented Dec 13, 2021

@filmaj - Re: Test coverage. Great questions you raised - Since this would be merged into a beta release branch feat-hermes it does decrease coverage on the branch, though ultimately if it's to be merged into main the reduction in coverage will need to be addressed.

I will bring this to the team to see what our M.O should be when we're doing beta release. This feature is a little trickier to support with tests as well, since a notification subscription request cannot be initiated from the test suite, despite the way the API method is named.

@srajiang
Copy link
Copy Markdown
Contributor Author

srajiang commented Dec 13, 2021

Is there a good way anyone knows of to mock trigger_id in integration testing? I’d like to know if it’s possible to write tests for our Subscribe APIs since there’s a manual step where the user has to initiate the subscribe flow in Slack Client - this action generates the trigger_id which is required to pass as params into create.

seratch
seratch suggested changes Dec 13, 2021
View reviewed changes
seratch
seratch reviewed Dec 13, 2021
View reviewed changes
@srajiang
Copy link
Copy Markdown
Contributor Author

srajiang commented Dec 13, 2021

@seratch - Things should be all synced now!

@srajiang srajiang requested a review from seratch December 13, 2021 23:10
seratch
seratch approved these changes Dec 13, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM. You can squash the commits when merging 👍

@srajiang srajiang merged commit 963437f into slackapi:feat-hermes Dec 13, 2021
srajiang added a commit to srajiang/python-slack-sdk that referenced this pull request Dec 15, 2021
@srajiang
[beta] Add support for Message Metadata and Subscribe in Slack API (s…
b83f17b 
…lackapi#1151)

* Add apps.notifications.subscriptions.* methods + metadata
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevengill stevengill stevengill approved these changes

+2 more reviewers

@seratch seratch seratch approved these changes

@filmaj filmaj filmaj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@srajiang srajiang

Labels

enhancement M-T: A feature request for new functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@srajiang @seratch @filmaj @stevengill