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

feat(channels): add conversation.started event #266

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

samuelmasse
Copy link
Contributor

Adds a conversation.started event that allows sending a proactive message

Closes DEV-1623
Closes DEV-2070

@linear
Copy link

linear bot commented Dec 1, 2021

DEV-1623 Content of proactive messages should not be handled by messaging

We should send a requests to our webhooks instead to inform them that a conversation has started and let them decide what message to send to the user

DEV-2070 Add a conversation.started event to messaging that is triggered when a new conversation is created and a proactive message is possible

Comment on lines +101 to +115
if (started) {
await this.app.stream.stream(
'conversation.started',
{ conversationId, channel: channel.name },
clientId,
userId,
{
conduit: { id: this.conduitId, endpoint }
}
)
} else {
return this.app.messages.create(conversationId, userId, endpoint.content, {
conduit: { id: this.conduitId, endpoint: _.omit(endpoint, 'content') }
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the cleanest implementation. Channels are going to be refactored when they are moved to their own package and this is going to be way better

@samuelmasse
Copy link
Contributor Author

samuelmasse commented Dec 1, 2021

@laurentlp Should we consider the /start command to be a proactive message? This might break users. We could make this change in the v2 version of the channel

Here the bot would not respond for example and you would need to send another message. The thing is that the /start command is sent automatically when you start a conversation, so it is sort of a proactive trigger

Screenshot from 2021-12-01 14-36-53

@@ -72,6 +72,8 @@ export class TeamsConduit extends ConduitInstance<TeamsConfig, TeamsContext> {
await turnContext.sendActivity(message)
})
}

await this.receive(turnContext, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

We send the event after sending the proactive message to teams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proactive message implementation above is a legacy implementation. The new way of doing this is receiving the proactive trigger in botpress and sending a message. We keep the old one as well to avoid breaking people's stuff.

@laurentlp
Copy link
Contributor

@laurentlp Should we consider the /start command to be a proactive message? This might break users. We could make this change in the v2 version of the channel

Here the bot would not respond for example and you would need to send another message. The thing is that the /start command is sent automatically when you start a conversation, so it is sort of a proactive trigger

I think we have to in order to support proactive messages on Telegram. Since we don't have access to any other hook telling us that a user created a new conversation with a given bot, we should use the /start command as the conversation.started event.

Also, is my understanding of this feature correct? We would receive a /start message, then send a conversation.started event which would allow the user to send a proactive message or discard it and go with the normal flow?

@samuelmasse
Copy link
Contributor Author

@laurentlp Rigth now we treat /start as a text message containing /start. Which is why I suggest we move the /start command to be a proactive trigger in the next version of the channel otherwise we migth break many people's implementation.

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.

None yet

2 participants