Skip to content
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

Added FILE_SHARE subtype for incoming message, new event added #86

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

butnotstupid
Copy link

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

@rampatra
Copy link
Owner

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?

@butnotstupid
Copy link
Author

butnotstupid commented Mar 4, 2018

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.

…-share-message

# Conflicts:
#	jbot/src/main/java/me/ramswaroop/jbot/core/slack/Bot.java
#	jbot/src/test/java/me/ramswaroop/jbot/core/slack/EventTypeTest.java
@butnotstupid
Copy link
Author

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?

@rampatra
Copy link
Owner

Hey,

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

@butnotstupid
Copy link
Author

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

@rampatra
Copy link
Owner

rampatra commented Apr 30, 2018

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.

@IgorBelyayev
Copy link

+1

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.

None yet

3 participants