Refactor remaining service and test code to remove references to old identity properties #399
Conversation
sbryzak
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
|
/retest |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #399 +/- ##
==========================================
- Coverage 78.40% 78.10% -0.31%
==========================================
Files 38 38
Lines 3306 3288 -18
==========================================
- Hits 2592 2568 -24
- Misses 620 624 +4
- Partials 94 96 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/retest |
|
| @@ -617,7 +585,7 @@ func (s *ServiceImpl) PhoneNumberAlreadyInUse(userID, username, phoneNumberOrHas | |||
| return errors.NewInternalError(err, "failed listing userSignups") | |||
| } | |||
| for _, signup := range userSignups { | |||
| if signup.Spec.Userid != userID && signup.Spec.Username != username && !states.Deactivated(signup) { // nolint:gosec | |||
| if signup.Spec.IdentityClaims.Sub != userID && signup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(signup) { // nolint:gosec | |||
There was a problem hiding this comment.
Let's rename the userID param to something like sub in the function signature in the line 574, so it's bit less confusing.
| if signup.Spec.IdentityClaims.Sub != userID && signup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(signup) { // nolint:gosec | |
| if signup.Spec.IdentityClaims.Sub != sub && signup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(signup) { // nolint:gosec |
There was a problem hiding this comment.
Looks good. Just one minor comment.
But could you please also create a PR for our sre repo to stop using the annotations in the usersignup. Basicaly the same PR as https://github.com/codeready-toolchain/sandbox-sre/pull/1406 but without the go dependency update. And we will need to get that PR merged right after this one so our banning CLI is still working.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, sbryzak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is an intermediate PR related to the following API PR:
codeready-toolchain/api#393
Since it only changes existing registration service code but doesn't actually update the API dependency it should be safe to merge independent of the other PRs referenced in the above API pull request.
Paired with codeready-toolchain/toolchain-e2e#892