-
Notifications
You must be signed in to change notification settings - Fork 215
Issue 2073 - Organization phase 1 #2431
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
Conversation
…ault) in init and also update default entry #2073
* master: Documentation for issue #1939 Minor fix Revert SMPP SMS encoding to GSM. And use -d encoding for API. Patch to support dialling a registered client if request comes from external network This close #2086 added comment fixed #2011, cc: #1939 removed unused import unignored added some useful tests updated test modify calltool to getCall from another account's resource #1939 set account sid to the account sid of from client account #1939 completed test for #1939 register sub-account-client with restcomm access cdr from subaccount teporarily ignored rest of test to test new one added shallow test added data for sub-account and sub-account-client Revert "added space" added space Conflicts resolved in: restcomm/restcomm.testsuite/src/test/resources/restcomm.script_callLifecycleTest
* master: (22 commits) Fixes for testsuite for LCM This refer to #1135 Update MGCP driver to 6.0.22 This refer to #2015 added DTMF_DETECTOR_TONE_DURATION=80 patched #2024 https://telestax.atlassian.net/browse/RESTCOMM-841 cs fix #2024 cs fix #2024 patched #2024 #2015 Set Media Server version to 5.1.0.43 added author #2015 Updated MGCP driver version to 6.0.23. patched #1988 #2015 Updated RMS version to 7.0.8. added config for rtp timout and start time params #1821 #1822 updated ascii docs #1135 #1907 #1312 completed mute unmute test #1135 #1907 #1312 added mute functionality for call added Mute param in Call Tools test #1135 #1312 #1907 added 2 more phones added shallow test structure added mute/unmute in callsendpoint #1135 ...
…, updated getClientByLogin to include organization sid #2106
* master: Removing Dialogic libraries and enabling automatic download at runtime, when XMS is configured. Closes #2384. Issue #2406: Minor cleanup, save messageType as local var Revert "Preliminar structure to download Dialogic libraries for XMS at runtime. Issue #2384." Preliminar structure to download Dialogic libraries for XMS at runtime. Issue #2384. Preliminar structure to download Dialogic libraries for XMS at runtime. Issue #2384. fix issue that creates loop in USSD PUSH
* master: updated sip servlet build
* master: Revert "Patch for Restcomm `acting-as-proxy` feature to provide option to disable SDP patching This close #2391" Revert "Patch for Restcomm acting as a proxy to disable SDP patching. This refer to #2391" Revert "Patch for Restcomm acting as a proxy to disable SDP patching. This refer to #2391" Revert "Patch for Restcomm acting as a proxy to disable SDP patching. This refer to #2391" Revert "Patch for Restcomm acting as a proxy to disable SDP patching. This refer to #2391" Revert "Patch for Restcomm acting as a proxy to disable SDP patching. This refer to #2391" Revert "Patch for Restcomm acting as a proxy to disable SDP patching. This refer to #2391" Issue #2411: Workaround: Add end transitions to processingInfo Issue #2411: Workaround: Send Bye when received DownloaderResponse in wrong State Conflicts resolved: restcomm/restcomm.commons/src/main/java/org/restcomm/connect/commons/util/SdpUtils.java restcomm/restcomm.telephony/src/main/java/org/restcomm/connect/telephony/CallManager.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq I provided my comments.
I run few tests and some of them fail, such as the:
- org.restcomm.connect.testsuite.telephony.ClientMessageTest
- org.restcomm.connect.testsuite.telephony.ClientsDialTest
Can you please check and also start a new CI build to check the testsuite?
Last, how it will be the migration of existing clients, IncomingPhoneNumbers etc? I guess it will be done manualy, right?
|
||
final String hostname = configuration.getString("hostname"); | ||
Organization organization = storage.getOrganizationsDao().getOrganization(new Sid(defaultOrganization)); | ||
if(organization != null){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq Shouldn't we create a new Organization in case the default one is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes agreed, done!
@@ -27,19 +28,20 @@ CREATE USER SA PASSWORD "" | |||
GRANT DBA TO SA | |||
SET WRITE_DELAY 10 | |||
SET SCHEMA PUBLIC | |||
INSERT INTO "restcomm_accounts" VALUES('ACae6e420f425248d6a26948c17a9e2acf','2012-04-24 00:00:00.000000000','2012-04-24 00:00:00.000000000','[email protected]','Default Administrator Account',NULL,'Full','uninitialized','77f8c12cc7b8f8423e5c38b035249166','Administrator','/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf') | |||
INSERT INTO "restcomm_organizations" VALUES('ORafbe225ad37541eba518a74248f0ac4c', 'default.restcomm.com', '2017-04-19 00:00:00.000000000','2017-04-19 00:00:00.000000000') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq I believe the default Organization domain name should be configurable at retcomm.xml, so Bootstrapper will create a default Org in case it's missing from the DB.
This way we can cover cloud deployment and on-premise deployments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvagenas it is configureable from restcomm.xml, we are using hostname
properly as domain name. on bootstap RC will update this domain name and set it using hostname
properly
/** | ||
* @author [email protected] (Maria Farooq) | ||
*/ | ||
public class DNSUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq I guess you provided this utility class just to help you with testing, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
context.setAttribute(InstanceId.class.getName(), instanceId); | ||
monitoring.tell(instanceId, null); | ||
RestcommConfiguration.getInstance().getMain().setInstanceId(instanceId.getId().toString()); | ||
|
||
generateDefaultDomainName(xml.subset("http-client"), storage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq If the default Domain Name generation fails nothing will happen. I think here we should either stop RC or try to recover from this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes agreed, done!
} | ||
}catch(Exception e){ | ||
logger.error("", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq we should provide a description to the log message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
dao.removeIncomingPhoneNumber(new Sid(sid)); | ||
return noContent().build(); | ||
}catch(Exception e){ | ||
logger.error("", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq we should provide a description to the log message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
return status(BAD_REQUEST).entity("21452").build(); | ||
}catch(Exception e){ | ||
logger.error("", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq we should provide a description to the log message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
IncomingPhoneNumber number = numbers.getIncomingPhoneNumber(to); | ||
final IncomingPhoneNumbersDao numbersDao = storage.getIncomingPhoneNumbersDao(); | ||
List<IncomingPhoneNumber> numbers = numbersDao.getIncomingPhoneNumber(to); | ||
IncomingPhoneNumber number = numbers.get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq The IncomingPhoneNumber number
here is used only for the logging statement, unless if I am missing something.
Shouldn't be better to remove the lookup and save resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! removed. done
@@ -500,13 +504,23 @@ private void invite(final Object message) throws IOException, NumberParseExcepti | |||
final ApplicationsDao applications = storage.getApplicationsDao(); | |||
// Try to find an application defined for the client. | |||
final SipURI fromUri = (SipURI) request.getFrom().getURI(); | |||
Sid sourceOrganizationSid = OrganizationUtil.getOrganizationSidBySipURIHost(storage, fromUri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq I think this part should be handled withn a lot of precaution.
If INVITE arrives from NEXMO or a sip endpoint that uses SIP URI, sourceOrganizationSid
will be null and thus the log statements below will throw exception, several operations down below will also fail.
Unless I am missing something here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvagenas as discussed on chat it is safe from NPE
@@ -538,8 +552,14 @@ private void invite(final Object message) throws IOException, NumberParseExcepti | |||
logger.info("mediaExternalIp: " + mediaExternalIp); | |||
logger.info("proxyIp: " + proxyIp); | |||
} | |||
|
|||
final Client toClient = clients.getClient(toUser); | |||
Sid toOrganizationSid = OrganizationUtil.getOrganizationSidBySipURIHost(storage, (SipURI) request.getTo().getURI()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maria-farooq similar to the previous comment, the toOrganizationSid
should be carefully handled becaue next log statement will throw NPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvagenas as discussed on chat it is safe from NPE
* master: Change CDR status to lowercase 'completed' when 'updateInCompleteCallDetailRecordsToCompletedByInstanceId' applies. Issue #2162
…for null organization based on from URI
* master: Push server HTTP request Content-Type is "application/json"
@gvagenas i have take care of all comments and also test suit is green now |
Telscale PR
https://bitbucket.org/telestax/telscale-restcomm/pull-requests/33/issue-2073-organization-phase-1/diff
test suit results:
https://mobicents.ci.cloudbees.com/job/RestComm-For-Specific-Branch/127/