From 2f6d59619e3bc41453ff818c1312703e2ee2921c Mon Sep 17 00:00:00 2001 From: eliegaboriau <93646702+eliegaboriau@users.noreply.github.com> Date: Wed, 4 May 2022 14:07:04 +0200 Subject: [PATCH] Fix nicknames uniqueness with different cases (#8792) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * Update CHANGELOG.md Co-authored-by: Andrés Pereira de Lucena * 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 --- CHANGELOG.md | 1 + .../decidim/profiles_controller.rb | 2 +- .../decidim/user_activities_controller.rb | 4 +- .../decidim/user_timeline_controller.rb | 7 +- .../events/decidim/change_nickname_event.rb | 21 ++++++ .../app/forms/decidim/account_form.rb | 5 +- .../app/forms/decidim/registration_form.rb | 6 +- decidim-core/config/locales/en.yml | 4 +- .../decidim/content_parsers/user_parser.rb | 6 +- decidim-core/lib/decidim/nicknamizable.rb | 2 +- .../decidim_fix_nickname_uniqueness.rake | 51 ++++++++++++++ .../decidim/user_parser_spec.rb | 13 ++++ .../decidim/profiles_controller_spec.rb | 25 +++++++ .../user_activities_controller_spec.rb | 8 +++ .../decidim/user_timeline_controller_spec.rb | 34 +++++++++ .../decidim/change_nickname_event_spec.rb | 17 +++++ .../spec/forms/registration_form_spec.rb | 2 +- .../spec/system/authentication_spec.rb | 21 ++++++ ...idim_tasks_fix_nickname_uniqueness_spec.rb | 70 +++++++++++++++++++ 19 files changed, 283 insertions(+), 16 deletions(-) create mode 100644 decidim-core/app/events/decidim/change_nickname_event.rb create mode 100644 decidim-core/lib/tasks/upgrade/decidim_fix_nickname_uniqueness.rake create mode 100644 decidim-core/spec/controllers/decidim/profiles_controller_spec.rb create mode 100644 decidim-core/spec/controllers/decidim/user_timeline_controller_spec.rb create mode 100644 decidim-core/spec/events/decidim/change_nickname_event_spec.rb create mode 100644 decidim-core/spec/tasks/decidim_tasks_fix_nickname_uniqueness_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 282f5e11d69cf..448120a1a7792 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/decidim-core/app/controllers/decidim/profiles_controller.rb b/decidim-core/app/controllers/decidim/profiles_controller.rb index 6834759d38e52..95bfe3f5fa205 100644 --- a/decidim-core/app/controllers/decidim/profiles_controller.rb +++ b/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 diff --git a/decidim-core/app/controllers/decidim/user_activities_controller.rb b/decidim-core/app/controllers/decidim/user_activities_controller.rb index 535006b6f0c30..bcc91d97c10c7 100644 --- a/decidim-core/app/controllers/decidim/user_activities_controller.rb +++ b/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 diff --git a/decidim-core/app/controllers/decidim/user_timeline_controller.rb b/decidim-core/app/controllers/decidim/user_timeline_controller.rb index ac6e9f0de89f1..9b24a0490d7ca 100644 --- a/decidim-core/app/controllers/decidim/user_timeline_controller.rb +++ b/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 diff --git a/decidim-core/app/events/decidim/change_nickname_event.rb b/decidim-core/app/events/decidim/change_nickname_event.rb new file mode 100644 index 0000000000000..ceca486b81156 --- /dev/null +++ b/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 diff --git a/decidim-core/app/forms/decidim/account_form.rb b/decidim-core/app/forms/decidim/account_form.rb index ef8898df91697..b5eef163117f0 100644 --- a/decidim-core/app/forms/decidim/account_form.rb +++ b/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 diff --git a/decidim-core/app/forms/decidim/registration_form.rb b/decidim-core/app/forms/decidim/registration_form.rb index d49da2d229095..58647490ac712 100644 --- a/decidim-core/app/forms/decidim/registration_form.rb +++ b/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 diff --git a/decidim-core/config/locales/en.yml b/decidim-core/config/locales/en.yml index 7ff976aa7637f..bdba29e23c0f3 100644 --- a/decidim-core/config/locales/en.yml +++ b/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 admin panel. + 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.
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. notification_event: notification_title: An event occured to %{resource_title}. 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. diff --git a/decidim-core/lib/decidim/content_parsers/user_parser.rb b/decidim-core/lib/decidim/content_parsers/user_parser.rb index 1864a554ba918..bb60e530ec45e 100644 --- a/decidim-core/lib/decidim/content_parsers/user_parser.rb +++ b/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 diff --git a/decidim-core/lib/decidim/nicknamizable.rb b/decidim-core/lib/decidim/nicknamizable.rb index 8239cbd5dfdc8..af77d19cb9929 100644 --- a/decidim-core/lib/decidim/nicknamizable.rb +++ b/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 diff --git a/decidim-core/lib/tasks/upgrade/decidim_fix_nickname_uniqueness.rake b/decidim-core/lib/tasks/upgrade/decidim_fix_nickname_uniqueness.rake new file mode 100644 index 0000000000000..92eb99c4235db --- /dev/null +++ b/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 diff --git a/decidim-core/spec/content_parsers/decidim/user_parser_spec.rb b/decidim-core/spec/content_parsers/decidim/user_parser_spec.rb index 524dd9768905c..39151e7b165e9 100644 --- a/decidim-core/spec/content_parsers/decidim/user_parser_spec.rb +++ b/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 diff --git a/decidim-core/spec/controllers/decidim/profiles_controller_spec.rb b/decidim-core/spec/controllers/decidim/profiles_controller_spec.rb new file mode 100644 index 0000000000000..67faac2021d93 --- /dev/null +++ b/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 diff --git a/decidim-core/spec/controllers/decidim/user_activities_controller_spec.rb b/decidim-core/spec/controllers/decidim/user_activities_controller_spec.rb index 7a1211684d6e0..b346add726426 100644 --- a/decidim-core/spec/controllers/decidim/user_activities_controller_spec.rb +++ b/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 diff --git a/decidim-core/spec/controllers/decidim/user_timeline_controller_spec.rb b/decidim-core/spec/controllers/decidim/user_timeline_controller_spec.rb new file mode 100644 index 0000000000000..4a14279010298 --- /dev/null +++ b/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 diff --git a/decidim-core/spec/events/decidim/change_nickname_event_spec.rb b/decidim-core/spec/events/decidim/change_nickname_event_spec.rb new file mode 100644 index 0000000000000..9f5f35743aeb4 --- /dev/null +++ b/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.
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.") + end + end +end diff --git a/decidim-core/spec/forms/registration_form_spec.rb b/decidim-core/spec/forms/registration_form_spec.rb index bdfc7c10ddf94..8f6acab3d68e9 100644 --- a/decidim-core/spec/forms/registration_form_spec.rb +++ b/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 } diff --git a/decidim-core/spec/system/authentication_spec.rb b/decidim-core/spec/system/authentication_spec.rb index 4c61b57e6bdfb..9f56f823e9811 100644 --- a/decidim-core/spec/system/authentication_spec.rb +++ b/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: "user@example.org" + 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) } diff --git a/decidim-core/spec/tasks/decidim_tasks_fix_nickname_uniqueness_spec.rb b/decidim-core/spec/tasks/decidim_tasks_fix_nickname_uniqueness_spec.rb new file mode 100644 index 0000000000000..0a1856a6c1f2c --- /dev/null +++ b/decidim-core/spec/tasks/decidim_tasks_fix_nickname_uniqueness_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe "rake decidim:upgrade:fix_nickname_uniqueness", type: :task do + context "when all users come from the same organization" do + let(:organization) { create(:organization) } + let!(:user1) { create(:user, :confirmed, nickname: "toto", organization: organization) } + let!(:user2) { create(:user, :confirmed, nickname: "Toto", organization: organization) } + let!(:user3) { create(:user, :confirmed, nickname: "TOTO", organization: organization) } + let!(:user4) { create(:user, :confirmed, nickname: "foO", organization: organization) } + let!(:user5) { create(:user, :confirmed, nickname: "Foo", organization: organization) } + + context "when executing task" do + it "have to be executed without failures" do + allow($stdin).to receive(:gets).and_return("N") + expect { task.execute }.not_to raise_error + end + + it "has to change nicknames" do + task.execute + + expect(user1.reload.nickname).to eq("toto") + expect(user2.reload.nickname).to eq("toto_2") + expect(user3.reload.nickname).to eq("toto_3") + expect(user4.reload.nickname).to eq("foO") + expect(user5.reload.nickname).to eq("foo_2") + end + + it "send notifications" do + expect(Decidim::EventsManager).to receive(:publish).exactly(3).times + + task.execute + end + end + end + + context "when users come from differents organizations" do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + let!(:user1) { create(:user, :confirmed, nickname: "toto", organization: organization1) } + let!(:user2) { create(:user, :confirmed, nickname: "Toto", organization: organization1) } + let!(:user3) { create(:user, :confirmed, nickname: "TOTO", organization: organization2) } + let!(:user4) { create(:user, :confirmed, nickname: "foO", organization: organization1) } + let!(:user5) { create(:user, :confirmed, nickname: "Foo", organization: organization2) } + + context "when executing task" do + it "have to be executed without failures" do + allow($stdin).to receive(:gets).and_return("N") + expect { task.execute }.not_to raise_error + end + + it "has to change nicknames" do + task.execute + + expect(user1.reload.nickname).to eq("toto") + expect(user2.reload.nickname).to match(/toto_2/) + expect(user3.reload.nickname).to eq("TOTO") + expect(user4.reload.nickname).to eq("foO") + expect(user5.reload.nickname).to eq("Foo") + end + + it "send notifications" do + expect(Decidim::EventsManager).to receive(:publish).once + + task.execute + end + end + end +end