Skip to content

[New] Drop Queue Window Scheduler - #160

Merged
ear-dev merged 4 commits into
masterfrom
drop_queue_window_scheduler
Jul 12, 2022
Merged

ear-dev merged 4 commits into
masterfrom
drop_queue_window_scheduler

Conversation

@AlexanderKanakis

@AlexanderKanakis AlexanderKanakis commented Jun 30, 2022 •

Copy link
Copy Markdown
Collaborator

For issue: WideChat/Rocket.Chat#1225

-Activates 120 second timer when queue window is set as active which drops the queue window.
-Resets timeout to 120 seconds if DF sends payload with continue_blackout parameter.

Comment thread enum/Scheduler.ts Outdated
export enum JobName {
SESSION_MAINTENANCE = 'session-maintenance',
EVENT_SCHEDULER = 'event-scheduler',
QUEUE_WINDOW_SCHEDULER = 'queue-window-scheduler',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we rename it to something relatable? How about CONTINUE_BLACKOUT_TIMEOUT_SCHEDULER?

Comment on lines +14 to +23
public async processor(jobContext: IJobContext, read: IRead, modify: IModify, http: IHttp, persistence: IPersistence): Promise<void> {
const sessionId = jobContext.rid;
try {
await setIsQueueWindowActive(read, persistence, sessionId, false);
} catch (error) {
// Failed to send event, so close blackout window
console.error(`${Logs.DIALOGFLOW_REST_API_ERROR}: { roomID: ${sessionId} } ${getError(error)}`);
}

}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You are trying to setsetIsQueueWindowActive flag to false But the actual flag that blocks user messages is setIsProcessingMessage So we need to use that maybe?

Comment thread lib/payloadAction.ts
Comment on lines +77 to +85
const continueBlackoutTask = {
id: JobName.QUEUE_WINDOW_SCHEDULER,
when: `120 seconds`,
data: {
rid,
sessionId: rid,
jobName: JobName.EVENT_SCHEDULER,
},
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here, the id is JobName.QUEUE_WINDOW_SCHEDULER, but the jobName is JobName.EVENT_SCHEDULER. It should be same right?

Comment thread lib/payloadAction.ts Outdated
if (params.continue_blackout) {
const continueBlackoutTask = {
id: JobName.QUEUE_WINDOW_SCHEDULER,
when: `120 seconds`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the past we faced some issue with passing literal strings here. It was scheduling job with some invalid date. And using Date object works fine https://github.com/WideChat/Apps.Dialogflow/blob/master/lib/payloadAction.ts#L63 Can you please try it?

Comment thread lib/payloadAction.ts Outdated
}
} else if (actionName === ActionIds.DROP_QUEUE) {
await setIsQueueWindowActive(read, persistence, rid, false);
await modify.getScheduler().cancelJobByDataQuery({sessionId : rid, jobName: JobName.QUEUE_WINDOW_SCHEDULER});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the lib/Scheduler.ts file we made some functions to cancel scheduled tasks. i.e. cancelAllEventSchedulerJobForSession We should create one for this scheduler as well.

Comment thread lib/EventTimeoutProcessor.ts Outdated
const queuedMessage = await getQueuedMessage(read, sessionId);

await setIsQueueWindowActive(read, persistence, sessionId, false);
await modify.getScheduler().cancelJobByDataQuery({sessionId, jobName: JobName.QUEUE_WINDOW_SCHEDULER});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We are setting and unsetting setIsQueueWindowActive in this function. The actual flag that blocks user messages is setIsProcessingMessage So maybe we don't need this here?

@ear-dev

ear-dev commented Jul 11, 2022

Copy link
Copy Markdown

@Shailesh351 it looks like Alexander responded to the comments? Please verify when you have a chance. Thanks.

Comment thread lib/EventTimeoutProcessor.ts Outdated
import { Dialogflow } from './Dialogflow';
import { handleResponse } from './payloadAction';
import { getQueuedMessage, getRoomAssoc, retrieveDataByAssociation, setIsProcessingMessage, setIsQueueWindowActive, setQueuedMessage } from './Persistence';
import { cancelAllBlackoutTimeoutSchedulerJobForSession } from './Scheduler';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This import is unused

Comment thread lib/EventTimeoutProcessor.ts Outdated
Comment on lines +33 to +42
const continueBlackoutTask = {
id: JobName.CONTINUE_BLACKOUT_TIMEOUT_SCHEDULER,
when: `120 seconds`,
data: {
rid: sessionId,
sessionId,
jobName: JobName.CONTINUE_BLACKOUT_TIMEOUT_SCHEDULER,
},
};
await modify.getScheduler().scheduleOnce(continueBlackoutTask);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we don't need this. Because in this function when we receive a response from the DF, and if it is having continue_blackout flag we are already scheduling a timer. Can you please check and confirm if it's working. Thanks!

Comment thread lib/QueueWindowProcessor.ts Outdated
import { IJobContext, IProcessor } from '@rocket.chat/apps-engine/definition/scheduler';
import { Logs } from '../enum/Logs';
import { getError } from './Helper';
import { setIsProcessingMessage, setIsQueueWindowActive} from './Persistence';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Import setIsQueueWindowActive is unused

@ear-dev
ear-dev merged commit 31820e0 into master Jul 12, 2022
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