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

Integration with Microsoft Bot Framework #381

Merged
merged 11 commits into from
Nov 13, 2016

Conversation

vkosuri
Copy link
Collaborator

@vkosuri vkosuri commented Nov 4, 2016

Closes #290

class Microsoft(OutputAdapter):
"""
An output adapter that allows a ChatterBot instance to send
responses to a HipChat room.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy paste error

@gunthercox
Copy link
Owner

gunthercox commented Nov 5, 2016

@vkosuri Looks good, is this ready to be reviewed?

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 5, 2016

Thanks , I am working on documentation part also, i will push some more
comits. by today.

On 05-Nov-2016 6:40 PM, "Gunther Cox" [email protected] wrote:

@vkosuri https://github.com/vkosuri Looks good, is this ready to be
merged?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#381 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANCAAV2HF-k74IE5x4xmWKfjLYZ59ESaks5q7IA5gaJpZM4KpznB
.

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 5, 2016

@gunthercox i haven't added tests for this PR, i will add soon, do you have any comments/suggestions on this PR?

@gunthercox
Copy link
Owner

@vkosuri From what I can see, this looks fantastic.

In the examples, is the value for directline_access_token_or_secret an actual access token?

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 6, 2016 via email

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 7, 2016

@gunthercox any comments/suggestions?

@gunthercox
Copy link
Owner

Comments & feedback

  • Overall, the code looks fantastic.
  • I'm very glad to see that logging statements were included.
  • It might be useful to include a simple example program in the examples directory. This would also be helpful for manual testing since there aren't any unit tests yet.

I would be happy to help write tests for this, let me know.

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 9, 2016

  • Input adapter
  • Output adapter
  • Tests
  • Documentation
  • Example Program
  • Logging

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 11, 2016

@gunthercox any comments/suggestions?

@gunthercox
Copy link
Owner

How did you get the value to use for the conversation_id? I'm trying to set up the example to run locally, do I need to manually authenticate with the API and start a new conversation to get a conversation_id?

@vkosuri
Copy link
Collaborator Author

vkosuri commented Nov 12, 2016

You eill get it from here

Request URL
https://directline.botframework.com/api/conversations
Response Body
{
  "conversationId": "ENwpIpwOny6",
  "token": "xtFDtPemROU.dAA.RQBOAHcAcABJAHAAdwBPAG4AeQA2AA.IEz11QI90gE.XXui3MZv4-8.kg2KPXCgTU8mcEE_DB07mUQHKG_C-t6c-ISlI0UDM0Q",
  "expires_in": 0
}

Copy link
Owner

@gunthercox gunthercox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left comments on a few lines where some very minor changes/improvements could be made.

def __init__(self, **kwargs):
super(Microsoft, self).__init__(**kwargs)

self.directline_host = kwargs.get('directline_host')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to add a default value here so that directline_host does not always need to be specified. I'm fairly certain that the direct line host endpoint will usually be the same for every developer.

kwargs.get('directline_host', 'https://directline.botframework.com')

def __init__(self, **kwargs):
super(Microsoft, self).__init__(**kwargs)

self.directline_host = kwargs.get("directline_host")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This get method should also have the default set.

kwargs.get('directline_host', 'https://directline.botframework.com')

input_adapter="chatterbot.adapters.input.Microsoft",
directline_host="https://directline.botframework.com",
dirctline_conversation_id="IEyJvnDULgn",
directline_access_token_or_secret="RCurR_XV9ZA.cwA.BKA.iaJrC8xpy8qbOF5xnR2vtCX7CZj0LdjAPGfiCpg4Fv0",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the directline_access_token_or_secret parameter was changed to direct_line_token_or_secret in the code.

output_adapter="chatterbot.adapters.output.Microsoft",
direct_line_host="https://directline.botframework.com",
direct_line_conservationId="IEyJvnDULgn",
direct_line_access_token_or_secret="RCurR_XV9ZA.cwA.BKA.iaJrC8xpy8qbOF5xnR2vtCX7CZj0LdjAPGfiCpg4Fv0",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in this example, direct_line_access_token_or_secret should also be changed to direct_line_token_or_secret.

@gunthercox
Copy link
Owner

I believe all the changes here look good.

@gunthercox gunthercox merged commit e99cff5 into gunthercox:master Nov 13, 2016
@vkosuri vkosuri deleted the issue-290 branch August 15, 2017 03:59
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.

2 participants