SSO login bug fix #2492
SSO login bug fix #2492
Conversation
muralibasani
force-pushed
the
sso-login-fix
branch
from
c60ba20 to
26e925d
Compare
| return "Failure. Registration already exists"; | ||
| Optional<RegisterUserInfo> optionalUserRegistration = | ||
| registerInfoRepo.findByUsername(userInfo.getUsername()); | ||
| if (optionalUserRegistration.isPresent()) { |
There was a problem hiding this comment.
If the User Registration is not present should we return "Failure. Registration doesn't exist"
There was a problem hiding this comment.
We should create a new one if not present which is done at line 401.
| status = NewUserStatus.DECLINED.value; | ||
| // In case if user registration is declined, delete the record from db, so user can try to | ||
| // register again with any new data. | ||
| registerInfoRepo.deleteById("" + registerUser.getId()); |
There was a problem hiding this comment.
NIT: a stylistic choice but could we use String.valueOf(registerUser.getId())
| @@ -633,7 +636,7 @@ public void showUserList() throws Exception { | |||
| .getContentAsString(); | |||
| List<UserInfoModelResponse> userInfoModelList = | |||
| new ObjectMapper().readValue(response, new TypeReference<>() {}); | |||
| assertThat(userInfoModelList).hasSizeBetween(3, 6); // superadmin, kwuserb, kwuserc | |||
| assertThat(userInfoModelList).hasSizeBetween(3, 9); // superadmin, kwuserb, kwuserc | |||
There was a problem hiding this comment.
Should this be a bit more precise, I am assuming it should equal the same amount every time, or use assertThat(userInfoModelList).containsAll(a,b,c,d) ?
There was a problem hiding this comment.
We added this as this test might run before or after other integration tests, and we are not cleaning up users after the integration test.
So there are about 9 users created totally.
There was a problem hiding this comment.
Ok I think we can maybe improve this later, it doesn't need to hold up this PR
| String jsonReq = OBJECT_MAPPER.writer().writeValueAsString(userInfoModel); | ||
|
|
||
| String registerUserResponse = | ||
| mvc.perform( |
There was a problem hiding this comment.
We can probably extract these three performs into a method each and re-use them.
It would reduce code duplication
| @@ -195,8 +195,7 @@ private static Stream<Arguments> insertIntoRegisterUsers() { | |||
| Arguments.of( | |||
| "[email protected]", "[email protected]", ApiResultStatus.SUCCESS.value), | |||
| Arguments.of("Octopus2", "[email protected]", "Failure. User already exists"), | |||
| Arguments.of( | |||
There was a problem hiding this comment.
Can we keep this test it covers a test case we still have.
There was a problem hiding this comment.
We currently have only userid field as primary key, however an emailid can be mentioned as userid. But this testcase assumes emailid column as primary key, hence removed.
Linked issue
Resolves: #2484
What kind of change does this PR introduce?
When a user request is declined, record is deleted from staging table, so user can register again with similar data
A bug fix when an email is sent out, decode was applied on plain text.
An if condition is fixed to allow staging and pending requests
Bug fix
New feature
Refactor
Docs update
CI update
What is the current behavior?
Describe the state of the application before this PR. Illustrations appreciated (videos, gifs, screenshots).
What is the new behavior?
Describe the state of the application after this PR. Illustrations appreciated (videos, gifs, screenshots).
Other information
Additional changes, explanations of the approach taken, unresolved issues, necessary follow ups, etc.
Requirements (all must be checked before review)
mainbranch have been pulledpnpm linthas been run successfully