Summary:

This module allows Drupal to receive and execute commands sent from Slack via their Outgoing Webhooks integration. Developers can use this module as a launching point for things like executing Drush commands and printing the output into a Slack channel, or for creating their own Drupal-powered SlackBots.

Slack will send POST data to a menu callback in Drupal which this module enables at /slack_receive. If the post contains the correct Slack Token, Slack Receive will then execute any functions that are implemented via hook_slack_receive_api.

Note: this module is a an API module and doesn't do anything on its own. The included slack_receive_example module contains working examples of functionality that are possible via Slack Receive.

Difference from existing Slack module:

The existing Slack module is meant for sending information from Drupal to Slack - this module does the opposite. I think keeping the modules separate makes sense as site maintainers may want one ability but not the other. Also, since the integration endpoints (and Tokens used for access) are totally different - it's effectively a completely different API on both ends.

Project Link: Slack Receive

Git clone link: git clone --branch 7.x-1.x https://git.drupal.org/sandbox/pninny2001/2677166.git

Comments

msherron created an issue. See original summary.

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxpninny20012677166git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

msherron’s picture

msherron commented
Issue summary: View changes
msherron’s picture

msherron commented

I've pushed some updates based on the PAReview bot. Thanks!

msherron’s picture

msherron commented
Status: Needs work » Needs review
msherron’s picture

msherron commented
Issue summary: View changes
rgristroph’s picture

rgristroph commented

Minor issues only --

1) There is a misspelling of "receive" on line 5 of the README.txt, also a typo on line 5 of slack_receive_admin.inc
2) Should there be a slack_receive.api.inc file ? It seems that the slack_example does a better job of documenting that, and I didn't see anything about api.inc files in the code standards. This might be a non-issue.

I also installed the module locally, did some basic testing.

pankajsachdeva’s picture

pankajsachdeva commented

Automated Review

Found some issues, you can see on this link : http://pareview.sh/pareview/httpgitdrupalorgsandboxpninny20012677166git

Manual Review

Individual user account
[Yes: Follows] the guidelines for individual user accounts.
No duplication
[Yes: Does not cause] module duplication and/or fragmentation.
Master Branch
[Yes: Follows] the guidelines for master branch.
Licensing
[Yes: Follows] the licensing requirements.
3rd party assets/code
[Yes: Follows] the guidelines for 3rd party assets/code.
README.txt/README.md
[Yes: Follows] the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
[Yes: Follows] the guidelines for project length and complexity.
Secure code
[Yes: Meets the security requirements]
Coding style & Drupal API usage
No blocker issue found.
I am not checking the functionality of the module because I don't have Slack account. I just checked the code.

This review uses the Project Application Review Template.

msherron’s picture

msherron commented
Issue summary: View changes
msherron’s picture

msherron commented

Docs update, added slack_receive.api.inc, and slack_receive_help per feedback.

steven.wichers’s picture

steven.wichers commented

Some quick things:

Example module is missing t() around some translatable strings.

Main module menu item implementation should probably leverage 'file path' instead of putting the path in the file name.

Receive callback should implement a different delivery callback to ensure proper headers and site behavior. See ajax_deliver https://api.drupal.org/api/drupal/includes!ajax.inc/function/ajax_deliver/7 for an example. I believe right now if there is an error then your receive callback will render a Drupal error page.

msherron’s picture

msherron commented

Added some t() and better file path handling per feedback. drupal_json_output() sets headers like you described and just to be sure, I've tested to make sure errors are printed normally without bootstrapping a whole page. Thanks!

pankajsachdeva’s picture

pankajsachdeva commented

Hi @msherron,

I am getting an error message when I am accessing front page of my website:

Notice: Undefined index: token in slack_receive() (line 45 of /Applications/MAMP/htdocs/fresh-drupal/sites/all/modules/2677166/slack_receive.module).

pankajsachdeva’s picture

pankajsachdeva commented
Status: Needs review » Needs work
msherron’s picture

msherron commented

I added a check to test if the token is empty. I also made a change to force JSON headers from within the delivery callback before the hook is created.

msherron’s picture

msherron commented
Status: Needs work » Needs review
steven.wichers’s picture

steven.wichers commented
Status: Needs review » Reviewed & tested by the community
ayesh’s picture

ayesh commented

Hi Michael,
I reviewed your module well, and I must say almost everything looks quite good! I really hope this would be a good example for others to get a project reviewed quickly.

I also agree with Steven marking this RTBC (#17). However, there are few points I have to mention. Not blockers IMO.

- It is not clear how Slack responds to different types of HTTP response codes. But this module always sends a 200 code, which can make Slack assume everything went well. Since you override the delivery callback of the router for the incoming hook, it becomes the new delivery callbacks responsibility to properly send a valid and appropriate response code. The site offline case could send a 503, invalid token could use a 403, and so on.

- In slack_receive() I don't think copying the $_POST data to $data is necessary. It requires twice the memory as the payload.

- A flood protection to block the IP address after a certain number of wrong token would be nice.

msherron’s picture

msherron commented

Hi @Ayesh,

I implemented some of your and @steven.wichers suggestions, and if you have some time, I'd love for you to give some feedback on the changes:

  1. I added some response code handling, albeit hesitantly. The issue is that Slack will only display a response within the Slack app if its status is a 200 - it discards anything else. So while I agree that setting the right status codes is more 'correct', this might be more confusing for the average site builder to debug without using something like Postman.
  2. Right on about copying $_post. I switched it to a reference because I hate typing/reading '$_POST'.
  3. I added flood protection if there are more than 60 requests /min, but I did not for invalid API keys. I took a look around the community and when flood protection is used for invalid passwords, they either block a form or throw a 403 - since we're already returning a super-fast 403 on an invalid key, the flood lookup feels superfluous.

Thanks for the help!

ayesh’s picture

ayesh commented
Status: Reviewed & tested by the community » Fixed

Hi Michael,
The 200 HTTP status thing I mentioned merely for the semantics of similar RESTful services. If Slack is working differently, by all means, please feel free to adjust it anyway you'd like.

Since this was in RTBC for 2 whole months and no objections...
Thanks for your contribution, Michael!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

msherron’s picture

msherron commented

Hi @Ayesh,

Thanks for the role! I really appreciated the feedback.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.