Add files.uploadv2 support (fixing #1541) #1544

srajiang · 2022-10-17T22:12:26Z

Summary

Adds files.uploadV2 support to node-slack-sdk.
Please refer to slackapi/python-slack-sdk#1272 for more context.

Fixes #1541

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.

seratch

Great work! Here are my quick review comments.

.github/workflows/ci-build.yml

packages/web-api/src/WebClient.spec.js

packages/web-api/src/WebClient.ts

seratch · 2022-10-18T00:55:58Z

packages/web-api/src/WebClient.ts

+    fileUploadsURLRes.forEach((res, idx) => {
+      const { status } = res;
+      if (status === 'rejected') {
+        this.logger.error(`Error initiating upload for ${fileUploads[idx].filename}`);


For all the patterns that you print error-level logs like this, I think that we can immediately throw an exception as we don't want to proceed with the following code (say, if you failed uploading any of the files, posting a message with some of the files is not an expected behavior, right?).

@seratch - Hmm... I think that made an assumption that each file_upload was independent, and so my initial instinct was that users would find this useful, because having all 10 uploads fail because one upload is incorrect seemed annoying. But then I do see that this pattern is supported in your implementation:

# New way with multiple files! response = client.files_upload_v2( file_uploads=[ { "file": "./logo.png", "title": "New company logo", }, { "content": "Minutes ....", "filename": "team-meeting-minutes-2022-03-01.md", "title": "Team meeting minutes (2022-03-01)", }, ], channel="C12345", initial_comment="Here is the latest version of our new company logo :wave:", )

Which sounds like the scenario where you'd want the entire upload to fail IF there are multiple uploads associated with a single initial_comment / message. Currently my changes don't account for this.

@srajiang I think that developers would like to know any of the files are not properly uploaded because it's a partial failure. Also, I haven't checked this yet but when any of the file IDs that are passed to the complete API call (= sharing the files) fails, the sharing process may not work as you expect.

Okay - it's true that it's simpler to fail the moment there's a single failure.

I do still need to add some logic to support a single initial_comment / message + multiple file uploads at the same time so I will work on those both tomorrow.

@seratch Now we are immediately throwing an exception when there's a failure in any step, and have added changes to support the multiple file uploads pattern above.

seratch · 2022-10-18T00:57:47Z

packages/web-api/src/WebClient.ts

+   * @param fileUploads
+   * @returns
+   */
+  private async postCompletedFileUploads(fileUploads: FileUploadV2Entry[]):


Can you add files.info API call for each file (for better compatibility with v1)? The response from complete API includes only file ID an its title. With that, we still have some issues like slackapi/python-slack-sdk#1277. That being said, I still believe that attaching full metadata should be less surprising to developers.

@seratch - The separate files:read scope is required to make files.info call. Bolt can't assume that this is something that the app has configured, so would it make sense for the developer to handle this in this case?

I've added a getFileInfo method for now, but can remove it if we decide not to make the additional files.info call. If we decide to keep that functionality to match v1 (as I do agree it would be nice to have), I supposed I could also handle the exception thrown and if due to missing scope I suppose I could log that a scope is needed. Seems a little unusual to be doing that though.

@srajiang Indeed, the necessity of files:read too is another difference from the v1 method (mentioned in this thread actually - slackapi/python-slack-sdk#1165 (comment) ) but I believe that it's safe enough to assume an app handling files tends to have both read and write scopes. I would like to keep having it.

If we receive requests to customize the behavior in the future, we can consider adding a flag to disable the last files.info API calls. Perhaps, the flag can be full_file_into_required: boolean (default: true) or something like that.

packages/web-api/src/errors.ts

packages/web-api/src/file-upload.ts

srajiang

Left some initial comments for review.

.github/workflows/ci-build.yml

srajiang · 2022-10-18T01:02:53Z

packages/web-api/src/WebClient.spec.js

@@ -319,7 +320,21 @@ describe('WebClient', function () {
          const warnClient = new WebClient(token, { logLevel: LogLevel.WARN, logger });
          return warnClient.apiCall(method, args)
            .then(() => {
-              assert.isTrue(logger.warn.callCount === 4);
+              // assume no warning about thread_ts has been sent


This test is unrelated related to the added features for files.uploadV2 support. It was failing when I added additional warnings re: ☝️. I realized that the test wasn't really specifically validating that the thread_ts warning was being logged, just counting the total number of overall warning loggings - this change should be a small improvement.

srajiang · 2022-10-18T01:03:09Z

packages/web-api/src/WebClient.spec.js

@@ -341,10 +356,46 @@ describe('WebClient', function () {
          const warnClient = new WebClient(token, { logLevel: LogLevel.WARN, logger });
          return warnClient.apiCall(method, args)
            .then(() => {
-              assert.isTrue(logger.warn.calledThrice);
+              logger.warn.getCalls().forEach((call) => { 


Same comment as ☝️

packages/web-api/src/WebClient.spec.js

srajiang · 2022-10-19T05:01:49Z

@seratch - Thanks for the initial comments! I've gone ahead and made updates based on your suggestions.

seratch

Thanks for the great work. It looks almost done! I don't think we should support sharing in multiple channels in v2 as it may not be supported by design (this means the behavior may be changed in the future).

seratch · 2022-10-19T09:05:09Z

packages/web-api/src/WebClient.ts

+   * @param fileUploads
+   * @returns
+   */
+  private async postCompletedFileUploads(fileUploads: FileUploadV2Entry[]):


@srajiang Indeed, the necessity of files:read too is another difference from the v1 method (mentioned in this thread actually - slackapi/python-slack-sdk#1165 (comment) ) but I believe that it's safe enough to assume an app handling files tends to have both read and write scopes. I would like to keep having it.

If we receive requests to customize the behavior in the future, we can consider adding a flag to disable the last files.info API calls. Perhaps, the flag can be full_file_into_required: boolean (default: true) or something like that.

packages/web-api/src/WebClient.ts

packages/web-api/src/file-upload.ts

seratch

Thank you! Everything looks great now

srajiang marked this pull request as ready for review October 18, 2022 00:47

seratch reviewed Oct 18, 2022

View reviewed changes

seratch added this to the web-api@6.8.0 milestone Oct 18, 2022

seratch assigned srajiang Oct 18, 2022

seratch changed the title ~~Add files.uploadv2 support~~ Add files.uploadv2 support (fixing #1541) Oct 18, 2022

srajiang commented Oct 18, 2022

View reviewed changes

srajiang added enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` labels Oct 18, 2022

srajiang added 6 commits October 17, 2022 19:16

Add files.uploadv2 support

074244f

Improve existing warnings tests

d255fa4

Remove unnecessary commented out

796bba4

Add test for warning behavior when files.upload (legacy) is used

ab1bb49

Remove comment

8947a9d

default title to filename if not provided

a0ceece

srajiang force-pushed the sjiang-add-files-upload-v2 branch from 4ad5ddb to a0ceece Compare October 18, 2022 02:16

srajiang added 2 commits October 18, 2022 20:23

Progress

e189ece

progress

deaf4d1

srajiang requested a review from seratch October 19, 2022 05:01

srajiang added 2 commits October 18, 2022 22:04

Cleanup

9ad6c5c

Add jsdoc for getAllFileUploadsToComplete

7adb1b1

seratch requested changes Oct 19, 2022

View reviewed changes

srajiang added 4 commits October 20, 2022 09:42

Minor tweaks

9a17d21

Remove multiple files upload to different channels support

8de84ac

Add files info request optional flag

5565edb

Update to

90beaad

srajiang requested a review from seratch October 25, 2022 00:57

seratch approved these changes Oct 25, 2022

View reviewed changes

srajiang merged commit 959bd40 into main Oct 25, 2022

seratch deleted the sjiang-add-files-upload-v2 branch October 25, 2022 03:04

seratch mentioned this pull request Nov 8, 2022

Add requestFileInfo param to MethodsClient#filesUploadV2 method slackapi/java-slack-sdk#1081

Merged

6 tasks

juliocbcotta mentioned this pull request Jul 6, 2023

file upload returns 408 slackapi/java-slack-sdk#1178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add files.uploadv2 support (fixing #1541) #1544

Add files.uploadv2 support (fixing #1541) #1544

srajiang commented Oct 17, 2022 •

edited by seratch

seratch left a comment

seratch Oct 18, 2022

srajiang Oct 18, 2022

seratch Oct 18, 2022

srajiang Oct 18, 2022

srajiang Oct 19, 2022

seratch Oct 18, 2022

srajiang Oct 19, 2022

srajiang Oct 19, 2022 •

edited

seratch Oct 19, 2022

srajiang left a comment

srajiang Oct 18, 2022

srajiang Oct 18, 2022

srajiang commented Oct 19, 2022

seratch left a comment

seratch Oct 19, 2022

seratch left a comment

Add files.uploadv2 support (fixing #1541) #1544

Add files.uploadv2 support (fixing #1541) #1544

Conversation

srajiang commented Oct 17, 2022 • edited by seratch

Summary

Requirements (place an x in each [ ])

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang commented Oct 19, 2022

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

srajiang commented Oct 17, 2022 •

edited by seratch

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

srajiang Oct 19, 2022 •

edited