Merge 6bf21e9 into 41d7a49

app/controllers/glossary_terms/versions_controller.rb

@@ -12,13 +12,22 @@ def show
       return unless find_glossary_term!
 
       @glossary_term.revert_to(params[:version].to_i)
+      @versions = @glossary_term.versions
     end
 
     private
 
     def find_glossary_term!
-      @glossary_term = find_or_goto_index(GlossaryTerm,
-                                          params[:id].to_s)
+      @glossary_term = GlossaryTerm.show_includes.safe_find(params[:id]) ||
+                       flash_error_and_goto_index(GlossaryTerm, params[:id])
+    end
+
+    def show_includes
+      [
+        :images,
+        { thumb_image: :image_votes },
+        :user, :versions
+      ]
     end
   end
 end

app/controllers/glossary_terms_controller.rb

@@ -19,6 +19,7 @@ def show
     @canonical_url = glossary_term_url
     @layout = calc_layout_params
     @other_images = @glossary_term.other_images.order(vote_cache: :desc)
+    @versions = @glossary_term.versions
   end
 
   # ---------- Actions to Display forms -- (new, edit, etc.) -------------------
@@ -118,8 +119,10 @@ def show_selected_glossary_terms(query, args = {})
   # --------- show, create, edit private methods
 
   def find_glossary_term!
-    @glossary_term = find_or_goto_index(GlossaryTerm,
-                                        params[:id].to_s)
+    # @glossary_term = find_or_goto_index(GlossaryTerm,
+    #                                     params[:id].to_s)
+    @glossary_term = GlossaryTerm.show_includes.safe_find(params[:id]) ||
+                     flash_error_and_goto_index(GlossaryTerm, params[:id])
   end
 
   def redirect_non_admins!
@@ -132,7 +135,7 @@ def redirect_non_admins!
 
   def destroy_unused_images(images)
     images.each do |image|
-      image.destroy if image&.all_subjects&.empty?
+      image.destroy if image.reload&.all_subjects&.empty?
     end
   end
 

app/controllers/locations/descriptions/shared_private_methods.rb

@@ -6,9 +6,9 @@ module SharedPrivateMethods
 
     # This either finds a description by id, or sets the ivar from the param.
     def find_description!(id = nil)
-      return find_or_goto_index(LocationDescription, id) if id
-
-      @description = find_or_goto_index(LocationDescription, params[:id].to_s)
+      desc_id = id || params[:id]
+      @description = LocationDescription.show_includes.safe_find(desc_id) ||
+                     flash_error_and_goto_index(LocationDescription, desc_id)
     end
   end
 end

app/controllers/locations/descriptions/versions_controller.rb

@@ -3,18 +3,20 @@
 # show_past_location_description
 module Locations::Descriptions
   class VersionsController < ApplicationController
+    include ::Locations::Descriptions::SharedPrivateMethods
+
     before_action :login_required
 
     # Show past version of LocationDescription.  Accessible only from
     # show_location_description page.
     def show
       store_location
       pass_query_params
-      @description = find_or_goto_index(LocationDescription, params[:id].to_s)
-      return unless @description
+      return unless find_description!
 
       @location = @description.location
       @description.revert_to(params[:version].to_i)
+      @versions = @description.versions
     end
   end
 end

app/controllers/locations/descriptions_controller.rb

@@ -115,6 +115,7 @@ def show
       update_view_stats(@description)
       @canonical_url = description_canonical_url(@description)
       @projects = users_projects_which_dont_have_desc_of_this(@location)
+      @versions = @description.versions
     end
 
     def new

app/controllers/locations/versions_controller.rb

@@ -9,15 +9,24 @@ class VersionsController < ApplicationController
     def show
       store_location
       pass_query_params
-      @location = find_or_goto_index(Location, params[:id].to_s)
-      return unless @location
+      return unless find_location!
 
       if params[:version]
         @location.revert_to(params[:version].to_i)
+        @versions = @location.versions
       else
         flash_error(:show_past_location_no_version.t)
         redirect_to(location_path(@location.id))
       end
     end
+
+    def find_location!
+      @location = Location.show_includes.safe_find(params[:id]) ||
+                  flash_error_and_goto_index(Location, params[:id])
+    end
+
+    def show_includes
+      [:user, :versions]
+    end
   end
 end

app/controllers/locations_controller.rb

@@ -274,6 +274,7 @@ def show
     update_view_stats(@location)
     update_view_stats(@description) if @description
 
+    @versions = @location.versions
     init_projects_ivar
   end
 
@@ -360,7 +361,8 @@ def destroy
   private
 
   def find_location!
-    @location = find_or_goto_index(Location, params[:id].to_s)
+    @location = Location.show_includes.safe_find(params[:id]) ||
+                flash_error_and_goto_index(Location, params[:id])
   end
 
   def render_new

app/controllers/names/descriptions/review_status_controller.rb

@@ -9,8 +9,7 @@ class ReviewStatusController < ApplicationController
     # from the show_name page.
     def update
       pass_query_params
-      id = params[:id].to_s
-      desc = NameDescription.find(id)
+      desc = NameDescription.find(params[:id].to_s)
       desc.update_review_status(params[:value]) if reviewer?
       redirect_with_query(name_path(desc.name_id))
     end

app/controllers/names/descriptions/shared_private_methods.rb

@@ -6,9 +6,9 @@ module SharedPrivateMethods
 
     # This either finds a description by id, or sets the ivar from the param.
     def find_description!(id = nil)
-      return find_or_goto_index(NameDescription, id) if id
-
-      @description = find_or_goto_index(NameDescription, params[:id].to_s)
+      desc_id = id || params[:id]
+      @description = NameDescription.show_includes.safe_find(desc_id) ||
+                     flash_error_and_goto_index(NameDescription, desc_id)
     end
   end
 end

app/controllers/names/descriptions/versions_controller.rb

@@ -3,18 +3,20 @@
 # show_past_name_description
 module Names::Descriptions
   class VersionsController < ApplicationController
+    include ::Names::Descriptions::SharedPrivateMethods
+
     before_action :login_required
 
     # Show past versions of NameDescription.  Accessible only from
     # show_name_description page.
     def show
       pass_query_params
       store_location
-      @description = find_or_goto_index(NameDescription, params[:id].to_s)
-      return unless @description
+      return unless find_description!
 
       @name = @description.name
       @description.revert_to(params[:version].to_i)
+      @versions = @description.versions
     end
   end
 end

app/controllers/names/descriptions_controller.rb

@@ -130,6 +130,7 @@ def show
       update_view_stats(@description)
       @canonical_url = description_canonical_url(@description)
       @projects = users_projects_which_dont_have_desc_of_this(@name)
+      @versions = @description.versions
     end
 
     ############################################################################

app/controllers/names/versions_controller.rb

@@ -9,17 +9,28 @@ class VersionsController < ApplicationController
     def show
       pass_query_params
       store_location
-      @name = find_or_goto_index(Name, params[:id].to_s)
-      return unless @name
+      return unless find_name!
 
       @name.revert_to(params[:version].to_i)
+      @versions = @name.versions
       @correct_spelling = ""
       return unless @name.is_misspelling?
 
       # Old correct spellings could have gotten merged with something else
-      # and no longer exist.
+      # and no longer exist. Note: this is a second db lookup
       @correct_spelling = Name.where(id: @name.correct_spelling_id).
                           pluck(:display_name)
     end
+
+    def find_name!
+      @name = Name.show_includes.safe_find(params[:id]) ||
+              flash_error_and_goto_index(Name, params[:id])
+    end
+
+    def show_includes
+      [:correct_spelling,
+       { observations: :user },
+       :user, :versions]
+    end
   end
 end

app/controllers/names_controller.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+# rubocop:disable Metrics/ClassLength
 class NamesController < ApplicationController
   # disable cop because index is defined in ApplicationController
   # rubocop:disable Rails/LexicallyScopedActionFilter
@@ -218,17 +219,12 @@ def show
   private
 
   def find_name!
-    @name = Name.includes(show_name_includes).find_by(id: params[:id]) ||
+    @name = Name.show_includes.safe_find(params[:id]) ||
             flash_error_and_goto_index(Name, params[:id])
   end
 
-  def show_name_includes
-    [
-      { observations: :user }
-    ]
-  end
-
   def init_related_query_ivars
+    @versions = @name.versions
     # Create query for immediate children.
     @children_query = create_query(:Name, :all,
                                    names: @name.id,
@@ -670,3 +666,4 @@ def permitted_name_params
     params.permit(name: [:author, :citation, :icn_id, :locked, :notes, :rank])
   end
 end
+# rubocop:enable Metrics/ClassLength

app/controllers/observations_controller/show.rb

@@ -44,7 +44,7 @@ def show
 
   def load_observation_for_show_observation_page
     includes = @user ? "show_includes" : "not_logged_in_show_includes" # scopes
-    @observation = Observation.send(includes).find_by(id: params[:id]) ||
+    @observation = Observation.send(includes).safe_find(params[:id]) ||
                    flash_error_and_goto_index(Observation, params[:id])
   end
 

app/helpers/footer_helper.rb

@@ -12,13 +12,15 @@ module FooterHelper
   #     Editors: <user>, <user>, ..., <user>
   #   </p>
   #
-  def show_authors_and_editors(obj:, user:)
+  # New: Must pass in @versions to avoid these and other helpers doing
+  # duplicate version lookups, which are slow.
+  def show_authors_and_editors(obj:, versions:, user:)
     type = obj.type_tag
 
     authors, editors = if /description/.match?(type.to_s)
                          html_description_authors_and_editors(obj, user)
                        else
-                         html_undescribed_obj_authors_and_editors(obj)
+                         html_undescribed_obj_authors_and_editors(obj, versions)
                        end
 
     content_tag(:p, authors + safe_br + editors)
@@ -64,10 +66,10 @@ def authors_plus_author_request(obj, authors)
     authors
   end
 
-  def html_undescribed_obj_authors_and_editors(obj)
+  def html_undescribed_obj_authors_and_editors(obj, versions)
     type = obj.type_tag
 
-    editors = obj.versions.map(&:user_id).uniq - [obj.user_id]
+    editors = versions.map(&:user_id).uniq - [obj.user_id]
     editors = User.where(id: editors).to_a
     authors = user_list(:"show_#{type}_creator", [obj.user])
     editors = user_list(:"show_#{type}_editor", editors)
@@ -112,13 +114,13 @@ def html_undescribed_obj_authors_and_editors(obj)
   #     </span>
   #   </p>
   #
-  def show_object_footer(obj)
-    num_versions = obj.respond_to?(:version) ? obj.versions.length : 0
+  def show_object_footer(obj, versions = [])
+    num_versions = versions.length
 
     html = if num_versions.positive? && obj.version < num_versions
              html_for_old_version_of_versioned_object(obj, num_versions)
            else
-             html_for_latest_version_or_non_versioned_object(obj, num_versions)
+             html_for_latest_version_or_non_versioned_object(obj, versions)
            end
 
     html.concat(link_to_rss_log(obj))
@@ -134,7 +136,7 @@ def html_for_old_version_of_versioned_object(obj, num_versions)
     html = [:footer_version_out_of.t(num: obj.version, total: num_versions)]
     return html unless obj.updated_at
 
-    html << :footer_updated_by.t(user: user_link(obj.user),
+    html << :footer_updated_by.t(user: user_link(obj.user_id),
                                  date: obj.updated_at.web_time)
   end
 
@@ -146,9 +148,9 @@ def link_to_rss_log(obj)
     end
   end
 
-  def html_for_latest_version_or_non_versioned_object(obj, num_versions)
-    html = if num_versions.positive?
-             html_for_latest_version(obj)
+  def html_for_latest_version_or_non_versioned_object(obj, versions)
+    html = if versions.length.positive?
+             html_for_latest_version(obj, versions)
            else
              html_for_non_versioned_object(obj)
            end
@@ -162,13 +164,16 @@ def html_for_latest_version_or_non_versioned_object(obj, num_versions)
     html
   end
 
-  def html_for_latest_version(obj)
-    latest_user = User.safe_find(obj.versions.latest.user_id)
+  def html_for_latest_version(obj, versions)
+    # This is yet another db lookup of users - let's try skipping it.
+    # latest_user = User.safe_find(versions.latest.user_id)
     html = html_created_by(obj)
 
-    if latest_user && obj.updated_at
-      html << :footer_last_updated_by.t(user: user_link(latest_user),
-                                        date: obj.updated_at.web_time)
+    if versions.latest.user_id && obj.updated_at
+      html << :footer_last_updated_by.t(
+        user: user_link(versions.latest.user_id),
+        date: obj.updated_at.web_time
+      )
     elsif obj.updated_at
       html << :footer_last_updated_at.t(date: obj.updated_at.web_time)
     end
@@ -178,7 +183,7 @@ def html_for_latest_version(obj)
 
   def html_created_by(obj)
     if obj.created_at
-      [:footer_created_by.t(user: user_link(obj.user),
+      [:footer_created_by.t(user: user_link(obj.user_id),
                             date: obj.created_at.web_time)]
     else
       []
@@ -207,6 +212,7 @@ def html_num_views(obj)
     :footer_viewed.t(date: date, times: times)
   end
 
+  # only for obs
   def html_last_viewed_by(obj)
     time = obj.old_last_viewed_by(User.current)&.web_time || :footer_never.l
     :footer_last_you_viewed.t(date: time)