-
Notifications
You must be signed in to change notification settings - Fork 387
Task with state machine #4562
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
base: task-refactor
Are you sure you want to change the base?
Task with state machine #4562
Conversation
4cbbe04 to
a92618d
Compare
a92618d to
5b416f6
Compare
| on: { | ||
| [TaskEvent.UNHOLD]: { | ||
| target: TaskState.CONNECTED, | ||
| cond: 'canResume', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we really need canResume guard check here... i feel it should be allowed irrsepctive right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only allow resume when its on hold , we need some check on the SDK not on the widgets ,
this normally could be enhanced adding intermediate state as well
| public data: TaskData; | ||
| public webCallMap: Record<TaskId, CallId>; | ||
| public taskUiControls: TaskUIControls; | ||
| public state: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small doubt, which I think we can discuss.
I like that the state is public... so for some cases, say HELD state, is it ok if the user directly accesses the state from this object rather than storing it themsleves? That ways this state can be the source of truth.
Wanted to know if that was also the thinking here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya @adhmenon that was the idea we just expose current state , state machine and intermediate state only gets used internally
| /** | ||
| * Can accept if in OFFERED or OFFERED_CONSULT state | ||
| */ | ||
| canAccept: (context: TaskContext, event: any, meta: {state: {value: StateValue}}): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not recommend using any in the code, so please assign a type to the event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
| * Validate state transition | ||
| * Returns true if transition from current state to target state is valid | ||
| */ | ||
| export function isValidTransition(currentState: TaskState, targetState: TaskState): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need separate function to validate the state transitions? State Machine takes care of validating them on its own and blocks the transitions from happening if not valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was just a helper method, not getting used will remove it
| // Determine UI control states based on current state and context | ||
| const isOffered = state.matches(TaskState.OFFERED) || state.matches(TaskState.OFFERED_CONSULT); | ||
| const isConnected = state.matches(TaskState.CONNECTED); | ||
| const isHeld = state.matches(TaskState.HELD); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is context being used here to determine the UI control state? From what I understand we are only verifying if the current state matches the passed state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no context is not getting used here. just the state , context only comes into picture for complex flows , there is only 3 or 4 or them , will be used more in conference
| * task.hold().then(()=>{}).catch(()=>{}) | ||
| * ``` | ||
| * */ | ||
| public async hold(): Promise<TaskResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have we changed this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a issue we expose hold in webrtc and task but voice only has holdResume and not hold, resume , not sure how we missed it , whats your thoughts on it ?
| const shouldHold = !this.data.interaction.media[this.data.mediaResourceId].isHold; | ||
|
|
||
| // Validate operation is allowed in current state | ||
| const state = this.stateMachineService?.state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the significance of this code, if we have state machine in place that should block the API invocation too instead of we need to add if else condition to throw error. This should be controlled via state machine itself
| */ | ||
| public async pauseRecording(): Promise<TaskResponse> { | ||
| // Validate recording is active | ||
| const context = this.stateMachineService?.state?.context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here
| public async consult(consultPayload?: ConsultPayload): Promise<TaskResponse> { | ||
| // Validate consult is allowed | ||
| const state = this.stateMachineService?.state; | ||
| const canConsult = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a canConsult guard as well as we are calculating the same thing here again using state machine. Feels redundant
| /** | ||
| * Can only start conference if consult destination agent has joined | ||
| */ | ||
| canStartConference: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditions that we have in these guards can be just be directly put in the actions itself that are being invoked from each of the state transitions and update context based on conditions
| }); | ||
|
|
||
| // Send event to task's state machine | ||
| taskWithStateMachine.stateMachineService.send(stateMachineEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change the second argument to payload
| public async pauseRecording(): Promise<TaskResponse> { | ||
| this.unsupportedMethodError('pauseRecording'); | ||
|
|
||
| return Promise.reject(new Error('pauseRecording not supported for this channel type')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use throw error rather then returning
Promise.reject
| * ``` | ||
| */ | ||
| public get taskUiControls(): TaskUIActions { | ||
| // Convert computed UI controls to TaskActionControl objects for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this , make sure we publish event on each UI control change , its will have UI control object
| * | ||
| * @returns UI control states for all task actions | ||
| */ | ||
| protected computeUIControls(): TaskUIControls { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returned object can be a loop one liner
| * This replaces the old updateUIControlsFromState() method. | ||
| * Returns plain objects instead of mutating taskUiControls. | ||
| */ | ||
| protected computeUIControls(): import('../Task').TaskUIControls { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix this
| // Consult transfer: visible during consulting | ||
| consultTransfer: { | ||
| visible: isConsulting, | ||
| enabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabled/show/visable /display :
supported
check on the naming
| // Validate operation is allowed in current state | ||
| const state = this.stateMachineService?.state; | ||
| if (state) { | ||
| const currentState = state.value as TaskState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this logic inside the hold function and resume function
| * Create initial context for a new task | ||
| */ | ||
| export function createInitialContext(): TaskContext { | ||
| return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc on how the context can be used for a specific feature , comment out all the unused values
| * Initialize task with offer data | ||
| */ | ||
| initializeTask: assign((context: TaskContext, event: TaskEventPayload) => { | ||
| if (isEventOfType(event, TaskEvent.OFFER) || isEventOfType(event, TaskEvent.OFFER_CONSULT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't seem to be invoking this action apart from these 2 events only so why do we have the need to add this condition here ?
|
|
||
| [TaskState.OFFERED]: { | ||
| on: { | ||
| [TaskEvent.ACCEPT]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between ACCEPT and ASSIGN ? Why do we need 2 events here ?
| target: TaskState.CONNECTED, | ||
| actions: ['updateTaskData'], | ||
| }, | ||
| [TaskEvent.RONA]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 events also can be one single event
| [TaskEvent.ACCEPT]: { | ||
| target: TaskState.CONSULTING, | ||
| }, | ||
| [TaskEvent.RONA]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here
| [TaskEvent.CONSULTING_ACTIVE]: { | ||
| actions: ['setConsultAgentJoined'], | ||
| }, | ||
| [TaskEvent.START_CONFERENCE]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start_conference and merge conference are not very different from each other so do we need separate states for it
| private registerTaskListeners() { | ||
| this.webSocketManager.on('message', (event) => { | ||
| const payload = JSON.parse(event); | ||
| if (payload?.keepalive === 'true' || payload?.keepalive === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we need this additional condition ?
| return; | ||
| } | ||
| if (payload?.data?.interaction) { | ||
| payload.data = normalizeTaskData(payload.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this normalizing required ?
| this.updateTaskData(task, payload.data); | ||
| task.emit(TASK_EVENTS.TASK_RECORDING_RESUME_FAILED, task); | ||
| break; | ||
| case CC_EVENTS.AGENT_CONSULT_CONFERENCING: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we remove this because we are going to add conference separately ? Maybe commenting would be better than removing
| this.emit(TASK_EVENTS.TASK_INCOMING, task); | ||
| } | ||
| break; | ||
| case CC_EVENTS.AGENT_OFFER_CONTACT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need other event handling here now that we have state machine taking care of it
| return result; | ||
| } catch (error) { | ||
| // Send failure event to transition back to previous state | ||
| if (this.stateMachineService) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this if condition required everywhere ?
|
Please make the PR ready for review so can run the pipeline too on the changes |
|
add check for normalizer to pass through if its a boolean @rajeshtezu |
|
remove the callbacks from the actions and add enum , rather then callbackl , add one callback |
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.