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

Srravich/feature/2039 restart conversation #2089

Merged
merged 11 commits into from Mar 5, 2020

Conversation

@srinaath
Copy link
Contributor

srinaath commented Feb 29, 2020

Fixes #2039

This PR adds the capability to replay activities in a conversation. It allows multiple conversations being replayed at the same time.

Screen Shot 2020-02-28 at 5 13 10 PM

Screen Shot 2020-02-28 at 5 05 56 PM

FinalReplay

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 29, 2020

Coverage Status

Coverage decreased (-0.4%) to 67.8% when pulling 723a8b2 on srravich/feature/2039-restart-conversation into 5ca8253 on master.

@@ -107,7 +106,7 @@ const defaultConfig = {
],
},

devtool: 'source-map',
devtool: 'eval-source-map',

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 2, 2020

Contributor

Does this fix the Web Chat source maps and preserve the Emulator source maps? I tried to change it locally from devtool: sourcemap to using the SourceMapDevTool plugin from web chat and it managed to fix the Web Chat source maps, but downgraded the quality of the Emulator source maps to point to the transpiled code.

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 2, 2020

Author Contributor

It does not Tony. So it still shows the transpiled code for webchat and debuggable code for emulator. So I was facing an issue with step through debugging on Chrome Dev tools failing when i use source-map. The moment I use eval-source-maps it does not break or Chrome dev tools does not seem to break when i step though and debug. Have u faced this issue before?

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

The issue I was facing was that the Web Chat sourcemaps were pointing to the wrong files (usually off by one file in the parent directory). So if in some Web Chat directory, there were files A, B, and C, in my debugger, trying to pull up file C would point to the sourcemap for file B.

Switching to the source map plugin fixed that for me, but also downgraded the sourcemaps of the Emulator code to point to the transpiled code instead of the TypeScript source.

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 5, 2020

Author Contributor

I have reverted it to use just source-map. Seems like we can come up with a good solution for R9 that enables source maps for emulator as well as enable it for our shared libs (webchat, shared/app, shared/sdk)

FROM_USER_ROLE: 'user',
FROM_BOT_ROLE: 'bot',
Comment on lines 219 to 220

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

Consider changing these to USER_ROLE and BOT_ROLE because they are the same values used in the recipient property of activities, and then could be used elsewhere in the application without having to make new constants for TO_USER_ROLE and TO_BOT_ROLE

const activities = await resp.json();
return activities;
} catch (ex) {
return [];

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

The pattern in the ConversationService is to return an HTTP response, so maybe consider returning the fetch and performing the resp.json() in the service-calling code.

Also, it might be a good idea to surface the exception in some way for debugging.

@@ -107,7 +106,7 @@ const defaultConfig = {
],
},

devtool: 'source-map',
devtool: 'eval-source-map',

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

The issue I was facing was that the Web Chat sourcemaps were pointing to the wrong files (usually off by one file in the parent directory). So if in some Web Chat directory, there were files A, B, and C, in my debugger, trying to pull up file C would point to the sourcemap for file B.

Switching to the source map plugin fixed that for me, but also downgraded the sourcemaps of the Emulator code to point to the transpiled code instead of the TypeScript source.

// import { EventEmitter } from 'events';

// import { Activity } from 'botframework-schema';
Comment on lines 33 to 35

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

delete

export interface WebChatActivityChannel {
sendWcEvents: (args: ChannelPayload) => void;
getWebchatChannelSubscriber: () => Channel<ChannelPayload>;
}

export interface WebchatEventPayload {
Comment on lines 41 to 46

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

align on capitalization pattern for Web Chat:

WebChatActivityChannel

WebchatEventPayload

is the C capitalized or not?

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

WebChat is what I'll go with. Reason being there are multiple usages like createWebChatStore etc written before. I'll probably stick to that pattern. Though, i think webchat semantically makes more sense (https://github.com/microsoft/BotFramework-webchat)

}

function createReplayActivitySniffer(documentId: string, meta: ReplayActivitySnifferProps = undefined) {
return ({ dispatch }) => next => async action => {

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

async not necessary

speechKey?: string;
speechRegion?: string;
user: User;

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

keep user here -- sorted alphabetically

meta.conversationQueue.getNextActivityForPost,
]);
if (postActivity) {
yield call(dispatchActivityToWebchat, dispatch, postActivity);

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

is there a reason you aren't using put here instead of calling dispatch?

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

Commented below

} finally {
yield put(updatePendingSpeechTokenRetrieval(documentId, false));
yield fork(ChatSagas.handleReplayIfRequired, { documentId, action, dispatch, meta });

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

why does dispatch need to be passed in if we have access to put ?

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

So, this is dispatching on the webchat store. Put automatically dispatches to our emulator store. So, thats why I'm using call on the dispatch i get from the middleware

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 4, 2020

Contributor

Ahhh, I didn't catch that. Sounds good to me 👍

yield* throwErrorFromResponse('Error occurred while retrieving the web socket port', res);
throw new Error(
`Error occurred while retrieving the WebSocket server port: ${res.status}: ${res.statusText ||
'No status text'}`
);
Comment on lines 498 to 684

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Mar 3, 2020

Contributor

change back to throwErrorFromResponse?

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

Nice catch.. Effects of resolving the Merge conflict

meta.conversationQueue.getNextActivityForPost,
]);
if (postActivity) {
yield call(dispatchActivityToWebchat, dispatch, postActivity);

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

Commented below

} finally {
yield put(updatePendingSpeechTokenRetrieval(documentId, false));
yield fork(ChatSagas.handleReplayIfRequired, { documentId, action, dispatch, meta });

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

So, this is dispatching on the webchat store. Put automatically dispatches to our emulator store. So, thats why I'm using call on the dispatch i get from the middleware

public static *watchForWcEvents() {
const wcEventChannel = ChatSagas.wcActivityChannel.getWebchatChannelSubscriber();
while (true) {
const { documentId, action, dispatch, meta }: ChannelPayload = yield take(wcEventChannel);

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

Dispatch I receive from webchat middleware

yield* throwErrorFromResponse('Error occurred while retrieving the web socket port', res);
throw new Error(
`Error occurred while retrieving the WebSocket server port: ${res.status}: ${res.statusText ||
'No status text'}`
);

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

Nice catch.. Effects of resolving the Merge conflict

export interface WebChatActivityChannel {
sendWcEvents: (args: ChannelPayload) => void;
getWebchatChannelSubscriber: () => Channel<ChannelPayload>;
}

export interface WebchatEventPayload {

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

WebChat is what I'll go with. Reason being there are multiple usages like createWebChatStore etc written before. I'll probably stick to that pattern. Though, i think webchat semantically makes more sense (https://github.com/microsoft/BotFramework-webchat)

requireNewUserId: boolean = false
requireNewUserId: boolean = false,
activity: Activity = undefined,
createObjectUrl: Function = undefined

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

So the reason was I just thought accessing the window object directly inside chatSaga might not be a good idea. Rather injecting it as a dependacy would help us with unit testing as well.

requireNewUserId: boolean = false
requireNewUserId: boolean = false,
activity: Activity = undefined,
createObjectUrl: Function = undefined

This comment has been minimized.

Copy link
@srinaath

srinaath Mar 4, 2020

Author Contributor

What do u think?

Srinaath Ravichandran added 10 commits Feb 12, 2020
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

get activities for a conversation

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Return updated activity

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Adding declaration maps for inspection

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Conversation routes set for fetch activities

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Safe commit

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Queues updated

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Safe commit

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Safe saga commit

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Resart multiple times the same conversation working. Major landmark

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Stable replay

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Restart from specific activity working

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

UI updates

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Safe saga flow

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Safe replay

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Forked post activity to let moving on to next activity

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Blocking webchat

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Simenatous conversations complete

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Error notficaition viewer completed

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Handling Dlspeech bot sniffer

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Conversation service spec completed

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Post activity test

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Mount conversation routes test

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Spec files updated

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Tests wrapped up for conversation queue

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Progressive response handling

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Stable commit to replay chat

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

UI tests restored to normality

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Tests working for conversationQueue

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Chatsagas stable tests

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

one more test working

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

All tests passing

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Chat saga test wrapped up

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Restart conversation queue tests completed

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

UI tests updated

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

More UI tests

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Renaming for consistent webchat captialized handling
Renaming functions

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

More PR feedback handled

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Srinaath Ravichandran
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
@srinaath srinaath force-pushed the srravich/feature/2039-restart-conversation branch from fbbb974 to 6ae438e Mar 5, 2020
Srinaath Ravichandran
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
@srinaath srinaath requested a review from tonyanziano Mar 5, 2020
Copy link
Contributor

tonyanziano left a comment

Looks good! Great job 😃

@srinaath

This comment has been minimized.

Copy link
Contributor Author

srinaath commented Mar 5, 2020

Looks good! Great job 😃

thanks Tony

@srinaath srinaath merged commit 81cd3a9 into master Mar 5, 2020
2 checks passed
2 checks passed
Emulator-CI-PR #110759 succeeded
Details
license/cla All CLA requirements met.
Details
@srinaath srinaath deleted the srravich/feature/2039-restart-conversation branch Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.