Implement account deletion #4914
Conversation
IOS-229 Implement account deletion button and logic
https://mullvad.atlassian.net/wiki/spaces/CLIEN/pages/3293413377/Account+deletion+on+iOS Implement account deletion button, dialogue and logic. See linked confluence page for details. |
mojganii
added
iOS
There was a problem hiding this comment.
Reviewed 3 of 17 files at r1, all commit messages.
Reviewable status: 3 of 17 files reviewed, 2 unresolved discussions (waiting on @mojganii)
ios/MullvadREST/RESTAccountsProxy.swift line 120 at r1 (raw file):
return addOperation( name: "delete-my-account",
NIT: This could just be delete-account.
ios/MullvadREST/RESTRequestFactory.swift line 122 at r1 (raw file):
} mutating func addValue(_ forHTTPHeaderField: String, value: String) {
I think this function should be called insertHeader or addHeader, as it wasn't obvious to me that a header was being added here.
There was a problem hiding this comment.
Reviewed 9 of 17 files at r1.
Reviewable status: 12 of 17 files reviewed, 3 unresolved discussions (waiting on @mojganii)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift line 22 at r1 (raw file):
"INVALID_ACCOUNT_NUMBER", tableName: "Account", value: "The four last digits of the account number is invalid.",
"Last four digits of the account number are incorrect" would be better, I think.
There was a problem hiding this comment.
Reviewable status: 12 of 17 files reviewed, 4 unresolved discussions (waiting on @mojganii)
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 467 at r1 (raw file):
) operation.addCondition(MutuallyExclusive(category: OperationCategory.deviceStateUpdate.category))
I take it this is so that the tunnel manager won't try to update the device state whilst an account deletion is taking place, right?
There was a problem hiding this comment.
Reviewable status: 12 of 17 files reviewed, 4 unresolved discussions (waiting on @pinkisemils)
ios/MullvadREST/RESTAccountsProxy.swift line 120 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
NIT: This could just be
delete-account.
for keeping the naming consistency I've gotten inspiration the pattern we've chosen for other operations on AccountsProxy
ios/MullvadREST/RESTRequestFactory.swift line 122 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
I think this function should be called
insertHeaderoraddHeader, as it wasn't obvious to me that a header was being added here.
I've borrowed the name Apple used for adding header to URLRequest.
https://developer.apple.com/documentation/foundation/urlrequest/2011522-addvalue
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 467 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
I take it this is so that the tunnel manager won't try to update the device state whilst an account deletion is taking place, right?
Indeed
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift line 22 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
"Last four digits of the account number are incorrect" would be better, I think.
Done.
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
2e310c1
to
6e91a3c
Compare
There was a problem hiding this comment.
Reviewed 5 of 17 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)
ios/CHANGELOG.md line 34 at r2 (raw file):
- Add redeeming voucher code on account view. - Add WireGuard port selection to settings. - Add account deletion ability to account view.
I've updated the changelog since all entries here except the one that is being added now are a part of the app that just was released. So you should rebase this and move the entry into the unreleased part.
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 467 at r1 (raw file):
Previously, mojganii wrote…
Indeed
There was no need to remove this, I am OK with these operations being mutually exclusive 😅
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
6e91a3c
to
e5b99a1
Compare
There was a problem hiding this comment.
Reviewable status: 15 of 17 files reviewed, 1 unresolved discussion (waiting on @pinkisemils)
ios/CHANGELOG.md line 34 at r2 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
I've updated the changelog since all entries here except the one that is being added now are a part of the app that just was released. So you should rebase this and move the entry into the unreleased part.
Done.
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 467 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
There was no need to remove this, I am OK with these operations being mutually exclusive 😅
Reverted!
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
e5b99a1
to
58b0b04
Compare
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
58b0b04
to
f8ddb37
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 5 of 17 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)
ios/MullvadREST/RESTAccountsProxy.swift line 89 at r4 (raw file):
accountNumber: String, retryStrategy: RetryStrategy, completion: @escaping CompletionHandler<Bool>
This doesn't need to be CompletionHandler<Bool> it can be CompletionHandler<Void> instead, the boolean value doesn't add any information since .success already tells us that the operation worked. The same goes for the DeleteAccountOperation and in the TunnelManager
Here's what the patch looks like after all the changes (Unfortunately I can't attach a diff file, apologies in advance for the seeminly big code blob here)
diff --git a/ios/MullvadREST/RESTAccountsProxy.swift b/ios/MullvadREST/RESTAccountsProxy.swift
index 4b1fa12a0..42669a794 100644
--- a/ios/MullvadREST/RESTAccountsProxy.swift
+++ b/ios/MullvadREST/RESTAccountsProxy.swift
@@ -86,7 +86,7 @@ extension REST {
public func deleteAccount(
accountNumber: String,
retryStrategy: RetryStrategy,
- completion: @escaping CompletionHandler<Bool>
+ completion: @escaping CompletionHandler<Void>
) -> Cancellable {
let requestHandler = AnyRequestHandler(createURLRequest: { endpoint, authorization in
var requestBuilder = try self.requestFactory.createRequestBuilder(
@@ -100,12 +100,12 @@ extension REST {
return requestBuilder.getRequest()
}, authorizationProvider: createAuthorizationProvider(accountNumber: accountNumber))
- let responseHandler = AnyResponseHandler { response, data -> ResponseHandlerResult<Bool> in
+ let responseHandler = AnyResponseHandler { response, data -> ResponseHandlerResult<Void> in
let statusCode = HTTPStatus(rawValue: response.statusCode)
switch statusCode {
case let statusCode where statusCode.isSuccess:
- return .success(true)
+ return .success(())
default:
return .unhandledResponse(
try? self.responseDecoder.decode(
diff --git a/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift b/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift
index 906228c91..3230e9fde 100644
--- a/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift
+++ b/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift
@@ -12,7 +12,7 @@ import MullvadREST
import MullvadTypes
import Operations
-class DeleteAccountOperation: ResultOperation<Bool> {
+class DeleteAccountOperation: ResultOperation<Void> {
private let logger = Logger(label: "\(DeleteAccountOperation.self)")
private let accountNumber: String
@@ -36,8 +36,8 @@ class DeleteAccountOperation: ResultOperation<Bool> {
completion: { [weak self] result in
self?.dispatchQueue.async {
switch result {
- case let .success(isDeleted):
- self?.finish(result: .success(isDeleted))
+ case .success:
+ self?.finish(result: .success(()))
case let .failure(error):
self?.logger.error(
error: error,
diff --git a/ios/MullvadVPN/TunnelManager/TunnelManager.swift b/ios/MullvadVPN/TunnelManager/TunnelManager.swift
index f45cb121f..6ace5425a 100644
--- a/ios/MullvadVPN/TunnelManager/TunnelManager.swift
+++ b/ios/MullvadVPN/TunnelManager/TunnelManager.swift
@@ -433,7 +433,7 @@ final class TunnelManager: StorePaymentObserver {
func deleteAccount(
accountNumber: String,
- completion: ((Result<Bool, Error>) -> Void)? = nil
+ completion: ((Result<Void, Error>) -> Void)? = nil
) -> Cancellable {
let operation = DeleteAccountOperation(
dispatchQueue: internalQueue,
@@ -449,7 +449,7 @@ final class TunnelManager: StorePaymentObserver {
self?.operationQueue.cancelAllOperations()
self?.wipeAllUserData()
self?.setDeviceState(.loggedOut, persist: true)
- completion?(.success(true))
+ completion?(.success(()))
}
case let .failure(error):
completion?(.failure(error))
diff --git a/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift b/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift
index f818054d3..d0dfd7458 100644
--- a/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift
+++ b/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift
@@ -48,7 +48,7 @@ class AccountDeletionInteractor {
}
}
- func delete(accountNumber: String, completionHandler: @escaping (Result<Bool, Error>) -> Void) -> Cancellable {
+ func delete(accountNumber: String, completionHandler: @escaping (Result<Void, Error>) -> Void) -> Cancellable {
return tunnelManager.deleteAccount(accountNumber: accountNumber, completion: completionHandler)
}
}ios/MullvadREST/RESTRequestFactory.swift line 122 at r4 (raw file):
} mutating func addValue(_ forHTTPHeaderField: String, value: String) {
If we are copying Apple's API names, then this should be named
addValue(_ value: String, forHTTPHeaderField field: String)
Here it's confusing which one is the HTTP header field and which one is the value otherwise.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 65 at r4 (raw file):
tableName: "Account", value: """ This logs out all devices using this account and all VPN access will be denied even if there is time left on the account. Enter the last 4 digits of the account number and hit OK if you really want to delete the account:
nit: Do we need the : at the end ?
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
f8ddb37
to
f580f79
Compare
There was a problem hiding this comment.
Reviewable status: 12 of 18 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @pinkisemils)
ios/MullvadREST/RESTAccountsProxy.swift line 89 at r4 (raw file):
Previously, buggmagnet wrote…
This doesn't need to be
CompletionHandler<Bool>it can beCompletionHandler<Void>instead, the boolean value doesn't add any information since.successalready tells us that the operation worked. The same goes for theDeleteAccountOperationand in theTunnelManagerHere's what the patch looks like after all the changes (Unfortunately I can't attach a diff file, apologies in advance for the seeminly big code blob here)
diff --git a/ios/MullvadREST/RESTAccountsProxy.swift b/ios/MullvadREST/RESTAccountsProxy.swift index 4b1fa12a0..42669a794 100644 --- a/ios/MullvadREST/RESTAccountsProxy.swift +++ b/ios/MullvadREST/RESTAccountsProxy.swift @@ -86,7 +86,7 @@ extension REST { public func deleteAccount( accountNumber: String, retryStrategy: RetryStrategy, - completion: @escaping CompletionHandler<Bool> + completion: @escaping CompletionHandler<Void> ) -> Cancellable { let requestHandler = AnyRequestHandler(createURLRequest: { endpoint, authorization in var requestBuilder = try self.requestFactory.createRequestBuilder( @@ -100,12 +100,12 @@ extension REST { return requestBuilder.getRequest() }, authorizationProvider: createAuthorizationProvider(accountNumber: accountNumber)) - let responseHandler = AnyResponseHandler { response, data -> ResponseHandlerResult<Bool> in + let responseHandler = AnyResponseHandler { response, data -> ResponseHandlerResult<Void> in let statusCode = HTTPStatus(rawValue: response.statusCode) switch statusCode { case let statusCode where statusCode.isSuccess: - return .success(true) + return .success(()) default: return .unhandledResponse( try? self.responseDecoder.decode( diff --git a/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift b/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift index 906228c91..3230e9fde 100644 --- a/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift +++ b/ios/MullvadVPN/TunnelManager/DeleteAccountOperation.swift @@ -12,7 +12,7 @@ import MullvadREST import MullvadTypes import Operations -class DeleteAccountOperation: ResultOperation<Bool> { +class DeleteAccountOperation: ResultOperation<Void> { private let logger = Logger(label: "\(DeleteAccountOperation.self)") private let accountNumber: String @@ -36,8 +36,8 @@ class DeleteAccountOperation: ResultOperation<Bool> { completion: { [weak self] result in self?.dispatchQueue.async { switch result { - case let .success(isDeleted): - self?.finish(result: .success(isDeleted)) + case .success: + self?.finish(result: .success(())) case let .failure(error): self?.logger.error( error: error, diff --git a/ios/MullvadVPN/TunnelManager/TunnelManager.swift b/ios/MullvadVPN/TunnelManager/TunnelManager.swift index f45cb121f..6ace5425a 100644 --- a/ios/MullvadVPN/TunnelManager/TunnelManager.swift +++ b/ios/MullvadVPN/TunnelManager/TunnelManager.swift @@ -433,7 +433,7 @@ final class TunnelManager: StorePaymentObserver { func deleteAccount( accountNumber: String, - completion: ((Result<Bool, Error>) -> Void)? = nil + completion: ((Result<Void, Error>) -> Void)? = nil ) -> Cancellable { let operation = DeleteAccountOperation( dispatchQueue: internalQueue, @@ -449,7 +449,7 @@ final class TunnelManager: StorePaymentObserver { self?.operationQueue.cancelAllOperations() self?.wipeAllUserData() self?.setDeviceState(.loggedOut, persist: true) - completion?(.success(true)) + completion?(.success(())) } case let .failure(error): completion?(.failure(error)) diff --git a/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift b/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift index f818054d3..d0dfd7458 100644 --- a/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift +++ b/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift @@ -48,7 +48,7 @@ class AccountDeletionInteractor { } } - func delete(accountNumber: String, completionHandler: @escaping (Result<Bool, Error>) -> Void) -> Cancellable { + func delete(accountNumber: String, completionHandler: @escaping (Result<Void, Error>) -> Void) -> Cancellable { return tunnelManager.deleteAccount(accountNumber: accountNumber, completion: completionHandler) } }
You are right.first I consider that server returns different Status code for the account has been removed already.
Done!
ios/MullvadREST/RESTRequestFactory.swift line 122 at r4 (raw file):
Previously, buggmagnet wrote…
If we are copying Apple's API names, then this should be named
addValue(_ value: String, forHTTPHeaderField field: String)Here it's confusing which one is the HTTP header field and which one is the value otherwise.
Done.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 65 at r4 (raw file):
Previously, buggmagnet wrote…
nit: Do we need the
:at the end ?
Done!
There was a problem hiding this comment.
Reviewed 5 of 6 files at r5, all commit messages.
Reviewable status: 17 of 18 files reviewed, 4 unresolved discussions (waiting on @mojganii and @pinkisemils)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 322 at r5 (raw file):
switch Action(rawValue: button.accessibilityIdentifier ?? "") { case .ok: delegate?.didTapDeleteButtonButton(contentView: self, button: button)
This enum case should really be delete instead of ok, because otherwise this line is very confusing to read.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift line 51 at r5 (raw file):
} func delete(accountNumber: String, completionHandler: @escaping (Result<Bool, Error>) -> Void) -> Cancellable {
This should also be Result<Void, Error>
Sorry if I missed it the first time
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionViewController.swift line 13 at r5 (raw file):
protocol AccountDeletionViewControllerDelegate: AnyObject { func deleteAccountDidSucceeded(controller: AccountDeletionViewController)
This should be named deleteAccountDidSucceed since we already have did we don't need the ed suffix.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionViewController.swift line 83 at r5 (raw file):
} func didTapDeleteButtonButton(contentView: AccountDeletionContentView, button: AppButton) {
I believe you meant didTapDeleteButton
There was a problem hiding this comment.
Reviewable status: 17 of 18 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @pinkisemils)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 322 at r5 (raw file):
Previously, buggmagnet wrote…
This enum case should really be
deleteinstead ofok, because otherwise this line is very confusing to read.
Done.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionInteractor.swift line 51 at r5 (raw file):
Previously, buggmagnet wrote…
This should also be
Result<Void, Error>Sorry if I missed it the first time
Done.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionViewController.swift line 13 at r5 (raw file):
Previously, buggmagnet wrote…
This should be named
deleteAccountDidSucceedsince we already havedidwe don't need theedsuffix.
Done.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionViewController.swift line 83 at r5 (raw file):
Previously, buggmagnet wrote…
I believe you meant
didTapDeleteButton
Done.
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
2 times, most recently
from
88936ee
to
04f4f88
Compare
There was a problem hiding this comment.
Reviewed 2 of 17 files at r1, 1 of 6 files at r5, 8 of 11 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 17 of 19 files reviewed, 4 unresolved discussions (waiting on @mojganii and @pinkisemils)
ios/MullvadVPN/Classes/AutomaticKeyboardResponder.swift line 23 at r6 (raw file):
This is not needed anymore since iOS 9.0
If your app targets iOS 9.0 and later or macOS 10.11 and later, and you used addObserver(_:selector:name:object:), you do not need to unregister the observer. If you forget or are unable to remove the observer, the system cleans up the next time it would have posted to it.
ios/MullvadVPN/Classes/AutomaticKeyboardResponder.swift line 162 at r6 (raw file):
guard let targetView, let superview = targetView.superview else { return } // layoutIfNeeded is a method that forces the view to immediately update its layout and the layout of its subviews.
I don't think we should be adding this comment, there already is documentation for the layoutIfneeded method
ios/MullvadVPN/Classes/AutomaticKeyboardResponder.swift line 163 at r6 (raw file):
// layoutIfNeeded is a method that forces the view to immediately update its layout and the layout of its subviews. targetView.layoutIfNeeded()
You want to call layoutIfNeeded after doing the frame changes, not before, otherwise the method has no effect.
Also I don't think it has any effect here, even if I remove the call to it, the buttons still jump around.
I suggest we remove these 2 layoutIfNeeded
ios/MullvadVPN/UI appearance/UIMetrics.swift line 39 at r7 (raw file):
enum RedeemVoucher { static let cornerRadius = 8.0 static let preferredContentSize = UIDevice.current.userInterfaceIdiom == .pad
This has been discussed this offline.
The conclusion is that we don't want to use static sizes because it doesn't work with dynamic layouts.
Instead we should let Autolayout work its magic.
Here is a patch that should address the resizing problem, we can expand on that and see if nothing else breaks by removing code.
diff --git a/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift b/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift
index e2a5271d3..7fbb6a548 100644
--- a/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift
+++ b/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift
@@ -25,9 +25,9 @@ struct ModalPresentationConfiguration {
vc.modalPresentationStyle = modalPresentationStyle
}
- if let preferredContentSize {
- vc.preferredContentSize = preferredContentSize
- }
+// if let preferredContentSize {
+// vc.preferredContentSize = preferredContentSize
+// }
if let isModalInPresentation {
vc.isModalInPresentation = isModalInPresentation
diff --git a/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift b/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift
index e472aa19e..e5c2f347f 100644
--- a/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift
+++ b/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift
@@ -53,27 +53,27 @@ class FormSheetPresentationController: UIPresentationController {
traitCollection.horizontalSizeClass == .compact
}
- override var frameOfPresentedViewInContainerView: CGRect {
- guard let containerView else {
- return super.frameOfPresentedViewInContainerView
- }
-
- if isInFullScreenPresentation {
- return containerView.bounds
- }
-
- let preferredContentSize = presentedViewController.preferredContentSize
-
- assert(preferredContentSize.width > 0 && preferredContentSize.height > 0)
-
- return CGRect(
- origin: CGPoint(
- x: containerView.bounds.midX - preferredContentSize.width * 0.5,
- y: containerView.bounds.midY - preferredContentSize.height * 0.5
- ),
- size: preferredContentSize
- )
- }
+// override var frameOfPresentedViewInContainerView: CGRect {
+// guard let containerView else {
+// return super.frameOfPresentedViewInContainerView
+// }
+//
+// if isInFullScreenPresentation {
+// return containerView.bounds
+// }
+//
+// let preferredContentSize = presentedViewController.preferredContentSize
+//
+// assert(preferredContentSize.width > 0 && preferredContentSize.height > 0)
+//
+// return CGRect(
+// origin: CGPoint(
+// x: containerView.bounds.midX - preferredContentSize.width * 0.5,
+// y: containerView.bounds.midY - preferredContentSize.height * 0.5
+// ),
+// size: preferredContentSize
+// )
+// }
override func containerViewWillLayoutSubviews() {
dimmingView.frame = containerView?.bounds ?? .zero
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
2 times, most recently
from
de3cbcd
to
471c2e0
Compare
There was a problem hiding this comment.
Reviewable status: 15 of 19 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @pinkisemils)
ios/MullvadVPN/Classes/AutomaticKeyboardResponder.swift line 23 at r6 (raw file):
Previously, buggmagnet wrote…
This is not needed anymore since iOS 9.0
If your app targets iOS 9.0 and later or macOS 10.11 and later, and you used addObserver(_:selector:name:object:), you do not need to unregister the observer. If you forget or are unable to remove the observer, the system cleans up the next time it would have posted to it.
Done.
ios/MullvadVPN/Classes/AutomaticKeyboardResponder.swift line 162 at r6 (raw file):
Previously, buggmagnet wrote…
I don't think we should be adding this comment, there already is documentation for the
layoutIfneededmethod
Done.
ios/MullvadVPN/Classes/AutomaticKeyboardResponder.swift line 163 at r6 (raw file):
Previously, buggmagnet wrote…
You want to call
layoutIfNeededafter doing the frame changes, not before, otherwise the method has no effect.
Also I don't think it has any effect here, even if I remove the call to it, the buttons still jump around.I suggest we remove these 2
layoutIfNeeded
Done
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/MullvadVPN/UI appearance/UIMetrics.swift line 39 at r7 (raw file):
Previously, buggmagnet wrote…
This has been discussed this offline.
The conclusion is that we don't want to use static sizes because it doesn't work with dynamic layouts.Instead we should let Autolayout work its magic.
Here is a patch that should address the resizing problem, we can expand on that and see if nothing else breaks by removing code.
diff --git a/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift b/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift index e2a5271d3..7fbb6a548 100644 --- a/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift +++ b/ios/MullvadVPN/Coordinators/Base/ModalPresentationConfiguration.swift @@ -25,9 +25,9 @@ struct ModalPresentationConfiguration { vc.modalPresentationStyle = modalPresentationStyle } - if let preferredContentSize { - vc.preferredContentSize = preferredContentSize - } +// if let preferredContentSize { +// vc.preferredContentSize = preferredContentSize +// } if let isModalInPresentation { vc.isModalInPresentation = isModalInPresentation diff --git a/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift b/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift index e472aa19e..e5c2f347f 100644 --- a/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift +++ b/ios/MullvadVPN/Presentation controllers/FormsheetPresentationController.swift @@ -53,27 +53,27 @@ class FormSheetPresentationController: UIPresentationController { traitCollection.horizontalSizeClass == .compact } - override var frameOfPresentedViewInContainerView: CGRect { - guard let containerView else { - return super.frameOfPresentedViewInContainerView - } - - if isInFullScreenPresentation { - return containerView.bounds - } - - let preferredContentSize = presentedViewController.preferredContentSize - - assert(preferredContentSize.width > 0 && preferredContentSize.height > 0) - - return CGRect( - origin: CGPoint( - x: containerView.bounds.midX - preferredContentSize.width * 0.5, - y: containerView.bounds.midY - preferredContentSize.height * 0.5 - ), - size: preferredContentSize - ) - } +// override var frameOfPresentedViewInContainerView: CGRect { +// guard let containerView else { +// return super.frameOfPresentedViewInContainerView +// } +// +// if isInFullScreenPresentation { +// return containerView.bounds +// } +// +// let preferredContentSize = presentedViewController.preferredContentSize +// +// assert(preferredContentSize.width > 0 && preferredContentSize.height > 0) +// +// return CGRect( +// origin: CGPoint( +// x: containerView.bounds.midX - preferredContentSize.width * 0.5, +// y: containerView.bounds.midY - preferredContentSize.height * 0.5 +// ), +// size: preferredContentSize +// ) +// } override func containerViewWillLayoutSubviews() { dimmingView.frame = containerView?.bounds ?? .zero
Fixed
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
2 times, most recently
from
8e57795
to
16036b2
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r9, 4 of 8 files at r10, all commit messages.
Reviewable status: 16 of 20 files reviewed, 4 unresolved discussions (waiting on @mojganii)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
guard self.accountTextField.isFirstResponder else { return } self.bottomsOfButtonsConstraint?.constant = -offset self.layoutIfNeeded()
Let's add a self.scrollView.flashScrollIndicators() before the layoutIfNeeded. It adds for a nicer UX.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionViewController.swift line 65 at r10 (raw file):
private func disableEditing() { guard contentView.isEditing else { return } self.contentView.isEditing = false
No need for self here
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherViewController.swift line 60 at r10 (raw file):
override func viewWillTransition(to size: CGSize, with coordinator: UIViewControllerTransitionCoordinator) { self.contentView.isEditing = false
We can remove self here and in enableEditing and disableEditing
There was a problem hiding this comment.
Reviewable status: 16 of 20 files reviewed, 4 unresolved discussions (waiting on @mojganii)
There was a problem hiding this comment.
Reviewed 4 of 8 files at r10.
Reviewable status: 19 of 20 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/UI appearance/UIMetrics.swift line 39 at r7 (raw file):
Previously, mojganii wrote…
Fixed
The default implementation of var frameOfPresentedViewInContainerView: CGRect returns the frame of container view which is used during rotation and presentation to set the presented view size. So commenting it out is probably not going to work.
I think traditionally preferredContentSize when set to .zero or lower is treated as intrinsic. But that has to be handled in the presentation controller.
If we were to handle that case then we could calculate the frame of the presented view using AutoLayout engine, i.e:
guard let presentedView else { return super.frameOfPresentedViewInContainerView }
var preferred = preferredContentSize
var calculated: CGSize
if preferred.width <= 0 || preferred.height <= 0 {
let layoutSize = CGSize(
width: preferred.width <= 0 ? UIView.layoutFittingCompressedSize.width : preferred.width,
height: preferred.height <= 0 ? UIView.layoutFittingCompressedSize.height : preferred.height
)
calculated = presentedView.systemLayoutSizeFitting(layoutSize)
} else {
calculated = preferred
}
return CGRect(...)This article could shed some light on how to compute view size: https://useyourloaf.com/blog/self-sizing-popovers/
I do however hope that such changes could be tested in isolation and not merged as a part of larger PR if we opt-in to work on that.
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
16036b2
to
416ac3d
Compare
There was a problem hiding this comment.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 192 at r10 (raw file):
var isEditing = false { didSet { _ = accountTextField.isFirstResponder
didSet acts as a toggle. Shouldn't it respect the value of isEditing?
if isEditing {
accountTextField.becomeFirstResponder()
} else {
accountTextField.resignFirstResponder()
}
There was a problem hiding this comment.
Reviewed 1 of 6 files at r5.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
Previously, buggmagnet wrote…
Let's add a
self.scrollView.flashScrollIndicators()before thelayoutIfNeeded. It adds for a nicer UX.
This closure will be called in response to keyboard events so flashing scrollbars is better be done from viewDidAppear of the view controller hosting this view.
There was a problem hiding this comment.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @mojganii and @pronebird)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
This closure will be called in response to keyboard events so flashing scrollbars is better be done from
viewDidAppearof the view controller hosting this view.
The view controller doesn't (and shouldn't) know that the content view has a scroll view. Exposing the scroll view would break encapsulation.
Also, I don't think the scroll indicator will show if there's nothing to scroll, so calling it before the keyboard appears on screen wouldn't work
There was a problem hiding this comment.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)
ios/MullvadVPN/UI appearance/UIMetrics.swift line 39 at r7 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
The default implementation of
var frameOfPresentedViewInContainerView: CGRectreturns the frame of container view which is used during rotation and presentation to set the presented view size. So commenting it out is probably not going to work.I think traditionally
preferredContentSizewhen set to.zeroor lower is treated as intrinsic. But that has to be handled in the presentation controller.If we were to handle that case then we could calculate the frame of the presented view using AutoLayout engine, i.e:
guard let presentedView else { return super.frameOfPresentedViewInContainerView } var preferred = preferredContentSize var calculated: CGSize if preferred.width <= 0 || preferred.height <= 0 { let layoutSize = CGSize( width: preferred.width <= 0 ? UIView.layoutFittingCompressedSize.width : preferred.width, height: preferred.height <= 0 ? UIView.layoutFittingCompressedSize.height : preferred.height ) calculated = presentedView.systemLayoutSizeFitting(layoutSize) } else { calculated = preferred } return CGRect(...)This article could shed some light on how to compute view size: https://useyourloaf.com/blog/self-sizing-popovers/
I do however hope that such changes could be tested in isolation and not merged as a part of larger PR if we opt-in to work on that.
I didn't touch this part.but we can spend time on it for the future to make that adaptable for different size classes
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 192 at r10 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
didSetacts as a toggle. Shouldn't it respect the value ofisEditing?if isEditing { accountTextField.becomeFirstResponder() } else { accountTextField.resignFirstResponder() }
Nope this property is for toggling value for out of the class and I'm changing that at viewWillTransition(to size: CGSize, with coordinator: UIViewControllerTransitionCoordinator).
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
Previously, buggmagnet wrote…
Let's add a
self.scrollView.flashScrollIndicators()before thelayoutIfNeeded. It adds for a nicer UX.
Done.
There was a problem hiding this comment.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 192 at r10 (raw file):
Previously, mojganii wrote…
Nope this property is for toggling value for out of the class and I'm changing that at
viewWillTransition(to size: CGSize, with coordinator: UIViewControllerTransitionCoordinator).
done
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
416ac3d
to
75c397b
Compare
There was a problem hiding this comment.
Reviewable status: 15 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionViewController.swift line 65 at r10 (raw file):
Previously, buggmagnet wrote…
No need for
selfhere
Done.
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherViewController.swift line 60 at r10 (raw file):
Previously, buggmagnet wrote…
We can remove
selfhere and inenableEditinganddisableEditing
Done.
There was a problem hiding this comment.
Reviewed 2 of 17 files at r1, 1 of 1 files at r4, 2 of 6 files at r5, 1 of 11 files at r6, 1 of 8 files at r10, 5 of 5 files at r12.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/UI appearance/UIMetrics.swift line 39 at r7 (raw file):
Previously, mojganii wrote…
I didn't touch this part.but we can spend time on it for the future to make that adaptable for different size classes
Cool. Agreed.
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
@buggmagnet that's how it's been with standard UITableViewController, see documentation states that explicitly:
- Implements the
viewDidAppear(_:)method and automatically flashes the table view’s scroll indicators when it first appears
https://developer.apple.com/documentation/uikit/uitableviewcontroller
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 276 at r12 (raw file):
override func didMoveToWindow() { if self.window != nil { NotificationCenter.default.addObserver(
Nit: technically you could add it from init() and never unregister it. Notifications for selector based handlers are automatically unregistered around time when observer (self) deinits and they don't retain the observer.
There was a problem hiding this comment.
Reviewed 1 of 8 files at r10, 2 of 5 files at r12.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @pronebird)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
@buggmagnet that's how it's been with standard
UITableViewController, see documentation states that explicitly:
- Implements the
viewDidAppear(_:)method and automatically flashes the table view’s scroll indicators when it first appearshttps://developer.apple.com/documentation/uikit/uitableviewcontroller
Right I understand that, but this is not a UITableViewController that we're dealing with here, and it's a separate problem.
The UX we want to have is that a view that's not scrollable becomes scrollable in certain cases (in our case, the content doesn't fit the screen size anymore because the keyboard eats the screen estate) .
When it becomes scrollable, we want to flash the scroll indicator.
It's not because UITableViewController flashes it in viewDidAppear that it means it's the only place where it should be called.
If you want to test is yourself, look in the iOS settings.
If you go to the VPN section, you won't see the flash indicator scrolling, because there is not enough content to scroll through.
However, if you open the General section, you will see the flash indicator scrolling about after a second.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
Previously, buggmagnet wrote…
Right I understand that, but this is not a
UITableViewControllerthat we're dealing with here, and it's a separate problem.
The UX we want to have is that a view that's not scrollable becomes scrollable in certain cases (in our case, the content doesn't fit the screen size anymore because the keyboard eats the screen estate) .
When it becomes scrollable, we want to flash the scroll indicator.It's not because
UITableViewControllerflashes it inviewDidAppearthat it means it's the only place where it should be called.
If you want to test is yourself, look in the iOS settings.If you go to the
VPNsection, you won't see the flash indicator scrolling, because there is not enough content to scroll through.
However, if you open theGeneralsection, you will see the flash indicator scrolling about after a second.
Alright. I don't recall stock apps flashing scroll indicators in response to keyboard but if you think it improves UX then it's fine by me.
My only suggestion would be, could we please use [weak self] here because the closure is retained by AutomaticKeyboardResponder and keyboardResponder is retained by self, so that would ultimately leak memory unless we break it with [weak self].
There was a problem hiding this comment.
Reviewed 2 of 11 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 436 at r12 (raw file):
func deleteAccount( accountNumber: String, completion: ((Result<Void, Error>) -> Void)? = nil
Nit: if no value is expected to be returned then you can instead of Result<Void, Error> you can use Error? instead. where nil is success, and .some(Error) means error.
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 448 at r12 (raw file):
switch result { case .success: self?.unsetTunnelConfiguration {
Nit: I'd rather see it being a part of DeleteAccountOperation and then you could cancel all operations from completion handler.
There was a problem hiding this comment.
Reviewed 1 of 17 files at r1, 2 of 11 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
75c397b
to
6e0685c
Compare
There was a problem hiding this comment.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)
ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift line 356 at r10 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Alright. I don't recall stock apps flashing scroll indicators in response to keyboard but if you think it improves UX then it's fine by me.
My only suggestion would be, could we please use
[weak self]here because the closure is retained byAutomaticKeyboardResponderandkeyboardResponderis retained byself, so that would ultimately leak memory unless we break it with[weak self].
good catch.Done!
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
6e0685c
to
11dc5ea
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 20 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @pronebird)
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 436 at r12 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Nit: if no value is expected to be returned then you can instead of
Result<Void, Error>you can useError?instead. wherenilis success, and.some(Error)means error.
Done
ios/MullvadVPN/TunnelManager/TunnelManager.swift line 448 at r12 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Nit: I'd rather see it being a part of
DeleteAccountOperationand then you could cancel all operations from completion handler.
the reason I've considered that in tunnel was,if user deleted the account via other devices and runs the app for the account that is no longer existing, I need to cancel all ongoing operations as well.for the sake of preventing from duplication code I aggregated all in tunnel.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r14.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)
mojganii
force-pushed
the
implement-account-deletion-button-and-logic-ios-229
branch
from
11dc5ea
to
e5b91ab
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r15.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
pronebird
deleted the
implement-account-deletion-button-and-logic-ios-229
branch
This PR aims to enable users to delete their accounts, giving them more control over the data they have shared with the service. To achieve this, several steps have been implemented:
Addition of an
Account Deletebutton on theAccountview to facilitate the account deletion process.Implementation of a warning message to inform the user about the consequences of deleting their account. This step ensures users are fully aware of the actions they are taking.
User verification is required to delete the account. To prevent accidental deletions, users are asked to input the four last digits of their account number. This ensures that the user is making a deliberate decision and minimizes the chances of accidental deletions.
Service revocation is performed to notify the backend about the user's decision to delete their account.
After the account has been successfully removed, the following actions are taken:
Unset VPN configuration to ensure the user's data is no longer accessible through any VPN connection.
Cancel all ongoing operations associated with the user's account.
Remove the user's account number from the system to protect their privacy.
Redirect the user to a
logged outstate, indicating that the account deletion process was successful.In case of any failure during the process, appropriate error messages are displayed to the user to inform them of the issue.
Overall, this PR enhances the user experience by providing a secure and deliberate process for account deletion, safeguarding user data and privacy.
This change is