search

ocf / slackbridge

Bridge between IRC and Slack running on Kubernetes
Other
12 stars 10 forks source link

Don't crash on video uploads (closes #74) #79

Closed jvperrin closed 5 years ago

jvperrin commented 5 years ago

This should stop crashes on video uploads by checking if the thumb_1024 attribute exists before trying to access it.

(finally pushed the fix I mentioned back in January lol: https://moffle.fuqu.jp/ocf/%23rebuild/20190112#L444)

nattofriends commented 5 years ago

HEAD all reported thumbs and choose the largest reported content length

Or parse the string, either way.

On Sat, Jul 6, 2019 at 11:42 AM Bernard Zhao notifications@github.com wrote:

@BernardZhao approved this pull request.

Does thumb_1024 cover cases for all video? After looking at the API docs https://api.slack.com/types/file, isn't this property dependent on the quality of the video?

Depending on the original file's size, you may even find a thumb_480, thumb_720, thumb_960, or thumb_1024 property.

As long as this catches most video uploaded by phones though, this should do the trick. LGTM

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ocf/slackbridge/pull/79?email_source=notifications&email_token=AAFTUFAIFDBD6ZJHK2IPMWTP6DRS5A5CNFSM4H6SZDG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5U6NJY#pullrequestreview-258598567, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFTUFDD3BSPM7QW7NG3H3TP6DRS5ANCNFSM4H6SZDGQ .

jvperrin commented 5 years ago

Yeah it's probably better to have some code here to pick the highest resolution image available, but my goal was to just prevent crashes since we hardcoded using this attribute a couple times anyway. Mirroring videos would be possible at some point, but it currently wouldn't work and if the video is too big we couldn't upload it to fluffy anyway.