Page MenuHomePhabricator

Repair Flowdock adapter for ChatBot
ClosedPublic

Authored by mholden on Aug 6 2014, 9:26 PM.

Details

Summary

Restores functionality for Flowdock->Chatbot adapter.

Most likely the result of API changes in the year since the original patch was contributed,
the flowdock adapter no longer worked.

This makes a few tweaks to both the base streaming adapter class and the flowdock adpater. I took care to not disturb the functionality of the campfire adapter, but I don't have any way to test it.

Test Plan

I am new here and I have no idea what to write other than sarcastic things but I'll most like amend this after review.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mholden updated this revision to Diff 24458.Aug 6 2014, 9:26 PM
mholden retitled this revision from to Repair Flowdock adapter for ChatBot.
mholden updated this object.
mholden edited the test plan for this revision. (Show Details)
mholden added a reviewer: epriestley.
epriestley accepted this revision.Aug 6 2014, 9:29 PM
epriestley edited edge metadata.

whatcouldgowrong

This revision is now accepted and ready to land.Aug 6 2014, 9:29 PM
epriestley closed this revision.Aug 6 2014, 9:30 PM
epriestley updated this revision to Diff 24459.

Closed by commit rP5b4fb3b155f1 (authored by @mholden, committed by @epriestley).

btrahan added a subscriber: btrahan.Aug 6 2014, 9:39 PM

re test plan - the idea is to explain how you tested it. For this diff, I think you'd say something like

  • configured a new flowbot setup, sent some test messages and it worked!
  • was unable to test campfire

...and if you really want to be a superstar, you could explain how you configured the flowbot setup, what test messages you sent exactly, etc.

The test plan becomes more useful if the product can be interacted with several ways, as it clearly delineates how it was tested and how it was not tested. Reviewers can then suggest additional test coverage they want to see. The test plan is also often helpful for reviewers who have no idea what the feature does - the test plan should kind of delineate how the new code was tested and thus what it actually does.

Writing the test plan is also often helpful as you may discover things you didn't test but really should now that you have to explain what you did. I personally find that it also gives me the fortitude to re-do all the testing after I make a small change; you really want your own test plan to pass.

@btrahan -- thanks so much. I didn't think this would get insta-merged; Should I go back and update the revision details in the web UI, even though the commit message will forever contain my derpishness?

src/infrastructure/daemon/bot/adapter/PhabricatorBotBaseStreamingProtocolAdapter.php
42–46

This introduction of the $ch variable just seemed like a good opportunity for code cleanup.

nah, no need to update. Thanks for the contribution btw!

My pleasure.

So I guess my resume will now say "Core Contributor | phabricator"

;-)