notes_controller_spec.rb 15.4 KB
Newer Older
1
require 'spec_helper'
ZJ van de Weg's avatar
ZJ van de Weg committed
2
3

describe Projects::NotesController do
4
5
  include ProjectForksHelper

ZJ van de Weg's avatar
ZJ van de Weg committed
6
  let(:user)    { create(:user) }
7
  let(:project) { create(:project) }
ZJ van de Weg's avatar
ZJ van de Weg committed
8
9
10
  let(:issue)   { create(:issue, project: project) }
  let(:note)    { create(:note, noteable: issue, project: project) }

Douwe Maan's avatar
Douwe Maan committed
11
12
13
14
15
16
17
18
  let(:request_params) do
    {
      namespace_id: project.namespace,
      project_id: project,
      id: note
    }
  end

19
  describe 'GET index' do
Douwe Maan's avatar
Douwe Maan committed
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
    let(:request_params) do
      {
        namespace_id: project.namespace,
        project_id: project,
        target_type: 'issue',
        target_id: issue.id,
        format: 'json'
      }
    end

    let(:parsed_response) { JSON.parse(response.body).with_indifferent_access }
    let(:note_json) { parsed_response[:notes].first }

    before do
      sign_in(user)
      project.team << [user, :developer]
    end

    it 'passes last_fetched_at from headers to NotesFinder' do
Douwe Maan's avatar
Douwe Maan committed
39
40
      last_fetched_at = 3.hours.ago.to_i

Douwe Maan's avatar
Douwe Maan committed
41
42
43
44
45
46
47
48
49
50
      request.headers['X-Last-Fetched-At'] = last_fetched_at

      expect(NotesFinder).to receive(:new)
        .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
        .and_call_original

      get :index, request_params
    end

    context 'for a discussion note' do
Douwe Maan's avatar
Douwe Maan committed
51
52
53
54
      let(:project) { create(:project, :repository) }
      let!(:note) { create(:discussion_note_on_merge_request, project: project) }

      let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }
Douwe Maan's avatar
Douwe Maan committed
55

Douwe Maan's avatar
Douwe Maan committed
56
      it 'responds with the expected attributes' do
Douwe Maan's avatar
Douwe Maan committed
57
        get :index, params
Douwe Maan's avatar
Douwe Maan committed
58
59
60
61

        expect(note_json[:id]).to eq(note.id)
        expect(note_json[:discussion_html]).not_to be_nil
        expect(note_json[:diff_discussion_html]).to be_nil
62
        expect(note_json[:discussion_line_code]).to be_nil
Douwe Maan's avatar
Douwe Maan committed
63
64
65
66
67
68
69
70
71
      end
    end

    context 'for a diff discussion note' do
      let(:project) { create(:project, :repository) }
      let!(:note) { create(:diff_note_on_merge_request, project: project) }

      let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }

Douwe Maan's avatar
Douwe Maan committed
72
      it 'responds with the expected attributes' do
Douwe Maan's avatar
Douwe Maan committed
73
74
75
76
77
        get :index, params

        expect(note_json[:id]).to eq(note.id)
        expect(note_json[:discussion_html]).not_to be_nil
        expect(note_json[:diff_discussion_html]).not_to be_nil
78
        expect(note_json[:discussion_line_code]).not_to be_nil
Douwe Maan's avatar
Douwe Maan committed
79
80
81
82
83
84
85
86
87
88
89
90
      end
    end

    context 'for a commit note' do
      let(:project) { create(:project, :repository) }
      let!(:note) { create(:note_on_commit, project: project) }

      context 'when displayed on a merge request' do
        let(:merge_request) { create(:merge_request, source_project: project) }

        let(:params) { request_params.merge(target_type: 'merge_request', target_id: merge_request.id) }

Douwe Maan's avatar
Douwe Maan committed
91
        it 'responds with the expected attributes' do
Douwe Maan's avatar
Douwe Maan committed
92
93
94
95
96
          get :index, params

          expect(note_json[:id]).to eq(note.id)
          expect(note_json[:discussion_html]).not_to be_nil
          expect(note_json[:diff_discussion_html]).to be_nil
97
          expect(note_json[:discussion_line_code]).to be_nil
Douwe Maan's avatar
Douwe Maan committed
98
99
100
101
102
103
        end
      end

      context 'when displayed on the commit' do
        let(:params) { request_params.merge(target_type: 'commit', target_id: note.commit_id) }

Douwe Maan's avatar
Douwe Maan committed
104
        it 'responds with the expected attributes' do
Douwe Maan's avatar
Douwe Maan committed
105
106
107
108
109
          get :index, params

          expect(note_json[:id]).to eq(note.id)
          expect(note_json[:discussion_html]).to be_nil
          expect(note_json[:diff_discussion_html]).to be_nil
110
          expect(note_json[:discussion_line_code]).to be_nil
Douwe Maan's avatar
Douwe Maan committed
111
        end
112
113
114
115
116
117
118
119
120
121
122
123
124

        context 'when user cannot read commit' do
          before do
            allow(Ability).to receive(:allowed?).and_call_original
            allow(Ability).to receive(:allowed?).with(user, :download_code, project).and_return(false)
          end

          it 'renders 404' do
            get :index, params

            expect(response).to have_gitlab_http_status(404)
          end
        end
Douwe Maan's avatar
Douwe Maan committed
125
126
127
128
      end
    end

    context 'for a regular note' do
Douwe Maan's avatar
Douwe Maan committed
129
130
131
      let!(:note) { create(:note_on_merge_request, project: project) }

      let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }
Douwe Maan's avatar
Douwe Maan committed
132

Douwe Maan's avatar
Douwe Maan committed
133
      it 'responds with the expected attributes' do
Douwe Maan's avatar
Douwe Maan committed
134
        get :index, params
Douwe Maan's avatar
Douwe Maan committed
135
136
137
138
139

        expect(note_json[:id]).to eq(note.id)
        expect(note_json[:html]).not_to be_nil
        expect(note_json[:discussion_html]).to be_nil
        expect(note_json[:diff_discussion_html]).to be_nil
140
        expect(note_json[:discussion_line_code]).to be_nil
Douwe Maan's avatar
Douwe Maan committed
141
142
      end
    end
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176

    context 'with cross-reference system note', :request_store do
      let(:new_issue) { create(:issue) }
      let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }

      before do
        note
        create(:discussion_note_on_issue, :system, noteable: issue, project: issue.project, note: cross_reference)
      end

      it 'filters notes that the user should not see' do
        get :index, request_params

        expect(parsed_response[:notes].count).to eq(1)
        expect(note_json[:id]).to eq(note.id)
      end

      it 'does not result in N+1 queries' do
        # Instantiate the controller variables to ensure QueryRecorder has an accurate base count
        get :index, request_params

        RequestStore.clear!

        control_count = ActiveRecord::QueryRecorder.new do
          get :index, request_params
        end.count

        RequestStore.clear!

        create_list(:discussion_note_on_issue, 2, :system, noteable: issue, project: issue.project, note: cross_reference)

        expect { get :index, request_params }.not_to exceed_query_limit(control_count)
      end
    end
177
178
  end

179
180
  describe 'POST create' do
    let(:merge_request) { create(:merge_request) }
181
    let(:project) { merge_request.source_project }
182
183
184
185
186
    let(:request_params) do
      {
        note: { note: 'some note', noteable_id: merge_request.id, noteable_type: 'MergeRequest' },
        namespace_id: project.namespace,
        project_id: project,
Douwe Maan's avatar
Douwe Maan committed
187
188
189
        merge_request_diff_head_sha: 'sha',
        target_type: 'merge_request',
        target_id: merge_request.id
190
191
192
193
194
      }
    end

    before do
      sign_in(user)
195
      project.add_developer(user)
196
197
198
199
200
    end

    it "returns status 302 for html" do
      post :create, request_params

201
      expect(response).to have_gitlab_http_status(302)
202
203
204
205
206
    end

    it "returns status 200 for json" do
      post :create, request_params.merge(format: :json)

207
      expect(response).to have_gitlab_http_status(200)
208
209
210
211
212
213
214
215
    end

    context 'when merge_request_diff_head_sha present' do
      before do
        service_params = {
          note: 'some note',
          noteable_id: merge_request.id.to_s,
          noteable_type: 'MergeRequest',
216
          merge_request_diff_head_sha: 'sha',
Douwe Maan's avatar
Douwe Maan committed
217
          in_reply_to_discussion_id: nil
218
219
220
221
222
223
224
225
        }

        expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true))
      end

      it "returns status 302 for html" do
        post :create, request_params

226
        expect(response).to have_gitlab_http_status(302)
227
228
      end
    end
229
230

    context 'when creating a commit comment from an MR fork' do
231
      let(:project) { create(:project, :repository) }
232

233
234
      let(:forked_project) do
        fork_project(project, nil, repository: true)
235
236
237
      end

      let(:merge_request) do
238
        create(:merge_request, source_project: forked_project, target_project: project, source_branch: 'feature', target_branch: 'master')
239
240
241
      end

      let(:existing_comment) do
242
        create(:note_on_commit, note: 'a note', project: forked_project, commit_id: merge_request.commit_shas.first)
243
244
245
246
247
248
249
250
251
      end

      def post_create(extra_params = {})
        post :create, {
               note: { note: 'some other note' },
               namespace_id: project.namespace,
               project_id: project,
               target_type: 'merge_request',
               target_id: merge_request.id,
252
               note_project_id: forked_project.id,
253
254
255
256
257
258
259
260
               in_reply_to_discussion_id: existing_comment.discussion_id
             }.merge(extra_params)
      end

      context 'when the note_project_id is not correct' do
        it 'returns a 404' do
          post_create(note_project_id: Project.maximum(:id).succ)

261
          expect(response).to have_gitlab_http_status(404)
262
263
264
265
266
267
268
        end
      end

      context 'when the user has no access to the fork' do
        it 'returns a 404' do
          post_create

269
          expect(response).to have_gitlab_http_status(404)
270
271
272
273
        end
      end

      context 'when the user has access to the fork' do
274
        let(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) }
275
276

        before do
277
          forked_project.add_developer(user)
278
279
280
281
282

          existing_comment
        end

        it 'creates the note' do
283
          expect { post_create }.to change { forked_project.notes.count }.by(1)
284
285
286
        end
      end
    end
287
288
289
290
291
292
293

    context 'when the merge request discussion is locked' do
      before do
        project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
        merge_request.update_attribute(:discussion_locked, true)
      end

294
295
296
297
298
      context 'when a noteable is not found' do
        it 'returns 404 status' do
          request_params[:note][:noteable_id] = 9999
          post :create, request_params.merge(format: :json)

299
          expect(response).to have_gitlab_http_status(404)
300
301
302
        end
      end

303
304
305
306
      context 'when a user is a team member' do
        it 'returns 302 status for html' do
          post :create, request_params

307
          expect(response).to have_gitlab_http_status(302)
308
309
310
311
312
        end

        it 'returns 200 status for json' do
          post :create, request_params.merge(format: :json)

313
          expect(response).to have_gitlab_http_status(200)
314
315
316
        end

        it 'creates a new note' do
317
          expect { post :create, request_params }.to change { Note.count }.by(1)
318
319
320
321
322
323
324
325
326
327
328
        end
      end

      context 'when a user is not a team member' do
        before do
          project.project_member(user).destroy
        end

        it 'returns 404 status' do
          post :create, request_params

329
          expect(response).to have_gitlab_http_status(404)
330
331
332
        end

        it 'does not create a new note' do
333
          expect { post :create, request_params }.not_to change { Note.count }
334
335
336
        end
      end
    end
337
338
  end

339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
  describe 'PUT update' do
    let(:request_params) do
      {
        namespace_id: project.namespace,
        project_id: project,
        id: note,
        format: :json,
        note: {
          note: "New comment"
        }
      }
    end

    before do
      sign_in(note.author)
      project.team << [note.author, :developer]
    end

    it "updates the note" do
      expect { put :update, request_params }.to change { note.reload.note }
    end
  end

362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
  describe 'DELETE destroy' do
    let(:request_params) do
      {
        namespace_id: project.namespace,
        project_id: project,
        id: note,
        format: :js
      }
    end

    context 'user is the author of a note' do
      before do
        sign_in(note.author)
        project.team << [note.author, :developer]
      end

      it "returns status 200 for html" do
        delete :destroy, request_params

381
        expect(response).to have_gitlab_http_status(200)
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
      end

      it "deletes the note" do
        expect { delete :destroy, request_params }.to change { Note.count }.from(1).to(0)
      end
    end

    context 'user is not the author of a note' do
      before do
        sign_in(user)
        project.team << [user, :developer]
      end

      it "returns status 404" do
        delete :destroy, request_params

398
        expect(response).to have_gitlab_http_status(404)
399
400
401
402
      end
    end
  end

403
  describe 'POST toggle_award_emoji' do
ZJ van de Weg's avatar
ZJ van de Weg committed
404
405
406
407
408
409
410
    before do
      sign_in(user)
      project.team << [user, :developer]
    end

    it "toggles the award emoji" do
      expect do
Douwe Maan's avatar
Douwe Maan committed
411
        post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
Z.J. van de Weg's avatar
Z.J. van de Weg committed
412
      end.to change { note.award_emoji.count }.by(1)
ZJ van de Weg's avatar
ZJ van de Weg committed
413

414
      expect(response).to have_gitlab_http_status(200)
ZJ van de Weg's avatar
ZJ van de Weg committed
415
416
    end

Z.J. van de Weg's avatar
Z.J. van de Weg committed
417
    it "removes the already awarded emoji" do
Douwe Maan's avatar
Douwe Maan committed
418
      post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
ZJ van de Weg's avatar
ZJ van de Weg committed
419
420

      expect do
Douwe Maan's avatar
Douwe Maan committed
421
        post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
ZJ van de Weg's avatar
ZJ van de Weg committed
422
423
      end.to change { AwardEmoji.count }.by(-1)

424
      expect(response).to have_gitlab_http_status(200)
ZJ van de Weg's avatar
ZJ van de Weg committed
425
426
    end
  end
427

Douwe Maan's avatar
Douwe Maan committed
428
  describe "resolving and unresolving" do
429
    let(:project) { create(:project, :repository) }
Douwe Maan's avatar
Douwe Maan committed
430
431
    let(:merge_request) { create(:merge_request, source_project: project) }
    let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
432

Douwe Maan's avatar
Douwe Maan committed
433
    describe 'POST resolve' do
434
      before do
Douwe Maan's avatar
Douwe Maan committed
435
        sign_in user
436
437
      end

Douwe Maan's avatar
Douwe Maan committed
438
      context "when the user is not authorized to resolve the note" do
439
        it "returns status 404" do
Douwe Maan's avatar
Douwe Maan committed
440
          post :resolve, request_params
441

442
          expect(response).to have_gitlab_http_status(404)
443
444
445
        end
      end

Douwe Maan's avatar
Douwe Maan committed
446
447
448
449
      context "when the user is authorized to resolve the note" do
        before do
          project.team << [user, :developer]
        end
450

Douwe Maan's avatar
Douwe Maan committed
451
452
453
454
        context "when the note is not resolvable" do
          before do
            note.update(system: true)
          end
455

Douwe Maan's avatar
Douwe Maan committed
456
457
458
          it "returns status 404" do
            post :resolve, request_params

459
            expect(response).to have_gitlab_http_status(404)
Douwe Maan's avatar
Douwe Maan committed
460
          end
461
462
        end

Douwe Maan's avatar
Douwe Maan committed
463
464
465
        context "when the note is resolvable" do
          it "resolves the note" do
            post :resolve, request_params
466

Douwe Maan's avatar
Douwe Maan committed
467
468
469
470
471
472
            expect(note.reload.resolved?).to be true
            expect(note.reload.resolved_by).to eq(user)
          end

          it "sends notifications if all discussions are resolved" do
            expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
473

Douwe Maan's avatar
Douwe Maan committed
474
475
476
477
478
            post :resolve, request_params
          end

          it "returns the name of the resolving user" do
            post :resolve, request_params
479

Douwe Maan's avatar
Douwe Maan committed
480
481
            expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
          end
482

Douwe Maan's avatar
Douwe Maan committed
483
484
          it "returns status 200" do
            post :resolve, request_params
485

486
            expect(response).to have_gitlab_http_status(200)
Douwe Maan's avatar
Douwe Maan committed
487
          end
488
489
490
491
        end
      end
    end

Douwe Maan's avatar
Douwe Maan committed
492
493
494
    describe 'DELETE unresolve' do
      before do
        sign_in user
495

Douwe Maan's avatar
Douwe Maan committed
496
        note.resolve!(user)
497
498
      end

Douwe Maan's avatar
Douwe Maan committed
499
      context "when the user is not authorized to resolve the note" do
500
        it "returns status 404" do
Douwe Maan's avatar
Douwe Maan committed
501
          delete :unresolve, request_params
502

503
          expect(response).to have_gitlab_http_status(404)
504
505
506
        end
      end

Douwe Maan's avatar
Douwe Maan committed
507
508
509
      context "when the user is authorized to resolve the note" do
        before do
          project.team << [user, :developer]
510
511
        end

Douwe Maan's avatar
Douwe Maan committed
512
513
514
515
516
517
518
519
        context "when the note is not resolvable" do
          before do
            note.update(system: true)
          end

          it "returns status 404" do
            delete :unresolve, request_params

520
            expect(response).to have_gitlab_http_status(404)
Douwe Maan's avatar
Douwe Maan committed
521
522
          end
        end
523

Douwe Maan's avatar
Douwe Maan committed
524
525
526
527
528
529
530
531
532
533
        context "when the note is resolvable" do
          it "unresolves the note" do
            delete :unresolve, request_params

            expect(note.reload.resolved?).to be false
          end

          it "returns status 200" do
            delete :unresolve, request_params

534
            expect(response).to have_gitlab_http_status(200)
Douwe Maan's avatar
Douwe Maan committed
535
          end
536
537
538
539
        end
      end
    end
  end
ZJ van de Weg's avatar
ZJ van de Weg committed
540
end