Skip to content

Conversation

@andrebispo5
Copy link
Contributor

@andrebispo5 andrebispo5 commented Dec 18, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-29113

📔 Objective

This pull request introduces functionality to prompt users to migrate their personal vault items to an organization when certain policy and feature flag conditions are met. It adds a new method to determine the earliest organization applying a specific policy, updates the Policy model to include a revision date, and integrates these changes into the sync process. Comprehensive tests are included for the new logic.

Vault migration feature integration:

  • Added a private method checkUserNeedsVaultMigration() to DefaultSyncService that checks if the user needs to migrate personal vault items to an organization based on a feature flag, the presence of the Personal Ownership policy, and the existence of personal vault items. If all conditions are met, it notifies the delegate via migrateVaultToMyItems(organizationId:).
  • Updated the SyncServiceDelegate protocol to include the migrateVaultToMyItems(organizationId:) method, which is called when a migration is required.

Policy service enhancements:

  • Added the getEarliestOrganizationApplyingPolicy(_:) method to PolicyService and implemented it in DefaultPolicyService. This method returns the organization ID with the earliest revision date for a given policy type.
  • Updated the Policy model to include an optional revisionDate property and ensured it is populated from response models and fixtures.
  • Updated policy fixtures to support the new revisionDate property for testing purposes.

Dependency and test updates:

  • Added configService as a dependency to ServiceContainer, DefaultSyncService, and related test setups to support feature flag checks.
  • Added comprehensive tests for the new getEarliestOrganizationApplyingPolicy(_:) logic, covering edge cases such as no policies, multiple organizations, nil revision dates, and filtering by policy type.

📸 Screenshots

Screen.Recording.2025-12-18.at.13.29.51.mov

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 18, 2025

Logo
Checkmarx One – Scan Summary & Details316d1839-55e6-4117-b13f-85258ba6a2d9

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Dec 18, 2025

Codecov Report

❌ Patch coverage is 98.56263% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.23%. Comparing base (b6284e4) to head (1e7c425).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ToMyItems/MigrateToMyItemsView+SnapshotTests.swift 0.00% 4 Missing ⚠️
...rateToMyItems/MigrateToMyItemsProcessorTests.swift 96.15% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2203      +/-   ##
==========================================
- Coverage   85.48%   84.23%   -1.25%     
==========================================
  Files        1743     1988     +245     
  Lines      147452   163087   +15635     
==========================================
+ Hits       126048   137382   +11334     
- Misses      21404    25705    +4301     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andrebispo5 andrebispo5 marked this pull request as ready for review December 19, 2025 16:24
Copilot AI review requested due to automatic review settings December 19, 2025 16:24
@andrebispo5 andrebispo5 requested a review from a team as a code owner December 19, 2025 16:24
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request introduces functionality to prompt users to migrate their personal vault items to an organization when certain policy and feature flag conditions are met. The implementation adds a new check during the sync process that evaluates whether the user needs to perform a vault migration based on the Personal Ownership policy and the presence of personal vault items.

Key changes include:

  • Added migrateVaultToMyItems delegate method to SyncServiceDelegate and integrated vault migration checking into the sync flow
  • Enhanced PolicyService with getEarliestOrganizationApplyingPolicy() method to determine which organization should receive migrated items
  • Extended the Policy model to include a revisionDate property for tracking when policies were last updated

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
BitwardenShared/Core/Vault/Services/SyncService.swift Added checkUserNeedsVaultMigration() method and migrateVaultToMyItems delegate method to handle vault migration logic during sync
BitwardenShared/Core/Vault/Services/SyncServiceTests.swift Added comprehensive test coverage for vault migration scenarios and mock delegate implementation
BitwardenShared/Core/Vault/Services/PolicyService.swift Implemented getEarliestOrganizationApplyingPolicy() to find the organization with the earliest policy revision date
BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift Added thorough tests for the new policy service method covering edge cases
BitwardenShared/Core/Vault/Models/Domain/Policy.swift Added revisionDate property to track when policies were last updated
BitwardenShared/Core/Vault/Models/Domain/Fixtures/Policy+Fixture.swift Updated fixture to support the new revisionDate property
BitwardenShared/Core/Vault/Services/TestHelpers/MockPolicyService.swift Added mock support for the new policy service method
BitwardenShared/UI/Platform/Application/AppProcessor.swift Implemented migrateVaultToMyItems() to handle navigation to the migration screen
BitwardenShared/UI/Platform/Application/AppProcessorTests.swift Added tests for the migration navigation logic
BitwardenShared/UI/Platform/Application/AppCoordinator.swift Added migration screen presentation logic and MigrateToMyItemsProcessorDelegate conformance
BitwardenShared/UI/Platform/Application/AppCoordinatorTests.swift Added tests for migration screen presentation and delegate handling
BitwardenShared/UI/Platform/Application/AppRoute.swift Added migrateToMyItems route to support navigation to the migration screen
BitwardenShared/UI/Vault/VaultItem/VaultItemRoute.swift Added migrateToMyItems route for vault item coordinator navigation
BitwardenShared/UI/Vault/VaultItem/VaultItemCoordinator.swift Added showMigrateToMyItems() method to present the migration view
BitwardenShared/UI/Vault/VaultItem/VaultItemCoordinatorTests.swift Added tests for migration navigation in the vault item coordinator
BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsState.swift Added organizationId property to state
BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsProcessor.swift Updated processor to load organization name by ID and removed dependency on PolicyService
BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsProcessorTests.swift Added tests for organization name loading and error handling
BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsView.swift Updated view previews to include required organizationId parameter
BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsView+ViewInspectorTests.swift Updated test setup to include organizationId
BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsView+SnapshotTests.swift Updated snapshot test setup to include organizationId
BitwardenShared/Core/Platform/Services/ServiceContainer.swift Added configService dependency to DefaultSyncService
BitwardenResources/Localizations/en.lproj/Localizable.strings Added "OrganizationNotFound" localization string

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
}

func migrateVaultToMyItems(organizationId: String) {
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The protocol declares this method as async (line 122 in SyncService.swift), but the implementation is missing the async keyword. This creates an inconsistency with the protocol definition. The method signature should be func migrateVaultToMyItems(organizationId: String) async to match the protocol.

Suggested change
func migrateVaultToMyItems(organizationId: String) {
func migrateVaultToMyItems(organizationId: String) async {

Copilot uses AI. Check for mistakes.
var migrateVaultToMyItemsCalled = false
var migrateVaultToMyItemsOrganizationId: String?

func migrateVaultToMyItems(organizationId: String) {
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The protocol declares this method as async (line 122 in SyncService.swift), but the mock implementation is missing the async keyword. This creates an inconsistency with the protocol definition. The method signature should be func migrateVaultToMyItems(organizationId: String) async to match the protocol.

Suggested change
func migrateVaultToMyItems(organizationId: String) {
func migrateVaultToMyItems(organizationId: String) async {

Copilot uses AI. Check for mistakes.
// TODO: PM-29113 Validate if user must do vault migration and error handling

state.organizationName = organization?.name ?? ""

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

There is trailing whitespace at the end of this line. This should be removed to maintain code consistency.

Suggested change

Copilot uses AI. Check for mistakes.
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.

2 participants