storing custom list into settings

[mojganii]

Storing custom lists in settings enhances the app's ability to access user preferences for relay connections.

---

This change is 

[linear]

IOS-464 Store custom lists in settings

[rablador]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)

---

ios/MullvadSettings/CustomList.swift line 15 at r1 (raw file):

    public let id: UUID
    public var name: String
    public var list: [RelayLocation] = []

How do we know the heirarchy here? A custom list can have nested items, eg:

Code snippet:

[
   Country,
   Country: [
      City: [
         Relay: {}
      ],
      City: [
         Relay: {},
         Relay: {}
      ]
   ]
]

---

ios/MullvadSettings/CustomListRepository.swift line 51 at r1 (raw file):

            }
        } catch {
            throw error

Nit: If all we do is rethrow the error I think we can remove the do-catch. Alternatively we can add a logger message as well and keep it.

---

ios/MullvadSettings/CustomListRepositoryProtocol.swift line 20 at r1 (raw file):

    func update(_ list: CustomList)

    /// Delete custom list  by id.

Nit: Extra spacing here and in a few texts below.

---

ios/MullvadVPNTests/CustomListRepositoryTests.swift line 66 at r1 (raw file):

    }

    func testFetchingAllCustomList() throws {

Nit: Maybe we could add more than one list here to check that it works with multiple lists.

[mojganii]

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/CustomList.swift line 15 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

How do we know the heirarchy here? A custom list can have nested items, eg:

in the current code LocationDataSource is passing RelayLocation to tunnel manager to set matched relay for connecting and when user expands the the node it's looking for children.hence It means it's enough to have access to hierarchy

---

ios/MullvadSettings/CustomListRepository.swift line 51 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: If all we do is rethrow the error I think we can remove the do-catch. Alternatively we can add a logger message as well and keep it.

the error should be shown in the UI; therefore, it needs to be thrown to be caught by the presentation layer.

---

ios/MullvadSettings/CustomListRepositoryProtocol.swift line 20 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Extra spacing here and in a few texts below.

Done

---

ios/MullvadVPNTests/CustomListRepositoryTests.swift line 66 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Maybe we could add more than one list here to check that it works with multiple lists.

Done

[rablador]

Reviewed 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[acb-mv]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[rablador]

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

---

ios/MullvadSettings/CustomListRepository.swift line 26 at r3 (raw file):

public struct CustomListRepository: CustomListRepositoryProtocol {
    public var publisher: AnyPublisher<[CustomList], Never> {

I don't think we need a publisher at this point, since we don't know yet how the repository will interact with the rest of the code. If needed, we can add one later and make use of it then.

[buggmagnet]

Reviewed 2 of 6 files at r1, 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadSettings/CustomListRepository.swift line 20 at r3 (raw file):

        switch self {
        case .duplicateName:
            "Name is already taken."

If this is a user facing error, let's localize it

---

ios/MullvadSettings/CustomListRepository.swift line 26 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

I don't think we need a publisher at this point, since we don't know yet how the repository will interact with the rest of the code. If needed, we can add one later and make use of it then.

I agree with @rablador, can we make it work without Combine here ?
We had some challenges when building API Access methods because of Combine, so I'd like to avoid future pain if we can avoid it, unless you feel it will make upcoming development much easier to manage.

---

ios/MullvadVPNTests/CustomListRepositoryTests.swift line 40 at r3 (raw file):

    func testAddingCustomList() throws {
        let name = "Netflix"

nit
I think we can rename this customListName to make it even more explicit.

Same with item below, we could name it customList

I like some of the examples by the way, I think it's nice that they represent how a user would use the API like in testFetchingAllCustomList 👍

[mojganii]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadSettings/CustomListRepository.swift line 20 at r3 (raw file):

Previously, buggmagnet wrote…

If this is a user facing error, let's localize it

done

---

ios/MullvadSettings/CustomListRepository.swift line 26 at r3 (raw file):

Previously, buggmagnet wrote…

I agree with @rablador, can we make it work without Combine here ?
We had some challenges when building API Access methods because of Combine, so I'd like to avoid future pain if we can avoid it, unless you feel it will make upcoming development much easier to manage.

Let's keep it for UI I would remove that if I saw I don't need that anymore. at the moment it's more likely to need that

[rablador]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

[rablador]

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

---

ios/MullvadSettings/CustomListRepository.swift line 26 at r3 (raw file):

Previously, mojganii wrote…

Let's keep it for UI I would remove that if I saw I don't need that anymore. at the moment it's more likely to need that

I don't think we should keep things around in anticipation that we might need it. Better to add when we actually have evidence that we do.

[rablador]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

[buggmagnet]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

[rablador]

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