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

maintenance: Add getNewUserRequests, approveNewUserRequests, declineN… #2520

Conversation

khatibtamal
Copy link
Contributor

…ewUserRequests and getRegistrationInfoFromId for UserTeamsControllerService

Linked issue

Resolves: #2519

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Docs update
  • CI update

What is the current behavior?

UsersTeamsControllerService.java lacks unit tests for getNewUserRequests, approveNewUserRequests, declineNewUserRequests and getRegistrationInfoFromId

What is the new behavior?

Unit tests added for getNewUserRequests, approveNewUserRequests, declineNewUserRequests and getRegistrationInfoFromId in UsersTeamsControllerServiceTest

Other information

A null check had to be added in UserTeamsControllerService.java because this test case generates an exception with null message

Requirements (all must be checked before review)

  • The pull request title follows our guidelines
  • Tests for the changes have been added (if relevant)
  • The latest changes from the main branch have been pulled
  • pnpm lint has been run successfully

…ewUserRequests and getRegistrationInfoFromId for UserTeamsControllerService

Signed-off-by: Khatib Tamal <[email protected]>
@khatibtamal
Copy link
Contributor Author

khatibtamal commented Jun 30, 2024

@aindriu-aiven In UserTeamsControllerService.java here there is no tenant wise filtering of the user requests, and the team name that is being pulled is based on the tenant of the authenticated user. Is this intentional?

@khatibtamal
Copy link
Contributor Author

@aindriu-aiven FYI tenantId and teamName are un used params here I avoided touching them to avoid cascading changes in other files. Please let me know if you want them removed

@aindriu-aiven
Copy link
Contributor

@khatibtamal I've set aside time tomorrow to do this review, sorry for the delay really appreciate it!

@khatibtamal
Copy link
Contributor Author

@khatibtamal I've set aside time tomorrow to do this review, sorry for the delay really appreciate it!

no problem, please do it at your convenience. thanks!

Copy link
Contributor

@aindriu-aiven aindriu-aiven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a million for your contribution!

@khatibtamal
Copy link
Contributor Author

LGTM! Thanks a million for your contribution!

Hello thanks! But I cannot merge it.

@aindriu-aiven aindriu-aiven merged commit 4cf66f8 into Aiven-Open:main Jul 4, 2024
16 checks passed
@aindriu-aiven
Copy link
Contributor

LGTM! Thanks a million for your contribution!

Hello thanks! But I cannot merge it.

Sorry I was waiting for the build to complete so I could merge it and got distracted, thanks for the reminder!

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

Successfully merging this pull request may close these issues.

UserTeamsControllerService New User Request Unit tests
2 participants