From 59dfb51a35ce37c91d34e9c1817c6e536735f1bb Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 18 Jan 2017 12:20:23 +0800 Subject: [PATCH 1/2] FIX: Don't change automatic group name if localized name has been taken. --- app/models/group.rb | 8 +++++++- spec/models/group_spec.rb | 39 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index b97c34356c5..fa6cb204d1f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -161,7 +161,13 @@ class Group < ActiveRecord::Base # don't allow shoddy localization to break this localized_name = I18n.t("groups.default_names.#{name}") validator = UsernameValidator.new(localized_name) - group.name = validator.valid_format? ? localized_name : name + + group.name = + if !Group.where(name: localized_name).exists? && validator.valid_format? + localized_name + else + name + end # the everyone group is special, it can include non-users so there is no # way to have the membership in a table diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 8a6477ed731..86f79597c33 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -154,9 +154,42 @@ describe Group do end - it "makes sure the everyone group is not visible" do - g = Group.refresh_automatic_group!(:everyone) - expect(g.visible).to eq(false) + describe '.refresh_automatic_group!' do + it "makes sure the everyone group is not visible" do + g = Group.refresh_automatic_group!(:everyone) + expect(g.visible).to eq(false) + end + + it "uses the localized name if name has not been taken" do + begin + default_locale = SiteSetting.default_locale + I18n.locale = SiteSetting.default_locale = 'de' + + group = Group.refresh_automatic_group!(:staff) + + expect(group.name).to_not eq('staff') + expect(group.name).to eq(I18n.t('groups.default_names.staff')) + ensure + I18n.locale = SiteSetting.default_locale = default_locale + end + end + + it "does not use the localized name if name has already been taken" do + begin + default_locale = SiteSetting.default_locale + I18n.locale = SiteSetting.default_locale = 'de' + + another_group = Fabricate(:group, + name: I18n.t('groups.default_names.staff') + ) + + group = Group.refresh_automatic_group!(:staff) + + expect(group.name).to eq('staff') + ensure + I18n.locale = SiteSetting.default_locale = default_locale + end + end end it "Correctly handles removal of primary group" do From 706b4f6b9faf1f820a294e83f82b0b5a5dba0475 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 18 Jan 2017 13:39:34 +0800 Subject: [PATCH 2/2] FEATURE: Remap group mentions when group name has been changed. --- app/jobs/regular/update_group_mentions.rb | 14 +++++++ app/models/group.rb | 12 ++++++ app/services/group_mentions_updater.rb | 14 +++++++ spec/services/group_mentions_updater_spec.rb | 39 ++++++++++++++++++++ 4 files changed, 79 insertions(+) create mode 100644 app/jobs/regular/update_group_mentions.rb create mode 100644 app/services/group_mentions_updater.rb create mode 100644 spec/services/group_mentions_updater_spec.rb diff --git a/app/jobs/regular/update_group_mentions.rb b/app/jobs/regular/update_group_mentions.rb new file mode 100644 index 00000000000..67ea356d3b0 --- /dev/null +++ b/app/jobs/regular/update_group_mentions.rb @@ -0,0 +1,14 @@ +module Jobs + + class UpdateGroupMentions < Jobs::Base + + def execute(args) + group = Group.find_by(id: args[:group_id]) + return unless group + + previous_group_name = args[:previous_name] + + GroupMentionsUpdater.update(group.name, previous_group_name) + end + end +end diff --git a/app/models/group.rb b/app/models/group.rb index fa6cb204d1f..618d71cf313 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -22,6 +22,9 @@ class Group < ActiveRecord::Base after_save :update_primary_group after_save :update_title + after_save :enqueue_update_mentions_job, + if: Proc.new { |g| g.name_was && g.name_changed? } + after_save :expire_cache after_destroy :expire_cache @@ -493,6 +496,15 @@ SQL builder.exec end end + + private + + def enqueue_update_mentions_job + Jobs.enqueue(:update_group_mentions, + previous_name: self.name_was, + group_id: self.id + ) + end end # == Schema Information diff --git a/app/services/group_mentions_updater.rb b/app/services/group_mentions_updater.rb new file mode 100644 index 00000000000..0bd07a6425a --- /dev/null +++ b/app/services/group_mentions_updater.rb @@ -0,0 +1,14 @@ +class GroupMentionsUpdater + def self.update(current_name, previous_name) + Post.where( + "cooked LIKE '%class=\"mention-group\"%' AND raw LIKE :previous_name", + previous_name: "%@#{previous_name}%" + ).find_in_batches do |posts| + + posts.each do |post| + post.raw.gsub!(/(^|\s)(@#{previous_name})(\s|$)/, "\\1@#{current_name}\\3") + post.save! + end + end + end +end diff --git a/spec/services/group_mentions_updater_spec.rb b/spec/services/group_mentions_updater_spec.rb new file mode 100644 index 00000000000..688e672c045 --- /dev/null +++ b/spec/services/group_mentions_updater_spec.rb @@ -0,0 +1,39 @@ +require 'rails_helper' + +RSpec.describe GroupMentionsUpdater do + let(:post) { Fabricate(:post) } + + describe '.update' do + it 'should update valid group mentions' do + new_group_name = 'awesome_team' + old_group_name = 'team' + + [ + ["@#{old_group_name} is awesome!", "@#{new_group_name} is awesome!"], + ["This @#{old_group_name} is awesome!", "This @#{new_group_name} is awesome!"], + ["Mention us @ @#{old_group_name}", "Mention us @ @#{new_group_name}"], + ].each do |raw, expected_raw| + group = Fabricate(:group, name: old_group_name) + post.update!(raw: raw) + group.update!(name: new_group_name) + post.reload + + expect(post.raw_mentions).to eq([new_group_name]) + expect(post.raw).to eq(expected_raw) + + group.destroy! + end + end + + it 'should not update invalid group mentions' do + group = Fabricate(:group, name: 'team') + post.update!(raw: 'This is not valid@team.com') + + expect(post.reload.raw_mentions).to eq([]) + + group.update!(name: 'new_team_name') + + expect(post.reload.raw_mentions).to eq([]) + end + end +end