Instrument PDF generation and make fields optional (#513)

* Instrument PDFs and relax optional fields

* Fix full-suite order dependencies

* Address PDF and unit review feedback

* Align unit PDF inspection ordering

* Verify unit PDF inspection reuse

Author: stefan-burke

app/controllers/inspections_controller.rb

@@ -354,7 +354,7 @@ def filtered_inspections_query_without_order = current_user.inspections
   def no_index = response.set_header("X-Robots-Tag", "noindex,nofollow")
 
   def set_inspection
-    @inspection = Inspection
+    inspection_query = Inspection
       .includes(
         :user, :inspector_company,
         *Inspection::ALL_ASSESSMENT_TYPES.keys,
@@ -363,7 +363,19 @@ def set_inspection
         photo_2_attachment: :blob,
         photo_3_attachment: :blob
       )
-      .find_by(id: params[:id]&.upcase)
+    inspection_id = params[:id]&.upcase
+
+    @inspection = if request.format.pdf?
+      PdfPerformance.measure(
+        :record_load,
+        pdf_type: :inspection,
+        record_id: inspection_id
+      ) do
+        inspection_query.find_by(id: inspection_id)
+      end
+    else
+      inspection_query.find_by(id: inspection_id)
+    end
 
     head :not_found unless @inspection
   end
@@ -432,14 +444,26 @@ def handle_failed_update
   end
 
   def send_inspection_pdf
-    result = PdfCacheService.fetch_or_generate_inspection_pdf(
-      @inspection,
-      debug_enabled: admin_debug_enabled?,
-      debug_queries: debug_sql_queries
-    )
-    @inspection.update(pdf_last_accessed_at: Time.current)
+    PdfPerformance.measure(
+      :total,
+      pdf_type: :inspection,
+      record_id: @inspection.id
+    ) do
+      result = PdfCacheService.fetch_or_generate_inspection_pdf(
+        @inspection,
+        debug_enabled: admin_debug_enabled?,
+        debug_queries: debug_sql_queries
+      )
+      PdfPerformance.measure(
+        :access_tracking,
+        pdf_type: :inspection,
+        record_id: @inspection.id
+      ) do
+        @inspection.update(pdf_last_accessed_at: Time.current)
+      end
 
-    handle_pdf_response(result, pdf_filename)
+      handle_pdf_response(result, pdf_filename)
+    end
   end
 
   def send_inspection_qr_code

app/controllers/units_controller.rb

@@ -196,9 +196,22 @@ def normalize_unit_id(raw_id)
 
   def no_index = response.set_header("X-Robots-Tag", "noindex,nofollow")
 
+  sig { void }
   def set_unit
-    @unit = Unit.includes(photo_attachment: :blob)
-      .find_by(id: params[:id].upcase)
+    unit_id = params[:id].upcase
+    unit_query = Unit.includes(photo_attachment: :blob)
+
+    @unit = if request.format.pdf?
+      PdfPerformance.measure(
+        :record_load,
+        pdf_type: :unit,
+        record_id: unit_id
+      ) do
+        unit_query.find_by(id: unit_id)
+      end
+    else
+      unit_query.find_by(id: unit_id)
+    end
 
     unless @unit
       # Always return 404 for non-existent resources regardless of login status
@@ -220,14 +233,19 @@ def check_assessments_enabled
   end
 
   def send_unit_pdf
-    # Unit already has photo loaded from set_unit
-    result = PdfCacheService.fetch_or_generate_unit_pdf(
-      @unit,
-      debug_enabled: admin_debug_enabled?,
-      debug_queries: debug_sql_queries
-    )
-
-    handle_pdf_response(result, pdf_filename)
+    PdfPerformance.measure(
+      :total,
+      pdf_type: :unit,
+      record_id: @unit.id
+    ) do
+      result = PdfCacheService.fetch_or_generate_unit_pdf(
+        @unit,
+        debug_enabled: admin_debug_enabled?,
+        debug_queries: debug_sql_queries
+      )
+
+      handle_pdf_response(result, pdf_filename)
+    end
   end
 
   def send_unit_qr_code

app/models/assessment_schema.rb

@@ -61,6 +61,9 @@ def initialize(raw)
     sig { returns(T::Boolean) }
     def required? = !!attributes[:required]
 
+    sig { returns(T::Boolean) }
+    def optional_for_completion? = attributes[:required] == false
+
     sig { returns(T::Boolean) }
     def numeric? = NUMERIC_PARTIALS.include?(partial)
 
@@ -163,6 +166,14 @@ def add_not_applicable_fields
     fields.select(&:add_not_applicable?).map(&:name)
   end
 
+  sig { returns(T::Array[Symbol]) }
+  def completion_optional_fields
+    @completion_optional_fields ||= fields
+      .select(&:optional_for_completion?)
+      .flat_map { [it.name, *it.composite_fields] }
+      .freeze
+  end
+
   # Returns a new schema with the named fields removed from each fieldset.
   # Used for permission-driven filtering (e.g. hiding admin-only fields).
   sig { params(field_names: Symbol).returns(AssessmentSchema) }

app/models/concerns/assessment_completion.rb

@@ -19,7 +19,10 @@ def complete?
 
   sig { returns(T::Array[Symbol]) }
   def incomplete_fields
-    (self.class.column_name_syms - SYSTEM_FIELDS)
+    fields = self.class.column_name_syms - SYSTEM_FIELDS
+    fields -= self.class.assessment_schema.completion_optional_fields
+
+    fields
       .reject { |f| f.end_with?("_comment") }
       .select { |f| field_is_incomplete?(f) }
       .reject { |f| field_allows_nil_when_na?(f) }

app/models/inspection.rb

@@ -474,6 +474,11 @@ def invalidate_pdf_cache
 
   sig { void }
   def invalidate_unit_pdf_cache
+    changed_attrs = saved_changes.keys
+    ignorable_attrs = ["pdf_last_accessed_at", "updated_at"]
+
+    return if (changed_attrs - ignorable_attrs).empty?
+
     PdfCacheService.invalidate_unit_cache(unit) if unit
   end
 end

app/models/unit.rb

@@ -21,7 +21,6 @@
 # Indexes
 #
 #  index_units_on_is_seed                  (is_seed)
-#  index_units_on_manufacturer_and_serial  (manufacturer,serial) UNIQUE
 #  index_units_on_serial_and_user_id       (serial,user_id) UNIQUE
 #  index_units_on_unit_type                (unit_type)
 #  index_units_on_user_id                  (user_id)
@@ -67,8 +66,7 @@ class Unit < ApplicationRecord
   before_destroy :check_complete_inspections
   before_destroy :destroy_draft_inspections
 
-  # All fields are required for Units
-  validates :name, :serial, :description, :manufacturer, presence: true
+  validates :description, :name, :serial, presence: true
   validates :serial, uniqueness: {scope: [:user_id]}
   validate :badge_id_valid, on: :create, if: -> { unit_badges_enabled? }
 

app/services/pdf_cache_service.rb

@@ -17,12 +17,11 @@ class << self
         .returns(CacheResult)
     end
     def fetch_or_generate_inspection_pdf(inspection, **options)
-      # Never cache incomplete inspections
-      unless caching_enabled? && inspection.complete?
-        return generate_pdf_result(inspection, :inspection, **options)
+      if caching_enabled? && inspection.complete?
+        fetch_or_generate(inspection, :inspection, **options)
+      else
+        generate_pdf_result(inspection, :inspection, **options)
       end
-
-      fetch_or_generate(inspection, :inspection, **options)
     end
 
     sig { params(unit: Unit, options: T.untyped).returns(CacheResult) }
@@ -52,8 +51,14 @@ def invalidate_unit_cache(unit)
       ).returns(CacheResult)
     end
     def fetch_or_generate(record, type, **options)
-      valid_cache = record.cached_pdf.attached? &&
-        cached_pdf_valid?(record.cached_pdf, record)
+      valid_cache = PdfPerformance.measure(
+        :cache_lookup,
+        pdf_type: type,
+        record_id: record.id
+      ) do
+        record.cached_pdf.attached? &&
+          cached_pdf_valid?(record.cached_pdf, record)
+      end
 
       if valid_cache
         Rails.logger.info "PDF cache hit for #{type} #{record.id}"
@@ -79,7 +84,13 @@ def fetch_or_generate(record, type, **options)
     end
     def generate_and_cache(record, type, **options)
       result = generate_pdf_result(record, type, **options)
-      store_cached_pdf(record, result.data)
+      PdfPerformance.measure(
+        :cache_store,
+        pdf_type: type,
+        record_id: record.id
+      ) do
+        store_cached_pdf(record, result.data)
+      end
       result
     end
 
@@ -91,14 +102,28 @@ def generate_and_cache(record, type, **options)
       ).returns(CacheResult)
     end
     def generate_pdf_result(record, type, **options)
-      pdf_document = case type
-      when :inspection
-        PdfGeneratorService.generate_inspection_report(record, **options)
-      when :unit
-        PdfGeneratorService.generate_unit_report(record, **options)
+      pdf_document = PdfPerformance.measure(
+        :document_build,
+        pdf_type: type,
+        record_id: record.id
+      ) do
+        case type
+        when :inspection
+          PdfGeneratorService.generate_inspection_report(record, **options)
+        when :unit
+          PdfGeneratorService.generate_unit_report(record, **options)
+        end
+      end
+
+      pdf_data = PdfPerformance.measure(
+        :document_render,
+        pdf_type: type,
+        record_id: record.id
+      ) do
+        pdf_document.render
       end
 
-      CacheResult.new(type: :pdf_data, data: pdf_document.render)
+      CacheResult.new(type: :pdf_data, data: pdf_data)
     end
 
     sig { params(record: T.any(Inspection, Unit)).void }