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.

Sign up for GitHub

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

Jump to bottom

Fix activity without "from" #1405

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Fix activity without "from" #1405

merged 2 commits into from
Nov 28, 2018

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Nov 28, 2018 •
edited
Loading

Will fix #1397.

Changes

  • Patch activity.from.role
    • "user" if from.id is user ID, otherwise,
    • "bot" if from.id is anything otherwise,
    • "channel", because either from or from.id was not defined (e.g. in ConversationUpdate activity)
  • Should not set suggested actions if there are nothing on the transcript

@compulim compulim mentioned this pull request Nov 28, 2018
Closed
@compulim compulim added this to the v4.2 milestone Nov 28, 2018
@compulim compulim added the 4.2 label Nov 28, 2018
@coveralls
Copy link

coveralls commented Nov 28, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 506

  • 4 of 6 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 46.598%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/src/sagas/incomingActivitySaga.js 4 6 66.67%
Totals Coverage Status
Change from base Build 483: 0.08%
Covered Lines: 713
Relevant Lines: 1370
💛 - Coveralls

corinagum
corinagum approved these changes Nov 28, 2018
View reviewed changes
Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

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

Approved, pending nitpick CHANGELOG.md update

CHANGELOG.md Show resolved Hide resolved
@compulim compulim merged commit 7d59219 into microsoft:master Nov 28, 2018
@compulim compulim deleted the fix-1397 branch November 28, 2018 21:12
@hansmbakker
Copy link

hansmbakker commented Apr 10, 2019 •
edited
Loading

@compulim @corinagum is this supposed to work in the embedded WebChat in the Azure Portal?
(at https://webchat.botframework.com/embed/<botId>?s=<secret>)

I was seeing the role being empty for activities sent from the bot, and I was wondering whether that is a bug or not?

I had the preview checkbox enabled in the webchat channel configuration.

@hansmbakker
Copy link

Also using the following HTML taken from the Full bundle sample the role property is not set in from for bot messages:

<!DOCTYPE html>
<html>
  <body>
    <div id="webchat" role="main"></div>
    <script src="https://cdn.botframework.com/botframework-webchat/latest/webchat.js"></script>
    <script>
      window.WebChat.renderWebChat({
        directLine: window.WebChat.createDirectLine({ token: 'somesecret' }),
        userID: 'myusername'
      }, document.getElementById('webchat'));
    </script>
  </body>
</html>

@hansmbakker
Copy link

This works correctly in the Bot Emulator though.

@hansmbakker hansmbakker mentioned this pull request Apr 10, 2019
Closed
@hansmbakker
Copy link

Tracking in #1886.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corinagum corinagum corinagum approved these changes

@tonyanziano tonyanziano Awaiting requested review from tonyanziano

@justinwilaby justinwilaby Awaiting requested review from justinwilaby

@stevengum stevengum Awaiting requested review from stevengum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v4.2
Development

Successfully merging this pull request may close these issues.

Greeting Message Web Chat v4 - Sending events from bot
4 participants
@compulim @coveralls @hansmbakker @corinagum