feat: Added timeout support for some Telephony package's dialogs #1479

luislera · 2023-03-08T19:09:42Z

Purpose

This PR includes timeout support for the following dialogs:

Aggregate DTMF (n)
Aggregate input Regex
Aggregate DTMF (#)

The timeout timer will be initialized when the dialog begins. If the user sends a valid input but does not meet the batch length, termination char or regex expression, the timer will start again.

Changes

Added the Timeout, Default Value and Default Value Response properties
Updated the schema files
Updated the package README file

Tests

No tests added. The existing tests were executed successfully.

#minor

stevengum · 2023-03-09T19:35:54Z

packages/Telephony/Readme.md

@@ -120,19 +120,22 @@ The Stop Recording action stops recording of the conversation. Note that it is n
 ## **Aggregate DTMF Input (n)**
 Prompts the user for multiple inputs that are aggregated until a specified character length is met or exceeded.
 Speech, DTMF inputs, and chat provided characters can all be used to provide input, but any inputs that aren't the characters 1,2,3,4,5,6,7,8,9,0,#,*, or some combination of said characters are dropped.
+In case the timeout in milliseconds is reached, the dialog will end and show the Default Value property to the user.


"Show the Default Value property to the user" could be reworded better to indicate that the Default Value property will be spoken to the user, if that's the intent.

Hi Steven, sure!. As requested, we will be doing additional changes to this PR. If you want I can let you know as soon as we pushed them.

Hey @luislera, I synced with Jim and agree with his requested changes, please disregard my comment for now and perform his changes. After his reqs are complete, I'll take another look at the PR ☺️

jameslew · 2023-03-22T15:58:41Z

@stevengum , @ninilang - can you review this? Has added the additional experience changes we discussed (splitting the default message from the default value, supporting speech field, etc).

stevengum · 2023-03-24T19:51:11Z

packages/Telephony/Actions/BatchRegexInput.cs

+            BotAdapter adapter = dc.Context.Adapter;
+            ConversationReference conversationReference = dc.Context.Activity.GetConversationReference();


These explicit typings are unnecessary and don't align with the rest of the changes, can you use var instead?

stevengum · 2023-03-24T20:26:08Z

packages/Telephony/Readme.md

@@ -120,23 +120,27 @@ The Stop Recording action stops recording of the conversation. Note that it is n
 ## **Aggregate DTMF Input (n)**
 Prompts the user for multiple inputs that are aggregated until a specified character length is met or exceeded.
 Speech, DTMF inputs, and chat provided characters can all be used to provide input, but any inputs that aren't the characters 1,2,3,4,5,6,7,8,9,0,#,*, or some combination of said characters are dropped.
+A timeout timer will be initialized when the dialog begins or when the user sent a response that has not meet or exceed the batch length.


The Timeout parameter isn't required, and in the code when the timeOutInMilliseconds value is null or less than 0, no timer is set.

The first part of the sentence is inaccurate and should be updated to reflect the functionality of the code.

If we want to make the timeout a requirement, there should be a default value of probably 20-40s and it needs to be signed off by @jameslew. I support the idea of making the timeout a required parameter due to the headless experience of phone calls, where a user might be distracted and forget what the bot asked, and the user has no easy way to ask the bot to repeat itself.

This IVR experience is different from the experience of interacting with a bot by typing messages over WebChat, etc.

For the current functionality in the code, a more accurate pair of statements would be:

When the Timeout parameter is set an integer greater than 0, a timer will be set whenever the Aggreate DTMF Input(n) node begins. This timer will be reset whenever the user responds to the bot, until the expected batch length is met.

stevengum · 2023-03-24T20:29:19Z

packages/Telephony/Actions/BatchRegexInput.cs

@@ -120,6 +151,19 @@ public override async Task<DialogTurnResult> ContinueDialogAsync(DialogContext d
            }
            else
            {
+                //If we didn't timeout then we have to manage our timer somehow.
+                //For starters, complete our existing timer.
+                var timerId = dc.State.GetValue<string>("this.TimerId");


Please turn this string into a private const.

stevengum · 2023-03-24T20:34:26Z

packages/Telephony/Actions/BatchRegexInput.cs

+                //If we didn't timeout then we have to manage our timer somehow.
+                //For starters, complete our existing timer.
+                var timerId = dc.State.GetValue<string>("this.TimerId");
+
+                //Should never happen but if it does, it shouldn't be fatal.
+                if (timerId != null)
+                {
+                    await stateMatrix.CompleteAsync(timerId).ConfigureAwait(false);
+                }
+
+                // Restart the timeout timer
+                await InitTimeoutTimerAsync(dc, cancellationToken).ConfigureAwait(false);


A timer isn't always set for this prompt, because users aren't required to provide a timeOutInMilliseconds value, which is the precusor to a timerId being generated. So timerId can be null.

It seems that placing await InitTimeoutTimerAsync(dc, cancellationToken).ConfigureAwait(false); inside of the if(timerId != null) { } block would be better for human readability, and a slight optimization.

stevengum · 2023-03-24T20:46:17Z

packages/Telephony/Actions/BatchRegexInput.cs

+        /// <summary>
+        /// Gets or sets the default value for the input dialog when a Timeout is reached.
+        /// </summary>
+        /// <value>
+        /// Value or expression which evaluates to a value.
+        /// </value>
+        [JsonProperty("defaultValue")]
+        public ValueExpression DefaultValue { get; set; }
+
+        /// <summary>
+        /// Gets or sets the activity template to send when a Timeout is reached and the default value is used.
+        /// </summary>
+        /// <value>
+        /// An activity template.
+        /// </value>
+        [JsonProperty("defaultValueResponse")]
+        public ITemplate<Activity> DefaultValueResponse { get; set; }


It seems to me that it should be straightforward to add a unit test verifying the functionality tied to defaultValue and defaultValueResponse when timeOutInMilliseconds is some small number, e.g., 1.

When timeOutInMilliseconds is 1, the bot should ask the user its prompt, then immediately send any defaultValueResponse and end with the defaultValue in the DialogTurnResult.

Please add a test that covers this scenario.

tracyboehrer · 2023-05-16T16:27:36Z

Is this still active?

luislera · 2023-05-16T18:08:23Z

Is this still active?

Hi Tracy, I think it is still pending the PR's revision.

auto-assign bot assigned luislera Mar 8, 2023

pr-triage bot added the PR: unreviewed label Mar 8, 2023

luislera requested a review from jameslew March 9, 2023 13:28

stevengum reviewed Mar 9, 2023

View reviewed changes

Added support to timeout for the DTMF dialogs

ff0bbc5

luislera removed the request for review from jameslew March 15, 2023 19:12

luislera force-pushed the southworks/feature/telephony-package branch from 3d1bd9f to ff0bbc5 Compare March 15, 2023 19:13

luislera requested a review from stevengum March 15, 2023 19:24

stevengum suggested changes Mar 24, 2023

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Mar 24, 2023

luislera added 2 commits March 28, 2023 17:23

Applied requested changes

e628363

Added test cases

8e5fc0f

pr-triage bot added PR: unreviewed and removed PR: reviewed-changes-requested labels Mar 28, 2023

luislera requested a review from stevengum March 28, 2023 20:46

Update BatchInputTests.cs

5e7b6a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added timeout support for some Telephony package's dialogs #1479

feat: Added timeout support for some Telephony package's dialogs #1479

luislera commented Mar 8, 2023 •

edited

Loading

stevengum Mar 9, 2023

luislera Mar 9, 2023

stevengum Mar 9, 2023

jameslew commented Mar 22, 2023

stevengum Mar 24, 2023

stevengum Mar 24, 2023

stevengum Mar 24, 2023

stevengum Mar 24, 2023

stevengum Mar 24, 2023

stevengum Mar 24, 2023

tracyboehrer commented May 16, 2023

luislera commented May 16, 2023

		BotAdapter adapter = dc.Context.Adapter;
		ConversationReference conversationReference = dc.Context.Activity.GetConversationReference();

feat: Added timeout support for some Telephony package's dialogs #1479

Are you sure you want to change the base?

feat: Added timeout support for some Telephony package's dialogs #1479

Conversation

luislera commented Mar 8, 2023 • edited Loading

Purpose

Changes

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslew commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tracyboehrer commented May 16, 2023

luislera commented May 16, 2023

luislera commented Mar 8, 2023 •

edited

Loading