From: Andy Allan Date: Wed, 17 Jan 2024 16:24:09 +0000 (+0000) Subject: Merge pull request #4405 from tomhughes/normalize-display-name X-Git-Tag: live~1278 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/d5efa4c357c1f39181f9af96024eaacbb6bd328b?hp=-c Merge pull request #4405 from tomhughes/normalize-display-name Require user names to be unique after unicode normalisation --- d5efa4c357c1f39181f9af96024eaacbb6bd328b diff --combined app/models/user.rb index bc95f20c1,5b48e3a68..b04f8e2b9 --- a/app/models/user.rb +++ b/app/models/user.rb @@@ -34,12 -34,13 +34,13 @@@ # # Indexes # - # users_auth_idx (auth_provider,auth_uid) UNIQUE - # users_display_name_idx (display_name) UNIQUE - # users_display_name_lower_idx (lower((display_name)::text)) - # users_email_idx (email) UNIQUE - # users_email_lower_idx (lower((email)::text)) - # users_home_idx (home_tile) + # users_auth_idx (auth_provider,auth_uid) UNIQUE + # users_display_name_canonical_idx (lower(NORMALIZE(display_name, NFKC))) + # users_display_name_idx (display_name) UNIQUE + # users_display_name_lower_idx (lower((display_name)::text)) + # users_email_idx (email) UNIQUE + # users_email_lower_idx (lower((email)::text)) + # users_home_idx (home_tile) # class User < ApplicationRecord @@@ -51,10 -52,9 +52,10 @@@ has_many :diary_comments, -> { order(:created_at => :desc) }, :inverse_of => :user has_many :diary_entry_subscriptions, :class_name => "DiaryEntrySubscription" has_many :diary_subscriptions, :through => :diary_entry_subscriptions, :source => :diary_entry - has_many :messages, -> { where(:to_user_visible => true).order(:sent_on => :desc).preload(:sender, :recipient) }, :foreign_key => :to_user_id - has_many :new_messages, -> { where(:to_user_visible => true, :message_read => false).order(:sent_on => :desc) }, :class_name => "Message", :foreign_key => :to_user_id + has_many :messages, -> { where(:to_user_visible => true, :muted => false).order(:sent_on => :desc).preload(:sender, :recipient) }, :foreign_key => :to_user_id + has_many :new_messages, -> { where(:to_user_visible => true, :muted => false, :message_read => false).order(:sent_on => :desc) }, :class_name => "Message", :foreign_key => :to_user_id has_many :sent_messages, -> { where(:from_user_visible => true).order(:sent_on => :desc).preload(:sender, :recipient) }, :class_name => "Message", :foreign_key => :from_user_id + has_many :muted_messages, -> { where(:to_user_visible => true, :muted => true).order(:sent_on => :desc).preload(:sender, :recipient) }, :class_name => "Message", :foreign_key => :to_user_id has_many :friendships, -> { joins(:befriendee).where(:users => { :status => %w[active confirmed] }) } has_many :friends, :through => :friendships, :source => :befriendee has_many :tokens, :class_name => "UserToken", :dependent => :destroy @@@ -76,9 -76,6 +77,9 @@@ has_many :blocks_created, :class_name => "UserBlock", :foreign_key => :creator_id, :inverse_of => :creator has_many :blocks_revoked, :class_name => "UserBlock", :foreign_key => :revoker_id, :inverse_of => :revoker + has_many :mutes, -> { order(:created_at => :desc) }, :class_name => "UserMute", :foreign_key => :owner_id, :inverse_of => :owner + has_many :muted_users, :through => :mutes, :source => :subject + has_many :roles, :class_name => "UserRole" has_many :issues, :class_name => "Issue", :foreign_key => :reported_user_id, :inverse_of => :reported_user @@@ -95,7 -92,7 +96,7 @@@ validates :display_name, :presence => true, :length => 3..255, :exclusion => %w[new terms save confirm confirm-email go_public reset-password forgot-password suspended] validates :display_name, :if => proc { |u| u.display_name_changed? }, - :uniqueness => { :case_sensitive => false } + :normalized_uniqueness => { :case_sensitive => false } validates :display_name, :if => proc { |u| u.display_name_changed? }, :characters => { :url_safe => true }, :whitespace => { :leading => false, :trailing => false } @@@ -119,6 -116,7 +120,6 @@@ alias_attribute :created_at, :creation_time - after_initialize :encrypt_password before_save :encrypt_password before_save :update_tile after_save :spam_check @@@ -132,7 -130,7 +133,7 @@@ user = find_by("email = ? OR display_name = ?", options[:username].strip, options[:username]) if user.nil? - users = where("LOWER(email) = LOWER(?) OR LOWER(display_name) = LOWER(?)", options[:username].strip, options[:username]) + users = where("LOWER(email) = LOWER(?) OR LOWER(NORMALIZE(display_name, NFKC)) = LOWER(NORMALIZE(?, NFKC))", options[:username].strip, options[:username]) user = users.first if users.count == 1 end diff --combined db/structure.sql index 0563417cd,7c5233d67..c9fdebbc8 --- a/db/structure.sql +++ b/db/structure.sql @@@ -951,8 -951,7 +951,8 @@@ CREATE TABLE public.messages to_user_id bigint NOT NULL, to_user_visible boolean DEFAULT true NOT NULL, from_user_visible boolean DEFAULT true NOT NULL, - body_format public.format_enum DEFAULT 'markdown'::public.format_enum NOT NULL + body_format public.format_enum DEFAULT 'markdown'::public.format_enum NOT NULL, + muted boolean DEFAULT false NOT NULL ); @@@ -1455,38 -1454,6 +1455,38 @@@ CREATE SEQUENCE public.user_blocks_id_s ALTER SEQUENCE public.user_blocks_id_seq OWNED BY public.user_blocks.id; +-- +-- Name: user_mutes; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.user_mutes ( + id bigint NOT NULL, + owner_id bigint NOT NULL, + subject_id bigint NOT NULL, + created_at timestamp(6) without time zone NOT NULL, + updated_at timestamp(6) without time zone NOT NULL +); + + +-- +-- Name: user_mutes_id_seq; Type: SEQUENCE; Schema: public; Owner: - +-- + +CREATE SEQUENCE public.user_mutes_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + + +-- +-- Name: user_mutes_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - +-- + +ALTER SEQUENCE public.user_mutes_id_seq OWNED BY public.user_mutes.id; + + -- -- Name: user_preferences; Type: TABLE; Schema: public; Owner: - -- @@@ -1868,13 -1835,6 +1868,13 @@@ ALTER TABLE ONLY public.reports ALTER C ALTER TABLE ONLY public.user_blocks ALTER COLUMN id SET DEFAULT nextval('public.user_blocks_id_seq'::regclass); +-- +-- Name: user_mutes id; Type: DEFAULT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.user_mutes ALTER COLUMN id SET DEFAULT nextval('public.user_mutes_id_seq'::regclass); + + -- -- Name: user_roles id; Type: DEFAULT; Schema: public; Owner: - -- @@@ -2256,14 -2216,6 +2256,14 @@@ ALTER TABLE ONLY public.user_block ADD CONSTRAINT user_blocks_pkey PRIMARY KEY (id); +-- +-- Name: user_mutes user_mutes_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.user_mutes + ADD CONSTRAINT user_mutes_pkey PRIMARY KEY (id); + + -- -- Name: user_preferences user_preferences_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@@ -2782,13 -2734,6 +2782,13 @@@ CREATE INDEX index_reports_on_user_id O CREATE INDEX index_user_blocks_on_user_id ON public.user_blocks USING btree (user_id); +-- +-- Name: index_user_mutes_on_owner_id_and_subject_id; Type: INDEX; Schema: public; Owner: - +-- + +CREATE UNIQUE INDEX index_user_mutes_on_owner_id_and_subject_id ON public.user_mutes USING btree (owner_id, subject_id); + + -- -- Name: messages_from_user_id_idx; Type: INDEX; Schema: public; Owner: - -- @@@ -2922,6 -2867,13 +2922,13 @@@ CREATE INDEX user_tokens_user_id_idx O CREATE UNIQUE INDEX users_auth_idx ON public.users USING btree (auth_provider, auth_uid); + -- + -- Name: users_display_name_canonical_idx; Type: INDEX; Schema: public; Owner: - + -- + + CREATE INDEX users_display_name_canonical_idx ON public.users USING btree (lower(NORMALIZE(display_name, NFKC))); + + -- -- Name: users_display_name_idx; Type: INDEX; Schema: public; Owner: - -- @@@ -3162,14 -3114,6 +3169,14 @@@ ALTER TABLE ONLY public.oauth_access_gr ADD CONSTRAINT fk_rails_330c32d8d9 FOREIGN KEY (resource_owner_id) REFERENCES public.users(id) NOT VALID; +-- +-- Name: user_mutes fk_rails_591dad3359; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.user_mutes + ADD CONSTRAINT fk_rails_591dad3359 FOREIGN KEY (owner_id) REFERENCES public.users(id); + + -- -- Name: oauth_access_tokens fk_rails_732cb83ab7; Type: FK CONSTRAINT; Schema: public; Owner: - -- @@@ -3218,14 -3162,6 +3225,14 @@@ ALTER TABLE ONLY public.oauth_applicati ADD CONSTRAINT fk_rails_cc886e315a FOREIGN KEY (owner_id) REFERENCES public.users(id) NOT VALID; +-- +-- Name: user_mutes fk_rails_e9dd4fb6c3; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.user_mutes + ADD CONSTRAINT fk_rails_e9dd4fb6c3 FOREIGN KEY (subject_id) REFERENCES public.users(id); + + -- -- Name: oauth_access_tokens fk_rails_ee63f25419; Type: FK CONSTRAINT; Schema: public; Owner: - -- @@@ -3581,12 -3517,11 +3588,13 @@@ INSERT INTO "schema_migrations" (versio ('23'), ('22'), ('21'), + ('20231213182102'), ('20231206141457'), ('20231117170422'), ('20231101222146'), ('20231029151516'), +('20231010203028'), +('20231010201451'), ('20231010194809'), ('20231007141103'), ('20230830115220'), diff --combined test/models/user_test.rb index 4e2675a2e,1443e70b0..d21512f2a --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@@ -10,7 -10,7 +10,7 @@@ class UserTest < ActiveSupport::TestCas :home_lat => nil, :home_lon => nil, :home_zoom => nil) - assert_not user.valid? + assert_not_predicate user, :valid? assert_predicate user.errors[:email], :any? assert_predicate user.errors[:pass_crypt], :any? assert_predicate user.errors[:display_name], :any? @@@ -27,10 -27,13 +27,13 @@@ end def test_unique_display_name - existing_user = create(:user) - new_user = build(:user, :display_name => existing_user.display_name) - assert_not new_user.save - assert_includes new_user.errors[:display_name], "has already been taken" + create(:user, :display_name => "H\u{e9}nryIV") + + %W[H\u{e9}nryIV he\u{301}nryiv H\u{c9}nry\u2163 he\u{301}nry\u2173].each do |name| + new_user = build(:user, :display_name => name) + assert_not new_user.save + assert_includes new_user.errors[:display_name], "has already been taken" + end end def test_email_valid @@@ -57,11 -60,11 +60,11 @@@ user.display_name = "123" assert_predicate user, :valid?, "should allow 3 char name name" user.display_name = "12" - assert_not user.valid?, "should not allow 2 char name" + assert_not_predicate user, :valid?, "should not allow 2 char name" user.display_name = "" - assert_not user.valid?, "should not allow blank/0 char name" + assert_not_predicate user, :valid?, "should not allow blank/0 char name" user.display_name = nil - assert_not user.valid?, "should not allow nil value" + assert_not_predicate user, :valid?, "should not allow nil value" end def test_display_name_valid @@@ -87,7 -90,7 +90,7 @@@ bad.each do |display_name| user = build(:user) user.display_name = display_name - assert_not user.valid?, "#{display_name} is valid when it shouldn't be" + assert_not_predicate user, :valid?, "#{display_name} is valid when it shouldn't be" end end @@@ -217,25 -220,25 +220,25 @@@ assert_predicate build(:user, :pending), :visible? assert_predicate build(:user, :active), :visible? assert_predicate build(:user, :confirmed), :visible? - assert_not build(:user, :suspended).visible? - assert_not build(:user, :deleted).visible? + assert_not_predicate build(:user, :suspended), :visible? + assert_not_predicate build(:user, :deleted), :visible? end def test_active? - assert_not build(:user, :pending).active? + assert_not_predicate build(:user, :pending), :active? assert_predicate build(:user, :active), :active? assert_predicate build(:user, :confirmed), :active? - assert_not build(:user, :suspended).active? - assert_not build(:user, :deleted).active? + assert_not_predicate build(:user, :suspended), :active? + assert_not_predicate build(:user, :deleted), :active? end def test_moderator? - assert_not create(:user).moderator? + assert_not_predicate create(:user), :moderator? assert_predicate create(:moderator_user), :moderator? end def test_administrator? - assert_not create(:user).administrator? + assert_not_predicate create(:user), :administrator? assert_predicate create(:administrator_user), :administrator? end @@@ -253,10 -256,10 +256,10 @@@ assert_predicate user.description, :blank? assert_nil user.home_lat assert_nil user.home_lon - assert_not user.avatar.attached? + assert_not_predicate user.avatar, :attached? assert_equal "deleted", user.status - assert_not user.visible? - assert_not user.active? + assert_not_predicate user, :visible? + assert_not_predicate user, :active? end def test_soft_destroy_revokes_oauth1_tokens