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

Fixed issue where restart conversation was not clearing history. #1325

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

tonyanziano
Copy link
Contributor

Fixes #1323

@coveralls
Copy link

coveralls commented Feb 21, 2019

Pull Request Test Coverage Report for Build 2103

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 54.916%

Files with Coverage Reduction New Missed Lines %
packages/app/client/src/ui/editor/emulator/emulator.tsx 1 72.02%
Totals Coverage Status
Change from base Build 2097: 0.03%
Covered Lines: 4861
Relevant Lines: 8310

💛 - Coveralls

await CommandServiceImpl.remoteCall(SharedConstants.Commands.Emulator.SetCurrentUser, newUserId);
this.props.updateChat(this.props.documentId, { userId: newUserId });
// start conversation with new convo id & user id
this.startNewConversation(undefined, true, true);
Copy link
Contributor

@compulim compulim Feb 22, 2019

Choose a reason for hiding this comment

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

Why don't we create unique ID before we call startNewConversation?

For example,

this.startNewConversation({
  ...this.props,
  document: {
    ...this.props.document,
    conversationId: uniqueIdv4(),
    userId: uniqueIdv4()
  }
});

And can we always call setCurrentUser every time we call startNewConversation?

Your existing logic may be good or have some reasons behind, I just didn't see through it yet. If there are any, please consider me approved. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why this approach wouldn't work. I actually like it a lot more than making a function call like (undefined, true, true) -- seems odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this approach out and it actually causes new livechat windows to be opened on every restart, as well as prevents webchat from connecting to directline when restarting with a new user id. I'm not entirely sure why though. I believe it might have to do with setting the current user every time.

break;
}

case SameUserId:
this.props.trackEvent('conversation_restart', {
userId: 'same',
});
this.startNewConversation();
// start conversation with new convo id
this.startNewConversation(undefined, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Or should it be like this?

Suggested change
this.startNewConversation(undefined, true);
this.startNewConversation(undefined, true, false);

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.

3 participants