-
Notifications
You must be signed in to change notification settings - Fork 0
Add file upload support #1
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,12 @@ | |
|
|
||
| const http = require('http'); | ||
| const https = require('https'); | ||
| const FormData = require('form-data'); | ||
| const messages = require('./messages'); | ||
| const querystring = require('querystring'); | ||
|
|
||
| const TEMP_UPLOAD_ID_TEXT = '----TEMPUPLOAD----'; | ||
|
|
||
| const cache = { | ||
| lines: {}, | ||
| line: function(workspace, channel, quiet = false) { | ||
|
|
@@ -217,30 +220,64 @@ async function cached(memo, key, method, parameter, argument, workspace, update | |
| return memo[key]; | ||
| } | ||
|
|
||
| function call(method, body, workspace) { | ||
| function authHeaders(workspace) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: More general name, as it might have other headers in the future
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name should be snake case |
||
| var token = cache.token(workspace); | ||
| if(!token) | ||
| token = workspace; | ||
| if(!token) | ||
| token = TOKEN_0; | ||
|
|
||
| return { | ||
| Authorization: 'Bearer ' + token, | ||
| } | ||
| } | ||
|
|
||
| var header = { | ||
| function call(method, body, workspace, content_type = 'application/json') { | ||
| var options = { | ||
| headers: { | ||
| Authorization: 'Bearer ' + token, | ||
| ...authHeaders(workspace), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change this to headers: authHeaders(workspace), (or whatever we call it) |
||
| }, | ||
| }; | ||
|
|
||
| var payload = ''; | ||
| var form = undefined; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're making it undefined, why initialize it at all? |
||
| if(body) { | ||
| header.method = 'POST'; | ||
| header.headers['Content-Type'] = 'application/json'; | ||
| payload = JSON.stringify(body); | ||
| options.method = 'POST'; | ||
| options.headers['Content-Type'] = content_type; | ||
| if(content_type == 'multipart/form-data') { | ||
| if(!Array.isArray(body) || body.length < 1 || !body[0].key) { | ||
| console.error(`Invalid body structure for ${content_type}`, body); | ||
| return; | ||
| } | ||
|
|
||
| form = new FormData(); | ||
| for(const part of body) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Why is this const? |
||
| form.append(part.key, part.value, { filename: part.filename }); | ||
| } | ||
| options.headers = { | ||
| ...options.headers, | ||
| ...form.getHeaders(), | ||
| }; | ||
| } | ||
| else if(content_type == 'application/json') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I've been omitting curly braces for one-line bodies because I'm a baddie |
||
| payload = JSON.stringify(body); | ||
| } | ||
| else { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Curly braces |
||
| payload = body; | ||
| } | ||
| } | ||
|
|
||
| var request = https.request('https://slack.com/api/' + method, header); | ||
| var response = new Promise(function(resolve) { | ||
| var request = https.request('https://slack.com/api/' + method, options); | ||
| if(form) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Curly braces |
||
| form.pipe(request); | ||
| } | ||
| var response = new Promise(function(resolve, reject) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I usually leave a blank line before a variable declaration following anything else |
||
| request.on('response', async function(res) { | ||
| resolve(JSON.parse(await stringify(res))); | ||
| }); | ||
| request.on('error', err => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh thank you! I would prefer this to be its own commit, since it's not part of the attachments work... |
||
| reject(err); | ||
| }); | ||
| }); | ||
| request.end(payload); | ||
| return response; | ||
|
|
@@ -267,6 +304,103 @@ async function collect_call(method, body, array, workspace) { | |
| return collected; | ||
| } | ||
|
|
||
| async function get_event_files_data(event, workspace) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have this accept a files array instead of an event |
||
| if(typeof event != 'object' || !Array.isArray(event.files)) { | ||
| console.error('File upload: invalid event object', event); | ||
| return []; | ||
| } | ||
|
|
||
| // TODO: figure out why this doesn't work | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop this. Rebase mistake? |
||
| //const workspace = await cache.workspace(event.team); | ||
|
|
||
| const files = await Promise.all(event.files.map(async file => { | ||
| try { | ||
| const file_data = await new Promise((resolve, reject) => { | ||
| https.get( | ||
| file.url_private, | ||
| { | ||
| headers: { | ||
| ...authHeaders(workspace), | ||
| }, | ||
| }, | ||
| res => { | ||
| let chunks = []; | ||
| res.on('data', data => { | ||
| chunks.push(data); | ||
| }); | ||
| res.on('end', () => { | ||
| resolve(Buffer.concat(chunks)); | ||
| }); | ||
| } | ||
| ).on('error', err => { | ||
| reject(err); | ||
| }); | ||
| }); | ||
|
|
||
| return { | ||
| ...file, | ||
| file_data | ||
| }; | ||
| } | ||
| catch(e) { | ||
| console.error('Error retrieving file from event: ', e, event); | ||
| } | ||
| })); | ||
| return files.filter(file => !!file); | ||
| } | ||
|
|
||
| async function upload_files(files_with_data, workspace, channel) { | ||
| if(!Array.isArray(files_with_data) || !workspace) { | ||
| console.error('File upload: invalid data passed to upload_files', files_with_data, workspace); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Log case looks inconsistent with convention |
||
| return []; | ||
| } | ||
|
|
||
| let files = await Promise.all(files_with_data.map(async file => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I haven't been using the => syntax: I always use e.g., async function()
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also haven't been using let, but this is probably something we should be changing anyway so let's keep that. |
||
| if(typeof file !== 'object' || !file.name || !Buffer.isBuffer(file.file_data)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I usually do this check as if(!file || !file.name || ...) |
||
| console.error('File upload: invalid file data in upload_files', file); | ||
| reject(); | ||
| } | ||
|
|
||
| try { | ||
| const res = await call( | ||
| 'files.upload', | ||
| [ | ||
| { key: 'filename', value: file.name }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This interface seems weird to me. What about a single object with keys and values, where any value that is a buffer is treated as a file? |
||
| { key: 'channels', value: channel }, | ||
| { key: 'file', value: file.file_data, filename: file.name }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than doing this, how about if whenever a key 'foo' corresponds to a value of type Buffer, we expect to find the filename in a key called 'fooname'? |
||
| { key: 'initial_comment', value: TEMP_UPLOAD_ID_TEXT }, | ||
| ], | ||
| workspace, | ||
| 'multipart/form-data' | ||
| ); | ||
|
|
||
| const channelId = res && res.file && res.file.channels && res.file.channels[0] ? res.file.channels[0] : undefined; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer this style:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Not my const style |
||
| if(channelId && res.file.shares && res.file.shares.public && res.file.shares.public[channelId] && res.file.shares.public[channelId]) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. res.file.shares.public[channelId] && res.file.shares.public[channelId] ? |
||
| const deleteRes = await call('chat.delete', { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: const style |
||
| channel: channelId, | ||
| ts: res.file.shares.public[channelId][0].ts, | ||
| }, workspace); | ||
|
|
||
| if(LOGGING) | ||
| console.log('file message delete response', deleteRes); | ||
| } | ||
| else { | ||
| console.error('could not find some data needed to delete temp. message', channelId, JSON.stringify(res.file)); | ||
| } | ||
|
|
||
| return res; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Brace stuff |
||
| catch(e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This belongs in a separate commit along with the rejection stuff |
||
| console.error('File upload: Error uploading file to destination', e, file); | ||
| } | ||
| })); | ||
|
|
||
| if(LOGGING) | ||
| console.log('file results', files); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Consistent log formatting |
||
|
|
||
| return files.filter(res => (res && res.ok)).map(res => res.file); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Closure style |
||
| } | ||
|
|
||
| function escaped(varname) { | ||
| return varname.replace(/-/g, '__hyphen__'); | ||
| } | ||
|
|
@@ -556,7 +690,9 @@ async function handle_command(payload) { | |
| } | ||
|
|
||
| async function handle_event(event) { | ||
| if(event.type == 'member_joined_channel') { | ||
| if(event.message && event.message.text && event.message.text.includes(TEMP_UPLOAD_ID_TEXT)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be exactly matching against TEMP_UPLOAD_ID_TEXT.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be combined with line 712 so it's clear what it's for. |
||
| return; | ||
| } else if(event.type == 'member_joined_channel') { | ||
| handle_join(event); | ||
| return; | ||
| } else if(event.type == 'member_left_channel') { | ||
|
|
@@ -570,11 +706,6 @@ async function handle_event(event) { | |
| + '\n_If you want the other channel to see, send an emoji message!_'); | ||
| } | ||
| return; | ||
| } else if(event.subtype == 'file_share') { | ||
| var workspace = await cache.workspace(cache.team(event.channel)); | ||
| warning(workspace, event.channel, event.user, | ||
| '*Warning:* File uploads are currently unsupported.' | ||
| + '\n_If you want the other channel to see, link to cloud storage instead!_'); | ||
| } else if(event.type != 'message') { | ||
| console.log('Unhandled type in event: ' + JSON.stringify(event)); | ||
| return; | ||
|
|
@@ -721,6 +852,23 @@ async function handle_event(event) { | |
| message.text, paired.workspace, users, event.channel); | ||
| } | ||
|
|
||
| if(event.files) { | ||
| const uploaded_files = await upload_files(await get_event_files_data(event, workspace), paired.workspace, paired.channel); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: const |
||
| message.attachments = uploaded_files | ||
| .filter(file => typeof file === 'object' && file.permalink) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove filter() because we can trust upload_files(). |
||
| .map(file => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Closure style |
||
| if(!file.mimetype || !file.mimetype.includes('image/')) { | ||
| return { | ||
| text: `<${file.permalink}|${file.name}>` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't appear to be working (at least for PDFs) |
||
| }; | ||
| } | ||
| return { | ||
| image_url: file.permalink, | ||
| text: file.name | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| var ack = await call('chat.postMessage', message, paired.workspace); | ||
| if(LOGGING) | ||
| console.log(ack); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| "start": "node index.js" | ||
| }, | ||
| "dependencies": { | ||
| "pg": "8.2.0" | ||
| "pg": "8.2.0", | ||
| "form-data": "3.0.0" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please alphabetize |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to change it to something like 'Loading attachments...' so users can understand it.