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

Added Chat Barge to Supervisor Barge Coach Feature #521

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

aestellwag
Copy link
Contributor

Summary

I've added a chat barge feature to the Supervisor Barge Coach feature. This simply adds a "Join" button to the Monitor Task canvas when monitoring an active conversation. I've added enhancements to state management as I could imagine supervisors may join multiple chats at the same time using this new feature.

The button will also remove them from there, it changes to "Leave", simply click this to leave the conversation when done.

Open to feedback on the UI experience and if you catch any bugs just holler at me.

https://github.com/twilio-professional-services/flex-project-template/blob/chatbarge/docs/static/img/features/supervisor-barge-coach/Supervisor-Barge-Coach-Plugin-10.gif

Checklist

  • [ x ] Tested changes end to end
  • [ x ] Updated documentation
  • [ x ] Requested one or more reviewers

@dremin
Copy link
Contributor

dremin commented Mar 20, 2024

Thanks for working on this! It's looking pretty good! Here's some feedback from my testing out the feature. I'll let others do the code review before I look to add any input there 😃

  • The user's identity shouldn't be retrieved from state.flex.worker.worker.name, as it is not the same as the identity that the conversations client uses. The identity is available instead from state.flex.session.identity. This can be tested using a worker with special characters in their name, as the values differ in this case.
  • The Join/Leave button is a bit cut off and misaligned, because Paste is silly. Try wrapping the Button in a Box with padding of space20, instead of the Stack, to match what the supervisor complete button is doing. It would be nice if possible if the buttons could be next to each other instead of spread out, but I'm not 100% certain how feasible that is.
  • The Join/Leave button shows for a conversation I am already a participant of. When I try to click it, the serverless correctly returns a 409, but the button still updates as if the request was successful. The button probably should be hidden or disabled to prevent encountering this.
  • The participant list from the conversation-transfer feature does not show the supervisor, but both the count is increased, and the 'end chat' button changes to 'leave chat'. If the agent clicks 'leave chat', we end up with an orphaned conversation--such that the customer can never reach us again. This happens because the agent leaving causes there to no longer be any task for that conversation, but because there is still the supervisor participant, the conversation itself stays open. However, the supervisor can no longer chat because the task being closed updates their UI. At this point the conversation must be closed via API.

…when a supervisor attempts to barge in (logic around if they are already a member of the conversation or not)
@aestellwag
Copy link
Contributor Author

Great catches @dremin . I've updated the code based on your feedback/suggestions. A few additions:

  • Largest change is that I've updated the Leave Chat logic for the Conversation Transfer feature. We now check if there are > 2 Interaction Participants vs just Conversation Participants. The way the chat barge works is it adds them as a conversation participant but since we aren't routing them a task, we don't have a Flex Interactions SID/Participant being added for the supervisor. Now it will end the chat if there isn't more than one agent within the conversation, avoiding the conversation going into the void and the customer being stuck in the conversation forever (or until you delete/close the conversation manually via the API)
  • I've added additional logic when the supervisor monitors the conversation. If we don't have history of them in redux/cache, we do a quick check to see if they are already a member of the conversation. Updating the button respectively (this will prevent the 409s you were seeing)
  • I've updated the workerName logic based on your experience
  • Finally updated the button to fit the theme of the monitor panel better (thanks for the suggestions there)

Copy link

github-actions bot commented Mar 26, 2024

0 ESLint error(s) and 2 ESLint warning(s) found in pull request changed files.
⚠️ Merging is still possible with these warnings, but please fix them if possible! Annotations are available in the "Files changed" tab. Run npm run lint if you would like to see results locally.

@dremin
Copy link
Contributor

dremin commented Mar 27, 2024

Nice updates @aestellwag!

I've added additional logic when the supervisor monitors the conversation. If we don't have history of them in redux/cache, we do a quick check to see if they are already a member of the conversation. Updating the button respectively (this will prevent the 409s you were seeing)

This brings a new problem--if I am an interaction participant for this conversation (i.e. I have a task for it), I should not have access to the Leave button. Clicking it keeps the task but then you can't see the conversation from the agent view.

Also, I noticed this part uses a serverless function, but doesn't Flex already have a full participant map in redux once the view loads? Or was it missing something needed for this?

@aestellwag
Copy link
Contributor Author

Enhancements added in the latest release:

  • Removed an unnecessary function/api call and now check the conversation state directly
  • Added logic to check if the supervisor is assigned the task for that conversation and disable the chat barge button
  • Simplified the logic throughout component

@aestellwag
Copy link
Contributor Author

aestellwag commented Mar 28, 2024

I've enhanced the Conversation Transfer participant tab/list to include the barged user. Prior to this enhancement it would only show Flex Interaction Participants (which the barged participant is not). IE it's showed there were 3 participants, but only list 2 in the list of participant details (as it was only checking the Interaction Flex Participants).

@dremin
Copy link
Contributor

dremin commented Mar 28, 2024

Sorry, found another edge case to handle!

If I am barged, and the agent tries to transfer the task to me, I cannot accept the task because I am already a participant. A few thoughts on how we might be able to handle this situation:

  1. We could disable the transfer button when someone is barged
  2. Using the supervisor's local state, we could remove the barged participant within the beforeAcceptTask action, to allow task acceptance to work (this might be the best solution?)
  3. The conversation-transfer feature could perform validation before executing the transfer (but we would need a way to tie the transfer target worker SID to a conversation participant's identity, which could be complex)
  4. We could prevent an available status from being used by the supervisor while barged

@aestellwag
Copy link
Contributor Author

Great catches @dremin , here are the changes made on this commit:

  • I've added logic to handle if an agent transfers the conversation (warm or cold) to a supervisor that is already on the conversation via the chat barge feature. This is done by adding a listener beforeAcceptTask and checking if they are part of the conversation already (this is done leveraging redux, which is used in another portion of the chat barge code). It will remove them from the conversation, then it will accept the task + add them to the conversation per the conversation transfer logic/feature
  • I also noticed some UI render bugs with the participant total when playing around with this, I patched that part of the conversation transfer code and was no longer able to reproduce that issue

Please let me know if you see anything else, it's been fun squashing these bugs/adding safeguards for these edge cases (hence why I'm up at 10pm writing code, haha)

@dremin
Copy link
Contributor

dremin commented Apr 2, 2024

Some more issues I noticed testing the latest changes:

  • In the participant list, the supervisor is listed as a customer participant
  • After transferring to a supervisor who was already barged, I noticed a few issues:
    • The participants list shows 1 participant; only the customer participant is shown
    • If the supervisor attempts to send a message, it does not appear
    • Sometimes it will say "you are not a chat participant"
  • When joining a chat from the teams view, the button briefly changes to say "leaving" instead of "joining"

…ssue after transfer was a timing issue and requires you "refresh" or relaunch the task. I was able to reproduce and fix each of the issues you identified.
@aestellwag
Copy link
Contributor Author

Great catch, took me some time to get back to patching this but with the latest merge I was no longer able to reproduce the bugs you surfaced (I was able to originally reproduce them). I found another bug in there as I was testing around the Leave/Join redux state not cleaning up properly when a supervisor was added.

Give this a test and let me know if any of you see any other bugs.

@dremin
Copy link
Contributor

dremin commented May 29, 2024

with the latest merge I was no longer able to reproduce the bugs you surfaced (I was able to originally reproduce them).

Yay!!

Give this a test and let me know if any of you see any other bugs.

  • Now the button correctly says "Joining" when joining a chat, but when leaving a chat, the button also says "Joining".
  • If the agent warm transfers the chat to a supervisor, and the supervisor goes to the Teams view, the "Join" button is incorrectly enabled if the agent's task for the same conversation is selected.
    • It looks like you have some code in there for checking if the supervisor worker is a participant, so maybe that can be reused for fixing this.
    • The "Join" button is correctly disabled when the supervisor's task is selected, however.
    • Clicking this "Join" button seems to expose a state management issue--it switches to "Leave" even though the serverless call returned a 409. I took a brief look at this code and it seems that the success of the serverless call is not checked before updating state.

Comment on lines 26 to +34
const replaceEndTaskButton = (task: ITask) => {
if (TaskHelper.isCBMTask(task) && task.taskStatus === 'assigned') {
const interactionParticipants =
props?.store?.getState()?.[reduxNamespace]?.supervisorBargeCoach?.interactionParticipants;
// more than two participants or are there any active invites?
const conversationState = StateHelper.getConversationStateForTask(task);
if (
conversationState &&
(conversationState.participants.size > 2 || countOfOutstandingInvitesForConversation(conversationState))
(interactionParticipants > 2 || countOfOutstandingInvitesForConversation(conversationState))
Copy link
Contributor

Choose a reason for hiding this comment

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

This would break functionality if the supervisor-barge-coach feature is removed, but not the conversation-transfer feature. This will need to be implemented a different way to not introduce a co-dependency of features.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could maybe approach it with the following

  1. Add a config value in conversation-transfer that checks whether supervisor barge coach is enabled or not (similar example is in custom-transfer-directory with its dependency on conversation-transfer)

  2. Use that config value here to determine whether to extract the participants count from redux or use the old method

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this piece of state is only set from the conversation-transfer feature, and only read by the conversation-transfer feature, so we shouldn't need multiple implementations. This piece of state can probably just live within the conversation-transfer feature, but after I've looked into things some more, I don't think we need to keep this state at all!

I think we should remove useParticipantCountEffect and instead fetch the participant list using existing data in Redux, which seems to be up-to-date and includes supervisor participants. Its location in Redux is flex.chat.conversations[conversationSid].participants. Using this should be a lot more efficient, as the current code makes 8 or so network requests when selecting the task.

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