Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1310 Add admin.conversations.bulk{Archive|Delete|Move} API method support #1311

Merged
merged 3 commits into from
Dec 9, 2022

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Dec 8, 2022

Summary

This pull request resolves slackapi/java-slack-sdk#1087

Note: I did not run the integration tests locally

Fixes #1310

Category

  • 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

  • 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.

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #1311 (be22856) into main (0fedf4f) will decrease coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1311      +/-   ##
==========================================
- Coverage   85.79%   85.37%   -0.42%     
==========================================
  Files          84      111      +27     
  Lines        7861    11334    +3473     
==========================================
+ Hits         6744     9676    +2932     
- Misses       1117     1658     +541     
Impacted Files Coverage Δ
slack_sdk/web/async_client.py 82.37% <100.00%> (ø)
slack_sdk/web/client.py 83.67% <100.00%> (ø)
slack_sdk/web/legacy_client.py 83.41% <100.00%> (+0.14%) ⬆️
slack_sdk/socket_mode/builtin/internals.py 70.38% <0.00%> (-1.72%) ⬇️
slack_sdk/rtm/__init__.py 90.73% <0.00%> (ø)
slack_sdk/web/slack_response.py 89.85% <0.00%> (ø)
slack_sdk/models/__init__.py 100.00% <0.00%> (ø)
slack_sdk/webhook/internal_utils.py 82.60% <0.00%> (ø)
slack_sdk/web/async_internal_utils.py 82.52% <0.00%> (ø)
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@seratch
Copy link
Member

seratch commented Dec 9, 2022

The integration test does not work. I will update the code later on

@seratch
Copy link
Member

seratch commented Dec 9, 2022

The error:

___________________________________________________________________ TestWebClient.test_sync ____________________________________________________________________

self = <test_admin_conversations_bulk.TestWebClient testMethod=test_sync>

    def test_sync(self):
        client = self.sync_client

        conv_creation = client.admin_conversations_create(
            is_private=False,
            name=self.channel_name,
            team_id=self.team_id,
        )
        self.assertIsNotNone(conv_creation)
        created_channel_id = conv_creation.data["channel_id"]

        self.assertIsNotNone(
>           client.admin_conversations_bulkMove(
                channel_ids=[created_channel_id],
                target_team_id=self.team_id_2,
            )
        )

integration_tests/web/test_admin_conversations_bulk.py:53:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
slack_sdk/web/client.py:837: in admin_conversations_bulkMove
    return self.api_call("admin.conversations.bulkMove", params=kwargs)
slack_sdk/web/base_client.py:156: in api_call
    return self._sync_send(api_url=api_url, req_args=req_args)
slack_sdk/web/base_client.py:187: in _sync_send
    return self._urllib_api_call(
slack_sdk/web/base_client.py:309: in _urllib_api_call
    return SlackResponse(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <slack_sdk.web.slack_response.SlackResponse object at 0x1064b2340>

    def validate(self):
        """Check if the response from Slack was successful.

        Returns:
            (SlackResponse)
                This method returns it's own object. e.g. 'self'

        Raises:
            SlackApiError: The request to the Slack API failed.
        """
        if self.status_code == 200 and self.data and (isinstance(self.data, bytes) or self.data.get("ok", False)):
            return self
        msg = f"The request to the Slack API failed. (url: {self.api_url})"
>       raise e.SlackApiError(message=msg, response=self)
E       slack_sdk.errors.SlackApiError: The request to the Slack API failed. (url: https://www.slack.com/api/admin.conversations.bulkMove)
E       The server responded with: {'ok': False, 'error': 'action_already_in_progress'}

slack_sdk/web/slack_response.py:199: SlackApiError

As the Java code does, test code has to wait until "action_already_in_progress" error dispappears.

@seratch
Copy link
Member

seratch commented Dec 9, 2022

I was going to contribute to the test code but I have to work on other tasks today. @WilliamBergamin can you fix it when you have a chance? No rush at all

Copy link
Member

@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.

Code changes, unit tests, document changes are all great. The e2e test does not work

@WilliamBergamin
Copy link
Contributor Author

WilliamBergamin commented Dec 9, 2022

@seratch I partially set up my local with the requirements of the integration tests, I've modified the tests and they pass on my end now

Copy link
Member

@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.

Thanks for updating the test. I've confirmed that the test works for me too. Looks great to me

@seratch seratch merged commit 9217bcb into slackapi:main Dec 9, 2022
@seratch seratch changed the title Fix #1087 Add admin.conversations.bulk{Archive|Delete|Move} API method support Fix #1310 Add admin.conversations.bulk{Archive|Delete|Move} API method support Dec 9, 2022
@WilliamBergamin WilliamBergamin deleted the issue-1087-bulk-apis branch November 22, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants