Add system hooks user_rename and group_renamewinh-namespace-rename-hooks

6 files changed, 129 insertions, 19 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 07fb62bb249..bb1d5aacdd7 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_save :path_changed_hook, if: :has_path_changed?
 
   class << self
     def supports_nested_groups?
@@ -297,6 +298,20 @@ class Group < Namespace
     users.find_each(&:update_two_factor_requirement)
   end
 
+  def has_path_changed?
+    path_changed? && !path_was.nil?
+  end
+
+  def full_path_was
+    return model.path_was unless has_parent?
+
+    "#{parent.full_path}/#{path_was}"
+  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 9459b6d4fa4..f02bacbfaa9 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_save :username_changed_hook, if: :has_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') }
@@ -873,6 +874,14 @@ class User < ActiveRecord::Base
     end
   end
 
+  def has_username_changed?
+    username_changed? && !username_was.nil?
+  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 }