Fix nicknames uniqueness with different cases (decidim#8792)

* 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 <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Andrés Pereira de Lucena <[email protected]>

* 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 <[email protected]>

CHANGELOG.md

@@ -151,6 +151,7 @@ Changes to:
 - **decidim-debates**, **decidim-initiatives**, **decidim-meetings**: The resource search classes `Decidim::Debates::DebateSearch`, `Decidim::Intitatives::InitiativeSearch` and `Decidim::Meetings::MeetingSearch` are rewritten for the Ransack searches due to Searchlight removal at [\#8748](https://github.com/decidim/decidim/pull/8748). The role of these classes is now to pass contextual information to the searches, such as the current user. All other search filtering should happen directly through Ransack.
 - **decidim-meetings**: The `visible_meetings_for` scope for the `Meeting` model has been renamed to `visible_for` in [\#8748](https://github.com/decidim/decidim/pull/8748) for consistency.
 - **decidim-core**: The `official_origin`, `participants_origin`, `user_group_origin` and `meeting_origin` scopes for the `Decidim::Authorable` and `Decidim::Coauthorable` concerns have been changed to `with_official_origin`, `with_participants_origin`, `with_user_group_origin` and `with_meeting_origin` respectively in [\#8748](https://github.com/decidim/decidim/pull/8748) for consistency. See the Searchlight removal change notes for reasoning.
+- **decidim-core**: Nicknames are now differents case insensitively, a rake task has been created to check every nickname and modify them if some are similar (Launch it with "bundle exec rake decidim:upgrade:fix_nickname_uniqueness"). Routing and mentions has been made case insensitive for every tab in profiles.
 
 #### Deprecation of `Rectify::Presenter`
 PR [\#8758](https://github.com/decidim/decidim/pull/8758) is deprecating the implementation of `Rectify::Presenter` in favour of `SimpleDelegator`

decidim-core/app/controllers/decidim/profiles_controller.rb

@@ -83,7 +83,7 @@ def ensure_profile_holder
     def profile_holder
       return if params[:nickname].blank?
 
-      @profile_holder ||= Decidim::UserBaseEntity.find_by(nickname: params[:nickname], organization: current_organization)
+      @profile_holder ||= Decidim::UserBaseEntity.find_by("LOWER(nickname) = ? AND decidim_organization_id = ?", params[:nickname].downcase, current_organization.id)
     end
   end
 end

decidim-core/app/controllers/decidim/user_activities_controller.rb

@@ -19,7 +19,9 @@ def index
     private
 
     def user
-      @user ||= current_organization.users.find_by(nickname: params[:nickname])
+      return unless params[:nickname]
+
+      @user ||= current_organization.users.find_by("LOWER(nickname) = ?", params[:nickname].downcase)
     end
 
     def activities

decidim-core/app/controllers/decidim/user_timeline_controller.rb

@@ -18,10 +18,9 @@ def index
     private
 
     def user
-      @user ||= Decidim::User.find_by(
-        organization: current_organization,
-        nickname: params[:nickname]
-      )
+      return unless params[:nickname]
+
+      @user ||= current_organization.users.find_by("LOWER(nickname) = ?", params[:nickname].downcase)
     end
 
     def activities

decidim-core/app/events/decidim/change_nickname_event.rb

@@ -0,0 +1,21 @@
+# frozen-string_literal: true
+
+module Decidim
+  class ChangeNicknameEvent < Decidim::Events::SimpleEvent
+    include Decidim::Events::NotificationEvent
+    delegate :organization, to: :user, prefix: false
+    delegate :url_helpers, to: "Decidim::Core::Engine.routes"
+
+    i18n_attributes :link_to_account_settings
+
+    def notification_title
+      I18n.t("decidim.events.nickname_event.notification_body", i18n_options).to_s.html_safe
+    end
+
+    def i18n_options
+      {
+        link_to_account_settings: url_helpers.account_path
+      }
+    end
+  end
+end

decidim-core/app/forms/decidim/account_form.rb

@@ -61,8 +61,9 @@ def unique_email
 
     def unique_nickname
       return true if Decidim::User.where(
-        organization: context.current_organization,
-        nickname: nickname
+        "decidim_organization_id = ? AND LOWER(nickname) = ? ",
+        context.current_organization.id,
+        nickname.downcase
       ).where.not(id: context.current_user.id).empty?
 
       errors.add :nickname, :taken

decidim-core/app/forms/decidim/registration_form.rb

@@ -15,7 +15,7 @@ class RegistrationForm < Form
     attribute :current_locale, String
 
     validates :name, presence: true
-    validates :nickname, presence: true, format: /\A[\w\-]+\z/, length: { maximum: Decidim::User.nickname_max_length }
+    validates :nickname, presence: true, format: Decidim::User::REGEXP_NICKNAME, length: { maximum: Decidim::User.nickname_max_length }
     validates :email, presence: true, 'valid_email_2/email': { disposable: true }
     validates :password, confirmation: true
     validates :password, password: { name: :name, email: :email, username: :nickname }
@@ -39,7 +39,9 @@ def email_unique_in_organization
     end
 
     def nickname_unique_in_organization
-      errors.add :nickname, :taken if User.no_active_invitation.find_by(nickname: nickname, organization: current_organization).present?
+      return false unless nickname
+
+      errors.add :nickname, :taken if User.no_active_invitation.find_by("LOWER(nickname)= ? AND decidim_organization_id = ?", nickname.downcase, current_organization.id).present?
     end
 
     def no_pending_invitations_exist

decidim-core/config/locales/en.yml

@@ -687,6 +687,8 @@ en:
           email_outro: You have received this notification because you are an admin of the platform.
           email_subject: A user group has updated its profile
           notification_title: The %{user_group_name} user group has updated its profile, leaving it unverified. You can now verify it in the <a href="%{groups_admin_path}">admin panel</a>.
+      nickname_event:
+        notification_body: 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. <br/> Your nickname was created after another one with the same name, so we have automatically renamed it. You can change it from <a href="%{link_to_account_settings}">your account settings</a>.
       notification_event:
         notification_title: An event occured to <a href="%{resource_path}">%{resource_title}</a>.
       reports:
@@ -1501,7 +1503,7 @@ en:
     invitations:
       edit:
         header: Finish creating your account
-        nickname_help: Your nickname in %{organization}.
+        nickname_help: Your nickname in %{organization}. Can only contain letters, numbers, '-' and '_'.
         submit_button: Save
         subtitle: If you accept the invitation please set your nickname and password.
       invitation_removed: Your invitation was removed.

decidim-core/lib/decidim/content_parsers/user_parser.rb

@@ -26,7 +26,7 @@ class UserParser < BaseParser
       # @return [String] the content with the valid mentions replaced by a global id
       def rewrite
         content.gsub(MENTION_REGEX) do |match|
-          users[match[1..-1]]&.to_global_id&.to_s || match
+          users[match[1..-1].downcase]&.to_global_id&.to_s || match
         end
       end
 
@@ -43,11 +43,11 @@ def users
       end
 
       def existing_users
-        @existing_users ||= Decidim::User.where(organization: current_organization, nickname: content_nicknames)
+        @existing_users ||= Decidim::User.where("decidim_organization_id = ? AND LOWER(nickname) IN (?)", current_organization.id, content_nicknames)
       end
 
       def content_nicknames
-        @content_nicknames ||= content.scan(MENTION_REGEX).flatten.uniq
+        @content_nicknames ||= content.scan(MENTION_REGEX).flatten.uniq.map!(&:downcase)
       end
 
       def current_organization

decidim-core/lib/decidim/nicknamizable.rb

@@ -52,7 +52,7 @@ def disambiguate(name, scope)
         candidate = name
 
         2.step do |n|
-          return candidate unless exists?(scope.merge(nickname: candidate))
+          return candidate if Decidim::User.where("nickname ILIKE ?", candidate.downcase).where(scope).empty?
 
           candidate = numbered_variation_of(name, n)
         end

decidim-core/lib/tasks/upgrade/decidim_fix_nickname_uniqueness.rake

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+namespace :decidim do
+  namespace :upgrade do
+    desc "Modify nicknames with random numbers when exists similar ones case insensitively"
+    task fix_nickname_uniqueness: :environment do
+      logger = Logger.new($stdout)
+      logger.info("Updating conflicting user nicknames...")
+
+      # list of users already changed in the process
+      has_changed = []
+
+      Decidim::User.not_deleted.find_each do |user|
+        next if has_changed.include? user.id
+
+        Decidim::User.where(organization: user.organization)
+                     .where("nickname ILIKE ?", user.nickname.downcase)
+                     .where.not(id: has_changed + [user.id])
+                     .not_deleted
+                     .order(:created_at)
+                     .each do |similar_user|
+          # change her nickname to the lowercased one with numbers if needed
+          begin
+            update_user_nickname(similar_user, Decidim::UserBaseEntity.nicknamize(similar_user.nickname, organization: similar_user.organization))
+          rescue ActiveRecord::RecordInvalid => e
+            logger.warn("User ID (#{similar_user.id}) : #{e}")
+          end
+          has_changed.append(similar_user.id)
+        end
+      end
+      logger.info("Process terminated, #{has_changed.count} users nickname have been updated.")
+    end
+
+    private
+
+    def send_notification_to(user)
+      Decidim::EventsManager.publish({
+                                       event: "decidim.events.nickname_event",
+                                       event_class: Decidim::ChangeNicknameEvent,
+                                       affected_users: [user],
+                                       resource: user
+                                     })
+    end
+
+    def update_user_nickname(user, new_nickname)
+      user.update!(nickname: new_nickname)
+      send_notification_to(user)
+      user
+    end
+  end
+end

decidim-core/spec/content_parsers/decidim/user_parser_spec.rb

@@ -62,5 +62,18 @@ module Decidim
         expect(parser.metadata.users).to eq([])
       end
     end
+
+    context "when mentioning a user with a wrong case" do
+      let(:content) { "This text mentions a user with wrong case : @#{user.nickname.upcase}" }
+
+      it "rewrite the good user" do
+        expect(parser.rewrite).to eq("This text mentions a user with wrong case : #{user.to_global_id}")
+      end
+
+      it "returns the correct metadata" do
+        expect(parser.metadata).to be_a(Decidim::ContentParsers::UserParser::Metadata)
+        expect(parser.metadata.users).to eq([user])
+      end
+    end
   end
 end

decidim-core/spec/controllers/decidim/profiles_controller_spec.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+module Decidim
+  describe ProfilesController, type: :controller do
+    routes { Decidim::Core::Engine.routes }
+
+    let(:organization) { create(:organization) }
+    let!(:user) { create(:user, nickname: "Nick", organization: organization) }
+
+    before do
+      request.env["decidim.current_organization"] = organization
+    end
+
+    describe "#badges" do
+      context "with an user with uppercase" do
+        it "returns the lowercased user" do
+          get :badges, params: { nickname: "NICK" }
+          expect(response).to render_template(:show)
+        end
+      end
+    end
+  end
+end

decidim-core/spec/controllers/decidim/user_activities_controller_spec.rb

@@ -7,6 +7,7 @@ module Decidim
     routes { Decidim::Core::Engine.routes }
 
     let(:organization) { create(:organization) }
+    let!(:user) { create(:user, nickname: "Nick", organization: organization) }
 
     before do
       request.env["decidim.current_organization"] = organization
@@ -20,6 +21,13 @@ module Decidim
           end.to raise_error(ActionController::RoutingError, "Missing user: foobar")
         end
       end
+
+      context "with an user with uppercase" do
+        it "returns the lowercased user" do
+          get :index, params: { nickname: "NICK" }
+          expect(response).to render_template(:index)
+        end
+      end
     end
   end
 end

decidim-core/spec/controllers/decidim/user_timeline_controller_spec.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+module Decidim
+  describe UserTimelineController, type: :controller do
+    routes { Decidim::Core::Engine.routes }
+
+    let(:organization) { create(:organization) }
+    let!(:user) { create(:user, :confirmed, nickname: "Nick", organization: organization) }
+
+    before do
+      request.env["decidim.current_organization"] = organization
+      sign_in user
+    end
+
+    describe "#index" do
+      context "with a different user than me" do
+        it "raises an ActionController::RoutingError" do
+          expect do
+            get :index, params: { nickname: "foobar" }
+          end.to raise_error(ActionController::RoutingError, "Not Found")
+        end
+      end
+
+      context "with my user with uppercase" do
+        it "returns the lowercased user" do
+          get :index, params: { nickname: "NICK" }
+          expect(response).to render_template(:index)
+        end
+      end
+    end
+  end
+end

decidim-core/spec/events/decidim/change_nickname_event_spec.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe Decidim::ChangeNicknameEvent do
+  include_context "when a simple event"
+
+  let(:event_name) { "decidim.events.change_nickname_event" }
+  let(:resource) { create :user }
+  let(:author) { resource }
+
+  describe "notification_title" do
+    it "is generated correctly" do
+      expect(subject.notification_title).to include("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. <br/> Your nickname was created after another one with the same name, so we have automatically renamed it. You can change it from <a href=\"/account\">your account settings</a>.")
+    end
+  end
+end

decidim-core/spec/forms/registration_form_spec.rb

@@ -78,7 +78,7 @@ module Decidim
     end
 
     context "when the nickname already exists" do
-      let!(:user) { create(:user, organization: organization, nickname: nickname) }
+      let!(:user) { create(:user, organization: organization, nickname: nickname.upcase) }
 
       it { is_expected.to be_invalid }
 

decidim-core/spec/system/authentication_spec.rb

@@ -226,6 +226,27 @@
       end
     end
 
+    context "when nickname is not unique case insensitively" do
+      let!(:user) { create(:user, nickname: "Nick", organization: organization) }
+
+      it "show an error message" do
+        find(".sign-up-link").click
+
+        within ".new_user" do
+          fill_in :registration_user_email, with: "[email protected]"
+          fill_in :registration_user_name, with: "Responsible Citizen"
+          fill_in :registration_user_nickname, with: "NiCk"
+          fill_in :registration_user_password, with: "DfyvHn425mYAy2HL"
+          fill_in :registration_user_password_confirmation, with: "DfyvHn425mYAy2HL"
+          check :registration_user_tos_agreement
+          check :registration_user_newsletter
+          find("*[type=submit]").click
+        end
+
+        expect(page).to have_content("has already been taken")
+      end
+    end
+
     context "when sign up is disabled" do
       let(:organization) { create(:organization, users_registration_mode: :existing) }