Skip to content

Report file uploads to IRC - #193

Merged
ekmartin merged 2 commits into
ekmartin:masterfrom
Aketzu:master
May 1, 2017
Merged

ekmartin merged 2 commits into
ekmartin:masterfrom
Aketzu:master

Conversation

@Aketzu

@Aketzu Aketzu commented Apr 18, 2017

Copy link
Copy Markdown

This handles file_share message subtype which is sent when e.g. screenshot is pasted to Slack channel. It shows both Slack-internal link and public link. For the public link to work Slack config "Enable public file URL creation" must be enabled and file uploader must also click "Create external link" on the file.

Looks like this in IRC channel

20:58 < slack> <aketzu> File uploaded 
             https://team.slack.com/files/aketzu/F60UD9R2L/pasted_image_at_2017_04_18_08_57_pm.png / 
             https://slack-files.com/T0LK1LT6W-F60UD9R2L-8fb1c182e4 - comment here

Fixes #22

@coveralls

coveralls commented Apr 18, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.6%) to 97.861% when pulling 2d8ee52 on Aketzu:master into 28dd48b on ekmartin:master.

@ekmartin ekmartin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks! Would you mind adding a test or two as well? Could probably look quite similar to this: https://github.com/ekmartin/slack-irc/blob/master/test/bot.test.js#L241-L252

Comment thread lib/bot.js
} else if (!message.subtype) {
text = `<${user.name}> ${text}`;
} else if (message.subtype === 'file_share') {
text = `<${user.name}> File uploaded ${message.file.permalink} / ${message.file.permalink_public}`;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Isn't showing one of these enough? I.e. message.file.permalink_public || message.file.permalink.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

permalink_public is always generated even if file is not shared. So it's rather hard to guess which would be better to show.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

permalink_public doesn't seem to work for logged in users before it is "created". On external link creation Slack seems to send file_change event but not file_public event. Additionally file_change has just file id without any other details and slack.rtm.dataStore doesn't seem to have method for looking up file by ID. So this would need some own temporary storage for storing file id + url, then on file_change event poll URL whether it works and finally generate another line to IRC if it succeeds.

So I'd guess showing both links is somewhat reasonable.

I also added tests.

@coveralls

coveralls commented Apr 18, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.009%) to 99.465% when pulling 7440faf on Aketzu:master into 28dd48b on ekmartin:master.

@ekmartin
ekmartin merged commit 2cd3e6e into ekmartin:master May 1, 2017
@ekmartin

ekmartin commented May 1, 2017

Copy link
Copy Markdown
Owner

Sorry about the slow response. This is now published in version 3.10.0 - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants