From 78722e8dbea10e0c661487db66fdb8934bec117e Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 20 Nov 2019 13:41:21 +0100 Subject: [PATCH] present error if title is missing --- lib/api/utilities/endpoints/modify.rb | 12 +- .../bcf/api/v2_1/endpoints/create.rb | 4 + .../bcf/api/v2_1/errors/error_mapper.rb | 60 ++++++ .../bcf/api/v2_1/topics/single_representer.rb | 8 + .../app/services/bcf/issues/create_service.rb | 12 +- .../issues/transform_attributes_service.rb | 21 ++- .../single_representer_rendering_spec.rb | 9 + .../requests/api/bcf/v2_1/shared_responses.rb | 2 +- .../requests/api/bcf/v2_1/topics_api_spec.rb | 57 ++++++ .../bcf/issues/create_service_spec.rb | 173 ++++++++++++++++++ 10 files changed, 339 insertions(+), 19 deletions(-) create mode 100644 modules/bcf/app/representers/bcf/api/v2_1/errors/error_mapper.rb create mode 100644 modules/bcf/spec/services/bcf/issues/create_service_spec.rb diff --git a/lib/api/utilities/endpoints/modify.rb b/lib/api/utilities/endpoints/modify.rb index 2d410854ad..ce6f1a3fb3 100644 --- a/lib/api/utilities/endpoints/modify.rb +++ b/lib/api/utilities/endpoints/modify.rb @@ -43,12 +43,16 @@ module API end def present_error(call) - errors = call.errors - errors = merge_dependent_errors call if errors.empty? + api_errors = [::API::Errors::ErrorBase.create_and_merge_errors(postprocess_errors(call))] - api_errors = [::API::Errors::ErrorBase.create_and_merge_errors(errors)] + fail(::API::Errors::MultipleErrors + .create_if_many(api_errors)) + end - fail ::API::Errors::MultipleErrors.create_if_many(api_errors) + def postprocess_errors(call) + errors = call.errors + errors = merge_dependent_errors call if errors.empty? + errors end def merge_dependent_errors(call) diff --git a/modules/bcf/app/controllers/bcf/api/v2_1/endpoints/create.rb b/modules/bcf/app/controllers/bcf/api/v2_1/endpoints/create.rb index f8e346cc0d..4aab66915f 100644 --- a/modules/bcf/app/controllers/bcf/api/v2_1/endpoints/create.rb +++ b/modules/bcf/app/controllers/bcf/api/v2_1/endpoints/create.rb @@ -37,6 +37,10 @@ module Bcf::API::V2_1::Endpoints .new(call.result) end + def postprocess_errors(call) + Bcf::API::V2_1::Errors::ErrorMapper.map(super) + end + private def deduce_process_service diff --git a/modules/bcf/app/representers/bcf/api/v2_1/errors/error_mapper.rb b/modules/bcf/app/representers/bcf/api/v2_1/errors/error_mapper.rb new file mode 100644 index 0000000000..c04ac96859 --- /dev/null +++ b/modules/bcf/app/representers/bcf/api/v2_1/errors/error_mapper.rb @@ -0,0 +1,60 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2018 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2017 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +module Bcf::API::V2_1::Errors + class ErrorMapper + extend ActiveModel::Naming + extend ActiveModel::Translation + + def read_attribute_for_validation(_attr) + nil + end + + def self.lookup_ancestors + [::Bcf::Issue] + end + + def self.map(original_errors) + mapped_errors = ActiveModel::Errors.new(new) + + original_errors.send(:error_symbols).each do |key, errors| + errors.map(&:first).each do |error| + mapped_errors.add(error_key_mapper(key), error) + end + end + + mapped_errors + end + + def self.error_key_mapper(key) + { subject: :title }[key] || key + end + end +end diff --git a/modules/bcf/app/representers/bcf/api/v2_1/topics/single_representer.rb b/modules/bcf/app/representers/bcf/api/v2_1/topics/single_representer.rb index 74745eb836..18353e0d2e 100644 --- a/modules/bcf/app/representers/bcf/api/v2_1/topics/single_representer.rb +++ b/modules/bcf/app/representers/bcf/api/v2_1/topics/single_representer.rb @@ -51,6 +51,14 @@ module Bcf::API::V2_1 .name } + property :priority, + as: :priority, + getter: ->(*) { + work_package + .priority + .name + } + property :reference_links, getter: ->(decorator:, **) { [decorator.api_v3_paths.work_package(work_package.id)] diff --git a/modules/bcf/app/services/bcf/issues/create_service.rb b/modules/bcf/app/services/bcf/issues/create_service.rb index 8eb75ae252..cd5abb21a3 100644 --- a/modules/bcf/app/services/bcf/issues/create_service.rb +++ b/modules/bcf/app/services/bcf/issues/create_service.rb @@ -37,11 +37,15 @@ module Bcf::Issues .new(user: user) .call(params) - issue_params = { - work_package: wp_call.result - }.merge(params.slice(:stage, :labels, :index)) + if wp_call.success? + issue_params = { + work_package: wp_call.result + }.merge(params.slice(:stage, :labels, :index)) - super(issue_params) + super(issue_params) + else + wp_call + end end end end diff --git a/modules/bcf/app/services/bcf/issues/transform_attributes_service.rb b/modules/bcf/app/services/bcf/issues/transform_attributes_service.rb index 5da7e081b4..ec0b864626 100644 --- a/modules/bcf/app/services/bcf/issues/transform_attributes_service.rb +++ b/modules/bcf/app/services/bcf/issues/transform_attributes_service.rb @@ -40,7 +40,11 @@ module Bcf::Issues ## # BCF issues might have empty titles. OP needs one. def title(attributes) - attributes[:title] || '(Imported BCF issue contained no title)' + if attributes[:title] + attributes[:title] + elsif attributes[:import_options] + '(Imported BCF issue contained no title)' + end end def author(project, attributes) @@ -67,15 +71,11 @@ module Bcf::Issues return unless import_options - return ::Type.default&.first if import_options[:unknown_types_action] == 'default' - - if import_options[:unknown_types_action] == 'chose' && - import_options[:unknown_types_chose_ids].any? + if import_options[:unknown_types_action] == 'default' + ::Type.default&.first + elsif import_options[:unknown_types_action] == 'chose' && + import_options[:unknown_types_chose_ids].any? ::Type.find_by(id: import_options[:unknown_types_chose_ids].first) - else - ServiceResult.new success: false, - errors: issue.errors, - result: issue end end @@ -113,7 +113,8 @@ module Bcf::Issues if import_options[:unknown_priorities_action] == 'use_default' # NOP The 'use_default' case gets already covered by OP. - elsif import_options[:unknown_priorities_action] == 'chose' && import_options[:unknown_priorities_chose_ids].any? + elsif import_options[:unknown_priorities_action] == 'chose' && + import_options[:unknown_priorities_chose_ids].any? ::IssuePriority.find_by(id: import_options[:unknown_priorities_chose_ids].first) end end diff --git a/modules/bcf/spec/representers/bcf/api/v2_1/topics/single_representer_rendering_spec.rb b/modules/bcf/spec/representers/bcf/api/v2_1/topics/single_representer_rendering_spec.rb index 51023a0d4a..a097ac48cb 100644 --- a/modules/bcf/spec/representers/bcf/api/v2_1/topics/single_representer_rendering_spec.rb +++ b/modules/bcf/spec/representers/bcf/api/v2_1/topics/single_representer_rendering_spec.rb @@ -41,11 +41,13 @@ describe Bcf::API::V2_1::Topics::SingleRepresenter, 'rendering' do let(:journals) { [first_journal, last_journal] } let(:type) { FactoryBot.build_stubbed(:type) } let(:status) { FactoryBot.build_stubbed(:status) } + let(:priority) { FactoryBot.build_stubbed(:priority) } let(:work_package) do FactoryBot.build_stubbed(:stubbed_work_package, assigned_to: assignee, due_date: Date.today, status: status, + priority: priority, type: type).tap do |wp| allow(wp) .to receive(:journals) @@ -80,6 +82,13 @@ describe Bcf::API::V2_1::Topics::SingleRepresenter, 'rendering' do end end + context 'priority' do + it_behaves_like 'attribute' do + let(:value) { priority.name } + let(:path) { 'priority' } + end + end + context 'reference_links' do it_behaves_like 'attribute' do let(:value) { [api_v3_paths.work_package(work_package.id)] } diff --git a/modules/bcf/spec/requests/api/bcf/v2_1/shared_responses.rb b/modules/bcf/spec/requests/api/bcf/v2_1/shared_responses.rb index 5eabcdfb2d..e0f98f0258 100644 --- a/modules/bcf/spec/requests/api/bcf/v2_1/shared_responses.rb +++ b/modules/bcf/spec/requests/api/bcf/v2_1/shared_responses.rb @@ -86,7 +86,7 @@ shared_examples_for 'bcf api not allowed response' do end shared_examples_for 'bcf api unprocessable response' do - it 'responds 403 NOT ALLOWED' do + it 'responds 422' do expect(subject.status) .to eql 422 end diff --git a/modules/bcf/spec/requests/api/bcf/v2_1/topics_api_spec.rb b/modules/bcf/spec/requests/api/bcf/v2_1/topics_api_spec.rb index 388adfdf0b..10062a60c2 100644 --- a/modules/bcf/spec/requests/api/bcf/v2_1/topics_api_spec.rb +++ b/modules/bcf/spec/requests/api/bcf/v2_1/topics_api_spec.rb @@ -184,6 +184,12 @@ describe 'BCF 2.1 topics resource', type: :request, content_type: :json, with_ma let!(:default_status) do FactoryBot.create(:default_status) end + let!(:default_type) do + FactoryBot.create(:type, is_default: true) + end + let!(:standard_type) do + FactoryBot.create(:type_standard) + end let!(:priority) do FactoryBot.create(:priority) end @@ -198,6 +204,7 @@ describe 'BCF 2.1 topics resource', type: :request, content_type: :json, with_ma { topic_type: type.name, topic_status: status.name, + priority: priority.name, title: 'BCF topic 101', labels: labels, stage: stage, @@ -223,6 +230,7 @@ describe 'BCF 2.1 topics resource', type: :request, content_type: :json, with_ma guid: issue&.uuid, topic_type: type.name, topic_status: status.name, + priority: priority.name, title: 'BCF topic 101', labels: labels, index: index, @@ -237,9 +245,58 @@ describe 'BCF 2.1 topics resource', type: :request, content_type: :json, with_ma modified_author: edit_member_user.mail, modified_date: work_package.updated_at.iso8601, description: description + } + end + end + + context 'with minimal parameters' do + let(:params) do + { + title: 'BCF topic 101' + } + end + + it_behaves_like 'bcf api successful response' do + let(:expected_status) { 201 } + let(:expected_body) do + issue = Bcf::Issue.last + work_package = WorkPackage.last + { + guid: issue&.uuid, + topic_type: standard_type.name, + topic_status: default_status.name, + priority: default_priority.name, + title: 'BCF topic 101', + labels: [], + index: nil, + reference_links: [ + api_v3_paths.work_package(work_package.id) + ], + assigned_to: nil, + due_date: nil, + stage: nil, + creation_author: edit_member_user.mail, + creation_date: work_package.created_at.iso8601, + modified_author: edit_member_user.mail, + modified_date: work_package.updated_at.iso8601, + description: nil + } + end + end + end + + context 'without a title' do + let(:params) do + { } end + + it_behaves_like 'bcf api unprocessable response' do + let(:message) do + "Title can't be blank" + end + end end end end diff --git a/modules/bcf/spec/services/bcf/issues/create_service_spec.rb b/modules/bcf/spec/services/bcf/issues/create_service_spec.rb new file mode 100644 index 0000000000..c30f707943 --- /dev/null +++ b/modules/bcf/spec/services/bcf/issues/create_service_spec.rb @@ -0,0 +1,173 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2018 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2017 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe Bcf::Issues::CreateService, type: :model do + let(:user) { FactoryBot.build_stubbed(:user) } + let(:contract_class) do + double('contract_class', '<=': true) + end + let(:issue_valid) { true } + let(:instance) do + described_class.new(user: user, + contract_class: contract_class) + end + let(:call_attributes) { { subject: 'Some name' } } + let(:set_attributes_success) do + true + end + let(:set_attributes_errors) do + double('set_attributes_errors') + end + let(:set_attributes_result) do + ServiceResult.new result: created_issue, + success: set_attributes_success, + errors: set_attributes_errors + end + let!(:created_work_package) do + FactoryBot.build_stubbed(:work_package) + end + let(:wp_create_errors) do + double('wp_create_errors') + end + let(:wp_create_result) do + ServiceResult.new result: created_work_package, + success: true, + errors: wp_create_errors + end + let!(:wp_create_service) do + wp_service = double('wp create service') + + allow(WorkPackages::CreateService) + .to receive(:new) + .with(user: user) + .and_return(wp_service) + + allow(wp_service) + .to receive(:call) + .and_return(wp_create_result) + + wp_service + end + let!(:created_issue) do + issue = FactoryBot.build_stubbed(:bcf_issue) + + allow(Bcf::Issue) + .to receive(:new) + .and_return(issue) + + allow(issue) + .to receive(:save) + .and_return(issue_valid) + + issue + end + let!(:set_attributes_service) do + service = double('set_attributes_service_instance') + + allow(Bcf::Issues::SetAttributesService) + .to receive(:new) + .with(user: user, + model: created_issue, + contract_class: contract_class) + .and_return(service) + + allow(service) + .to receive(:call) + .and_return(set_attributes_result) + end + + describe '#call' do + subject { instance.call(call_attributes) } + + it 'is successful' do + expect(subject.success?).to be_truthy + end + + it 'returns the result of the SetAttributesService' do + expect(subject) + .to eql set_attributes_result + end + + it 'persists the issue' do + expect(created_issue) + .to receive(:save) + .and_return(issue_valid) + + subject + end + + it 'creates a issue' do + expect(subject.result) + .to eql created_issue + end + + context 'if the SetAttributeService is unsuccessful' do + let(:set_attributes_success) { false } + + it 'is unsuccessful' do + expect(subject.success?).to be_falsey + end + + it 'returns the result of the SetAttributesService' do + expect(subject) + .to eql set_attributes_result + end + + it 'does not persist the changes' do + expect(created_issue) + .to_not receive(:save) + + subject + end + + it "exposes the contract's errors" do + subject + + expect(subject.errors).to eql set_attributes_errors + end + end + + context 'when the issue is invalid' do + let(:issue_valid) { false } + + it 'is unsuccessful' do + expect(subject.success?).to be_falsey + end + + it "exposes the issue's errors" do + subject + + expect(subject.errors).to eql created_issue.errors + end + end + end +end