[Home]

Summary:ASTERISK-11189: [patch] GotoIfTime currently only handles the 'true' case. This patch adds proper Goto behavior for the 'false' case as well.
Reporter:Kenneth Shumard (kshumard)Labels:
Date Opened:2008-01-09 17:06:04.000-0600Date Closed:2008-02-15 17:03:52.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) gotoiftime.twobranches.patch
Description:The principle of least surprise says that GotoIfTime should function like GotoIf, and branch properly down either the 'true' or the 'false' path. However, when (e.g.) exten => 1,1,GotoIfTime(15:00-15:02,*,*,*?true:false) is evaluated, the current behavior is that if true the label "true:false" is used and if false processing continues to the next priority. This patch implements the typical GotoIf behavior, by properly handling labels for both "true" and "false" evaluations of the current time against the specified time. Thus, true evaluation of the condition Goto's the "true" label, and false evaluation of the condition Goto's the "false" label.

The idea of a label "true:false" makes me shudder, but this is valid with the current behavior of GotoIfTime.

****** STEPS TO REPRODUCE ******

exten => 1,1,GotoIfTime(15:00-15:02,*,*,*?true:false)
exten => 1,n,Noop(Normal priority processing)
exten => 1,n,Hangup
exten => 1,n(true),Noop(Hit "true" label from GotoIfTime)
exten => 1,n,Hangup
exten => 1,n(false),Noop(Hit "false" label from GotoIfTime)
exten => 1,n,Hangup
exten => 1,n(true:false),Noop(Ewww... what an ugly label)
exten => 1,n,Hangup


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

This is a defensive programming practice. The ability to fork in either direction doesn't really add a whole lot of exciting functionality, but it does add consistency and potentially reduce confusion about how the function should work.

FWIW, Jared Smith and I both assumed in a Bootcamp class today that GotoIfTime _already_ behaved the way this patch implements.
Comments:By: Jared Smith (jsmith) 2008-01-09 17:12:45.000-0600

Looks great to me!
By: Tilghman Lesher (tilghman) 2008-01-10 12:29:38.000-0600

kshumard:  what happens if the first application contains an argument with a ':' character?
By: Kenneth Shumard (kshumard) 2008-01-10 13:01:53.000-0600

Corydon76: Do you mean "label" instead of "application"? What will happen if there's a ':' in the label is that Asterisk won't behave as expected. With an invocation like

exten => 1,1,GotoIfTime(15:00-15:02,*,*,*?tr:ue:false),

the true condition will branch to label "tr" and the false condition will branch to "ue:false". Not exactly pretty, but this makes it consistent with GotoIf which already behaves this way. I copied much of the logic for this patch straight out of pbx_builtin_gotoif.
By: Tilghman Lesher (tilghman) 2008-01-10 14:06:02.000-0600

Oh, duh.  I was thinking about ExecIfTime.
By: Jason Parker (jparker) 2008-01-17 13:50:15.000-0600

Looks good.  Any reason not to commit?
By: Digium Subversion (svnbot) 2008-02-15 17:03:52.000-0600

Repository: asterisk
Revision: 103738

U   trunk/main/pbx.c

------------------------------------------------------------------------
r103738 | mmichelson | 2008-02-15 17:03:51 -0600 (Fri, 15 Feb 2008) | 9 lines

Add proper "false" case behavior to GotoIfTime

(closes issue ASTERISK-11189)
Reported by: kshumard
Patches:
     gotoiftime.twobranches.patch uploaded by kshumard (license 92)
 Tested by: kshumard


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

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