From c7e9239212739b7e7dec9b55cab71f16d5bd8d61 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20G=C3=BCnther?= <mail@oliverguenther.de>
Date: Wed, 14 Nov 2018 21:29:43 +0100
Subject: [PATCH 1/2] [29003] Add PATCH api/v3/time_entries/:id endpoint

Adds updating time entries to APIv3.

https://community.openproject.com/wp/29003
---
 app/contracts/time_entries/update_contract.rb |  58 +++++++++
 app/services/time_entries/update_service.rb   |  35 +++---
 docs/api/apiv3/endpoints/time_entries.apib    |  85 ++++++++++++++
 lib/api/v3/time_entries/time_entries_api.rb   |  20 ++++
 .../time_entries/update_contract_spec.rb      | 111 ++++++++++++++++++
 .../api/v3/time_entry_resource_spec.rb        |  55 +++++++++
 6 files changed, 346 insertions(+), 18 deletions(-)
 create mode 100644 app/contracts/time_entries/update_contract.rb
 create mode 100644 spec/contracts/time_entries/update_contract_spec.rb

diff --git a/app/contracts/time_entries/update_contract.rb b/app/contracts/time_entries/update_contract.rb
new file mode 100644
index 0000000000..88660cbe29
--- /dev/null
+++ b/app/contracts/time_entries/update_contract.rb
@@ -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
+        return edit_own || edit_all
+      else
+        return edit_all
+      end
+    end
+  end
+end
diff --git a/app/services/time_entries/update_service.rb b/app/services/time_entries/update_service.rb
index 516175531d..901bf83e71 100644
--- a/app/services/time_entries/update_service.rb
+++ b/app/services/time_entries/update_service.rb
@@ -29,18 +29,28 @@
 #++
 
 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_yield(time_entry, user) do
+      ##
+      # Perform additional validations on the model,
+      # since the errors from reform are not merged into the model for form errors
+      validate_visible_work_package
+
+      time_entry.errors.empty? && time_entry.save
+    end
+
+    ServiceResult.new success: success, errors: errors, result: time_entry
   end
 
   private
@@ -50,27 +60,16 @@ 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)
+    return unless time_entry.work_package || time_entry.work_package_id_changed?
+
+    if time_entry.work_package.nil? || !time_entry.work_package.visible?(user)
+      time_entry.errors.add :work_package_id, :invalid
     end
   end
 end
diff --git a/docs/api/apiv3/endpoints/time_entries.apib b/docs/api/apiv3/endpoints/time_entries.apib
index 646040891b..878684ea67 100644
--- a/docs/api/apiv3/endpoints/time_entries.apib
+++ b/docs/api/apiv3/endpoints/time_entries.apib
@@ -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
 
diff --git a/lib/api/v3/time_entries/time_entries_api.rb b/lib/api/v3/time_entries/time_entries_api.rb
index 1ae5894782..c19296eb02 100644
--- a/lib/api/v3/time_entries/time_entries_api.rb
+++ b/lib/api/v3/time_entries/time_entries_api.rb
@@ -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
diff --git a/spec/contracts/time_entries/update_contract_spec.rb b/spec/contracts/time_entries/update_contract_spec.rb
new file mode 100644
index 0000000000..5210ed5f4f
--- /dev/null
+++ b/spec/contracts/time_entries/update_contract_spec.rb
@@ -0,0 +1,111 @@
+#-- 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 TimeEntries::UpdateContract 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_activity) { FactoryBot.build_stubbed(:time_entry_activity) }
+  let(:time_entry_user) { current_user }
+  let(:time_entry_spent_on) { Date.today }
+  let(:time_entry_hours) { 5 }
+  let(:time_entry_comments) { "A comment" }
+  let(:time_entry) do
+    TimeEntry.create(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(edit_time_entries) }
+
+  subject(:contract) { described_class.new(time_entry, current_user) }
+
+  before do
+    allow(Project).to receive(:find).with(time_entry_project.id).and_return(time_entry_project)
+  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 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 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
diff --git a/spec/requests/api/v3/time_entry_resource_spec.rb b/spec/requests/api/v3/time_entry_resource_spec.rb
index 1d04925bbc..188836308e 100644
--- a/spec/requests/api/v3/time_entry_resource_spec.rb
+++ b/spec/requests/api/v3/time_entry_resource_spec.rb
@@ -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

From 198ac0918ae60ded3eb46bf722572404597801d4 Mon Sep 17 00:00:00 2001
From: Jens Ulferts <j.ulferts@finn.de>
Date: Mon, 19 Nov 2018 11:26:05 +0100
Subject: [PATCH 2/2] move time entry validations to contract

---
 app/contracts/time_entries/base_contract.rb   |  35 ++++
 app/contracts/time_entries/update_contract.rb |   4 +-
 app/controllers/timelog_controller.rb         |  15 +-
 app/helpers/error_message_helper.rb           |   9 +
 app/models/time_entry.rb                      |  20 ---
 app/services/time_entries/update_service.rb   |  18 +-
 app/views/timelog/edit.html.erb               |   2 +-
 .../time_entries/create_contract_spec.rb      | 157 ++++--------------
 .../time_entries/shared_contract_examples.rb  | 147 ++++++++++++++++
 .../time_entries/update_contract_spec.rb      | 103 +++++-------
 spec/models/time_entry_spec.rb                |  10 +-
 11 files changed, 275 insertions(+), 245 deletions(-)
 create mode 100644 spec/contracts/time_entries/shared_contract_examples.rb

diff --git a/app/contracts/time_entries/base_contract.rb b/app/contracts/time_entries/base_contract.rb
index eead32d5cc..a0c0429bed 100644
--- a/app/contracts/time_entries/base_contract.rb
+++ b/app/contracts/time_entries/base_contract.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
diff --git a/app/contracts/time_entries/update_contract.rb b/app/contracts/time_entries/update_contract.rb
index 88660cbe29..2bf0084d12 100644
--- a/app/contracts/time_entries/update_contract.rb
+++ b/app/contracts/time_entries/update_contract.rb
@@ -49,9 +49,9 @@ module TimeEntries
       edit_own = user.allowed_to?(:edit_own_time_entries, model.project)
 
       if model.user == user
-        return edit_own || edit_all
+        edit_own || edit_all
       else
-        return edit_all
+        edit_all
       end
     end
   end
diff --git a/app/controllers/timelog_controller.rb b/app/controllers/timelog_controller.rb
index ff1028fcb2..53aef44769 100644
--- a/app/controllers/timelog_controller.rb
+++ b/app/controllers/timelog_controller.rb
@@ -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)
diff --git a/app/helpers/error_message_helper.rb b/app/helpers/error_message_helper.rb
index 6d5269022a..8d521dbe70 100644
--- a/app/helpers/error_message_helper.rb
+++ b/app/helpers/error_message_helper.rb
@@ -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
 
diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb
index d66e95343f..f775a2708e 100644
--- a/app/models/time_entry.rb
+++ b/app/models/time_entry.rb
@@ -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
diff --git a/app/services/time_entries/update_service.rb b/app/services/time_entries/update_service.rb
index 901bf83e71..c2a746cb7f 100644
--- a/app/services/time_entries/update_service.rb
+++ b/app/services/time_entries/update_service.rb
@@ -41,15 +41,7 @@ class TimeEntries::UpdateService
   def call(attributes: {})
     set_attributes attributes
 
-    success, errors = validate_and_yield(time_entry, user) do
-      ##
-      # Perform additional validations on the model,
-      # since the errors from reform are not merged into the model for form errors
-      validate_visible_work_package
-
-      time_entry.errors.empty? && time_entry.save
-    end
-
+    success, errors = validate_and_save(time_entry, user)
     ServiceResult.new success: success, errors: errors, result: time_entry
   end
 
@@ -64,12 +56,4 @@ class TimeEntries::UpdateService
       time_entry.project_id = time_entry.work_package.project_id
     end
   end
-
-  def validate_visible_work_package
-    return unless time_entry.work_package || time_entry.work_package_id_changed?
-
-    if time_entry.work_package.nil? || !time_entry.work_package.visible?(user)
-      time_entry.errors.add :work_package_id, :invalid
-    end
-  end
 end
diff --git a/app/views/timelog/edit.html.erb b/app/views/timelog/edit.html.erb
index 65c0bfd86d..1e50d7b182 100644
--- a/app/views/timelog/edit.html.erb
+++ b/app/views/timelog/edit.html.erb
@@ -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">
diff --git a/spec/contracts/time_entries/create_contract_spec.rb b/spec/contracts/time_entries/create_contract_spec.rb
index 192b8455f7..74f1e6860b 100644
--- a/spec/contracts/time_entries/create_contract_spec.rb
+++ b/spec/contracts/time_entries/create_contract_spec.rb
@@ -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
diff --git a/spec/contracts/time_entries/shared_contract_examples.rb b/spec/contracts/time_entries/shared_contract_examples.rb
new file mode 100644
index 0000000000..64765e2459
--- /dev/null
+++ b/spec/contracts/time_entries/shared_contract_examples.rb
@@ -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
diff --git a/spec/contracts/time_entries/update_contract_spec.rb b/spec/contracts/time_entries/update_contract_spec.rb
index 5210ed5f4f..6ed1c41f7d 100644
--- a/spec/contracts/time_entries/update_contract_spec.rb
+++ b/spec/contracts/time_entries/update_contract_spec.rb
@@ -28,83 +28,62 @@
 #++
 
 require 'spec_helper'
+require_relative './shared_contract_examples'
 
 describe TimeEntries::UpdateContract 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
+  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
-  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_activity) { FactoryBot.build_stubbed(:time_entry_activity) }
-  let(:time_entry_user) { current_user }
-  let(:time_entry_spent_on) { Date.today }
-  let(:time_entry_hours) { 5 }
-  let(:time_entry_comments) { "A comment" }
-  let(:time_entry) do
-    TimeEntry.create(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(edit_time_entries) }
-
-  subject(:contract) { described_class.new(time_entry, current_user) }
-
-  before do
-    allow(Project).to receive(:find).with(time_entry_project.id).and_return(time_entry_project)
-  end
-
-  def expect_valid(valid, symbols = {})
-    expect(contract.validate).to eq(valid)
+    subject(:contract) { described_class.new(time_entry, current_user) }
+    let(:permissions) { %i(edit_time_entries) }
 
-    symbols.each do |key, arr|
-      expect(contract.errors.symbols_for(key)).to match_array arr
-    end
-  end
+    context 'when user is not allowed to edit time entries' do
+      let(:permissions) { [] }
 
-  shared_examples 'is valid' do
-    it 'is valid' do
-      expect_valid(true)
+      it 'is invalid' do
+        expect_valid(false, base: %i(error_unauthorized))
+      end
     end
-  end
 
-  it_behaves_like 'is valid'
+    context 'when the user is nil' do
+      let(:time_entry_user) { nil }
 
-  context 'when user is not allowed to edit time entries' do
-    let(:permissions) { [] }
+      it 'is invalid' do
+        expect_valid(false, user_id: %i(blank))
+      end
+    end
 
-    it 'is invalid' do
-      expect_valid(false, base: %i(error_unauthorized))
+    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
-  end
 
-  context 'when time_entry user is not contract user' do
-    let(:time_entry_user) { other_user }
+    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] }
+      context 'when has permission' do
+        let(:permissions) { %i[edit_time_entries] }
 
-      it 'is valid' do
-        expect_valid(true)
+        it 'is valid' do
+          expect_valid(true)
+        end
       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))
+      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
diff --git a/spec/models/time_entry_spec.rb b/spec/models/time_entry_spec.rb
index 2eb4fb1305..bfd5af1266 100644
--- a/spec/models/time_entry_spec.rb
+++ b/spec/models/time_entry_spec.rb
@@ -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