From fb93ff4c313864a4ad97f140c42e2e88fc004d4d Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Tue, 3 May 2022 13:09:44 +0200 Subject: [PATCH 1/2] [#42204] add file links to work package resource - https://community.openproject.org/work_packages/42204 - added file links include module for other resources - fixed some rubocop issues in work package representer - added representer tests --- .../components/schemas/work_package_model.yml | 18 +++ .../work_packages/work_package_representer.rb | 27 ++--- .../file_link_relation_representer.rb | 77 +++++++++++++ .../work_package_representer_spec.rb | 105 ++++++++++++++++-- 4 files changed, 206 insertions(+), 21 deletions(-) create mode 100644 modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb diff --git a/docs/api/apiv3/components/schemas/work_package_model.yml b/docs/api/apiv3/components/schemas/work_package_model.yml index 5a49507543..8444f00a33 100644 --- a/docs/api/apiv3/components/schemas/work_package_model.yml +++ b/docs/api/apiv3/components/schemas/work_package_model.yml @@ -307,6 +307,24 @@ properties: **Permission** view work packages readOnly: true + addFileLink: + allOf: + - $ref: './link.yml' + - description: |- + Add a file link to the work package + + # Conditions + + **Permission**: manage file links + fileLinks: + allOf: + - $ref: './link.yml' + - description: |- + Gets the file link collection of this work package + + # Conditions + + **Permission**: view file links parent: allOf: - "$ref": "./link.yml" diff --git a/lib/api/v3/work_packages/work_package_representer.rb b/lib/api/v3/work_packages/work_package_representer.rb index 0d08c9f144..655c4de884 100644 --- a/lib/api/v3/work_packages/work_package_representer.rb +++ b/lib/api/v3/work_packages/work_package_representer.rb @@ -35,6 +35,7 @@ module API include API::Decorators::FormattableProperty include API::Caching::CachedRepresenter include ::API::V3::Attachments::AttachableRepresenterMixin + include ::API::V3::FileLinks::FileLinkRelationRepresenter extend ::API::V3::Utilities::CustomFieldInjector::RepresenterClass cached_representer key_parts: %i(project), @@ -351,10 +352,10 @@ module API next unless doc.key?('date') date = decorator - .datetime_formatter - .parse_date(doc['date'], - name.to_s.camelize(:lower), - allow_nil: true) + .datetime_formatter + .parse_date(doc['date'], + name.to_s.camelize(:lower), + allow_nil: true) self.due_date = self.start_date = date }, @@ -473,10 +474,10 @@ module API new_parent = if href id = ::API::Utilities::ResourceLinkParser - .parse_id href, - property: 'parent', - expected_version: '3', - expected_namespace: 'work_packages' + .parse_id href, + property: 'parent', + expected_version: '3', + expected_namespace: 'work_packages' WorkPackage.find_by(id: id) || ::WorkPackage::InexistentWorkPackage.new(id: id) @@ -536,10 +537,10 @@ module API def relations self_path = api_v3_paths.work_package_relations(represented.id) visible_relations = represented - .visible_relations(current_user) - .direct - .non_hierarchy - .includes(::API::V3::Relations::RelationCollectionRepresenter.to_eager_load) + .visible_relations(current_user) + .direct + .non_hierarchy + .includes(::API::V3::Relations::RelationCollectionRepresenter.to_eager_load) ::API::V3::Relations::RelationCollectionRepresenter.new(visible_relations, self_link: self_path, @@ -562,7 +563,7 @@ module API def derived_estimated_time=(value) represented.derived_estimated_hours = datetime_formatter - .parse_duration_to_hours(value, 'derivedEstimatedTime', allow_nil: true) + .parse_duration_to_hours(value, 'derivedEstimatedTime', allow_nil: true) end def spent_time=(value) diff --git a/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb b/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb new file mode 100644 index 0000000000..ea34f73e97 --- /dev/null +++ b/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb @@ -0,0 +1,77 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# 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 COPYRIGHT and LICENSE files for more details. +#++ + +module API + module V3 + module FileLinks + module FileLinkRelationRepresenter + extend ActiveSupport::Concern + + included do + link :fileLinks, cache_if: -> { render_file_links? } do + { + href: api_v3_paths.file_links(represented.id) + } + end + + link :addFileLink, cache_if: -> { render_action_link? } do + { + href: api_v3_paths.file_links(represented.id), + method: :post + } + end + + property :file_links, + embedded: true, + exec_context: :decorator, + if: ->(*) { embed_links && render_file_links? }, + uncacheable: true + + def file_links + ::API::V3::FileLinks::FileLinkCollectionRepresenter.new(represented.file_links, + self_link: api_v3_paths.file_links(represented.id), + current_user: current_user) + end + + private + + def render_action_link? + render_file_links? && + current_user.allowed_to?(:manage_file_links, represented.project) + end + + def render_file_links? + OpenProject::FeatureDecisions.storages_module_active? && + represented.project.module_enabled?('storages') && + current_user.allowed_to?(:view_file_links, represented.project) + end + end + end + end + end +end 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 5ff32d31d2..91fb5177b4 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 @@ -39,18 +39,18 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do described_class.create(work_package, current_user: current_user, embed_links: embed_links) end let(:parent) { nil } - let(:priority) { build_stubbed(:priority, updated_at: Time.now) } + let(:priority) { build_stubbed(:priority, updated_at: Time.zone.now) } let(:assignee) { nil } let(:responsible) { nil } let(:schedule_manually) { nil } - let(:start_date) { Date.today.to_datetime } - let(:due_date) { Date.today.to_datetime } + let(:start_date) { Time.zone.today.to_datetime } + let(:due_date) { Time.zone.today.to_datetime } let(:type_milestone) { false } let(:estimated_hours) { nil } let(:derived_estimated_hours) { nil } let(:spent_hours) { 0 } - let(:derived_start_date) { Date.today - 4.days } - let(:derived_due_date) { Date.today - 5.days } + let(:derived_start_date) { Time.zone.today - 4.days } + let(:derived_due_date) { Time.zone.today - 5.days } let(:budget) { build_stubbed(:budget, project: project) } let(:work_package) do build_stubbed(:stubbed_work_package, @@ -97,6 +97,8 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do add_work_packages view_time_entries view_changesets + view_file_links + manage_file_links delete_work_packages ] end @@ -109,7 +111,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do type end - let(:status) { build_stubbed(:status, updated_at: Time.now) } + let(:status) { build_stubbed(:status, updated_at: Time.zone.now) } let(:available_custom_fields) { [] } before(:each) do @@ -141,7 +143,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do it_behaves_like 'API V3 formattable', 'description' do let(:format) { 'markdown' } let(:raw) { work_package.description } - let(:html) { '

' + work_package.description + '

' } + let(:html) { "

#{work_package.description}

" } end describe 'scheduleManually' do @@ -715,6 +717,60 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end end + describe 'fileLinks' do + let(:storages_feature_enabled) { true } + + before do + allow(OpenProject::FeatureDecisions).to receive(:storages_module_active?).and_return(storages_feature_enabled) + end + + it_behaves_like 'has an untitled link' do + let(:link) { 'fileLinks' } + let(:href) { api_v3_paths.file_links(work_package.id) } + end + + it_behaves_like 'has an untitled action link' do + let(:permission) { :manage_file_links } + let(:link) { 'addFileLink' } + let(:href) { api_v3_paths.file_links(work_package.id) } + let(:method) { 'post' } + end + + context 'if user has no permission to view file links' do + let(:permissions) { all_permissions - %i[view_file_links] } + + it_behaves_like 'has no link' do + let(:link) { 'fileLinks' } + end + end + + context 'if file storages feature is disabled' do + let(:storages_feature_enabled) { false } + + it_behaves_like 'has no link' do + let(:link) { 'fileLinks' } + end + + it_behaves_like 'has no link' do + let(:link) { 'addFileLink' } + end + end + + context 'if project has storages module disabled' do + before do + project.enabled_module_names = project.enabled_module_names - ['storages'] + end + + it_behaves_like 'has no link' do + let(:link) { 'fileLinks' } + end + + it_behaves_like 'has no link' do + let(:link) { 'addFileLink' } + end + end + end + context 'when the user is not watching the work package' do it 'should have a link to watch' do expect(subject) @@ -1151,6 +1207,39 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end end + describe 'fileLinks' do + let(:storages_feature_enabled) { true } + let(:storage) { build_stubbed(:storage) } + let(:file_link) { build_stubbed(:file_link, storage: storage, container: work_package) } + + before do + allow(OpenProject::FeatureDecisions).to receive(:storages_module_active?).and_return(storages_feature_enabled) + allow(work_package).to receive(:file_links).and_return([file_link]) + end + + it 'embeds a collection' do + is_expected + .to be_json_eql('Collection'.to_json) + .at_path('_embedded/fileLinks/_type') + end + + it 'embeds with an href containing the work_package' do + is_expected + .to be_json_eql(api_v3_paths.file_links(work_package.id).to_json) + .at_path('_embedded/fileLinks/_links/self/href') + end + + it 'embeds the visible file links' do + is_expected + .to be_json_eql(1.to_json) + .at_path('_embedded/fileLinks/total') + + is_expected + .to be_json_eql(api_v3_paths.file_link(file_link.id).to_json) + .at_path('_embedded/fileLinks/_embedded/elements/0/_links/self/href') + end + end + describe 'customActions' do it 'has an array of customActions' do unassign_action = build_stubbed(:custom_action, @@ -1223,7 +1312,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end it 'changes when the work_package is updated' do - work_package.updated_at = Time.now + 20.seconds + work_package.updated_at = Time.zone.now + 20.seconds expect(representer.json_cache_key) .not_to eql former_cache_key From 44392cfb9611ae11f6daf3f027e4a9b63d342be2 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Mon, 9 May 2022 10:08:05 +0200 Subject: [PATCH 2/2] [#42204] fixed tests and permission check --- .../file_link_relation_representer.rb | 19 ++-------- .../work_package_representer_spec.rb | 36 +------------------ 2 files changed, 4 insertions(+), 51 deletions(-) diff --git a/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb b/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb index ea34f73e97..595891d821 100644 --- a/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb +++ b/modules/storages/lib/api/v3/file_links/file_link_relation_representer.rb @@ -33,13 +33,13 @@ module API extend ActiveSupport::Concern included do - link :fileLinks, cache_if: -> { render_file_links? } do + link :fileLinks, cache_if: -> { current_user.allowed_to?(:view_file_links, represented.project) } do { href: api_v3_paths.file_links(represented.id) } end - link :addFileLink, cache_if: -> { render_action_link? } do + link :addFileLink, cache_if: -> { current_user.allowed_to?(:manage_file_links, represented.project) } do { href: api_v3_paths.file_links(represented.id), method: :post @@ -49,7 +49,7 @@ module API property :file_links, embedded: true, exec_context: :decorator, - if: ->(*) { embed_links && render_file_links? }, + if: ->(*) { embed_links && current_user.allowed_to?(:view_file_links, represented.project) }, uncacheable: true def file_links @@ -57,19 +57,6 @@ module API self_link: api_v3_paths.file_links(represented.id), current_user: current_user) end - - private - - def render_action_link? - render_file_links? && - current_user.allowed_to?(:manage_file_links, represented.project) - end - - def render_file_links? - OpenProject::FeatureDecisions.storages_module_active? && - represented.project.module_enabled?('storages') && - current_user.allowed_to?(:view_file_links, represented.project) - end end end end 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 91fb5177b4..fb10e83620 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 @@ -718,12 +718,6 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end describe 'fileLinks' do - let(:storages_feature_enabled) { true } - - before do - allow(OpenProject::FeatureDecisions).to receive(:storages_module_active?).and_return(storages_feature_enabled) - end - it_behaves_like 'has an untitled link' do let(:link) { 'fileLinks' } let(:href) { api_v3_paths.file_links(work_package.id) } @@ -743,32 +737,6 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do let(:link) { 'fileLinks' } end end - - context 'if file storages feature is disabled' do - let(:storages_feature_enabled) { false } - - it_behaves_like 'has no link' do - let(:link) { 'fileLinks' } - end - - it_behaves_like 'has no link' do - let(:link) { 'addFileLink' } - end - end - - context 'if project has storages module disabled' do - before do - project.enabled_module_names = project.enabled_module_names - ['storages'] - end - - it_behaves_like 'has no link' do - let(:link) { 'fileLinks' } - end - - it_behaves_like 'has no link' do - let(:link) { 'addFileLink' } - end - end end context 'when the user is not watching the work package' do @@ -1208,12 +1176,10 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end describe 'fileLinks' do - let(:storages_feature_enabled) { true } let(:storage) { build_stubbed(:storage) } let(:file_link) { build_stubbed(:file_link, storage: storage, container: work_package) } before do - allow(OpenProject::FeatureDecisions).to receive(:storages_module_active?).and_return(storages_feature_enabled) allow(work_package).to receive(:file_links).and_return([file_link]) end @@ -1312,7 +1278,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end it 'changes when the work_package is updated' do - work_package.updated_at = Time.zone.now + 20.seconds + work_package.updated_at = 20.seconds.from_now expect(representer.json_cache_key) .not_to eql former_cache_key