From ecec07b5203f3492dcc496bc554ef07fc288b0f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karel=20Pi=C4=8Dman?= Date: Thu, 23 Apr 2020 10:13:06 +0200 Subject: [PATCH] Status 404 after moving the folder to another project #1106 --- .../dmsf_context_menus_controller.rb | 2 - app/controllers/dmsf_controller.rb | 12 +-- .../dmsf_folders_copy_controller.rb | 16 ++-- app/controllers/dmsf_links_controller.rb | 3 +- app/models/dmsf_file.rb | 10 ++- app/models/dmsf_folder.rb | 34 ++++---- app/models/dmsf_link.rb | 17 ++-- app/models/dmsf_query.rb | 46 +++++++--- app/views/dmsf/edit.html.erb | 5 -- app/views/dmsf/show.html.erb | 2 +- app/views/dmsf_context_menus/_folder.html.erb | 26 +++--- app/views/dmsf_links/_form.html.erb | 20 ++--- config/routes.rb | 1 + ...423071301_add_indexes_on_dmsf_folder_id.rb | 28 +++++++ .../patches/queries_helper_patch.rb | 8 +- lib/redmine_dmsf/webdav/dmsf_resource.rb | 49 ++++------- test/fixtures/dmsf_file_revisions.yml | 22 +++++ test/functional/dmsf_controller_test.rb | 5 +- .../dmsf_folders_copy_controller_test.rb | 6 ++ test/functional/dmsf_links_controller_test.rb | 4 +- .../webdav/dmsf_webdav_move_test.rb | 49 +++++++++-- test/unit/dmsf_folder_test.rb | 34 ++++++-- test/unit/dmsf_link_test.rb | 84 +++++++++---------- 23 files changed, 299 insertions(+), 184 deletions(-) create mode 100644 db/migrate/20200423071301_add_indexes_on_dmsf_folder_id.rb diff --git a/app/controllers/dmsf_context_menus_controller.rb b/app/controllers/dmsf_context_menus_controller.rb index ddf949b6..6fad99ad 100644 --- a/app/controllers/dmsf_context_menus_controller.rb +++ b/app/controllers/dmsf_context_menus_controller.rb @@ -83,8 +83,6 @@ class DmsfContextMenusController < ApplicationController def find_folder @folder = DmsfFolder.find params[:folder_id] if params[:folder_id].present? - rescue DmsfAccessError - render_403 rescue ActiveRecord::RecordNotFound render_404 end diff --git a/app/controllers/dmsf_controller.rb b/app/controllers/dmsf_controller.rb index 551cec38..f3e4506f 100644 --- a/app/controllers/dmsf_controller.rb +++ b/app/controllers/dmsf_controller.rb @@ -30,13 +30,13 @@ class DmsfController < ApplicationController :autocomplete_for_user] before_action :find_parent, only: [:new, :create] before_action :permissions - # also try to lookup folder by title if this is API call + # Also try to lookup folder by title if this is an API call before_action :find_folder_by_title, only: [:show] before_action :get_query, only: [:expand_folder, :show, :trash] accept_api_auth :show, :create, :save, :delete - helper :all # TODO: Is it needed? + helper :custom_fields helper :dmsf_folder_permissions helper :queries include QueriesHelper @@ -48,7 +48,6 @@ class DmsfController < ApplicationController def expand_folder @idnt = params[:idnt].present? ? params[:idnt].to_i + 1 : 0 - #@query = retrieve_query(DmsfQuery, true) @query.dmsf_folder_id = @folder.id @query.deleted = false respond_to do |format| @@ -62,8 +61,6 @@ class DmsfController < ApplicationController @folder_manipulation_allowed = User.current.allowed_to?(:folder_manipulation, @project) @file_manipulation_allowed = User.current.allowed_to?(:file_manipulation, @project) @trash_enabled = @folder_manipulation_allowed && @file_manipulation_allowed - #use_session = !request.format.csv? - #@query = retrieve_query(DmsfQuery, use_session) @query.dmsf_folder_id = @folder ? @folder.id : nil @query.deleted = false if (@folder && @folder.deleted?) || (params[:folder_title].present? && !@folder) @@ -89,7 +86,6 @@ class DmsfController < ApplicationController @folder_manipulation_allowed = User.current.allowed_to? :folder_manipulation, @project @file_manipulation_allowed = User.current.allowed_to? :file_manipulation, @project @file_delete_allowed = User.current.allowed_to? :file_delete, @project - #@query = retrieve_query(DmsfQuery, true) @query.deleted = true respond_to do |format| format.html { @@ -604,9 +600,7 @@ class DmsfController < ApplicationController end def find_folder - @folder = DmsfFolder.find_by!(id: params[:folder_id], project_id: @project.id) if params[:folder_id].present? - rescue DmsfAccessError - render_403 + @folder = DmsfFolder.find(params[:folder_id]) if params[:folder_id].present? rescue ActiveRecord::RecordNotFound render_404 end diff --git a/app/controllers/dmsf_folders_copy_controller.rb b/app/controllers/dmsf_folders_copy_controller.rb index 53b26b52..6ab18bef 100644 --- a/app/controllers/dmsf_folders_copy_controller.rb +++ b/app/controllers/dmsf_folders_copy_controller.rb @@ -38,24 +38,22 @@ class DmsfFoldersCopyController < ApplicationController def copy new_folder = @folder.copy_to(@target_project, @target_folder) - unless new_folder.errors.empty? + if new_folder.errors.empty? + flash[:notice] = l(:notice_successful_update) + redirect_to dmsf_folder_path(id: @target_project, folder_id: new_folder) + else flash[:error] = new_folder.errors.full_messages.to_sentence - redirect_to action: 'new', id: @folder, target_project_id: @target_project, target_folder_id: @target_folder - return + redirect_to :back end - flash[:notice] = l(:notice_successful_update) - redirect_to dmsf_folder_path(id: @target_project, folder_id: new_folder) end def move - @folder.project = @target_project - @folder.dmsf_folder = @target_folder - if @folder.save + if @folder.move_to(@target_project, @target_folder) flash[:notice] = l(:notice_successful_update) redirect_to dmsf_folder_path(id: @target_project, folder_id: @folder) else flash[:error] = @folder.errors.full_messages.to_sentence - redirect_to action: 'new', id: @folder, target_project_id: @target_project, target_folder_id: @target_folder + redirect_to :back end end diff --git a/app/controllers/dmsf_links_controller.rb b/app/controllers/dmsf_links_controller.rb index cfb6c406..03efbccc 100644 --- a/app/controllers/dmsf_links_controller.rb +++ b/app/controllers/dmsf_links_controller.rb @@ -155,7 +155,8 @@ class DmsfLinksController < ApplicationController if params[:dmsf_link][:dmsf_file_id].present? redirect_to dmsf_file_path(@dmsf_link.target_file) else - redirect_to edit_dmsf_path(id: params[:dmsf_link][:project_id], folder_id: params[:dmsf_link][:dmsf_folder_id]) + folder = @dmsf_link.target_folder.dmsf_folder if @dmsf_link.target_folder + redirect_to dmsf_folder_path(id: @project, folder_id: folder) end end } diff --git a/app/models/dmsf_file.rb b/app/models/dmsf_file.rb index cd307094..4616738b 100644 --- a/app/models/dmsf_file.rb +++ b/app/models/dmsf_file.rb @@ -235,8 +235,12 @@ class DmsfFile < ActiveRecord::Base errors[:base] << l(:error_file_is_locked) return false end + unless last_revision + errors[:base] << l(:error_at_least_one_revision_must_be_present) + return false + end source = "#{project.identifier}:#{dmsf_path_str}" - self.project_id = project.id + self.project = project self.dmsf_folder = folder new_revision = last_revision.clone new_revision.workflow = nil @@ -251,12 +255,12 @@ class DmsfFile < ActiveRecord::Base last_revision.custom_values.each do |cv| new_revision.custom_values << CustomValue.new({ custom_field: cv.custom_field, value: cv.value }) end - set_last_revision(new_revision) + set_last_revision new_revision save && new_revision.save end def copy_to(project, folder = nil) - copy_to_filename(project, folder, name) + copy_to_filename project, folder, name end def copy_to_filename(project, folder, filename) diff --git a/app/models/dmsf_folder.rb b/app/models/dmsf_folder.rb index 2a60d2f3..4efb59ae 100644 --- a/app/models/dmsf_folder.rb +++ b/app/models/dmsf_folder.rb @@ -247,6 +247,23 @@ class DmsfFolder < ActiveRecord::Base projects end + def move_to(target_project, target_folder) + if self.project != target_project + dmsf_files.visible.find_each do |f| + f.move_to target_project, f.dmsf_folder + end + dmsf_folders.visible.find_each do |s| + s.move_to target_project, s.dmsf_folder + end + dmsf_links.visible.find_each do |l| + l.move_to target_project, l.dmsf_folder + end + self.project = target_project + end + self.dmsf_folder = target_folder + save + end + def copy_to(project, folder) new_folder = DmsfFolder.new new_folder.dmsf_folder = folder ? folder : nil @@ -254,7 +271,6 @@ class DmsfFolder < ActiveRecord::Base new_folder.title = title new_folder.description = description new_folder.user = User.current - new_folder.custom_values = [] custom_values.each do |cv| v = CustomValue.new @@ -262,36 +278,22 @@ class DmsfFolder < ActiveRecord::Base v.value = cv.value new_folder.custom_values << v end - unless new_folder.save Rails.logger.error new_folder.errors.full_messages.to_sentence return new_folder end - dmsf_files.visible.find_each do |f| f.copy_to project, new_folder end - dmsf_folders.visible.find_each do |s| s.copy_to project, new_folder end - - folder_links.visible.find_each do |l| + dmsf_links.visible.find_each do |l| l.copy_to project, new_folder end - - file_links.visible.find_each do |l| - l.copy_to project, new_folder - end - - url_links.visible.find_each do |l| - l.copy_to project, new_folder - end - dmsf_folder_permissions.find_each do |p| p.copy_to new_folder end - new_folder end diff --git a/app/models/dmsf_link.rb b/app/models/dmsf_link.rb index bde48b24..fe8de71a 100644 --- a/app/models/dmsf_link.rb +++ b/app/models/dmsf_link.rb @@ -79,13 +79,6 @@ class DmsfLink < ActiveRecord::Base @target_project end - def folder - if !@folder && dmsf_folder_id - @folder = DmsfFolder.find_by(id: dmsf_folder_id) - end - @folder - end - def title name end @@ -114,6 +107,12 @@ class DmsfLink < ActiveRecord::Base path end + def move_to(target_project, target_folder) + self.project = target_project + self.dmsf_folder = target_folder + save + end + def copy_to(project, folder) link = DmsfLink.new link.target_project_id = target_project_id @@ -128,8 +127,8 @@ class DmsfLink < ActiveRecord::Base end def container - if folder && folder.system - Issue.find_by(id: folder.title) + if dmsf_folder && dmsf_folder.system + Issue.find_by id: dmsf_folder.title end end diff --git a/app/models/dmsf_query.rb b/app/models/dmsf_query.rb index ca041817..b310aa02 100644 --- a/app/models/dmsf_query.rb +++ b/app/models/dmsf_query.rb @@ -189,9 +189,13 @@ class DmsfQuery < Query joins('LEFT JOIN users ON dmsf_folders.user_id = users.id'). visible(!deleted) if deleted - scope.where(dmsf_folders: { project_id: project.id, deleted: deleted }) + scope.where dmsf_folders: { project_id: project.id, deleted: deleted } else - scope.where(dmsf_folders: { project_id: project.id, dmsf_folder_id: dmsf_folder_id, deleted: deleted }) + if dmsf_folder_id + scope.where dmsf_folders: { dmsf_folder_id: dmsf_folder_id, deleted: deleted } + else + scope.where dmsf_folders: { project_id: project.id, dmsf_folder_id: nil, deleted: deleted } + end end end @@ -204,7 +208,7 @@ class DmsfQuery < Query select(%{ dmsf_links.id AS id, COALESCE(dmsf_folders.project_id, dmsf_links.project_id) AS project_id, - CAST(NULL AS #{ActiveRecord::Base.connection.type_to_sql(:decimal)}) AS revision_id, + dmsf_links.target_id AS revision_id, dmsf_links.name AS title, dmsf_folders.title AS filename, CAST(NULL AS #{ActiveRecord::Base.connection.type_to_sql(:decimal)}) AS size, @@ -222,10 +226,13 @@ class DmsfQuery < Query joins('LEFT JOIN dmsf_folders ON dmsf_links.target_id = dmsf_folders.id'). joins('LEFT JOIN users ON users.id = COALESCE(dmsf_folders.user_id, dmsf_links.user_id)') if deleted - scope.where(dmsf_links: { target_type: 'DmsfFolder', project_id: project.id, deleted: deleted }) + scope.where dmsf_links: { target_type: 'DmsfFolder', project_id: project.id, deleted: deleted } else - scope.where(dmsf_links: { target_type: 'DmsfFolder', project_id: project.id, dmsf_folder_id: dmsf_folder_id, - deleted: deleted }) + if dmsf_folder_id + scope.where dmsf_links: { target_type: 'DmsfFolder', dmsf_folder_id: dmsf_folder_id, deleted: deleted } + else + scope.where dmsf_links: { target_type: 'DmsfFolder', project_id: project.id, dmsf_folder_id: nil, deleted: deleted } + end end end @@ -257,9 +264,14 @@ class DmsfQuery < Query joins('LEFT JOIN users ON dmsf_file_revisions.user_id = users.id '). where('dmsf_file_revisions.created_at = (SELECT MAX(r.created_at) FROM dmsf_file_revisions r WHERE r.dmsf_file_id = dmsf_file_revisions.dmsf_file_id)') if deleted - scope.where(dmsf_files: { project_id: project.id, deleted: deleted }) + scope.where dmsf_files: { project_id: project.id, deleted: deleted } else - scope.where(dmsf_files: { project_id: project.id, dmsf_folder_id: dmsf_folder_id, deleted: deleted }) + # Consider files belonging to the folder but with wrong project (#1106) + if dmsf_folder_id + scope.where dmsf_files: { dmsf_folder_id: dmsf_folder_id, deleted: deleted } + else + scope.where dmsf_files: { project_id: project.id, dmsf_folder_id: nil, deleted: deleted } + end end end @@ -272,7 +284,7 @@ class DmsfQuery < Query select(%{ dmsf_links.id AS id, dmsf_files.project_id AS project_id, - dmsf_file_revisions.id AS revision_id, + dmsf_files.id AS revision_id, dmsf_links.name AS title, dmsf_file_revisions.name AS filename, dmsf_file_revisions.size AS size, @@ -292,9 +304,13 @@ class DmsfQuery < Query joins('LEFT JOIN users ON dmsf_file_revisions.user_id = users.id '). where('dmsf_file_revisions.created_at = (SELECT MAX(r.created_at) FROM dmsf_file_revisions r WHERE r.dmsf_file_id = dmsf_file_revisions.dmsf_file_id)') if deleted - scope.where(project_id: project.id, deleted: deleted) + scope.where project_id: project.id, deleted: deleted else - scope.where(project_id: project.id, dmsf_folder_id: dmsf_folder_id, deleted: deleted) + if dmsf_folder_id + scope.where dmsf_folder_id: dmsf_folder_id, deleted: deleted + else + scope.where project_id: project.id, dmsf_folder_id: nil, deleted: deleted + end end end @@ -324,9 +340,13 @@ class DmsfQuery < Query 1 AS sort #{cf_columns}}). joins('LEFT JOIN users ON dmsf_links.user_id = users.id ') if deleted - scope.where(target_type: 'DmsfUrl', project_id: project.id, deleted: deleted) + scope.where target_type: 'DmsfUrl', project_id: project.id, deleted: deleted else - scope.where(target_type: 'DmsfUrl', project_id: project.id, dmsf_folder_id: dmsf_folder_id, deleted: deleted) + if dmsf_folder_id + scope.where target_type: 'DmsfUrl', dmsf_folder_id: dmsf_folder_id, deleted: deleted + else + scope.where target_type: 'DmsfUrl', project_id: project.id, dmsf_folder_id: nil, deleted: deleted + end end end diff --git a/app/views/dmsf/edit.html.erb b/app/views/dmsf/edit.html.erb index 0045cbd2..e9acefa6 100644 --- a/app/views/dmsf/edit.html.erb +++ b/app/views/dmsf/edit.html.erb @@ -47,11 +47,6 @@ class: 'icon icon-email-add') %> <% end %> <% end %> - <%= link_to(l(:label_link_to), - new_dmsf_link_path(project_id: @project.id, dmsf_folder_id: @folder.id, type: 'link_to'), - title: l(:title_create_link), class: 'icon icon-link') %> - <%= link_to("#{l(:button_copy)}/#{l(:button_move)}", copy_folder_path(id: @folder), - title: l(:title_copy), class: 'icon icon-copy') %> <% unless @folder.locked? %> <%= link_to(l(:button_delete), delete_dmsf_path(id: @project, folder_id: @folder), data: { confirm: l(:text_are_you_sure) }, diff --git a/app/views/dmsf/show.html.erb b/app/views/dmsf/show.html.erb index af97ffff..7606293c 100644 --- a/app/views/dmsf/show.html.erb +++ b/app/views/dmsf/show.html.erb @@ -25,7 +25,7 @@ <% html_title l(:dmsf) %>
<% if @file_manipulation_allowed && !@locked_for_user && !@system_folder %> - <%= link_to l(:label_attachment_new), multi_dmsf_upload_path(id: @project, dmsf_folder_id: @folder), + <%= link_to l(:label_attachment_new), multi_dmsf_upload_path(id: @project, folder_id: @folder), class: 'icon icon-add' %> <% end %> <% if defined?(EasyExtensions) %> diff --git a/app/views/dmsf_context_menus/_folder.html.erb b/app/views/dmsf_context_menus/_folder.html.erb index c7719966..9933ffae 100644 --- a/app/views/dmsf_context_menus/_folder.html.erb +++ b/app/views/dmsf_context_menus/_folder.html.erb @@ -22,36 +22,40 @@
  • <%= context_menu_link l(:button_edit), edit_dmsf_path(id: project, folder_id: dmsf_folder), - class: 'icon icon-edit', - disabled: !allowed || locked %> + class: 'icon icon-edit', disabled: !allowed || locked %> +
    • +
  • + <%= context_menu_link "#{l(:button_copy)}/#{l(:button_move)}", copy_folder_path(id: dmsf_folder), + class: 'icon icon-copy', disabled: !allowed || locked %> +
    • +
  • + <%= context_menu_link l(:label_link_to), + new_dmsf_link_path(project_id: project.id, dmsf_folder_id: dmsf_folder.id, type: 'link_to'), + class: 'icon icon-link', disabled: !allowed || locked %>
  • <% if locked %> <%= context_menu_link l(:button_unlock), unlock_dmsf_path(id: project, folder_id: dmsf_folder), - class: 'icon icon-unlock', - disabled: !allowed || !unlockable %> + class: 'icon icon-unlock', disabled: !allowed || !unlockable %> <% else %> <%= context_menu_link l(:button_lock), lock_dmsf_path(id: project, folder_id: dmsf_folder), - class: 'icon icon-lock', - disabled: !allowed %> + class: 'icon icon-lock', disabled: !allowed %> <% end %>
  • <% if dmsf_folder.notification %> <%= context_menu_link l(:label_notifications_off), notify_deactivate_dmsf_path(id: project, folder_id: dmsf_folder), - class: 'icon icon-email', - disabled: !allowed || locked || !dmsf_folder.notification? %> + class: 'icon icon-email', disabled: !allowed || locked || !dmsf_folder.notification? %> <% else %> <%= context_menu_link l(:label_notifications_on), notify_activate_dmsf_path(id: project, folder_id: dmsf_folder), - class: 'icon icon-email-add', - disabled: !allowed || locked || dmsf_folder.notification? %> + class: 'icon icon-email-add', disabled: !allowed || locked || dmsf_folder.notification? %> <% end %>
  • <%= context_menu_link l(:button_delete), dmsf_link ? dmsf_link_path(dmsf_link) : delete_dmsf_path(id: project, folder_id: dmsf_folder), data: { confirm: l(:text_are_you_sure) }, method: :delete, class: 'icon icon-del', id: 'dmsf-cm-delete', - disabled: !allowed || locked || !dmsf_folder.empty? %> + disabled: !allowed || locked || (dmsf_link ? false : !dmsf_folder.empty?) %>
  • <%= context_menu_link l(:button_download), entries_operations_dmsf_path(id: project, folder_id: folder, diff --git a/app/views/dmsf_links/_form.html.erb b/app/views/dmsf_links/_form.html.erb index 2d1b5277..74b3249b 100644 --- a/app/views/dmsf_links/_form.html.erb +++ b/app/views/dmsf_links/_form.html.erb @@ -32,8 +32,8 @@
    <% if (@type == 'link_from') && !@container %>

    - <%= radio_button_tag(:external_link, 'false', true) %> <%= l(:label_internal) %>
    - <%= radio_button_tag(:external_link, 'true', false) %> <%= l(:label_external) %> + <%= radio_button_tag :external_link, 'false', true %> <%= l(:label_internal) %>
    + <%= radio_button_tag :external_link, 'true', false %> <%= l(:label_external) %>

    <% end %> diff --git a/config/routes.rb b/config/routes.rb index 4f2d3d0a..234dbec4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -72,6 +72,7 @@ if Redmine::Plugin.installed? :redmine_dmsf get '/projects/:id/dmsf/upload/multi_upload', controller: 'dmsf_upload', action: 'multi_upload', as: 'multi_dmsf_upload' post '/projects/:id/dmsf/upload/files', controller: 'dmsf_upload', action: 'upload_files' + get '/projects/:id/dmsf/upload/files', controller: 'dmsf_upload', action: 'upload_files' post '/projects/:id/dmsf/upload/file', controller: 'dmsf_upload', action: 'upload_file' post '/projects/:id/dmsf/upload', controller: 'dmsf_upload', action: 'upload' post '/projects/:id/dmsf/upload/commit', controller: 'dmsf_upload', action: 'commit_files' diff --git a/db/migrate/20200423071301_add_indexes_on_dmsf_folder_id.rb b/db/migrate/20200423071301_add_indexes_on_dmsf_folder_id.rb new file mode 100644 index 00000000..26a0e9d1 --- /dev/null +++ b/db/migrate/20200423071301_add_indexes_on_dmsf_folder_id.rb @@ -0,0 +1,28 @@ +# encoding: utf-8 +# +# Redmine plugin for Document Management System "Features" +# +# Copyright © 2011-20 Karel Pičman +# +# 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. + +class AddIndexesOnDmsfFolderId < ActiveRecord::Migration[5.2] + + def change + add_index :dmsf_files, :dmsf_folder_id + add_index :dmsf_links, :dmsf_folder_id + end + +end diff --git a/lib/redmine_dmsf/patches/queries_helper_patch.rb b/lib/redmine_dmsf/patches/queries_helper_patch.rb index 2b4b6998..b019defd 100644 --- a/lib/redmine_dmsf/patches/queries_helper_patch.rb +++ b/lib/redmine_dmsf/patches/queries_helper_patch.rb @@ -76,7 +76,7 @@ module RedmineDmsf else tag = "".html_safe + link_to(h(value), - dmsf_folder_path(id: item.project_id, folder_id: item.id), + dmsf_folder_path(id: item.project, folder_id: item.id), class: 'icon icon-folder', title: h(value)) end @@ -86,8 +86,9 @@ module RedmineDmsf tag = content_tag('span', value, class: 'icon icon-folder') else tag = "".html_safe + + # For links we use revision_id containing dmsf_folder.id in fact link_to(h(value), - dmsf_folder_path(id: item.project_id, folder_id: item.id), + dmsf_folder_path(id: item.project, folder_id: item.revision_id), class: 'icon icon-folder', title: h(value)) end @@ -96,7 +97,8 @@ module RedmineDmsf if item.deleted && (item.deleted > 0) tag = content_tag('span', value, class: "icon icon-file #{DmsfHelper.filetype_css(item.filename)}") else - file_view_url = url_for({ controller: :dmsf_files, action: 'view', id: item.id }) + # For links we use revision_id containing dmsf_file.id in fact + file_view_url = url_for({ controller: :dmsf_files, action: 'view', id: (item.type == 'file') ? item.id : item.revision_id }) content_type = Redmine::MimeType.of(value) content_type = 'application/octet-stream' if content_type.blank? tag = "".html_safe + diff --git a/lib/redmine_dmsf/webdav/dmsf_resource.rb b/lib/redmine_dmsf/webdav/dmsf_resource.rb index 30e42935..63f24460 100644 --- a/lib/redmine_dmsf/webdav/dmsf_resource.rb +++ b/lib/redmine_dmsf/webdav/dmsf_resource.rb @@ -31,7 +31,7 @@ module RedmineDmsf def initialize(path, request, response, options) @folder = nil @file = nil - super(path, request, response, options) + super path, request, response, options end # Here we make sure our folder and file methods are not aliased - it should shave a few cycles off of processing @@ -227,64 +227,52 @@ module RedmineDmsf resource = dest.is_a?(ResourceProxy) ? dest.resource : dest return PreconditionFailed if !resource.is_a?(DmsfResource) || resource.project.nil? parent = resource.parent - raise Forbidden unless (!parent.exist? || !parent.folder || DmsfFolder.permissions?(parent.folder, false)) - + if !parent.exist? || (!User.current.admin? && (!DmsfFolder.permissions?(folder, false) || + !DmsfFolder.permissions?(parent.folder, false))) + raise Forbidden + end if collection? - if dest.exist? return overwrite ? NotImplemented : PreconditionFailed end - - # At the moment we don't support cross project destinations - return NotImplemented unless (project.id == resource.project.id) - raise Forbidden unless User.current.admin? || User.current.allowed_to?(:folder_manipulation, project) - raise Forbidden unless DmsfFolder.permissions?(folder, false) - + if !User.current.admin? && (!User.current.allowed_to?(:folder_manipulation, project) || + !User.current.allowed_to?(:folder_manipulation, resource.project)) + raise Forbidden + end # Current object is a folder, so now we need to figure out information about Destination if dest.exist? return overwrite ? NotImplemented : PreconditionFailed else - if parent.projectless_path == '/' # Project root - folder.dmsf_folder_id = nil - else - return PreconditionFailed unless parent.exist? && parent.folder - folder.dmsf_folder_id = parent.folder.id - end - folder.title = resource.basename - folder.save ? Created : PreconditionFailed + folder.move_to(resource.project, parent.folder) ? Created : PreconditionFailed end else - raise Forbidden unless User.current.admin? || - User.current.allowed_to?(:folder_manipulation, project) || - User.current.allowed_to?(:folder_manipulation, resource.project) - + if !User.current.admin? && (!User.current.allowed_to?(:file_manipulation, project) || + !User.current.allowed_to?(:file_manipulation, resource.project)) + raise Forbidden + end if dest.exist? return PreconditionFailed unless overwrite if (project == resource.project) && file.name.match(/.\.tmp$/i) # Renaming a *.tmp file to an existing file in the same project, probably Office that is saving a file. Rails.logger.info "WebDAV MOVE: #{file.name} -> #{resource.basename} (exists), possible MSOffice rename from .tmp when saving" - if resource.file.last_revision.size == 0 || reuse_version_for_locked_file(resource.file) # Last revision in the destination has zero size so reuse that revision new_revision = resource.file.last_revision else # Create a new revison by cloning the last revision in the destination new_revision = resource.file.last_revision.clone - new_revision.increase_version(1) + new_revision.increase_version 1 end - # The file on disk must be renamed from .tmp to the correct filetype or else Xapian won't know how to index. # Copy file.last_revision.disk_file to new_revision.disk_file new_revision.size = file.last_revision.size new_revision.disk_filename = new_revision.new_storage_filename Rails.logger.info "WebDAV MOVE: Copy file #{file.last_revision.disk_filename} -> #{new_revision.disk_filename}" File.open(file.last_revision.disk_file, 'rb') do |f| - new_revision.copy_file_content(f) + new_revision.copy_file_content f end - # Save new_revision.save && resource.file.save - # Delete (and destroy) the file that should have been renamed and return what should have been returned in case of a copy file.delete(true) ? Created : PreconditionFailed else @@ -300,11 +288,10 @@ module RedmineDmsf f = parent.folder end return PreconditionFailed unless exist? && file - if (project == resource.project) && resource.basename.match(/.\.tmp$/i) Rails.logger.info "WebDAV MOVE: #{file.name} -> #{resource.basename}, possible MSOffice rename to .tmp when saving." # Renaming the file to X.tmp, might be Office that is saving a file. Keep the original file. - file.copy_to_filename(resource.project, f, resource.basename) + file.copy_to_filename resource.project, f, resource.basename Created else if (project == resource.project) && (file.last_revision.size == 0) @@ -313,14 +300,12 @@ module RedmineDmsf else return InternalServerError unless file.move_to(resource.project, f) end - # Update Revision and names of file [We can link to old physical resource, as it's not changed] if file.last_revision file.last_revision.name = resource.basename file.last_revision.title = DmsfFileRevision.filename_to_title(resource.basename) end file.name = resource.basename - # Save Changes (file.last_revision.save && file.save) ? Created : PreconditionFailed end diff --git a/test/fixtures/dmsf_file_revisions.yml b/test/fixtures/dmsf_file_revisions.yml index 7b0c218c..003f7af5 100644 --- a/test/fixtures/dmsf_file_revisions.yml +++ b/test/fixtures/dmsf_file_revisions.yml @@ -190,3 +190,25 @@ dmsf_file_revisions_009: dmsf_workflow_assigned_by_user_id: NULL dmsf_workflow_started_by_user_id: NULL created_at: 2017-04-18 14:52:27 +02:00 + +dmsf_file_revisions_010: + id: 10 + dmsf_file_id: 5 + source_dmsf_file_revision_id: NULL + name: "test.txt" + disk_filename: "test.txt" + size: 4 + mime_type: text/plain + title: "Test File" + description: 'Some file :-)' + workflow: 1 # DmsfWorkflow::STATE_WAITING_FOR_APPROVAL + minor_version: 0 + major_version: 1 + comment: NULL + deleted: 0 + deleted_by_user_id: NULL + user_id: 1 + dmsf_workflow_assigned_by_user_id: 1 + dmsf_workflow_started_by_user_id: 1 + digest: '81dc9bdb52d04dc20036dbd8313ed055' + created_at: 2017-04-18 14:52:27 +02:00 \ No newline at end of file diff --git a/test/functional/dmsf_controller_test.rb b/test/functional/dmsf_controller_test.rb index 3f647c1b..deb4ae6c 100644 --- a/test/functional/dmsf_controller_test.rb +++ b/test/functional/dmsf_controller_test.rb @@ -253,9 +253,10 @@ class DmsfControllerTest < RedmineDmsf::Test::TestCase def test_show_folder_doesnt_correspond_the_project @role.add_permission! :view_dmsf_files @role.add_permission! :view_dmsf_folders - # project1 X project2.folder3 + # Despite the fact that project != @folder3.project + assert @project != @folder3.project get :show, params: { id: @project.id, folder_id: @folder3.id } - assert_response :not_found + assert_response :success end def test_new_forbidden diff --git a/test/functional/dmsf_folders_copy_controller_test.rb b/test/functional/dmsf_folders_copy_controller_test.rb index bb665663..0f433691 100644 --- a/test/functional/dmsf_folders_copy_controller_test.rb +++ b/test/functional/dmsf_folders_copy_controller_test.rb @@ -170,6 +170,12 @@ class DmsfFoldersCopyControllerTest < RedmineDmsf::Test::TestCase assert_nil flash[:error] end + def test_move_to_another_project + post :move, params: { id: @folder1.id, target_project_id: @project2.id, target_folder_id: 'Documents' } + assert_response :redirect + assert_nil flash[:error] + end + def test_move_the_same_target post :move, params: { id: @folder1.id, target_project_id: @folder1.project.id, target_folder_id: @folder1.dmsf_folder } assert_equal flash[:error], l(:error_target_folder_same) diff --git a/test/functional/dmsf_links_controller_test.rb b/test/functional/dmsf_links_controller_test.rb index dacda952..b05898a0 100644 --- a/test/functional/dmsf_links_controller_test.rb +++ b/test/functional/dmsf_links_controller_test.rb @@ -322,7 +322,7 @@ class DmsfLinksControllerTest < RedmineDmsf::Test::TestCase type: 'link_to' }} end - assert_redirected_to edit_dmsf_path(id: @project1.id, folder_id: @folder1.id) + assert_redirected_to dmsf_folder_path(id: @project1, folder_id: @folder1.dmsf_folder) end def test_create_folder_link_to_f2 @@ -337,7 +337,7 @@ class DmsfLinksControllerTest < RedmineDmsf::Test::TestCase type: 'link_to' }} end - assert_redirected_to edit_dmsf_path(id: @project1.id, folder_id: @folder1.id) + assert_redirected_to dmsf_folder_path(id: @project1, folder_id: @folder1.dmsf_folder) end def test_destroy diff --git a/test/integration/webdav/dmsf_webdav_move_test.rb b/test/integration/webdav/dmsf_webdav_move_test.rb index e806441e..37782cad 100644 --- a/test/integration/webdav/dmsf_webdav_move_test.rb +++ b/test/integration/webdav/dmsf_webdav_move_test.rb @@ -37,13 +37,16 @@ class DmsfWebdavMoveTest < RedmineDmsf::Test::IntegrationTest @jsmith_user = User.find_by(login: 'jsmith') @admin_user = User.find_by(login: 'admin') @project1 = Project.find 1 + @project1.enable_module! :dmsf + @project2 = Project.find 2 + @project2.enable_module! :dmsf @file1 = DmsfFile.find 1 @file10 = DmsfFile.find 10 @folder1 = DmsfFolder.find 1 - # Fix permissions for jsmith's role - @role = Role.find 1 # + @role = Role.find 1 @role.add_permission! :view_dmsf_folders @role.add_permission! :folder_manipulation + @role.add_permission! :file_manipulation @dmsf_webdav = Setting.plugin_redmine_dmsf['dmsf_webdav'] Setting.plugin_redmine_dmsf['dmsf_webdav'] = true @dmsf_webdav_strategy = Setting.plugin_redmine_dmsf['dmsf_webdav_strategy'] @@ -68,6 +71,7 @@ class DmsfWebdavMoveTest < RedmineDmsf::Test::IntegrationTest def test_truth assert_kind_of Project, @project1 + assert_kind_of Project, @project2 assert_kind_of Role, @role assert_kind_of DmsfFile, @file1 assert_kind_of DmsfFile, @file10 @@ -85,8 +89,8 @@ class DmsfWebdavMoveTest < RedmineDmsf::Test::IntegrationTest end end - def test_move_to_new_filename_without_folder_manipulation_permission - @role.remove_permission! :folder_manipulation + def test_move_to_new_filename_without_file_manipulation_permission + @role.remove_permission! :file_manipulation new_name = "#{@file1.name}.moved" assert_no_difference '@file1.dmsf_file_revisions.count' do process :move, "/dmsf/webdav/#{@project1.identifier}/#{@file1.name}", params: nil, @@ -95,8 +99,8 @@ class DmsfWebdavMoveTest < RedmineDmsf::Test::IntegrationTest end end - def test_move_to_new_filename_without_folder_manipulation_permission_as_admin - @role.remove_permission! :folder_manipulation + def test_move_to_new_filename_without_file_manipulation_permission_as_admin + @role.remove_permission! :file_manipulation new_name = "#{@file1.name}.moved" assert_difference '@file1.dmsf_file_revisions.count', +1 do process :move, "/dmsf/webdav/#{@project1.identifier}/#{@file1.name}", params: nil, @@ -107,6 +111,37 @@ class DmsfWebdavMoveTest < RedmineDmsf::Test::IntegrationTest end end + def test_without_folder_manipulation_permission + @role.remove_permission! :folder_manipulation + new_name = "#{@folder1.title}.moved" + process :move, "/dmsf/webdav/#{@project1.identifier}/#{@folder1.title}", params: nil, + headers: @jsmith.merge!({ destination: "http://www.example.com/dmsf/webdav/#{@project1.identifier}/#{new_name}" }) + assert_response :forbidden + end + + def test_without_folder_manipulation_permission_as_admin + @role.remove_permission! :folder_manipulation + new_name = "#{@folder1.title}.moved" + process :move, "/dmsf/webdav/#{@project1.identifier}/#{@folder1.title}", params: nil, + headers: @admin.merge!({ destination: "http://www.example.com/dmsf/webdav/#{@project1.identifier}/#{new_name}" }) + assert_response :created + end + + def test_move_folder_to_another_project + process :move, "/dmsf/webdav/#{@project1.identifier}/#{@folder1.title}", params: nil, + headers: @admin.merge!({ destination: "http://www.example.com/dmsf/webdav/#{@project2.identifier}/#{@folder1.title}" }) + assert_response :created + @folder1.dmsf_folders.each do |d| + assert_equal @project2, d.project + end + @folder1.dmsf_files.each do |f| + assert_equal @project2, f.project + end + @folder1.dmsf_links.each do |l| + assert_equal @project2, l.project + end + end + def test_move_non_existent_file process :move, "/dmsf/webdav/#{@project1.identifier}/not_a_file.txt", params: nil, headers: @jsmith.merge!({ destination: "http://www.example.com/dmsf/webdav/#{@project1.identifier}/moved_file.txt" }) @@ -242,7 +277,7 @@ class DmsfWebdavMoveTest < RedmineDmsf::Test::IntegrationTest assert_response :success # Created end end - + def test_move_msoffice_save_locked_file # When some versions of MsOffice save a file they use the following sequence: # 1. Save changes to a new temporary document, XXX.tmp diff --git a/test/unit/dmsf_folder_test.rb b/test/unit/dmsf_folder_test.rb index e9684353..257373d4 100644 --- a/test/unit/dmsf_folder_test.rb +++ b/test/unit/dmsf_folder_test.rb @@ -27,8 +27,10 @@ class DmsfFolderTest < RedmineDmsf::Test::UnitTest :dmsf_folder_permissions def setup - @project = Project.find 1 - @project.enable_module! :dmsf + @project1 = Project.find 1 + @project1.enable_module! :dmsf + @project2 = Project.find 2 + @project2.enable_module! :dmsf @folder1 = DmsfFolder.find 1 @folder2 = DmsfFolder.find 2 @folder4 = DmsfFolder.find 4 @@ -51,7 +53,8 @@ class DmsfFolderTest < RedmineDmsf::Test::UnitTest assert_kind_of DmsfFolder, @folder5 assert_kind_of DmsfFolder, @folder6 assert_kind_of DmsfFolder, @folder7 - assert_kind_of Project, @project + assert_kind_of Project, @project1 + assert_kind_of Project, @project2 assert_kind_of User, @manager assert_kind_of User, @developer assert_kind_of Role, @manager_role @@ -69,7 +72,7 @@ class DmsfFolderTest < RedmineDmsf::Test::UnitTest assert_equal 4, DmsfFolder.visible.where(project_id: 1).all.size # Anonymous user User.current = User.anonymous - @project.add_default_member User.anonymous + @project1.add_default_member User.anonymous assert_equal 5, DmsfFolder.visible.where(project_id: 1).all.size end @@ -145,7 +148,7 @@ class DmsfFolderTest < RedmineDmsf::Test::UnitTest end def test_directory_tree - tree = DmsfFolder.directory_tree(@project) + tree = DmsfFolder.directory_tree(@project1) assert tree # [["Documents", nil], # ["...folder7", 7], @@ -157,7 +160,7 @@ class DmsfFolderTest < RedmineDmsf::Test::UnitTest end def test_directory_tree_id - tree = DmsfFolder.directory_tree(@project.id) + tree = DmsfFolder.directory_tree(@project1.id) assert tree # [["Documents", nil], # ["...folder7", 7], @@ -193,4 +196,23 @@ class DmsfFolderTest < RedmineDmsf::Test::UnitTest assert_equal 1, users.size end + def test_move_to + assert @folder1.move_to(@project2, nil) + assert_equal @project2, @folder1.project + @folder1.dmsf_folders.each do |d| + assert_equal @project2, d.project + end + @folder1.dmsf_files.each do |f| + assert_equal @project2, f.project + end + @folder1.dmsf_links.each do |l| + assert_equal @project2, l.project + end + end + + def test_copy_to + assert @folder1.copy_to(@project2, nil) + assert DmsfFolder.find_by_title(@project2, nil, @folder1.title) + end + end \ No newline at end of file diff --git a/test/unit/dmsf_link_test.rb b/test/unit/dmsf_link_test.rb index 5e00231d..d628453f 100644 --- a/test/unit/dmsf_link_test.rb +++ b/test/unit/dmsf_link_test.rb @@ -43,10 +43,10 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest assert_kind_of DmsfFile, @file1 assert_kind_of DmsfFile, @file4 assert_kind_of DmsfLink, @folder_link - assert_kind_of DmsfLink, @file_link + assert_kind_of DmsfLink, @file_link end - - def test_create_folder_link + + def test_create_folder_link folder_link = DmsfLink.new folder_link.target_project_id = @project1.id folder_link.target_id = @folder1.id @@ -57,7 +57,7 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest folder_link.updated_at = DateTime.current assert folder_link.save, folder_link.errors.full_messages.to_sentence end - + def test_create_file_link file_link = DmsfLink.new file_link.target_project_id = @project1.id @@ -69,7 +69,7 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest file_link.updated_at = DateTime.current assert file_link.save, file_link.errors.full_messages.to_sentence end - + def test_create_external_link external_link = DmsfLink.new external_link.target_project_id = @project1.id @@ -81,25 +81,25 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest external_link.updated_at = DateTime.current assert external_link.save, external_link.errors.full_messages.to_sentence end - + def test_validate_name_length @folder_link.name = 'a' * 256 - assert !@folder_link.save, + assert !@folder_link.save, "Folder link #{@folder_link.name} should have not been saved" assert_equal 1, @folder_link.errors.size end - + def test_validate_name_presence @folder_link.name = '' assert !@folder_link.save, "Folder link #{@folder_link.name} should have not been saved" assert_equal 1, @folder_link.errors.size end - + def test_validate_external_url_length @file_link.target_type = 'DmsfUrl' @file_link.external_url = 'https://localhost/' + 'a' * 256 - assert !@file_link.save, + assert !@file_link.save, "External URL link #{@file_link.name} should have not been saved" assert_equal 1, @file_link.errors.size end @@ -124,7 +124,7 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest @file_link.external_url = 'https://www.google.com/search?q=寿司' assert @file_link.save end - + def test_belongs_to_project @project1.destroy assert_nil DmsfLink.find_by(id: 1) @@ -136,53 +136,53 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest assert_nil DmsfLink.find_by(id: 1) assert_nil DmsfLink.find_by(id: 2) end - + def test_target_folder_id assert_equal 2, @file_link.target_folder_id - assert_equal 1, @folder_link.target_folder_id + assert_equal 1, @folder_link.target_folder_id end - + def test_target_folder assert_equal @folder2, @file_link.target_folder assert_equal @folder1, @folder_link.target_folder end - + def test_target_file_id assert_equal 4, @file_link.target_file_id assert_nil @folder_link.target_file end - + def test_target_file assert_equal @file4, @file_link.target_file assert_nil @folder_link.target_file end - + def test_target_project assert_equal @project1, @file_link.target_project assert_equal @project1, @folder_link.target_project end - + def test_folder - assert_equal @folder1, @file_link.folder - assert_nil @folder_link.folder + assert_equal @folder1, @file_link.dmsf_folder + assert_nil @folder_link.dmsf_folder end - + def test_title assert_equal @file_link.name, @file_link.title assert_equal @folder_link.name, @folder_link.title end - - def test_find_link_by_file_name - file_link = DmsfLink.find_link_by_file_name(@file_link.project, - @file_link.folder, @file_link.target_file.name) + + def test_find_link_by_file_name + file_link = DmsfLink.find_link_by_file_name(@file_link.project, + @file_link.dmsf_folder, @file_link.target_file.name) assert file_link, 'File link not found by its name' end - + def test_path assert_equal 'folder1/folder2/test4.txt', @file_link.path assert_equal 'folder1', @folder_link.path end - + def test_file_kink_copy_to file_link_copy = @file_link.copy_to @folder2.project, @folder2 assert_not_nil file_link_copy, 'File link copying failed' @@ -191,54 +191,54 @@ class DmsfLinksTest < RedmineDmsf::Test::UnitTest assert_equal file_link_copy.target_type, @file_link.target_type assert_equal file_link_copy.name, @file_link.name assert_equal file_link_copy.project_id, @folder2.project.id - assert_equal file_link_copy.dmsf_folder_id, @folder2.id + assert_equal file_link_copy.dmsf_folder_id, @folder2.id end - + def test_folder_link_copy_to folder_link_copy = @folder_link.copy_to @folder2.project, @folder2 assert_not_nil folder_link_copy, 'Folder link copying failed' - assert_equal folder_link_copy.target_project_id, + assert_equal folder_link_copy.target_project_id, @folder_link.target_project_id assert_equal folder_link_copy.target_id, @folder_link.target_id assert_equal folder_link_copy.target_type, @folder_link.target_type assert_equal folder_link_copy.name, @folder_link.name assert_equal folder_link_copy.project_id, @folder2.project.id - assert_equal folder_link_copy.dmsf_folder_id, @folder2.id + assert_equal folder_link_copy.dmsf_folder_id, @folder2.id end - - def test_delete_file_link + + def test_delete_file_link assert @file_link.delete(false), @file_link.errors.full_messages.to_sentence assert @file_link.deleted?, "File link hasn't been deleted" end - + def test_restore_file_link assert @file_link.delete(false), @file_link.errors.full_messages.to_sentence assert @file_link.deleted?, "File link hasn't been deleted" assert @file_link.restore, @file_link.errors.full_messages.to_sentence assert !@file_link.deleted?, "File link hasn't been restored" end - + def test_delete_folder_link - assert @folder_link.delete(false), + assert @folder_link.delete(false), @folder_link.errors.full_messages.to_sentence assert @folder_link.deleted?, "Folder link hasn't been deleted" end - + def test_restore_folder_link - assert @folder_link.delete(false), + assert @folder_link.delete(false), @folder_link.errors.full_messages.to_sentence assert @folder_link.deleted?, "Folder link hasn't been deleted" assert @folder_link.restore, @folder_link.errors.full_messages.to_sentence assert !@folder_link.deleted?, "Folder link hasn't been restored" end - - def test_destroy_file_link + + def test_destroy_file_link assert @file_link.delete(true), @file_link.errors.full_messages.to_sentence assert_nil DmsfLink.find_by(id: @file_link.id) end - + def test_destroy_folder_link - assert @folder_link.delete(true), + assert @folder_link.delete(true), @folder_link.errors.full_messages.to_sentence assert_nil DmsfLink.find_by(id: @folder_link.id) end