New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIP Refer support to implement call transfer #1736

Merged
merged 5 commits into from Jan 30, 2017

Conversation

Projects
None yet
5 participants
@vmykhailiuk

vmykhailiuk commented Jan 24, 2017

No description provided.

@otsakir otsakir added the Peer Review label Jan 24, 2017

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Jan 25, 2017

Thanks @vmykhailiuk , I will review as soon as possible

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Jan 26, 2017

@otsakir can you please take a quick look at the Admin UI part of the PR?

@gvagenas gvagenas requested review from gvagenas and otsakir Jan 26, 2017

@gvagenas gvagenas self-assigned this Jan 26, 2017

@gvagenas gvagenas added this to the 8.1.0 milestone Jan 26, 2017

@gvagenas

Hi @vmykhailiuk ,

Please see my comments her. Important is that there is no response for the Refer request and thus we have re-transimitions.

George

@@ -399,6 +410,9 @@ public VoiceInterpreter(final Configuration configuration, final Sid account, fi
this.fallbackMethod = fallbackMethod;
this.statusCallback = statusCallback;
this.statusCallbackMethod = statusCallbackMethod;
this.referUrl = referUrl;
this.referMethod = referMethod;
this.referTarget = referTarget;

This comment has been minimized.

@gvagenas

gvagenas Jan 27, 2017

Collaborator

@vmykhailiuk referTarget should be in the download parameters (method parameters()) to be send to application server. This way the application server can create RCML that dials the transfer target <Dial>${referTarget}</Dial>

@otsakir can you please add this as a variable at RVD?

This comment has been minimized.

@otsakir
}
builder.setReferMethod((number.getReferMethod() != null && number.getReferMethod().length() > 0) ? number.getReferMethod() : "POST");
builder.setReferTarget(request.getHeader("Refer-To").replace("<", "").replace(">", ""));

This comment has been minimized.

@gvagenas

gvagenas Jan 27, 2017

Collaborator

@vmykhailiuk better don't parse string, prefer using SIP URI like :
((SipURI)request.getAddressHeader("Refer-To").getURI()).getUser()

@@ -649,6 +653,134 @@ private void info(final SipServletRequest request) throws IOException {
}
}
private void transfer(SipServletRequest request) throws Exception {

This comment has been minimized.

@gvagenas

gvagenas Jan 27, 2017

Collaborator

@vmykhailiuk there is no response to the incoming Refer request and as a result there are several retransmitions of the REFER request and later when session expires there is an expection also.

As we have the Call actor, the IncomingPhoneNumber and the Refer url it will be safe to send 202 Accepted and Refer Notify 100 Trying.

listOfRelatedCalls = (List<ActorRef>) answer;
}
if(logger.isInfoEnabled()) {

This comment has been minimized.

@gvagenas

gvagenas Jan 27, 2017

Collaborator

@vmykhailiuk better change these log statements to match the transfer operation here instead of LCM

@otsakir

otsakir requested changes Jan 27, 2017 edited

@vmykhailiuk , in IncomingPhoneNumbersEndpoint:createFrom() it would be better to initialize the refer* properties like for other types of requests (voice, sms, etc). For example here is how this is done for ussd:

builder.setUssdUrl(getUrl("UssdUrl", data));
builder.setUssdMethod(getMethod("UssdMethod", data));        builder.setUssdFallbackUrl(getUrl("UssdFallbackUrl", data));
builder.setUssdFallbackMethod(getMethod("UssdFallbackMethod",data));
builder.setUssdApplicationSid(getSid("UssdApplicationSid",data));
<div class="col-md-2" style="padding-right: 0;">
<h4 class="voice-video-refer-request">Refer Request</h4>
</div>
<rc-endpoint-url details-loaded="numberDetails.$resolved" sid-var="numberDetails.refer_application_sid" url-only-var="false" method-var="numberDetails.refer_method" url-var="numberDetails.refer_url" apps="localReferApps"></rc-endpoint-url>

This comment has been minimized.

@otsakir

otsakir Jan 27, 2017

Contributor

If applications listed in Refer request are by definition voice application, why not use directly the localVoiceApps variable?

Thus the <rc-endpoint-url /...> will contain:
apps="localVoiceApps"

@@ -70,6 +70,7 @@ rcMod.controller('NumberDetailsCtrl', function ($scope, $stateParams, $location,
$scope.localVoiceApps = Applications.filterByKind(localApps, 'voice');
$scope.localSmsApps = Applications.filterByKind(localApps, 'sms');
$scope.localUssdApps = Applications.filterByKind(localApps, 'ussd');
$scope.localReferApps = Applications.filterByKind(localApps, 'voice');

This comment has been minimized.

@otsakir

otsakir Jan 27, 2017

Contributor

$scope.localReferApps can be removed if localVoiceApps are used in numbers-incoming-details.html

@vmykhailiuk

This comment has been minimized.

vmykhailiuk commented Jan 27, 2017

@gvagenas, @otsakir,
Please review updated version with comments taken into account.

@gvagenas

Besides the comment here, I will prepare a binary to test and will update here.

IncomingPhoneNumber number = storage.getIncomingPhoneNumbersDao().getIncomingPhoneNumber(cdr.getTo());
if (number.getReferUrl() == null && number.getReferApplicationSid() == null) {
SipServletResponse servletResponse = request.createResponse(SC_TEMPORARILY_UNAVAILABLE, "Set either incoming phone number Refer URL or Refer application");

This comment has been minimized.

@gvagenas

gvagenas Jan 29, 2017

Collaborator

@vmykhailiuk I think that if referUrl is null we should response with a more appropriate error such as 404 or 488.

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Jan 30, 2017

@vmykhailiuk thanks for the updates, I reviewed and accepted them. I made some modification to make the transfer method more robust and I will push to vmykhailiuk-refer branch and then merge with master.
Please update your branch with origin vmykhailiuk-refer.

@gvagenas gvagenas merged commit 57b4a11 into RestComm:master Jan 30, 2017

@gvagenas gvagenas removed the Peer Review label Jan 30, 2017

@ivelin

This comment has been minimized.

Contributor

ivelin commented Feb 11, 2017

Congrats on a fantastic high end enterprise comms feature. Just read the blog announcement. 🥇

... and made a few comments on UX :) @otsakir @ipsilantide @deruelle
https://telestax.com/call-transfer-restcomm/#comment-1399

@deruelle

This comment has been minimized.

Member

deruelle commented Feb 12, 2017

@ivelin

– Wouldn’t it make more UX sense for Refer to be a branch off of a Voice app. The way its implemented does not seem intuitive. At a high level we have applications triggered by the type of media – Voice, SMS, USSD. I can see in the future Video (with screen sharing, file transfer and similar visual components in RVD), Location (geo-fence related RVD components), Refer does not seem to fit here. I would expect it to be sort of an exception handling branch in a voice app. Also using the technical term Refer in the UX can be confusing to the RVD user. Wouldn’t Call Transfer/On Hold or similar commonly used terms be more appropriate for the UI?

The screenshot has not been updated but yes it's already like this. I asked @gvagenas to update the screenshot on slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment