Skip to content

Fix #1108 add support for admin.conversations retention#1109

Merged
seratch merged 9 commits intoslackapi:mainfrom
ruberVulpes:fix-1108
Sep 7, 2021
Merged

Fix #1108 add support for admin.conversations retention#1109
seratch merged 9 commits intoslackapi:mainfrom
ruberVulpes:fix-1108

Conversation

@ruberVulpes
Copy link
Copy Markdown
Contributor

@ruberVulpes ruberVulpes commented Sep 3, 2021 •
edited
Loading

Summary

Implements #1108

Adding support for the admin conversation retention methods.

https://api.slack.com/methods/admin.conversations.getCustomRetention
https://api.slack.com/methods/admin.conversations.removeCustomRetention
https://api.slack.com/methods/admin.conversations.getCustomRetention

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 ./docs.sh?)
  • /docs-src-v2 (Documents, have you run ./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.

@gitwave gitwave bot added the untriaged label Sep 3, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 3, 2021 •
edited
Loading

Codecov Report

Merging #1109 (1665317) into main (01993c7) will decrease coverage by 0.53%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1109      +/-   ##
==========================================
- Coverage   86.23%   85.69%   -0.54%     
==========================================
  Files         110      110              
  Lines       10416    10443      +27     
==========================================
- Hits         8982     8949      -33     
- Misses       1434     1494      +60
Impacted Files Coverage Δ
slack_sdk/web/async_client.py 89.97% <100.00%> (-2.00%) ⬇️
slack_sdk/web/client.py 91.20% <100.00%> (-2.01%) ⬇️
slack_sdk/web/legacy_client.py 91.01% <100.00%> (-2.01%) ⬇️

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 01993c7...1665317. Read the comment docs.

@seratch seratch added this to the 3.11.0 milestone Sep 3, 2021
@seratch seratch added enhancement M-T: A feature request for new functionality Version: 3x web-client and removed untriaged labels Sep 3, 2021
@seratch seratch self-assigned this Sep 3, 2021
@ruberVulpes ruberVulpes marked this pull request as ready for review September 4, 2021 23:32
@ruberVulpes
Copy link
Copy Markdown
Contributor Author

ruberVulpes commented Sep 4, 2021

I based the integration tests off of /integration_tests/web/test_admin_conversations_restrictAccess.py. I'm not sure why the coverage seems to be lower.

seratch
seratch suggested changes Sep 6, 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.

Sorry, I forgot to mention this. Can you remove these exclusions?

python-slack-sdk/tests/slack_sdk_async/web/test_web_client_coverage.py

Lines 51 to 53 in 5f94019

"admin.conversations.getCustomRetention",
"admin.conversations.removeCustomRetention",
"admin.conversations.setCustomRetention",

@ruberVulpes ruberVulpes requested a review from seratch September 6, 2021 21:51
@seratch
Copy link
Copy Markdown
Contributor

seratch commented Sep 7, 2021

@ruberVulpes Thanks for updating the test quickly. The changes already look good to me but I'm still waiting for the change on my sandbox org to run the tests. Once I've verified if the integration tests work for me, I will merge this PR!

@seratch
Copy link
Copy Markdown
Contributor

seratch commented Sep 7, 2021

I've confirmed that the integration tests work for me too 👍

@seratch seratch merged commit 9f3240d into slackapi:main Sep 7, 2021
@seratch seratch mentioned this pull request Sep 7, 2021
Closed
9 tasks
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

@seratch seratch

Labels

enhancement M-T: A feature request for new functionality Version: 3x web-client

Projects

None yet

Milestone

3.11.0

Development

Successfully merging this pull request may close these issues.

2 participants

@ruberVulpes @seratch