Ashley Sanders has posted comments on this change. Change subject: Rewrite sip_attended_transfer test to stop failing. ......................................................................
Patch Set 2: Code-Review-1 (6 comments) Overall, this is pretty good. The flow was mostly easy to follow. I only have a few findings with respect to some opportunities to use abstractions (that could help reduce the complexity of working around the caveats of the pjsua api). https://gerrit.asterisk.org/#/c/19/2/tests/channels/SIP/sip_attended_transfer/attended_transfer.py File tests/channels/SIP/sip_attended_transfer/attended_transfer.py: Line 36: class BobCallCallback(pj.CallCallback): Just as something to mention, BobCallCallback and CarolCallCallback can be reduced into one class, with the function to callback as a ctor parameter. The transfer object itself is not needed. Line 86: self.bridge1 = None Another fyi - The bridge1 and bridge2 could be made into a class and you could have a attributes on the class, bridge_id and 'active' or 'is_bridged' to hold the state of the object, rather than having 2 variables for each bridge. I can see this pattern as being a little bit cleaner and easier to read if anyone ever has to come back and revisit this in the future. Line 121: self.call_carol() I think that you are calling this twice, once in the BobCallCallback.on_state handler (ln 48) callback and here again, on the ami.bridge_enter handler. Line 132: self.transfer_call() Here, too, this seems to be called twice; once in the CarolCallCallback.on_state handler (ln 69) and again here, in the ami.bridge_enter handler. Line 154: if (self.state == BOB_CALLED and self.bridge1_bridged and I don't think you need the BOB_CALLED state; if bob's call is up, then you know that Bob has made his call. Line 162: if (self.state == CAROL_CALLED and self.bridge2_bridged and I don't think you need the CAROL_CALLED state; if carol's call is up, then you know that Carol has made her call. -- To view, visit https://gerrit.asterisk.org/19 To unsubscribe, visit https://gerrit.asterisk.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1676801d90bcafc28ba25e8b6889f40ab08cc90e Gerrit-PatchSet: 2 Gerrit-Project: testsuite Gerrit-Branch: master Gerrit-Owner: Mark Michelson <[email protected]> Gerrit-Reviewer: Ashley Sanders <[email protected]> Gerrit-HasComments: Yes -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
