<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2072/">https://reviewboard.asterisk.org/r/2072/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 24th, 2012, 8:55 p.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is really for the test review, but I wanted to post it on this review since it really is dependent on the behavior in this patch. The following are the 'high-level' scenarios that need to be tested. In each scenario, multiple permutations are possible based on the presence of user profile flags - how in depth we go with this depends on how much we feel these need to be exercised.
Marked users:
Conference should automatically start for Marked users and those users should
never be in an inactive conference state.
Marked/Unmarked users:
Conference should automatically start with two or more Unmarked users, or if a
Marked user enters. An Unmarked user exiting should not send the conference
to an inactive state if the only user left is Marked; otherwise the
conference should go inactive and the user's profile options honored.
Marked/WaitMarked Users:
WaitMarked users should not trigger conference starting. Marked users should
automatically trigger a conference starting. When marked users leave a
conference, WaitMarked users should be put back into an inactive conference.
Marked/Unmarked Users:
Unmarked users should not trigger a conference start unless there are two or
more of them. A Marked user should automatically trigger a conference start.
If an Unmarked user leaves and a Marked user is present and the last person,
the conference should continue. If a Marked user leaves and an Unmarked user
is the last person, the conference should be put into an inactive state.
Unmarked Users:
Unmarked users should not trigger a conference start unless there are two or
more of them. If a conference is down to its last person and the last person
is an Unmarked user, the conference should be put into an inactive state.
Unmarked/WaitMarked Users:
Unmarked and WaitMarked users should never interact. If a WaitMarked user
enters the conference at any point in time, and no Marked user exists in
the conference, the conference should appear as if it were inactive. At
the same time, Unmarked users should not notice the presence of WaitMarked
users who are in an inactive state. Regardless of the number of WaitMarked
users waiting, a single Unmarked user should be put in an inactive state if
they are the only Unmarked user in a conference. If two or more Unmarked
users are in the conference then the conference should be active; if the
count drops to a single Unmarked user then the conference should be in an
inactive state.
WaitMarked users:
Regardless of the number of WaitMarked users who attempt to join the
conference, the conference remains in an inactive state. The conference
remains in the inactive state when WaitMarked users leave.
Unmarked/WaitMarked/Marked users:
As a permutation to the Unmarked/WaitMarked scenario, it should be verified
that when multiple Unmarked users are in a conference with multiple WaitMarked
users waiting to join the conference, the presence of a Marked user causes the
WaitMarked users to join, and the leaving of the last Marked user causes the
WaitMarked users to re-enter an inactive state while the Unmarked users remain
in an active conference.
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Just realized I had Marked/Unmarked in there twice. Whoops! Description differences aside, at least the behavior described was the same between the two.</pre>
<br />
<p>- Matt</p>
<br />
<p>On September 17th, 2012, 11:51 a.m., Terry Wilson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and Neil Tallim.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated Sept. 17, 2012, 11:51 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thank's to flan's tireless testing, issue reporting, and patches it became clear that app_confbridge had some complex logic and was missing some corner cases. In particular, it didn't handle mixing unmarked and waitmarked users very well and was inconsistent in how MOH and muting was applied. flan's patches seem to fix the issue, but they highlight how hard the code could be to maintain. In an attempt to make things easier to maintain and to more fully enumerate the various cases that exist, I have attempted to break up the logic into a state machine-like setup.
This should fix all of the issues in the linked bugs, as well as a couple of other issues. I still need to add some documentation, but I thought I'd give people a chance to look at it test it out, and comment.
One thing to note is that this solution involves treating waitmarked users without a marked user as essentially outside of the conference. For now, I have the announce_user_count stuff taking into account both the counts for waiting and active users which can lead to things like having a single waiting user followed by an unmarked user and the announcement that "there is one other user in the conference" (because we count the waiting user there) followed by "you are the only user in the conference" (because that prompt plays when you are the only active user) being played to the unmarked user. This is easy to fix, we just need to decide which way to go. Count users who are essentially outside the conference (they can't speak or hear) or not?</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Lots of calls. Need to do more formalized testing.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19562">ASTERISK-19562</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19726">ASTERISK-19726</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20181">ASTERISK-20181</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/10/apps/app_confbridge.c <span style="color: grey">(373118)</span></li>
<li>/branches/10/apps/confbridge/conf_state.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_empty.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_inactive.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_marked.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_multi.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_single.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_single_marked.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/include/conf_state.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/include/confbridge.h <span style="color: grey">(373118)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2072/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>