Merge pull request #6843 from fupduck/feature/apiv3/time-entries-delete

[29029] DELETE option for TimeEntries

.gitignore

@@ -41,6 +41,9 @@ npm-debug.log*
 /tmp
 *.swp
 
+# Ignore Visual Studio Code files
+/.vscode
+
 # Ignore RubyMine files
 /.idea
 /frontend/.idea

app/contracts/time_entries/delete_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 DeleteContract < BaseContract
+    def validate
+      unless user_allowed_to_delete?
+        errors.add :base, :error_unauthorized
+      end
+
+      super
+    end
+
+    private
+
+    ##
+    # Users may delete time entries IF
+    # they have the :edit_time_entries or
+    # user == deleting user and :edit_own_time_entries
+    def user_allowed_to_delete?
+      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

app/services/time_entries/delete_service.rb

@@ -0,0 +1,54 @@
+#-- 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.
+#++
+
+##
+# Implements the deletion of a time entry.
+class TimeEntries::DeleteService
+  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::DeleteContract
+  end
+
+  ##
+  # Deletes the given time entry if allowed.
+  #
+  # @return True if the deletion has been initiated, false otherwise.
+  def call
+    result, errors = validate_and_yield(time_entry, user) do
+      time_entry.destroy
+    end
+
+    ServiceResult.new(success: result, errors: errors, result: time_entry)
+  end
+end

docs/api/apiv3/endpoints/time_entries.apib

@@ -107,6 +107,48 @@ Depending on custom fields defined for time entries, additional properties might
                 "message": "The requested resource could not be found."
             }
 
+
+## Delete time entry [DELETE]
+
+Permanently deletes the specified time entry.
+
++ Parameters
+    + id (required, integer, `1`) ... Time entry id
+
++ Response 202
+
+    Returned if the time entry was deleted successfully.
+
+    Note that the response body is empty as of now. In future versions of the API a body
+    *might* be returned, indicating the progress of deletion.
+
+    + Body
+
++ Response 403 (application/hal+json)
+
+    Returned if the client does not have sufficient permissions
+
+    + Body
+
+            {
+                "_type": "Error",
+                "errorIdentifier": "urn:openproject-org:api:v3:errors:MissingPermission",
+                "message": "You are not authorized to access this resource."
+            }
+
++ Response 404 (application/hal+json)
+
+    Returned if the time entry does not exist.
+
+    + Body
+
+            {
+                "_type": "Error",
+                "errorIdentifier": "urn:openproject-org:api:v3:errors:NotFound",
+                "message": "The requested resource could not be found."
+            }
+
+
 ## Time entries [/api/v3/time_entries{?offset,pageSize,filters,sortBy}]
 
 + Model

frontend/npm-shrinkwrap.json

@@ -7445,7 +7445,7 @@
     "jquery-mockjax": {
       "version": "2.2.2",
       "resolved": "https://registry.npmjs.org/jquery-mockjax/-/jquery-mockjax-2.2.2.tgz",
-      "integrity": "sha512-stVgUln5bo7g14KjeU45c31nAYlBHAUVBo4JbdoDYIJ3i6Nzlu8mkjKcAviwad9AU1dBEHHpuzA7R6mKcWgvZA==",
+      "integrity": "sha1-nH0rEDpEz2HJnMeKnueHhQS2CQc=",
       "dev": true,
       "requires": {
         "jquery": ">=1.5.2"

lib/api/v3/time_entries/time_entries_api.rb

@@ -105,6 +105,16 @@ module API
                 fail ::API::Errors::ErrorBase.create_and_merge_errors(result.errors)
               end
             end
+
+            delete do
+              call = ::TimeEntries::DeleteService.new(time_entry: @time_entry, user: current_user).call
+
+              if call.success?
+                status 202
+              else
+                fail ::API::Errors::ErrorBase.create_and_merge_errors(call.errors)
+              end
+            end
           end
 
           mount ::API::V3::TimeEntries::TimeEntriesActivityAPI

spec/contracts/time_entries/delete_contract_spec.rb

@@ -0,0 +1,117 @@
+#-- 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::DeleteContract 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 }
+  let(:permissions) { %i[edit_time_entries] }
+
+  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
+
+  before do
+    allow(time_entry_work_package)
+      .to receive(:visible?)
+      .with(current_user)
+      .and_return(work_package_visible)
+  end
+
+  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 delete 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

spec/requests/api/v3/time_entry_resource_spec.rb

@@ -493,4 +493,71 @@ describe 'API v3 time_entry resource', type: :request do
       end
     end
   end
+
+  describe 'DELETE api/v3/time_entries/:id' do
+    let(:path) { api_v3_paths.time_entry(time_entry.id) }
+    let(:permissions) { %i(edit_own_time_entries view_time_entries view_work_packages) }
+    let(:params) {}
+
+    before do
+      time_entry
+      other_time_entry
+      custom_value
+
+      delete path, params.to_json, 'CONTENT_TYPE' => 'application/json'
+    end
+
+    it 'deleted the time entry' do
+      expect(subject.status).to eq(202)
+    end
+
+    context 'when lacking permissions' do
+      let(:permissions) { %i(view_time_entries) }
+
+      it 'returns 403' do
+        expect(subject.status)
+          .to eql(403)
+      end
+    end
+
+    subject(:response) { last_response }
+
+    shared_examples_for 'deletes the time_entry' do
+      it 'responds with HTTP No Content' do
+        expect(subject.status).to eq 202
+      end
+
+      it 'removes the time_entry from the DB' do
+        expect(TimeEntry.exists?(time_entry.id)).to be_falsey
+      end
+    end
+
+    shared_examples_for 'does not delete the time_entry' do |status = 403|
+      it "responds with #{status}" do
+        expect(subject.status).to eq status
+      end
+
+      it 'does not delete the time_entry' do
+        expect(TimeEntry.exists?(time_entry.id)).to be_truthy
+      end
+    end
+
+    context 'with the user being the author' do
+      it_behaves_like 'deletes the time_entry'
+    end
+
+    context 'with the user not being the author' do
+      let(:time_entry) { other_time_entry } 
+
+      context 'but permission to edit all time entries' do
+        let(:permissions) { %i(edit_time_entries view_time_entries view_work_packages) }
+
+        it_behaves_like 'deletes the time_entry'
+      end
+
+      context 'with permission to delete own time entries' do
+        it_behaves_like 'does not delete the time_entry', 403
+      end 
+    end
+  end
 end