summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2020-01-21 09:08:43 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2020-01-21 09:08:43 +0000
commitafe057a8ff8546f0032e439a9a200307fb6de86a (patch)
tree0f6699ac8a4863344f4f4db6500d33ce509e44c5
parentab0dd39a49e43f6beed9bdb6414a0f04bcf671b4 (diff)
downloadgitlab-ce-afe057a8ff8546f0032e439a9a200307fb6de86a.tar.gz
Add latest changes from gitlab-org/gitlab@master
-rw-r--r--app/policies/project_policy.rb1
-rw-r--r--app/services/error_tracking/base_service.rb8
-rw-r--r--app/services/error_tracking/issue_details_service.rb2
-rw-r--r--app/services/error_tracking/issue_latest_event_service.rb2
-rw-r--r--app/services/error_tracking/issue_update_service.rb58
-rw-r--r--app/services/error_tracking/list_issues_service.rb2
-rw-r--r--app/services/error_tracking/list_projects_service.rb2
-rw-r--r--app/services/system_note_service.rb6
-rw-r--r--changelogs/unreleased/39825-close-issue-after-error-resolve.yml5
-rw-r--r--doc/development/documentation/styleguide.md4
-rw-r--r--locale/gitlab.pot3
-rw-r--r--spec/controllers/projects/error_tracking_controller_spec.rb2
-rw-r--r--spec/fixtures/api/schemas/error_tracking/update_issue.json8
-rw-r--r--spec/services/error_tracking/issue_details_service_spec.rb19
-rw-r--r--spec/services/error_tracking/issue_latest_event_service_spec.rb19
-rw-r--r--spec/services/error_tracking/issue_update_service_spec.rb106
-rw-r--r--spec/services/error_tracking/list_issues_service_spec.rb18
-rw-r--r--spec/support/shared_contexts/sentry_error_tracking_shared_context.rb22
18 files changed, 224 insertions, 63 deletions
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index e38eef527be..2789152e175 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -222,6 +222,7 @@ class ProjectPolicy < BasePolicy
enable :read_deployment
enable :read_merge_request
enable :read_sentry_issue
+ enable :update_sentry_issue
enable :read_prometheus
end
diff --git a/app/services/error_tracking/base_service.rb b/app/services/error_tracking/base_service.rb
index 430d9952332..4fe01716704 100644
--- a/app/services/error_tracking/base_service.rb
+++ b/app/services/error_tracking/base_service.rb
@@ -7,7 +7,7 @@ module ErrorTracking
return unauthorized if unauthorized
begin
- response = fetch
+ response = perform
rescue Sentry::Client::Error => e
return error(e.message, :bad_request)
rescue Sentry::Client::MissingKeysError => e
@@ -22,7 +22,7 @@ module ErrorTracking
private
- def fetch
+ def perform
raise NotImplementedError,
"#{self.class} does not implement #{__method__}"
end
@@ -62,5 +62,9 @@ module ErrorTracking
def can_read?
can?(current_user, :read_sentry_issue, project)
end
+
+ def can_update?
+ can?(current_user, :update_sentry_issue, project)
+ end
end
end
diff --git a/app/services/error_tracking/issue_details_service.rb b/app/services/error_tracking/issue_details_service.rb
index 368cd4517fc..31fb6a9618c 100644
--- a/app/services/error_tracking/issue_details_service.rb
+++ b/app/services/error_tracking/issue_details_service.rb
@@ -4,7 +4,7 @@ module ErrorTracking
class IssueDetailsService < ErrorTracking::BaseService
private
- def fetch
+ def perform
project_error_tracking_setting.issue_details(issue_id: params[:issue_id])
end
diff --git a/app/services/error_tracking/issue_latest_event_service.rb b/app/services/error_tracking/issue_latest_event_service.rb
index b6ad8f8028b..dd6b7f8285f 100644
--- a/app/services/error_tracking/issue_latest_event_service.rb
+++ b/app/services/error_tracking/issue_latest_event_service.rb
@@ -4,7 +4,7 @@ module ErrorTracking
class IssueLatestEventService < ErrorTracking::BaseService
private
- def fetch
+ def perform
project_error_tracking_setting.issue_latest_event(issue_id: params[:issue_id])
end
diff --git a/app/services/error_tracking/issue_update_service.rb b/app/services/error_tracking/issue_update_service.rb
index e433b4a11f2..db754d54fef 100644
--- a/app/services/error_tracking/issue_update_service.rb
+++ b/app/services/error_tracking/issue_update_service.rb
@@ -4,6 +4,16 @@ module ErrorTracking
class IssueUpdateService < ErrorTracking::BaseService
private
+ def perform
+ response = fetch
+
+ unless parse_errors(response).present?
+ response[:closed_issue_iid] = update_related_issue&.iid
+ end
+
+ response
+ end
+
def fetch
project_error_tracking_setting.update_issue(
issue_id: params[:issue_id],
@@ -11,12 +21,58 @@ module ErrorTracking
)
end
+ def update_related_issue
+ issue = related_issue
+ return unless issue
+
+ close_and_create_note(issue)
+ end
+
+ def close_and_create_note(issue)
+ return unless resolving? && issue.opened?
+
+ processed_issue = close_issue(issue)
+ return unless processed_issue.reset.closed?
+
+ create_system_note(processed_issue)
+ processed_issue
+ end
+
+ def close_issue(issue)
+ Issues::CloseService
+ .new(project, current_user)
+ .execute(issue, system_note: false)
+ end
+
+ def create_system_note(issue)
+ SystemNoteService.close_after_error_tracking_resolve(issue, project, current_user)
+ end
+
+ def related_issue
+ SentryIssueFinder
+ .new(project, current_user: current_user)
+ .execute(params[:issue_id])
+ &.issue
+ end
+
+ def resolving?
+ update_params[:status] == 'resolved'
+ end
+
def update_params
params.except(:issue_id)
end
def parse_response(response)
- { updated: response[:updated].present? }
+ {
+ updated: response[:updated].present?,
+ closed_issue_iid: response[:closed_issue_iid]
+ }
+ end
+
+ def check_permissions
+ return error('Error Tracking is not enabled') unless enabled?
+ return error('Access denied', :unauthorized) unless can_update?
end
end
end
diff --git a/app/services/error_tracking/list_issues_service.rb b/app/services/error_tracking/list_issues_service.rb
index 132e9dfa7bd..d34ea8aa3b0 100644
--- a/app/services/error_tracking/list_issues_service.rb
+++ b/app/services/error_tracking/list_issues_service.rb
@@ -12,7 +12,7 @@ module ErrorTracking
private
- def fetch
+ def perform
project_error_tracking_setting.list_sentry_issues(
issue_status: issue_status,
limit: limit,
diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb
index 09a0b952e84..6523a66bbed 100644
--- a/app/services/error_tracking/list_projects_service.rb
+++ b/app/services/error_tracking/list_projects_service.rb
@@ -12,7 +12,7 @@ module ErrorTracking
private
- def fetch
+ def perform
project_error_tracking_setting.list_sentry_projects
end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 38e0a7d34ad..033c80fd8ed 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -99,6 +99,12 @@ module SystemNoteService
::SystemNotes::TimeTrackingService.new(noteable: noteable, project: project, author: author).change_time_spent
end
+ def close_after_error_tracking_resolve(issue, project, author)
+ body = _('resolved the corresponding error and closed the issue.')
+
+ create_note(NoteSummary.new(issue, project, author, body, action: 'closed'))
+ end
+
def change_status(noteable, project, author, status, source = nil)
::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_status(status, source)
end
diff --git a/changelogs/unreleased/39825-close-issue-after-error-resolve.yml b/changelogs/unreleased/39825-close-issue-after-error-resolve.yml
new file mode 100644
index 00000000000..9a59c812290
--- /dev/null
+++ b/changelogs/unreleased/39825-close-issue-after-error-resolve.yml
@@ -0,0 +1,5 @@
+---
+title: Close Issue when resolving corresponding Sentry error
+merge_request: 22744
+author:
+type: added
diff --git a/doc/development/documentation/styleguide.md b/doc/development/documentation/styleguide.md
index 173ca324f59..c7c59a72e9c 100644
--- a/doc/development/documentation/styleguide.md
+++ b/doc/development/documentation/styleguide.md
@@ -275,10 +275,10 @@ Do not include the same information in multiple places. [Link to a SSOT instead.
- Do not use "may" and "might" interchangeably:
- Use "might" to indicate the probability of something occurring. "If you skip this step, the import process might fail."
- Use "may" to indicate giving permission for someone to do something, or consider using "can" instead. "You may select either option on this screen." Or, "you can select either option on this screen."
-- We recommend avoiding Latin abbreviations, such as "e.g.," "i.e.," or "etc.,"
+- We discourage use of Latin abbreviations, such as "e.g.," "i.e.," or "etc.,"
as even native users of English might misunderstand them.
- Instead of "i.e.", use "that is."
- - Instead of "e.g.", use "for example."
+ - Instead of "e.g.", use "for example," "such as," "for instance," or "like."
- Instead of "etc.", either use "and so on" or consider editing it out, since it can be vague.
## Text
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 942c81aafd5..f905986bf0d 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -22788,6 +22788,9 @@ msgstr[1] ""
msgid "reset it."
msgstr ""
+msgid "resolved the corresponding error and closed the issue."
+msgstr ""
+
msgid "score"
msgstr ""
diff --git a/spec/controllers/projects/error_tracking_controller_spec.rb b/spec/controllers/projects/error_tracking_controller_spec.rb
index 588c4b05528..22826938de2 100644
--- a/spec/controllers/projects/error_tracking_controller_spec.rb
+++ b/spec/controllers/projects/error_tracking_controller_spec.rb
@@ -301,7 +301,7 @@ describe Projects::ErrorTrackingController do
context 'update result is successful' do
before do
expect(issue_update_service).to receive(:execute)
- .and_return(status: :success, updated: true)
+ .and_return(status: :success, updated: true, closed_issue_iid: 1234)
update_issue
end
diff --git a/spec/fixtures/api/schemas/error_tracking/update_issue.json b/spec/fixtures/api/schemas/error_tracking/update_issue.json
index 72514ce647d..75f2c1152d9 100644
--- a/spec/fixtures/api/schemas/error_tracking/update_issue.json
+++ b/spec/fixtures/api/schemas/error_tracking/update_issue.json
@@ -6,9 +6,15 @@
"properties" : {
"result": {
"type": "object",
+ "required" : [
+ "status",
+ "updated",
+ "closed_issue_iid"
+ ],
"properties": {
"status": { "type": "string" },
- "updated": { "type": "boolean" }
+ "updated": { "type": "boolean" },
+ "closed_issue_iid": { "type": ["integer", "null"] }
}
}
},
diff --git a/spec/services/error_tracking/issue_details_service_spec.rb b/spec/services/error_tracking/issue_details_service_spec.rb
index 4d5505bb5a9..26bb3b44126 100644
--- a/spec/services/error_tracking/issue_details_service_spec.rb
+++ b/spec/services/error_tracking/issue_details_service_spec.rb
@@ -3,24 +3,7 @@
require 'spec_helper'
describe ErrorTracking::IssueDetailsService do
- let_it_be(:user) { create(:user) }
- let_it_be(:project) { create(:project) }
-
- let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
- let(:token) { 'test-token' }
- let(:result) { subject.execute }
-
- let(:error_tracking_setting) do
- create(:project_error_tracking_setting, api_url: sentry_url, token: token, project: project)
- end
-
- subject { described_class.new(project, user) }
-
- before do
- expect(project).to receive(:error_tracking_setting).at_least(:once).and_return(error_tracking_setting)
-
- project.add_reporter(user)
- end
+ include_context 'sentry error tracking context'
describe '#execute' do
context 'with authorized user' do
diff --git a/spec/services/error_tracking/issue_latest_event_service_spec.rb b/spec/services/error_tracking/issue_latest_event_service_spec.rb
index cda15042814..7f53eabd717 100644
--- a/spec/services/error_tracking/issue_latest_event_service_spec.rb
+++ b/spec/services/error_tracking/issue_latest_event_service_spec.rb
@@ -3,24 +3,7 @@
require 'spec_helper'
describe ErrorTracking::IssueLatestEventService do
- let_it_be(:user) { create(:user) }
- let_it_be(:project) { create(:project) }
-
- let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
- let(:token) { 'test-token' }
- let(:result) { subject.execute }
-
- let(:error_tracking_setting) do
- create(:project_error_tracking_setting, api_url: sentry_url, token: token, project: project)
- end
-
- subject { described_class.new(project, user) }
-
- before do
- expect(project).to receive(:error_tracking_setting).at_least(:once).and_return(error_tracking_setting)
-
- project.add_reporter(user)
- end
+ include_context 'sentry error tracking context'
describe '#execute' do
context 'with authorized user' do
diff --git a/spec/services/error_tracking/issue_update_service_spec.rb b/spec/services/error_tracking/issue_update_service_spec.rb
new file mode 100644
index 00000000000..ad1dafe6ccc
--- /dev/null
+++ b/spec/services/error_tracking/issue_update_service_spec.rb
@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ErrorTracking::IssueUpdateService do
+ include_context 'sentry error tracking context'
+
+ let(:arguments) { { issue_id: 1234, status: 'resolved' } }
+
+ subject { described_class.new(project, user, arguments) }
+
+ shared_examples 'does not perform close issue flow' do
+ it 'does not call the close issue service' do
+ expect(issue_close_service)
+ .not_to receive(:execute)
+ end
+
+ it 'does not create system note' do
+ expect(SystemNoteService).not_to receive(:close_after_error_tracking_resolve)
+ end
+ end
+
+ describe '#execute' do
+ context 'with authorized user' do
+ context 'when update_issue returns success' do
+ let(:update_issue_response) { { updated: true } }
+
+ before do
+ expect(error_tracking_setting)
+ .to receive(:update_issue).and_return(update_issue_response)
+ end
+
+ it 'returns the response' do
+ expect(result).to eq(update_issue_response.merge(status: :success, closed_issue_iid: nil))
+ end
+
+ it 'updates any related issue' do
+ expect(subject).to receive(:update_related_issue)
+
+ result
+ end
+
+ context 'related issue and resolving' do
+ let(:issue) { create(:issue, project: project) }
+ let(:sentry_issue) { create(:sentry_issue, issue: issue) }
+ let(:arguments) { { issue_id: sentry_issue.sentry_issue_identifier, status: 'resolved' } }
+
+ let(:issue_close_service) { spy(:issue_close_service) }
+
+ before do
+ allow_any_instance_of(SentryIssueFinder)
+ .to receive(:execute)
+ .and_return(sentry_issue)
+
+ allow(Issues::CloseService)
+ .to receive(:new)
+ .and_return(issue_close_service)
+ end
+
+ after do
+ result
+ end
+
+ it 'closes the issue' do
+ expect(issue_close_service)
+ .to receive(:execute)
+ .with(issue, system_note: false)
+ .and_return(issue)
+ end
+
+ it 'creates a system note' do
+ expect(SystemNoteService).to receive(:close_after_error_tracking_resolve)
+ end
+
+ it 'returns a response with closed issue' do
+ closed_issue = create(:issue, :closed, project: project)
+
+ expect(issue_close_service)
+ .to receive(:execute)
+ .with(issue, system_note: false)
+ .and_return(closed_issue)
+
+ expect(result).to eq(status: :success, updated: true, closed_issue_iid: closed_issue.iid)
+ end
+
+ context 'issue is already closed' do
+ let(:issue) { create(:issue, :closed, project: project) }
+
+ include_examples 'does not perform close issue flow'
+ end
+
+ context 'status is not resolving' do
+ let(:arguments) { { issue_id: sentry_issue.sentry_issue_identifier, status: 'ignored' } }
+
+ include_examples 'does not perform close issue flow'
+ end
+ end
+ end
+
+ include_examples 'error tracking service sentry error handling', :update_issue
+ end
+
+ include_examples 'error tracking service unauthorized user'
+ include_examples 'error tracking service disabled'
+ end
+end
diff --git a/spec/services/error_tracking/list_issues_service_spec.rb b/spec/services/error_tracking/list_issues_service_spec.rb
index ecb6bcc541b..1e39146fd18 100644
--- a/spec/services/error_tracking/list_issues_service_spec.rb
+++ b/spec/services/error_tracking/list_issues_service_spec.rb
@@ -3,8 +3,8 @@
require 'spec_helper'
describe ErrorTracking::ListIssuesService do
- let_it_be(:user) { create(:user) }
- let_it_be(:project) { create(:project) }
+ include_context 'sentry error tracking context'
+
let(:params) { { search_term: 'something', sort: 'last_seen', cursor: 'some-cursor' } }
let(:list_sentry_issues_args) do
{
@@ -16,22 +16,8 @@ describe ErrorTracking::ListIssuesService do
}
end
- let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
- let(:token) { 'test-token' }
- let(:result) { subject.execute }
-
- let(:error_tracking_setting) do
- create(:project_error_tracking_setting, api_url: sentry_url, token: token, project: project)
- end
-
subject { described_class.new(project, user, params) }
- before do
- expect(project).to receive(:error_tracking_setting).at_least(:once).and_return(error_tracking_setting)
-
- project.add_reporter(user)
- end
-
describe '#execute' do
context 'with authorized user' do
context 'when list_sentry_issues returns issues' do
diff --git a/spec/support/shared_contexts/sentry_error_tracking_shared_context.rb b/spec/support/shared_contexts/sentry_error_tracking_shared_context.rb
new file mode 100644
index 00000000000..ba729f21e58
--- /dev/null
+++ b/spec/support/shared_contexts/sentry_error_tracking_shared_context.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+shared_context 'sentry error tracking context' do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:project) { create(:project) }
+
+ let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
+ let(:token) { 'test-token' }
+ let(:result) { subject.execute }
+
+ subject { described_class.new(project, user) }
+
+ let(:error_tracking_setting) do
+ create(:project_error_tracking_setting, api_url: sentry_url, token: token, project: project)
+ end
+
+ before do
+ expect(project).to receive(:error_tracking_setting).at_least(:once).and_return(error_tracking_setting)
+
+ project.add_reporter(user)
+ end
+end