issue #2189 #2211

muhammadbilal19 · 2017-06-09T03:30:37Z

Add Pagination code for IncomingPhoneNumber API and UI. Please review and let me know with your feedback.

Add pagination changes for IncomingPhoneNumbers API Add pagination changes on number-incoming.html

Change pagination footer like 1 to 10 of 1000 RestComm#2189

gvagenas

Hi @muhammadbilal19, thanks for your contribution, great job.
Here are my comments:

You need to provide a Mybatis mapper for MySQL along with the one you provided for HSQLDB. The MySQL mapper have to be at restcomm.application/src/main/webapp/WEB-INF/scripts/mariadb/sql
You have to make test with a live MySQL DB to make sure the mapper and the DAO are working fine with MySQL
You have to provide test cases in the testsuite for the IncomingPhoneNumber pagination feature. Check how it is done for the Calls API org.restcomm.connect.testsuite.http.CallsEndpointTest

Thanks
George

muhammadbilal19 · 2017-06-09T09:10:34Z

Thanks @gvagenas i will complete requested changes.

jaimecasero · 2017-06-09T10:10:57Z

restcomm/restcomm.application/src/main/webapp/WEB-INF/sql/incoming-phone-numbers.xml

@@ -68,7 +68,21 @@
    <if test="phoneNumber != null">
      AND "n"."phone_number" like #{phoneNumber}
    </if>
+    LIMIT #{limit} OFFSET #{offset}


pagination filtering done at DB level should provide better performance, good!!

Fixes RestComm#2175

jaimecasero · 2017-06-09T10:12:50Z

...comm/restcomm.http/src/main/java/org/restcomm/connect/http/IncomingPhoneNumbersEndpoint.java

+
+        int limit = Integer.parseInt(pageSize);
+        int offset = (page == "0") ? 0
+                : (((Integer.parseInt(page) - 1) * Integer.parseInt(pageSize)) + Integer.parseInt(pageSize));


can we reduce the number of times we convert incoming queryString params into proper type for validation?? For performance and error handling point of view good be much better...

Thanks @jaimecasero very nice point, changes are now available 7957dff
Feel free to comment if you have any other point :)

- null is returned and the error is treated as a weak-password case since AccountsEndpoint:469 mutes all errors Fixes RestComm#2175

…validation_error_logging Issue2175 js password validation error logging

Add pagination changes for IncomingPhoneNumbers API Add pagination changes on number-incoming.html

Change pagination footer like 1 to 10 of 1000 RestComm#2189

…omm-Connect into issue-2189

…nbound call before MS has send respond to CRCX This close RestComm#2180

Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

…ne Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189" This reverts commit ccf03b6.

Add RestCommIncomingPhone tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

Add pagination changes for IncomingPhoneNumbers API Add pagination changes on number-incoming.html

Change pagination footer like 1 to 10 of 1000 RestComm#2189

Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

…ne Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189" This reverts commit ccf03b6.

Add RestCommIncomingPhone tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

…omm-Connect into issue-2189

ammendonca

@muhammadbilal19 I ahve found a couple of issues with the UI:

When clicking on the items per page, the selected option doesn't show as selected after clicking somewhere else on the page, only if clicked twice on the same option:
- Clicking on 25 and then somewhere else on the page:
- After clicking again on 25 and then somewhere else on the page:
Sorting only works at first, after changing the number of items per page, it stops working:

muhammadbilal19 · 2017-06-13T20:49:19Z

@ammendonca i will try to fix these issues today

muhammadbilal19 · 2017-06-14T09:52:40Z

@ammendonca items per page button and sorting issue is fixed for this PR. Please have a look and let me know your comments

ammendonca · 2017-06-14T17:16:46Z

Thanks @muhammadbilal19, the mentioned issues are now fixed. A (new?) minor issue seems to be present tho, having 12 numbers, with the list with 10 per page, on the page 2 it shows "3 to 4 of 12 Numbers". Not sure this was present before and I haven't noticed.

scottbarstow · 2017-06-14T18:03:38Z

@*ammendonca @gvagenas *can we get this deployed on cloud asap?

…

On Wed, Jun 14, 2017 at 1:16 PM, Alexandre Mendonça < ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2211 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtjo-i2kyjec4NGcaM916b45h7oXD2Fks5sEBWIgaJpZM4N03TK> .

-- scottbarstow.com @scottbarstow +1-919-349-9473 (m)

muhammadbilal19 · 2017-06-14T20:51:24Z

@ammendonca Thanks for the update. I just tried to reproduce that minor issue which you mentioned but i am unable to reproduced it. Not sure why it is happening on your side.

ammendonca · 2017-06-14T21:33:44Z

@muhammadbilal19 I just checked and you need to sort by numbers ascending for it to show. Actually looking at the sorted list, it doesn't seem to be properly sorted:

@scottbarstow for me it's fine, I've approved the PR already. The issue I've found about record numbering is minor and not blocking.. the fact that the list isn't correctly sorted might be more relevant in cases where we have long list of numbers.

muhammadbilal19 · 2017-06-15T00:19:25Z

@ammendonca yes i am able to see that issue now. I am not a really good UI developer and tester so bear with me :) I guess it would be good if we create another issue for sorting issue and once @gvagenas completed the review and approved that PR merge pagination changes only.
If we need to fix sorting issue with this PR i am working on it and will commit it once it is fixed.
@scottbarstow @deruelle thoughts??

scottbarstow · 2017-06-15T00:24:00Z

I'd rather not do two releases so if we can fix it while @gvagenas reviews then just merge to the existing PR prior to merge to master that's ideal. Right now I'd roll it in with this issue if @ammendonca is ok with it. If it's a stopper, then we fix now. His call

…

On Wed, Jun 14, 2017 at 8:19 PM, Muhammad Bilal ***@***.***> wrote: @ammendonca <https://github.com/ammendonca> yes i am able to see that issue now. I am not a really good UI developer and tester so bear with me :) I guess it would be good if we create another issue for sorting issue and once @gvagenas <https://github.com/gvagenas> completed the review and approved that PR merge pagination changes only. If we need to fix sorting issue with this PR i am working on it and will commit it once it is fixed. @scottbarstow <https://github.com/scottbarstow> @deruelle <https://github.com/deruelle> thoughts?? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2211 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtjow4SY_4ox9P3xEe967gaXL8HfldMks5sEHiOgaJpZM4N03TK> .

-- scottbarstow.com @scottbarstow +1-919-349-9473 (m)

ammendonca · 2017-06-15T01:36:46Z

@muhammadbilal19 Just tested and you can fix the sort issue by replacing sort in <tr class="number-row" ng-repeat="pn in filtered = (numbersList | filter:query| orderBy:sort:reverse)"> by predicate which is the actual variable containing the field to sort by.

As per the "3 to 4 of 12 Numbers", the issue is with page_size returned when the Reverse=true is used. With Reverse=false the page_size (even for the last page, which has 2 entries) is 10. But if we pass Reverse=true the page_size returned is 2. That page_size is also used to generate the returned start and end params which are used to show that text.

Hope it helps!

@scottbarstow I'm OK to go with current status, but just provided the solution to the most critical UI issue, so that should likely make it in.

ammendonca · 2017-06-15T01:38:31Z

restcomm/restcomm.ui/src/main/webapp/modules/numbers-incoming.html

            <th class="width-10pc">&nbsp;</th>
            </thead>
            <tbody>
-            <tr class="number-row" ng-repeat="pn in filtered = (numbersList | filter:query)">
+            <tr class="number-row" ng-repeat="pn in filtered = (numbersList | filter:query| orderBy:sort:reverse)">


Replace sort by predicate which is where the field to sort is stored at (from https://github.com/RestComm/Restcomm-Connect/pull/2211/files#diff-b0cba56bc32531ce8ec4f5174dcd4fe6R97)

…2189

muhammadbilal19 · 2017-06-15T08:22:33Z

Thanks @ammendonca for writing the solution I committed my latest changes i made some other changes as well so please have and look and let me know if looks good to you now.Sorting issue and other minor issue both are now fixed

gvagenas · 2017-06-15T10:13:57Z

Great work @muhammadbilal19, thanks!

gvagenas · 2017-06-15T12:07:13Z

@muhammadbilal19 can you please create a new pull request for the same issue since there was a problem during the merge and I had to revert ?

muhammadbilal19 · 2017-06-15T13:49:19Z

@gvagenas sure i am creating new PR give me few minutes

gvagenas · 2017-06-15T14:05:22Z

Great, thanks @muhammadbilal19 . Make sure you merge master into your branch to have the latest changes

muhammadbilal19 added 2 commits June 9, 2017 11:56

RestComm#2189

5b319d7

Add pagination changes for IncomingPhoneNumbers API Add pagination changes on number-incoming.html

Add sorting by friendly_name and phone_number

c795e8a

Change pagination footer like 1 to 10 of 1000 RestComm#2189

otsakir added the Peer Review label Jun 9, 2017

muhammadbilal19 mentioned this pull request Jun 9, 2017

Paging Capabilities for IncomingPhoneNumbers API #2189

Closed

deruelle requested review from gvagenas and brainslog June 9, 2017 07:41

gvagenas suggested changes Jun 9, 2017

View reviewed changes

jaimecasero reviewed Jun 9, 2017

View reviewed changes

Logging for JS-based password validation mechanism failures

5baed55

Fixes RestComm#2175

jaimecasero reviewed Jun 9, 2017

View reviewed changes

otsakir and others added 19 commits June 9, 2017 13:22

Restored error handling rational in case ScriptException is thrown

81f7f11

- null is returned and the error is treated as a weak-password case since AccountsEndpoint:469 mutes all errors Fixes RestComm#2175

Merge pull request RestComm#2214 from RestComm/issue2175_js_password_…

908cc98

…validation_error_logging Issue2175 js password validation error logging

Mybatis mapper for MySQL for pagination change RestComm#2189

2ad79f4

RestComm#2189

ba3d58d

Add pagination changes for IncomingPhoneNumbers API Add pagination changes on number-incoming.html

Add sorting by friendly_name and phone_number

3f2f964

Change pagination footer like 1 to 10 of 1000 RestComm#2189

Mybatis mapper for MySQL for pagination change RestComm#2189

17ab793

Merge branch 'issue-2189' of https://github.com/muhammadbilal19/Restc…

94fd7ea

…omm-Connect into issue-2189

Patch for proper MGCP DLCX for when a Call actor receives CANCEL on i…

89e9cbd

…nbound call before MS has send respond to CRCX This close RestComm#2180

Merge branch 'master' of github.com:RestComm/Restcomm-Connect

0847f34

Adding test cases for pagination functionality of IncomingPhone

ccf03b6

Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

Revert "Adding test cases for pagination functionality of IncomingPho…

17f5252

…ne Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189" This reverts commit ccf03b6.

Adding test cases for pagination functionality of IncomingPhone

b0181e7

Add RestCommIncomingPhone tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

RestComm#2189

b8ff71f

Add pagination changes for IncomingPhoneNumbers API Add pagination changes on number-incoming.html

Add sorting by friendly_name and phone_number

fec68a7

Change pagination footer like 1 to 10 of 1000 RestComm#2189

Mybatis mapper for MySQL for pagination change RestComm#2189

72c0642

Adding test cases for pagination functionality of IncomingPhone

4622be8

Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

Revert "Adding test cases for pagination functionality of IncomingPho…

cdd03dd

…ne Add RestComm tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189" This reverts commit ccf03b6.

Adding test cases for pagination functionality of IncomingPhone

d9405e9

Add RestCommIncomingPhone tools class to remove some repeating code. Replace System.out to logger.info IncomingPhoneNumbersEndpointTest RestComm#2189

Merge branch 'issue-2189' of https://github.com/muhammadbilal19/Restc…

be69ba4

…omm-Connect into issue-2189

ammendonca self-requested a review June 13, 2017 15:41

ammendonca suggested changes Jun 13, 2017

View reviewed changes

muhammadbilal19 mentioned this pull request Jun 13, 2017

Pagination for client api and dashboard #2154 #2228

Open

Fixed page size button and sorting issue RestComm#2189

ad11263

ammendonca approved these changes Jun 14, 2017

View reviewed changes

ammendonca reviewed Jun 15, 2017

View reviewed changes

Implement sorting with db query and start and end value fix RestComm#…

f79e0e7

…2189

gvagenas approved these changes Jun 15, 2017

View reviewed changes

gvagenas closed this Jun 15, 2017

gvagenas reopened this Jun 15, 2017

gvagenas merged commit bbaf3f6 into RestComm:master Jun 15, 2017

muhammadbilal19 deleted the issue-2189 branch June 15, 2017 10:38

gvagenas mentioned this pull request Jun 15, 2017

Revert PR for "issue #2189" #2237

Merged

muhammadbilal19 restored the issue-2189 branch June 15, 2017 13:47

ammendonca mentioned this pull request Jun 20, 2017

Unplanned work for "Athena" RestComm/olympus#121

Closed

gvagenas added this to the George 8.2.0 Sprint 4 milestone Jun 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #2189 #2211

issue #2189 #2211

muhammadbilal19 commented Jun 9, 2017 •

edited

Loading

gvagenas left a comment

muhammadbilal19 commented Jun 9, 2017

jaimecasero Jun 9, 2017

jaimecasero Jun 9, 2017

muhammadbilal19 Jun 12, 2017 •

edited

Loading

ammendonca left a comment

muhammadbilal19 commented Jun 13, 2017

muhammadbilal19 commented Jun 14, 2017

ammendonca commented Jun 14, 2017

scottbarstow commented Jun 14, 2017 via email

muhammadbilal19 commented Jun 14, 2017

ammendonca commented Jun 14, 2017

muhammadbilal19 commented Jun 15, 2017

scottbarstow commented Jun 15, 2017 via email

ammendonca commented Jun 15, 2017

ammendonca Jun 15, 2017

muhammadbilal19 commented Jun 15, 2017

gvagenas commented Jun 15, 2017

gvagenas commented Jun 15, 2017

muhammadbilal19 commented Jun 15, 2017

gvagenas commented Jun 15, 2017

issue #2189 #2211

issue #2189 #2211

Conversation

muhammadbilal19 commented Jun 9, 2017 • edited Loading

gvagenas left a comment

Choose a reason for hiding this comment

muhammadbilal19 commented Jun 9, 2017

jaimecasero Jun 9, 2017

Choose a reason for hiding this comment

jaimecasero Jun 9, 2017

Choose a reason for hiding this comment

muhammadbilal19 Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

ammendonca left a comment

Choose a reason for hiding this comment

muhammadbilal19 commented Jun 13, 2017

muhammadbilal19 commented Jun 14, 2017

ammendonca commented Jun 14, 2017

scottbarstow commented Jun 14, 2017 via email

muhammadbilal19 commented Jun 14, 2017

ammendonca commented Jun 14, 2017

muhammadbilal19 commented Jun 15, 2017

scottbarstow commented Jun 15, 2017 via email

ammendonca commented Jun 15, 2017

ammendonca Jun 15, 2017

Choose a reason for hiding this comment

muhammadbilal19 commented Jun 15, 2017

gvagenas commented Jun 15, 2017

gvagenas commented Jun 15, 2017

muhammadbilal19 commented Jun 15, 2017

gvagenas commented Jun 15, 2017

muhammadbilal19 commented Jun 9, 2017 •

edited

Loading

muhammadbilal19 Jun 12, 2017 •

edited

Loading