-
Notifications
You must be signed in to change notification settings - Fork 87
[PM-28997] Migrate personal vault to My Items #2196
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
Changes from all commits
11155e7
987a85c
e2356e4
670ea84
358f81c
1789eb8
20c3d1e
5a051b3
c89daf4
8584963
7cec72d
7a3d92a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "item-transfer-warning.pdf", | ||
| "idiom" : "universal" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "author" : "xcode", | ||
| "version" : 1 | ||
| }, | ||
| "properties" : { | ||
| "preserves-vector-representation" : true | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "item-transfer.pdf", | ||
| "idiom" : "universal" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "author" : "xcode", | ||
| "version" : 1 | ||
| }, | ||
| "properties" : { | ||
| "preserves-vector-representation" : true | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // MARK: - MigrateToMyItemsAction | ||
|
|
||
| /// Actions that can be processed by a `MigrateToMyItemsProcessor`. | ||
| /// | ||
| enum MigrateToMyItemsAction: Equatable, Sendable { | ||
| /// The user tapped the back button on the decline confirmation screen. | ||
| case backTapped | ||
|
|
||
| /// The user tapped the "Decline and leave" button. | ||
| case declineAndLeaveTapped | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // MARK: - MigrateToMyItemsEffect | ||
|
|
||
| /// Effects that can be processed by a `MigrateToMyItemsProcessor`. | ||
| /// | ||
| enum MigrateToMyItemsEffect: Equatable, Sendable { | ||
| /// The user tapped the "Accept transfer" button. | ||
| case acceptTransferTapped | ||
|
|
||
| /// The view appeared on screen. | ||
| case appeared | ||
|
|
||
| /// The user tapped the "Leave {organization}" button to confirm leaving. | ||
| case leaveOrganizationTapped | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| import BitwardenKit | ||
| import BitwardenResources | ||
| import Foundation | ||
|
|
||
| // MARK: - MigrateToMyItemsProcessorDelegate | ||
|
|
||
| /// A delegate for the `MigrateToMyItemsProcessor` to communicate events back to the coordinator. | ||
| /// | ||
| @MainActor | ||
| protocol MigrateToMyItemsProcessorDelegate: AnyObject { | ||
| /// Called when the user has left the organization. | ||
| /// | ||
| func didLeaveOrganization() | ||
| } | ||
|
|
||
| // MARK: - MigrateToMyItemsProcessor | ||
|
|
||
| /// The processor used to manage state and handle actions for the migrate to my items screen. | ||
| /// | ||
| final class MigrateToMyItemsProcessor: StateProcessor< | ||
| MigrateToMyItemsState, | ||
| MigrateToMyItemsAction, | ||
| MigrateToMyItemsEffect, | ||
| > { | ||
| // MARK: Types | ||
|
|
||
| typealias Services = HasErrorReporter | ||
| & HasPolicyService | ||
| & HasVaultRepository | ||
|
|
||
| // MARK: Private Properties | ||
|
|
||
| /// The coordinator that handles navigation. | ||
| private let coordinator: AnyCoordinator<VaultItemRoute, VaultItemEvent> | ||
|
|
||
| /// The delegate to notify of events. | ||
| private weak var delegate: MigrateToMyItemsProcessorDelegate? | ||
|
|
||
| /// The services used by this processor. | ||
| private let services: Services | ||
|
|
||
| // MARK: Initialization | ||
|
|
||
| /// Creates a new `MigrateToMyItemsProcessor`. | ||
| /// | ||
| /// - Parameters: | ||
| /// - coordinator: The coordinator that handles navigation. | ||
| /// - delegate: The delegate to notify of events. | ||
| /// - services: The services required by this processor. | ||
| /// - state: The initial state of the processor. | ||
| /// | ||
| init( | ||
| coordinator: AnyCoordinator<VaultItemRoute, VaultItemEvent>, | ||
| delegate: MigrateToMyItemsProcessorDelegate?, | ||
| services: Services, | ||
| state: MigrateToMyItemsState, | ||
| ) { | ||
| self.coordinator = coordinator | ||
| self.delegate = delegate | ||
| self.services = services | ||
| super.init(state: state) | ||
| } | ||
|
|
||
| // MARK: Methods | ||
|
|
||
| override func perform(_ effect: MigrateToMyItemsEffect) async { | ||
| switch effect { | ||
| case .acceptTransferTapped: | ||
| await acceptTransfer() | ||
| case .appeared: | ||
| await loadOrganizationName() | ||
| case .leaveOrganizationTapped: | ||
| await leaveOrganization() | ||
| } | ||
| } | ||
|
|
||
| override func receive(_ action: MigrateToMyItemsAction) { | ||
| switch action { | ||
| case .backTapped: | ||
| state.page = .transfer | ||
| case .declineAndLeaveTapped: | ||
| state.page = .declineConfirmation | ||
| } | ||
| } | ||
|
|
||
| // MARK: Private Methods | ||
|
|
||
| /// Accepts the item transfer. | ||
| /// | ||
| private func acceptTransfer() async { | ||
| coordinator.showLoadingOverlay(LoadingOverlayState(title: Localizations.loading)) | ||
|
|
||
| // TODO: PM-29709 Implement accept transfer API call | ||
|
|
||
| defer { coordinator.hideLoadingOverlay() } | ||
| coordinator.navigate(to: .dismiss()) | ||
| } | ||
|
|
||
| /// Leaves the organization after declining the item transfer. | ||
| /// | ||
| private func leaveOrganization() async { | ||
| coordinator.showLoadingOverlay(LoadingOverlayState(title: Localizations.loading)) | ||
|
|
||
| // TODO: PM-29710 Implement leave organization API call | ||
|
|
||
| defer { coordinator.hideLoadingOverlay() } | ||
| delegate?.didLeaveOrganization() | ||
| } | ||
|
|
||
| /// Loads the organization name from the policy service and vault repository. | ||
| /// | ||
| private func loadOrganizationName() async { | ||
| do { | ||
| let organizationIds = await services.policyService.organizationsApplyingPolicyToUser(.personalOwnership) | ||
| guard let organizationId = organizationIds.first else { | ||
| coordinator.showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred)) | ||
| return | ||
| } | ||
|
Comment on lines
+115
to
+118
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ค 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll defer most of the logic and test of the processor to another PR, it is basically a scaffolding as it is now. |
||
| let organization = try await services.vaultRepository.fetchOrganization(withId: organizationId) | ||
| // TODO: PM-29113 Validate if user must do vault migration and error handling | ||
|
|
||
| state.organizationName = organization?.name ?? "" | ||
| } catch { | ||
| services.errorReporter.log(error: error) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ค Same case, should it go back if an error is thrown and caught here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import Foundation | ||
|
|
||
| // MARK: - MigrateToMyItemsState | ||
|
|
||
| /// An object that defines the current state of a `MigrateToMyItemsView`. | ||
| /// | ||
| struct MigrateToMyItemsState: Equatable, Sendable { | ||
| // MARK: Types | ||
|
|
||
| /// An enumeration of the pages in the migrate to my items flow. | ||
| /// | ||
| enum Page: Equatable, Sendable { | ||
| /// The main screen prompting the user to accept or decline the transfer. | ||
| case transfer | ||
|
|
||
| /// The confirmation screen shown when the user chooses to decline. | ||
| case declineConfirmation | ||
| } | ||
|
|
||
| // MARK: Properties | ||
|
|
||
| /// The name of the organization requesting the item transfer. | ||
| var organizationName: String = "" | ||
|
|
||
| /// The current page being displayed. | ||
| var page: Page = .transfer | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // swiftlint:disable:this file_name | ||
| import BitwardenKit | ||
| import BitwardenKitMocks | ||
| import SnapshotTesting | ||
| import XCTest | ||
|
|
||
| @testable import BitwardenShared | ||
|
|
||
| class MigrateToMyItemsViewSnapshotTests: BitwardenTestCase { | ||
| // MARK: Properties | ||
|
|
||
| var processor: MockProcessor<MigrateToMyItemsState, MigrateToMyItemsAction, MigrateToMyItemsEffect>! | ||
| var subject: MigrateToMyItemsView! | ||
|
|
||
| // MARK: Setup & Teardown | ||
|
|
||
| override func setUp() { | ||
| super.setUp() | ||
|
|
||
| processor = MockProcessor(state: MigrateToMyItemsState(organizationName: "Acme Corporation")) | ||
| let store = Store(processor: processor) | ||
|
|
||
| subject = MigrateToMyItemsView(store: store) | ||
| } | ||
|
|
||
| override func tearDown() { | ||
| super.tearDown() | ||
|
|
||
| processor = nil | ||
| subject = nil | ||
| } | ||
|
|
||
| // MARK: Snapshots | ||
|
|
||
| /// The transfer page renders correctly. | ||
| @MainActor | ||
| func disabletest_snapshot_transferPage() { | ||
| assertSnapshots( | ||
| of: subject.navStackWrapped, | ||
| as: [.defaultPortrait, .defaultPortraitDark, .defaultPortraitAX5], | ||
| ) | ||
| } | ||
|
|
||
| /// The decline confirmation page renders correctly. | ||
| @MainActor | ||
| func disabletest_snapshot_declineConfirmationPage() { | ||
| processor.state.page = .declineConfirmation | ||
| assertSnapshots( | ||
| of: subject.navStackWrapped, | ||
| as: [.defaultPortrait, .defaultPortraitDark, .defaultPortraitAX5], | ||
| ) | ||
| } | ||
| } |
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.
๐ค Should hide loading be deferred to after the coordinator dismisses the view?
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.
same as below