]> source.dussan.org Git - redmine.git/commitdiff
Generalize issues imports (#28234).
authorGo MAEDA <maeda@farend.jp>
Thu, 9 May 2019 07:40:06 +0000 (07:40 +0000)
committerGo MAEDA <maeda@farend.jp>
Thu, 9 May 2019 07:40:06 +0000 (07:40 +0000)
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.

Patch by Gregor Schmidt.

git-svn-id: http://svn.redmine.org/redmine/trunk@18145 e93f8b46-1217-0410-a6f0-8f06a7374b81

21 files changed:
app/controllers/imports_controller.rb
app/helpers/imports_helper.rb
app/models/import.rb
app/models/issue_import.rb
app/views/imports/_fields_mapping.html.erb [deleted file]
app/views/imports/_issues_fields_mapping.html.erb [new file with mode: 0644]
app/views/imports/_issues_mapping.html.erb [new file with mode: 0644]
app/views/imports/_issues_mapping.js.erb [new file with mode: 0644]
app/views/imports/_issues_saved_objects.html.erb [new file with mode: 0644]
app/views/imports/_issues_sidebar.html.erb [new file with mode: 0644]
app/views/imports/mapping.html.erb
app/views/imports/mapping.js.erb
app/views/imports/new.html.erb
app/views/imports/run.html.erb
app/views/imports/settings.html.erb
app/views/imports/show.html.erb
config/routes.rb
lib/redmine.rb
test/functional/imports_controller_test.rb
test/integration/routing/imports_test.rb
test/unit/issue_import_test.rb

index 54c739cca556f00b658a3d535a2e6444e9b1679c..fa02318bc9580bffc4d72308ef0330e2fe62ca07 100644 (file)
 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
@@ -98,6 +99,14 @@ class ImportsController < ApplicationController
     end
   end
 
+  def current_menu(project)
+    if import_layout == 'admin'
+      nil
+    else
+      :application_menu
+    end
+  end
+
   private
 
   def find_import
@@ -123,4 +132,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
index 22f070cf5e0fe5f567ca583865b6b6bfb819ebb2..b97576d6f195a9490f6be754bc7112d5c827b77a 100644 (file)
 # 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)} --" : "&nbsp;".html_safe
index 7009f2e7e9373e5d7268cc76ec912c49d439dbc9..df6b085a6c466f0e7bffd00141acd7032f118392 100644 (file)
@@ -37,6 +37,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 ||= {}
index 6c59b26db6cdd6d07559be7c4966a37ec4c70ba2..5a3542291c0c4749a257ba87f45ffa9d3dbd6720 100644 (file)
 # 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/_fields_mapping.html.erb
deleted file mode 100644 (file)
index f593501..0000000
+++ /dev/null
@@ -1,90 +0,0 @@
-<p>
-  <label for="import_mapping_project_id"><%= l(:label_project) %></label>
-  <%= select_tag 'import_settings[mapping][project_id]',
-        options_for_select(project_tree_options_for_select(@import.allowed_target_projects, :selected => @import.project)),
-        :id => 'import_mapping_project_id' %>
-</p>
-<p>
-  <label for="import_mapping_tracker"><%= l(:label_tracker) %></label>
-  <%= mapping_select_tag @import, 'tracker', :required => true,
-        :values => @import.allowed_target_trackers.sorted.map {|t| [t.name, t.id]} %>
-</p>
-<p>
-  <label for="import_mapping_status"><%= l(:field_status) %></label>
-  <%= mapping_select_tag @import, 'status' %>
-</p>
-
-<div class="splitcontent">
-<div class="splitcontentleft">
-<p>
-  <label for="import_mapping_subject"><%= l(:field_subject) %></label>
-  <%= mapping_select_tag @import, 'subject', :required => true %>
-</p>
-<p>
-  <label for="import_mapping_description"><%= l(:field_description) %></label>
-  <%= mapping_select_tag @import, 'description' %>
-</p>
-<p>
-  <label for="import_mapping_priority"><%= l(:field_priority) %></label>
-  <%= mapping_select_tag @import, 'priority' %>
-</p>
-<p>
-  <label for="import_mapping_category"><%= l(:field_category) %></label>
-  <%= mapping_select_tag @import, 'category' %>
-  <% if User.current.allowed_to?(:manage_categories, @import.project) %>
-    <label class="block">
-      <%= check_box_tag 'import_settings[mapping][create_categories]', '1', @import.create_categories? %>
-      <%= l(:label_create_missing_values) %>
-    </label>
-  <% end %>
-</p>
-<p>
-  <label for="import_mapping_assigned_to"><%= l(:field_assigned_to) %></label>
-  <%= mapping_select_tag @import, 'assigned_to' %>
-</p>
-<p>
-  <label for="import_mapping_fixed_version"><%= l(:field_fixed_version) %></label>
-  <%= mapping_select_tag @import, 'fixed_version' %>
-  <% if User.current.allowed_to?(:manage_versions, @import.project) %>
-    <label class="block">
-      <%= check_box_tag 'import_settings[mapping][create_versions]', '1', @import.create_versions? %>
-      <%= l(:label_create_missing_values) %>
-    </label>
-  <% end %>
-</p>
-<% @custom_fields.each do |field| %>
-  <p>
-    <label for="import_mapping_cf_<% field.id %>"><%= field.name %></label>
-    <%= mapping_select_tag @import, "cf_#{field.id}" %>
-  </p>
-<% end %>
-</div>
-
-<div class="splitcontentright">
-<p>
-  <label for="import_mapping_is_private"><%= l(:field_is_private) %></label>
-  <%= mapping_select_tag @import, 'is_private' %>
-</p>
-<p>
-  <label for="import_mapping_parent_issue_id"><%= l(:field_parent_issue) %></label>
-  <%= mapping_select_tag @import, 'parent_issue_id' %>
-</p>
-<p>
-  <label for="import_mapping_start_date"><%= l(:field_start_date) %></label>
-  <%= mapping_select_tag @import, 'start_date' %>
-</p>
-<p>
-  <label for="import_mapping_due_date"><%= l(:field_due_date) %></label>
-  <%= mapping_select_tag @import, 'due_date' %>
-</p>
-<p>
-  <label for="import_mapping_estimated_hours"><%= l(:field_estimated_hours) %></label>
-  <%= mapping_select_tag @import, 'estimated_hours' %>
-</p>
-<p>
-  <label for="import_mapping_done_ratio"><%= l(:field_done_ratio) %></label>
-  <%= mapping_select_tag @import, 'done_ratio' %>
-</p>
-</div>
-</div>
-
diff --git a/app/views/imports/_issues_fields_mapping.html.erb b/app/views/imports/_issues_fields_mapping.html.erb
new file mode 100644 (file)
index 0000000..542a956
--- /dev/null
@@ -0,0 +1,90 @@
+<p>
+  <label for="import_mapping_project_id"><%= l(:label_project) %></label>
+  <%= select_tag 'import_settings[mapping][project_id]',
+        options_for_select(project_tree_options_for_select(@import.allowed_target_projects, :selected => @import.project)),
+        :id => 'import_mapping_project_id' %>
+</p>
+<p>
+  <label for="import_mapping_tracker"><%= l(:label_tracker) %></label>
+  <%= mapping_select_tag @import, 'tracker', :required => true,
+        :values => @import.allowed_target_trackers.sorted.map {|t| [t.name, t.id]} %>
+</p>
+<p>
+  <label for="import_mapping_status"><%= l(:field_status) %></label>
+  <%= mapping_select_tag @import, 'status' %>
+</p>
+
+<div class="splitcontent">
+<div class="splitcontentleft">
+<p>
+  <label for="import_mapping_subject"><%= l(:field_subject) %></label>
+  <%= mapping_select_tag @import, 'subject', :required => true %>
+</p>
+<p>
+  <label for="import_mapping_description"><%= l(:field_description) %></label>
+  <%= mapping_select_tag @import, 'description' %>
+</p>
+<p>
+  <label for="import_mapping_priority"><%= l(:field_priority) %></label>
+  <%= mapping_select_tag @import, 'priority' %>
+</p>
+<p>
+  <label for="import_mapping_category"><%= l(:field_category) %></label>
+  <%= mapping_select_tag @import, 'category' %>
+  <% if User.current.allowed_to?(:manage_categories, @import.project) %>
+    <label class="block">
+      <%= check_box_tag 'import_settings[mapping][create_categories]', '1', @import.create_categories? %>
+      <%= l(:label_create_missing_values) %>
+    </label>
+  <% end %>
+</p>
+<p>
+  <label for="import_mapping_assigned_to"><%= l(:field_assigned_to) %></label>
+  <%= mapping_select_tag @import, 'assigned_to' %>
+</p>
+<p>
+  <label for="import_mapping_fixed_version"><%= l(:field_fixed_version) %></label>
+  <%= mapping_select_tag @import, 'fixed_version' %>
+  <% if User.current.allowed_to?(:manage_versions, @import.project) %>
+    <label class="block">
+      <%= check_box_tag 'import_settings[mapping][create_versions]', '1', @import.create_versions? %>
+      <%= l(:label_create_missing_values) %>
+    </label>
+  <% end %>
+</p>
+<% @custom_fields.each do |field| %>
+  <p>
+    <label for="import_mapping_cf_<%= field.id %>"><%= field.name %></label>
+    <%= mapping_select_tag @import, "cf_#{field.id}" %>
+  </p>
+<% end %>
+</div>
+
+<div class="splitcontentright">
+<p>
+  <label for="import_mapping_is_private"><%= l(:field_is_private) %></label>
+  <%= mapping_select_tag @import, 'is_private' %>
+</p>
+<p>
+  <label for="import_mapping_parent_issue_id"><%= l(:field_parent_issue) %></label>
+  <%= mapping_select_tag @import, 'parent_issue_id' %>
+</p>
+<p>
+  <label for="import_mapping_start_date"><%= l(:field_start_date) %></label>
+  <%= mapping_select_tag @import, 'start_date' %>
+</p>
+<p>
+  <label for="import_mapping_due_date"><%= l(:field_due_date) %></label>
+  <%= mapping_select_tag @import, 'due_date' %>
+</p>
+<p>
+  <label for="import_mapping_estimated_hours"><%= l(:field_estimated_hours) %></label>
+  <%= mapping_select_tag @import, 'estimated_hours' %>
+</p>
+<p>
+  <label for="import_mapping_done_ratio"><%= l(:field_done_ratio) %></label>
+  <%= mapping_select_tag @import, 'done_ratio' %>
+</p>
+</div>
+</div>
+
diff --git a/app/views/imports/_issues_mapping.html.erb b/app/views/imports/_issues_mapping.html.erb
new file mode 100644 (file)
index 0000000..dc97d9c
--- /dev/null
@@ -0,0 +1,16 @@
+<fieldset class="box tabular">
+  <legend><%= l(:label_fields_mapping) %></legend>
+  <div id="fields-mapping">
+    <%= render :partial => 'issues_fields_mapping' %>
+  </div>
+</fieldset>
+
+<%= 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 (file)
index 0000000..012ccc5
--- /dev/null
@@ -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 (file)
index 0000000..f708a1c
--- /dev/null
@@ -0,0 +1,7 @@
+<ul id="saved-items">
+  <% saved_objects.each do |issue| %>
+    <li><%= link_to_issue issue %></li>
+  <% end %>
+</ul>
+
+<p><%= link_to l(:label_issue_view_all), issues_path(:set_filter => 1, :status_id => '*', :issue_id => saved_objects.map(&:id).join(',')) %></p>
diff --git a/app/views/imports/_issues_sidebar.html.erb b/app/views/imports/_issues_sidebar.html.erb
new file mode 100644 (file)
index 0000000..e098175
--- /dev/null
@@ -0,0 +1,3 @@
+<% content_for :sidebar do %>
+  <%= render :partial => 'issues/sidebar' %>
+<% end %>
index 2e225d6c218c5326716dde784767d0ee150c5ada..1822f280222ccf303282adf6cf6bef46a07df178 100644 (file)
@@ -1,12 +1,7 @@
-<h2><%= l(:label_import_issues) %></h2>
+<h2><%= import_title %></h2>
 
 <%= form_tag(import_mapping_path(@import), :id => "import-form") do %>
-  <fieldset class="box tabular">
-    <legend><%= l(:label_fields_mapping) %></legend>
-    <div id="fields-mapping">
-      <%= render :partial => 'fields_mapping' %>
-    </div>
-  </fieldset>
+  <%= render :partial => "#{import_partial_prefix}_mapping" %>
 
   <div class="autoscroll">
   <fieldset class="box">
   </p>
 <% 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 %>
index 8fdf14a3644b96e3c92713ca6a4223313649842f..4a435842795674922dd209b8e471e26ac19aa9b5 100644 (file)
@@ -1 +1 @@
-$('#fields-mapping').html('<%= escape_javascript(render :partial => 'fields_mapping') %>');
+<%= render :partial => "#{import_partial_prefix}_mapping" %>
index e20be353ad8c163eb38041894dd2137c6f80adb2..41b27d6c98cf756e28d7a5e2012081e6f634aa43 100644 (file)
@@ -1,6 +1,7 @@
-<h2><%= l(:label_import_issues) %></h2>
+<h2><%= import_title %></h2>
 
 <%= form_tag(imports_path, :multipart => true) do %>
+  <%= hidden_field_tag 'type', @import.type %>
   <fieldset class="box">
     <legend><%= l(:label_select_file_to_import) %> (CSV)</legend>
     <p>
@@ -10,6 +11,4 @@
   <p><%= submit_tag l(:label_next).html_safe + " &#187;".html_safe, :name => nil %></p>
 <% end %>
 
-<% content_for :sidebar do %>
-    <%= render :partial => 'issues/sidebar' %>
-<% end %>
+<%= render :partial => "#{import_partial_prefix}_sidebar" %>
index 2a723537ee863e1da1a3fa83d87ec20109e9b2dd..50b47836d2912e4bf60d3d0c5db587c368de5814 100644 (file)
@@ -1,12 +1,10 @@
-<h2><%= l(:label_import_issues) %></h2>
+<h2><%= import_title %></h2>
 
 <div id="import-details">
   <div id="import-progress"><div id="progress-label">0 / <%= @import.total_items.to_i %></div></div>
 </div>
 
-<% content_for :sidebar do %>
-  <%= render :partial => 'issues/sidebar' %>
-<% end %>
+<%= render :partial => "#{import_partial_prefix}_sidebar" %>
 
 <%= javascript_tag do %>
 $(document).ready(function() {
index 6edfb10affdc62331da29aac9bdcb29a9ed4f901..0fbb01857edb4351470ed0d97bff3b6f249ccae3 100644 (file)
@@ -1,4 +1,4 @@
-<h2><%= l(:label_import_issues) %></h2>
+<h2><%= import_title %></h2>
 
 <%= form_tag(import_settings_path(@import), :id => "import-form") do %>
   <fieldset class="box tabular">
@@ -25,6 +25,4 @@
   <p><%= submit_tag l(:label_next).html_safe + " &#187;".html_safe, :name => nil %></p>
 <% end %>
 
-<% content_for :sidebar do %>
-    <%= render :partial => 'issues/sidebar' %>
-<% end %>
+<%= render :partial => "#{import_partial_prefix}_sidebar" %>
index 19c874c91a2d1b86f9f8c5c7a8e93dcab4726951..ca963ab3713ae36387477449f3c0d63d0d2397aa 100644 (file)
@@ -1,15 +1,9 @@
-<h2><%= l(:label_import_issues) %></h2>
+<h2><%= import_title %></h2>
 
 <% if @import.saved_items.count > 0 %>
   <p><%= l(:notice_import_finished, :count => @import.saved_items.count) %>:</p>
 
-  <ul id="saved-items">
-  <% @import.saved_objects.each do |issue| %>
-    <li><%= link_to_issue issue %></li>
-  <% end %>
-  </ul>
-
-  <p><%= link_to l(:label_issue_view_all), issues_path(:set_filter => 1, :status_id => '*', :issue_id => @import.saved_objects.map(&:id).join(',')) %></p>
+  <%= render :partial => "#{import_partial_prefix}_saved_objects", :locals => { saved_objects: @import.saved_objects } %>
 <% end %>
 
 <% if @import.unsaved_items.count > 0 %>
@@ -33,6 +27,4 @@
   </table>
 <% end %>
 
-<% content_for :sidebar do %>
-    <%= render :partial => 'issues/sidebar' %>
-<% end %>
+<%= render :partial => "#{import_partial_prefix}_sidebar" %>
index 27cf36a4ba71fc68978fca89c78e90f80ffd975c..4c65f976b4c04c5a9c5f2cc92bd4b3dee3772e34 100644 (file)
@@ -64,7 +64,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'
index 45e9f102423ecce0068c93934c9b38700d4cd911..2de351c07197c8552ff3762f595c149b3edc5ea5 100644 (file)
@@ -118,7 +118,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
index d6a110056f1f98816644924c041edc700fba616b..a094b81eb064b4f9a9d68eacd180413c9dc5391c 100644 (file)
@@ -44,7 +44,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
@@ -52,6 +52,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
index ec71470d087e86bbe88e5935e7581e3899370c48..eb96a9a981dbe6c3f06444a572d9ecdb0c649d33 100644 (file)
@@ -21,7 +21,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'
index 9c4eafba1884cfe506c1c7283bca7240579996a7..4def8b120ab6aec169a125dba99e8a38b8751e54 100644 (file)
@@ -41,6 +41,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')