Skip to content
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.

Sign up for GitHub

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

On delete account screen, add account number validation during input #5069

Merged
merged 1 commit into from
Sep 11, 2023
Merged

On delete account screen, add account number validation during input #5069

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,18 @@ class AccountDeletionContentView: UIView {
}
}

private var isAccountNumberLengthSatisfied: Bool {
let length = accountTextField.text?.count ?? 0
return length == 4
private var isInputValid: Bool {
guard let input = accountTextField.text,
let accountNumber = viewModel?.accountNumber,
!accountNumber.isEmpty
else {
return false
}

let inputLengthIsValid = input.count == 4
let inputMatchesAccountNumber = accountNumber.suffix(4) == input

return inputLengthIsValid && inputMatchesAccountNumber
Comment on lines +264 to +267
Copy link
Contributor

Choose a reason for hiding this comment

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

Is inputLengthIsValid really needed?

accountNumber.suffix(4) == input seems to imply that input is indeed 4 characters long when it matches accountNumber.suffix(4)... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The param in .suffix() specifies a max length, meaning the result could be less than that. In practice the account number - if present - should never be less than 4 digits, so for most (or perhaps all realistic) cases it should work as intended.
Theoretically though, accountNumber.suffix(4) could for some reason return something less than 4 digits, and if so the condition would be incorrect (eg. 3 digit account number matching 3 digit input). It's a safeguard, but perhaps it's not really necessary...

Copy link
Contributor

@MrChocolatine MrChocolatine Sep 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In practice the account number - if present - should never be less than 4 digits

Yep this is what I assumed that'shy. But okay 👍 sorry for the bother!

}

weak var delegate: AccountDeletionContentViewDelegate?
Expand Down Expand Up @@ -334,7 +343,7 @@ class AccountDeletionContentView: UIView {
} else {
activityIndicator.stopAnimating()
}
deleteButton.isEnabled = isDeleteButtonEnabled && isAccountNumberLengthSatisfied
deleteButton.isEnabled = isDeleteButtonEnabled && isInputValid
Copy link
Contributor

Choose a reason for hiding this comment

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

Should isDeleteButtonEnabled be updated too?

Currently it still says the button should be enabled upon failure:

mullvadvpn-app/ios/MullvadVPN/View controllers/AccountDeletion/AccountDeletionContentView.swift

Lines 221 to 228 in 9cc7a43

private var isDeleteButtonEnabled: Bool {
switch state {
case .initial, .failure:
return true
case .loading:
return false
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to disable the button when loading (to avoid interaction while trying to delete the account), otherwise enabled.

statusLabel.text = text
statusLabel.textColor = textColor
}
Expand Down
Loading