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

Activity Status not being rendered when adding reactions as per sample #3473

Closed
nathanlindorff opened this issue Sep 11, 2020 · 6 comments · Fixed by #3601
Closed

Activity Status not being rendered when adding reactions as per sample #3473

nathanlindorff opened this issue Sep 11, 2020 · 6 comments · Fixed by #3601
Assignees
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-replied-to Required for internal reporting. Do not delete. customer-reported Required for internal Azure reporting. Do not delete. p1 Painful if we don't fix, won't block releasing
Milestone

Comments

@nathanlindorff
Copy link

nathanlindorff commented Sep 11, 2020

Adding reaction buttons as per https://github.com/microsoft/BotFramework-WebChat/tree/master/samples/05.custom-components/d.reaction-buttons/ results in the Activity Status (eg "5 minutes ago") not being rendered. Running the demo page associated with the sample shows the same issue (See screenshot)

I came across the issue while upgrading an older webchat from version 4.5 to 4.10. In the debugging I did for that the renderActivityStatus and renderAvatar functions are not being passed through as properties to the StackedLayout.

I realised after that that the sample had also changed, but the updated sample also displays the same issue.

Screenshots

From https://microsoft.github.io/BotFramework-WebChat/05.custom-components/d.reaction-buttons/

image

Debug screenshots from the bot i'm upgrading:

Props from StackedLayout bot message without reactions
image image

Props from StackedLayout bot message with reactions
image image

I haven't had a chance to update the code to match the updated reaction sample, but as stated the outcome (the missing status) is the same on the demo page.

class ThumbActivity extends React.Component<any> {
   public handleDownvoteButton = () => this.props.postActivity({ type: 'messageReaction', reactionsAdded: [{ activityID: this.props.activityID, helpful: -1 }] })
   public handleUpvoteButton = () => this.props.postActivity({ type: 'messageReaction', reactionsAdded: [{ activityID: this.props.activityID, helpful: 1 }] })

   public render() {
      const { props } = this

      return (
         <div style={thumbActivityStyle}>
            <div style={childActivityStyle}>{props.children}</div>
            <ul style={thumbsButtonBarStyle}>
               <li style={thumbsButtonListItemStyle}>
                  <button style={thumbButtonStyle} onClick={this.handleUpvoteButton}>👍</button>
               </li>
               <li style={thumbsButtonListItemStyle}>
                  <button style={thumbButtonStyle} onClick={this.handleDownvoteButton}>👎</button>
               </li>
            </ul>
         </div>
      )
   }
}

const ConnectedThumbActivity = (window as any).WebChat.connectToWebChat(
   ({ postActivity }) => ({ postActivity })
)(props => <ThumbActivity {...props} />)

export const thumbsMiddleware = () => next => card => {
   const allowReaction: boolean = card.activity.channelData && card.activity.channelData.allowReaction
   if (card.activity.from.role === 'bot' && allowReaction) {
      return children => (
         <ConnectedThumbActivity activityID={card.activity.id}>
            {next(card)(children)}
         </ConnectedThumbActivity>
      )
   } else {
      return next(card)
   }
}

I will update the code to use the new recommended syntax, but it appears based on the new sample that it will still be an issue.

Version

Version: 4.10 (Upgrading from 4.5)
Using NPM package
Embeded in iFrame

Describe the bug

When adding middleware to add reactions, the Activity Status and Avatar are not rendered.

Steps to reproduce

  1. See description and screenshots

Expected behavior

Expect message to render as per normal, with additional reaction elements.

[Bug]

@nathanlindorff nathanlindorff added Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-reported Required for internal Azure reporting. Do not delete. labels Sep 11, 2020
@nathanlindorff
Copy link
Author

I can confirm that after converting the React Component to use the new style the same issue occurs

image image

@nathanlindorff
Copy link
Author

Dropping back to 4.9.2 everything has started working as expected again. I suspect the issue may have something to do with the breaking changes mentioned in 4.10.0 release notes given the mention of StackedLayout, but our code makes no direct reference to them.

@corinagum corinagum added the needs-repro Waiting for repro or investigation label Sep 11, 2020
@corinagum
Copy link
Contributor

Thanks for filing this issue, @nathanlindorff. I'm adding it to our Investigate list for R11, for which the planning meeting is happening today.

@corinagum corinagum added the customer-replied-to Required for internal reporting. Do not delete. label Sep 11, 2020
@cwhitten cwhitten added the R11 label Sep 11, 2020
@corinagum corinagum added this to the R11 milestone Sep 11, 2020
@corinagum corinagum removed needs-repro Waiting for repro or investigation R11 labels Sep 23, 2020
@cwhitten cwhitten added the p1 Painful if we don't fix, won't block releasing label Oct 14, 2020
@corinagum corinagum modified the milestones: R11, R12 Oct 22, 2020
@corinagum
Copy link
Contributor

Quick update: I'm still investigating the cause of this issue. My initial assumption was that it would be a sample fix, but that is incorrect. This most likely won't make the R11 release.

Note that this also affects the user highlighting sample.

@nathanlindorff
Copy link
Author

@corinagum It may actually end up being a sample/documentation change. A colleague was having the same issue on a similar project and wasn't keen on dropping back a version. After a bit of digging into the webchat code, and looking through some other samples, It appears that the middleware/renderer for StackedLayout has changed from requiring one argument (children) to two (renderAttachment, props).

As a result, spreading the children seems to have fixed the issue in my colleague's instance.

eg

export const myActivityMiddleware = () => (next: any) => (card: any) => {
    if (card.activity.from.role === 'bot') {
        return (children: any) => (
            <MyActivityDecorator>
                {next(card)(children)}
            </MyActivityDecorator>
        );
    }
    return next(card);
};

Causes the issue, but

export const myActivityMiddleware = () => (next: any) => (card: any) => {
    if (card.activity.from.role === 'bot') {
        return (...children: any) => (
            <MyActivityDecorator>
                {next(card)(...children)}
            </MyActivityDecorator>
        );
    }
    return next(card);
};

Appears to resolve the issue.

I'm not familiar enough with the internals of the framework to be 100% sure thats the issue. I've also not had a chance to try it in the code I reported the issue on, but hopefully that helps.

@corinagum
Copy link
Contributor

@nathanlindorff thanks for the info!

You're right, since my last update I figured out that this is a quick sample update from when our middleware format changed. I definitely plan on getting the fix in R12. In the meantime, check out the middleware design change for more info: https://github.com/microsoft/BotFramework-WebChat/blob/master/CHANGELOG.md#breaking-changes-1 Hope that helps you out, and sorry I haven't gotten the fix in yet.

@compulim compulim mentioned this issue Mar 2, 2021
52 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-replied-to Required for internal reporting. Do not delete. customer-reported Required for internal Azure reporting. Do not delete. p1 Painful if we don't fix, won't block releasing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants