[Home]

Summary:ASTERISK-13354: queue-thankyou should be played only if needed
Reporter:caspy (caspy)Labels:
Date Opened:2009-01-13 07:27:55.000-0600Date Closed:2009-03-13 16:27:09
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 14227_v2.patch
( 1) 14227_v3.patch
( 2) 14227.patch
Description:In say_position() in app_queue.c there is a command to play 'queue-thankyou' sound:
 res = play_file(qe->chan, qe->parent->sound_thanks);

This sound is played unconditionally, but should be played only in case if position or holdtime was played.
Current code is generating 'queue-thankyou' without anything else, for example, in case of position turned off + holdtime turned on, and hold time less than a 1 minute.

****** ADDITIONAL INFORMATION ******

Also, if holdtime is 1 minute exactly, than time is not announced, but 'queue-holdtime' phrase played.
It seems that you should change this lines:
               if (avgholdmins > 1) {...
               if (avgholdsecs > 1) {...
to:
               if (avgholdmins > 0) {...
               if (avgholdsecs > 0) {...
so, than even 1 minute/second would be played too.
Comments:By: Leif Madsen (lmadsen) 2009-01-13 12:46:27.000-0600

This seems like possibly an easy fix. Assigned to otherwiseguy, but please reassign to putnopvut should you be unable to take this issue on.

Thanks!
By: Leif Madsen (lmadsen) 2009-01-20 14:42:00.000-0600

I've changed this status to 'feature' as I believe this kinda falls under the title of feature request, but I certainly see the value in this, so I'll leave it up to otherwiseguy to determine the fate of this issue :)
By: Mark Michelson (mmichelson) 2009-02-02 15:18:32.000-0600

This is one of those issues where I need to be careful not to be disruptive with a behavioral change.

I can certainly understand caspy's point about having what is seemingly an out-of-place "Thank you for your patience" message play when not announcing a hold time or position. I also have to keep in mind that this prompt is completely configurable from within queues.conf. It is quite possible that administrators count on a file to always play and say something like "Thank you for calling. Your call is important to us and we will be with you as shortly as possible" instead of the default sound.

Now, as I mentioned, the queues.conf file can be configured to play whatever sound it is that you want for queue-thankyou. The default is to use the sound of the same name, but you can actually specify tt-weasels or one of your own home-rolled files if you want. What this also means is that you can set it as

queue-thankyou=

This explicitly tells the queue application not to play a sound when it wants to play queue-thankyou. With the current state of the code, this will work and there will be no adverse consequences from doing so, but you will see warnings on the CLI because the file '' will not be able to be played when app_queue tries to play queue-thankyou.

I will attach a patch here which will make it so that app_queue will only try to play the queue-thankyou sound if the sound that it should be playing is not a zero-length string. I'll also add a note to the queues.conf.sample file noting that if you wish to not play a specific sound, you can do so as I showed above by setting the option to a zero-length argument.

Your note in the additional information about when the minute is singular is absolutely right and I will include an update for that logic in the patch I upload.
By: Mark Michelson (mmichelson) 2009-02-02 16:09:11.000-0600

I've uploaded 14227.patch. This contains

* An explicit check in app_queue's play_file function to return early if attempting to play a file whose name is zero-length

* A change to the comparison of avgholdmins in the say_announcement function so that hold times between 1 minute and 2 minutes may be reported.

* A change to the queues.conf.sample file which makes it clear that you may disable a sound from being played by specifying a zero-length value for the option.

Give it a test to be sure it does what you want and let me know if you think the update to queues.conf.sample is clear enough. Thanks!
By: caspy (caspy) 2009-02-03 03:38:09.000-0600

i'll test patch in production in a few days.
By: caspy (caspy) 2009-02-03 09:43:44.000-0600

Tested.

Now, in case of turned ON holdtime announce and holdtime == 00:00,
we have a 'queue-thankyou' sound played without(!) holdtime announce.
It's not good.

So, i have a suggestion: wrap playing of 'queue-thankyou' like this:
if ( !((avgholdmins+avgholdsecs) = 0 && qe->parent->announceholdtime) ) {
 res = play_file(qe->chan, qe->parent->sound_thanks);
}

and after this current logic will be just the same:
- in case of turned OFF holdtime announcement, 'queue-thankyou' always played;
- in case of turned ON holdtime announcement and holdtime>0, holdtime and 'queue-thankyou' played together;
- in case of turned ON holdtime announcement and holdtime=0, nothing played (and it is good! 'queue-thankyou' should not be played too, cause estimated holdtime is very-very low. it is useless and annoying).
By: Mark Michelson (mmichelson) 2009-02-03 10:23:44.000-0600

Ah, I see what you're saying now. It's not enough to just put a blank queue-thankyou in queues.conf because you *do* want the "thank you" sound to be played if a hold time is announced, but not when no holdtime was announced.

I will upload a new patch which incorporates your idea.
By: Mark Michelson (mmichelson) 2009-02-03 10:35:49.000-0600

I have uploaded a new patch, 14227_v2.patch which has everything from 14227.patch, plus now the queue-thankyou sound will only play if holdtime was announced or if the position in the queue was announced to the caller.

You'll notice I didn't do this exactly as you described because for one thing, I wanted to still play the sound for position announcements, and there's already a block of code that executes for holdtime announcements, so I placed the queue-thankyou sound playback there.

Let me know how it works for you!
By: caspy (caspy) 2009-02-03 11:16:37.000-0600

Hmm...

I did not tried 14227_v2.patch, but i think this is completly what we do not want.
Look,
- if you turn on announcement of position and holdtime together, 'thank you' will be played twice one by one.
- if you set to announce holdtime and holdtime == 0, 'thank you' still will be played.
- this breaks yours idea to play 'thank you' when both position and holdtime turned off (i mean idea about 'your call is important for us').


Please, look once again for my previous suggestion. It is more expectable in its behavior for any user.
By: Mark Michelson (mmichelson) 2009-02-03 11:25:32.000-0600

You're right. I misread one of the gotos in the section for announcing position. Scrap that patch.

note to self: don't write code early in the morning without triple-checking its accuracy
By: Mark Michelson (mmichelson) 2009-02-05 18:54:02.000-0600

I tried going with your method for my next effort at this patch, but gcc issued warnings that avgholdmins and avgholdsecs could be used uninitialized, so I went to work to make a different version. I've uploaded the new version as 14227_v3.patch.

In this version, there is a say_thanks variable which governs whether we play the "thank you" file. By default, it is set to 1. We will set it to 0 if we do not play a hold time, we do have announce-holdtime set, and we don't have announce-position set.

This way, the thank you sound will play under the following conditions:

1. announcefrequency is set to some value but holdtime and position are not set to be played.
2. a position announcement was played.
3. a holdtime was played.

It will not play if holdtime is set to be played but we did not actually announce a holdtime.
By: caspy (caspy) 2009-02-06 02:10:16.000-0600

Greetings!

i've looked through v3, and it seems to be ok. i'll test it for a few days in my production, and post results here.

Thank you!
By: caspy (caspy) 2009-02-11 13:36:44.000-0600

Yes! It's work excellent.
By: Digium Subversion (svnbot) 2009-02-11 17:08:01.000-0600

Repository: asterisk
Revision: 174948

U   trunk/apps/app_queue.c

------------------------------------------------------------------------
r174948 | mmichelson | 2009-02-11 17:03:08 -0600 (Wed, 11 Feb 2009) | 20 lines

Fix odd "thank you" sound playing behavior in app_queue.c

If someone has configured the queue to play an position or holdtime
announcement, then it is odd and potentially unexpected to hear a
"Thank you for your patience" sound when no position or holdtime
was actually announced.

This fixes the announcement so that the "thanks" sound is only played
in the case that a position or holdtime was actually announced.

There is a way that the "thank you" sound can be played without a
position or holdtime, and that is to set announce-frequency to a value
but keep announce-position and announce-holdtime both turned off.

(closes issue ASTERISK-13354)
Reported by: caspy
Patches:
     14227_v3.patch uploaded by putnopvut (license 60)
Tested by: caspy

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=174948
By: Digium Subversion (svnbot) 2009-02-11 17:10:50.000-0600

Repository: asterisk
Revision: 174949

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_queue.c

------------------------------------------------------------------------
r174949 | mmichelson | 2009-02-11 17:04:10 -0600 (Wed, 11 Feb 2009) | 27 lines

Merged revisions 174948 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r174948 | mmichelson | 2009-02-11 17:03:08 -0600 (Wed, 11 Feb 2009) | 35 lines
 
 Fix odd "thank you" sound playing behavior in app_queue.c
 
 If someone has configured the queue to play an position or holdtime
 announcement, then it is odd and potentially unexpected to hear a
 "Thank you for your patience" sound when no position or holdtime
 was actually announced.
 
 This fixes the announcement so that the "thanks" sound is only played
 in the case that a position or holdtime was actually announced.
 
 There is a way that the "thank you" sound can be played without a
 position or holdtime, and that is to set announce-frequency to a value
 but keep announce-position and announce-holdtime both turned off.
 
 (closes issue ASTERISK-13354)
 Reported by: caspy
 Patches:
       14227_v3.patch uploaded by putnopvut (license 60)
 Tested by: caspy
 ................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=174949
By: Digium Subversion (svnbot) 2009-02-11 17:11:34.000-0600

Repository: asterisk
Revision: 174950

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_queue.c

------------------------------------------------------------------------
r174950 | mmichelson | 2009-02-11 17:11:34 -0600 (Wed, 11 Feb 2009) | 27 lines

Merged revisions 174948 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r174948 | mmichelson | 2009-02-11 17:03:08 -0600 (Wed, 11 Feb 2009) | 20 lines
 
 Fix odd "thank you" sound playing behavior in app_queue.c
 
 If someone has configured the queue to play an position or holdtime
 announcement, then it is odd and potentially unexpected to hear a
 "Thank you for your patience" sound when no position or holdtime
 was actually announced.
 
 This fixes the announcement so that the "thanks" sound is only played
 in the case that a position or holdtime was actually announced.
 
 There is a way that the "thank you" sound can be played without a
 position or holdtime, and that is to set announce-frequency to a value
 but keep announce-position and announce-holdtime both turned off.
 
 (closes issue ASTERISK-13354)
 Reported by: caspy
 Patches:
       14227_v3.patch uploaded by putnopvut (license 60)
 Tested by: caspy
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=174950
By: caspy (caspy) 2009-03-03 16:08:22.000-0600

while trying to do the best patch, we have missed useful and important changes from '14227.patch'.
it should be submitted to svn too (in addition(!) to successfully commited 14227_v3.patch)
By: Mark Michelson (mmichelson) 2009-03-03 16:40:00.000-0600

Yes, you're right. I'll add the change to queues.conf.sample.
By: caspy (caspy) 2009-03-03 16:43:11.000-0600

Not only to queues.conf.sample, but also to app_queue.c (comparation of file name to zero, and comparation avgholdmins with ones).
By: Mark Michelson (mmichelson) 2009-03-03 16:45:59.000-0600

Actually, the avgholdmins comparison was fixed. Now we check if it is > 0 instead of > 1. THis is the same as >= 1 as was in 14227.patch. I'll also add the zero-length filename check.
By: Digium Subversion (svnbot) 2009-03-03 16:48:19.000-0600

Repository: asterisk
Revision: 180006

U   branches/1.4/apps/app_queue.c
U   branches/1.4/configs/queues.conf.sample

------------------------------------------------------------------------
r180006 | mmichelson | 2009-03-03 16:48:18 -0600 (Tue, 03 Mar 2009) | 17 lines

Clarify some documentation of queues.conf.sample

It had always been possible to explicitly specify a "blank"
value for a sound file in queues.conf and have no sound played
back. The problem with this is that it would result in some ugly
CLI warnings from file.c.

This commit introduces a check when playing a file in app_queue
to see if the name of the file is zero-length and return early if
that is the case. Also, the ability to specify the blank sound
files in queues.conf is now mentioned more clearly in queues.conf.sample

(closes issue ASTERISK-13354)
Reported by: caspy



------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=180006
By: Digium Subversion (svnbot) 2009-03-03 16:49:09.000-0600

Repository: asterisk
Revision: 180007

_U  trunk/
U   trunk/apps/app_queue.c
U   trunk/configs/queues.conf.sample

------------------------------------------------------------------------
r180007 | mmichelson | 2009-03-03 16:49:08 -0600 (Tue, 03 Mar 2009) | 22 lines

Merged revisions 180006 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r180006 | mmichelson | 2009-03-03 16:48:18 -0600 (Tue, 03 Mar 2009) | 17 lines
 
 Clarify some documentation of queues.conf.sample
 
 It had always been possible to explicitly specify a "blank"
 value for a sound file in queues.conf and have no sound played
 back. The problem with this is that it would result in some ugly
 CLI warnings from file.c.
 
 This commit introduces a check when playing a file in app_queue
 to see if the name of the file is zero-length and return early if
 that is the case. Also, the ability to specify the blank sound
 files in queues.conf is now mentioned more clearly in queues.conf.sample
 
 (closes issue ASTERISK-13354)
 Reported by: caspy
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=180007
By: Digium Subversion (svnbot) 2009-03-03 16:49:33.000-0600

Repository: asterisk
Revision: 180008

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_queue.c
U   branches/1.6.0/configs/queues.conf.sample

------------------------------------------------------------------------
r180008 | mmichelson | 2009-03-03 16:49:32 -0600 (Tue, 03 Mar 2009) | 29 lines

Merged revisions 180007 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r180007 | mmichelson | 2009-03-03 16:49:07 -0600 (Tue, 03 Mar 2009) | 22 lines
 
 Merged revisions 180006 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r180006 | mmichelson | 2009-03-03 16:48:18 -0600 (Tue, 03 Mar 2009) | 17 lines
   
   Clarify some documentation of queues.conf.sample
   
   It had always been possible to explicitly specify a "blank"
   value for a sound file in queues.conf and have no sound played
   back. The problem with this is that it would result in some ugly
   CLI warnings from file.c.
   
   This commit introduces a check when playing a file in app_queue
   to see if the name of the file is zero-length and return early if
   that is the case. Also, the ability to specify the blank sound
   files in queues.conf is now mentioned more clearly in queues.conf.sample
   
   (closes issue ASTERISK-13354)
   Reported by: caspy
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=180008
By: Digium Subversion (svnbot) 2009-03-03 16:49:52.000-0600

Repository: asterisk
Revision: 180009

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_queue.c
U   branches/1.6.1/configs/queues.conf.sample

------------------------------------------------------------------------
r180009 | mmichelson | 2009-03-03 16:49:51 -0600 (Tue, 03 Mar 2009) | 29 lines

Merged revisions 180007 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r180007 | mmichelson | 2009-03-03 16:49:07 -0600 (Tue, 03 Mar 2009) | 22 lines
 
 Merged revisions 180006 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r180006 | mmichelson | 2009-03-03 16:48:18 -0600 (Tue, 03 Mar 2009) | 17 lines
   
   Clarify some documentation of queues.conf.sample
   
   It had always been possible to explicitly specify a "blank"
   value for a sound file in queues.conf and have no sound played
   back. The problem with this is that it would result in some ugly
   CLI warnings from file.c.
   
   This commit introduces a check when playing a file in app_queue
   to see if the name of the file is zero-length and return early if
   that is the case. Also, the ability to specify the blank sound
   files in queues.conf is now mentioned more clearly in queues.conf.sample
   
   (closes issue ASTERISK-13354)
   Reported by: caspy
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=180009
By: caspy (caspy) 2009-03-03 16:55:00.000-0600

sorry for second reopen, but it seems we missunderstand each other.

look for code (from 1.6.0 branch):

1899     if ((avgholdmins+avgholdsecs) > 0 && qe->parent->announceholdtime &&
...
1906 if (avgholdmins > 1) {
...
1911 if (avgholdmins == 1) {

in line 1899 - comparation ok (>0), but in 1906 and 1911 lines - wrong (>1).

By: caspy (caspy) 2009-03-13 07:04:11

mentioned line 1911 - is my mistype. correct:

1899     if ((avgholdmins+avgholdsecs) > 0 && qe->parent->announceholdtime &&
...
1906 if (avgholdmins > 1) {
...
1921 if (avgholdsecs > 1) {

in line 1899 - comparation ok (>0), but in 1906 and 1921 lines - wrong (>1).
By: Digium Subversion (svnbot) 2009-03-13 16:26:21

Repository: asterisk
Revision: 182121

U   trunk/apps/app_queue.c

------------------------------------------------------------------------
r182121 | mmichelson | 2009-03-13 16:26:21 -0500 (Fri, 13 Mar 2009) | 6 lines

Change faulty comparison used when announcing average hold minutes and seconds

(closes issue ASTERISK-13354)
Reported by: caspy


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=182121
By: Digium Subversion (svnbot) 2009-03-13 16:26:47

Repository: asterisk
Revision: 182122

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_queue.c

------------------------------------------------------------------------
r182122 | mmichelson | 2009-03-13 16:26:47 -0500 (Fri, 13 Mar 2009) | 12 lines

Merged revisions 182121 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r182121 | mmichelson | 2009-03-13 16:26:20 -0500 (Fri, 13 Mar 2009) | 6 lines
 
 Change faulty comparison used when announcing average hold minutes and seconds
 
 (closes issue ASTERISK-13354)
 Reported by: caspy
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=182122
By: Digium Subversion (svnbot) 2009-03-13 16:27:08

Repository: asterisk
Revision: 182123

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_queue.c

------------------------------------------------------------------------
r182123 | mmichelson | 2009-03-13 16:27:08 -0500 (Fri, 13 Mar 2009) | 12 lines

Merged revisions 182121 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r182121 | mmichelson | 2009-03-13 16:26:20 -0500 (Fri, 13 Mar 2009) | 6 lines
 
 Change faulty comparison used when announcing average hold minutes and seconds
 
 (closes issue ASTERISK-13354)
 Reported by: caspy
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=182123