From da4702493d046e6de24c0a071b0162818e9d990c Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 19 Jun 2019 14:17:08 +0100 Subject: [PATCH 1/2] Fix label serialisation in issue and note hooks We were not calling hook_attrs on the labels correctly. Specs were passing because the issues under test did not have any labels! --- changelogs/unreleased/fix-labels-in-hooks.yml | 5 +++++ lib/gitlab/data_builder/note.rb | 2 +- lib/gitlab/hook_data/issue_builder.rb | 2 +- spec/lib/gitlab/data_builder/note_spec.rb | 19 ++++++++++++------- .../gitlab/hook_data/issue_builder_spec.rb | 4 +++- 5 files changed, 22 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/fix-labels-in-hooks.yml diff --git a/changelogs/unreleased/fix-labels-in-hooks.yml b/changelogs/unreleased/fix-labels-in-hooks.yml new file mode 100644 index 00000000000..c0904a860c5 --- /dev/null +++ b/changelogs/unreleased/fix-labels-in-hooks.yml @@ -0,0 +1,5 @@ +--- +title: Fix label serialization in issue and note hooks +merge_request: 29850 +author: +type: fixed diff --git a/lib/gitlab/data_builder/note.rb b/lib/gitlab/data_builder/note.rb index 16e62622ed4..9a2fe4e9abf 100644 --- a/lib/gitlab/data_builder/note.rb +++ b/lib/gitlab/data_builder/note.rb @@ -44,7 +44,7 @@ module Gitlab data[:commit] = build_data_for_commit(project, user, note) elsif note.for_issue? data[:issue] = note.noteable.hook_attrs - data[:issue][:labels] = note.noteable.labels(&:hook_attrs) + data[:issue][:labels] = note.noteable.labels.map(&:hook_attrs) elsif note.for_merge_request? data[:merge_request] = note.noteable.hook_attrs elsif note.for_snippet? diff --git a/lib/gitlab/hook_data/issue_builder.rb b/lib/gitlab/hook_data/issue_builder.rb index cfc9ebe4f92..22ca23cde2a 100644 --- a/lib/gitlab/hook_data/issue_builder.rb +++ b/lib/gitlab/hook_data/issue_builder.rb @@ -45,7 +45,7 @@ module Gitlab human_time_estimate: issue.human_time_estimate, assignee_ids: issue.assignee_ids, assignee_id: issue.assignee_ids.first, # This key is deprecated - labels: issue.labels + labels: issue.labels.map(&:hook_attrs) } issue.attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) diff --git a/spec/lib/gitlab/data_builder/note_spec.rb b/spec/lib/gitlab/data_builder/note_spec.rb index ed9a1e23529..1b5dd2538e0 100644 --- a/spec/lib/gitlab/data_builder/note_spec.rb +++ b/spec/lib/gitlab/data_builder/note_spec.rb @@ -38,9 +38,11 @@ describe Gitlab::DataBuilder::Note do end describe 'When asking for a note on issue' do + let(:label) { create(:label, project: project) } + let(:issue) do - create(:issue, created_at: fixed_time, updated_at: fixed_time, - project: project) + create(:labeled_issue, created_at: fixed_time, updated_at: fixed_time, + project: project, labels: [label]) end let(:note) do @@ -48,13 +50,16 @@ describe Gitlab::DataBuilder::Note do end it 'returns the note and issue-specific data' do + without_timestamps = lambda { |label| label.except('created_at', 'updated_at') } + hook_attrs = issue.reload.hook_attrs + expect(data).to have_key(:issue) - expect(data[:issue].except('updated_at')) - .to eq(issue.reload.hook_attrs.except('updated_at')) + expect(data[:issue].except('updated_at', 'labels')) + .to eq(hook_attrs.except('updated_at', 'labels')) expect(data[:issue]['updated_at']) - .to be >= issue.hook_attrs['updated_at'] - expect(data[:issue]['labels']) - .to eq(issue.hook_attrs['labels']) + .to be >= hook_attrs['updated_at'] + expect(data[:issue]['labels'].map(&without_timestamps)) + .to eq(hook_attrs['labels'].map(&without_timestamps)) end context 'with confidential issue' do diff --git a/spec/lib/gitlab/hook_data/issue_builder_spec.rb b/spec/lib/gitlab/hook_data/issue_builder_spec.rb index f066c0e3813..b06d05c1c7f 100644 --- a/spec/lib/gitlab/hook_data/issue_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/issue_builder_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::HookData::IssueBuilder do - set(:issue) { create(:issue) } + set(:label) { create(:label) } + set(:issue) { create(:labeled_issue, labels: [label], project: label.project) } let(:builder) { described_class.new(issue) } describe '#build' do @@ -39,6 +40,7 @@ describe Gitlab::HookData::IssueBuilder do expect(data).to include(:human_time_estimate) expect(data).to include(:human_total_time_spent) expect(data).to include(:assignee_ids) + expect(data).to include('labels' => [label.hook_attrs]) end context 'when the issue has an image in the description' do From 4189ffe2142e0a0969a3f28ec55e069c3b5abb9f Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Mon, 24 Jun 2019 11:51:34 +0200 Subject: [PATCH 2/2] Added labels_hook_attrs method Based on review comment fetching labels hook_attrs is now wrapped in an issue's model method. --- app/models/issue.rb | 4 ++++ lib/gitlab/data_builder/note.rb | 2 +- lib/gitlab/hook_data/issue_builder.rb | 2 +- spec/models/issue_spec.rb | 9 +++++++++ 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 6da6fbe55cb..30e29911758 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -254,6 +254,10 @@ class Issue < ApplicationRecord merge_requests_closing_issues.count end + def labels_hook_attrs + labels.map(&:hook_attrs) + end + private def ensure_metrics diff --git a/lib/gitlab/data_builder/note.rb b/lib/gitlab/data_builder/note.rb index 9a2fe4e9abf..2c4ef73a688 100644 --- a/lib/gitlab/data_builder/note.rb +++ b/lib/gitlab/data_builder/note.rb @@ -44,7 +44,7 @@ module Gitlab data[:commit] = build_data_for_commit(project, user, note) elsif note.for_issue? data[:issue] = note.noteable.hook_attrs - data[:issue][:labels] = note.noteable.labels.map(&:hook_attrs) + data[:issue][:labels] = note.noteable.labels_hook_attrs elsif note.for_merge_request? data[:merge_request] = note.noteable.hook_attrs elsif note.for_snippet? diff --git a/lib/gitlab/hook_data/issue_builder.rb b/lib/gitlab/hook_data/issue_builder.rb index 22ca23cde2a..e5f86ca02b5 100644 --- a/lib/gitlab/hook_data/issue_builder.rb +++ b/lib/gitlab/hook_data/issue_builder.rb @@ -45,7 +45,7 @@ module Gitlab human_time_estimate: issue.human_time_estimate, assignee_ids: issue.assignee_ids, assignee_id: issue.assignee_ids.first, # This key is deprecated - labels: issue.labels.map(&:hook_attrs) + labels: issue.labels_hook_attrs } issue.attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index a5c7e9db2a1..d5b016dc8f6 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -862,4 +862,13 @@ describe Issue do end end end + + describe "#labels_hook_attrs" do + let(:label) { create(:label) } + let(:issue) { create(:labeled_issue, labels: [label]) } + + it "returns a list of label hook attributes" do + expect(issue.labels_hook_attrs).to eq([label.hook_attrs]) + end + end end