Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 162 additions & 14 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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----';
Copy link
Contributor

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.


const cache = {
lines: {},
line: function(workspace, channel, quiet = false) {
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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),
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand All @@ -267,6 +304,103 @@ async function collect_call(method, body, array, workspace) {
return collected;
}

async function get_event_files_data(event, workspace) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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()

Copy link
Contributor

Choose a reason for hiding this comment

The 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 },
Copy link
Contributor

Choose a reason for hiding this comment

The 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 },
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this style:
let channelId;
if(res && res.file && ...)
channel)d = res.file.channels[0];

Copy link
Contributor

Choose a reason for hiding this comment

The 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]) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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', {
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Brace stuff

catch(e) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Closure style

}

function escaped(varname) {
return varname.replace(/-/g, '__hyphen__');
}
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be exactly matching against TEMP_UPLOAD_ID_TEXT.

Copy link
Contributor

Choose a reason for hiding this comment

The 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') {
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove filter() because we can trust upload_files().

.map(file => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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}>`
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"start": "node index.js"
},
"dependencies": {
"pg": "8.2.0"
"pg": "8.2.0",
"form-data": "3.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please alphabetize

}
}