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.

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

Prevent duplicate list names in custom lists #6105

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Apr 10, 2024

It shouldn't be possible to save a custom list with an already existing list name.


This change is Reviewable

@rablador rablador requested review from mojganii and acb-mv April 10, 2024 12:59
@rablador rablador marked this pull request as draft April 10, 2024 13:33
@rablador rablador removed request for mojganii and acb-mv April 10, 2024 13:33
@rablador rablador force-pushed the custom-list-crashes-when-naming-a-list-with-the-name-of-an-ios-599 branch from c283112 to ecf69cf Compare April 10, 2024 13:50
@rablador rablador requested review from mojganii and acb-mv April 10, 2024 13:51
@rablador rablador marked this pull request as ready for review April 10, 2024 13:51
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

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


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

        let persistedListWithSameName = lists.first {
            $0.name.localizedCaseInsensitiveCompare(list.name) == .orderedSame

do we really to compare the localized value of a name? I'm of the opinion it's just a name regardless of its language.


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

        }

        if let persistedListWithSameName, persistedListWithSameName.id != list.id {

nit : duplicatedName

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

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


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

Previously, mojganii wrote…

nit : duplicatedName

the name of class defines the object takes the responsibility of storing, having persisted is redundant.

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @mojganii)


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

Previously, mojganii wrote…

do we really to compare the localized value of a name? I'm of the opinion it's just a name regardless of its language.

Maybe you're right, I'll make a change.


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

Previously, mojganii wrote…

the name of class defines the object takes the responsibility of storing, having persisted is redundant.

It's the same variable, just that it's unwrapped here, just like guard let self else ....

I think it's good to distinguish where the two lists comes from. One is passed in to the function as param, the other is loaded from store. listWithSameName feels a little too ambiguous for me.

@rablador rablador force-pushed the custom-list-crashes-when-naming-a-list-with-the-name-of-an-ios-599 branch from ecf69cf to 0f2b655 Compare April 12, 2024 14:47
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

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


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

Previously, rablador (Jon Petersson) wrote…

It's the same variable, just that it's unwrapped here, just like guard let self else ....

I think it's good to distinguish where the two lists comes from. One is passed in to the function as param, the other is loaded from store. listWithSameName feels a little too ambiguous for me.

Did some refactoring to make it a little less obtuse.

@rablador rablador added bug iOS Issues related to iOS labels Apr 12, 2024
Copy link
Contributor

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the custom-list-crashes-when-naming-a-list-with-the-name-of-an-ios-599 branch from 0f2b655 to c3d575e Compare April 15, 2024 09:29
@buggmagnet buggmagnet merged commit 6973de9 into main Apr 15, 2024
7 checks passed
@buggmagnet buggmagnet deleted the custom-list-crashes-when-naming-a-list-with-the-name-of-an-ios-599 branch April 15, 2024 09:30
Copy link

🚨 End to end tests failed. Please check the failed workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants