From 8ccbf8c2477b8cd6f1badd1255782147f6b5117e Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 2 Feb 2015 13:54:16 +0100 Subject: [PATCH] allow PATCHing start and due date - adding new errors - adding more tests - extending the specification for error conditions --- config/locales/de.yml | 1 + config/locales/en.yml | 1 + doc/apiv3-documentation.apib | 2 + lib/api/errors/property_format_error.rb | 39 ++++++ lib/api/v3/errors/error_representer.rb | 2 + .../form/work_package_payload_representer.rb | 42 +++++- .../v3/work_packages/work_package_contract.rb | 2 + spec/factories/work_package_factory.rb | 2 + spec/lib/api/v3/support/date_time_examples.rb | 53 ++++++++ .../work_package_payload_representer_spec.rb | 121 +++++++++++++++++- .../work_package_representer_spec.rb | 48 +++---- 11 files changed, 277 insertions(+), 36 deletions(-) create mode 100644 lib/api/errors/property_format_error.rb create mode 100644 spec/lib/api/v3/support/date_time_examples.rb diff --git a/config/locales/de.yml b/config/locales/de.yml index ca6433fc72..c97e6dff14 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -1674,6 +1674,7 @@ de: multiple_errors: "Die Integrität mehrerer Eigenschaften wurde verletzt." parse_error: "Die Abfrage enthielt kein valides JSON." writing_read_only_attributes: "Nur-lesbare Eigeschaften dürfen nicht geschrieben werden." + invalid_format: "Ungültiges Format für Eigenschaft '%{property}': Ein Format der Art '%{expected_format}' wurde erwartet, aber '%{actual}' wurde übergeben." render: context_not_found: "Es wurde kein Kontext gefunden." unsupported_context: "Der Kontext wird nicht unterstützt." diff --git a/config/locales/en.yml b/config/locales/en.yml index cd6052f5c8..5e3013b105 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1665,6 +1665,7 @@ en: multiple_errors: "Multiple fields violated their constraints." parse_error: "The request body was neither empty, nor did it contain a single JSON object." writing_read_only_attributes: "You must not write a read-only attribute." + invalid_format: "Invalid format for property '%{property}': Expected format like '%{expected_format}', but got '%{actual}'." render: context_not_found: "No context found." unsupported_context: "Unsupported context found." diff --git a/doc/apiv3-documentation.apib b/doc/apiv3-documentation.apib index 6889e05259..16c0292b0e 100644 --- a/doc/apiv3-documentation.apib +++ b/doc/apiv3-documentation.apib @@ -180,6 +180,7 @@ embedded errors or simply state that multiple errors occured. * `urn:openproject-org:api:v3:errors:PropertyIsReadOnly` (**HTTP 422**) - The client tried to set or change a property that is not writable * `urn:openproject-org:api:v3:errors:PropertyConstraintViolation` (**HTTP 422**) - The client tried to set a property to an invalid value * `urn:openproject-org:api:v3:errors:PropertyValueNotAvailableAnymore` (**HTTP 422**) - An unchanged property needs to be changed, because other changes to the resource make it unavailable +* `urn:openproject-org:api:v3:errors:PropertyFormatError` (**HTTP 422**) - A property value was provided in a format that the server does not understand or accept * `urn:openproject-org:api:v3:errors:InternalServerError` (**HTTP 500**) - Default for HTTP 500 when no further information is available * `urn:openproject-org:api:v3:errors:MultipleErrors` - Multiple errors occured. See the embedded `errors` array for more details. @@ -2644,6 +2645,7 @@ The value of `lockVersion` is used to implement [optimistic locking](http://en.w Returned if: * the client tries to modify a read-only property * a constraint for a property was violated + * a property was provided in an unreadable format + Body diff --git a/lib/api/errors/property_format_error.rb b/lib/api/errors/property_format_error.rb new file mode 100644 index 0000000000..ce648837a6 --- /dev/null +++ b/lib/api/errors/property_format_error.rb @@ -0,0 +1,39 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 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-2013 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 doc/COPYRIGHT.rdoc for more details. +#++ + +module API + module Errors + class PropertyFormatError < ErrorBase + def initialize(property, expected_format, actual_value) + message = I18n.t('api_v3.errors.invalid_format', property: property, expected_format: expected_format, actual: actual_value) + super 422, message + end + end + end +end diff --git a/lib/api/v3/errors/error_representer.rb b/lib/api/v3/errors/error_representer.rb index 7741e3ac34..911b8e9f2e 100644 --- a/lib/api/v3/errors/error_representer.rb +++ b/lib/api/v3/errors/error_representer.rb @@ -66,6 +66,8 @@ module API 'urn:openproject-org:api:v3:errors:MissingPermission' when ::API::Errors::UnwritableProperty 'urn:openproject-org:api:v3:errors:PropertyIsReadOnly' + when ::API::Errors::PropertyFormatError + 'urn:openproject-org:api:v3:errors:PropertyFormatError' when ::API::Errors::Validation 'urn:openproject-org:api:v3:errors:PropertyConstraintViolation' when ::API::Errors::InvalidRenderContext diff --git a/lib/api/v3/work_packages/form/work_package_payload_representer.rb b/lib/api/v3/work_packages/form/work_package_payload_representer.rb index deed02002f..9bd80b80f6 100644 --- a/lib/api/v3/work_packages/form/work_package_payload_representer.rb +++ b/lib/api/v3/work_packages/form/work_package_payload_representer.rb @@ -80,12 +80,25 @@ module API property :project_id, getter: -> (*) { nil }, render_nil: false + property :start_date, - getter: -> (*) { nil }, - render_nil: false + exec_context: :decorator, + getter: -> (*) { + represented.start_date.to_date.iso8601 unless represented.start_date.nil? + }, + setter: -> (value, *) { + represented.start_date = parse_date_only(value, 'startDate') + }, + render_nil: true property :due_date, - getter: -> (*) { nil }, - render_nil: false + exec_context: :decorator, + getter: -> (*) { + represented.due_date.to_date.iso8601 unless represented.due_date.nil? + }, + setter: -> (value, *) { + represented.due_date = parse_date_only(value, 'dueDate') + }, + render_nil: true property :version_id, getter: -> (*) { nil }, setter: -> (value, *) { self.fixed_version_id = value }, @@ -108,6 +121,27 @@ module API def description_renderer ::API::Utilities::Renderer::TextileRenderer.new(represented.description, represented) end + + def parse_date_only(value, property_name) + return nil if value.nil? + + begin + date_and_time = DateTime.iso8601(value) + rescue ArgumentError + raise API::Errors::PropertyFormatError.new(property_name, 'YYYY-MM-DD', value) + end + + date_only = date_and_time.to_date + + # we only want to accept "timeless" dates, e.g. "2015-01-31", + # but not "2015-01-31T01:02:03". + # However Date.iso8601 is too generous and would accept that + unless date_and_time == date_only + raise API::Errors::PropertyFormatError.new(property_name, 'YYYY-MM-DD', value) + end + + date_only + end end end end diff --git a/lib/api/v3/work_packages/work_package_contract.rb b/lib/api/v3/work_packages/work_package_contract.rb index 858ed6a427..3206e64111 100644 --- a/lib/api/v3/work_packages/work_package_contract.rb +++ b/lib/api/v3/work_packages/work_package_contract.rb @@ -39,6 +39,8 @@ module API 'subject', 'parent_id', 'description', + 'start_date', + 'due_date', 'status_id', 'assigned_to_id', 'responsible_id', diff --git a/spec/factories/work_package_factory.rb b/spec/factories/work_package_factory.rb index a3e06f9077..9f8d122c9f 100644 --- a/spec/factories/work_package_factory.rb +++ b/spec/factories/work_package_factory.rb @@ -38,6 +38,8 @@ FactoryGirl.define do sequence(:subject) { |n| "WorkPackage No. #{n}" } description { |i| "Description for '#{i.subject}'" } author factory: :user + created_at { DateTime.now } + updated_at { DateTime.now } callback(:after_build) do |work_package, evaluator| work_package.type = work_package.project.types.first unless work_package.type diff --git a/spec/lib/api/v3/support/date_time_examples.rb b/spec/lib/api/v3/support/date_time_examples.rb new file mode 100644 index 0000000000..8a82b53a4f --- /dev/null +++ b/spec/lib/api/v3/support/date_time_examples.rb @@ -0,0 +1,53 @@ +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 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-2013 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 doc/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +shared_examples_for 'has ISO 8601 date only' do + let(:iso_date_only_string) { '%d-%02d-%02d' % [ date.year, date.month, date.day ] } + + it 'exists' do + is_expected.to have_json_path(json_path) + end + + it 'indicates date only as ISO 8601' do + is_expected.to be_json_eql(iso_date_only_string.to_json).at_path(json_path) + end +end + +shared_examples_for 'has UTC ISO 8601 date and time' do + let(:iso_string) { date.utc.iso8601 } + + it 'exists' do + is_expected.to have_json_path(json_path) + end + + it 'indicates date and time as ISO 8601' do + is_expected.to be_json_eql(iso_string.to_json).at_path(json_path) + end +end diff --git a/spec/lib/api/v3/work_packages/form/work_package_payload_representer_spec.rb b/spec/lib/api/v3/work_packages/form/work_package_payload_representer_spec.rb index cd83ac80b6..e6637a806e 100644 --- a/spec/lib/api/v3/work_packages/form/work_package_payload_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/form/work_package_payload_representer_spec.rb @@ -31,6 +31,8 @@ require 'spec_helper' describe ::API::V3::WorkPackages::Form::WorkPackagePayloadRepresenter do let(:work_package) { FactoryGirl.build(:work_package, + start_date: Date.today.to_datetime, + due_date: Date.today.to_datetime, created_at: DateTime.now, updated_at: DateTime.now) } @@ -59,6 +61,36 @@ describe ::API::V3::WorkPackages::Form::WorkPackagePayloadRepresenter do it { is_expected.to be_json_eql(work_package.lock_version.to_json).at_path('lockVersion') } end + + describe 'startDate' do + it_behaves_like 'has ISO 8601 date only' do + let(:date) { work_package.start_date } + let(:json_path) { 'startDate' } + end + + context 'no start date' do + let(:work_package) { FactoryGirl.build(:work_package, start_date: nil) } + + it 'renders as null' do + is_expected.to be_json_eql(nil.to_json).at_path('startDate') + end + end + end + + describe 'dueDate' do + it_behaves_like 'has ISO 8601 date only' do + let(:date) { work_package.due_date } + let(:json_path) { 'dueDate' } + end + + context 'no due date' do + let(:work_package) { FactoryGirl.build(:work_package, due_date: nil) } + + it 'renders as null' do + is_expected.to be_json_eql(nil.to_json).at_path('dueDate') + end + end + end end describe '_links' do @@ -133,14 +165,95 @@ describe ::API::V3::WorkPackages::Form::WorkPackagePayloadRepresenter do end describe 'parsing' do + let(:attributes) { {} } let(:links) { {} } let(:json) do - { - _links: links - }.to_json + copy = attributes.clone + copy[:_links] = links + copy.to_json + end + + subject { representer.from_json(json) } + + describe 'startDate' do + let(:attributes) do + { + startDate: dateString + } + end + + context 'with an ISO formatted date' do + let(:dateString) { '2015-01-31' } + + it 'sets the date' do + expect(subject.start_date).to eql(Date.new(2015, 1, 31)) + end + end + + context 'with null' do + let(:dateString) { nil } + + it 'sets the date to nil' do + expect(subject.start_date).to eql(nil) + end + end + + context 'with a non ISO formatted date' do + let(:dateString) { '31.01.2015' } + + it 'raises an error' do + expect{ subject }.to raise_error(API::Errors::PropertyFormatError) + end + end + + context 'with an ISO formatted date and time' do + let(:dateString) { '2015-01-31T13:37:00Z' } + + it 'raises an error' do + expect{ subject }.to raise_error(API::Errors::PropertyFormatError) + end + end end - subject(:parsed) { representer.from_json(json) } + describe 'dueDate' do + let(:attributes) do + { + dueDate: dateString + } + end + + context 'with an ISO formatted date' do + let(:dateString) { '2015-01-31' } + + it 'sets the date' do + expect(subject.due_date).to eql(Date.new(2015, 1, 31)) + end + end + + context 'with null' do + let(:dateString) { nil } + + it 'sets the date to nil' do + expect(subject.due_date).to eql(nil) + end + end + + context 'with a non ISO formatted date' do + let(:dateString) { '31.01.2015' } + + it 'raises an error' do + expect{ subject }.to raise_error(API::Errors::PropertyFormatError) + end + end + + context 'with an ISO formatted date and time' do + let(:dateString) { '2015-01-31T13:37:00Z' } + + it 'raises an error' do + expect{ subject }.to raise_error(API::Errors::PropertyFormatError) + end + end + end describe 'version' do let(:id) { 5 } diff --git a/spec/lib/api/v3/work_packages/work_package_representer_spec.rb b/spec/lib/api/v3/work_packages/work_package_representer_spec.rb index 5f67dd71f7..69cf980c39 100644 --- a/spec/lib/api/v3/work_packages/work_package_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_representer_spec.rb @@ -74,30 +74,6 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do it { is_expected.to include_json('WorkPackage'.to_json).at_path('_type') } describe 'work_package' do - shared_examples_for 'ISO 8601 date only' do - let(:iso_date_only_string) { '%d-%02d-%02d' % [ date.year, date.month, date.day ] } - - it 'exists' do - is_expected.to have_json_path(json_path) - end - - it 'indicates date only as ISO 8601' do - is_expected.to be_json_eql(iso_date_only_string.to_json).at_path(json_path) - end - end - - shared_examples_for 'UTC ISO 8601 date and time' do - let(:iso_string) { date.utc.iso8601 } - - it 'exists' do - is_expected.to have_json_path(json_path) - end - - it 'indicates date and time as ISO 8601' do - is_expected.to be_json_eql(iso_string.to_json).at_path(json_path) - end - end - it { is_expected.to have_json_path('id') } it_behaves_like 'API V3 formattable', 'description' do @@ -112,28 +88,44 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do it { is_expected.to have_json_path('projectName') } describe 'startDate' do - it_behaves_like 'ISO 8601 date only' do + it_behaves_like 'has ISO 8601 date only' do let(:date) { work_package.start_date } let(:json_path) { 'startDate' } end + + context 'no start date' do + let(:work_package) { FactoryGirl.build(:work_package, start_date: nil) } + + it 'renders as null' do + is_expected.to be_json_eql(nil.to_json).at_path('startDate') + end + end end describe 'dueDate' do - it_behaves_like 'ISO 8601 date only' do + it_behaves_like 'has ISO 8601 date only' do let(:date) { work_package.due_date } let(:json_path) { 'dueDate' } end + + context 'no due date' do + let(:work_package) { FactoryGirl.build(:work_package, due_date: nil) } + + it 'renders as null' do + is_expected.to be_json_eql(nil.to_json).at_path('dueDate') + end + end end describe 'createdAt' do - it_behaves_like 'UTC ISO 8601 date and time' do + it_behaves_like 'has UTC ISO 8601 date and time' do let(:date) { work_package.created_at } let(:json_path) { 'createdAt' } end end describe 'updatedAt' do - it_behaves_like 'UTC ISO 8601 date and time' do + it_behaves_like 'has UTC ISO 8601 date and time' do let(:date) { work_package.updated_at } let(:json_path) { 'updatedAt' } end