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

Restoring chat history should not mark user activities as "send failed" #4501

Closed
2 tasks
compulim opened this issue Nov 11, 2022 · 1 comment · Fixed by #4532
Closed
2 tasks

Restoring chat history should not mark user activities as "send failed" #4501

compulim opened this issue Nov 11, 2022 · 1 comment · Fixed by #4532
Assignees
@compulim
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. icm Linked from IcM
Milestone
imminent

Comments

@compulim
Copy link
Contributor

compulim commented Nov 11, 2022 •
edited
Loading

Is it an issue related to Adaptive Cards?

  • Yes, this is an Adaptive Card issue but it is specific to Web Chat.

Do you have any screenshots?

image

What version of Web Chat are you using?

Latest production

The bug first introduced in 4.15.3 and did not repro on 4.15.2.

Which distribution are you using Web Chat from?

Bundle (webchat.js)

Which hosting environment does this issue primarily affect?

Web apps

Which browsers and platforms do the issue happened?

Browser: Edge (latest), Browser: Chrome (latest), Browser: Firefox (latest), Browser: Safari (latest), Browser: IE Mode (latest), Platform: Windows, Platform: macOS, Platform: iOS/iPadOS, Platform: Android

Which area does this issue affect?

Chat history

Is this an accessibility issue?

  • Yes, this is an accessibility issue.

Please describe the bug

When restoring chat history from ABS, activities sent by user are wrongly tagged as "Send failed. Retry."

This only repro when the localTimestamp field is present.

Do you see any errors in console log?

No response

How to reproduce the issue?

Use this HTML in WebDriver.

<!DOCTYPE html>
<html lang="en-US">
  <head>
    <link href="/assets/index.css" rel="stylesheet" type="text/css" />
    <script crossorigin="anonymous" src="/test-harness.js"></script>
    <script crossorigin="anonymous" src="/test-page-object.js"></script>
    <script crossorigin="anonymous" src="/__dist__/webchat-es5.js"></script>
    <!-- <script crossorigin="anonymous" src="https://cdn.botframework.com/botframework-webchat/4.15.2/webchat-es5.js"></script> -->
  </head>
  <body>
    <div id="webchat"></div>
    <script>
      run(async function () {
        const store = testHelpers.createStore();
        const directLine = testHelpers.createDirectLineEmulator(store);

        WebChat.renderWebChat(
          { directLine, store, userID: 'u-00001' },
          document.getElementById('webchat')
        );

        await pageConditions.uiConnected();

        await directLine.emulateIncomingActivity({
          type: 'message',
          id: 'c00001-us|0000000',
          timestamp: '2022-11-11T19:34:00.000Z',
          // If `localTimestamp` is not present, this will not repro.
          localTimestamp: '2022-11-11T11:34:00.000-08:00',
          from: {
            id: 'u00001',
            role: 'user'
          },
          text: 'Hello, World!',
          channelData: {
            clientActivityID: 'ca00001'
          }
        });
      });
    </script>
  </body>
</html>

What is the expected and actual behavior?

Expected:
The send status for the first activity should be "Just now." or a date.

Actual:
The send status is "Send failed. Retry."

Adaptive Card JSON

No response

Additional context

Possibly related to PR #4362.

Related to ticket 346670699.
@compulim compulim added bug Indicates an unexpected problem or an unintended behavior. customer-reported Required for internal Azure reporting. Do not delete. Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-replied-to Required for internal reporting. Do not delete. icm Linked from IcM labels Nov 11, 2022
@compulim compulim added this to the imminent milestone Nov 11, 2022
@compulim compulim self-assigned this Nov 11, 2022
@compulim compulim mentioned this issue Nov 16, 2022
Closed
2 tasks
@compulim
Copy link
Contributor Author

compulim commented Nov 29, 2022 •
edited
Loading

This is because a missing piece from the following logic in reducers/activities.ts#L193:

// If the incoming activity is an echo back, we should keep the existing `channelData['webchat:send-status']` field.
//
// Otherwise, it will fail following scenario:
//
// 1. Send an activity to the service
// 2. Service echoed back the activity
// 3. Service did NOT return `postActivity` call
// -  EXPECT: `channelData['webchat:send-status']` should be "sending".
// -  ACTUAL: `channelData['webchat:send-status']` is `undefined` because the activity get overwritten by the echo back activity.
//            The echo back activity contains no `channelData['webchat:send-status']`.

When an echo back activity arrives, reducers/activities.ts will copy/patch the channelData['webchat:send-status'] field from existing activity in the chat history. This is to ensure "all activities must have non-undefined send status at all time". Without this patch, the activity could be momentarily undefined until the completion of postActivitySaga (or POST_ACTIVITY_FULFILLED).

This is actually the fix of a bug when we are using the channelData.state, which send status of echo back activities are momentarily undefined before it turn into "sent".

However, when in restoring process, no activities with same ID would be found. Thus, we will leave the channelData['webchat:send-status'] field as undefined. Thus, it is treated as "send failed".

The fix would be:

  • On echo back, copy/patch the channelData['webchat:send-status']
  • On restore, the channelData['webchat:send-status'] must always be "sent", because the server already received this message

To differentiate echo back vs. restore:

  • If the current chat history has the activity of same ID, this is echo back. In other words, when sending an activity, we always put it in the chat history before it is sent
  • Otherwise, this is restore

@compulim compulim mentioned this issue Nov 29, 2022
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@compulim compulim

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. icm Linked from IcM
Projects
None yet
Milestone
imminent
1 participant
@compulim