Fix #1283 - Add conversations (slack connect) and admin.auth.policy API methods by srajiang · Pull Request #1284 · slackapi/node-slack-sdk

srajiang · 2021-07-12T06:42:51Z

Summary

This PR fixes #1283 with the addition of the following Slack Connect conversations.* + Admin admin.auth.policy.* methods:

conversations.acceptSharedInvite
conversations.approveSharedInvite
conversations.declineSharedInvite
conversations.listConnectInvites
conversations.inviteShared
admin.auth.policy.getEntities
admin.auth.policy.assignEntities
admin.auth.policy.removeEntities

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.

srajiang

Thanks in advance for taking a look!

packages/web-api/src/methods.ts

srajiang · 2021-07-12T06:58:40Z

packages/web-api/src/methods.ts

 }
 cursorPaginationEnabledMethods.add('conversations.list');
+export interface ConversationsListConnectInvitesArguments extends WebAPICallOptions, TokenOverridable {
+  count?: number;


This method accepts a count and a cursor option, and so it doesn't to conform to CursorPaginationEnabled or TraditionalPagingEnabled interface.

Oh... the 3rd pattern came here.

srajiang · 2021-07-12T07:00:32Z

packages/web-api/src/response/AdminAuthPolicyAssignEntitiesResponse.ts

@@ -0,0 +1,9 @@
+/* tslint:disable */


Generated by generate-web-api-types.sh

srajiang · 2021-07-12T07:01:25Z

packages/web-api/src/response/SearchAllResponse.ts

  attachments?:  MatchAttachment[];
  is_mpim?:      boolean;
  score?:        number;
+  files?:        File[];


Changes here aren't relevant to PR, but auto-generated by generate-web-api-types.sh
See also, SearchMessagesResponse.ts

Yes, this is fine. We've detected this pattern and updated the type def very recently.

seratch

I cannot leave comments on the Slack Connect APIs as I haven't verified how they work yet (I will do early next week!). The others already look good to me 👍

If you want to add integration tests with the prod Slack APIs, you can have some code here. https://github.com/slackapi/node-slack-sdk/tree/main/prod-server-integration-tests/test

For the admin.auth.policy.*, you can do the same with Python SDK: https://github.com/slackapi/python-slack-sdk/blob/main/integration_tests/web/test_admin_auth_policy.py

As for the Slack Connect APIs, running tests without human interactions may not be feasible (I'm not sure in detail yet, though).

packages/web-api/src/methods.ts

seratch · 2021-07-12T07:16:27Z

packages/web-api/src/methods.ts

 }
 cursorPaginationEnabledMethods.add('conversations.list');
+export interface ConversationsListConnectInvitesArguments extends WebAPICallOptions, TokenOverridable {
+  count?: number;


Oh... the 3rd pattern came here.

seratch · 2021-07-12T07:17:16Z

packages/web-api/src/response/SearchAllResponse.ts

  attachments?:  MatchAttachment[];
  is_mpim?:      boolean;
  score?:        number;
+  files?:        File[];


Yes, this is fine. We've detected this pattern and updated the type def very recently.

seratch · 2021-07-12T07:36:49Z

@srajiang There is a lint error. Can you fix it? https://github.com/slackapi/node-slack-sdk/pull/1284/checks?check_run_id=3044242462

> @slack/web-api@6.2.4 lint /home/runner/work/node-slack-sdk/node-slack-sdk/packages/web-api
> tslint --project .


ERROR: /home/runner/work/node-slack-sdk/node-slack-sdk/packages/web-api/src/methods.ts:777:1 - Exceeds maximum line length of 120

seratch

Having the integration tests for admin.auth.policy endpoints is a great addition 👍

seratch · 2021-07-13T04:07:18Z

prod-server-integration-tests/test/admin-web-api.js

+          entity_type: "USER",
+          policy_name: "email_password",
+        });
+        assert.equal(res.ok, true);


Having the following assertion is a bit easier to check the result than just knowing false

assert.isUndefined(res.error);

I have added these assertions!

@srajiang Thanks. Perhaps, you can remove assert.equal(res.ok, true); in L88 because the old assertion fails before the newly added one 😸

seratch · 2021-07-13T04:12:19Z

prod-server-integration-tests/test/admin-web-api.js

+        });
+        assert.equal(res.ok, true);
+      } catch (error) {
+        console.log(error);


I can imagine the reason why you went this way (accepting the possible errors due to the data state, right?). This is fine for now 👍 Ideally, we can make these to be more like a robust scenario test but we don't need to put so much efforts for the perfection here.

Yes, the try catch was helpful here in inspecting the error state (more than just the error reason), but since tests that should fail due to error show as passing, I think it's probably best to avoid false positives in our tests, so I have removed the try catch.

stevengill

Great work!

* Add Slack Connect API response types to #1284 (#1283) * Fix lint erorrs * Fix lint errors again

Add api methods

418b6a3

gitwave bot added the untriaged label Jul 12, 2021

srajiang added enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` semver:minor and removed untriaged labels Jul 12, 2021

srajiang added this to the web-api@6.3 milestone Jul 12, 2021

srajiang commented Jul 12, 2021

View reviewed changes

srajiang marked this pull request as ready for review July 12, 2021 07:13

srajiang requested review from misscoded and seratch July 12, 2021 07:13

srajiang self-assigned this Jul 12, 2021

srajiang requested a review from stevengill July 12, 2021 07:16

seratch reviewed Jul 12, 2021

View reviewed changes

Fix spacing errors

797c23f

srajiang added 2 commits July 12, 2021 17:38

Fix linter error

4db21b9

Add admin auth api tests

efba24f

seratch reviewed Jul 13, 2021

View reviewed changes

seratch mentioned this pull request Jul 13, 2021

Update the Web API response types (incl. token rotation fields) #1285

Merged

2 tasks

stevengill approved these changes Jul 13, 2021

View reviewed changes

Remove false positive

4cf4134

srajiang merged commit df61f14 into slackapi:main Jul 14, 2021

seratch mentioned this pull request Aug 16, 2021

Add Slack Connect API response types to #1284 (#1283) #1307

Merged

2 tasks

seratch added a commit that referenced this pull request Aug 17, 2021

Add Slack Connect API response types to #1284 (#1283) (#1307)

2c16c1e

* Add Slack Connect API response types to #1284 (#1283) * Fix lint erorrs * Fix lint errors again

Conversation

srajiang commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements (place an x in each [ ])

Uh oh!

srajiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch commented Jul 12, 2021

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevengill left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srajiang commented Jul 12, 2021 •

edited

Loading

Requirements (place an `x` in each `[ ]`)