Issue 2620 #2641

ddhuy · 2017-11-18T14:22:04Z

Now we can be able to change the email address of an account.

deruelle · 2017-11-23T13:16:03Z

@gsaslis can you please help review this one ?

deruelle · 2017-11-23T13:16:48Z

@ddhuy can you please sign the Contributor License Agreement at https://telestax.com/open-source/#Contribute so we can accept the contribution ?

ddhuy · 2017-11-23T15:14:17Z

@deruelle , I have just signed the CLA.

gsaslis

Great start @ddhuy !

Please look into the changes requested and let us know.

gsaslis · 2017-11-23T20:51:36Z

restcomm/restcomm.identity/src/main/java/org/restcomm/connect/identity/EmailValidator.java

+     * @param email: the email address need to be verified
+     * @return true if email format is valid, false if email format is invalid
+     */
+    public static boolean isValidEmailFormat ( String email ) {


I would favour use of an external library that already handles email validations, rather than including our own regex here. Take a look at the JavaMail or the Apache Commons Validator for some ideas.

@gsaslis I have just incorporated your comment using JavaMail API. I agree with you that using external library is better than regex.
And I found in class EmailMessageEndpoint we are using the regex for validating email format, so should we change with our new method or keep it as it is?

@ddhuy yes, i would suggest we include that change as well in this PR.

@gsaslis , I changed to JavaMail as mentioned in class EmailMessageEndpoint as well. Please have a review and share with me your feedback.

gsaslis

In addition to the inline comments, could you please also explain your changes with commit b001a77 ?

It seems the InvalidEmailException we used to have has both been made public but also had its type changed? Would you elaborate on why this was needed please?

gsaslis · 2017-12-07T13:42:13Z

restcomm/restcomm.identity/src/main/java/org/restcomm/connect/identity/EmailValidator.java

+            isValid = true;
+        } catch (AddressException ex) {
+            isValid = false;
+            logger.error("Email " + email + " is invalid");


i would change the log level here to warn (if not info).

(error is usually reserved for severe issues that affect the application health. on the other hand, this is very much a normal operation of handling invalid user input - info sounds like a better fit imho)

@gsaslis Agree with you about the log level. I am prefer changing it to warn. Thanks for your suggestion.
About the b001a77, I see that we had two different classes with exactly same purpose. So I think it should be better if we only have one interface to raise invalid email exception. Later on it will be more easier for maintenance and it is likely to have no duplicate in our code, IMO.

gsaslis · 2017-12-11T14:58:42Z

@ddhuy the problem with commit b001a77 is that you've changed the type of the InvalidEmailException: used to be Exception, now it's RuntimeException.

This means that its usages (e.g.

Restcomm-Connect/restcomm/restcomm.http/src/main/java/org/restcomm/connect/http/EmailMessagesEndpoint.java

Line 91 in e449462

} catch (final InvalidEmailException exception) {

) after your change now only catch a RuntimeException, instead of the more general Exception.

I am not sure this is a valid / intended change... ?

ddhuy · 2017-12-15T16:26:56Z

@gsaslis , Thanks for your reviewing time and comments. I think make the InvalidEmailException sub-class from RuntimeException is more cleaner and easier for usage. But since sub-class from Exception or RuntimeException has not much different, so I will revert it back to keep minimum effect of the change.

…Connect into restcomm-1543

…ema access

This refer to #RESTCOMM-1618

Fixes RestComm#2770

This close #RESTCOMM-1618

This refer to #RESTCOMM-1512

This refer to #RESTCOMM-1618

…d catch clause to catch a Throwable This refer to #RESTCOMM-1618

fixes restcomm1786, unlink only removes given link

fixes restcomm1786 added extension mocks and int test cases

Restcomm1831 adjusted pre/post execpoint consistency

Restcomm 1741

* SDR service event stream implementation * sms implementation * Revert "sms implementation" This reverts commit 0d3a4bd * SDR service sms messages implementation * Revert "SDR service sms messages implementation" This reverts commit 1b464b3 * SDR service sms messages implementation * SDR service sms messages implementation

Fixes RestComm#2858

comment not applicable removed

fixes restcomm1851, constraint about status check

Fixes duplicate jars in classpath

ddhuy · 2018-03-08T18:08:57Z

Hi all, I have tried to resolve the merging conflict. Please help cross check them again

gsaslis · 2018-03-09T10:17:44Z

Thanks @ddhuy !

@jaimecasero / @RestComm/vvs-squad does this look good to merge?

jaimecasero · 2018-03-09T11:27:16Z

i was thinking email is sometimes used as account key, so im not sure making email address modifiable could have some impact. Let me think about it...

maria-farooq

@gsaslis @ddhuy it seems like PR is missing integartion test when email update operation via http request. please check existing tests for accounts endpoint here https://github.com/RestComm/Restcomm-Connect/blob/master/restcomm/restcomm.testsuite/src/test/java/org/restcomm/connect/testsuite/http/AccountsEndpointTest.java

gsaslis suggested changes Nov 23, 2017

View reviewed changes

ddhuy force-pushed the issue_2620 branch from 5612a33 to b001a77 Compare November 30, 2017 15:07

gsaslis suggested changes Dec 7, 2017

View reviewed changes

ddhuy force-pushed the issue_2620 branch 3 times, most recently from b639e23 to 74e55d9 Compare December 8, 2017 18:18

jaimecasero and others added 19 commits January 24, 2018 05:35

Merge branch 'restcomm-1543' of https://github.com/RestComm/Restcomm-…

165be48

…Connect into restcomm-1543

delete profile associations on removal

ab15d23

fixed bullet list, and code section

93c1b38

schemas relocated into specific subresoruce, and sec disabled for sch…

b7b207a

…ema access

fixed test

c5a4e69

profile tables added to all scripts

d16d803

Fixed log statements for NumberSelector service.

addccb1

This refer to #RESTCOMM-1618

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

8fb1c56

Fixed log statements during the method redirectToHostedApp

7d9e537

Add countly support to console

f9bafc1

Fixes RestComm#2770

fixed duplicate profile tables

74d84c2

Patch to protect ExtensionsController action methods from NPE

2d49fbe

This close #RESTCOMM-1618

Fixed log statements during the method redirectToHostedApp

12dc0a1

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

186e2ab

Merge branch 'master' into restcomm-1618

23b6c21

Fixes for Extension API execution points

d4661e2

This refer to #RESTCOMM-1512

Patch to protect ExtensionsController action methods from NPE

f57c3fc

This refer to #RESTCOMM-1618

ignore unstable test

80c01b6

Patch to protect ExtensionsController action methods from NPE. Change…

a5abfba

…d catch clause to catch a Throwable This refer to #RESTCOMM-1618

abdulazizali77 and others added 24 commits February 28, 2018 14:16

Modify mysql profile-assc mapper deleteProfileAssociationByTargetSid

7c93800

Merge pull request RestComm#2850 from RestComm/restcomm1786squash

439b531

fixes restcomm1786, unlink only removes given link

Merge pull request RestComm#2851 from RestComm/restcomm1786

be99803

fixes restcomm1786 added extension mocks and int test cases

Merge pull request RestComm#2852 from RestComm/restcomm1831

0717741

Restcomm1831 adjusted pre/post execpoint consistency

Merge pull request RestComm#2845 from RestComm/RESTCOMM-1741

b05aca3

Restcomm 1741

fixed unlink profile to remove only given assoc

76bd092

Skip authentication for Logout API. Issue RestComm#2835

8e42cbf

fixes getting associated profile restcomm1840

5a90fe1

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

765f9cf

RESTCOMM-1595 #Reverting patch as it triggers a SMS loop on SMSC

160c1f1

RESTCOMM-1595 #Including missing import

1223ff5

Merge branch 'master' into Issue2835_LogoutSecurityFilter

1fbeea3

RESTCOMM-1847 #Default profile must allow all features

45c235b

Console: Send only modified fields on Profile update

f624765

Fixes RestComm#2858

fixes restcomm1851, constraint about status check

7352f01

doc adjusted to latest changes

e68e1ed

comment not applicable removed

Merge pull request RestComm#2859 from RestComm/restcomm1851

9bbe8f7

fixes restcomm1851, constraint about status check

Fixes duplicate jars in classpath

a40b67f

Merge pull request RestComm#2861 from RestComm/restcomm-1854

8de25ab

Fixes duplicate jars in classpath

Fixes for TestDialPartOne test case

73db760

Tag Unstable CallLifeCycle test case

66608e1

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

977c1b5

Merge branch 'master' into issue_2620

6f447b0

maria-farooq reviewed Mar 9, 2018

View reviewed changes

gvagenas force-pushed the master branch 2 times, most recently from 9469191 to c67f142 Compare January 13, 2022 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2620 #2641

Issue 2620 #2641

ddhuy commented Nov 18, 2017 •

edited

Loading

deruelle commented Nov 23, 2017

deruelle commented Nov 23, 2017

ddhuy commented Nov 23, 2017 •

edited

Loading

gsaslis left a comment

gsaslis Nov 23, 2017

ddhuy Nov 24, 2017 •

edited

Loading

gsaslis Nov 28, 2017

ddhuy Nov 30, 2017

gsaslis left a comment

gsaslis Dec 7, 2017

ddhuy Dec 8, 2017 •

edited

Loading

gsaslis commented Dec 11, 2017

ddhuy commented Dec 15, 2017 •

edited

Loading

ddhuy commented Mar 8, 2018 •

edited

Loading

gsaslis commented Mar 9, 2018

jaimecasero commented Mar 9, 2018

maria-farooq left a comment

Issue 2620 #2641

Are you sure you want to change the base?

Issue 2620 #2641

Conversation

ddhuy commented Nov 18, 2017 • edited Loading

deruelle commented Nov 23, 2017

deruelle commented Nov 23, 2017

ddhuy commented Nov 23, 2017 • edited Loading

gsaslis left a comment

Choose a reason for hiding this comment

gsaslis Nov 23, 2017

Choose a reason for hiding this comment

ddhuy Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

gsaslis Nov 28, 2017

Choose a reason for hiding this comment

ddhuy Nov 30, 2017

Choose a reason for hiding this comment

gsaslis left a comment

Choose a reason for hiding this comment

gsaslis Dec 7, 2017

Choose a reason for hiding this comment

ddhuy Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

gsaslis commented Dec 11, 2017

ddhuy commented Dec 15, 2017 • edited Loading

ddhuy commented Mar 8, 2018 • edited Loading

gsaslis commented Mar 9, 2018

jaimecasero commented Mar 9, 2018

maria-farooq left a comment

Choose a reason for hiding this comment

ddhuy commented Nov 18, 2017 •

edited

Loading

ddhuy commented Nov 23, 2017 •

edited

Loading

ddhuy Nov 24, 2017 •

edited

Loading

ddhuy Dec 8, 2017 •

edited

Loading

ddhuy commented Dec 15, 2017 •

edited

Loading

ddhuy commented Mar 8, 2018 •

edited

Loading