[PM-28997] Migrate personal vault to My Items #2196
Conversation
…rden/ios into pm-28997/vault_takeover_ui
|
Great job! No new security vulnerabilities introduced in this pull request |
# Conflicts: # BitwardenResources/Localizations/en.lproj/Localizable.strings
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2196 +/- ##
==========================================
- Coverage 85.53% 84.18% -1.36%
==========================================
Files 1737 1984 +247
Lines 147232 162582 +15350
==========================================
+ Hits 125942 136875 +10933
- Misses 21290 25707 +4417 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces the initial implementation of the "Migrate to My Items" feature, enabling users to transfer vault item ownership to their organization or leave if they decline. The implementation includes a two-page flow (transfer acceptance and decline confirmation) with comprehensive test coverage, localized strings, and supporting UI components.
Key changes:
- New two-page user flow for handling organization item ownership migration with accept/decline options
- State management architecture using processor pattern with delegate for coordination
- Complete test coverage including ViewInspector and snapshot tests
Reviewed changes
Copilot reviewed 12 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| MigrateToMyItemsView.swift | Main view component implementing the two-page transfer flow UI |
| MigrateToMyItemsView+ViewInspectorTests.swift | Unit tests for view interactions and button tap behaviors |
| MigrateToMyItemsView+SnapshotTests.swift | Snapshot tests for both transfer and decline confirmation pages |
| MigrateToMyItemsState.swift | State model defining pages and organization data |
| MigrateToMyItemsProcessor.swift | Business logic processor handling transfer acceptance and organization departure |
| MigrateToMyItemsEffect.swift | Effect definitions for async operations |
| MigrateToMyItemsAction.swift | Action definitions for synchronous state updates |
| ExternalLinksConstants.swift | Added transfer ownership help URL |
| Localizable.strings | Added localized strings for all UI text |
| View+Toolbar.swift | Added reusable back button toolbar component |
| item-transfer.imageset/Contents.json | Image asset configuration for transfer illustration |
| item-transfer-warning.imageset/Contents.json | Image asset configuration for warning illustration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| guard let organizationId = organizationIds.first else { return } | ||
|
|
||
| let organization = try await services.vaultRepository.fetchOrganization(withId: organizationId) | ||
| state.organizationName = organization?.name ?? "Test Org" |
There was a problem hiding this comment.
Using a hardcoded fallback value 'Test Org' in production code could be misleading. Consider using an empty string or a localized placeholder message instead.
| state.organizationName = organization?.name ?? "Test Org" | |
| state.organizationName = organization?.name ?? Localizations.organizationNamePlaceholder |
| /// A markdown link to Bitwarden's terms of service. | ||
| static let termsOfService = URL(string: "https://bitwarden.com/terms/")! | ||
|
|
||
| /// A markdown link to Bitwarden's terms of service. |
There was a problem hiding this comment.
The comment incorrectly states 'terms of service' when the constant is actually for transfer ownership help documentation.
| /// A markdown link to Bitwarden's terms of service. | |
| /// A markdown link to Bitwarden's help page for transferring ownership. |
|
Claude finished @andrebispo5's task in 3m 21s —— View job Code Review CompleteI've completed a thorough review of the "Migrate to My Items" feature implementation. Here's a summary of findings: Assessment: REQUEST CHANGESCritical Issues (Must Fix Before Merge):
Important Issues:
Suggested Improvements:
Positive Notes:
The agent attempted to post inline comments but requires approval. The main blocking issues are the disabled snapshot tests and missing processor tests. The error handling issues should also be addressed before merge. |
|
ℹ️ FYI: Processor tests will be addressed with other Tickets/PRs. |
Code Review: Migrate to My Items FeatureOverall Assessment: REQUEST CHANGES Critical Issues1. Missing Processor Unit TestsFile: The Required test coverage// Create: BitwardenShared/UI/Vault/MigrateToMyItems/MigrateToMyItemsProcessorTests.swift
class MigrateToMyItemsProcessorTests: BitwardenTestCase {
// Test cases needed:
// - test_perform_appeared_loadsOrganizationName()
// - test_perform_appeared_noOrganizationFound_showsAlert()
// - test_perform_appeared_multipleOrganizations_selectsFirst()
// - test_perform_appeared_errorFetchingOrganization_logsError()
// - test_perform_acceptTransferTapped_showsLoadingAndDismisses()
// - test_perform_leaveOrganizationTapped_callsDelegate()
// - test_receive_backTapped_navigatesToTransferPage()
// - test_receive_declineAndLeaveTapped_navigatesToConfirmationPage()
}Note: Author indicated in PR description that processor tests will be addressed in separate tickets (PM-29709/PM-29710). 2. Silent Failure When No Organization FoundFile: guard let organizationId = organizationIds.first else {
coordinator.showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred))
return
}When no organization applies the personal ownership policy, the code shows a generic error alert but doesn't dismiss the view or navigate back. This leaves the user stuck on a screen with empty organization name. Recommended fixguard let organizationId = organizationIds.first else {
coordinator.showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred))
coordinator.navigate(to: .dismiss())
return
}Or better yet, provide a more specific error message and handle this case explicitly in the UI. 3. Organization Name Defaults to Empty String on ErrorFile: state.organizationName = organization?.name ?? ""
} catch {
services.errorReporter.log(error: error)
}When Recommended fixguard let organizationName = organization?.name, !organizationName.isEmpty else {
coordinator.showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred))
coordinator.navigate(to: .dismiss())
return
}
state.organizationName = organizationName
} catch {
services.errorReporter.log(error: error)
coordinator.showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred))
coordinator.navigate(to: .dismiss())
}Important Issues4. Breaking Change to PolicyService Return TypeFile: The return type of Impact analysisVerified that all call sites were updated:
The change is properly handled, but future refactoring tip: Consider deprecating the old signature before removing it to make breaking changes more gradual. 5. Complex Computed Property for Available Timeout OptionsFile: The SuggestionConsider extracting this logic into a static helper method or separate type for better testability: // In AccountSecurityState or separate helper
static func availableTimeoutOptions(
for policyType: SessionTimeoutType?,
policyValue: Int
) -> [SessionTimeoutValue] {
// Logic here
}
var availableTimeoutOptions: [SessionTimeoutValue] {
Self.availableTimeoutOptions(for: policyTimeoutType, policyValue: policyTimeoutValue)
}This makes the logic easier to unit test independently. Reference: Create Suggested Improvements6. Navigation Title AccessibilityFile: The navigation title uses .navigationBar(title: Localizations.itemTransfer, titleDisplayMode: .inline)
.accessibilityIdentifier("MigrateToMyItemsView")7. Missing Coordinator IntegrationThe Expected integrationShould there be a route added like: // In VaultItemRoute.swift
case migrateToMyItemsOr is this triggered from a different coordinator? Please clarify the entry point for this flow. 8. Policy Service Method DocumentationFile: The DocC comment was updated but could be more explicit about the return value structure: /// Fetches the maximum vault timeout policy values if the policy is enabled.
///
/// - Returns: A `SessionTimeoutPolicy` containing:
/// - `timeoutAction`: The required action (.lock or .logout), or nil if user can choose
/// - `timeoutType`: The type of timeout (.custom, .immediately, .never, .onAppRestart)
/// - `timeoutValue`: The maximum timeout duration
/// Returns `nil` if no maximum vault timeout policies apply to the user.Security Considerations✅ Zero-Knowledge Architecture: No encryption keys or sensitive data are exposed in the new code. Architecture Compliance✅ Unidirectional Data Flow: Properly uses State/Action/Effect pattern Testing CoverageImplemented:
Missing:
Documentation✅ DocC comments present on:
✅ All public properties and methods have documentation Code Style✅ Follows Swift code style guidelines Action Items
🤖 Generated with Claude Code Co-Authored-By: Claude noreply@anthropic.com |
| "EncryptionSettingsUpdated" = "Encryption settings updated"; | ||
| "ItemTransfer" = "Item transfer"; | ||
| "TransferItemsToX" = "Transfer items to %1$@"; | ||
| "XIsRequiringAllItemsToBeOwnedByTheOrganization" = "%1$@ is requiring all items to be owned by the organization for security and compliance. Click accept to transfer ownership of your items."; |
There was a problem hiding this comment.
⛏️ Append DescriptionLong to the key to be consistent with other long strings such as TheNewRecommendedEncryptionSettingsDescriptionLong
There was a problem hiding this comment.
I'll add it but I notice we have some long strings not following this suffix rule that maybe we could address as tech dept for consistency.
There was a problem hiding this comment.
Indeed, those were added before we started appending the suffix and I agree they should be updated at some moment as tech-debt.
| "DeclineAndLeave" = "Decline and leave"; | ||
| "WhyAmISeeingThis" = "Why am I seeing this?"; | ||
| "AreYouSureYouWantToLeave" = "Are you sure you want to leave?"; | ||
| "ByDecliningYourPersonalItemsWillStayInYourAccount" = "By declining, your personal items will stay in your account, but you'll lose access to shared items and organization features."; |
There was a problem hiding this comment.
⛏️ Append DescriptionLong to the key to be consistent with other long strings such as TheNewRecommendedEncryptionSettingsDescriptionLong
| /// A markdown link to Bitwarden's terms of service. | ||
| static let termsOfService = URL(string: "https://bitwarden.com/terms/")! | ||
|
|
||
| /// A markdown link to Bitwarden's terms of service. |
| /// A link to Bitwarden's help page for My vault items migration. | ||
| static let websiteIconsHelp = URL(string: "https://bitwarden.com/help/website-icons/")! |
There was a problem hiding this comment.
There was a problem hiding this comment.
Changed the wrong one 😶🌫️
| defer { coordinator.hideLoadingOverlay() } | ||
| coordinator.navigate(to: .dismiss()) |
There was a problem hiding this comment.
🤔 Should hide loading be deferred to after the coordinator dismisses the view?
| guard let organizationId = organizationIds.first else { | ||
| coordinator.showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred)) | ||
| return | ||
| } |
There was a problem hiding this comment.
🤔 What happens with the state of the view in this case? Does the user have any feedback after "An error has occurred" of what to do from there? Should the user stay on this view or go back?
❓ If there are more organizations with the personal ownership policy enabled, is it ok that this always take into consideration the first one?
There was a problem hiding this comment.
I'll defer most of the logic and test of the processor to another PR, it is basically a scaffolding as it is now.
|
|
||
| state.organizationName = organization?.name ?? "" | ||
| } catch { | ||
| services.errorReporter.log(error: error) |
There was a problem hiding this comment.
🤔 Same case, should it go back if an error is thrown and caught here?
| /// The transfer page renders correctly. | ||
| @MainActor | ||
| func test_snapshot_transferPage() { | ||
| assertSnapshots( | ||
| of: subject.navStackWrapped, | ||
| as: [.defaultPortrait, .defaultPortraitDark, .defaultPortraitAX5], | ||
| ) | ||
| } | ||
|
|
||
| /// The decline confirmation page renders correctly. | ||
| @MainActor | ||
| func test_snapshot_declineConfirmationPage() { | ||
| processor.state.page = .declineConfirmation | ||
| assertSnapshots( | ||
| of: subject.navStackWrapped, | ||
| as: [.defaultPortrait, .defaultPortraitDark, .defaultPortraitAX5], | ||
| ) | ||
| } |
There was a problem hiding this comment.
⛏️ Rename these tests before merging to disabletest_ so it's consistent with others until we decide to enable all snapshot tests again.
|
|
||
| /// A view that prompts the user to accept or decline an item transfer request from an organization. | ||
| /// | ||
| struct MigrateToMyItemsView: View { |
There was a problem hiding this comment.
Can the user navigate correctly through this views/pages using VoiceOver?
There was a problem hiding this comment.
Tested with the Accessibility inspector and it looked fine 👍
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28997
📔 Objective
This pull request introduces the initial implementation of the "Migrate to My Items" feature, which guides users through transferring ownership of vault items to their organization or leaving the organization if they decline. It includes new UI views, state management, processor logic, localized strings, and supporting assets.
These changes lay the groundwork for the "Migrate to My Items" feature, providing a interface and state-driven logic for handling organization item ownership policies.
📸 Screenshots
In the screenshots the organization name is missing, it will be added later on.
⏰ 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