[PM-20134] Fix overwriteExisting and largeImport causing users to be deleted #737
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #737 +/- ##
========================================
+ Coverage 7.27% 7.85% +0.58%
========================================
Files 66 68 +2
Lines 2682 2751 +69
Branches 473 473
========================================
+ Hits 195 216 +21
- Misses 2475 2523 +48
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
There was a problem hiding this comment.
Can you please add some unit tests for the following:
- SingleRequestBuilder - make sure that the options (removeDisabled and overwriteExisting) are set properly on the request object based on inputs
- BatchRequestBuilder - make sure that removeDisabled is set properly on the request object based on inputs (you've already covered overwriteExisting which is good)
Can you also consider what integration tests we can add for these? I didn't think it was worth it previously because these are directory-agnostic and unit tests should be enough. But our recent experience shows that a few happy path integration tests would also be a good safeguard.
I want to set up full end-to-end integration tests against a real server, but for now we can just work within the openldap test suite we already have.
eliykat
changed the title
There was a problem hiding this comment.
I forgot these tests are testing the directory service specifically, whereas our new tests are testing syncService.
After thinking about it some more I recommend:
- moving the new tests to a new spec file (
sync.service.integration.spec.ts) - only test the SyncService result/side effects
- still integrate against the openldap container
| constants.batchSize = 4; | ||
|
|
||
| const result = await directoryService.getEntries(true, true); | ||
| const syncResult = await syncService.sync(false, false); |
There was a problem hiding this comment.
Also assert against the call received by the mock api service. That's an important side-effect here, and it lets you check the other request properties that aren't included in the return value, particularly overwriteExisting.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-20134
📔 Objective
This PR changes the
request-builderinterface method and caller insync-serviceto accept an object with sync configuration rather than booleans as its params.batch-request-builderwill now throw an error if you attempt to perform a sync with bothlargeImportandoverwriteExistingenabled to prevent overwriting the imported users as the batched calls are made.📸 Screenshots
11k users sync before:
Screen.Recording.2025-04-10.at.12.53.03.PM.mov
11k users sync after:
Screen.Recording.2025-04-10.at.12.56.31.PM.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes