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

Fix nicknames uniqueness with different cases #8792

Merged
merged 42 commits into from May 4, 2022

Conversation

eliegaboriau
Copy link
Contributor

@eliegaboriau eliegaboriau commented Feb 8, 2022

🎩 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
Contributor

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
Member

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
Contributor

Alright, we will implement it asap, thanks !

@andreslucena
Copy link
Member

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
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
Copy link
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
@andreslucena andreslucena added this to Pending review from Product in Maintainers via automation Feb 14, 2022
@carolromero
Copy link
Member

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."

@andreslucena
Copy link
Member

Hello @andreslucena,

I keep an eye on this PR and remain available if needed,
I am a bit lost if we forgot something or not, if so please tell me and I will update it asap
Thanks ! :)

Yes @Quentinchampenois, sorry about that, it was in my side and I actually forgot about this one, I've just finished a last review, tiny changes and for me, it'd be ready to be merged. We'll need the @ahukkanen review too.

I've just tried it locally one last time and it works as expected:

Before

> Decidim::User.find(1,2,3).pluck(:id, :email, :created_at, :nickname)
=> [[1, "admin@example.org", Tue, 19 Apr 2022 13:59:34.332461000 UTC +00:00, "ANickname"], [2, "user@example.org", Fri, 22 Apr 2022 09:34:36.579928000 UTC +00:00, "anickname"], [3, "user2@example.org", Tue, 19 Apr 2022 13:59:34.783918000 UTC +00:00, "ANICKNAME"]]

After

> Decidim::User.find(1,2,3).pluck(:id, :email, :created_at, :nickname)
=> [[1, "admin@example.org", Tue, 19 Apr 2022 13:59:34.332461000 UTC +00:00, "ANickname"], [2, "user@example.org", Fri, 22 Apr 2022 09:34:36.579928000 UTC +00:00, "anickname-16743"], [3, "user2@example.org", Tue, 19 Apr 2022 13:59:34.783918000 UTC +00:00, "ANICKNAME-57490"]]

private fonctions
Copy link
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
Contributor Author

Few refactoring ideas for the fixing task.

Everything's added !

Copy link
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.

Copy link
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
andreslucena added a commit that referenced this pull request May 6, 2022
* 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 added a commit that referenced this pull request May 6, 2022
* 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>
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
@carolromero carolromero mentioned this pull request Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core type: fix PRs that implement a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User nicknames don't check cases when checking for uniqueness
7 participants