<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7461">View Change</a></p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for review.<br>For line 15608, used ast_variable_find_in_list() instead of<br>astman_get_header() because "Call-ID" should be in "Variable"<br>header.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Ah I see what you are doing now, but still stand behind my finding.  In this function the AMI "Variables" are being used to add data headers/content to the notify packet.  "Call-ID" is an argument and should be provided in a normal AMI header (read with astman_get_header).  This would also remove the need to skip "Call-ID" when adding variables to the SIP NOTIFY headers.  If you think I'm wrong please explain why it should be in "Variable".</p><p style="white-space: pre-wrap; word-wrap: break-word;">I forgot to mention in the first review, you need to document the new optional parameter in the xml "DOCUMENTATION" comment (around line 583).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Finally the only comment I made which was not addressed is testing.  If you have not created a testsuite test can you please explain how you've tested it?  If you have not yet you should test with REF_DEBUG to ensure you are not leaking SIP dialogs (that would be a major leak).</p><p>(1 comment)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7461/3//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/3//COMMIT_MSG@10">Patch Set #3, Line 10:</a> <code style="font-family:monospace,monospace">with "SIPnotify" AMI action for latest (14.x and 15.x) asterisk.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please remove " for latest (14.x and 15.x) asterisk".</p><p style="white-space: pre-wrap; word-wrap: break-word;">Unless tests are added this feature will not be available for 15.  Asterisk 14 is in security fix only mode so no features/improvements are being accepted to 14.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7461">change 7461</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/7461"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5797ded4752acd966db6b13971284db684cc5ab4 </div>
<div style="display:none"> Gerrit-Change-Number: 7461 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Yasuhiko Kamata <yasuhiko.kamata@nxtg.co.jp> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Yasuhiko Kamata <yasuhiko.kamata@nxtg.co.jp> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 08 Dec 2017 20:51:20 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>