[PM-33951] feat(admin-console): Add bulk confirmation and pending auto-confirmation#7661
[PM-33951] feat(admin-console): Add bulk confirmation and pending auto-confirmation#7661JaredScar wants to merge 8 commits into
Conversation
…ion methods for organization users - Implemented ConfirmManyOrganizationUsersAsync to confirm multiple users in a single operation. - Added GetManyPendingAutoConfirmAsync to retrieve users pending automatic confirmation. - Created stored procedures for bulk confirmation and fetching pending users. - Updated relevant repository interfaces and implementations across Dapper and Entity Framework.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR adds bulk confirmation ( Code Review Details
|
JaredScar
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7661 +/- ##
==========================================
+ Coverage 64.86% 64.92% +0.06%
==========================================
Files 2140 2141 +1
Lines 94629 94715 +86
Branches 8445 8459 +14
==========================================
+ Hits 61378 61496 +118
+ Misses 31155 31121 -34
- Partials 2096 2098 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
r-tome
left a comment
r-tome
left a comment
There was a problem hiding this comment.
Claude pointed out a few things to fix. Let me know when to review this again
|
There are similar database changes in PR 7527, as well as this one. Do both PR's need a review from dbops or only this one? |
Valid question! @mkincaid-bw -- Sorry for the confusion. @eliykat suggested we split up that PR into smaller PRs like this one that we merge first, then that original PR will be smaller and easier to review since that big chunk of code are broken into pieces. Theoretically speaking, this should be the only one DB ops needs to worry about 😄 |
…ationUsersAsync to IReadOnlyCollection - Updated the ConfirmManyOrganizationUsersAsync method signature in the IOrganizationUserRepository and its implementations to use IReadOnlyCollection instead of IEnumerable for better performance and clarity. - Adjusted related repository methods in both Dapper and Entity Framework implementations to reflect this change. - Added unit tests to ensure the new implementation behaves as expected, including scenarios for mixed batches and idempotency.
There was a problem hiding this comment.
Thanks @JaredScar for splitting this to a separate PR - I cannot tell you how much easier it is to review smaller PRs.
Aside from my comments below, please check the CI runs:
- db integration test is failing - potentially faulty prod or test logic
- db validation is failing - this ensures that the migration scripts match the
src/Sqlfiles - validate new migration naming and order - this is failing because later migrations have overtaken you and you need to bump the dates on your script
More info available in each of the runs, or hit me up on Slack if you have questions.
EDIT: please also add more info to your PR description, e.g.
This PR contains the database changes needed for #7527, split to their own PR to manage PR size.
The PR link is particularly useful to understand what's going on.
…s part of database cleanup.
…dingAutoConfirm methods - Introduced ConfirmManyOrganizationUsersTests to validate the confirmation of multiple organization users, ensuring only accepted users are confirmed and idempotency is maintained. - Added GetManyPendingAutoConfirmTests to verify retrieval of pending auto-confirm users, ensuring only eligible users are returned based on specific criteria. - Removed duplicate test implementations from OrganizationUserRepositoryTests to maintain clarity and organization in the test suite.
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
My changes have been addressed, I can approve once @mkincaid-bw is happy and CI checks are passing.
…e related repository method - Added OrganizationUser_UpdateStatusKey stored procedure to handle updating the status and key of organization users based on a JSON input. - Updated OrganizationUserRepository to call the new stored procedure instead of the previous confirmation procedure. - Modified OrganizationUser_ReadByPendingAutoConfirm stored procedure to filter users by a new type value. - Enhanced integration tests to verify the correct behavior of the updated confirmation logic, ensuring revision dates are accurately tracked.
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
Couple more minor changes.
…anyStatusKey - Renamed the stored procedure to OrganizationUser_UpdateManyStatusKey to better reflect its functionality of updating multiple organization users' statuses. - Updated the OrganizationUserRepository to call the new stored procedure. - Adjusted the migration script to create or alter the procedure accordingly.
|
| DECLARE @UsersToUpdate AS TABLE ( | ||
| [Id] UNIQUEIDENTIFIER NOT NULL, | ||
| [Key] NVARCHAR(MAX) NULL | ||
| ) | ||
|
|
||
| INSERT INTO @UsersToUpdate | ||
| SELECT | ||
| [Id], | ||
| [Key] | ||
| FROM OPENJSON(@UsersJson) | ||
| WITH ( | ||
| [Id] UNIQUEIDENTIFIER '$.Id', | ||
| [Key] NVARCHAR(MAX) '$.Key' | ||
| ) |
There was a problem hiding this comment.
[Key] as NVARCHAR(MAX) but the SP source-of-truth file uses VARCHAR(MAX), which will fail the SSDT drift check in CI.
Details and fix
The source file src/Sql/dbo/Stored Procedures/OrganizationUser_UpdateManyStatusKey.sql defines the temp table column and the OPENJSON output as VARCHAR(MAX), matching the underlying OrganizationUser.[Key] column (which is VARCHAR(MAX) per src/Sql/dbo/Tables/OrganizationUser.sql:6). This migration file still has NVARCHAR(MAX) in both places.
.github/workflows/test-database.yml runs sqlpackage /action:DeployReport to compare the dacpac (built from src/Sql/) against the database after migrations are applied. Any <Operations> in the report fails the job — so this divergence will block the merge.
DECLARE @UsersToUpdate AS TABLE (
[Id] UNIQUEIDENTIFIER NOT NULL,
[Key] VARCHAR(MAX) NULL
)
INSERT INTO @UsersToUpdate
SELECT
[Id],
[Key]
FROM OPENJSON(@UsersJson)
WITH (
[Id] UNIQUEIDENTIFIER '$.Id',
[Key] VARCHAR(MAX) '$.Key'
)This was flagged earlier (threads PRRC_kwDOAsltcc7EH_zb and PRRC_kwDOAsltcc7EIBCm) but the SP rename in 4d5f03c94 only carried the fix into the source SP file, not into this migration.
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
Sorry, I should have flagged the data type changes in the migration script as well. Also have some really minor, petty spacing nit picks 😄
|
|
||
| DECLARE @UsersToUpdate AS TABLE ( | ||
| [Id] UNIQUEIDENTIFIER NOT NULL, | ||
| [Key] NVARCHAR(MAX) NULL |
There was a problem hiding this comment.
| [Key] NVARCHAR(MAX) NULL | |
| [Key] VARCHAR(MAX) NULL |
| FROM OPENJSON(@UsersJson) | ||
| WITH ( | ||
| [Id] UNIQUEIDENTIFIER '$.Id', | ||
| [Key] NVARCHAR(MAX) '$.Key' |
There was a problem hiding this comment.
| [Key] NVARCHAR(MAX) '$.Key' | |
| [Key] VARCHAR(MAX) '$.Key' |
|
|
||
| DECLARE @UsersToUpdate AS TABLE ( | ||
| [Id] UNIQUEIDENTIFIER NOT NULL, | ||
| [Key] VARCHAR(MAX) NULL |
There was a problem hiding this comment.
⛏️
| [Key] VARCHAR(MAX) NULL | |
| [Key] VARCHAR(MAX) NULL |
| FROM OPENJSON(@UsersJson) | ||
| WITH ( | ||
| [Id] UNIQUEIDENTIFIER '$.Id', | ||
| [Key] VARCHAR(MAX) '$.Key' |
There was a problem hiding this comment.
⛏️
| [Key] VARCHAR(MAX) '$.Key' | |
| [Key] VARCHAR(MAX) '$.Key' |
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33951
📔 Objective
This PR contains the database changes needed for #7527, split to their own PR to manage PR size.