search

paked / messenger

Package messenger is used for making bots for use with Facebook messenger
MIT License
274 stars 74 forks source link

Implement support for AccountLinking process #39

Closed dgellow closed 6 years ago

dgellow commented 6 years ago

This pull request adds a method HandleAccountLinking and the corresponding message type AccountLinking.

As specified in the documentation about the authentication process, an event messaging_account_linking is emitted after a successful linking/unlinking action.

I tried to follow what was already existing, tell me if something should be changed.

Also, if you're interested, I have a version of the example in cmd/bot that demonstrates the entire account linking process. That could be quite useful for beginners I think, though the example would become more complex. I would gladly create another pull request for that purpose.

paked commented 6 years ago

It might be worth creating a example to showcase the more complex behaviour.

Maybe we could move /cmd/bot to /examples/basic, then add /examples/linked-account etc.

paked commented 6 years ago

Apart from that, LGTM.

dgellow commented 6 years ago

Maybe we could move /cmd/bot to /examples/basic, then add /examples/linked-account etc.

Yes, that sounds good.

dgellow commented 6 years ago

To give you an idea of the result, you have here a screenshot of a session interacting with the new bot showing the support for the account linking process.

screen shot 2018-03-15 at 21 34 34

And a view of the login form presented to the user during the process.

screen shot 2018-03-15 at 21 34 12
dgellow commented 6 years ago

For the new example I haven't used your library github.com/paked/configure, the reason was to remove dependencies. That way the example can be built with just a go build. I would suggest to do the same for the basic example, but it's your call.

Also, when we will have a travis job setup (cf https://github.com/paked/messenger/pull/40#issuecomment-373225571), we will be able to add a step go build ./examples/... to ensure that PRs don't break the examples.

paked commented 6 years ago

OK. I've just merged the Travis PR in. I don't think I can manually trigger a build on this PR, but if you make another commit (or git commit --amend) it will trigger.

RE: not using configure, that's fine. If you're going down that road in this example we should probably do it in both. Would you mind removing the use of it in the basic example?

Once this is done, will merge.

dgellow commented 6 years ago

I rebased on master, removed paked/configure from the example basic, added the step building examples to travis config.

dgellow commented 6 years ago

@paked Is anything missing for this Pull Request being merged?

paked commented 6 years ago

Sorry for the delay, merged.

Thanks for your contribution!