From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 2 Nov 2022 11:06:00 +0000 (+0000)
Subject: Enable the ActionOrder cop for remaining controllers
X-Git-Tag: live~1497^2~1
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/22946d703a1186d0ec7fb18a663f73855bb49546?hp=49cde0e9013848d2d3bd33f19fe106296505b6b2

Enable the ActionOrder cop for remaining controllers

Where actions were reordered, the rails standard actions were
also moved to the top of each controller.
---

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index adc5d8fc8..c03cf665c 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -112,27 +112,6 @@ Rails/ActionControllerFlashBeforeRender:
     - 'app/controllers/user_blocks_controller.rb'
     - 'app/controllers/users_controller.rb'
 
-# Offense count: 18
-# This cop supports safe autocorrection (--autocorrect).
-# Configuration parameters: ExpectedOrder, Include.
-# ExpectedOrder: index, show, new, edit, create, update, destroy
-# Include: app/controllers/**/*.rb
-Rails/ActionOrder:
-  Exclude:
-    - 'app/controllers/api/changesets_controller.rb'
-    - 'app/controllers/api/nodes_controller.rb'
-    - 'app/controllers/api/notes_controller.rb'
-    - 'app/controllers/api/relations_controller.rb'
-    - 'app/controllers/api/traces_controller.rb'
-    - 'app/controllers/api/users_controller.rb'
-    - 'app/controllers/api/ways_controller.rb'
-    - 'app/controllers/diary_entries_controller.rb'
-    - 'app/controllers/messages_controller.rb'
-    - 'app/controllers/oauth_clients_controller.rb'
-    - 'app/controllers/redactions_controller.rb'
-    - 'app/controllers/traces_controller.rb'
-    - 'app/controllers/users_controller.rb'
-
 # Offense count: 5
 # Configuration parameters: Database, Include.
 # SupportedDatabases: mysql, postgresql
diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb
index 24e7fb925..56070951a 100644
--- a/app/controllers/api/changesets_controller.rb
+++ b/app/controllers/api/changesets_controller.rb
@@ -19,6 +19,20 @@ module Api
     # Helper methods for checking consistency
     include ConsistencyValidations
 
+    ##
+    # Return XML giving the basic info about the changeset. Does not
+    # return anything about the nodes, ways and relations in the changeset.
+    def show
+      @changeset = Changeset.find(params[:id])
+      @include_discussion = params[:include_discussion].presence
+      render "changeset"
+
+      respond_to do |format|
+        format.xml
+        format.json
+      end
+    end
+
     # Create a changeset from XML.
     def create
       assert_method :put
@@ -35,20 +49,6 @@ module Api
       render :plain => cs.id.to_s
     end
 
-    ##
-    # Return XML giving the basic info about the changeset. Does not
-    # return anything about the nodes, ways and relations in the changeset.
-    def show
-      @changeset = Changeset.find(params[:id])
-      @include_discussion = params[:include_discussion].presence
-      render "changeset"
-
-      respond_to do |format|
-        format.xml
-        format.json
-      end
-    end
-
     ##
     # marks a changeset as closed. this may be called multiple times
     # on the same changeset, so is idempotent.
diff --git a/app/controllers/api/nodes_controller.rb b/app/controllers/api/nodes_controller.rb
index 92779dd67..6934a13c0 100644
--- a/app/controllers/api/nodes_controller.rb
+++ b/app/controllers/api/nodes_controller.rb
@@ -15,15 +15,21 @@ module Api
 
     before_action :set_request_formats, :except => [:create, :update, :delete]
 
-    # Create a node from XML.
-    def create
-      assert_method :put
+    # Dump the details on many nodes whose ids are given in the "nodes" parameter.
+    def index
+      raise OSM::APIBadUserInput, "The parameter nodes is required, and must be of the form nodes=id[,id[,id...]]" unless params["nodes"]
 
-      node = Node.from_xml(request.raw_post, :create => true)
+      ids = params["nodes"].split(",").collect(&:to_i)
 
-      # Assume that Node.from_xml has thrown an exception if there is an error parsing the xml
-      node.create_with_history current_user
-      render :plain => node.id.to_s
+      raise OSM::APIBadUserInput, "No nodes were given to search for" if ids.empty?
+
+      @nodes = Node.find(ids)
+
+      # Render the result
+      respond_to do |format|
+        format.xml
+        format.json
+      end
     end
 
     # Dump the details on a node given in params[:id]
@@ -43,6 +49,17 @@ module Api
       end
     end
 
+    # Create a node from XML.
+    def create
+      assert_method :put
+
+      node = Node.from_xml(request.raw_post, :create => true)
+
+      # Assume that Node.from_xml has thrown an exception if there is an error parsing the xml
+      node.create_with_history current_user
+      render :plain => node.id.to_s
+    end
+
     # Update a node from given XML
     def update
       node = Node.find(params[:id])
@@ -66,22 +83,5 @@ module Api
       node.delete_with_history!(new_node, current_user)
       render :plain => node.version.to_s
     end
-
-    # Dump the details on many nodes whose ids are given in the "nodes" parameter.
-    def index
-      raise OSM::APIBadUserInput, "The parameter nodes is required, and must be of the form nodes=id[,id[,id...]]" unless params["nodes"]
-
-      ids = params["nodes"].split(",").collect(&:to_i)
-
-      raise OSM::APIBadUserInput, "No nodes were given to search for" if ids.empty?
-
-      @nodes = Node.find(ids)
-
-      # Render the result
-      respond_to do |format|
-        format.xml
-        format.json
-      end
-    end
   end
 end
diff --git a/app/controllers/api/notes_controller.rb b/app/controllers/api/notes_controller.rb
index d915d8126..5e24aa785 100644
--- a/app/controllers/api/notes_controller.rb
+++ b/app/controllers/api/notes_controller.rb
@@ -52,6 +52,26 @@ module Api
       end
     end
 
+    ##
+    # Read a note
+    def show
+      # Check the arguments are sane
+      raise OSM::APIBadUserInput, "No id was given" unless params[:id]
+
+      # Find the note and check it is valid
+      @note = Note.find(params[:id])
+      raise OSM::APINotFoundError unless @note
+      raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible? || current_user&.moderator?
+
+      # Render the result
+      respond_to do |format|
+        format.xml
+        format.rss
+        format.json
+        format.gpx
+      end
+    end
+
     ##
     # Create a new note
     def create
@@ -88,6 +108,36 @@ module Api
       end
     end
 
+    ##
+    # Delete (hide) a note
+    def destroy
+      # Check the arguments are sane
+      raise OSM::APIBadUserInput, "No id was given" unless params[:id]
+
+      # Extract the arguments
+      id = params[:id].to_i
+      comment = params[:text]
+
+      # Find the note and check it is valid
+      @note = Note.find(id)
+      raise OSM::APINotFoundError unless @note
+      raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible?
+
+      # Mark the note as hidden
+      Note.transaction do
+        @note.status = "hidden"
+        @note.save
+
+        add_comment(@note, comment, "hidden", :notify => false)
+      end
+
+      # Return a copy of the updated note
+      respond_to do |format|
+        format.xml { render :action => :show }
+        format.json { render :action => :show }
+      end
+    end
+
     ##
     # Add a comment to an existing note
     def comment
@@ -209,56 +259,6 @@ module Api
       end
     end
 
-    ##
-    # Read a note
-    def show
-      # Check the arguments are sane
-      raise OSM::APIBadUserInput, "No id was given" unless params[:id]
-
-      # Find the note and check it is valid
-      @note = Note.find(params[:id])
-      raise OSM::APINotFoundError unless @note
-      raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible? || current_user&.moderator?
-
-      # Render the result
-      respond_to do |format|
-        format.xml
-        format.rss
-        format.json
-        format.gpx
-      end
-    end
-
-    ##
-    # Delete (hide) a note
-    def destroy
-      # Check the arguments are sane
-      raise OSM::APIBadUserInput, "No id was given" unless params[:id]
-
-      # Extract the arguments
-      id = params[:id].to_i
-      comment = params[:text]
-
-      # Find the note and check it is valid
-      @note = Note.find(id)
-      raise OSM::APINotFoundError unless @note
-      raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible?
-
-      # Mark the note as hidden
-      Note.transaction do
-        @note.status = "hidden"
-        @note.save
-
-        add_comment(@note, comment, "hidden", :notify => false)
-      end
-
-      # Return a copy of the updated note
-      respond_to do |format|
-        format.xml { render :action => :show }
-        format.json { render :action => :show }
-      end
-    end
-
     ##
     # Return a list of notes matching a given string
     def search
diff --git a/app/controllers/api/relations_controller.rb b/app/controllers/api/relations_controller.rb
index 69b145268..19aed6a85 100644
--- a/app/controllers/api/relations_controller.rb
+++ b/app/controllers/api/relations_controller.rb
@@ -13,14 +13,20 @@ module Api
 
     before_action :set_request_formats, :except => [:create, :update, :delete]
 
-    def create
-      assert_method :put
+    def index
+      raise OSM::APIBadUserInput, "The parameter relations is required, and must be of the form relations=id[,id[,id...]]" unless params["relations"]
 
-      relation = Relation.from_xml(request.raw_post, :create => true)
+      ids = params["relations"].split(",").collect(&:to_i)
 
-      # Assume that Relation.from_xml has thrown an exception if there is an error parsing the xml
-      relation.create_with_history current_user
-      render :plain => relation.id.to_s
+      raise OSM::APIBadUserInput, "No relations were given to search for" if ids.empty?
+
+      @relations = Relation.find(ids)
+
+      # Render the result
+      respond_to do |format|
+        format.xml
+        format.json
+      end
     end
 
     def show
@@ -37,6 +43,16 @@ module Api
       end
     end
 
+    def create
+      assert_method :put
+
+      relation = Relation.from_xml(request.raw_post, :create => true)
+
+      # Assume that Relation.from_xml has thrown an exception if there is an error parsing the xml
+      relation.create_with_history current_user
+      render :plain => relation.id.to_s
+    end
+
     def update
       logger.debug request.raw_post
 
@@ -131,22 +147,6 @@ module Api
       end
     end
 
-    def index
-      raise OSM::APIBadUserInput, "The parameter relations is required, and must be of the form relations=id[,id[,id...]]" unless params["relations"]
-
-      ids = params["relations"].split(",").collect(&:to_i)
-
-      raise OSM::APIBadUserInput, "No relations were given to search for" if ids.empty?
-
-      @relations = Relation.find(ids)
-
-      # Render the result
-      respond_to do |format|
-        format.xml
-        format.json
-      end
-    end
-
     def relations_for_way
       relations_for_object("Way")
     end
diff --git a/app/controllers/api/traces_controller.rb b/app/controllers/api/traces_controller.rb
index 8121764a1..07b6208af 100644
--- a/app/controllers/api/traces_controller.rb
+++ b/app/controllers/api/traces_controller.rb
@@ -19,6 +19,35 @@ module Api
       head :forbidden unless @trace.public? || @trace.user == current_user
     end
 
+    def create
+      tags = params[:tags] || ""
+      description = params[:description] || ""
+      visibility = params[:visibility]
+
+      if visibility.nil?
+        visibility = if params[:public]&.to_i&.nonzero?
+                       "public"
+                     else
+                       "private"
+                     end
+      end
+
+      if params[:file].respond_to?(:read)
+        trace = do_create(params[:file], tags, description, visibility)
+
+        if trace.id
+          TraceImporterJob.perform_later(trace)
+          render :plain => trace.id.to_s
+        elsif trace.valid?
+          head :internal_server_error
+        else
+          head :bad_request
+        end
+      else
+        head :bad_request
+      end
+    end
+
     def update
       trace = Trace.visible.find(params[:id])
 
@@ -64,35 +93,6 @@ module Api
       end
     end
 
-    def create
-      tags = params[:tags] || ""
-      description = params[:description] || ""
-      visibility = params[:visibility]
-
-      if visibility.nil?
-        visibility = if params[:public]&.to_i&.nonzero?
-                       "public"
-                     else
-                       "private"
-                     end
-      end
-
-      if params[:file].respond_to?(:read)
-        trace = do_create(params[:file], tags, description, visibility)
-
-        if trace.id
-          TraceImporterJob.perform_later(trace)
-          render :plain => trace.id.to_s
-        elsif trace.valid?
-          head :internal_server_error
-        else
-          head :bad_request
-        end
-      else
-        head :bad_request
-      end
-    end
-
     private
 
     def do_create(file, tags, description, visibility)
diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb
index d4baf4a82..9da9d3226 100644
--- a/app/controllers/api/users_controller.rb
+++ b/app/controllers/api/users_controller.rb
@@ -12,6 +12,22 @@ module Api
 
     before_action :set_request_formats, :except => [:gpx_files]
 
+    def index
+      raise OSM::APIBadUserInput, "The parameter users is required, and must be of the form users=id[,id[,id...]]" unless params["users"]
+
+      ids = params["users"].split(",").collect(&:to_i)
+
+      raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty?
+
+      @users = User.visible.find(ids)
+
+      # Render the result
+      respond_to do |format|
+        format.xml
+        format.json
+      end
+    end
+
     def show
       if @user.visible?
         # Render the result
@@ -33,22 +49,6 @@ module Api
       end
     end
 
-    def index
-      raise OSM::APIBadUserInput, "The parameter users is required, and must be of the form users=id[,id[,id...]]" unless params["users"]
-
-      ids = params["users"].split(",").collect(&:to_i)
-
-      raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty?
-
-      @users = User.visible.find(ids)
-
-      # Render the result
-      respond_to do |format|
-        format.xml
-        format.json
-      end
-    end
-
     def gpx_files
       @traces = current_user.traces.reload
       render :content_type => "application/xml"
diff --git a/app/controllers/api/ways_controller.rb b/app/controllers/api/ways_controller.rb
index ca4acd611..c4fce48d0 100644
--- a/app/controllers/api/ways_controller.rb
+++ b/app/controllers/api/ways_controller.rb
@@ -13,14 +13,20 @@ module Api
 
     before_action :set_request_formats, :except => [:create, :update, :delete]
 
-    def create
-      assert_method :put
+    def index
+      raise OSM::APIBadUserInput, "The parameter ways is required, and must be of the form ways=id[,id[,id...]]" unless params["ways"]
 
-      way = Way.from_xml(request.raw_post, :create => true)
+      ids = params["ways"].split(",").collect(&:to_i)
 
-      # Assume that Way.from_xml has thrown an exception if there is an error parsing the xml
-      way.create_with_history current_user
-      render :plain => way.id.to_s
+      raise OSM::APIBadUserInput, "No ways were given to search for" if ids.empty?
+
+      @ways = Way.find(ids)
+
+      # Render the result
+      respond_to do |format|
+        format.xml
+        format.json
+      end
     end
 
     def show
@@ -39,6 +45,16 @@ module Api
       end
     end
 
+    def create
+      assert_method :put
+
+      way = Way.from_xml(request.raw_post, :create => true)
+
+      # Assume that Way.from_xml has thrown an exception if there is an error parsing the xml
+      way.create_with_history current_user
+      render :plain => way.id.to_s
+    end
+
     def update
       way = Way.find(params[:id])
       new_way = Way.from_xml(request.raw_post)
@@ -87,22 +103,6 @@ module Api
       end
     end
 
-    def index
-      raise OSM::APIBadUserInput, "The parameter ways is required, and must be of the form ways=id[,id[,id...]]" unless params["ways"]
-
-      ids = params["ways"].split(",").collect(&:to_i)
-
-      raise OSM::APIBadUserInput, "No ways were given to search for" if ids.empty?
-
-      @ways = Way.find(ids)
-
-      # Render the result
-      respond_to do |format|
-        format.xml
-        format.json
-      end
-    end
-
     ##
     # returns all the ways which are currently using the node given in the
     # :id parameter. note that this used to return deleted ways as well, but
diff --git a/app/controllers/diary_entries_controller.rb b/app/controllers/diary_entries_controller.rb
index 467c4a38f..8e6215dbd 100644
--- a/app/controllers/diary_entries_controller.rb
+++ b/app/controllers/diary_entries_controller.rb
@@ -11,6 +11,67 @@ class DiaryEntriesController < ApplicationController
   before_action :check_database_writable, :only => [:new, :create, :edit, :update, :comment, :hide, :hidecomment, :subscribe, :unsubscribe]
   before_action :allow_thirdparty_images, :only => [:new, :create, :edit, :update, :index, :show, :comments]
 
+  def index
+    if params[:display_name]
+      @user = User.active.find_by(:display_name => params[:display_name])
+
+      if @user
+        @title = t "diary_entries.index.user_title", :user => @user.display_name
+        @entries = @user.diary_entries
+      else
+        render_unknown_user params[:display_name]
+        return
+      end
+    elsif params[:friends]
+      if current_user
+        @title = t "diary_entries.index.title_friends"
+        @entries = DiaryEntry.where(:user_id => current_user.friends)
+      else
+        require_user
+        return
+      end
+    elsif params[:nearby]
+      if current_user
+        @title = t "diary_entries.index.title_nearby"
+        @entries = DiaryEntry.where(:user_id => current_user.nearby)
+      else
+        require_user
+        return
+      end
+    else
+      @entries = DiaryEntry.joins(:user).where(:users => { :status => %w[active confirmed] })
+
+      if params[:language]
+        @title = t "diary_entries.index.in_language_title", :language => Language.find(params[:language]).english_name
+        @entries = @entries.where(:language_code => params[:language])
+      else
+        @title = t "diary_entries.index.title"
+      end
+    end
+
+    @params = params.permit(:display_name, :friends, :nearby, :language)
+
+    @page = (params[:page] || 1).to_i
+    @page_size = 20
+
+    @entries = @entries.visible unless can? :unhide, DiaryEntry
+    @entries = @entries.order("created_at DESC")
+    @entries = @entries.offset((@page - 1) * @page_size)
+    @entries = @entries.limit(@page_size)
+    @entries = @entries.includes(:user, :language)
+  end
+
+  def show
+    @entry = @user.diary_entries.visible.where(:id => params[:id]).first
+    if @entry
+      @title = t "diary_entries.show.title", :user => params[:display_name], :title => @entry.title
+      @comments = can?(:unhidecomment, DiaryEntry) ? @entry.comments : @entry.visible_comments
+    else
+      @title = t "diary_entries.no_such_entry.title", :id => params[:id]
+      render :action => "no_such_entry", :status => :not_found
+    end
+  end
+
   def new
     @title = t "diary_entries.new.title"
 
@@ -21,6 +82,17 @@ class DiaryEntriesController < ApplicationController
     render :action => "new"
   end
 
+  def edit
+    @title = t "diary_entries.edit.title"
+    @diary_entry = DiaryEntry.find(params[:id])
+
+    redirect_to diary_entry_path(@diary_entry.user, @diary_entry) if current_user != @diary_entry.user
+
+    set_map_location
+  rescue ActiveRecord::RecordNotFound
+    render :action => "no_such_entry", :status => :not_found
+  end
+
   def create
     @title = t "diary_entries.new.title"
 
@@ -45,17 +117,6 @@ class DiaryEntriesController < ApplicationController
     end
   end
 
-  def edit
-    @title = t "diary_entries.edit.title"
-    @diary_entry = DiaryEntry.find(params[:id])
-
-    redirect_to diary_entry_path(@diary_entry.user, @diary_entry) if current_user != @diary_entry.user
-
-    set_map_location
-  rescue ActiveRecord::RecordNotFound
-    render :action => "no_such_entry", :status => :not_found
-  end
-
   def update
     @title = t "diary_entries.edit.title"
     @diary_entry = DiaryEntry.find(params[:id])
@@ -114,56 +175,6 @@ class DiaryEntriesController < ApplicationController
     render :action => "no_such_entry", :status => :not_found
   end
 
-  def index
-    if params[:display_name]
-      @user = User.active.find_by(:display_name => params[:display_name])
-
-      if @user
-        @title = t "diary_entries.index.user_title", :user => @user.display_name
-        @entries = @user.diary_entries
-      else
-        render_unknown_user params[:display_name]
-        return
-      end
-    elsif params[:friends]
-      if current_user
-        @title = t "diary_entries.index.title_friends"
-        @entries = DiaryEntry.where(:user_id => current_user.friends)
-      else
-        require_user
-        return
-      end
-    elsif params[:nearby]
-      if current_user
-        @title = t "diary_entries.index.title_nearby"
-        @entries = DiaryEntry.where(:user_id => current_user.nearby)
-      else
-        require_user
-        return
-      end
-    else
-      @entries = DiaryEntry.joins(:user).where(:users => { :status => %w[active confirmed] })
-
-      if params[:language]
-        @title = t "diary_entries.index.in_language_title", :language => Language.find(params[:language]).english_name
-        @entries = @entries.where(:language_code => params[:language])
-      else
-        @title = t "diary_entries.index.title"
-      end
-    end
-
-    @params = params.permit(:display_name, :friends, :nearby, :language)
-
-    @page = (params[:page] || 1).to_i
-    @page_size = 20
-
-    @entries = @entries.visible unless can? :unhide, DiaryEntry
-    @entries = @entries.order("created_at DESC")
-    @entries = @entries.offset((@page - 1) * @page_size)
-    @entries = @entries.limit(@page_size)
-    @entries = @entries.includes(:user, :language)
-  end
-
   def rss
     if params[:display_name]
       user = User.active.find_by(:display_name => params[:display_name])
@@ -198,17 +209,6 @@ class DiaryEntriesController < ApplicationController
     @entries = @entries.visible.includes(:user).order("created_at DESC").limit(20)
   end
 
-  def show
-    @entry = @user.diary_entries.visible.where(:id => params[:id]).first
-    if @entry
-      @title = t "diary_entries.show.title", :user => params[:display_name], :title => @entry.title
-      @comments = can?(:unhidecomment, DiaryEntry) ? @entry.comments : @entry.visible_comments
-    else
-      @title = t "diary_entries.no_such_entry.title", :id => params[:id]
-      render :action => "no_such_entry", :status => :not_found
-    end
-  end
-
   def hide
     entry = DiaryEntry.find(params[:id])
     entry.update(:visible => false)
diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index cc311f93f..fd8cf7cf9 100644
--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -11,6 +11,23 @@ class MessagesController < ApplicationController
   before_action :check_database_writable, :only => [:new, :create, :reply, :mark, :destroy]
   before_action :allow_thirdparty_images, :only => [:new, :create, :show]
 
+  # Show a message
+  def show
+    @title = t ".title"
+    @message = Message.find(params[:id])
+
+    if @message.recipient == current_user || @message.sender == current_user
+      @message.message_read = true if @message.recipient == current_user
+      @message.save
+    else
+      flash[:notice] = t ".wrong_user", :user => current_user.display_name
+      redirect_to login_path(:referer => request.fullpath)
+    end
+  rescue ActiveRecord::RecordNotFound
+    @title = t "messages.no_such_message.title"
+    render :action => "no_such_message", :status => :not_found
+  end
+
   # Allow the user to write a new message to another user. This action also
   # deals with the sending of that message to the other user when the user
   # clicks send.
@@ -39,6 +56,23 @@ class MessagesController < ApplicationController
     end
   end
 
+  # Destroy the message.
+  def destroy
+    @message = Message.where("to_user_id = ? OR from_user_id = ?", current_user.id, current_user.id).find(params[:id])
+    @message.from_user_visible = false if @message.sender == current_user
+    @message.to_user_visible = false if @message.recipient == current_user
+    if @message.save && !request.xhr?
+      flash[:notice] = t ".destroyed"
+
+      referer = safe_referer(params[:referer]) if params[:referer]
+
+      redirect_to referer || { :action => :inbox }
+    end
+  rescue ActiveRecord::RecordNotFound
+    @title = t "messages.no_such_message.title"
+    render :action => "no_such_message", :status => :not_found
+  end
+
   # Allow the user to reply to another message.
   def reply
     message = Message.find(params[:message_id])
@@ -64,23 +98,6 @@ class MessagesController < ApplicationController
     render :action => "no_such_message", :status => :not_found
   end
 
-  # Show a message
-  def show
-    @title = t ".title"
-    @message = Message.find(params[:id])
-
-    if @message.recipient == current_user || @message.sender == current_user
-      @message.message_read = true if @message.recipient == current_user
-      @message.save
-    else
-      flash[:notice] = t ".wrong_user", :user => current_user.display_name
-      redirect_to login_path(:referer => request.fullpath)
-    end
-  rescue ActiveRecord::RecordNotFound
-    @title = t "messages.no_such_message.title"
-    render :action => "no_such_message", :status => :not_found
-  end
-
   # Display the list of messages that have been sent to the user.
   def inbox
     @title = t ".title"
@@ -111,23 +128,6 @@ class MessagesController < ApplicationController
     render :action => "no_such_message", :status => :not_found
   end
 
-  # Destroy the message.
-  def destroy
-    @message = Message.where("to_user_id = ? OR from_user_id = ?", current_user.id, current_user.id).find(params[:id])
-    @message.from_user_visible = false if @message.sender == current_user
-    @message.to_user_visible = false if @message.recipient == current_user
-    if @message.save && !request.xhr?
-      flash[:notice] = t ".destroyed"
-
-      referer = safe_referer(params[:referer]) if params[:referer]
-
-      redirect_to referer || { :action => :inbox }
-    end
-  rescue ActiveRecord::RecordNotFound
-    @title = t "messages.no_such_message.title"
-    render :action => "no_such_message", :status => :not_found
-  end
-
   private
 
   ##
diff --git a/app/controllers/oauth_clients_controller.rb b/app/controllers/oauth_clients_controller.rb
index b925002ec..c17d3856d 100644
--- a/app/controllers/oauth_clients_controller.rb
+++ b/app/controllers/oauth_clients_controller.rb
@@ -11,20 +11,6 @@ class OauthClientsController < ApplicationController
     @tokens = current_user.oauth_tokens.authorized
   end
 
-  def new
-    @client_application = ClientApplication.new
-  end
-
-  def create
-    @client_application = current_user.client_applications.build(application_params)
-    if @client_application.save
-      flash[:notice] = t "oauth_clients.create.flash"
-      redirect_to :action => "show", :id => @client_application.id
-    else
-      render :action => "new"
-    end
-  end
-
   def show
     @client_application = current_user.client_applications.find(params[:id])
   rescue ActiveRecord::RecordNotFound
@@ -32,6 +18,10 @@ class OauthClientsController < ApplicationController
     render :action => "not_found", :status => :not_found
   end
 
+  def new
+    @client_application = ClientApplication.new
+  end
+
   def edit
     @client_application = current_user.client_applications.find(params[:id])
   rescue ActiveRecord::RecordNotFound
@@ -39,6 +29,16 @@ class OauthClientsController < ApplicationController
     render :action => "not_found", :status => :not_found
   end
 
+  def create
+    @client_application = current_user.client_applications.build(application_params)
+    if @client_application.save
+      flash[:notice] = t "oauth_clients.create.flash"
+      redirect_to :action => "show", :id => @client_application.id
+    else
+      render :action => "new"
+    end
+  end
+
   def update
     @client_application = current_user.client_applications.find(params[:id])
     if @client_application.update(application_params)
diff --git a/app/controllers/redactions_controller.rb b/app/controllers/redactions_controller.rb
index 30425bd42..6e28f3625 100644
--- a/app/controllers/redactions_controller.rb
+++ b/app/controllers/redactions_controller.rb
@@ -14,10 +14,14 @@ class RedactionsController < ApplicationController
     @redactions = Redaction.order(:id)
   end
 
+  def show; end
+
   def new
     @redaction = Redaction.new
   end
 
+  def edit; end
+
   def create
     @redaction = Redaction.new
     @redaction.user = current_user
@@ -33,10 +37,6 @@ class RedactionsController < ApplicationController
     end
   end
 
-  def show; end
-
-  def edit; end
-
   def update
     # NOTE: don't update the user ID
     @redaction.title = params[:redaction][:title]
diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 23f8ce7ec..86b09215e 100644
--- a/app/controllers/traces_controller.rb
+++ b/app/controllers/traces_controller.rb
@@ -69,10 +69,6 @@ class TracesController < ApplicationController
     @target_user = target_user
   end
 
-  def mine
-    redirect_to :action => :index, :display_name => current_user.display_name
-  end
-
   def show
     @trace = Trace.find(params[:id])
 
@@ -93,6 +89,20 @@ class TracesController < ApplicationController
     @trace = Trace.new(:visibility => default_visibility)
   end
 
+  def edit
+    @trace = Trace.find(params[:id])
+
+    if !@trace.visible?
+      head :not_found
+    elsif current_user.nil? || @trace.user != current_user
+      head :forbidden
+    else
+      @title = t ".title", :name => @trace.name
+    end
+  rescue ActiveRecord::RecordNotFound
+    head :not_found
+  end
+
   def create
     @title = t ".upload_trace"
 
@@ -127,42 +137,6 @@ class TracesController < ApplicationController
     end
   end
 
-  def data
-    trace = Trace.find(params[:id])
-
-    if trace.visible? && (trace.public? || (current_user && current_user == trace.user))
-      if Acl.no_trace_download(request.remote_ip)
-        head :forbidden
-      elsif request.format == Mime[:xml]
-        send_data(trace.xml_file.read, :filename => "#{trace.id}.xml", :type => request.format.to_s, :disposition => "attachment")
-      elsif request.format == Mime[:gpx]
-        send_data(trace.xml_file.read, :filename => "#{trace.id}.gpx", :type => request.format.to_s, :disposition => "attachment")
-      elsif trace.file.attached?
-        redirect_to rails_blob_path(trace.file, :disposition => "attachment")
-      else
-        send_file(trace.trace_name, :filename => "#{trace.id}#{trace.extension_name}", :type => trace.mime_type, :disposition => "attachment")
-      end
-    else
-      head :not_found
-    end
-  rescue ActiveRecord::RecordNotFound
-    head :not_found
-  end
-
-  def edit
-    @trace = Trace.find(params[:id])
-
-    if !@trace.visible?
-      head :not_found
-    elsif current_user.nil? || @trace.user != current_user
-      head :forbidden
-    else
-      @title = t ".title", :name => @trace.name
-    end
-  rescue ActiveRecord::RecordNotFound
-    head :not_found
-  end
-
   def update
     @trace = Trace.find(params[:id])
 
@@ -199,6 +173,32 @@ class TracesController < ApplicationController
     head :not_found
   end
 
+  def mine
+    redirect_to :action => :index, :display_name => current_user.display_name
+  end
+
+  def data
+    trace = Trace.find(params[:id])
+
+    if trace.visible? && (trace.public? || (current_user && current_user == trace.user))
+      if Acl.no_trace_download(request.remote_ip)
+        head :forbidden
+      elsif request.format == Mime[:xml]
+        send_data(trace.xml_file.read, :filename => "#{trace.id}.xml", :type => request.format.to_s, :disposition => "attachment")
+      elsif request.format == Mime[:gpx]
+        send_data(trace.xml_file.read, :filename => "#{trace.id}.gpx", :type => request.format.to_s, :disposition => "attachment")
+      elsif trace.file.attached?
+        redirect_to rails_blob_path(trace.file, :disposition => "attachment")
+      else
+        send_file(trace.trace_name, :filename => "#{trace.id}#{trace.extension_name}", :type => trace.mime_type, :disposition => "attachment")
+      end
+    else
+      head :not_found
+    end
+  rescue ActiveRecord::RecordNotFound
+    head :not_found
+  end
+
   def georss
     @traces = Trace.visible_to_all.visible
 
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 933cec53d..603feb4db 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -17,6 +17,109 @@ class UsersController < ApplicationController
   before_action :lookup_user_by_name, :only => [:set_status, :destroy]
   before_action :allow_thirdparty_images, :only => [:show]
 
+  ##
+  # display a list of users matching specified criteria
+  def index
+    if request.post?
+      ids = params[:user].keys.collect(&:to_i)
+
+      User.where(:id => ids).update_all(:status => "confirmed") if params[:confirm]
+      User.where(:id => ids).update_all(:status => "deleted") if params[:hide]
+
+      redirect_to url_for(:status => params[:status], :ip => params[:ip], :page => params[:page])
+    else
+      @params = params.permit(:status, :ip)
+
+      conditions = {}
+      conditions[:status] = @params[:status] if @params[:status]
+      conditions[:creation_ip] = @params[:ip] if @params[:ip]
+
+      @user_pages, @users = paginate(:users,
+                                     :conditions => conditions,
+                                     :order => :id,
+                                     :per_page => 50)
+    end
+  end
+
+  def show
+    @user = User.find_by(:display_name => params[:display_name])
+
+    if @user &&
+       (@user.visible? || current_user&.administrator?)
+      @title = @user.display_name
+    else
+      render_unknown_user params[:display_name]
+    end
+  end
+
+  def new
+    @title = t "users.new.title"
+    @referer = if params[:referer]
+                 safe_referer(params[:referer])
+               else
+                 session[:referer]
+               end
+
+    append_content_security_policy_directives(
+      :form_action => %w[accounts.google.com *.facebook.com login.live.com github.com meta.wikimedia.org]
+    )
+
+    if current_user
+      # The user is logged in already, so don't show them the signup
+      # page, instead send them to the home page
+      redirect_to @referer || { :controller => "site", :action => "index" }
+    elsif params.key?(:auth_provider) && params.key?(:auth_uid)
+      self.current_user = User.new(:email => params[:email],
+                                   :email_confirmation => params[:email],
+                                   :display_name => params[:nickname],
+                                   :auth_provider => params[:auth_provider],
+                                   :auth_uid => params[:auth_uid])
+
+      flash.now[:notice] = render_to_string :partial => "auth_association"
+    else
+      check_signup_allowed
+
+      self.current_user = User.new
+    end
+  end
+
+  def create
+    self.current_user = User.new(user_params)
+
+    if check_signup_allowed(current_user.email)
+      session[:referer] = safe_referer(params[:referer]) if params[:referer]
+
+      Rails.logger.info "create: #{session[:referer]}"
+
+      if current_user.auth_provider.present? && current_user.pass_crypt.empty?
+        # We are creating an account with external authentication and
+        # no password was specified so create a random one
+        current_user.pass_crypt = SecureRandom.base64(16)
+        current_user.pass_crypt_confirmation = current_user.pass_crypt
+      end
+
+      if current_user.invalid?
+        # Something is wrong with a new user, so rerender the form
+        render :action => "new"
+      elsif current_user.auth_provider.present?
+        # Verify external authenticator before moving on
+        session[:new_user] = current_user
+        redirect_to auth_url(current_user.auth_provider, current_user.auth_uid), :status => :temporary_redirect
+      else
+        # Save the user record
+        session[:new_user] = current_user
+        redirect_to :action => :terms
+      end
+    end
+  end
+
+  ##
+  # destroy a user, marking them as deleted and removing personal data
+  def destroy
+    @user.soft_destroy!
+    redirect_to user_path(:display_name => params[:display_name])
+  end
+
   def terms
     @legale = params[:legale] || OSM.ip_to_country(request.remote_ip) || Settings.default_legale
     @text = OSM.legal_text_for_country(@legale)
@@ -121,78 +224,6 @@ class UsersController < ApplicationController
     redirect_to edit_account_path
   end
 
-  def new
-    @title = t "users.new.title"
-    @referer = if params[:referer]
-                 safe_referer(params[:referer])
-               else
-                 session[:referer]
-               end
-
-    append_content_security_policy_directives(
-      :form_action => %w[accounts.google.com *.facebook.com login.live.com github.com meta.wikimedia.org]
-    )
-
-    if current_user
-      # The user is logged in already, so don't show them the signup
-      # page, instead send them to the home page
-      redirect_to @referer || { :controller => "site", :action => "index" }
-    elsif params.key?(:auth_provider) && params.key?(:auth_uid)
-      self.current_user = User.new(:email => params[:email],
-                                   :email_confirmation => params[:email],
-                                   :display_name => params[:nickname],
-                                   :auth_provider => params[:auth_provider],
-                                   :auth_uid => params[:auth_uid])
-
-      flash.now[:notice] = render_to_string :partial => "auth_association"
-    else
-      check_signup_allowed
-
-      self.current_user = User.new
-    end
-  end
-
-  def create
-    self.current_user = User.new(user_params)
-
-    if check_signup_allowed(current_user.email)
-      session[:referer] = safe_referer(params[:referer]) if params[:referer]
-
-      Rails.logger.info "create: #{session[:referer]}"
-
-      if current_user.auth_provider.present? && current_user.pass_crypt.empty?
-        # We are creating an account with external authentication and
-        # no password was specified so create a random one
-        current_user.pass_crypt = SecureRandom.base64(16)
-        current_user.pass_crypt_confirmation = current_user.pass_crypt
-      end
-
-      if current_user.invalid?
-        # Something is wrong with a new user, so rerender the form
-        render :action => "new"
-      elsif current_user.auth_provider.present?
-        # Verify external authenticator before moving on
-        session[:new_user] = current_user
-        redirect_to auth_url(current_user.auth_provider, current_user.auth_uid), :status => :temporary_redirect
-      else
-        # Save the user record
-        session[:new_user] = current_user
-        redirect_to :action => :terms
-      end
-    end
-  end
-
-  def show
-    @user = User.find_by(:display_name => params[:display_name])
-
-    if @user &&
-       (@user.visible? || current_user&.administrator?)
-      @title = @user.display_name
-    else
-      render_unknown_user params[:display_name]
-    end
-  end
-
   ##
   # sets a user's status
   def set_status
@@ -205,37 +236,6 @@ class UsersController < ApplicationController
     redirect_to user_path(:display_name => params[:display_name])
   end
 
-  ##
-  # destroy a user, marking them as deleted and removing personal data
-  def destroy
-    @user.soft_destroy!
-    redirect_to user_path(:display_name => params[:display_name])
-  end
-
-  ##
-  # display a list of users matching specified criteria
-  def index
-    if request.post?
-      ids = params[:user].keys.collect(&:to_i)
-
-      User.where(:id => ids).update_all(:status => "confirmed") if params[:confirm]
-      User.where(:id => ids).update_all(:status => "deleted") if params[:hide]
-
-      redirect_to url_for(:status => params[:status], :ip => params[:ip], :page => params[:page])
-    else
-      @params = params.permit(:status, :ip)
-
-      conditions = {}
-      conditions[:status] = @params[:status] if @params[:status]
-      conditions[:creation_ip] = @params[:ip] if @params[:ip]
-
-      @user_pages, @users = paginate(:users,
-                                     :conditions => conditions,
-                                     :order => :id,
-                                     :per_page => 50)
-    end
-  end
-
   ##
   # omniauth success callback
   def auth_success