From 2d118da55c3ba674b541b478bdc6a54c7be714f9 Mon Sep 17 00:00:00 2001 From: Gregor Schmidt Date: Tue, 20 Feb 2018 15:54:37 +0100 Subject: [PATCH 1/3] Generalize issues imports Extend import controller to support arbitrary imports based on Import subclasses. This way, we may add other kinds of imports, by providing some views and a custom import class. This may also be done from within plugins. --- app/controllers/imports_controller.rb | 44 ++++++++++++++++++++-- app/helpers/imports_helper.rb | 8 ++++ app/models/import.rb | 12 ++++++ app/models/issue_import.rb | 7 ++++ ...ng.html.erb => _issues_fields_mapping.html.erb} | 2 +- app/views/imports/_issues_mapping.html.erb | 16 ++++++++ app/views/imports/_issues_mapping.js.erb | 1 + app/views/imports/_issues_saved_objects.html.erb | 7 ++++ app/views/imports/_issues_sidebar.html.erb | 3 ++ app/views/imports/mapping.html.erb | 23 ++--------- app/views/imports/mapping.js.erb | 2 +- app/views/imports/new.html.erb | 7 ++-- app/views/imports/run.html.erb | 6 +-- app/views/imports/settings.html.erb | 6 +-- app/views/imports/show.html.erb | 14 ++----- config/routes.rb | 2 +- lib/redmine.rb | 2 +- test/functional/imports_controller_test.rb | 3 +- test/integration/routing/imports_test.rb | 3 +- test/unit/issue_import_test.rb | 6 +++ 20 files changed, 121 insertions(+), 53 deletions(-) rename app/views/imports/{_fields_mapping.html.erb => _issues_fields_mapping.html.erb} (97%) create mode 100644 app/views/imports/_issues_mapping.html.erb create mode 100644 app/views/imports/_issues_mapping.js.erb create mode 100644 app/views/imports/_issues_saved_objects.html.erb create mode 100644 app/views/imports/_issues_sidebar.html.erb diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 661eb7405..271b015cc 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -18,19 +18,20 @@ require 'csv' class ImportsController < ApplicationController - menu_item :issues - before_action :find_import, :only => [:show, :settings, :mapping, :run] - before_action :authorize_global + before_action :authorize_import + + layout :import_layout helper :issues helper :queries def new + @import = import_type.new end def create - @import = IssueImport.new + @import = import_type.new @import.user = User.current @import.file = params[:file] @import.set_default_settings @@ -94,6 +95,14 @@ class ImportsController < ApplicationController end end + def current_menu(project) + if import_layout == 'admin' + nil + else + :application_menu + end + end + private def find_import @@ -119,4 +128,31 @@ class ImportsController < ApplicationController def max_items_per_request 5 end + + def import_layout + import_type && import_type.layout || 'base' + end + + def menu_items + menu_item = import_type ? import_type.menu_item : nil + + { self.controller_name.to_sym => { :actions => {}, :default => menu_item } } + end + + def authorize_import + return render_404 unless import_type + return render_403 unless import_type.authorized?(User.current) + end + + def import_type + return @import_type if defined? @import_type + + @import_type = + if @import + @import.class + else + type = Object.const_get(params[:type]) rescue nil + type && type < Import ? type : nil + end + end end diff --git a/app/helpers/imports_helper.rb b/app/helpers/imports_helper.rb index 39f3b95ab..1a4836519 100644 --- a/app/helpers/imports_helper.rb +++ b/app/helpers/imports_helper.rb @@ -18,6 +18,14 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. module ImportsHelper + def import_title + l(:"label_import_#{import_partial_prefix}") + end + + def import_partial_prefix + @import.class.name.sub('Import', '').underscore.pluralize + end + def options_for_mapping_select(import, field, options={}) tags = "".html_safe blank_text = options[:required] ? "-- #{l(:actionview_instancetag_blank_option)} --" : " ".html_safe diff --git a/app/models/import.rb b/app/models/import.rb index d2c53baac..194f46aa9 100644 --- a/app/models/import.rb +++ b/app/models/import.rb @@ -35,6 +35,18 @@ class Import < ActiveRecord::Base '%d-%m-%Y' ] + def self.menu_item + nil + end + + def self.layout + 'base' + end + + def self.authorized?(user) + user.admin? + end + def initialize(*args) super self.settings ||= {} diff --git a/app/models/issue_import.rb b/app/models/issue_import.rb index ad04c0be5..3e193ee3a 100644 --- a/app/models/issue_import.rb +++ b/app/models/issue_import.rb @@ -16,6 +16,13 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. class IssueImport < Import + def self.menu_item + :issues + end + + def self.authorized?(user) + user.allowed_to?(:import_issues, nil, :global => true) + end # Returns the objects that were imported def saved_objects diff --git a/app/views/imports/_fields_mapping.html.erb b/app/views/imports/_issues_fields_mapping.html.erb similarity index 97% rename from app/views/imports/_fields_mapping.html.erb rename to app/views/imports/_issues_fields_mapping.html.erb index f59350116..542a956ff 100644 --- a/app/views/imports/_fields_mapping.html.erb +++ b/app/views/imports/_issues_fields_mapping.html.erb @@ -54,7 +54,7 @@

<% @custom_fields.each do |field| %>

- + <%= mapping_select_tag @import, "cf_#{field.id}" %>

<% end %> diff --git a/app/views/imports/_issues_mapping.html.erb b/app/views/imports/_issues_mapping.html.erb new file mode 100644 index 000000000..dc97d9c3a --- /dev/null +++ b/app/views/imports/_issues_mapping.html.erb @@ -0,0 +1,16 @@ +
+ <%= l(:label_fields_mapping) %> +
+ <%= render :partial => 'issues_fields_mapping' %> +
+
+ +<%= javascript_tag do %> + $('#fields-mapping').on('change', '#import_mapping_project_id, #import_mapping_tracker', function(){ + $.ajax({ + url: '<%= import_mapping_path(@import, :format => 'js') %>', + type: 'post', + data: $('#import-form').serialize() + }); + }); +<% end %> diff --git a/app/views/imports/_issues_mapping.js.erb b/app/views/imports/_issues_mapping.js.erb new file mode 100644 index 000000000..012ccc537 --- /dev/null +++ b/app/views/imports/_issues_mapping.js.erb @@ -0,0 +1 @@ +$('#fields-mapping').html('<%= escape_javascript(render :partial => 'issues_fields_mapping') %>'); diff --git a/app/views/imports/_issues_saved_objects.html.erb b/app/views/imports/_issues_saved_objects.html.erb new file mode 100644 index 000000000..f708a1c23 --- /dev/null +++ b/app/views/imports/_issues_saved_objects.html.erb @@ -0,0 +1,7 @@ + + +

<%= link_to l(:label_issue_view_all), issues_path(:set_filter => 1, :status_id => '*', :issue_id => saved_objects.map(&:id).join(',')) %>

diff --git a/app/views/imports/_issues_sidebar.html.erb b/app/views/imports/_issues_sidebar.html.erb new file mode 100644 index 000000000..e098175ec --- /dev/null +++ b/app/views/imports/_issues_sidebar.html.erb @@ -0,0 +1,3 @@ +<% content_for :sidebar do %> + <%= render :partial => 'issues/sidebar' %> +<% end %> diff --git a/app/views/imports/mapping.html.erb b/app/views/imports/mapping.html.erb index 2e225d6c2..1822f2802 100644 --- a/app/views/imports/mapping.html.erb +++ b/app/views/imports/mapping.html.erb @@ -1,12 +1,7 @@ -

<%= l(:label_import_issues) %>

+

<%= import_title %>

<%= form_tag(import_mapping_path(@import), :id => "import-form") do %> -
- <%= l(:label_fields_mapping) %> -
- <%= render :partial => 'fields_mapping' %> -
-
+ <%= render :partial => "#{import_partial_prefix}_mapping" %>
@@ -28,25 +23,13 @@

<% end %> -<% content_for :sidebar do %> - <%= render :partial => 'issues/sidebar' %> -<% end %> - +<%= render :partial => "#{import_partial_prefix}_sidebar" %> <%= javascript_tag do %> $(document).ready(function() { - $('#fields-mapping').on('change', '#import_mapping_project_id, #import_mapping_tracker', function(){ - $.ajax({ - url: '<%= import_mapping_path(@import, :format => 'js') %>', - type: 'post', - data: $('#import-form').serialize() - }); - }); - $('#import-form').submit(function(){ $('#import-details').show().addClass('ajax-loading'); $('#import-progress').progressbar({value: 0, max: <%= @import.total_items || 0 %>}); }); - }); <% end %> diff --git a/app/views/imports/mapping.js.erb b/app/views/imports/mapping.js.erb index 8fdf14a36..4a4358427 100644 --- a/app/views/imports/mapping.js.erb +++ b/app/views/imports/mapping.js.erb @@ -1 +1 @@ -$('#fields-mapping').html('<%= escape_javascript(render :partial => 'fields_mapping') %>'); +<%= render :partial => "#{import_partial_prefix}_mapping" %> diff --git a/app/views/imports/new.html.erb b/app/views/imports/new.html.erb index e20be353a..41b27d6c9 100644 --- a/app/views/imports/new.html.erb +++ b/app/views/imports/new.html.erb @@ -1,6 +1,7 @@ -

<%= l(:label_import_issues) %>

+

<%= import_title %>

<%= form_tag(imports_path, :multipart => true) do %> + <%= hidden_field_tag 'type', @import.type %>
<%= l(:label_select_file_to_import) %> (CSV)

@@ -10,6 +11,4 @@

<%= submit_tag l(:label_next).html_safe + " »".html_safe, :name => nil %>

<% end %> -<% content_for :sidebar do %> - <%= render :partial => 'issues/sidebar' %> -<% end %> +<%= render :partial => "#{import_partial_prefix}_sidebar" %> diff --git a/app/views/imports/run.html.erb b/app/views/imports/run.html.erb index 2a723537e..50b47836d 100644 --- a/app/views/imports/run.html.erb +++ b/app/views/imports/run.html.erb @@ -1,12 +1,10 @@ -

<%= l(:label_import_issues) %>

+

<%= import_title %>

0 / <%= @import.total_items.to_i %>
-<% content_for :sidebar do %> - <%= render :partial => 'issues/sidebar' %> -<% end %> +<%= render :partial => "#{import_partial_prefix}_sidebar" %> <%= javascript_tag do %> $(document).ready(function() { diff --git a/app/views/imports/settings.html.erb b/app/views/imports/settings.html.erb index 6edfb10af..0fbb01857 100644 --- a/app/views/imports/settings.html.erb +++ b/app/views/imports/settings.html.erb @@ -1,4 +1,4 @@ -

<%= l(:label_import_issues) %>

+

<%= import_title %>

<%= form_tag(import_settings_path(@import), :id => "import-form") do %>
@@ -25,6 +25,4 @@

<%= submit_tag l(:label_next).html_safe + " »".html_safe, :name => nil %>

<% end %> -<% content_for :sidebar do %> - <%= render :partial => 'issues/sidebar' %> -<% end %> +<%= render :partial => "#{import_partial_prefix}_sidebar" %> diff --git a/app/views/imports/show.html.erb b/app/views/imports/show.html.erb index 19c874c91..ca963ab37 100644 --- a/app/views/imports/show.html.erb +++ b/app/views/imports/show.html.erb @@ -1,15 +1,9 @@ -

<%= l(:label_import_issues) %>

+

<%= import_title %>

<% if @import.saved_items.count > 0 %>

<%= l(:notice_import_finished, :count => @import.saved_items.count) %>:

-
    - <% @import.saved_objects.each do |issue| %> -
  • <%= link_to_issue issue %>
    • - <% end %> -
- -

<%= link_to l(:label_issue_view_all), issues_path(:set_filter => 1, :status_id => '*', :issue_id => @import.saved_objects.map(&:id).join(',')) %>

+ <%= render :partial => "#{import_partial_prefix}_saved_objects", :locals => { saved_objects: @import.saved_objects } %> <% end %> <% if @import.unsaved_items.count > 0 %> @@ -33,6 +27,4 @@ <% end %> -<% content_for :sidebar do %> - <%= render :partial => 'issues/sidebar' %> -<% end %> +<%= render :partial => "#{import_partial_prefix}_sidebar" %> diff --git a/config/routes.rb b/config/routes.rb index e5b25f904..4e18fd4fe 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -63,7 +63,7 @@ Rails.application.routes.draw do get 'projects/:id/issues/report', :to => 'reports#issue_report', :as => 'project_issues_report' get 'projects/:id/issues/report/:detail', :to => 'reports#issue_report_details', :as => 'project_issues_report_details' - get '/issues/imports/new', :to => 'imports#new', :as => 'new_issues_import' + get '/issues/imports/new', :to => 'imports#new', :defaults => { :type => 'IssueImport' }, :as => 'new_issues_import' post '/imports', :to => 'imports#create', :as => 'imports' get '/imports/:id', :to => 'imports#show', :as => 'import' match '/imports/:id/settings', :to => 'imports#settings', :via => [:get, :post], :as => 'import_settings' diff --git a/lib/redmine.rb b/lib/redmine.rb index 1937ba8ee..54e63a4d6 100644 --- a/lib/redmine.rb +++ b/lib/redmine.rb @@ -115,7 +115,7 @@ Redmine::AccessControl.map do |map| map.permission :view_issue_watchers, {}, :read => true map.permission :add_issue_watchers, {:watchers => [:new, :create, :append, :autocomplete_for_user]} map.permission :delete_issue_watchers, {:watchers => :destroy} - map.permission :import_issues, {:imports => [:new, :create, :settings, :mapping, :run, :show]} + map.permission :import_issues, {} # Issue categories map.permission :manage_categories, {:projects => :settings, :issue_categories => [:index, :show, :new, :create, :edit, :update, :destroy]}, :require => :member end diff --git a/test/functional/imports_controller_test.rb b/test/functional/imports_controller_test.rb index 2802d1524..6636ed821 100644 --- a/test/functional/imports_controller_test.rb +++ b/test/functional/imports_controller_test.rb @@ -42,7 +42,7 @@ class ImportsControllerTest < Redmine::ControllerTest end def test_new_should_display_the_upload_form - get :new + get :new, :params => { :type => 'IssueImport' } assert_response :success assert_select 'input[name=?]', 'file' end @@ -50,6 +50,7 @@ class ImportsControllerTest < Redmine::ControllerTest def test_create_should_save_the_file import = new_record(Import) do post :create, :params => { + :type => 'IssueImport', :file => uploaded_test_file('import_issues.csv', 'text/csv') } assert_response 302 diff --git a/test/integration/routing/imports_test.rb b/test/integration/routing/imports_test.rb index 6f52c2ad1..98d0120b7 100644 --- a/test/integration/routing/imports_test.rb +++ b/test/integration/routing/imports_test.rb @@ -19,7 +19,8 @@ require File.expand_path('../../../test_helper', __FILE__) class RoutingImportsTest < Redmine::RoutingTest def test_imports - should_route 'GET /issues/imports/new' => 'imports#new' + should_route 'GET /issues/imports/new' => 'imports#new', :type => 'IssueImport' + should_route 'POST /imports' => 'imports#create' should_route 'GET /imports/4ae6bc' => 'imports#show', :id => '4ae6bc' diff --git a/test/unit/issue_import_test.rb b/test/unit/issue_import_test.rb index bcbf942ae..02606856b 100644 --- a/test/unit/issue_import_test.rb +++ b/test/unit/issue_import_test.rb @@ -38,6 +38,12 @@ class IssueImportTest < ActiveSupport::TestCase set_language_if_valid 'en' end + def test_authorized + assert IssueImport.authorized?(User.find(1)) # admins + assert IssueImport.authorized?(User.find(2)) # has import_issues permission + assert !IssueImport.authorized?(User.find(3)) # does not have permission + end + def test_create_versions_should_create_missing_versions import = generate_import_with_mapping import.mapping.merge!('fixed_version' => '9', 'create_versions' => '1') -- 2.14.1