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.

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 6bff1b0983e5ce98b6e025814a2c6c1b5f3ad2b9..2f6ea0087252f4da718d6035100c28cc8d7c0ed9 100644 (file)
--- a/.rubocop_todo.yml
+++ b/.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.

diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 86de77706b0769491be4cc855701e7b9d7a8a241..8994d8a2e58df90e17535edf5ae63a6cf08b258e 100644 (file)
--- a/app/controllers/traces_controller.rb
+++ b/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)
-
-      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
-
-          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")
+    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 @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(: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

diff --git a/app/views/traces/list.html.erb b/app/views/traces/list.html.erb
index d63d5c52bdfe39681eb7963b22676b0fabc582b7..3cd90f3e7345894f7144212a92caf56c46a30138 100644 (file)
--- a/app/views/traces/list.html.erb
+++ b/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' %>

diff --git a/app/views/traces/create.html.erb b/app/views/traces/new.html.erb
similarity index 100%
rename from app/views/traces/create.html.erb
rename to app/views/traces/new.html.erb

diff --git a/config/locales/en.yml b/config/locales/en.yml
index 4f6734b329b415cd338cadafc1d85c346e290251..a7efaf98566a9426c432ed7a34b63f3b836fc017 100644 (file)
--- a/config/locales/en.yml
+++ b/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}"

diff --git a/config/routes.rb b/config/routes.rb
index b46f9287743942e92d029cdf6cc09c223b9e04fa..9ab3f57da5d4cf2abb0628745e66fb93dc8f8864 100644 (file)
--- a/config/routes.rb
+++ b/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+/

diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index 9600ff77eba3da0a5d9b5da49a97f59f624c70e2..456bf9f8a505426cd26d69caee8b39c82fa058ac 100644 (file)
--- a/test/controllers/traces_controller_test.rb
+++ b/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