Add bookmarks.* API support #1176
Conversation
filmaj
added
enhancement
Codecov Report
@@ Coverage Diff @@
## main #1176 +/- ##
==========================================
+ Coverage 86.63% 86.68% +0.04%
==========================================
Files 110 110
Lines 10793 10829 +36
==========================================
+ Hits 9351 9387 +36
Misses 1442 1442
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good. You generated the changes to legacy_client by running the codegen scripts, yes?
I left one small comment. Ultimately I think it's up to the api docs to specify whether this is a required field when type='link', and the docs can be improved, so I am also fine with leaving as is.
Yes! I was mostly using previous "add some new API" pull requests to this repo as a template, and saw that they included changes to legacy and async clients, but when I went to edit those I saw the big comment at the top of the file telling me to generate the changes off of |
| type: str, | ||
| emoji: Optional[str] = None, | ||
| entity_id: Optional[str] = None, | ||
| link: Optional[str] = None, # include when type is 'link' |
There was a problem hiding this comment.
This comment will be removed when the codegen command is executed next time. Can you add this change to client.py and run either python setup.py codegen or ./scripts/run_validation.sh?
There was a problem hiding this comment.
Will do! I am also working on an integration test (mostly so I can have some more hands-on experience with what this API does), so I will include this suggested change as well as the integration test in a moment, and ask for your review again 🙇
There was a problem hiding this comment.
Good call out @seratch I had meant to leave this in the the client.py file! 🙇
There was a problem hiding this comment.
FYI when I moved the comment to client.py and re-ran python3 setup.py codegen, the comment only shows up in legacy_client.py and not async_client.py 🤔
filmaj
force-pushed
the
bookmark-apis
branch
from
8368842
to
75d83d7
Compare
| @@ -0,0 +1,71 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
The test code itself is great! but I have a few suggestions:
- Let's remove unused imports
- Let's run
blackcode formatter
The following is an example that fixes all the above:
import logging
import os
import unittest
from integration_tests.env_variable_names import (
SLACK_SDK_TEST_BOT_TOKEN,
SLACK_SDK_TEST_WEB_TEST_CHANNEL_ID,
)
from slack_sdk.web import WebClient
class TestBookmarks(unittest.TestCase):
"""Runs integration tests with real Slack API testing the bookmarks.* APIs"""
def setUp(self):
if not hasattr(self, "logger"):
self.logger = logging.getLogger(__name__)
self.bot_token = os.environ[SLACK_SDK_TEST_BOT_TOKEN]
self.sync_client: WebClient = WebClient(token=self.bot_token)
self.channel_id = os.environ[SLACK_SDK_TEST_WEB_TEST_CHANNEL_ID]
def tearDown(self):
pass
def test_adding_listing_editing_removing_bookmark(self):
client = self.sync_client
# create a new bookmark
bookmark = client.bookmarks_add(
channel_id=self.channel_id,
title="slack!",
type="link",
link="https://slack.com",
)
self.assertIsNotNone(bookmark)
bookmark_id = bookmark["bookmark"]["id"]
# make sure we find the bookmark we just added
all_bookmarks = client.bookmarks_list(channel_id=self.channel_id)
self.assertIsNotNone(all_bookmarks)
self.assertIsNotNone(
next(
(b for b in all_bookmarks["bookmarks"] if b["id"] == bookmark_id), None
)
)
# edit the bookmark
bookmark = client.bookmarks_edit(
bookmark_id=bookmark_id,
channel_id=self.channel_id,
title="slack api!",
type="link",
link="https://api.slack.com",
)
self.assertIsNotNone(bookmark)
# make sure we find the edited bookmark we just added
all_bookmarks = client.bookmarks_list(channel_id=self.channel_id)
self.assertIsNotNone(all_bookmarks)
edited_bookmark = next(
(b for b in all_bookmarks["bookmarks"] if b["id"] == bookmark_id), None
)
self.assertIsNotNone(edited_bookmark)
self.assertEqual(edited_bookmark["title"], "slack api!")
# remove the bookmark
removed_bookmark = client.bookmarks_remove(
bookmark_id=bookmark_id, channel_id=self.channel_id
)
self.assertIsNotNone(removed_bookmark)
# make sure we cannot find the bookmark we just removed
all_bookmarks = client.bookmarks_list(channel_id=self.channel_id)
self.assertIsNotNone(all_bookmarks)
self.assertIsNone(
next((b for b in all_bookmarks if b["id"] == bookmark_id), None)
)There was a problem hiding this comment.
Oh interesting @seratch - I saw that some code formatters were run as part of the run_validation.sh script, so I assumed that was run against the test code as well. Looks like we run black against the tests/ directory but not against the integration_tests/ directory. Should we?
There was a problem hiding this comment.
Looks like we run black against the tests/ directory but not against the integration_tests/ directory. Should we?
Good point. I think that enabling black for the directory too would be a nice improvement
… vs. optional. Add integration tests for bookmarks API.
filmaj
force-pushed
the
bookmark-apis
branch
from
75d83d7
to
c572675
Compare
Summary
This PR adds support for the add/edit/list/remove bookmark APIs.
This is my first PR adding support for new APIs, so reviews are appreciated. Maybe I missed some stuff? I.e. maybe integration tests? Not sure, would very much appreciate your guidance @seratch 🙇
Category (place an
xin each of the[ ])/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
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.