-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[AC-2328] Add a Bulk OrganizationUsersController.GetResetPasswordDetails endpoint #4079
Conversation
…ple organization user IDs
…rganizationUserAsync
…organization users
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4079 +/- ##
==========================================
+ Coverage 39.05% 39.08% +0.03%
==========================================
Files 1202 1202
Lines 58081 58118 +37
Branches 5350 5351 +1
==========================================
+ Hits 22684 22717 +33
- Misses 34328 34330 +2
- Partials 1069 1071 +2 ☔ View full report in Codecov by Sentry. |
New Issues
Fixed Issues
|
@@ -186,6 +186,19 @@ public async Task<OrganizationUserResetPasswordDetailsResponseModel> GetResetPas | |||
return new OrganizationUserResetPasswordDetailsResponseModel(new OrganizationUserResetPasswordDetails(organizationUser, user, org)); | |||
} | |||
|
|||
[HttpPost("reset-password-details")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset master password has been renamed "account recovery" across the product and documentation, but we still have a lot of lingering code calling things by the old "reset master password" name. Part of the tech debt epic to clean up account recovery (here) is going to involve renaming code to use "account recovery" verbiage.
We can avoid having to rename this endpoint and its backing logic later if we go ahead and call this by the correct name now. I'd like to suggest we dump any new instances of "reset password" and replace them with "account recovery", so this endpoint would be at /account-recovery-details
and the method would be called GetAccountRecoveryDetails
. This could be followed through to the new repository method too and name it GetManyAccountRecoveryDeatilsByOrganizationUserAsync
. I think the rest of the names are used in other bits of code, so those should be left alone.
public OrganizationUserResetPasswordDetails() | ||
{ | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a one-liner
@@ -3,7 +3,6 @@ | |||
using Bit.Billing.Services; | |||
using Bit.Core; | |||
using Bit.Core.AdminConsole.Entities; | |||
using Bit.Core.AdminConsole.Enums.Provider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I logged a separate PR for this lint failure stuff here
@@ -43,6 +43,7 @@ public interface IOrganizationUserRepository : IRepository<OrganizationUser, Gui | |||
Task RestoreAsync(Guid id, OrganizationUserStatusType status); | |||
Task<IEnumerable<OrganizationUserPolicyDetails>> GetByUserIdWithPolicyDetailsAsync(Guid userId, PolicyType policyType); | |||
Task<int> GetOccupiedSmSeatCountByOrganizationIdAsync(Guid organizationId); | |||
Task<IEnumerable<OrganizationUserResetPasswordDetails>> GetManyResetPasswordDetailsByOrganizationUserAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add an integration test for this in test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs
.
@@ -0,0 +1,24 @@ | |||
CREATE PROCEDURE [dbo].[OrganizationUser_ReadManyResetPasswordDetailsByOrganizationUserIds] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comment about naming things in the controller and repository layers: this stored procedure name should use "account recovery" verbiage instead of the outdated "reset password". We can't do anything about the column right now, but that should be included in the tech debt epic.
Also, in regards to the question on slack about what to do with the single-response method: We should log refactoring that method as follow up work on the Jira epic. My preference would be to leave the endpoint and have it reference the same repository logic, leaving |
…account recovery details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
LaunchDarkly flag references🔍 1 flag added or modified
|
Type of change
Objective
The current endpoint requires multiple database calls, this new endpoint gets all data using one new stored procedure.
Code changes
Clients PR
@bitwarden/team-billing-dev, you've been added as reviewers because
dotnet format
removed a few usings onStripeController
OrganizationUserId
property to be able to identify each one on the result listOrganizationUserId
property and a parameterless constructor for the auto mapperBefore you submit
dotnet format --verify-no-changes
) (required)