Skip to content

Fix nicknames uniqueness with different cases#8792

Merged
ahukkanen merged 42 commits intodecidim:developfrom
eliegaboriau:fix/nickname_uniqueness-6092
May 4, 2022
Merged

Fix nicknames uniqueness with different cases#8792
ahukkanen merged 42 commits intodecidim:developfrom
eliegaboriau:fix/nickname_uniqueness-6092

Conversation

@eliegaboriau
Copy link
Copy Markdown
Contributor

@eliegaboriau eliegaboriau commented Feb 8, 2022 •
edited
Loading

🎩 What? Why?

Please describe your pull request.

This PR ensure nickname is unique, case insensitively

Also, routing for user public profile and mentions are now redirecting to the unique user even if the case is not the same.
Rake task rename similar nicknames (with a series of digits at the end), sending a notification to the user when updating her nickname

image

📌 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

  • add users with similar nickname, as "Toto", "TOTO" and "toto"

  • run the rake tasks

  • see them turning into "Toto", "TOTO-XXXXX" and "toto-XXXXX"

  • Try adding a user with a nickname similar to one

  • fail

  • Try going to an user profile page (or mentioning her) by writing her nickname with a different case

  • See you're going to the good profile

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.*

Additional informations

Please note that the current migration will rename all duplicated nicknames and rename all nicknames in database to lowercase

♥️ Thank you!

@Quentinchampenois
Copy link
Copy Markdown
Contributor

Quentinchampenois commented Feb 8, 2022

Hello,

As discussed in the related issue, the notification is not implemented when a user nickname is renamed in migration. Let us know if it is better to implement it, even if the nickname is still editable in account.

@eliegaboriau eliegaboriau marked this pull request as ready for review February 8, 2022 12:57
@armandfardeau armandfardeau changed the title Fix/nickname uniqueness 6092 Fix/nickname uniqueness Feb 8, 2022
@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Feb 9, 2022

Let us know if it is better to implement it, even if the nickname is still editable in account.

Just discussed it in the @decidim/product meeting, and we agreed that we'd like the notification also. Can you leave a placeholder for now? We'll work on this text between today and tomorrow. Thanks!

@Quentinchampenois
Copy link
Copy Markdown
Contributor

Quentinchampenois commented Feb 9, 2022

Alright, we will implement it asap, thanks !

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Feb 9, 2022

Ok! After this is done, let me know. It'd be great that @Quentinchampenois or @armandfardeau can give it a first look; leave it with an approved review when you fell confident about the code.

Also welcome @eliegaboriau!!

@eliegaboriau eliegaboriau marked this pull request as draft February 9, 2022 15:38
@eliegaboriau
Add notification when updating nickname (#3)
9a66796 
add spec for event
add translation for notification
normalize locales
add logger
refactor
add random numbers to prevent errors
@eliegaboriau eliegaboriau marked this pull request as ready for review February 14, 2022 08:55
Quentinchampenois
Quentinchampenois previously approved these changes Feb 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Quentinchampenois Quentinchampenois left a comment

Choose a reason for hiding this comment

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

Hello @eliegaboriau,

Thank you for your contribution and welcome !
It seems good to me.

@andreslucena, just a point, we decided not increment the nickname with -1, -2, etc but adding a random between 0 and 99999 to reduce the possibility of nickname already taken error.
For now, the translation for notification title is a placeholder, feel free to propose another one.

If anyone else wants to review, don't hesitate.
Thanks !

@andreslucena andreslucena linked an issue Feb 14, 2022 that may be closed by this pull request
User nicknames don't check cases when checking for uniqueness #6092
Closed
@carolromero
Copy link
Copy Markdown
Member

carolromero commented Feb 16, 2022

Hi there, sorry for the delay. Here's the proposed notification message:

"We have corrected the way nicknames are used so that there are no duplicates, and that's why we have removed the case sensitive rule.

Your nickname was created after another one with the same name, so we have automatically renamed it.

You can change it from your account settings."

@Quentinchampenois
Copy link
Copy Markdown
Contributor

Quentinchampenois commented Feb 16, 2022

Hello @carolromero,

Thank you, the translation will be added in this pull request

I think we should convert it to draft and reopen it once conflicts are resolved and translation updated.

@eliegaboriau
remove the if statement
caf4f91 
private fonctions
ahukkanen
ahukkanen requested changes Apr 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Few refactoring ideas for the fixing task.

@eliegaboriau
Copy link
Copy Markdown
Contributor Author

eliegaboriau commented May 2, 2022

Few refactoring ideas for the fixing task.

Everything's added !

@eliegaboriau eliegaboriau requested a review from ahukkanen May 2, 2022 10:05
ahukkanen
ahukkanen requested changes May 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Can you also exclude the deleted users from both queries as deleted users are not consuming the nicknames (it's empty for all deleted users) and we should not update deleted users here.

Please also see my other comment above regarding the nicknamize method and taking the capitalization into account there too.

@eliegaboriau eliegaboriau requested a review from ahukkanen May 2, 2022 12:21
andreslucena
andreslucena reviewed May 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽

@ahukkanen ahukkanen merged commit 2f6d596 into decidim:develop May 4, 2022
@ahukkanen ahukkanen mentioned this pull request May 4, 2022
Merged
12 tasks
andreslucena added a commit that referenced this pull request May 6, 2022
@eliegaboriau @andreslucena
Fix nicknames uniqueness with different cases (#8792)
2ef2d76 
* allowing only lowercase in the nickname

* nickname test system

* add in locales the "lowercase" precision

* changing routing for timeline

* changing routing for activities

* changing routing for other profile tabs

* migration to change commune nicknames

* linter

* minor changes
increase performance for migration

* Refactor migration for lisibility

* add fix into changelog

* linter

* Update CHANGELOG.md

* add notification when updating nickname

* fix number of notification

* Add notification when updating nickname (#3)

add spec for event
add translation for notification
normalize locales
add logger
refactor
add random numbers to prevent errors

* update message in notification

* update test
linter

* minor changes

* Change requests nickname (#8)

* back to "any case" nickname

* check uniqueness case insensitivity

* change profiles url

* mention, parse case insensitively

* mention, render nicknames case insensitively

* mentions spec

* update migration

* linter

* migration to rake tasks
spec rake task

* linter

* add test notification
fix test nickname

* linter

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Update CHANGELOG.md

* Update account_form.rb

* linter

* remove the if statement
private fonctions

* remove test task

* relaunch tests

* add organization criteria
refactors

* lint

* use nicknamize instead of random numbers

* add the not deleted scope

* update nicknamizable to check for case

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@andreslucena andreslucena mentioned this pull request May 6, 2022
Closed
andreslucena added a commit that referenced this pull request May 6, 2022
@eliegaboriau @andreslucena
Fix nicknames uniqueness with different cases (#8792)
319cbd0 
* allowing only lowercase in the nickname

* nickname test system

* add in locales the "lowercase" precision

* changing routing for timeline

* changing routing for activities

* changing routing for other profile tabs

* migration to change commune nicknames

* linter

* minor changes
increase performance for migration

* Refactor migration for lisibility

* add fix into changelog

* linter

* Update CHANGELOG.md

* add notification when updating nickname

* fix number of notification

* Add notification when updating nickname (#3)

add spec for event
add translation for notification
normalize locales
add logger
refactor
add random numbers to prevent errors

* update message in notification

* update test
linter

* minor changes

* Change requests nickname (#8)

* back to "any case" nickname

* check uniqueness case insensitivity

* change profiles url

* mention, parse case insensitively

* mention, render nicknames case insensitively

* mentions spec

* update migration

* linter

* migration to rake tasks
spec rake task

* linter

* add test notification
fix test nickname

* linter

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Update CHANGELOG.md

* Update account_form.rb

* linter

* remove the if statement
private fonctions

* remove test task

* relaunch tests

* add organization criteria
refactors

* lint

* use nicknamize instead of random numbers

* add the not deleted scope

* update nicknamizable to check for case

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@microstudi microstudi mentioned this pull request Jul 14, 2022
Merged
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
@carolromero carolromero mentioned this pull request Jul 24, 2023
Closed
@andreslucena andreslucena mentioned this pull request Mar 12, 2025
Merged
@alecslupu alecslupu mentioned this pull request Nov 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreslucena andreslucena andreslucena approved these changes

@ahukkanen ahukkanen ahukkanen approved these changes

@armandfardeau armandfardeau Awaiting requested review from armandfardeau

@Quentinchampenois Quentinchampenois Awaiting requested review from Quentinchampenois

Assignees

No one assigned

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

User nicknames don't check cases when checking for uniqueness

7 participants

@eliegaboriau @Quentinchampenois @andreslucena @carolromero @ahukkanen @armandfardeau @alecslupu