Merge pull request #6828 from opf/feature/apiv3/time-entries-update

[29003] Add PATCH api/v3/time_entries/:id endpoint
pull/6839/head
ulferts 6 years ago committed by GitHub
commit 5865367cdd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 35
      app/contracts/time_entries/base_contract.rb
  2. 58
      app/contracts/time_entries/update_contract.rb
  3. 15
      app/controllers/timelog_controller.rb
  4. 9
      app/helpers/error_message_helper.rb
  5. 20
      app/models/time_entry.rb
  6. 27
      app/services/time_entries/update_service.rb
  7. 2
      app/views/timelog/edit.html.erb
  8. 85
      docs/api/apiv3/endpoints/time_entries.apib
  9. 20
      lib/api/v3/time_entries/time_entries_api.rb
  10. 157
      spec/contracts/time_entries/create_contract_spec.rb
  11. 147
      spec/contracts/time_entries/shared_contract_examples.rb
  12. 90
      spec/contracts/time_entries/update_contract_spec.rb
  13. 10
      spec/models/time_entry_spec.rb
  14. 55
      spec/requests/api/v3/time_entry_resource_spec.rb

@ -36,6 +36,14 @@ module TimeEntries
TimeEntry
end
def validate
validate_hours_are_in_range
validate_project_is_set
validate_work_package
super
end
attribute :project_id
attribute :work_package_id
attribute :activity_id
@ -45,5 +53,32 @@ module TimeEntries
attribute :tyear
attribute :tmonth
attribute :tweek
private
def validate_work_package
return unless model.work_package || model.work_package_id_changed?
if work_package_invisible? ||
work_package_not_in_project?
errors.add :work_package_id, :invalid
end
end
def validate_hours_are_in_range
errors.add :hours, :invalid if model.hours&.negative?
end
def validate_project_is_set
errors.add :project_id, :invalid if model.project.nil?
end
def work_package_invisible?
model.work_package.nil? || !model.work_package.visible?(user)
end
def work_package_not_in_project?
model.work_package && model.project != model.work_package.project
end
end
end

@ -0,0 +1,58 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2017 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 doc/COPYRIGHT.rdoc for more details.
#++
module TimeEntries
class UpdateContract < BaseContract
def validate
unless user_allowed_to_update?
errors.add :base, :error_unauthorized
end
super
end
private
##
# Users may update time entries IFF
# they have the :edit_time_entries or
# user == editing user and :edit_own_time_entries
def user_allowed_to_update?
edit_all = user.allowed_to?(:edit_time_entries, model.project)
edit_own = user.allowed_to?(:edit_own_time_entries, model.project)
if model.user == user
edit_own || edit_all
else
edit_all
end
end
end
end

@ -163,7 +163,7 @@ class TimelogController < ApplicationController
@time_entry = call.result
respond_for_saving call.success?
respond_for_saving call
end
def edit
@ -174,8 +174,8 @@ class TimelogController < ApplicationController
def update
service = TimeEntries::UpdateService.new user: current_user, time_entry: @time_entry
result = service.call(attributes: permitted_params.time_entry)
respond_for_saving result.success?
call = service.call(attributes: permitted_params.time_entry)
respond_for_saving call
end
def destroy
@ -256,13 +256,10 @@ class TimelogController < ApplicationController
time_entry
end
def save_time_entry_and_respond(time_entry)
call_hook(:controller_timelog_edit_before_save, params: params, time_entry: time_entry)
respond_for_saving @time_entry.save
end
def respond_for_saving(call)
@errors = call.errors
def respond_for_saving(success)
if success
if call.success?
respond_to do |format|
format.html do
flash[:notice] = l(:notice_successful_update)

@ -37,11 +37,20 @@ module ErrorMessageHelper
render_error_messages_partial(error_messages, options[:object])
end
# Will take a contract to display the errors in a rails form.
# In order to have faulty field highlighted, the method sets
# all errors in the contract on the object as well.
def error_messages_for_contract(object, errors)
return unless errors
error_messages = errors.full_messages
errors.details.each do |attribute, details|
details.map { |d| d[:error] }.flatten.each do |message|
object.errors.add(attribute, message)
end
end
render_error_messages_partial(error_messages, object)
end

@ -49,10 +49,6 @@ class TimeEntry < ActiveRecord::Base
validates_numericality_of :hours, allow_nil: true, message: :invalid
validates_length_of :comments, maximum: 255, allow_nil: true
validate :validate_hours_are_in_range
validate :validate_project_is_set
validate :validate_consistency_of_work_package_id
scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) }
def self.visible(*args)
@ -105,20 +101,4 @@ class TimeEntry < ActiveRecord::Base
activity.root
end
end
private
# TODO: move to contract
def validate_hours_are_in_range
errors.add :hours, :invalid if hours&.negative?
end
def validate_project_is_set
errors.add :project_id, :invalid if project.nil?
end
def validate_consistency_of_work_package_id
errors.add :work_package_id, :invalid if (work_package_id && !work_package) || (work_package && project != work_package.project)
end
end

@ -29,18 +29,20 @@
#++
class TimeEntries::UpdateService
include Concerns::Contracted
attr_accessor :user, :time_entry
def initialize(user:, time_entry:)
self.user = user
self.time_entry = time_entry
self.contract_class = TimeEntries::UpdateContract
end
def call(attributes: {})
set_attributes attributes
success = validate_and_save
ServiceResult.new success: success, errors: time_entry.errors, result: time_entry
success, errors = validate_and_save(time_entry, user)
ServiceResult.new success: success, errors: errors, result: time_entry
end
private
@ -50,27 +52,8 @@ class TimeEntries::UpdateService
##
# Update project context if moving time entry
if time_entry.work_package_id_changed?
if time_entry.work_package && time_entry.work_package_id_changed?
time_entry.project_id = time_entry.work_package.project_id
end
end
def validate_and_save
##
# Perform additional validations on the model,
# since the errors from reform are not merged into the model for form errors
validate_visible_work_package
if time_entry.errors.empty?
time_entry.save
else
false
end
end
def validate_visible_work_package
if time_entry.work_package
time_entry.errors.add :work_package_id, :invalid unless time_entry.work_package.visible?(user)
end
end
end

@ -29,7 +29,7 @@ See docs/COPYRIGHT.rdoc for more details.
<%= toolbar title: t(:label_spent_time) %>
<%= labelled_tabular_form_for [@time_entry.project, @time_entry], as: :time_entry do |f| %>
<%= error_messages_for 'time_entry' %>
<%= error_messages_for_contract @time_entry, @errors %>
<%= back_url_hidden_field_tag %>
<div class="form--field">

@ -331,6 +331,91 @@ There is no form and schema resource for time entries, yet.
}
}
## Update Time entry [PATCH]
Updates the given time entry by applying the attributes provided in the body. Please note that while there is a fixed set of attributes, custom fields can extend a time entries' attributes and are accepted by the endpoint.
There is no form and schema resource for time entries, yet.
+ Request Update time entry
+ Body
{
"_links": {
"activity": {
"href": "/api/v3/time_entries/activities/18",
},
"workPackage": {
"href": "/api/v3/work_packages/5"
},
"customField4": {
"href": "/api/v3/users/5"
},
"customField51": {
"href": "/api/v3/custom_options/11"
}
},
"hours": 'PT5H',
"comment": "some comment",
"spentOn": "2017-07-28",
"customField1": {
raw: 'some text custom field value'
},
"customField8": 5
}
+ Response 200
[Time entry][]
+ Response 400 (application/hal+json)
Occurs when the client did not send a valid JSON object in the request body.
+ Body
{
"_type": "Error",
"errorIdentifier": "urn:openproject-org:api:v3:errors:InvalidRequestBody",
"message": "The request body was not a single JSON object."
}
+ Response 403 (application/hal+json)
Returned if the client does not have sufficient permissions.
**Required permission:** Edit (own) time entries, depending on what time entry is being modified.
+ Body
{
"_type": "Error",
"errorIdentifier": "urn:openproject-org:api:v3:errors:MissingPermission",
"message": "You are not authorized to access this resource."
}
+ Response 422 (application/hal+json)
Returned if:
* a constraint for a property was violated (`PropertyConstraintViolation`)
+ Body
{
"_type": "Error",
"errorIdentifier": "urn:openproject-org:api:v3:errors:PropertyConstraintViolation",
"message": "Work package is invalid.",
"_embedded": {
"details": {
"attribute"=>"workPackage"
}
}
}
# Group Time Entry Activities

@ -85,6 +85,26 @@ module API
current_user: current_user,
embed_links: true)
end
patch do
params = API::V3::ParseResourceParamsService
.new(current_user, TimeEntry, TimeEntryRepresenter)
.call(request_body)
.result
result = ::TimeEntries::UpdateService
.new(time_entry: @time_entry, user: current_user)
.call(attributes: params)
if result.success?
updated_entry = result.result
TimeEntryRepresenter.create(updated_entry,
current_user: current_user,
embed_links: true)
else
fail ::API::Errors::ErrorBase.create_and_merge_errors(result.errors)
end
end
end
mount ::API::V3::TimeEntries::TimeEntriesActivityAPI

@ -28,147 +28,46 @@
#++
require 'spec_helper'
require_relative './shared_contract_examples'
describe TimeEntries::CreateContract do
let(:current_user) do
FactoryBot.build_stubbed(:user) do |user|
allow(user)
.to receive(:allowed_to?) do |permission, permission_project|
permissions.include?(permission) && time_entry_project == permission_project
end
end
end
let(:other_user) { FactoryBot.build_stubbed(:user) }
let(:time_entry_work_package) do
FactoryBot.build_stubbed(:work_package,
project: time_entry_project)
end
let(:time_entry_project) { FactoryBot.build_stubbed(:project) }
let(:time_entry_user) { current_user }
let(:time_entry_activity) { FactoryBot.build_stubbed(:time_entry_activity) }
let(:time_entry_spent_on) { Date.today }
let(:time_entry_hours) { 5 }
let(:time_entry_comments) { "A comment" }
let(:time_entry) do
TimeEntry.new(project: time_entry_project,
work_package: time_entry_work_package,
user: time_entry_user,
activity: time_entry_activity,
spent_on: time_entry_spent_on,
hours: time_entry_hours,
comments: time_entry_comments)
end
let(:permissions) { %i(log_time) }
subject(:contract) { described_class.new(time_entry, current_user) }
def expect_valid(valid, symbols = {})
expect(contract.validate).to eq(valid)
symbols.each do |key, arr|
expect(contract.errors.symbols_for(key)).to match_array arr
end
end
shared_examples 'is valid' do
it 'is valid' do
expect_valid(true)
end
end
it_behaves_like 'is valid'
context 'when user is not allowed to log time' do
let(:permissions) { [] }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
context 'when time_entry user is not contract user' do
let(:time_entry_user) { other_user }
it 'is invalid' do
expect_valid(false, user_id: %i(invalid))
end
end
context 'when the work_package is within a differnt project than the provided project' do
let(:time_entry_work_package) { FactoryBot.build_stubbed(:work_package) }
it 'is invalid' do
expect_valid(false, work_package_id: %i(invalid))
end
end
context 'when the project is nil' do
let(:time_entry_project) { nil }
it 'is invalid' do
expect_valid(false, project_id: %i(invalid blank))
end
end
context 'when the user is nil' do
let(:time_entry_user) { nil }
it 'is invalid' do
expect_valid(false, user_id: %i(blank invalid))
end
end
context 'when activity is nil' do
let(:time_entry_activity) { nil }
it 'is invalid' do
expect_valid(false, activity_id: %i(blank))
end
end
context 'when spent_on is nil' do
let(:time_entry_spent_on) { nil }
it 'is invalid' do
expect_valid(false, spent_on: %i(blank))
it_behaves_like 'time entry contract' do
let(:time_entry) do
TimeEntry.new(project: time_entry_project,
work_package: time_entry_work_package,
user: time_entry_user,
activity: time_entry_activity,
spent_on: time_entry_spent_on,
hours: time_entry_hours,
comments: time_entry_comments)
end
end
let(:permissions) { %i(log_time) }
let(:other_user) { FactoryBot.build_stubbed(:user) }
context 'when hours is nil' do
let(:time_entry_hours) { nil }
subject(:contract) { described_class.new(time_entry, current_user) }
it 'is invalid' do
expect_valid(false, hours: %i(blank))
end
end
context 'when user is not allowed to log time' do
let(:permissions) { [] }
context 'when hours is smaller negative' do
let(:time_entry_hours) { -1 }
it 'is invalid' do
expect_valid(false, hours: %i(invalid))
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
end
context 'when hours is nil' do
let(:time_entry_hours) { nil }
context 'when time_entry user is not contract user' do
let(:time_entry_user) { other_user }
it 'is invalid' do
expect_valid(false, hours: %i(blank))
it 'is invalid' do
expect_valid(false, user_id: %i(invalid))
end
end
end
context 'when comment is longer than 255' do
let(:time_entry_comments) { "a" * 256 }
context 'when the user is nil' do
let(:time_entry_user) { nil }
it 'is invalid' do
expect_valid(false, comments: %i(too_long))
it 'is invalid' do
expect_valid(false, user_id: %i(blank invalid))
end
end
end
context 'when comment is nil' do
let(:time_entry_comments) { nil }
it_behaves_like 'is valid'
end
end

@ -0,0 +1,147 @@
#-- 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'
shared_examples_for 'time entry contract' do
let(:current_user) do
FactoryBot.build_stubbed(:user) do |user|
allow(user)
.to receive(:allowed_to?) do |permission, permission_project|
permissions.include?(permission) && time_entry_project == permission_project
end
end
end
let(:other_user) { FactoryBot.build_stubbed(:user) }
let(:time_entry_work_package) do
FactoryBot.build_stubbed(:work_package,
project: time_entry_project)
end
let(:time_entry_project) { FactoryBot.build_stubbed(:project) }
let(:time_entry_user) { current_user }
let(:time_entry_activity) { FactoryBot.build_stubbed(:time_entry_activity) }
let(:time_entry_spent_on) { Date.today }
let(:time_entry_hours) { 5 }
let(:time_entry_comments) { "A comment" }
let(:work_package_visible) { true }
before do
allow(time_entry_work_package)
.to receive(:visible?)
.with(current_user)
.and_return(work_package_visible)
end
def expect_valid(valid, symbols = {})
expect(contract.validate).to eq(valid)
symbols.each do |key, arr|
expect(contract.errors.symbols_for(key)).to match_array arr
end
end
shared_examples 'is valid' do
it 'is valid' do
expect_valid(true)
end
end
it_behaves_like 'is valid'
context 'when the work_package is within a different project than the provided project' do
let(:time_entry_work_package) { FactoryBot.build_stubbed(:work_package) }
it 'is invalid' do
expect_valid(false, work_package_id: %i(invalid))
end
end
context 'when the project is nil' do
let(:time_entry_project) { nil }
it 'is invalid' do
expect_valid(false, project_id: %i(invalid blank))
end
end
context 'when activity is nil' do
let(:time_entry_activity) { nil }
it 'is invalid' do
expect_valid(false, activity_id: %i(blank))
end
end
context 'when spent_on is nil' do
let(:time_entry_spent_on) { nil }
it 'is invalid' do
expect_valid(false, spent_on: %i(blank))
end
end
context 'when hours is nil' do
let(:time_entry_hours) { nil }
it 'is invalid' do
expect_valid(false, hours: %i(blank))
end
end
context 'when hours is negative' do
let(:time_entry_hours) { -1 }
it 'is invalid' do
expect_valid(false, hours: %i(invalid))
end
end
context 'when hours is nil' do
let(:time_entry_hours) { nil }
it 'is invalid' do
expect_valid(false, hours: %i(blank))
end
end
context 'when comment is longer than 255' do
let(:time_entry_comments) { "a" * 256 }
it 'is invalid' do
expect_valid(false, comments: %i(too_long))
end
end
context 'when comment is nil' do
let(:time_entry_comments) { nil }
it_behaves_like 'is valid'
end
end

@ -0,0 +1,90 @@
#-- 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'
require_relative './shared_contract_examples'
describe TimeEntries::UpdateContract do
it_behaves_like 'time entry contract' do
let(:time_entry) do
FactoryBot.build_stubbed(:time_entry,
project: time_entry_project,
work_package: time_entry_work_package,
user: time_entry_user,
activity: time_entry_activity,
spent_on: time_entry_spent_on,
hours: time_entry_hours,
comments: time_entry_comments)
end
subject(:contract) { described_class.new(time_entry, current_user) }
let(:permissions) { %i(edit_time_entries) }
context 'when user is not allowed to edit time entries' do
let(:permissions) { [] }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
context 'when the user is nil' do
let(:time_entry_user) { nil }
it 'is invalid' do
expect_valid(false, user_id: %i(blank))
end
end
context 'when the user is changed' do
it 'is invalid' do
time_entry.user = other_user
expect_valid(false, user_id: %i(error_readonly))
end
end
context 'when time_entry user is not contract user' do
let(:time_entry_user) { other_user }
context 'when has permission' do
let(:permissions) { %i[edit_time_entries] }
it 'is valid' do
expect_valid(true)
end
end
context 'when has no permission' do
let(:permissions) { %i[edit_own_time_entries] }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
end
end
end

@ -69,14 +69,14 @@ describe TimeEntry, type: :model do
let(:child_activity_active) do
FactoryBot.create(:time_entry_activity,
parent: activity,
project: project1)
parent: activity,
project: project1)
end
let(:child_activity_inactive) do
FactoryBot.create(:time_entry_activity,
parent: activity,
project: project2,
active: false)
parent: activity,
project: project2,
active: false)
end
before do

@ -438,4 +438,59 @@ describe 'API v3 time_entry resource', type: :request do
end
end
end
describe 'PATCH api/v3/time_entries/:id' do
let(:path) { api_v3_paths.time_entry(time_entry.id) }
let(:permissions) { %i(edit_time_entries view_time_entries view_work_packages) }
let(:params) do
{
"hours": 'PT10H'
}
end
before do
time_entry
custom_value
patch path, params.to_json, 'CONTENT_TYPE' => 'application/json'
end
it 'updates the time entry' do
expect(subject.status).to eq(200)
time_entry.reload
expect(time_entry.hours).to eq 10
end
context 'when lacking permissions' do
let(:permissions) { %i(view_time_entries) }
it 'returns 403' do
expect(subject.status)
.to eql(403)
end
end
context 'when sending invalid params' do
let(:params) do
{
"_links": {
"workPackage": {
"href": api_v3_paths.work_package(work_package.id + 1)
}
}
}
end
it 'returns 422 and complains about work packages' do
expect(subject.status)
.to eql(422)
expect(subject.body)
.to be_json_eql("Work package is invalid.".to_json)
.at_path("message")
end
end
end
end

Loading…
Cancel
Save