From b0989cf1077de07e0fba771e277e17816068bf6b Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 15 Jul 2026 12:27:44 +0100 Subject: [PATCH 1/5] Instrument PDFs and relax optional fields --- app/controllers/inspections_controller.rb | 42 ++++-- app/controllers/units_controller.rb | 38 +++-- app/models/assessment_schema.rb | 11 ++ app/models/concerns/assessment_completion.rb | 5 +- app/models/inspection.rb | 5 + app/models/unit.rb | 3 +- app/services/pdf_cache_service.rb | 53 +++++-- app/services/pdf_generator_service.rb | 131 +++++++++++++----- .../pdf_generator_service/image_processor.rb | 88 ++++++++---- .../pdf_generator_service/photos_renderer.rb | 1 - app/services/pdf_performance.rb | 36 +++++ app/views/units/_form.html.erb | 17 ++- app/views/units/edit.html.erb | 2 +- app/views/units/new.html.erb | 2 +- app/views/units/new_from_inspection.html.erb | 1 + config/forms/structure_assessment.yml | 5 + config/forms/user_height_assessment.yml | 4 + config/locales/units.en.yml | 1 - ...structure_assessment_trough_fields_spec.rb | 11 +- .../inspection_incomplete_fields_spec.rb | 29 +++- spec/features/pdfs/pdf_integration_spec.rb | 31 +++++ spec/features/units/units_form_spec.rb | 41 ++++-- spec/models/assessment_schema_spec.rb | 37 +++++ .../concerns/assessment_completion_spec.rb | 29 ++++ spec/models/inspection_spec.rb | 4 + spec/models/unit_spec.rb | 11 +- spec/requests/inspections/inspections_spec.rb | 2 + .../image_processor_spec.rb | 17 +++ .../photos_renderer_spec.rb | 4 +- spec/services/pdf_performance_spec.rb | 51 +++++++ 30 files changed, 586 insertions(+), 126 deletions(-) create mode 100644 app/services/pdf_performance.rb create mode 100644 spec/services/pdf_performance_spec.rb diff --git a/app/controllers/inspections_controller.rb b/app/controllers/inspections_controller.rb index ad1185320..80d95c635 100644 --- a/app/controllers/inspections_controller.rb +++ b/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 diff --git a/app/controllers/units_controller.rb b/app/controllers/units_controller.rb index 24ffd2efc..ab0e91054 100644 --- a/app/controllers/units_controller.rb +++ b/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 diff --git a/app/models/assessment_schema.rb b/app/models/assessment_schema.rb index 3ccadde36..db6b04649 100644 --- a/app/models/assessment_schema.rb +++ b/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) } diff --git a/app/models/concerns/assessment_completion.rb b/app/models/concerns/assessment_completion.rb index 17474cb57..deb5b62d2 100644 --- a/app/models/concerns/assessment_completion.rb +++ b/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) } diff --git a/app/models/inspection.rb b/app/models/inspection.rb index 2a2f11fe1..d13009b48 100644 --- a/app/models/inspection.rb +++ b/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 diff --git a/app/models/unit.rb b/app/models/unit.rb index 90f222780..94da5c992 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -67,8 +67,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? } diff --git a/app/services/pdf_cache_service.rb b/app/services/pdf_cache_service.rb index 69a3e4f6e..d2b00cd0a 100644 --- a/app/services/pdf_cache_service.rb +++ b/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 } diff --git a/app/services/pdf_generator_service.rb b/app/services/pdf_generator_service.rb index bb30af14a..d0dec80de 100644 --- a/app/services/pdf_generator_service.rb +++ b/app/services/pdf_generator_service.rb @@ -13,40 +13,74 @@ def self.generate_inspection_report(inspection, debug_enabled: false, debug_quer # Initialize array to collect all assessment blocks assessment_blocks = [] - # Header section - HeaderGenerator.generate_inspection_pdf_header(pdf, inspection) - - # Unit details section - generate_inspection_unit_details(pdf, inspection) - - # Risk assessment section (if present) - generate_risk_assessment_section(pdf, inspection) - - # Generate all assessment sections in the correct UI order from applicable_tabs - generate_assessments_in_ui_order(inspection, assessment_blocks) + PdfPerformance.measure( + :primary_content, + pdf_type: :inspection, + record_id: inspection.id + ) do + HeaderGenerator.generate_inspection_pdf_header(pdf, inspection) + generate_inspection_unit_details(pdf, inspection) + generate_risk_assessment_section(pdf, inspection) + end + + PdfPerformance.measure( + :assessment_data, + pdf_type: :inspection, + record_id: inspection.id + ) do + generate_assessments_in_ui_order(inspection, assessment_blocks) + end # Render footer and photo first to measure actual space used cursor_before_footer = pdf.cursor - # Disclaimer footer (only on first page) - DisclaimerFooterRenderer.render_disclaimer_footer(pdf, inspection.user) - disclaimer_height = DisclaimerFooterRenderer.measure_footer_height(unbranded: false) - - # Add unit photo in bottom right corner - photo_height = ImageProcessor.measure_unit_photo_height(pdf, inspection.unit, 4) - ImageProcessor.add_unit_photo_footer(pdf, inspection.unit, 4) if inspection.unit&.photo + disclaimer_height, photo_height = PdfPerformance.measure( + :footer, + pdf_type: :inspection, + record_id: inspection.id + ) do + DisclaimerFooterRenderer.render_disclaimer_footer(pdf, inspection.user) + measured_footer = DisclaimerFooterRenderer.measure_footer_height( + unbranded: false + ) + measured_photo = ImageProcessor.add_unit_photo_footer( + pdf, + inspection.unit, + 4, + pdf_type: :inspection, + record_id: inspection.id + ) + [measured_footer, measured_photo] + end # Reset cursor to render assessments with proper space accounting pdf.move_cursor_to(cursor_before_footer) # Render all collected assessments in newspaper-style columns - render_assessment_blocks_in_columns(pdf, assessment_blocks, disclaimer_height, photo_height) + PdfPerformance.measure( + :assessment_layout, + pdf_type: :inspection, + record_id: inspection.id + ) do + render_assessment_blocks_in_columns( + pdf, + assessment_blocks, + disclaimer_height, + photo_height + ) + end # Add DRAFT watermark overlay for draft inspections (except in test env) Utilities.add_draft_watermark(pdf) if !inspection.complete? && !Rails.env.test? # Add photos page if photos are attached - PhotosRenderer.generate_photos_page(pdf, inspection) + PdfPerformance.measure( + :report_photos, + pdf_type: :inspection, + record_id: inspection.id + ) do + PhotosRenderer.generate_photos_page(pdf, inspection) + end # Add debug info page if enabled (admins only) DebugInfoRenderer.add_debug_info_page(pdf, debug_queries) if debug_enabled && debug_queries.present? @@ -58,25 +92,56 @@ def self.generate_unit_report(unit, debug_enabled: false, debug_queries: []) unbranded = Rails.configuration.units.reports_unbranded - # Preload all inspections once to avoid N+1 queries - completed_inspections = unit.inspections - .includes(:user, inspector_company: {logo_attachment: :blob}) - .complete - .order(inspection_date: :desc) + completed_inspections = PdfPerformance.measure( + :inspection_history_load, + pdf_type: :unit, + record_id: unit.id + ) do + unit.inspections + .includes(:user, inspector_company: {logo_attachment: :blob}) + .complete + .order(inspection_date: :desc) + .load + end last_inspection = completed_inspections.first Prawn::Document.new(page_size: "A4", page_layout: :portrait) do |pdf| Configuration.setup_pdf_fonts(pdf) - HeaderGenerator.generate_unit_pdf_header(pdf, unit, unbranded: unbranded) - generate_unit_details_with_inspection(pdf, unit, last_inspection) - generate_unit_inspection_history_with_data(pdf, unit, completed_inspections) - - # Disclaimer footer (only on first page, not for unbranded reports) - DisclaimerFooterRenderer.render_disclaimer_footer(pdf, unit.user, unbranded: unbranded) - # Add unit photo in bottom right corner (for unit PDFs, always use 3 columns) - ImageProcessor.add_unit_photo_footer(pdf, unit, 3) if unit.photo + PdfPerformance.measure( + :primary_content, + pdf_type: :unit, + record_id: unit.id + ) do + HeaderGenerator.generate_unit_pdf_header(pdf, unit, unbranded: unbranded) + generate_unit_details_with_inspection(pdf, unit, last_inspection) + end + + PdfPerformance.measure( + :inspection_history_layout, + pdf_type: :unit, + record_id: unit.id + ) do + generate_unit_inspection_history_with_data( + pdf, + unit, + completed_inspections + ) + end + + PdfPerformance.measure( + :footer, + pdf_type: :unit, + record_id: unit.id + ) do + DisclaimerFooterRenderer.render_disclaimer_footer( + pdf, + unit.user, + unbranded: unbranded + ) + ImageProcessor.add_unit_photo_footer(pdf, unit, 3) if unit.photo + end # Add debug info page if enabled (admins only) DebugInfoRenderer.add_debug_info_page(pdf, debug_queries) if debug_enabled && debug_queries.present? diff --git a/app/services/pdf_generator_service/image_processor.rb b/app/services/pdf_generator_service/image_processor.rb index 80ea6308e..c0132c732 100644 --- a/app/services/pdf_generator_service/image_processor.rb +++ b/app/services/pdf_generator_service/image_processor.rb @@ -18,30 +18,22 @@ def self.generate_qr_code_header(pdf, entity) pdf.image StringIO.new(qr_code_png), image_options end - def self.add_unit_photo_footer(pdf, unit, column_count = 3) - return unless unit&.photo&.blob + def self.add_unit_photo_footer( + pdf, + unit, + column_count = 3, + pdf_type: :unit, + record_id: unit&.id + ) + return 0 unless unit&.photo&.blob pdf_width = pdf.bounds.width attachment = unit.photo - image = create_image(attachment) + context = performance_context(attachment).merge(pdf_type:, record_id:) + image = create_image(attachment, context) dimensions = calculate_footer_photo_dimensions(pdf, image, column_count) photo_width, photo_height = dimensions - photo_x = pdf_width - photo_width - photo_y = calculate_photo_y(pdf, photo_height) - - render_processed_image(pdf, image, photo_x, photo_y, - photo_width, photo_height, attachment) - end - - def self.measure_unit_photo_height(pdf, unit, column_count = 3) - return 0 unless unit&.photo&.blob - - attachment = unit.photo - image = create_image(attachment) - dimensions = calculate_footer_photo_dimensions(pdf, image, column_count) - _photo_width, photo_height = dimensions - if photo_height <= 0 error_message = I18n.t( "pdf_generator.errors.zero_photo_height", @@ -50,15 +42,27 @@ def self.measure_unit_photo_height(pdf, unit, column_count = 3) raise error_message end + photo_x = pdf_width - photo_width + photo_y = calculate_photo_y(pdf, photo_height) + + render_processed_image(pdf, image, photo_x, photo_y, + photo_width, photo_height, attachment, context) + photo_height rescue Prawn::Errors::UnsupportedImageType => e raise ImageError.build_detailed_error(e, attachment) end def self.process_image_with_orientation(attachment) - image = create_image(attachment) + context = performance_context(attachment) + image = create_image(attachment, context) # Images are already orientation-corrected from upload processing - image.write_to_buffer(".png") + PdfPerformance.measure( + :image_transcode, + **context + ) do + image.write_to_buffer(".png") + end end def self.calculate_footer_photo_dimensions(pdf, image, column_count = 3) @@ -88,9 +92,15 @@ def self.calculate_height_from_aspect(photo_width, original_width, end end - def self.render_processed_image(pdf, image, x, y, width, height, attachment) + def self.render_processed_image(pdf, image, x, y, width, height, attachment, + context) # Images are already orientation-corrected from upload processing - processed_image = image.write_to_buffer(".png") + processed_image = PdfPerformance.measure( + :image_transcode, + **context + ) do + image.write_to_buffer(".png") + end image_options = { at: [x, y], @@ -98,13 +108,28 @@ def self.render_processed_image(pdf, image, x, y, width, height, attachment) height: height } ImageError.with_error_handling(attachment) do - pdf.image StringIO.new(processed_image), image_options + PdfPerformance.measure( + :image_embed, + **context + ) do + pdf.image StringIO.new(processed_image), image_options + end end end - def self.create_image(attachment) - image_data = attachment.blob.download - Vips::Image.new_from_buffer(image_data, "") + def self.create_image(attachment, context) + image_data = PdfPerformance.measure( + :image_download, + **context + ) do + attachment.blob.download + end + PdfPerformance.measure( + :image_decode, + **context + ) do + Vips::Image.new_from_buffer(image_data, "") + end end def self.calculate_photo_y(pdf, photo_height) @@ -115,5 +140,16 @@ def self.calculate_photo_y(pdf, photo_height) Configuration::QR_CODE_BOTTOM_OFFSET + photo_height end end + + def self.performance_context(attachment) + record = attachment.record + { + pdf_type: record.class.model_name.singular.to_sym, + record_id: record.id, + attachment: attachment.name + } + end + + private_class_method :performance_context end end diff --git a/app/services/pdf_generator_service/photos_renderer.rb b/app/services/pdf_generator_service/photos_renderer.rb index b159a6f05..a522368e3 100644 --- a/app/services/pdf_generator_service/photos_renderer.rb +++ b/app/services/pdf_generator_service/photos_renderer.rb @@ -78,7 +78,6 @@ def self.calculate_needed_space(max_photo_height) def self.render_photo(pdf, photo, label, max_height) ImageError.with_error_handling(photo) do - photo.blob.download processed_image = ImageProcessor.process_image_with_orientation(photo) image_width, image_height = calculate_photo_dimensions_from_blob( diff --git a/app/services/pdf_performance.rb b/app/services/pdf_performance.rb new file mode 100644 index 000000000..2b1331fe3 --- /dev/null +++ b/app/services/pdf_performance.rb @@ -0,0 +1,36 @@ +# typed: strict +# frozen_string_literal: true + +class PdfPerformance + extend T::Sig + + EVENT = "pdf.performance" + + class << self + extend T::Sig + + sig do + params( + stage: Symbol, + pdf_type: Symbol, + record_id: T.untyped, + metadata: T.untyped, + block: T.proc.returns(T.untyped) + ).returns(T.untyped) + end + def measure(stage, pdf_type:, record_id:, **metadata, &block) + started_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + block.call + ensure + duration = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started_at + Rails.logger.info({ + event: EVENT, + stage: stage, + pdf_type: pdf_type, + record_id: record_id, + duration_ms: (duration * 1000).round(1), + **metadata + }) + end + end +end diff --git a/app/views/units/_form.html.erb b/app/views/units/_form.html.erb index 914d7f01c..e713571b8 100644 --- a/app/views/units/_form.html.erb +++ b/app/views/units/_form.html.erb @@ -1,7 +1,7 @@ <% - # Determine form context - unit = @unit || local_assigns[:unit] - form_url = local_assigns[:form_url] || nil + unit = local_assigns.fetch(:unit) + form_url = local_assigns[:form_url] + prefilled_badge = local_assigns.fetch(:prefilled_badge, false) %> <%= render 'chobble_forms/form_context', @@ -11,10 +11,10 @@ <%= render 'chobble_forms/fieldset', legend_key: 'unit_details' do %> <%# Show ID field when UNIT_BADGES is enabled %> <% if Rails.configuration.units.badges_enabled %> - <% if unit.persisted? || @prefilled_badge %> + <% if unit.persisted? || prefilled_badge %> <%# Display ID as read-only for existing units or pre-filled badges %> <%= render 'chobble_forms/display_field', field: :id %> - <% if @prefilled_badge %> + <% if prefilled_badge %> <%= form.hidden_field :id %> <% end %> <% else %> @@ -52,14 +52,13 @@ <%= render 'chobble_forms/field_with_link', field_type: :text_field, field: :manufacturer, - required: true, link_url: units_path(manufacturer: unit.manufacturer), link_text: t('units.labels.all_count', count: manufacturer_count) %> <% else %> - <%= render 'chobble_forms/text_field', field: :manufacturer, required: true %> + <%= render 'chobble_forms/text_field', field: :manufacturer %> <% end %> <% else %> - <%= render 'chobble_forms/text_field', field: :manufacturer, required: true %> + <%= render 'chobble_forms/text_field', field: :manufacturer %> <% end %> <%= render 'chobble_forms/text_field', field: :serial, required: true %> @@ -76,7 +75,7 @@

- <%= unit.last_inspection.width %>m × <%= unit.last_inspection.length %>m × <%= unit.last_inspection.height %>m + <%= unit.last_inspection.width %><%= t("shared.metres_by") %> <%= unit.last_inspection.length %><%= t("shared.metres_by") %> <%= unit.last_inspection.height %>m <%= t("units.labels.from_last_inspection") %>

diff --git a/app/views/units/edit.html.erb b/app/views/units/edit.html.erb index a9187fdd5..4de0a5c0c 100644 --- a/app/views/units/edit.html.erb +++ b/app/views/units/edit.html.erb @@ -3,7 +3,7 @@ <%= render 'shared/action_buttons', actions: unit_actions(@unit) %>
-<%= render 'form' %> +<%= render 'form', unit: @unit %> <% if @unit.deletable? %> <%= button_to t('units.buttons.delete'), unit_path(@unit), diff --git a/app/views/units/new.html.erb b/app/views/units/new.html.erb index 36da3112e..e9a41f64a 100644 --- a/app/views/units/new.html.erb +++ b/app/views/units/new.html.erb @@ -1,2 +1,2 @@ <%= render 'shared/page_header', title: t('units.titles.new') %> -<%= render 'form' %> +<%= render 'form', unit: @unit, prefilled_badge: @prefilled_badge %> diff --git a/app/views/units/new_from_inspection.html.erb b/app/views/units/new_from_inspection.html.erb index 477b89ae2..eca763c14 100644 --- a/app/views/units/new_from_inspection.html.erb +++ b/app/views/units/new_from_inspection.html.erb @@ -2,6 +2,7 @@ <%= render 'units/form', unit: @unit, + prefilled_badge: false, form_url: create_unit_from_inspection_path(@inspection) %> <%= link_to t("shared.buttons.cancel"), inspection_path(@inspection) %> diff --git a/config/forms/structure_assessment.yml b/config/forms/structure_assessment.yml index ca4743c12..3c27231c2 100644 --- a/config/forms/structure_assessment.yml +++ b/config/forms/structure_assessment.yml @@ -10,6 +10,8 @@ form_fields: field: :air_loss - partial: :pass_fail_comment field: :straight_walls + attributes: + required: false - partial: :pass_fail_comment field: :sharp_edges - legend_i18n_key: structural_safety @@ -23,6 +25,7 @@ form_fields: attributes: step: 1 min: 0 + required: false - partial: :number_pass_fail_comment field: :platform_height attributes: @@ -45,10 +48,12 @@ form_fields: attributes: min: 0 add_not_applicable: true + required: false - partial: :integer_comment field: :trough_adjacent_panel_width attributes: min: 0 + required: false - partial: :pass_fail_comment field: :trough - legend_i18n_key: safety_compliance diff --git a/config/forms/user_height_assessment.yml b/config/forms/user_height_assessment.yml index b7d4b3e1e..096eb627d 100644 --- a/config/forms/user_height_assessment.yml +++ b/config/forms/user_height_assessment.yml @@ -31,21 +31,25 @@ form_fields: attributes: min: 0 step: 1 + required: false - partial: :number field: :users_at_1200mm attributes: min: 0 step: 1 + required: false - partial: :number field: :users_at_1500mm attributes: min: 0 step: 1 + required: false - partial: :number field: :users_at_1800mm attributes: min: 0 step: 1 + required: false - legend_i18n_key: tallest_user_capacity fields: - partial: :text_area diff --git a/config/locales/units.en.yml b/config/locales/units.en.yml index d3618adec..ced24ecea 100644 --- a/config/locales/units.en.yml +++ b/config/locales/units.en.yml @@ -115,7 +115,6 @@ en: validations: id_blank: ID can't be blank name_blank: Name can't be blank - manufacturer_blank: Manufacturer can't be blank serial_blank: Serial can't be blank serial: Serial number can't be blank description_blank: Description can't be blank diff --git a/spec/features/assessment_forms/structure_assessment_trough_fields_spec.rb b/spec/features/assessment_forms/structure_assessment_trough_fields_spec.rb index 318fc7cac..a9ae7482f 100644 --- a/spec/features/assessment_forms/structure_assessment_trough_fields_spec.rb +++ b/spec/features/assessment_forms/structure_assessment_trough_fields_spec.rb @@ -12,11 +12,16 @@ describe "structure assessment form" do before { visit edit_inspection_path(inspection) } - it "shows trough fields in structure tab" do + scenario "shows optional trough fields in structure tab" do click_link I18n.t("inspections.tabs.structure") - expect(page).to have_content("Trough Depth") - expect(page).to have_content("Trough Adjacent Panel Width") + trough_depth = I18n.t("forms.structure.fields.trough_depth") + adjacent_width = I18n.t( + "forms.structure.fields.trough_adjacent_panel_width" + ) + + expect(find_field(trough_depth)[:required]).to be_nil + expect(find_field(adjacent_width)[:required]).to be_nil end it "saves trough field values" do diff --git a/spec/features/inspections/inspection_incomplete_fields_spec.rb b/spec/features/inspections/inspection_incomplete_fields_spec.rb index 557f60c08..7da040315 100644 --- a/spec/features/inspections/inspection_incomplete_fields_spec.rb +++ b/spec/features/inspections/inspection_incomplete_fields_spec.rb @@ -105,7 +105,7 @@ width: 8.0, inspection_fields: [:inspection_date], assessment_fields: { - user_height_assessment: [:users_at_1000mm] + user_height_assessment: [:containing_wall_height] } ) @@ -115,7 +115,32 @@ expect_incomplete_section("inspection") expect_incomplete_section("user_height") expect_incomplete_field("inspection", "inspection_date") - expect_incomplete_field("user_height", "users_at_1000mm") + expect_incomplete_field("user_height", "containing_wall_height") + end + + scenario "excludes fields configured as optional for completion" do + optional_inspection = create_incomplete_inspection( + assessment_fields: { + structure_assessment: %i[ + step_ramp_size + step_ramp_size_pass + straight_walls_pass + trough_adjacent_panel_width + trough_depth + ], + user_height_assessment: %i[ + users_at_1000mm + users_at_1200mm + users_at_1500mm + users_at_1800mm + ] + } + ) + + visit edit_inspection_path(optional_inspection) + + expect(page).not_to have_css("details.incomplete-fields-details") + expect(page).to have_button(I18n.t("inspections.buttons.mark_complete")) end scenario "does not show incomplete fields when truly complete" do diff --git a/spec/features/pdfs/pdf_integration_spec.rb b/spec/features/pdfs/pdf_integration_spec.rb index 7dd668486..da8816c58 100644 --- a/spec/features/pdfs/pdf_integration_spec.rb +++ b/spec/features/pdfs/pdf_integration_spec.rb @@ -15,6 +15,37 @@ sign_in user end + scenario "records PDF generation performance timings" do + performance_stages = [] + allow(Rails.logger).to receive(:info) do |message| + if message.is_a?(Hash) && message[:event] == "pdf.performance" + performance_stages << message[:stage] + end + end + + visit inspection_path(inspection, format: :pdf) + + expect(page.source).to start_with("%PDF") + expect(Rails.logger).to have_received(:info).with( + hash_including( + event: "pdf.performance", + stage: :document_build, + pdf_type: :inspection, + record_id: inspection.id + ) + ) + expect(Rails.logger).to have_received(:info).with( + hash_including( + event: "pdf.performance", + stage: :total, + pdf_type: :inspection, + record_id: inspection.id + ) + ) + expect(performance_stages.index(:access_tracking)) + .to be < performance_stages.index(:total) + end + scenario "generates comprehensive PDF with complete field coverage" do visit inspection_path(inspection, format: :pdf) diff --git a/spec/features/units/units_form_spec.rb b/spec/features/units/units_form_spec.rb index 8f912f26d..88603ad2f 100644 --- a/spec/features/units/units_form_spec.rb +++ b/spec/features/units/units_form_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" -RSpec.describe "Units Form", type: :feature do +RSpec.feature "Units Form", type: :feature do let(:user) { create(:user) } before do @@ -14,7 +14,7 @@ visit "/units/new" end - it "successfully creates a unit with valid data" do + scenario "successfully creates a unit with valid data" do if Rails.configuration.units.badges_enabled badge = create(:badge, badge_batch: create(:badge_batch)) fill_in_form :units, :id, badge.id @@ -41,13 +41,38 @@ expect(page).not_to have_content(I18n.t("events.messages.view_changes")) end - it "shows validation errors for missing required fields" do + scenario "creates a unit without optional manufacture details" do + if Rails.configuration.units.badges_enabled + badge = create(:badge, badge_batch: create(:badge_batch)) + fill_in_form :units, :id, badge.id + end + + fill_in_form :units, :name, "Unit without manufacture details" + fill_in_form :units, :serial, "OPTIONAL-001" + fill_in_form :units, :description, "Manufacture details are unknown" + + submit_form :units + + unit = Unit.find_by!(serial: "OPTIONAL-001") + expect(page).to have_current_path(unit_path(unit)) + expect(unit.manufacture_date).to be_nil + expect(unit.manufacturer).to be_blank + end + + scenario "does not mark manufacture details as required" do + manufacture_date = I18n.t("forms.units.fields.manufacture_date") + manufacturer = I18n.t("forms.units.fields.manufacturer") + + expect(find_field(manufacture_date)[:required]).to be_nil + expect(find_field(manufacturer)[:required]).to be_nil + end + + scenario "shows validation errors for missing required fields" do submit_form :units - expected_count = Rails.configuration.units.badges_enabled ? 5 : 4 + expected_count = Rails.configuration.units.badges_enabled ? 4 : 3 expect_form_errors :units, count: expected_count expect(page).to have_content(I18n.t("units.validations.name_blank")) - expect(page).to have_content(I18n.t("units.validations.manufacturer_blank")) expect(page).to have_content(I18n.t("units.validations.serial_blank")) expect(page).to have_content(I18n.t("units.validations.description_blank")) @@ -56,7 +81,7 @@ end end - it "validates serial uniqueness per user" do + scenario "validates serial uniqueness per user" do create(:unit, user: user, serial: "DUPLICATE-001") fill_in_form :units, :name, "Test Unit" @@ -77,13 +102,13 @@ visit edit_unit_path(unit) end - it "populates form with existing unit data" do + scenario "populates form with existing unit data" do expect(page).to have_field(I18n.t("forms.units.fields.name"), with: unit.name) expect(page).to have_field(I18n.t("forms.units.fields.manufacturer"), with: unit.manufacturer) expect(page).to have_button(I18n.t("forms.units.submit")) end - it "successfully updates unit with new data and creates audit log" do + scenario "successfully updates unit with new data and creates audit log" do fill_in_form :units, :name, "Updated Name" fill_in_form :units, :description, "Updated description" diff --git a/spec/models/assessment_schema_spec.rb b/spec/models/assessment_schema_spec.rb index e1f03605c..9334e4d91 100644 --- a/spec/models/assessment_schema_spec.rb +++ b/spec/models/assessment_schema_spec.rb @@ -120,6 +120,21 @@ expect(ic_schema.find_field(:city)).not_to be_required end + it "detects fields explicitly optional for completion" do + optional = AssessmentSchema::Field.new( + field: :optional, + partial: :number, + attributes: {required: false} + ) + unspecified = AssessmentSchema::Field.new( + field: :unspecified, + partial: :number + ) + + expect(optional).to be_optional_for_completion + expect(unspecified).not_to be_optional_for_completion + end + it "computes composite fields via ChobbleForms::FieldUtils" do field = schema.find_field(:num_low_anchors) expect(field.composite_fields) @@ -176,6 +191,28 @@ end end + describe "#completion_optional_fields" do + it "includes the persisted fields represented by optional form fields" do + optional = AssessmentSchema::Field.new( + field: :measurement, + partial: :number_pass_fail_comment, + attributes: {required: false} + ) + required = AssessmentSchema::Field.new( + field: :check, + partial: :pass_fail_comment + ) + fieldset = AssessmentSchema::Fieldset.new(:section, [optional, required]) + schema = described_class.new("custom", [fieldset]) + + expect(schema.completion_optional_fields).to contain_exactly( + :measurement, + :measurement_pass, + :measurement_comment + ) + end + end + describe "#exclude" do let(:schema) { AssessmentSchema.for(InspectorCompany) } diff --git a/spec/models/concerns/assessment_completion_spec.rb b/spec/models/concerns/assessment_completion_spec.rb index 1a92cc927..b39a42d2f 100644 --- a/spec/models/concerns/assessment_completion_spec.rb +++ b/spec/models/concerns/assessment_completion_spec.rb @@ -61,6 +61,35 @@ expect(assessment).to be_complete end end + + context "with fields configured as optional" do + let(:assessment) { inspection.structure_assessment } + + it "does not return their values or checks as incomplete" do + optional_fields = %i[ + step_ramp_size + step_ramp_size_pass + straight_walls_pass + trough_adjacent_panel_width + trough_depth + ] + + expect(assessment.incomplete_fields).not_to include(*optional_fields) + end + end + + context "with optional user count fields" do + it "does not return blank height counts as incomplete" do + optional_fields = %i[ + users_at_1000mm + users_at_1200mm + users_at_1500mm + users_at_1800mm + ] + + expect(assessment.incomplete_fields).not_to include(*optional_fields) + end + end end describe "#incomplete_fields_grouped" do diff --git a/spec/models/inspection_spec.rb b/spec/models/inspection_spec.rb index e1fd9c8bd..c5be50d48 100644 --- a/spec/models/inspection_spec.rb +++ b/spec/models/inspection_spec.rb @@ -540,7 +540,9 @@ describe "#invalidate_pdf_cache" do it "does not invalidate cache when only pdf_last_accessed_at changes" do + inspection expect(PdfCacheService).not_to receive(:invalidate_inspection_cache) + expect(PdfCacheService).not_to receive(:invalidate_unit_cache) inspection.update!(pdf_last_accessed_at: Time.current) end @@ -553,7 +555,9 @@ end it "invalidates cache when other attributes change" do + inspection expect(PdfCacheService).to receive(:invalidate_inspection_cache).with(inspection) + expect(PdfCacheService).to receive(:invalidate_unit_cache).with(inspection.unit) inspection.update!(risk_assessment: "Updated risk assessment") end diff --git a/spec/models/unit_spec.rb b/spec/models/unit_spec.rb index b595786f2..45c70db05 100644 --- a/spec/models/unit_spec.rb +++ b/spec/models/unit_spec.rb @@ -52,14 +52,20 @@ end describe "validations" do - it "validates presence of all required fields" do + it "validates presence of required fields" do unit = build(:unit, user: user, name: nil, serial: nil, description: nil, manufacturer: nil) expect(unit).not_to be_valid expect(unit.errors[:name]).to be_present expect(unit.errors[:serial]).to be_present expect(unit.errors[:description]).to be_present - expect(unit.errors[:manufacturer]).to be_present + expect(unit.errors[:manufacturer]).to be_empty + end + + it "allows manufacture details to be blank" do + unit = build(:unit, user:, manufacture_date: nil, manufacturer: nil) + + expect(unit).to be_valid end it "validates serial uniqueness within user" do @@ -205,7 +211,6 @@ unit = build(:unit, user: user, manufacturer: nil, serial: nil) expect(unit).not_to be_valid - expect(unit.errors[:manufacturer]).to be_present expect(unit.errors[:serial]).to be_present end diff --git a/spec/requests/inspections/inspections_spec.rb b/spec/requests/inspections/inspections_spec.rb index 957b70516..1b5666a89 100644 --- a/spec/requests/inspections/inspections_spec.rb +++ b/spec/requests/inspections/inspections_spec.rb @@ -180,6 +180,8 @@ it "serves PDF" do allow(PdfGeneratorService).to receive(:generate_inspection_report).and_return(double(render: "PDF")) + inspection + expect(PdfCacheService).not_to receive(:invalidate_unit_cache) get "/inspections/#{inspection.id}.pdf" expect(response).to have_http_status(:success) diff --git a/spec/services/pdf_generator_service/image_processor_spec.rb b/spec/services/pdf_generator_service/image_processor_spec.rb index cd88be52f..5b1bd98b8 100644 --- a/spec/services/pdf_generator_service/image_processor_spec.rb +++ b/spec/services/pdf_generator_service/image_processor_spec.rb @@ -55,6 +55,12 @@ it "respects column count parameter" do expect { described_class.add_unit_photo_footer(pdf, unit_with_photo, 4) }.not_to raise_error end + + it "returns the rendered photo height" do + height = described_class.add_unit_photo_footer(pdf, unit_with_photo) + + expect(height).to be_positive + end end context "with unit without photo" do @@ -86,6 +92,9 @@ describe "integration with real PDF generation" do it "generates complete PDF with QR code and photos" do pdf_content = nil + photo_blob = unit_with_photo.photo.blob + expect(photo_blob).to receive(:download).once.and_call_original + allow(Rails.logger).to receive(:info) expect { pdf_content = PdfGeneratorService.generate_inspection_report(inspection_with_photo).render @@ -93,6 +102,14 @@ expect(pdf_content).to be_a(String) expect(pdf_content[0..3]).to eq("%PDF") + expect(Rails.logger).to have_received(:info).with( + hash_including( + event: "pdf.performance", + stage: :image_download, + pdf_type: :inspection, + record_id: inspection_with_photo.id + ) + ) end it "generates complete PDF without photos" do diff --git a/spec/services/pdf_generator_service/photos_renderer_spec.rb b/spec/services/pdf_generator_service/photos_renderer_spec.rb index 6dd1c7470..f8c8eb8df 100644 --- a/spec/services/pdf_generator_service/photos_renderer_spec.rb +++ b/spec/services/pdf_generator_service/photos_renderer_spec.rb @@ -317,8 +317,8 @@ allow(described_class).to receive(:add_photo_label) end - it "downloads and processes the image" do - expect(photo_blob).to receive(:download) + it "delegates the image download to the image processor" do + expect(photo_blob).not_to receive(:download) expect(PdfGeneratorService::ImageProcessor).to receive(:process_image_with_orientation).with(photo_attached_one) described_class.render_photo(pdf, photo_attached_one, "Test Label", 300) end diff --git a/spec/services/pdf_performance_spec.rb b/spec/services/pdf_performance_spec.rb new file mode 100644 index 000000000..56b5d019f --- /dev/null +++ b/spec/services/pdf_performance_spec.rb @@ -0,0 +1,51 @@ +# typed: false +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe PdfPerformance do + describe ".measure" do + it "returns the measured block result and logs its duration" do + allow(Process).to receive(:clock_gettime).and_return(10.0, 10.125) + allow(Rails.logger).to receive(:info) + + result = described_class.measure( + :document_build, + pdf_type: :inspection, + record_id: "inspection-id", + cached: false + ) { :result } + + expect(result).to eq(:result) + expect(Rails.logger).to have_received(:info).with({ + event: "pdf.performance", + stage: :document_build, + pdf_type: :inspection, + record_id: "inspection-id", + duration_ms: 125.0, + cached: false + }) + end + + it "logs timing and reraises errors from the measured block" do + allow(Process).to receive(:clock_gettime).and_return(20.0, 20.01) + allow(Rails.logger).to receive(:info) + + expect { + described_class.measure( + :document_render, + pdf_type: :unit, + record_id: "unit-id" + ) { raise "render failed" } + }.to raise_error("render failed") + + expect(Rails.logger).to have_received(:info).with( + hash_including( + event: "pdf.performance", + stage: :document_render, + duration_ms: 10.0 + ) + ) + end + end +end From d586a87e38d1f7920928532126568db9fe108532 Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 15 Jul 2026 12:50:20 +0100 Subject: [PATCH 2/5] Fix full-suite order dependencies --- spec/features/details_links_spec.rb | 2 +- .../inspections/turbo_incomplete_fields_spec.rb | 4 ++-- spec/lib/form_yaml_database_schema_spec.rb | 10 +++++----- spec/support/test_translations.en.yml | 7 +++++++ spec/support/test_translations.rb | 15 ++++----------- 5 files changed, 19 insertions(+), 19 deletions(-) create mode 100644 spec/support/test_translations.en.yml diff --git a/spec/features/details_links_spec.rb b/spec/features/details_links_spec.rb index 04d9aa58f..cdb22c720 100644 --- a/spec/features/details_links_spec.rb +++ b/spec/features/details_links_spec.rb @@ -31,7 +31,7 @@ end scenario "multiple clicks on links work consistently" do - inspection.user_height_assessment.update_column(:users_at_1000mm, nil) + inspection.user_height_assessment.update_column(:containing_wall_height, nil) find("summary.incomplete-fields-summary").click diff --git a/spec/features/inspections/turbo_incomplete_fields_spec.rb b/spec/features/inspections/turbo_incomplete_fields_spec.rb index d98f7587b..cef151496 100644 --- a/spec/features/inspections/turbo_incomplete_fields_spec.rb +++ b/spec/features/inspections/turbo_incomplete_fields_spec.rb @@ -12,7 +12,7 @@ create(:inspection, :completed, user:, unit:).tap do |insp| insp.update_columns(complete_date: nil) # Make an assessment field incomplete to have something to test with - insp.user_height_assessment.update_column(:users_at_1000mm, nil) + insp.user_height_assessment.update_column(:containing_wall_height, nil) end end @@ -28,7 +28,7 @@ expect(summary.text).to match(/Show 1 incomplete field/) visit edit_inspection_path(inspection, tab: "user_height") - height_label = I18n.t("forms.user_height.fields.users_at_1000mm") + height_label = I18n.t("forms.user_height.fields.containing_wall_height") fill_in height_label, with: "2" submit_form :user_height diff --git a/spec/lib/form_yaml_database_schema_spec.rb b/spec/lib/form_yaml_database_schema_spec.rb index 42e3b1242..8bc4e5544 100644 --- a/spec/lib/form_yaml_database_schema_spec.rb +++ b/spec/lib/form_yaml_database_schema_spec.rb @@ -39,17 +39,17 @@ # Define allowed attributes for each partial type PARTIAL_ALLOWED_ATTRIBUTES = { - # Number fields can have step, min, max - number: %i[step min max], - number_pass_fail_comment: %i[step min max], + # Number fields can have presentation and required metadata + number: %i[max min required step], + number_pass_fail_comment: %i[max min required step], number_pass_fail_na_comment: %i[step min max], decimal_comment: %i[step min max], - integer_comment: %i[step min max add_not_applicable], + integer_comment: %i[add_not_applicable max min required step], # Most partials don't allow any attributes text_field: [], text_area: [], pass_fail: [], - pass_fail_comment: [], + pass_fail_comment: %i[required], pass_fail_na_comment: [], checkbox: [], yes_no_radio: [], diff --git a/spec/support/test_translations.en.yml b/spec/support/test_translations.en.yml new file mode 100644 index 000000000..6a33fbe53 --- /dev/null +++ b/spec/support/test_translations.en.yml @@ -0,0 +1,7 @@ +--- +en: + test: + access_denied: Access denied + admin_emails_pattern: "^admin\\d*@example\\.com$" + invalid_password: wrongpassword + password: password123 diff --git a/spec/support/test_translations.rb b/spec/support/test_translations.rb index fc5b68c54..4fa949ce8 100644 --- a/spec/support/test_translations.rb +++ b/spec/support/test_translations.rb @@ -1,13 +1,6 @@ # typed: false -# Test-specific translations that are only used in specs -# These are not part of the production application - -I18n.backend.store_translations(:en, { - test: { - password: "password123", - access_denied: "Access denied", - invalid_password: "wrongpassword", - admin_emails_pattern: "^admin\\d*@example\\.com$" - } -}) +# Test-specific translations must survive I18n.backend.reload! calls. +test_translations = Rails.root.join("spec/support/test_translations.en.yml") +I18n.load_path << test_translations.to_s +I18n.backend.load_translations(test_translations) From 34a0382f402c8ef806a64991f32c885ae281c977 Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 15 Jul 2026 13:15:41 +0100 Subject: [PATCH 3/5] Address PDF and unit review feedback --- app/models/unit.rb | 1 - app/services/pdf_generator_service.rb | 26 ++++++++++++++---- .../pdf_generator_service/header_generator.rb | 26 +++++++++++++----- .../pdf_generator_service/image_processor.rb | 22 +++++++-------- .../pdf_generator_service/table_builder.rb | 17 +++--------- ...ve_manufacturer_serial_index_from_units.rb | 8 ++++++ db/schema.rb | 3 +-- spec/factories/units.rb | 1 - spec/features/details_links_spec.rb | 1 + spec/models/unit_spec.rb | 11 +++++++- spec/seeds/seeds_spec.rb | 4 +++ .../table_builder_spec.rb | 27 ++++++++++++++----- spec/services/pdf_generator_service_spec.rb | 6 +++++ 13 files changed, 104 insertions(+), 49 deletions(-) create mode 100644 db/migrate/20260715120000_remove_manufacturer_serial_index_from_units.rb diff --git a/app/models/unit.rb b/app/models/unit.rb index 94da5c992..00e6b5e41 100644 --- a/app/models/unit.rb +++ b/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) diff --git a/app/services/pdf_generator_service.rb b/app/services/pdf_generator_service.rb index d0dec80de..deedc4091 100644 --- a/app/services/pdf_generator_service.rb +++ b/app/services/pdf_generator_service.rb @@ -114,7 +114,12 @@ def self.generate_unit_report(unit, debug_enabled: false, debug_queries: []) pdf_type: :unit, record_id: unit.id ) do - HeaderGenerator.generate_unit_pdf_header(pdf, unit, unbranded: unbranded) + HeaderGenerator.generate_unit_pdf_header( + pdf, + unit, + last_inspection:, + unbranded: + ) generate_unit_details_with_inspection(pdf, unit, last_inspection) end @@ -153,19 +158,30 @@ def self.generate_inspection_unit_details(pdf, inspection) return unless unit - unit_data = TableBuilder.build_unit_details_table_with_inspection(unit, inspection, :inspection) + unit_data = TableBuilder.build_unit_details_table_with_inspection( + unit, + inspection + ) TableBuilder.create_unit_details_table(pdf, I18n.t("pdf.inspection.equipment_details"), unit_data) # Hide the table entirely when no unit is associated end def self.generate_unit_details(pdf, unit) - unit_data = TableBuilder.build_unit_details_table(unit, :unit) + unit_data = TableBuilder.build_unit_details_table( + unit, + :unit, + unit.last_inspection + ) TableBuilder.create_unit_details_table(pdf, I18n.t("pdf.unit.details"), unit_data) end - def self.generate_unit_details_with_inspection(pdf, unit, _last_inspection) - unit_data = TableBuilder.build_unit_details_table(unit, :unit) + def self.generate_unit_details_with_inspection(pdf, unit, last_inspection) + unit_data = TableBuilder.build_unit_details_table( + unit, + :unit, + last_inspection + ) TableBuilder.create_unit_details_table(pdf, I18n.t("pdf.unit.details"), unit_data) end diff --git a/app/services/pdf_generator_service/header_generator.rb b/app/services/pdf_generator_service/header_generator.rb index d7e1d2a97..7eb1f420f 100644 --- a/app/services/pdf_generator_service/header_generator.rb +++ b/app/services/pdf_generator_service/header_generator.rb @@ -23,18 +23,29 @@ def self.create_inspection_header(pdf, inspection) pdf.move_down Configuration::STATUS_SPACING end - def self.generate_unit_pdf_header(pdf, unit, unbranded: false) - create_unit_header(pdf, unit, unbranded: unbranded) + def self.generate_unit_pdf_header(pdf, unit, last_inspection:, + unbranded: false) + create_unit_header( + pdf, + unit, + last_inspection:, + unbranded: + ) # Generate QR code in top left corner ImageProcessor.generate_qr_code_header(pdf, unit) end - def self.create_unit_header(pdf, unit, unbranded: false) + def self.create_unit_header(pdf, unit, last_inspection:, unbranded: false) user = unbranded ? nil : unit.user unit_id_text = build_unit_id_text(unit) render_header_with_logo(pdf, user) do |logo_width| - render_unit_text_section(pdf, unit, unit_id_text, logo_width) + render_unit_text_section( + pdf, + last_inspection, + unit_id_text, + logo_width + ) end pdf.move_down Configuration::STATUS_SPACING @@ -116,14 +127,15 @@ def render_inspection_text_section(pdf, inspection, report_id_text, end end - def render_unit_text_section(pdf, unit, unit_id_text, logo_width) + def render_unit_text_section(pdf, last_inspection, unit_id_text, + logo_width) header_text_box(pdf, logo_width) do pdf.text unit_id_text, size: Configuration::HEADER_TEXT_SIZE, style: :bold expiry_label = I18n.t("pdf.unit.fields.expiry_date") - expiry_value = if unit.last_inspection&.reinspection_date - Utilities.format_date(unit.last_inspection.reinspection_date) + expiry_value = if last_inspection&.reinspection_date + Utilities.format_date(last_inspection.reinspection_date) else I18n.t("pdf.unit.fields.na") end diff --git a/app/services/pdf_generator_service/image_processor.rb b/app/services/pdf_generator_service/image_processor.rb index c0132c732..59e77f8df 100644 --- a/app/services/pdf_generator_service/image_processor.rb +++ b/app/services/pdf_generator_service/image_processor.rb @@ -57,12 +57,7 @@ def self.process_image_with_orientation(attachment) context = performance_context(attachment) image = create_image(attachment, context) # Images are already orientation-corrected from upload processing - PdfPerformance.measure( - :image_transcode, - **context - ) do - image.write_to_buffer(".png") - end + transcode_to_png(image, context) end def self.calculate_footer_photo_dimensions(pdf, image, column_count = 3) @@ -95,12 +90,7 @@ def self.calculate_height_from_aspect(photo_width, original_width, def self.render_processed_image(pdf, image, x, y, width, height, attachment, context) # Images are already orientation-corrected from upload processing - processed_image = PdfPerformance.measure( - :image_transcode, - **context - ) do - image.write_to_buffer(".png") - end + processed_image = transcode_to_png(image, context) image_options = { at: [x, y], @@ -132,6 +122,12 @@ def self.create_image(attachment, context) end end + def self.transcode_to_png(image, context) + PdfPerformance.measure(:image_transcode, **context) do + image.write_to_buffer(".png") + end + end + def self.calculate_photo_y(pdf, photo_height) if pdf.page_number == 1 Configuration::FOOTER_HEIGHT + @@ -150,6 +146,6 @@ def self.performance_context(attachment) } end - private_class_method :performance_context + private_class_method :performance_context, :transcode_to_png end end diff --git a/app/services/pdf_generator_service/table_builder.rb b/app/services/pdf_generator_service/table_builder.rb index c7b74c1a5..989734f33 100644 --- a/app/services/pdf_generator_service/table_builder.rb +++ b/app/services/pdf_generator_service/table_builder.rb @@ -200,13 +200,11 @@ def self.apply_history_table_column_widths(table, pdf_width) table.column_widths = [date_width, result_width, inspector_width] end - def self.build_unit_details_table(unit, context) - # Get dimensions from last inspection if available - last_inspection = unit.last_inspection + def self.build_unit_details_table(unit, context, last_inspection) if context == :unit build_unit_details_table_for_unit_pdf(unit, last_inspection) else - build_unit_details_table_with_inspection(unit, last_inspection, context) + build_unit_details_table_with_inspection(unit, last_inspection) end end @@ -224,15 +222,8 @@ def self.build_unit_details_table_for_unit_pdf(unit, last_inspection) ] end - def self.build_unit_details_table_with_inspection(unit, last_inspection, context) - dimensions_text = build_dimensions_text(last_inspection) - - # Get inspector details from current inspection (for inspection PDF) or last inspection (for unit PDF) - inspection = if context == :inspection - last_inspection - else - unit.last_inspection - end + def self.build_unit_details_table_with_inspection(unit, inspection) + dimensions_text = build_dimensions_text(inspection) inspector_name = inspection&.user&.name rpii_number = inspection&.user&.rpii_inspector_number diff --git a/db/migrate/20260715120000_remove_manufacturer_serial_index_from_units.rb b/db/migrate/20260715120000_remove_manufacturer_serial_index_from_units.rb new file mode 100644 index 000000000..e2968a4e8 --- /dev/null +++ b/db/migrate/20260715120000_remove_manufacturer_serial_index_from_units.rb @@ -0,0 +1,8 @@ +# typed: false + +class RemoveManufacturerSerialIndexFromUnits < ActiveRecord::Migration[8.0] + def change + remove_index :units, [:manufacturer, :serial], + name: "index_units_on_manufacturer_and_serial" + end +end diff --git a/db/schema.rb b/db/schema.rb index 081df3b98..c8b21763d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_03_25_000003) do +ActiveRecord::Schema[8.1].define(version: 2026_07_15_120000) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -634,7 +634,6 @@ t.datetime "updated_at", null: false t.string "user_id", limit: 12, null: false t.index ["is_seed"], name: "index_units_on_is_seed" - t.index ["manufacturer", "serial"], name: "index_units_on_manufacturer_and_serial", unique: true t.index ["serial", "user_id"], name: "index_units_on_serial_and_user_id", unique: true t.index ["unit_type"], name: "index_units_on_unit_type" t.index ["user_id"], name: "index_units_on_user_id" diff --git a/spec/factories/units.rb b/spec/factories/units.rb index 8240e3e4c..1ef378782 100644 --- a/spec/factories/units.rb +++ b/spec/factories/units.rb @@ -20,7 +20,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) diff --git a/spec/features/details_links_spec.rb b/spec/features/details_links_spec.rb index cdb22c720..41b9a2d1c 100644 --- a/spec/features/details_links_spec.rb +++ b/spec/features/details_links_spec.rb @@ -32,6 +32,7 @@ scenario "multiple clicks on links work consistently" do inspection.user_height_assessment.update_column(:containing_wall_height, nil) + visit edit_inspection_path(inspection) find("summary.incomplete-fields-summary").click diff --git a/spec/models/unit_spec.rb b/spec/models/unit_spec.rb index 45c70db05..8b828f1cb 100644 --- a/spec/models/unit_spec.rb +++ b/spec/models/unit_spec.rb @@ -20,7 +20,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) @@ -83,6 +82,16 @@ unit2 = build(:unit, user: user2, manufacturer: "TestCorp", serial: "ABC123") expect(unit2).to be_valid end + + it "allows same serial and blank manufacturer for different users" do + user1 = create(:user) + user2 = create(:user) + create(:unit, user: user1, manufacturer: "", serial: "ABC123") + + unit2 = create(:unit, user: user2, manufacturer: "", serial: "ABC123") + + expect(unit2).to be_persisted + end end describe "custom ID generation" do diff --git a/spec/seeds/seeds_spec.rb b/spec/seeds/seeds_spec.rb index baab9e1bb..c1038dbf7 100644 --- a/spec/seeds/seeds_spec.rb +++ b/spec/seeds/seeds_spec.rb @@ -23,6 +23,10 @@ load Rails.root.join("db/seeds.rb") end + after(:all) do + DatabaseCleaner.clean_with(:truncation) + end + describe "Inspector Companies" do it "creates expected number of inspector companies" do expect(InspectorCompany.count).to eq(3) diff --git a/spec/services/pdf_generator_service/table_builder_spec.rb b/spec/services/pdf_generator_service/table_builder_spec.rb index 546d0b3f6..2e662597e 100644 --- a/spec/services/pdf_generator_service/table_builder_spec.rb +++ b/spec/services/pdf_generator_service/table_builder_spec.rb @@ -33,7 +33,11 @@ end it "formats unit data into 4x4 table structure" do - result = described_class.build_unit_details_table(unit, :inspection) + result = described_class.build_unit_details_table( + unit, + :inspection, + inspection + ) expect(result).to be_an(Array) expect(result.length).to eq(4) @@ -41,7 +45,11 @@ end it "includes key unit information" do - result = described_class.build_unit_details_table(unit, :inspection) + result = described_class.build_unit_details_table( + unit, + :inspection, + inspection + ) flattened = result.flatten expect(flattened).to include(unit.name) @@ -52,10 +60,13 @@ end it "includes dimensions when available" do - result = described_class.build_unit_details_table(unit, :inspection) + result = described_class.build_unit_details_table( + unit, + :inspection, + inspection + ) dimensions_text = result[2][1] # Size field - # Just verify dimensions field exists - depends on unit.last_inspection expect(dimensions_text).to be_a(String) end end @@ -64,7 +75,7 @@ let(:unit) { create(:unit) } it "formats unit data into 2-column table structure" do - result = described_class.build_unit_details_table(unit, :unit) + result = described_class.build_unit_details_table(unit, :unit, nil) expect(result).to be_an(Array) expect(result.length).to eq(5) @@ -78,7 +89,11 @@ end it "handles missing fields with empty values" do - result = described_class.build_unit_details_table(unit, "inspection") + result = described_class.build_unit_details_table( + unit, + "inspection", + nil + ) expect(result[0][1]).to eq("") # empty name (truncate_text converts nil to "") expect(result[1][1]).to be_nil # nil description diff --git a/spec/services/pdf_generator_service_spec.rb b/spec/services/pdf_generator_service_spec.rb index 99dcfb01e..4ec9d97e4 100644 --- a/spec/services/pdf_generator_service_spec.rb +++ b/spec/services/pdf_generator_service_spec.rb @@ -158,6 +158,12 @@ "shared.pass_pdf", "shared.fail_pdf") end + + it "reuses the inspection loaded for the history" do + expect(unit).not_to receive(:last_inspection) + + PdfGeneratorService.generate_unit_report(unit).render + end end context "with missing manufacturer" do From 51eeb4f1f76e11cf08e04e732a235fcc2e3a3206 Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 15 Jul 2026 13:34:46 +0100 Subject: [PATCH 4/5] Align unit PDF inspection ordering --- app/services/pdf_generator_service.rb | 2 +- spec/services/pdf_generator_service_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/services/pdf_generator_service.rb b/app/services/pdf_generator_service.rb index deedc4091..0cc51e165 100644 --- a/app/services/pdf_generator_service.rb +++ b/app/services/pdf_generator_service.rb @@ -100,7 +100,7 @@ def self.generate_unit_report(unit, debug_enabled: false, debug_queries: []) unit.inspections .includes(:user, inspector_company: {logo_attachment: :blob}) .complete - .order(inspection_date: :desc) + .order(complete_date: :desc) .load end diff --git a/spec/services/pdf_generator_service_spec.rb b/spec/services/pdf_generator_service_spec.rb index 4ec9d97e4..46f2fda08 100644 --- a/spec/services/pdf_generator_service_spec.rb +++ b/spec/services/pdf_generator_service_spec.rb @@ -164,6 +164,26 @@ PdfGeneratorService.generate_unit_report(unit).render end + + it "uses the most recently completed inspection for unit details" do + passed_inspection.update_columns( + complete_date: 1.day.ago, + inspection_date: 2.days.ago + ) + failed_inspection.update_columns( + complete_date: 2.days.ago, + inspection_date: 1.day.ago + ) + header_generator = PdfGeneratorService::HeaderGenerator + expect(header_generator).to receive(:generate_unit_pdf_header).with( + anything, + unit, + last_inspection: passed_inspection, + unbranded: anything + ).and_call_original + + PdfGeneratorService.generate_unit_report(unit).render + end end context "with missing manufacturer" do From c38400fd4b8f98e1db54523d2a4ed86a91227e6b Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 15 Jul 2026 13:47:53 +0100 Subject: [PATCH 5/5] Verify unit PDF inspection reuse --- spec/services/pdf_generator_service_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/services/pdf_generator_service_spec.rb b/spec/services/pdf_generator_service_spec.rb index 46f2fda08..0f55488e8 100644 --- a/spec/services/pdf_generator_service_spec.rb +++ b/spec/services/pdf_generator_service_spec.rb @@ -181,6 +181,13 @@ last_inspection: passed_inspection, unbranded: anything ).and_call_original + expect(described_class).to receive( + :generate_unit_details_with_inspection + ).with( + anything, + unit, + passed_inspection + ).and_call_original PdfGeneratorService.generate_unit_report(unit).render end