jbot icon indicating copy to clipboard operation
jbot copied to clipboard

Added FILE_SHARE subtype for incoming message, new event added

Open butnotstupid opened this issue 7 years ago • 7 comments

Hi there,

Been using JBot for recieving files sent to slack channel. Faced an issue with FILE_SHARED message, the File object I received was containing nulls for most of the fields.

Looked it up here: https://api.slack.com/tutorials/working-with-files. Turned out that "file_shared" event isn't what I needed to listen to and event "message" with subtype "file_share" is.

This way you'll get pretty populated File object (I needed urls for download) right away in the event. Otherwise all you could get is File ID and then have to make an additional request to Slack API for the url.

So all I've done was adding FILE_SHARE_MESSAGE event type as you can see to catch it properly. Hope it makes sense.

Thanks for the JBot, Fedor

butnotstupid avatar Jan 28 '18 16:01 butnotstupid

I will merge it in a day or two but by any chance is it possible for you to write a unit test for your changes?

rampatra avatar Feb 26 '18 22:02 rampatra

Hi Ram, thanks a lot, looking forward to it!

Actually I don't know if it's possible to unit test my change -- I've added a new event type, so as I understand this should rather be tested by some integration test.

And in this case it would involve quite a few more changes, because for now there is just one simple unit test and no integration test harness implemented.

butnotstupid avatar Mar 04 '18 14:03 butnotstupid

Hi Ram,

I've found some conflicts, so I pulled the changes from origin/master and resolved them.

Then I've found a failed unit test which helped me to fix an empty subtype handilng. And also with its help I've managed to cover my new feature with the test.

So, it seems like all set now. What do you think, can we merege the PR?

butnotstupid avatar Mar 31 '18 12:03 butnotstupid

Hey,

Thanks for PR (and the unit test). Will review it and merge shortly.

rampatra avatar Mar 31 '18 19:03 rampatra

Hi Ram,

Sorry to bother you, but is there anything I can do to help merge the PR?

We're using this new functionality in a small pet project, and merging to master and build new release version would really ease the building process as I could just update JBot version then.

Thanks a lot, Fedor

butnotstupid avatar Apr 29 '18 14:04 butnotstupid

Hi Fedor, I haven't merged this PR because there are many subtypes under message event type. So, I was thinking of including everything as events in EventType.java so that you could listen to them like @Controller(events = EventType.MESSAGE_FILE_SHARE) and @Controller(events = EventType.MESSAGE_BOT_MESSAGE) etc. I may do this change shortly but if you can manage to raise a PR for the same, it will be great and if not, you can listen for MESSAGE event type and check for the subtype inside the controller function.

Also, we need to modify EventType.java to contain the string value which Slack has (to avoid overlapping between events and subevents) so that we don't have to declare all the strings in Bot.java like you've done in this PR.

rampatra avatar Apr 30 '18 19:04 rampatra

+1

IgorBelyayev avatar Feb 01 '19 22:02 IgorBelyayev