Issue2517 application api with numbers #2550
Conversation
- Updated Application entity - Provided integration test - Some work on ApplicationDao layer Refers #2517
Also updated application.xml in all places in codebase Refers #2517
- Added entity, converter and updated related components - Added unit test for converter Refers #2517
…into issue2517_application_api_with_Numbers_summary
…into issue2517_application_api_with_Numbers_summary
otsakir
changed the title
| @@ -0,0 +1,37 @@ | |||
| CREATE SCHEMA PUBLIC AUTHORIZATION DBA | |||
There was a problem hiding this comment.
We have been discussing on maintenance cost of hsql scripts needed for tests-suit:
imo we should handle a new test's logic by reusing an existing script and add new rows of our desire.
There was a problem hiding this comment.
@maria-farooq , i agree in principle. Is this script in place ? There should be one with guarantees that it won't contain any more INSERTS that may affect the results of other tests.
In any case, i consider what you describe a separate issue altogether.
| @@ -36,6 +36,9 @@ | |||
|
|
|||
| List<Application> getApplications(Sid accountSid); | |||
|
|
|||
| // this may optionally return related numbers as part of an Application | |||
| List<Application> getApplications(Sid accountSid, boolean includeNumbers); | |||
There was a problem hiding this comment.
why dont we create a simple getApplicationsWithNumbers, who is goign to call getApplications(sid,false)??
|
|
||
| final SqlSession session = sessions.openSession(); | ||
| try { | ||
| final List<Map<String, Object>> results = session.selectList(namespace + "getApplicationsAndNumbers", accountSid.toString()); |
There was a problem hiding this comment.
again, the method with boolean flag, and the sql query involved doesnt make much sense. basically "includeNumber==false" is like calling getApplications
| Application app = null; | ||
| for (final Map<String, Object> result : results) { | ||
| app = toApplication(result); | ||
| if (previousApp != null && previousApp.getSid().equals(app.getSid()) ) |
There was a problem hiding this comment.
is this based on query results being order by appId, i didnt see any orderBy clause when the query was specified?
…nsistency in production Refers #2517
| Assert.assertEquals(5, apps.size()); | ||
| Assert.assertNotNull(searchApplicationBySid(new Sid("AP73926e7113fa4d95981aa96b76eca854"),apps).getNumbers()); | ||
| // applications bound with many numbers are property returned | ||
| Assert.assertEquals("Three (3) numbers should be bound to this application", searchApplicationBySid(new Sid("AP73926e7113fa4d95981aa96b76eca854"),apps).getNumbers().size(), 3); |
There was a problem hiding this comment.
i think expectedValue is second argument. If "3" is placed in proper place, there is not much requirement on putting custom message for this assertion, since the default one will be self explanatory
|
testsuite results are positive |
This is about issue #2517 .
The following automated tests have been added/updated:
ApplicationRetrievalTest
ApplicationConverterTest
ApplicationEndpointTest
Also, manual testing has been done on a customized Restcomm-JBoss-AS7-8.2.0.1319 and using both hsql and mysql storage backends.