Skip to content
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

sync businessPhone validation with member api #333

Open
vikasrohit opened this issue Jan 2, 2020 · 7 comments
Open

sync businessPhone validation with member api #333

vikasrohit opened this issue Jan 2, 2020 · 7 comments

Comments

@vikasrohit
Copy link

As of now user can enter phone number in a format which is not acceptable by the member api (trait endpoint) which causes the api to fail while trying to create user trait.
Request:
Screen Shot 2020-01-02 at 4 32 06 PM
Response:
Screen Shot 2020-01-02 at 4 32 13 PM
In this particular example, I guess, the issue is missing + as prefix of the business phone string.
Ideally, our front end validation should have prevented user entering such value or automatically append the +.

fyi @maxceem @RishiRajSahu

@maxceem
Copy link
Collaborator

maxceem commented Jan 5, 2020

Here are all the validation rules from the Member Service:

if (item.getFirstNLastName() == null || item.getFirstNLastName().trim().length() == 0) {
    result.add("The first and last Name should be provided");
}
if (item.getBusinessEmail() != null && item.getBusinessEmail().trim().length() != 0) {
    Pattern pattern = Pattern.compile("^[A-Z0-9._%+-]+@[A-Z0-9.-]+\\.[A-Z]{2,6}$", Pattern.CASE_INSENSITIVE);
    if (!pattern.matcher(item.getBusinessEmail().trim()).matches()) {
        result.add("The business email should be provided in correct format");
    }
}
if (item.getBusinessPhone() != null && item.getBusinessPhone().trim().length() != 0) {
    Pattern pattern = Pattern.compile("^\\+(?:[0-9] ?){6,14}[0-9]$");
    if (!pattern.matcher(item.getBusinessPhone().trim()).matches()) {
        result.add("The business phone should be provided in correct format");
    }
}
if (item.getTitle() == null || item.getTitle().trim().length() == 0) {
    result.add("The title should be provided");
} 
if (item.getCompanyName() == null || item.getCompanyName().trim().length() == 0) {
    result.add("The company name should be provided");
}
if (item.getCompanySize() == null || item.getCompanySize().trim().length() == 0) {
    result.add("The company size should be provided");
}

I guess the best we can do is:

  1. automatically add + for the phone number
  2. make sure our client-side validation doesn't skip values which are not valid for member service:
    1. we either can make sure our RegExps are stronger
    2. or we can add to checks: our client-side check we have right now + plus additional check as at member service
    3. or we can replace our client-side validation with the same we have at member service
  3. We might also check that we validate other fields client-side the same as service-side.

@vikasrohit
Copy link
Author

I guess if we use the same regex as member api is using, we would be in safest position right?

@maxceem
Copy link
Collaborator

maxceem commented Jan 6, 2020

I guess if we use the same regex as member api is using, we would be in safest position right?

It would the easiest and safe solution. Our current validator is much more complicated than member service regexp.

@vikasrohit
Copy link
Author

Go with that.

@maxceem
Copy link
Collaborator

maxceem commented Jan 6, 2020

Sum up:

To avoid the server-side issue when creating a user or update user details, we should make the validation rules for the phone number we have client-side, to be the same we have server-side:

  • automatically add + for the phone number before sending to the server
  • disable/remove the client-side phone validation which we currently have as we want to have only one Regexp validation same like on the server
  • use the same validation rule to validate phone as we use server-side:
    if (item.getBusinessPhone() != null && item.getBusinessPhone().trim().length() != 0) {
        Pattern pattern = Pattern.compile("^\\+(?:[0-9] ?){6,14}[0-9]$");
        if (!pattern.matcher(item.getBusinessPhone().trim()).matches()) {
            result.add("The business phone should be provided in correct format");
        }
     }
    • + should be optional in regexp, as we would add it automatically before sending to the server
  • the fix should be done in the phone component which we have in react-components library which is used in both the Registration Page and Profile Settings page
  • the registration page is also implemented inside react-components repo, to see demo run npm run dev and navigate to http://localhost:8080/WizardExamples
  • the Profile Settings page is implemented inside Connect App
  • submit a pull request for https://github.com/appirio-tech/react-components/tree/feature/connectv2 branch feature/connectv2. Optionally, submit PR for Connect App https://github.com/appirio-tech/connect-app branch cf20 and Accounts App https://github.com/appirio-tech/accounts-app branch dev if necessary.

@yoution
Copy link

yoution commented Jan 22, 2020

@maxceem the code can only be changed in react-components? but if user not input '+', which will add before send request, these login can be written in connect-app

@maxceem
Copy link
Collaborator

maxceem commented Jan 22, 2020

The code can be changed in all repositories including connect-app and accounts-app if necessary.
Please, feel free to add + in other repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants