Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Support entire message objects in convo handlers. Fixes #1793 and #1710 #1801

Closed
wants to merge 13 commits into from
Closed

Support entire message objects in convo handlers. Fixes #1793 and #1710 #1801

wants to merge 13 commits into from

Conversation

naikus
Copy link

@naikus naikus commented Sep 8, 2019

This merge request allows botkit conversation handlers (those that go with addQuestion api) to receive full message objets. ( Fixes #1710 )

Also any augmentation done by botkit middlewares to messages are now available in handlers. This is especially useful for NLU intent and entity identification (i.e. if any of the handlers need to know them) ( Fixes #1793 )

@benbrown
Copy link
Contributor

benbrown commented Sep 9, 2019

Thanks for this work.

I have some initial concerns about breaking changes - even things like changing text from optional to required could break things for devs.

Does this change the signature of any handlers?

@naikus
Copy link
Author

naikus commented Sep 9, 2019

Hi @benbrown, thanks for replying. This does not change signatures for any existing handlers. Except the optional -> required 'text' property, the change is completely internal to botkit.

In fact existing conversation handlers (in botkit v0.7x) expected message objects which changed to only text. Also in case of questions only text can anyway be captured in capture key (as a convo var)

The BotkitMessage text was made a 'required' property because the class that it extends from has that set as required.

Thanks,
A

@benbrown benbrown self-assigned this Sep 13, 2019
@benbrown
Copy link
Contributor

I am on this and plan to get it reviewed and included in the next release if at all possible.

@benbrown benbrown added this to the 4.6 milestone Sep 13, 2019
@benbrown
Copy link
Contributor

Hi @naikus,
After careful review of this I can't accept it in its current state.
The key issue is that changing the signature of the handler functions that currently receive a string value to receiving the entire message payload is a breaking change.

However, I do think this is a valid issue and you've done 90% of the work.

In order to merge this, I request the following change:

Instead of swapping the text field for the full message, add an additional parameter to the handler signature.
This would result in a function that receives (answer, convo, bot, full_message)

This way, the change is additive and will not break existing bots.

@benbrown benbrown modified the milestones: 4.6, 4.6.+ Oct 31, 2019
@naikus
Copy link
Author

naikus commented Nov 2, 2019

Hi @benbrown, I agree. I'll make the changes and to the signature and push the changes for review.
Thanks!

@benbrown
Copy link
Contributor

Thanks @naikus! Ping me when you push the new commits.

@etiennellipse
Copy link
Contributor

@naikus Any updates on this?

@naikus
Copy link
Author

naikus commented Dec 13, 2019

@naikus Any updates on this?

Hi @etiennellipse
I'm a bit busy with some tasks. But I'll have this push in soon. (End of dec tentatively or earlier if possible)
Thanks,
Aniket

@naikus
Copy link
Author

naikus commented Jan 31, 2020

Hi @benbrown,
I've pushed the commits according to the API you suggested.
The callback handlers for questions now take 4 arguments:

convo.addQuestion(
    "what?",
    async (response, convo, bot, message) => {
      // This logs the entire message payload (BotkitMessage) where message.text is same as response
      console.log(message); 
      await convo.gotoThread('thread-3');
    }, 
   "what",
   "thread-2"
);

@deepakmahakale
Copy link
Contributor

deepakmahakale commented Jan 31, 2020

I have been waiting for this change for months now 😄
Kudos

Hope this comes in next version +1

@NxP4Code
Copy link
Contributor

I am also waiting for this one for a long time .. hope this gets approved soon and comes in next release.

It may fix #1862

@benbrown
Copy link
Contributor

I'm on it!

@naikus
Copy link
Author

naikus commented Jan 31, 2020

I've done some e2e testing with this. I was wondering if it'd help if there were unit tests for this. I can start on those. Only I did not see any existing tests for callbacks for convo.addQuestion

@benbrown
Copy link
Contributor

@naikus yes, new tests would be wonderful.

…ers to test message objects are passed to them when they are invoked
…ers to test message objects are passed to them when they are invoked
@naikus
Copy link
Author

naikus commented Jan 31, 2020

@benbrown I've added necessary unit tests, thanks.

@sureshvarman
Copy link

+1

Copy link

@adam-resdiary adam-resdiary left a comment

Choose a reason for hiding this comment

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

Any update on this? I've just bumped into this problem as well when trying to update a bot from 0.5.8 to the latest version. We were relying on the message being available in the handler so that we could call bot.replyInteractive(). It would be great to get this added so that we can update our bot.

user: message.from.id,
channel: message.conversation.id,
value: message.value,
incoming_messge: message,

Choose a reason for hiding this comment

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

Should this not be incoming_message?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes good catch I will probably not merge this piece anyways since it is sort of out of scope of the main change.

@benbrown
Copy link
Contributor

working on merging this now.

@benbrown
Copy link
Contributor

snag #1: the way this is implemented to create a new turncontext breaks a bunch of the expected adapter behavior regarding the turn state. however, without it, we don't get access to the message object we want.

experimenting!

@benbrown
Copy link
Contributor

phew! I was able to work around that issue by using the turnContext.turnState to store the botkit message and make it available inside the dialog.

@benbrown benbrown mentioned this pull request Mar 11, 2020
@benbrown
Copy link
Contributor

benbrown commented Mar 11, 2020

OK! This functionality is included in the next release -> #1929

@adam-resdiary
Copy link

@benbrown nice one! Any idea when the new release is likely to be made available?

@benbrown
Copy link
Contributor

@adam-resdiary this week unless a big snag is encountered on the botbuilder 4.8 release, which I am waiting for. Should be tomorrow!

@benbrown
Copy link
Contributor

AT LONG LAST, this has been merged!

@benbrown benbrown closed this Mar 17, 2020
@pgoldweic
Copy link

@benbrown, does this work also for the convo.after handler? The documentation does not include a full_message parameter in there. I'm realizing this would resolve a legacy migration issue that I'm having ( in which I need the full message object once a -single question- dialog ends). In legacy botkit I was able to use a dynamic convo for this (created within a controller.hears() handler).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants