Update API access methods functionality UI to conform with designs

[rablador]

---

This change is 

[linear]

IOS-411 Iterate on API access design

[buggmagnet]

The designs specify that the method currently in use should have a label "In use" below its title

Reviewed 53 of 66 files at r1, 8 of 9 files at r2, 6 of 7 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/AccessMethodRepository.swift line 53 at r3 (raw file):

            try writeApiAccessMethods(storedMethods)
        } catch {
            print("Could not update access methods: \(storedMethods) \nError: \(error)")

Shouldn't we use our logging API for this instead ?
This comment is valid of all the print uses

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 19 at r4 (raw file):

    static let shared = ProxyConfigurationTester()

    init() {}

nit
I'm guessing this was meant to be a singleton, but we didn't define the init as private which defeats the purpose of having a singleton in the first place.

Just a nit because I will fix it in my upcoming work

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/ShadowsocksSectionHandler.swift line 57 at r3 (raw file):

        var contentConfiguration = TextCellContentConfiguration()
        contentConfiguration.text = itemIdentifier.text
        contentConfiguration.setPlaceholder(type: .required)

Apologies if that was not discussed properly, but it turns out the password is .optional for the shadowsocks case in the end.
We can revert this change back to .optional

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodInteractor.swift line 15 at r4 (raw file):

struct EditAccessMethodInteractor: EditAccessMethodInteractorProtocol {
    let subject: CurrentValueSubject<AccessMethodViewModel, Never>
    let repo: AccessMethodRepositoryProtocol

Let's not use abbreviations if we can afford writing the full word.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 334 at r4 (raw file):

                "METHOD_SETTINGS_SAVE_PROMPT",
                tableName: "APIAccess",
                value: "Delete \(subject.value.name)?",

If the method was defined without a name, we just show "Delete ?" here.
We should probably add the default name we show in the methods list in that case, and set that as the method name.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsValidationErrorContentView.swift line 64 at r4 (raw file):

    }

    private func configureSubviews(previousConfiguration: MethodSettingsValidationErrorContentConfiguration? = nil) {

previousConfiguration doesn't seem to be used here, is it really needed ?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 310 at r3 (raw file):

                "METHOD_SETTINGS_SAVING_CHANGES",
                tableName: "APIAccess",
                value: "Saving changes...",

This should say "Adding method..." if we are adding the method , and "Saving changes..." if we are saving changes

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 555 at r4 (raw file):

    }

    // swiftlint:disable:next file_length

nit
I wonder if we could split this file to avoid doing this 🤔
Or maybe just regroup the Misc / Cell handling stuff in a different file ? I don't know, not a big deal probably.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodKind.swift line 47 at r3 (raw file):

            NSLocalizedString("DIRECT", tableName: "APIAccess", value: "Direct", comment: "")
        case .bridges:
            NSLocalizedString("BRIDGES", tableName: "APIAccess", value: "Bridges", comment: "")

The specs mention the copy "Mullvad bridges"

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodKind.swift line 51 at r3 (raw file):

            NSLocalizedString("SHADOWSOCKS", tableName: "APIAccess", value: "Shadowsocks", comment: "")
        case .socks5:
            NSLocalizedString("SOCKS_V5", tableName: "APIAccess", value: "Socks5", comment: "")

According to the specs, it should be "SOCKS5" (all uppercase)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodValidationError.swift line 71 at r4 (raw file):

            "\(field) cannot be empty."
        case .invalidIPAddress:
            "Please enter a valid IPv4 or IPv6 address."

Shouldn't those be localised strings ?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift line 103 at r4 (raw file):

            }

            if !(username.isEmpty && password.isEmpty) {

The username cannot be more than 255 characters as per the RFC, same goes for the password.

We should probably have unit tests for this.

[mojganii]

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadSettings/AccessMethodRepository.swift line 53 at r3 (raw file):

Previously, buggmagnet wrote…

Shouldn't we use our logging API for this instead ?
This comment is valid of all the print uses

+1

---

ios/MullvadSettings/AccessMethodRepository.swift line 98 at r4 (raw file):

            try writeApiAccessMethods(storedMethods)
        } catch {
            print("Could not update access methods: \(storedMethods) \nError: \(error)")

we can log it with debug level

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 19 at r4 (raw file):

Previously, buggmagnet wrote…

nit
I'm guessing this was meant to be a singleton, but we didn't define the init as private which defeats the purpose of having a singleton in the first place.

Just a nit because I will fix it in my upcoming work

as I remember we avoid using singleton which is anti design pattern. do we really need to share this object across classes instead of injecting that as a dependency?

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 23 at r4 (raw file):

    func start(configuration: PersistentProxyConfiguration, completion: @escaping (Error?) -> Void) {
        let workItem = DispatchWorkItem {
            let randomResult = (0 ... 255).randomElement()?.isMultiple(of: 2) ?? true

nit : IMHO I would rather put Todo annotation instead of making a random result

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodInteractor.swift line 15 at r4 (raw file):

Previously, buggmagnet wrote…

Let's not use abbreviations if we can afford writing the full word.

+1

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 117 at r4 (raw file):

    override func tableView(_ tableView: UITableView, heightForFooterInSection section: Int) -> CGFloat {
        guard let sectionIdentifier = dataSource?.snapshot().sectionIdentifiers[section] else { return 0 }
        let marginToDeleteMethodItem: CGFloat = 24

Use UIMetrics, please.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 24 at r4 (raw file):

    private var contentValidationErrors: [AccessMethodFieldValidationError] = []

    private var dataSource: UITableViewDiffableDataSource<

nit: maybe it was better to create typealias for this.

[rablador]

I waited with this one on purpose, since the mechanics for the method currently in use wasn't available to me until now. I'll add it.

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadSettings/AccessMethodRepository.swift line 53 at r3 (raw file):

Previously, mojganii wrote…

+1

Done.

---

ios/MullvadSettings/AccessMethodRepository.swift line 98 at r4 (raw file):

Previously, mojganii wrote…

we can log it with debug level

I put .error for all actual errors. Sounds ok?

---

ios/MullvadVPN/AccessMethodRepository/ProxyConfigurationTester.swift line 23 at r4 (raw file):

Previously, mojganii wrote…

nit : IMHO I would rather put Todo annotation instead of making a random result

This whole thing will go away when we connect the real testing.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/ShadowsocksSectionHandler.swift line 57 at r3 (raw file):

Previously, buggmagnet wrote…

Apologies if that was not discussed properly, but it turns out the password is .optional for the shadowsocks case in the end.
We can revert this change back to .optional

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodInteractor.swift line 15 at r4 (raw file):

Previously, mojganii wrote…

+1

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 117 at r4 (raw file):

Previously, mojganii wrote…

Use UIMetrics, please.

This is a very specific metric that's not going to be used elsewhere. Do we really need to add more to UIMetrics because of it?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 334 at r4 (raw file):

Previously, buggmagnet wrote…

If the method was defined without a name, we just show "Delete ?" here.
We should probably add the default name we show in the methods list in that case, and set that as the method name.

Name has been changed to required field, so this is not relevant anymore.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsValidationErrorContentView.swift line 64 at r4 (raw file):

Previously, buggmagnet wrote…

previousConfiguration doesn't seem to be used here, is it really needed ?

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 310 at r3 (raw file):

Previously, buggmagnet wrote…

This should say "Adding method..." if we are adding the method , and "Saving changes..." if we are saving changes

Since we're using the same vc for both scenarios there's currently no way to differentiate between new and existing. We could add methods to the interactor for this or do something else to get the right "context", but perhaps changing to "Saving method..." would be simpler?

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 24 at r4 (raw file):

Previously, mojganii wrote…

nit: maybe it was better to create typealias for this.

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 555 at r4 (raw file):

Previously, buggmagnet wrote…

nit
I wonder if we could split this file to avoid doing this 🤔
Or maybe just regroup the Misc / Cell handling stuff in a different file ? I don't know, not a big deal probably.

Yeah, I was thinking about this, but it felt like it could make things a little disjointed. Gave it a try now anyway.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodKind.swift line 47 at r3 (raw file):

Previously, buggmagnet wrote…

The specs mention the copy "Mullvad bridges"

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodKind.swift line 51 at r3 (raw file):

Previously, buggmagnet wrote…

According to the specs, it should be "SOCKS5" (all uppercase)

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodValidationError.swift line 71 at r4 (raw file):

Previously, buggmagnet wrote…

Shouldn't those be localised strings ?

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift line 103 at r4 (raw file):

Previously, buggmagnet wrote…

The username cannot be more than 255 characters as per the RFC, same goes for the password.

We should probably have unit tests for this.

Done. What about shadowsocks?

[mojganii]

Reviewable status: 53 of 74 files reviewed, 12 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewed 20 of 21 files at r5.
Reviewable status: 73 of 74 files reviewed, 4 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 117 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

This is a very specific metric that's not going to be used elsewhere. Do we really need to add more to UIMetrics because of it?

Given the discussion from yesterday, I think it's ok to keep it locally

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 310 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Since we're using the same vc for both scenarios there's currently no way to differentiate between new and existing. We could add methods to the interactor for this or do something else to get the right "context", but perhaps changing to "Saving method..." would be simpler?

Let's just have "saving changes..." everywhere then, that's what we seem to have right now, and it's functionally correct.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 228 at r5 (raw file):

        validateInput()
        dataSoruceConfiguration?.updateDataSource(

nit
Soruce isntead of Source

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift line 103 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Done. What about shadowsocks?

Let's discuss with the team

[mojganii]

Reviewable status: 73 of 74 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

[rablador]

Reviewable status: 62 of 75 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/EditAccessMethodViewController.swift line 117 at r4 (raw file):

Previously, buggmagnet wrote…

Given the discussion from yesterday, I think it's ok to keep it locally

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 310 at r3 (raw file):

Previously, buggmagnet wrote…

Let's just have "saving changes..." everywhere then, that's what we seem to have right now, and it's functionally correct.

Done.

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsViewController.swift line 228 at r5 (raw file):

Previously, buggmagnet wrote…

nit
Soruce isntead of Source

I think we should go with dataSaurus instead. :D

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift line 103 at r4 (raw file):

Previously, buggmagnet wrote…

Let's discuss with the team

Done.

[buggmagnet]

Reviewed 13 of 13 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodKind.swift line 45 at r6 (raw file):

        switch self {
        case .direct, .bridges:
            ""

I'm not sure I follow, why do we return an empty string here ?

[rablador]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodKind.swift line 45 at r6 (raw file):

Previously, buggmagnet wrote…

I'm not sure I follow, why do we return an empty string here ?

localizedDescription is used in func toListItem() -> ListAccessMethodItem in ListAccessMethodInteractor, but for the default (ie direct and bridges) items we don't want any secondary text except when they're disabled. It's a necessary evil unless we want to change some (minor) foundations Andrej laid.

[buggmagnet]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)

[buggmagnet]

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)

[buggmagnet]

Reviewed 9 of 9 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)