diff options
-rw-r--r-- | app/models/group.rb | 11 | ||||
-rw-r--r-- | app/models/user.rb | 5 | ||||
-rw-r--r-- | app/services/system_hooks_service.rb | 48 | ||||
-rw-r--r-- | changelogs/unreleased/winh-namespace-rename-hooks.yml | 5 | ||||
-rw-r--r-- | doc/system_hooks/system_hooks.md | 70 | ||||
-rw-r--r-- | spec/models/concerns/routable_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/group_spec.rb | 41 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 36 | ||||
-rw-r--r-- | spec/services/system_hooks_service_spec.rb | 38 |
9 files changed, 236 insertions, 19 deletions
diff --git a/app/models/group.rb b/app/models/group.rb index 4e8023cdb7f..c660de7fcb6 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -42,6 +42,7 @@ class Group < Namespace after_create :post_create_hook after_destroy :post_destroy_hook after_save :update_two_factor_requirement + after_update :path_changed_hook, if: :path_changed? class << self def supports_nested_groups? @@ -295,6 +296,12 @@ class Group < Namespace list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten end + def full_path_was + return path_was unless has_parent? + + "#{parent.full_path}/#{path_was}" + end + private def update_two_factor_requirement @@ -303,6 +310,10 @@ class Group < Namespace users.find_each(&:update_two_factor_requirement) end + def path_changed_hook + system_hook_service.execute_hooks_for(self, :rename) + end + def visibility_level_allowed_by_parent return if visibility_level_allowed_by_parent? diff --git a/app/models/user.rb b/app/models/user.rb index 6c9349ed9dd..bcda4564595 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -168,6 +168,7 @@ class User < ActiveRecord::Base before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) } before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? } after_save :ensure_namespace_correct + after_update :username_changed_hook, if: :username_changed? after_destroy :post_destroy_hook after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') } after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') } @@ -871,6 +872,10 @@ class User < ActiveRecord::Base end end + def username_changed_hook + system_hook_service.execute_hooks_for(self, :rename) + end + def post_destroy_hook log_info("User \"#{name}\" (#{email}) was removed") system_hook_service.execute_hooks_for(self, :destroy) diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index 5d275967821..911cc919bb8 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -35,24 +35,22 @@ class SystemHooksService data[:old_path_with_namespace] = model.old_path_with_namespace end when User - data.merge!({ - name: model.name, - email: model.email, - user_id: model.id, - username: model.username - }) + data.merge!(user_data(model)) + + if event == :rename + data[:old_username] = model.username_was + end when ProjectMember data.merge!(project_member_data(model)) when Group - owner = model.owner + data.merge!(group_data(model)) - data.merge!( - name: model.name, - path: model.path, - group_id: model.id, - owner_name: owner.respond_to?(:name) ? owner.name : nil, - owner_email: owner.respond_to?(:email) ? owner.email : nil - ) + if event == :rename + data.merge!( + old_path: model.path_was, + old_full_path: model.full_path_was + ) + end when GroupMember data.merge!(group_member_data(model)) end @@ -104,6 +102,19 @@ class SystemHooksService } end + def group_data(model) + owner = model.owner + + { + name: model.name, + path: model.path, + full_path: model.full_path, + group_id: model.id, + owner_name: owner.try(:name), + owner_email: owner.try(:email) + } + end + def group_member_data(model) { group_name: model.group.name, @@ -116,4 +127,13 @@ class SystemHooksService group_access: model.human_access } end + + def user_data(model) + { + name: model.name, + email: model.email, + user_id: model.id, + username: model.username + } + end end diff --git a/changelogs/unreleased/winh-namespace-rename-hooks.yml b/changelogs/unreleased/winh-namespace-rename-hooks.yml new file mode 100644 index 00000000000..f5090b03b74 --- /dev/null +++ b/changelogs/unreleased/winh-namespace-rename-hooks.yml @@ -0,0 +1,5 @@ +--- +title: Add system hooks user_rename and group_rename +merge_request: 15123 +author: +type: changed diff --git a/doc/system_hooks/system_hooks.md b/doc/system_hooks/system_hooks.md index a45a4eb9e49..f2a9b1d769b 100644 --- a/doc/system_hooks/system_hooks.md +++ b/doc/system_hooks/system_hooks.md @@ -1,6 +1,24 @@ # System hooks -Your GitLab instance can perform HTTP POST requests on the following events: `project_create`, `project_destroy`, `project_rename`, `project_transfer`, `project_update`, `user_add_to_team`, `user_remove_from_team`, `user_create`, `user_destroy`, `key_create`, `key_destroy`, `group_create`, `group_destroy`, `user_add_to_group` and `user_remove_from_group`. +Your GitLab instance can perform HTTP POST requests on the following events: + +- `project_create` +- `project_destroy` +- `project_rename` +- `project_transfer` +- `project_update` +- `user_add_to_team` +- `user_remove_from_team` +- `user_create` +- `user_destroy` +- `user_rename` +- `key_create` +- `key_destroy` +- `group_create` +- `group_destroy` +- `group_rename` +- `user_add_to_group` +- `user_remove_from_group` The triggers for most of these are self-explanatory, but `project_update` and `project_rename` deserve some clarification: `project_update` is fired any time an attribute of a project is changed (name, description, tags, etc.) *unless* the `path` attribute is also changed. In that case, a `project_rename` is triggered instead (so that, for instance, if all you care about is the repo URL, you can just listen for `project_rename`). @@ -72,6 +90,9 @@ X-Gitlab-Event: System Hook } ``` +Note that `project_rename` is not triggered if the namespace changes. +Please refer to `group_rename` and `user_rename` for that case. + **Project transferred:** ```json @@ -175,6 +196,21 @@ X-Gitlab-Event: System Hook } ``` +**User renamed:** + +```json +{ + "event_name": "user_rename", + "created_at": "2017-11-01T11:21:04Z", + "updated_at": "2017-11-01T14:04:47Z", + "name": "new-name", + "email": "best-email@example.tld", + "user_id": 58, + "username": "new-exciting-name", + "old_username": "old-boring-name" +} +``` + **Key added** ```json @@ -209,13 +245,15 @@ X-Gitlab-Event: System Hook "updated_at": "2012-07-21T07:38:22Z", "event_name": "group_create", "name": "StoreCloud", - "owner_email": "johnsmith@gmail.com", - "owner_name": "John Smith", + "owner_email": null, + "owner_name": null, "path": "storecloud", "group_id": 78 } ``` +`owner_name` and `owner_email` are always `null`. Please see https://gitlab.com/gitlab-org/gitlab-ce/issues/39675. + **Group removed:** ```json @@ -224,13 +262,35 @@ X-Gitlab-Event: System Hook "updated_at": "2012-07-21T07:38:22Z", "event_name": "group_destroy", "name": "StoreCloud", - "owner_email": "johnsmith@gmail.com", - "owner_name": "John Smith", + "owner_email": null, + "owner_name": null, "path": "storecloud", "group_id": 78 } ``` +`owner_name` and `owner_email` are always `null`. Please see https://gitlab.com/gitlab-org/gitlab-ce/issues/39675. + +**Group renamed:** + +```json +{ + "event_name": "group_rename", + "created_at": "2017-10-30T15:09:00Z", + "updated_at": "2017-11-01T10:23:52Z", + "name": "Better Name", + "path": "better-name", + "full_path": "parent-group/better-name", + "group_id": 64, + "owner_name": null, + "owner_email": null, + "old_path": "old-name", + "old_full_path": "parent-group/old-name" +} +``` + +`owner_name` and `owner_email` are always `null`. Please see https://gitlab.com/gitlab-org/gitlab-ce/issues/39675. + **New Group Member:** ```json diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index ab8773b7ede..3106207811a 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -134,6 +134,7 @@ describe Group, 'Routable' do context 'with RequestStore active', :request_store do it 'does not load the route table more than once' do + group.expires_full_path_cache expect(group).to receive(:uncached_full_path).once.and_call_original 3.times { group.full_path } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index f36d6eeb327..0e1a7fdce0b 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -488,6 +488,47 @@ describe Group do end end + describe '#path_changed_hook' do + let(:system_hook_service) { SystemHooksService.new } + + context 'for a new group' do + let(:group) { build(:group) } + + before do + expect(group).to receive(:system_hook_service).and_return(system_hook_service) + end + + it 'does not trigger system hook' do + expect(system_hook_service).to receive(:execute_hooks_for).with(group, :create) + + group.save! + end + end + + context 'for an existing group' do + let(:group) { create(:group, path: 'old-path') } + + context 'when the path is changed' do + let(:new_path) { 'very-new-path' } + + it 'triggers the rename system hook' do + expect(group).to receive(:system_hook_service).and_return(system_hook_service) + expect(system_hook_service).to receive(:execute_hooks_for).with(group, :rename) + + group.update_attributes!(path: new_path) + end + end + + context 'when the path is not changed' do + it 'does not trigger system hook' do + expect(group).not_to receive(:system_hook_service) + + group.update_attributes!(name: 'new name') + end + end + end + end + describe '#secret_variables_for' do let(:project) { create(:project, group: group) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fb03e320734..e0896d64c8f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2217,6 +2217,42 @@ describe User do end end + describe '#username_changed_hook' do + context 'for a new user' do + let(:user) { build(:user) } + + it 'does not trigger system hook' do + expect(user).not_to receive(:system_hook_service) + + user.save! + end + end + + context 'for an existing user' do + let(:user) { create(:user, username: 'old-username') } + + context 'when the username is changed' do + let(:new_username) { 'very-new-name' } + + it 'triggers the rename system hook' do + system_hook_service = SystemHooksService.new + expect(system_hook_service).to receive(:execute_hooks_for).with(user, :rename) + expect(user).to receive(:system_hook_service).and_return(system_hook_service) + + user.update_attributes!(username: new_username) + end + end + + context 'when the username is not changed' do + it 'does not trigger system hook' do + expect(user).not_to receive(:system_hook_service) + + user.update_attributes!(email: 'asdf@asdf.com') + end + end + end + end + describe '#sync_attribute?' do let(:user) { described_class.new } diff --git a/spec/services/system_hooks_service_spec.rb b/spec/services/system_hooks_service_spec.rb index 8f7aea533dc..46cd10cdc12 100644 --- a/spec/services/system_hooks_service_spec.rb +++ b/spec/services/system_hooks_service_spec.rb @@ -69,11 +69,48 @@ describe SystemHooksService do expect(data[:project_visibility]).to eq('private') end + + context 'group_rename' do + it 'contains old and new path' do + allow(group).to receive(:path_was).and_return('old-path') + + data = event_data(group, :rename) + + expect(data).to include(:event_name, :name, :created_at, :updated_at, :full_path, :path, :group_id, :old_path, :old_full_path) + expect(data[:path]).to eq(group.path) + expect(data[:full_path]).to eq(group.path) + expect(data[:old_path]).to eq(group.path_was) + expect(data[:old_full_path]).to eq(group.path_was) + end + + it 'contains old and new full_path for subgroup' do + subgroup = create(:group, parent: group) + allow(subgroup).to receive(:path_was).and_return('old-path') + + data = event_data(subgroup, :rename) + + expect(data[:full_path]).to eq(subgroup.full_path) + expect(data[:old_path]).to eq('old-path') + end + end + + context 'user_rename' do + it 'contains old and new username' do + allow(user).to receive(:username_was).and_return('old-username') + + data = event_data(user, :rename) + + expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :old_username) + expect(data[:username]).to eq(user.username) + expect(data[:old_username]).to eq(user.username_was) + end + end end context 'event names' do it { expect(event_name(user, :create)).to eq "user_create" } it { expect(event_name(user, :destroy)).to eq "user_destroy" } + it { expect(event_name(user, :rename)).to eq 'user_rename' } it { expect(event_name(project, :create)).to eq "project_create" } it { expect(event_name(project, :destroy)).to eq "project_destroy" } it { expect(event_name(project, :rename)).to eq "project_rename" } @@ -85,6 +122,7 @@ describe SystemHooksService do it { expect(event_name(key, :destroy)).to eq 'key_destroy' } it { expect(event_name(group, :create)).to eq 'group_create' } it { expect(event_name(group, :destroy)).to eq 'group_destroy' } + it { expect(event_name(group, :rename)).to eq 'group_rename' } it { expect(event_name(group_member, :create)).to eq 'user_add_to_group' } it { expect(event_name(group_member, :destroy)).to eq 'user_remove_from_group' } end |