microsoft / BotFramework-Hubot

Hubot adapter for botframework
MIT License
111 stars 40 forks source link

Optional authorization and cards for Hubot used with MS Teams #29

Closed MSMeMend closed 6 years ago

MSMeMend commented 6 years ago

Starting the PR for this now, but I'm still in the process of writing unit tests, so those will be added as I finish them. I was also having some issues with coffee-coverage, but my first priority is to finish unit tests for all of the files. I kept the hubot dependency the same in the package.json because I thought it made more sense for the dependency change to be separate from the other changes this PR makes.

msftclas commented 6 years ago

CLA assistant check
All CLA requirements met.

MSMeMend commented 6 years ago

@jayongg

coveralls commented 6 years ago

Coverage Status

Coverage increased (+18.007%) to 91.141% when pulling 2dc57e15d36cb9ee92906f1886eb31c02b1ccaf0 on MSMeMend:master into 8d0cf959ff651f49cb4454c26cae968bd092f3a0 on Microsoft:master.

MSMeMend commented 6 years ago

Almost done with tests-- not yet though, even though it says all are passing

maxpert commented 6 years ago

I would not recommend merging these changes at all. I think the first PR should be around adapter.coffee adding support for promises, so that anyone doing anything specific on network or authorization can do that and resolve/reject promise. Adapter should proceed with success and failure cases invoking send/raising error based upon promise.

That approach as two upsides:

I would also suggest moving additional files under a directory (msteams probably) that contains card specific structure.

MSMeMend commented 6 years ago

I agree to moving the additional cards files to another directory, my only concern was that adaptive cards are meant to eventually work across different Bot Framework channels, though they're not at that point yet (status here). That being said, since there's only cards for Teams in this change, I can move the extra files to an msteams sub-directory if that works better.

MSMeMend commented 6 years ago

I've created issues #32, #33, #34, and #35 for the refactoring of the adapter including into JavaScript/TypeScript. These changes will act as blockers for the next full release of the adapter.

In terms of this PR, because code coverage has increased (91% as of the latest commit) and the existing functionality will not be broken, we're planning to release these changes as an alpha version; I've updated the package.json to version 0.12.0-alpha1 to reflect this.