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

Implement message, recording, notification, transcription filter on server side following this issue: https://github.com/RestComm/Restcomm-Connect/issues/1395 #1638

Merged
merged 19 commits into from Mar 16, 2017

Conversation

Projects
None yet
4 participants
@forever3000
Collaborator

forever3000 commented Dec 11, 2016

Fix this issue #1395

@otsakir otsakir added the Peer Review label Dec 11, 2016

@deruelle

This comment has been minimized.

Member

deruelle commented Dec 11, 2016

Thanks @forever3000. @otsakir can you please review ?

@forever3000

This comment has been minimized.

Collaborator

forever3000 commented Dec 12, 2016

Hi @otsakir ,
Sorry for make 3 commits in one pull request. Please help me review them.
Thanks

@otsakir

This comment has been minimized.

Contributor

otsakir commented Dec 13, 2016

@forever3000 ,thanks for submitting the patch. I will review asap and let you know.

@otsakir

This comment has been minimized.

Contributor

otsakir commented Dec 16, 2016

Hi @forever3000.

Due to heavy load, I plan to start reviewing your patch next Wednesday. I hope that's ok with you.

@forever3000

This comment has been minimized.

Collaborator

forever3000 commented Dec 16, 2016

Hi @otsakir sure
Thanks

@otsakir

This comment has been minimized.

Contributor

otsakir commented Dec 22, 2016

Hi @forever3000. Thanks for your patience. I did a first run through your patch. Great work there.

I noticed however there are no integration tests for the added functionality. Obviously because there were no test classes already in place. Also, no documentation is included. You think you could provide those too?

For testing, we mostly do integration testing using arquilian. It's all about adding the test class, override the default restcomm.script with another one from /resources with proper sample data and running the test.

You can get a rough idea of how this works by looking into CallsEndpointTest.java. Here are some more comments to help you get the feel of it.

  • The createWebArchiveNoGw() method at the end builds the custom binary to be tested.
  • All @Test-anotated methods do the actual testing typically by hiting the binary using a REST client and validating the responses.
  • You could also create a tool to host all client requests like RestcommCallsTool or use jersey directly as illustrated in RoleSensitiveTest.java.
  • You could extend EndpointTest to save you from writing some boilerplate code too. It's not much saving though.

Orestis

@forever3000

This comment has been minimized.

Collaborator

forever3000 commented Dec 23, 2016

Hi @otsakir ,

Thank for your review and recommendation. I will try to investigate then provide test code for added functionality. Maybe I will ask something if stuck somewhere.

Thanks and Best Regards,
Vu Nguyen

@forever3000 forever3000 force-pushed the forever3000:master branch from 413e076 to 2218e70 Dec 26, 2016

@forever3000

This comment has been minimized.

Collaborator

forever3000 commented Dec 27, 2016

Hi @otsakir ,

I nearly finish test code and I have two questions.

  1. We use rescomm_withData.script to create test data logs for call, sms, recording,... And it has more than 400 call data logs, so how many message logs need to create is enough as well as notification, transcription, ... Is there any generation tool or we must do it manually?
  2. About the documentation, do you have any form or template?

Thanks and Best Regards,
Vu Nguyen

@otsakir

This comment has been minimized.

Contributor

otsakir commented Dec 29, 2016

@forever3000 sounds great!

Regarding 1,

there is no automated tool to produce these records. I suppose for Calls, they have been copied out of production environments. This is not an option in your case. I would go with taking a sample message/notification/transcriptions record from the script file and replicating it several times. Then, manually, make the sid unique (values like SM00000000000000000000000000000001, SM00000000000000000000000000000002 should work to make your life easier) and also play a little with the dates, day-of-month etc. Then, when it comes to paging, use a small page size in your tests to verify proper operations of paging functionality. If that's the case, you can get away with 30-40 or maybe less records for each entity. It's not the best option but test-wise it covers most aspects i believe. You think that would work ?

Regarding 2,

as you will probably have noticed, restcomm documentation is part of our codebase and is written in asciidoc. The Calls API doc page seems the closest existing part documentation. List filter and Paging Information sections are what you need to supply i think.

Here they are:

https://github.com/RestComm/RestComm-Connect/blob/879c3c07f1235e3b74516b34b0d677059bf5044e/restcomm/restcomm.docs/sources-asciidoc/src/main/asciidoc/api/calls-api.adoc#list-filter

https://github.com/RestComm/RestComm-Connect/blob/879c3c07f1235e3b74516b34b0d677059bf5044e/restcomm/restcomm.docs/sources-asciidoc/src/main/asciidoc/api/calls-api.adoc#paging-information

So, it comes down to filling up the respective *-api.adoc documents under /restcomm/restcomm.docs/sources-asciidoc/src/main/asciidoc.

I hope this helps you move forward. Ping me if you need any more feedback. I will be available tomorrow (friday) and then on Jan 4th.

forever3000 added some commits Jan 1, 2017

Correct parameter name in Sms, Notification, Recording, Transcription…
… converter

Remove debug log in TranscriptionEndpointTest

Fill up document for Sms, Notification, Recording, Transcription log

According to this issue #1395
@forever3000

This comment has been minimized.

Collaborator

forever3000 commented Jan 1, 2017

Hi @otsakir

Thank for your very good recommendations. I have finish test code as well as documentation for sms, recording, notification and transcription filter. Please help me review them.

Thanks and Best Regards,
Vu Nguyen

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 9, 2017

Hi @forever3000!

I've started reviewing your work and will also merge in latest changes from AdminUI revamp. I will get back to you as soon as it's done.

Thanks

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 10, 2017

@forever3000 , the review is almost finished. Master branch has been merged into your code and some fixes have been provided for the UI layer and the testsuite. I've noticed however that there are no Mybatis query files provided for the maria DB layer. Hsql is fine.

You think you could provide these too?

They are located under restcomm/restcomm.application/src/main/webapp/WEB-INF/scripts/mariadb/sql

Other that that, merging went smooth. Great job there!

Btw, you can pull the merged version of the code from forever3000-master branch from upstream repo (https://github.com/RestComm/Restcomm-Connect.git). Better to base your last touch on that if you decide to move forward.

Finally, i'm baking a binary with the latest version of this branch accessible under:
https://mobicents.ci.cloudbees.com/view/RestComm/job/RestComm-For-Specific-Branch/20/
in case it is of any value to you.

Regards

@deruelle

This comment has been minimized.

Member

deruelle commented Jan 24, 2017

@forever3000 any progress on @otsakir latest comments ?

@forever3000

This comment has been minimized.

Collaborator

forever3000 commented Jan 24, 2017

Hi @otsakir ,
Thank for your very good comment. Sorry for a bit late. Please help me review and merge this code.

Hi @deruelle ,
Yes, Already finish.

@otsakir otsakir self-requested a review Jan 25, 2017

@otsakir otsakir added REST API tech_debt and removed tech_debt labels Jan 25, 2017

Removed 'quotes' from mariadb mybatis descriptors
The added queries in the .xml files were copied from the respective hsql ones
and the double quotes in the table names caused trouble.

Refers #1395
@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 25, 2017

@forever3000, your branch has been reviewed and minor fixes applied. Thanks for contributing this!
👍

@gvagenas , the patch is clear from my side. Minor manual testing has been done for mariadb too. You may proceed with merging to master. I've also committed minor fixes on top the head of the PR on main Restcomm repo:

https://github.com/RestComm/Restcomm-Connect/tree/forever3000-master

@otsakir otsakir added the Core engine label Jan 25, 2017

@otsakir otsakir added this to the 8.1.0 milestone Jan 25, 2017

@otsakir

otsakir approved these changes Feb 6, 2017

@deruelle deruelle requested a review from gvagenas Feb 13, 2017

@deruelle

This comment has been minimized.

Member

deruelle commented Feb 14, 2017

@gvagenas can you review as well as it was approved by @otsakir ?

@gvagenas gvagenas merged commit bb3a859 into RestComm:master Mar 16, 2017

@gvagenas gvagenas removed the Peer Review label Mar 16, 2017

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