#164: bug with multiple registrations by changing case fixed

[IgorMaksymov]

164: bug with multiple registrations by changing case fixed

connect to #164

[IgorMaksymov]

@AndriyBaibak time spent: 30m

[vaa25]

@IgorMaksymov

1. no tests
2. why do you think SQLIntegrityConstraintViolationException is bad exception?

[IgorMaksymov]

@vaa25 test for multiple registrations is not working. because it is not throwing any SQLIntegrityConstraintViolationException, its throwing PSQLException, which is SQLException. issue https://github.com/JuniorsJava/itevents/issues/155

[IgorMaksymov]

blocked with #155 @AndriyBaibak @romach

[IgorMaksymov]

@vaa25 its not blocked anymore

[vaa25]

@IgorMaksymov no test showing solved problem in task

[vaa25]

@AndriyBaibak 50 min with other reviews today before

[IgorMaksymov]

@vaa25 fixed

[vaa25]

@IgorMaksymov It must be integration test. it shoud try register new user via controller through service and dao. Dao have to work with dbunit. dbunit must contain similar login. Test have to check response code and message. Also last commit has conflicts with master

[IgorMaksymov]

@vaa25 my unit test shows that in DAO layer user comes with LOWERCASE, but in SERVICE layer incoming parameter is in UPPERCASE. we dont need such complicated test, as you described.

[IgorMaksymov]

@vaa25 fixed

[romach]

@IgorMaksymov I think that manipulations with case of username in UserService is a bad idea. It is better to use validation in UserRestController.registerNewSubscriber() to solve this issue.

[romach]

@AndriyBaibak time spent: 30 minutes

[IgorMaksymov]

@romach you want @email validation? the problem is, that in database saves "emAil", when there already exist login "email"

or you want to send message to user, that he must type his login with lovercase? in this case, its better to handle this in our side.

[vaa25]

@romach are this manipulations are part of business logic?

[romach]

are this manipulations are part of business logic?

@vaa25 yes, they are. But I think that this business logic is wrong. For example, I try to register with username Roma@mail.com (such username is absent in DB). user.setLogin(user.getLogin().toLowerCase()); changes my username and do not throw any errors that this username is wrong. After registration I will figure out that my username is roma@mail.com (maybe I don't want this). I think that: 1) if uppercase letters in username are not allowed, exception must be thrown to user; 2) our system shouldn't change user input data;

[IgorMaksymov]

@romach 1) login is an EMAIL, so it should be in lowercase. 2) bothering user with exceptions, because his email not like we need, is not good, so i decided to handle this in our side. 3) i dont see possibilities to make 'Roma/romA/roma/rOMa' to be the same for our database.

[romach]

@IgorMaksymov

1) login is an EMAIL, so it should be in lowercase.

according to Wikipedia email can contain uppercase letters;

3) i dont see possibilities to make 'Roma/romA/roma/rOMa' to be the same for our database.

ok, maybe it is better to create separate method checkEmailForUniqIgnoreCase() in UserService to which we will pass lowercased username but in database we will store original username.

[IgorMaksymov]

@romach yes, it can, and its 2 different emails, read further. this is overengineering, we have simple .toLowerCase, other will not make code more readable..

[romach]

@IgorMaksymov resolve merge conflicts, please

[IgorMaksymov]

@romach done

[vaa25]

@IgorMaksymov where is integration test?

[IgorMaksymov]

@vaa25 as is said before, we dont need it. All manipulations is handled in current tests. if you remove .toLoverCase() test will fail.

[alex-anakin]

@IgorMaksymov

1. Issue describe the problem: You can re-register your mail by changing case. So your solution should forbid register user with the same email in different cases. I didn't find the test that shows how user tries to register with the same email in different cases and fails.
2. For example, the user registered with login Address@mail.com. Can user log in with the same login?
3. You make adding the new user with login forcibly formatted to lowercase. Why do you format username to lower case when getting it from the database?

[IgorMaksymov]

@alex-anakin 1) yes, he can 2) database will not find user Address@mail.com even if address@mail.com exist

[vaa25]

@IgorMaksymov If user has Address@mail.com, but we store address@mail.com, can we guarantee that otp and other subscriptions will reach right destination?

[IgorMaksymov]

@vaa25 we can move our toLowerCase() to addSubscriver(), also i sent email from our service to adress IVMaksymov@gmail.com and received it.

[IgorMaksymov]

@romach passed or not?

[romach]

@IgorMaksymov issue branch builds (locally) with exception:

org.itevents.integration.UserRestControllerIntegrationTest > shouldExpect422IfRegisteringExistingLoginWithUpperCase FAILED
    java.lang.IllegalStateException: Failed to load ApplicationContext

        Caused by:                                      
        org.springframework.beans.factory.BeanDefinitionStoreException: Invalid bean definition with name 'sendgrid' defined in class path resource [ap
plicationContextIntegrationTestAddon.xml]: Could not resolve placeholder 'sendgrid.api.key' in string value "${sendgrid.api.key}"; nested exception is
java.lang.IllegalArgumentException: Could not resolve placeholder 'sendgrid.api.key' in string value "${sendgrid.api.key}"

            Caused by:                                  
            java.lang.IllegalArgumentException: Could not resolve placeholder 'sendgrid.api.key' in string value "${sendgrid.api.key}"

[IgorMaksymov]

@romach TRAVIS build works fine. update properties file, or i forgot to add properties to a template, please check this. Take properties from Travis properties

i've made some additional properties file.

[romach]

@IgorMaksymov

@romach TRAVIS build works fine. update properties file, or i forgot to add properties to a template, please check this. Take properties from Travis properties i've made some additional properties file.

Fix test-local.properties.TEMPLATE because we must give properties to local.properties from it

[IgorMaksymov]

@romach i dont think that this is more readable. fixed previous mistakes.

[vaa25]

@IgorMaksymov you shoudn't create own applicationContext for test

[vaa25]

@IgorMaksymov remove duplicated variables from local.properties.TEMPLATE (look global.properties) @AndriyBaibak 10min

[IgorMaksymov]

@vaa25 it should be done within #221

[vaa25]

@IgorMaksymov it added in this pr, compare with master to see it