Fix nicknames uniqueness with different cases (#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 <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>

CHANGELOG.md

@@ -245,6 +245,45 @@ rails decidim:active_storage_migrations:migrate_inline_images_to_active_storage[
 - **decidim-core**: Disable unconfirmed access by default [\#8233](https://github.com/decidim/decidim/pull/8233)
 - **decidim-meetings**: Rename 'upcoming events' content block to 'upcoming meetings' [\#8412](https://github.com/decidim/decidim/pull/8412)
 - **decidim-core**: Change user workflows to prevent user enumeration attacks [\#8537](https://github.com/decidim/decidim/pull/8537)
+### Rename data portability to download your data
+
+"Data portability" has been renamed to "Download you data" at [\#9196](https://github.com/decidim/decidim/pull/9196), you should update your cron job via crontab -e.
+```
+0 0 * * * cd /home/user/decidim_application && RAILS_ENV=production bundle exec rake decidim:delete_data_portability_files
+```
+Changes to:
+```
+0 0 * * * cd /home/user/decidim_application && RAILS_ENV=production bundle exec rake decidim:delete_download_your_data_files
+```
+
+
+- **decidim-core**: The `Decidim::ActivitySearch` class has been rewritten as `Decidim::PublicActivities` which is now a `Rectify::Query` class instead of `Searchlight::Search` class due to the removal of Searchlight at [\#8748](https://github.com/decidim/decidim/pull/8748).
+- **decidim-core**: The `Decidim::ResourceSearch` class now inherits from `Ransack::Search` instead of `Searchlight::Search` as of [\#8748](https://github.com/decidim/decidim/pull/8748). The new `ResourceSearch` class provides extra search functionality for contextual searches that require context information in addition to the search parameters, such as current user or current component. It has barely anything to do with the `ResourceSearch` class in the previous versions which contained much more logic. Please review all your search classes that were inheriting from this class. You should migrate your search filtering to Ransack.
+- **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`
+
+#### Searchlight removal causes changes in the participant searches
+
+The `searchlight` gem has been removed in favor of Ransack as of [\#8748](https://github.com/decidim/decidim/pull/8748) in order to standardize all searches within Decidim around a single way of performing searches. Ransack was selected as the preferred search backend because it is better maintained and has a larger community of developers around it compared to Searchlight.
+
+Ransack provides a search API that produces the search queries semi-automatically against the available database columns and ActiveRecord scopes made available for the Ransack searches while Searchlight used to require to write all the search logic manually in the search classes. Due to the inner workings of the Ransack gem and for consistency reasons, the following changes have been made for the search filtering:
+
+- For search scopes that are doing more than matching against a specific column in the database or require special programming logic during the searches, there is a new scope convention introduced with the `with_*` and `with_any_*` scope names. The `with_*` convention should be used when providing a search scope that searches against one key, such as `with_category(1)` and the `with_any_*` convention should be used when providing a search scope that searches against one or multiple keys, such as `with_any_category(1, 2, 3)`.
+  * An example of such scope is `with_any_category` provided by the `HasCategory` concern which searches against the provided category IDs or any sub-category of those category IDs. You can find all the introduced (or changed) scopes by searching for `scope :with_` within the Decidim codebase.
+  * With Searchlight, these search parameters were provided e.g. as `category_id` which was then used to perform the explained search query manually in the ResourceSearch class which is now used for a different purpose. As the search now happens through Ransack and the ActiveRecord scopes, these parameters have been renamed to better explain what they do. With Ransack, matching e.g. against the `category_id_eq` key would mean that the search is done against this specific column in the record's database table and only searching for the provided search input (and not e.g. the parent categories in the category case).
+- The origin scopes provided by `Decidim::Authorable` and `Decidim::Coauthorable` have been renamed with the `with_` prefix as explained above.
+- All the filtering key changes have been reflected to the participant filtering views (`_filters.html.erb` in most modules) as well as the controller methods `default_filter_params` where applicable.
+- The `default_filter_params` method within the participant-facing controllers now defines all the parameters that are allowed in the search queries and only these parameters are passed to the Ransack search. This limitation is made in order to protect the participant views from providing more searching options through the URL parameters than they are supposed to provide. In the past, the `Searchlight::Search` classes took care of utilizing only the allowed parameters but Ransacker does not have any middle-layer that would do the same, which is why the limitation is done at the controller side.
+- The `search_collection` method now defines the base collection used for the searches within the filtering controllers. In previous versions, there used to be a method that defined a `search_klass` method that defined the `Searchlight::Search` class to be used as the basis for the search. Now, the `search_collection` defines the base collection instead against which the Ransack search is run.
+
+3rd party developers that have developed their own modules or customizations for the core controllers or filtering views, should revisit their customizations and make sure they reflect these changes made for the controllers or filtering views. It is suggested to remove the customizations related to the filtering views/controllers and re-do from scratch what needs to be customized in order to ensure full compatibility with the changed filtering APIs. In case you had created your own `Searchlight::Search` (or `ResourceSearch`) classes, you should scrap those and start over using Ransack.
+
+More information on using Ransack can be found from the [Ransack documentation](https://github.com/activerecord-hackery/ransack). You can find examples for filtering in the core filtering views and controllers.
 
 ### Fixed
 

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

@@ -685,6 +685,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:
@@ -1465,7 +1467,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 }