Merge branch 'notes-ee-feature' into 'master'

Move admin notes feature to GitLab Core

Closes #30934

See merge request gitlab-org/gitlab!31457

app/controllers/admin/users_controller.rb

@@ -241,7 +241,8 @@ class Admin::UsersController < Admin::ApplicationController
       :theme_id,
       :twitter,
       :username,
-      :website_url
+      :website_url,
+      :note
     ]
   end
 

app/services/users/build_service.rb

@@ -82,7 +82,8 @@ module Users
         :organization,
         :location,
         :public_email,
-        :user_type
+        :user_type,
+        :note
       ]
     end
 

app/views/admin/users/_form.html.haml

@@ -83,7 +83,7 @@
         .col-sm-10
           = f.text_field :website_url, class: 'form-control'
 
-    = render_if_exists 'admin/users/admin_notes', f: f
+    = render 'admin/users/admin_notes', f: f
 
     .form-actions
       - if @user.new_record?

app/views/admin/users/_user_detail.html.haml

@@ -6,7 +6,7 @@
         = image_tag avatar_icon_for_user(user), class: 'avatar s16 d-xs-flex d-md-none mr-1 gl-mt-2', alt: _('Avatar for %{name}') % { name: sanitize_name(user.name) }
         = link_to user.name, admin_user_path(user), class: 'text-plain js-user-link', data: { user_id: user.id, qa_selector: 'username_link' }
 
-      = render_if_exists 'admin/users/user_listing_note', user: user
+      = render 'admin/users/user_listing_note', user: user
 
       - user_badges_in_admin_section(user).each do |badge|
         - css_badge = "badge badge-#{badge[:variant]}" if badge[:variant].present?

ee/app/views/admin/users/_user_detail_note.html.haml → app/views/admin/users/_user_detail_note.html.haml

@@ -2,6 +2,6 @@
   - text = @user.note
   .card.border-info
     .card-header.bg-info.text-white
-      Admin Note
+      = _('Admin Note')
     .card-body
       %p= text

app/views/admin/users/show.html.haml

@@ -154,7 +154,7 @@
             %br
             = link_to 'Confirm user', confirm_admin_user_path(@user), method: :put, class: "btn btn-info", data: { confirm: 'Are you sure?', qa_selector: 'confirm_user_button' }
 
-      = render_if_exists 'admin/users/user_detail_note'
+      = render 'admin/users/user_detail_note'
 
       - if @user.deactivated?
         .card.border-info

changelogs/unreleased/notes-ee-feature.yml

+---
+title: Move Admin note feature to GitLab Core
+merge_request: 31457
+author: Rajendra
+type: added

doc/api/users.md

@@ -104,6 +104,7 @@ GET /users
     "color_scheme_id": 2,
     "projects_limit": 100,
     "current_sign_in_at": "2012-06-02T06:36:55Z",
+    "note": "DMCA Request: 2018-11-05 | DMCA Violation | Abuse | https://gitlab.zendesk.com/agent/tickets/123",
     "identities": [
       {"provider": "github", "extern_uid": "2435223452345"},
       {"provider": "bitbucket", "extern_uid": "john.smith"},
@@ -154,7 +155,7 @@ GET /users
 ]
 ```
 
-Users on GitLab [Starter, Bronze, or higher](https://about.gitlab.com/pricing/) will also see the `shared_runners_minutes_limit`, `extra_shared_runners_minutes_limit`, and `note` parameters.
+Users on GitLab [Starter, Bronze, or higher](https://about.gitlab.com/pricing/) will also see the `shared_runners_minutes_limit`, and `extra_shared_runners_minutes_limit` parameters.
 
 ```json
 [
@@ -163,7 +164,6 @@ Users on GitLab [Starter, Bronze, or higher](https://about.gitlab.com/pricing/)
     ...
     "shared_runners_minutes_limit": 133,
     "extra_shared_runners_minutes_limit": 133,
-    "note": "DMCA Request: 2018-11-05 | DMCA Violation | Abuse | https://gitlab.zendesk.com/agent/tickets/123",
     ...
   }
 ]
@@ -296,6 +296,7 @@ Example Responses:
   "color_scheme_id": 2,
   "projects_limit": 100,
   "current_sign_in_at": "2012-06-02T06:36:55Z",
+  "note": "DMCA Request: 2018-11-05 | DMCA Violation | Abuse | https://gitlab.zendesk.com/agent/tickets/123",
   "identities": [
     {"provider": "github", "extern_uid": "2435223452345"},
     {"provider": "bitbucket", "extern_uid": "john.smith"},
@@ -316,7 +317,7 @@ Example Responses:
 NOTE: **Note:** The `plan` and `trial` parameters are only available on GitLab Enterprise Edition.
 
 Users on GitLab [Starter, Bronze, or higher](https://about.gitlab.com/pricing/) will also see
-the `shared_runners_minutes_limit`, `extra_shared_runners_minutes_limit`, and `note` parameters.
+the `shared_runners_minutes_limit`, and `extra_shared_runners_minutes_limit` parameters.
 
 ```json
 {
@@ -324,7 +325,6 @@ the `shared_runners_minutes_limit`, `extra_shared_runners_minutes_limit`, and `n
   "username": "john_smith",
   "shared_runners_minutes_limit": 133,
   "extra_shared_runners_minutes_limit": 133,
-  "note": "DMCA Request: 2018-11-05 | DMCA Violation | Abuse | https://gitlab.zendesk.com/agent/tickets/123",
   ...
 }
 ```
@@ -338,7 +338,6 @@ see the `group_saml` option:
   "username": "john_smith",
   "shared_runners_minutes_limit": 133,
   "extra_shared_runners_minutes_limit": 133,
-  "note": "DMCA Request: 2018-11-05 | DMCA Violation | Abuse | https://gitlab.zendesk.com/agent/tickets/123",
   "identities": [
     {"provider": "github", "extern_uid": "2435223452345"},
     {"provider": "bitbucket", "extern_uid": "john.smith"},
@@ -391,6 +390,7 @@ Parameters:
 | `linkedin`                           | No       | LinkedIn                                                                                                                                                |
 | `location`                           | No       | User's location                                                                                                                                         |
 | `name`                               | Yes      | Name                                                                                                                                                    |
+| `note`                               | No       | Admin notes for this user                                                                                                                               |
 | `organization`                       | No       | Organization name                                                                                                                                       |
 | `password`                           | No       | Password                                                                                                                                                |
 | `private_profile`                    | No       | User's profile is private - true, false (default), or null (will be converted to false)                                                                 |
@@ -432,7 +432,7 @@ Parameters:
 | `linkedin`                           | No       | LinkedIn                                                                                                                                                |
 | `location`                           | No       | User's location                                                                                                                                         |
 | `name`                               | No       | Name                                                                                                                                                    |
-| `note`                               | No       | Admin notes for this user **(STARTER)**                                                                                                                 |
+| `note`                               | No       | Admin notes for this user                                                                                                                  |
 | `organization`                       | No       | Organization name                                                                                                                                       |
 | `password`                           | No       | Password                                                                                                                                                |
 | `private_profile`                    | No       | User's profile is private - true, false (default), or null (will be converted to false)                                                                 |

ee/app/controllers/ee/admin/users_controller.rb

@@ -33,7 +33,6 @@ module EE
 
       def allowed_user_params
         super + [
-          :note,
           namespace_attributes: [
             :id,
             :shared_runners_minutes_limit,

ee/lib/ee/api/entities/user_with_admin.rb

-# frozen_string_literal: true
-
-module EE
-  module API
-    module Entities
-      module UserWithAdmin
-        extend ActiveSupport::Concern
-
-        prepended do
-          expose :note
-        end
-      end
-    end
-  end
-end

ee/lib/ee/api/helpers/users_helpers.rb

@@ -12,7 +12,6 @@ module EE
             optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this user'
             optional :extra_shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Extra pipeline minutes quota for this user'
             optional :group_id_for_saml, type: Integer, desc: 'ID for group where SAML has been configured'
-            optional :note, type: String, desc: 'Admin note for this user'
           end
 
           params :optional_index_params_ee do

ee/spec/requests/api/users_spec.rb

@@ -138,145 +138,6 @@ describe API::Users do
     end
   end
 
-  context 'admin notes' do
-    let(:admin) { create(:admin, note: '2019-10-06 | 2FA added | user requested | www.gitlab.com') }
-    let(:user) { create(:user, note: '2018-11-05 | 2FA removed | user requested | www.gitlab.com') }
-
-    describe 'GET /users/:id' do
-      context 'when unauthenticated' do
-        it 'does not contain the note of the user' do
-          get api("/users/#{user.id}")
-
-          expect(json_response).not_to have_key('note')
-        end
-      end
-
-      context 'when authenticated' do
-        context 'as an admin' do
-          it 'contains the note of the user' do
-            get api("/users/#{user.id}", admin)
-
-            expect(json_response).to have_key('note')
-            expect(json_response['note']).to eq(user.note)
-          end
-        end
-
-        context 'as a regular user' do
-          it 'does not contain the note of the user' do
-            get api("/users/#{user.id}", user)
-
-            expect(json_response).not_to have_key('note')
-          end
-        end
-      end
-    end
-
-    describe "PUT /users/:id" do
-      context 'when user is an admin' do
-        it "updates note of the user" do
-          new_note = '2019-07-07 | Email changed | user requested | www.gitlab.com'
-
-          expect do
-            put api("/users/#{user.id}", admin), params: { note: new_note }
-          end.to change { user.reload.note }
-                   .from('2018-11-05 | 2FA removed | user requested | www.gitlab.com')
-                   .to(new_note)
-
-          expect(response).to have_gitlab_http_status(:success)
-          expect(json_response['note']).to eq(new_note)
-        end
-      end
-
-      context 'when user is not an admin' do
-        it "cannot update their own note" do
-          expect do
-            put api("/users/#{user.id}", user), params: { note: 'new note' }
-          end.not_to change { user.reload.note }
-
-          expect(response).to have_gitlab_http_status(:forbidden)
-        end
-      end
-    end
-
-    describe 'GET /users/' do
-      context 'when unauthenticated' do
-        it "does not contain the note of users" do
-          get api("/users"), params: { username: user.username }
-
-          expect(json_response.first).not_to have_key('note')
-        end
-      end
-
-      context 'when authenticated' do
-        context 'as a regular user' do
-          it 'does not contain the note of users' do
-            get api("/users", user), params: { username: user.username }
-
-            expect(json_response.first).not_to have_key('note')
-          end
-        end
-
-        context 'as an admin' do
-          it 'contains the note of users' do
-            get api("/users", admin), params: { username: user.username }
-
-            expect(response).to have_gitlab_http_status(:success)
-            expect(json_response.first).to have_key('note')
-            expect(json_response.first['note']).to eq '2018-11-05 | 2FA removed | user requested | www.gitlab.com'
-          end
-        end
-      end
-    end
-
-    describe 'GET /user' do
-      context 'when authenticated' do
-        context 'as an admin' do
-          context 'accesses their own profile' do
-            it 'contains the note of the user' do
-              get api("/user", admin)
-
-              expect(json_response).to have_key('note')
-              expect(json_response['note']).to eq(admin.note)
-            end
-          end
-
-          context 'sudo' do
-            let(:admin_personal_access_token) { create(:personal_access_token, user: admin, scopes: %w[api sudo]).token }
-
-            context 'accesses the profile of another regular user' do
-              it 'does not contain the note of the user' do
-                get api("/user?private_token=#{admin_personal_access_token}&sudo=#{user.id}")
-
-                expect(json_response['id']).to eq(user.id)
-                expect(json_response).not_to have_key('note')
-              end
-            end
-
-            context 'accesses the profile of another admin' do
-              let(:admin_2) {create(:admin, note: '2010-10-10 | 2FA added | admin requested | www.gitlab.com')}
-
-              it 'contains the note of the user' do
-                get api("/user?private_token=#{admin_personal_access_token}&sudo=#{admin_2.id}")
-
-                expect(json_response['id']).to eq(admin_2.id)
-                expect(json_response).to have_key('note')
-                expect(json_response['note']).to eq(admin_2.note)
-              end
-            end
-          end
-        end
-
-        context 'as a regular user' do
-          it 'does not contain the note of the user' do
-            get api("/user", user)
-
-            expect(json_response).not_to have_key('note')
-          end
-        end
-      end
-    end
-  end
-
   describe 'GET /user/:id' do
     context 'when authenticated' do
       context 'as an admin' do

lib/api/entities/user_with_admin.rb

@@ -4,8 +4,7 @@ module API
   module Entities
     class UserWithAdmin < UserPublic
       expose :admin?, as: :is_admin
+      expose :note
     end
   end
 end
-
-API::Entities::UserWithAdmin.prepend_if_ee('EE::API::Entities::UserWithAdmin', with_descendants: true)

lib/api/users.rb

@@ -55,6 +55,7 @@ module API
           optional :theme_id, type: Integer, desc: 'The GitLab theme for the user'
           optional :color_scheme_id, type: Integer, desc: 'The color scheme for the file viewer'
           optional :private_profile, type: Boolean, desc: 'Flag indicating the user has a private profile'
+          optional :note, type: String, desc: 'Admin note for this user'
           all_or_none_of :extern_uid, :provider
 
           use :optional_params_ee

locale/gitlab.pot

@@ -1427,6 +1427,9 @@ msgstr ""
 msgid "Admin Area"
 msgstr ""
 
+msgid "Admin Note"
+msgstr ""
+
 msgid "Admin Overview"
 msgstr ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -254,6 +254,18 @@ describe Admin::UsersController do
       errors = assigns[:user].errors
       expect(errors).to contain_exactly(errors.full_message(:email, I18n.t('errors.messages.invalid')))
     end
+
+    context 'admin notes' do
+      it 'creates the user with note' do
+        note = '2020-05-12 | Note | DCMA | Link'
+        user_params = attributes_for(:user, note: note)
+
+        expect { post :create, params: { user: user_params } }.to change { User.count }.by(1)
+
+        new_user = User.last
+        expect(new_user.note).to eq(note)
+      end
+    end
   end
 
   describe 'POST update' do
@@ -338,6 +350,20 @@ describe Admin::UsersController do
         end
       end
     end
+
+    context 'admin notes' do
+      it 'updates the note for the user' do
+        note = '2020-05-12 | Note | DCMA | Link'
+        params = {
+          id: user.to_param,
+          user: {
+            note: note
+          }
+        }
+
+        expect { post :update, params: params }.to change { user.reload.note }.to(note)
+      end
+    end
   end
 
   describe "DELETE #remove_email" do

spec/requests/api/users_spec.rb

@@ -15,6 +15,177 @@ describe API::Users, :do_not_mock_admin_mode do
   let(:not_existing_pat_id) { (PersonalAccessToken.maximum('id') || 0 ) + 10 }
   let(:private_user) { create(:user, private_profile: true) }
 
+  context 'admin notes' do
+    let(:admin) { create(:admin, note: '2019-10-06 | 2FA added | user requested | www.gitlab.com') }
+    let(:user) { create(:user, note: '2018-11-05 | 2FA removed | user requested | www.gitlab.com') }
+
+    describe 'POST /users' do
+      context 'when unauthenticated' do
+        it 'return authentication error' do
+          post api('/users')
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      context 'when authenticated' do
+        context 'as an admin' do
+          it 'contains the note of the user' do
+            optional_attributes = { note: 'Awesome Note' }
+            attributes = attributes_for(:user).merge(optional_attributes)
+
+            post api('/users', admin), params: attributes
+
+            expect(response).to have_gitlab_http_status(:created)
+            expect(json_response['note']).to eq(optional_attributes[:note])
+          end
+        end
+
+        context 'as a regular user' do
+          it 'does not allow creating new user' do
+            post api('/users', user), params: attributes_for(:user)
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
+      end
+    end
+
+    describe 'GET /users/:id' do
+      context 'when unauthenticated' do
+        it 'does not contain the note of the user' do
+          get api("/users/#{user.id}")
+
+          expect(json_response).not_to have_key('note')
+        end
+      end
+
+      context 'when authenticated' do
+        context 'as an admin' do
+          it 'contains the note of the user' do
+            get api("/users/#{user.id}", admin)
+
+            expect(json_response).to have_key('note')
+            expect(json_response['note']).to eq(user.note)
+          end
+        end
+
+        context 'as a regular user' do
+          it 'does not contain the note of the user' do
+            get api("/users/#{user.id}", user)
+
+            expect(json_response).not_to have_key('note')
+          end
+        end
+      end
+    end
+
+    describe "PUT /users/:id" do
+      context 'when user is an admin' do
+        it "updates note of the user" do
+          new_note = '2019-07-07 | Email changed | user requested | www.gitlab.com'
+
+          expect do
+            put api("/users/#{user.id}", admin), params: { note: new_note }
+          end.to change { user.reload.note }
+                   .from('2018-11-05 | 2FA removed | user requested | www.gitlab.com')
+                   .to(new_note)
+
+          expect(response).to have_gitlab_http_status(:success)
+          expect(json_response['note']).to eq(new_note)
+        end
+      end
+
+      context 'when user is not an admin' do
+        it "cannot update their own note" do
+          expect do
+            put api("/users/#{user.id}", user), params: { note: 'new note' }
+          end.not_to change { user.reload.note }
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
+    end
+
+    describe 'GET /users/' do
+      context 'when unauthenticated' do
+        it "does not contain the note of users" do
+          get api("/users"), params: { username: user.username }
+
+          expect(json_response.first).not_to have_key('note')
+        end
+      end
+
+      context 'when authenticated' do
+        context 'as a regular user' do
+          it 'does not contain the note of users' do
+            get api("/users", user), params: { username: user.username }
+
+            expect(json_response.first).not_to have_key('note')
+          end
+        end
+
+        context 'as an admin' do
+          it 'contains the note of users' do
+            get api("/users", admin), params: { username: user.username }
+
+            expect(response).to have_gitlab_http_status(:success)
+            expect(json_response.first).to have_key('note')
+            expect(json_response.first['note']).to eq '2018-11-05 | 2FA removed | user requested | www.gitlab.com'
+          end
+        end
+      end
+    end
+
+    describe 'GET /user' do
+      context 'when authenticated' do
+        context 'as an admin' do
+          context 'accesses their own profile' do
+            it 'contains the note of the user' do
+              get api("/user", admin)
+
+              expect(json_response).to have_key('note')
+              expect(json_response['note']).to eq(admin.note)
+            end
+          end
+
+          context 'sudo' do
+            let(:admin_personal_access_token) { create(:personal_access_token, user: admin, scopes: %w[api sudo]).token }
+
+            context 'accesses the profile of another regular user' do
+              it 'does not contain the note of the user' do
+                get api("/user?private_token=#{admin_personal_access_token}&sudo=#{user.id}")
+
+                expect(json_response['id']).to eq(user.id)
+                expect(json_response).not_to have_key('note')
+              end
+            end
+
+            context 'accesses the profile of another admin' do
+              let(:admin_2) {create(:admin, note: '2010-10-10 | 2FA added | admin requested | www.gitlab.com')}
+
+              it 'contains the note of the user' do
+                get api("/user?private_token=#{admin_personal_access_token}&sudo=#{admin_2.id}")
+
+                expect(json_response['id']).to eq(admin_2.id)
+                expect(json_response).to have_key('note')
+                expect(json_response['note']).to eq(admin_2.note)
+              end
+            end
+          end
+        end
+
+        context 'as a regular user' do
+          it 'does not contain the note of the user' do
+            get api("/user", user)
+
+            expect(json_response).not_to have_key('note')
+          end
+        end
+      end
+    end
+  end
+
   shared_examples 'rendering user status' do
     it 'returns the status if there was one' do
       create(:user_status, user: user)