diff options
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 |