Refactor trace creation pages

Split the trace creation into new and create methods, with standard resourceful routing. Provide a redirect for external requests to the old url.

.rubocop_todo.yml

@@ -67,7 +67,7 @@ Metrics/AbcSize:
 # Offense count: 41
 # Configuration parameters: CountComments, ExcludedMethods.
 Metrics/BlockLength:
-  Max: 240
+  Max: 241
 
 # Offense count: 12
 # Configuration parameters: CountBlocks.

app/controllers/traces_controller.rb

@@ -4,16 +4,16 @@ class TracesController < ApplicationController
   skip_before_action :verify_authenticity_token, :only => [:api_create, :api_read, :api_update, :api_delete, :api_data]
   before_action :authorize_web
   before_action :set_locale
-  before_action :require_user, :only => [:mine, :create, :edit, :delete]
+  before_action :require_user, :only => [:mine, :new, :create, :edit, :delete]
   before_action :authorize, :only => [:api_create, :api_read, :api_update, :api_delete, :api_data]
   before_action :check_database_readable, :except => [:api_read, :api_data]
-  before_action :check_database_writable, :only => [:create, :edit, :delete, :api_create, :api_update, :api_delete]
+  before_action :check_database_writable, :only => [:new, :create, :edit, :delete, :api_create, :api_update, :api_delete]
   before_action :check_api_readable, :only => [:api_read, :api_data]
   before_action :check_api_writable, :only => [:api_create, :api_update, :api_delete]
   before_action :require_allow_read_gpx, :only => [:api_read, :api_data]
   before_action :require_allow_write_gpx, :only => [:api_create, :api_update, :api_delete]
   before_action :offline_warning, :only => [:mine, :view]
-  before_action :offline_redirect, :only => [:create, :edit, :delete, :data, :api_create, :api_delete, :api_data]
+  before_action :offline_redirect, :only => [:new, :create, :edit, :delete, :data, :api_create, :api_delete, :api_data]
   around_action :api_call_handle_error, :only => [:api_create, :api_read, :api_update, :api_delete, :api_data]
 
   # Counts and selects pages of GPX traces for various criteria (by user, tags, public etc.).
@@ -104,39 +104,40 @@ class TracesController < ApplicationController
     redirect_to :action => "list"
   end
 
+  def new
+    @title = t ".upload_trace"
+    @trace = Trace.new(:visibility => default_visibility)
+  end
+
   def create
-    if request.post?
-      logger.info(params[:trace][:gpx_file].class.name)
+    logger.info(params[:trace][:gpx_file].class.name)
 
-      if params[:trace][:gpx_file].respond_to?(:read)
-        begin
-          do_create(params[:trace][:gpx_file], params[:trace][:tagstring],
-                    params[:trace][:description], params[:trace][:visibility])
-        rescue StandardError => ex
-          logger.debug ex
-        end
+    if params[:trace][:gpx_file].respond_to?(:read)
+      begin
+        do_create(params[:trace][:gpx_file], params[:trace][:tagstring],
+                  params[:trace][:description], params[:trace][:visibility])
+      rescue StandardError => ex
+        logger.debug ex
+      end
 
-        if @trace.id
-          flash[:notice] = t ".trace_uploaded"
-          flash[:warning] = t ".traces_waiting", :count => current_user.traces.where(:inserted => false).count if current_user.traces.where(:inserted => false).count > 4
+      if @trace.id
+        flash[:notice] = t ".trace_uploaded"
+        flash[:warning] = t ".traces_waiting", :count => current_user.traces.where(:inserted => false).count if current_user.traces.where(:inserted => false).count > 4
 
-          redirect_to :action => :list, :display_name => current_user.display_name
-        end
-      else
-        @trace = Trace.new(:name => "Dummy",
-                           :tagstring => params[:trace][:tagstring],
-                           :description => params[:trace][:description],
-                           :visibility => params[:trace][:visibility],
-                           :inserted => false, :user => current_user,
-                           :timestamp => Time.now.getutc)
-        @trace.valid?
-        @trace.errors.add(:gpx_file, "can't be blank")
+        redirect_to :action => :list, :display_name => current_user.display_name
       end
     else
-      @trace = Trace.new(:visibility => default_visibility)
+      @trace = Trace.new(:name => "Dummy",
+                         :tagstring => params[:trace][:tagstring],
+                         :description => params[:trace][:description],
+                         :visibility => params[:trace][:visibility],
+                         :inserted => false, :user => current_user,
+                         :timestamp => Time.now.getutc)
+      @trace.valid?
+      @trace.errors.add(:gpx_file, "can't be blank")
+      @title = t ".upload_trace"
+      render :action => "new"
     end
-
-    @title = t ".upload_trace"
   end
 
   def data

app/views/traces/list.html.erb

@@ -3,7 +3,7 @@
   <ul class='secondary-actions clearfix'>
     <li><%= t('.description') %></li>
     <li><%= rss_link_to :action => 'georss', :display_name => @display_name, :tag => @tag %></li>
-    <li><%= link_to t('.upload_trace'), :action => 'create' %></li>
+    <li><%= link_to t('.upload_trace'), new_trace_path %></li>
     <% if @tag %>
       <li><%= link_to t('.see_all_traces'), :controller => 'traces', :action => 'list', :display_name => nil, :tag => nil, :page => nil %></li>
       <li><%= link_to t('.see_my_traces'), :action => 'mine', :tag => nil, :page => nil %></li>
@@ -39,7 +39,7 @@
 
   <%= render :partial => 'trace_paging_nav' %>
 <% else %>
-  <h4><%= t '.empty_html', :upload_link => trace_create_path %></h4>
+  <h4><%= t '.empty_html', :upload_link => new_trace_path %></h4>
 <% end %>
 
 <%= render :partial => 'trace_optionals' %>

config/locales/en.yml

@@ -1577,12 +1577,8 @@ en:
       public: "Public (shown in trace list and as anonymous, unordered points)"
       trackable: "Trackable (only shared as anonymous, ordered points with timestamps)"
       identifiable: "Identifiable (shown in trace list and as identifiable, ordered points with timestamps)"
-    create:
+    new:
       upload_trace: "Upload GPS Trace"
-      trace_uploaded: "Your GPX file has been uploaded and is awaiting insertion in to the database. This will usually happen within half an hour, and an email will be sent to you on completion."
-      traces_waiting:
-        one: "You have %{count} trace waiting for upload. Please consider waiting for these to finish before uploading any more, so as not to block the queue for other users."
-        other: "You have %{count} traces waiting for upload. Please consider waiting for these to finish before uploading any more, so as not to block the queue for other users."
       upload_gpx: "Upload GPX File:"
       description: "Description:"
       tags: "Tags:"
@@ -1593,6 +1589,12 @@ en:
       upload_button: "Upload"
       help: "Help"
       help_url: "https://wiki.openstreetmap.org/wiki/Upload"
+    create:
+      upload_trace: "Upload GPS Trace"
+      trace_uploaded: "Your GPX file has been uploaded and is awaiting insertion in to the database. This will usually happen within half an hour, and an email will be sent to you on completion."
+      traces_waiting:
+        one: "You have %{count} trace waiting for upload. Please consider waiting for these to finish before uploading any more, so as not to block the queue for other users."
+        other: "You have %{count} traces waiting for upload. Please consider waiting for these to finish before uploading any more, so as not to block the queue for other users."
     edit:
       title: "Editing trace %{name}"
       heading: "Editing trace %{name}"

config/routes.rb

@@ -207,7 +207,9 @@ OpenStreetMap::Application.routes.draw do
   get "/traces/mine/tag/:tag" => "traces#mine"
   get "/traces/mine/page/:page" => "traces#mine"
   get "/traces/mine" => "traces#mine"
-  match "/trace/create" => "traces#create", :via => [:get, :post]
+  resources :traces, :only => [:new, :create]
+  post "/trace/create" => "traces#create" # remove after deployment
+  get "/trace/create", :to => redirect(:path => "/traces/new")
   get "/trace/:id/data" => "traces#data", :id => /\d+/, :as => "trace_data"
   match "/trace/:id/edit" => "traces#edit", :via => [:get, :post], :id => /\d+/, :as => "trace_edit"
   post "/trace/:id/delete" => "traces#delete", :id => /\d+/

test/controllers/traces_controller_test.rb

@@ -135,11 +135,11 @@ class TracesControllerTest < ActionController::TestCase
     )
 
     assert_routing(
-      { :path => "/trace/create", :method => :get },
-      { :controller => "traces", :action => "create" }
+      { :path => "/traces/new", :method => :get },
+      { :controller => "traces", :action => "new" }
     )
     assert_routing(
-      { :path => "/trace/create", :method => :post },
+      { :path => "/traces", :method => :post },
       { :controller => "traces", :action => "create" }
     )
     assert_routing(
@@ -508,34 +508,34 @@ class TracesControllerTest < ActionController::TestCase
     assert_response :not_found
   end
 
-  # Test fetching the create page
-  def test_create_get
+  # Test fetching the new trace page
+  def test_new_get
     # First with no auth
-    get :create
+    get :new
     assert_response :redirect
-    assert_redirected_to :controller => :user, :action => :login, :referer => trace_create_path
+    assert_redirected_to :controller => :user, :action => :login, :referer => new_trace_path
 
     # Now authenticated as a user with gps.trace.visibility set
     user = create(:user)
     create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "identifiable")
-    get :create, :session => { :user => user }
+    get :new, :session => { :user => user }
     assert_response :success
-    assert_template :create
+    assert_template :new
     assert_select "select#trace_visibility option[value=identifiable][selected]", 1
 
     # Now authenticated as a user with gps.trace.public set
     second_user = create(:user)
     create(:user_preference, :user => second_user, :k => "gps.trace.public", :v => "default")
-    get :create, :session => { :user => second_user }
+    get :new, :session => { :user => second_user }
     assert_response :success
-    assert_template :create
+    assert_template :new
     assert_select "select#trace_visibility option[value=public][selected]", 1
 
     # Now authenticated as a user with no preferences
     third_user = create(:user)
-    get :create, :session => { :user => third_user }
+    get :new, :session => { :user => third_user }
     assert_response :success
-    assert_template :create
+    assert_template :new
     assert_select "select#trace_visibility option[value=private][selected]", 1
   end