#48-otp

[IgorMaksymov]

generation of OTP and user activation added connect to #48

[witjem]

You have to fix merge conflict

[IgorMaksymov]

@roma-ilnitsky @romach fixed, review pls

[witjem]

@IgorMaksymov generateOtpByUserIdAndSendItToUserEmail() you have to divided this method because it do not match SOLID.

I offer:

 @Override
    public void addSubscriber(String username, String password) throws Exception {
        User user = UserBuilder.anUser()
                .login(username)
                .role(roleService.getRoleByName("guest"))
                .build();
        addUser(user, passwordEncoder.encode(password));

        OneTimePassword otp = oneTimePassword.generateOtp(otpLifetime);
        registerOtp(otp);
        sendOtpToUserEmail(otp);
    }

[IgorMaksymov]

@roma-ilnitsky updated, review pls

[witjem]

• passed

[vaa25]

@IgorMaksymov Failed merge from master. Previous issues in master deleted.

[IgorMaksymov]

@vaa25 what?This branch has no conflicts with the base branch and Travis check passed. Check your database. in some cases there may so much changes from diffent Changesets, so it becomes incompatible with other braches. To avoid this we need to do rollback before every checkout.

[vaa25]

@IgorMaksymov I checkouted another 48 branch. Why we have three of them?

[IgorMaksymov]

@vaa25 because we need all of them. IgorMaksymov wants to merge 16 commits into master from #48-otp

[vaa25]

@IgorMaksymov why we need all of them? Maybe you need its locally?

[IgorMaksymov]

@vaa25 not locally, we need it to share code with partner

[vaa25]

@IgorMaksymov has ignored tests @AndriyBaibak 15 min

[IgorMaksymov]

@ i opened tasks to fix them. i'm out of scope here.

[vaa25]

@IgorMaksymov UserService: setOtpToUser is used only in UserService. getUserByOtp is not used anywhewe It seems task is not finished. What is target of this task? No tests, no demos... How to check it? What to check? Only code style?

[IgorMaksymov]

@vaa25 i dont understand purpose of your comment, it's to criticize me or something else?

which of setOtpToUser? i should make it private or add ot to UserDao.addSubscriber ? what is not tested? demo is after merge.

[vaa25]

@IgorMaksymov yes, task seems finished except: UserService:

1. why getUserByOtp is separate method in UserService?
2. activateUserWithOtp has broken indents

OtpExpiredServiceExceprion:

3. has unused constructor
4. has mistake in the name

MyBatisUserServiceTest:

5. What doing verify in shouldNotUseOtpIfExpired?
6. Magic number in shouldUseOtpAndSetRoleToSubscriber

UserRestControllerTest:

7. shouldActivateUserByOtp has incorrect path

ControllerHandlerTest:

8. no test for expired otp

OneTimePasswordTest:

9. Magic number
10. Why you don't use bean OneTimePassword in tests?

And of course ignored tests of changed methods @AndriyBaibak 1.5 hours

[Garazd]

@AndriyBaibak 2 hours

[vaa25]

• PASSED

[IgorMaksymov]

@AndriyBaibak time spent: 1h