From 405a9c513e2693084d1ac9e4c807c242e2d30509 Mon Sep 17 00:00:00 2001 From: mirotriad Date: Mon, 6 Jan 2025 14:14:36 +0000 Subject: [PATCH] perf/PSD-3232 - Improve performance of Show and Update methods in ProductController and add tests --- .../notifications/product_controller.rb | 215 ++++++++++++++---- .../notifications/product_controller_spec.rb | 96 +++++++- cosmetics-web/spec/rails_helper.rb | 2 +- .../spec/support/database_query_counter.rb | 74 ++++++ 4 files changed, 340 insertions(+), 47 deletions(-) create mode 100644 cosmetics-web/spec/support/database_query_counter.rb diff --git a/cosmetics-web/app/controllers/responsible_persons/notifications/product_controller.rb b/cosmetics-web/app/controllers/responsible_persons/notifications/product_controller.rb index 688add5eb5..261cabd286 100644 --- a/cosmetics-web/app/controllers/responsible_persons/notifications/product_controller.rb +++ b/cosmetics-web/app/controllers/responsible_persons/notifications/product_controller.rb @@ -18,7 +18,8 @@ class ResponsiblePersons::Notifications::ProductController < SubmitApplicationCo add_product_image: :single_or_multi_component, }.freeze - before_action :set_notification + before_action :set_notification_and_authorize + before_action :check_notification_submitted before_action :contains_nanomaterials_form, if: -> { step == :contains_nanomaterials } before_action :single_or_multi_component_form, if: -> { step == :single_or_multi_component } @@ -27,7 +28,7 @@ def show when :completed @notification.set_state_on_product_wizard_completed! @continue_path = continue_path - render template: "responsible_persons/notifications/task_completed", locals: { continue_path: continue_path } + render template: "responsible_persons/notifications/task_completed", locals: { continue_path: @continue_path } when :add_product_image @clone_image_job = NotificationCloner::JobTracker.new(@notification.id) if @notification.cloned? render_wizard @@ -48,11 +49,7 @@ def update @clone_image_job = NotificationCloner::JobTracker.new(@notification.id) if @notification.cloned? update_add_product_image_step else - if @notification.update_with_context(notification_params, step) - render_next_step @notification - else - rerender_current_step - end + update_column_or_fallback end end @@ -62,57 +59,107 @@ def new private + def set_notification_and_authorize + base_query = Notification + .select(selected_notification_columns + responsible_person_columns) + .joins(:responsible_person) + + base_query = case step + when :contains_nanomaterials, :completed + base_query.includes(:nano_materials, :components) + when :single_or_multi_component + base_query.includes(:components) + else + base_query + end + + @notification = base_query.find_by!(reference_number: params[:notification_reference_number]) + + @responsible_person = @notification.responsible_person + + session_key = "authorized_for_notification_#{@notification.id}" + unless session[session_key] + authorized = ResponsiblePersonUser.exists?( + user_id: current_user.id, + responsible_person_id: @notification.responsible_person_id, + ) + raise Pundit::NotAuthorizedError unless authorized + + session[session_key] = true + end + end + + def check_notification_submitted + return unless @notification.state == "notification_complete" && @notification.notification_complete_at.present? + + redirect_to responsible_person_notification_path(@notification.responsible_person, @notification) + end + def continue_path - if @notification.nano_materials.present? - new_responsible_person_notification_nanomaterial_build_path(@notification.responsible_person, @notification, @notification.nano_materials.first) - elsif @notification.multi_component? - new_responsible_person_notification_product_kit_path(@notification.responsible_person, @notification) + @continue_path ||= if @notification.nano_materials.any? + new_responsible_person_notification_nanomaterial_build_path(@responsible_person, @notification, @notification.nano_materials.first) + elsif @notification.multi_component? + new_responsible_person_notification_product_kit_path(@responsible_person, @notification) + else + component = @notification.components.first || @notification.components.create! + new_responsible_person_notification_component_build_path(@responsible_person, @notification, component) + end + end + + def update_with_context_and_render + if @notification.update_with_context(notification_params, step) + render_next_step @notification else - component = @notification.components.first_or_create! - new_responsible_person_notification_component_build_path(@notification.responsible_person, @notification, component) + rerender_current_step end end def update_add_internal_reference case params.dig(:notification, :add_internal_reference) when "yes" - model.save_routing_answer(step, "yes") - if @notification.update_with_context(notification_params, step) - render_next_step @notification - else - rerender_current_step + @notification.transaction do + model.save_routing_answer(step, "yes") + if @notification.update_with_context(notification_params, step) + render_next_step @notification + else + rerender_current_step + end end when "no" - model.save_routing_answer(step, "no") - @notification.industry_reference = nil - render_next_step @notification + @notification.transaction do + model.save_routing_answer(step, "no") + @notification.update_column(:industry_reference, nil) + render_next_step @notification + end else @notification.errors.add :add_internal_reference, "Select yes to add an internal reference" rerender_current_step end end - # Run this step only when notification doesn't already have nanomaterials def update_contains_nanomaterials - return render_next_step @notification if @notification.nano_materials.any? + return render_next_step @notification if @notification.nano_materials.loaded? ? @notification.nano_materials.any? : @notification.nano_materials.exists? form = contains_nanomaterials_form return rerender_current_step unless form.valid? - model.save_routing_answer(step, form.contains_nanomaterials) - @notification.make_ready_for_nanomaterials!(form.nanomaterials_count.to_i) - render_next_step @notification + @notification.transaction do + model.save_routing_answer(step, form.contains_nanomaterials) + @notification.make_ready_for_nanomaterials!(form.nanomaterials_count.to_i) + render_next_step @notification + end end - # Run this step only when notifications does not have multiple components def update_single_or_multi_component_step return render_next_step @notification if @notification.multi_component? form = single_or_multi_component_form return rerender_current_step unless form.valid? - @notification.make_single_ready_for_components!(form.components_count.to_i) - render_next_step @notification + @notification.transaction do + @notification.make_single_ready_for_components!(form.components_count.to_i) + render_next_step @notification + end end def update_add_product_image_step @@ -120,34 +167,37 @@ def update_add_product_image_step params[:image_upload].each { |img| @notification.add_image(img) } return rerender_current_step if @notification.errors.present? - @notification.save - if params[:back_to_edit] == "true" - redirect_to edit_responsible_person_notification_path(@notification.responsible_person, @notification) - elsif params[:after_save] == "upload_another" - rerender_current_step - else - render_next_step @notification - end + @notification.save! + handle_existing_image_uploads elsif @notification.image_uploads.present? - if params[:back_to_edit] == "true" - redirect_to edit_responsible_person_notification_path(@notification.responsible_person, @notification) - elsif params[:after_save] == "upload_another" - rerender_current_step - else - render_next_step @notification - end + handle_existing_image_uploads else @notification.errors.add :image_uploads, "Select an image" rerender_current_step end end + def handle_existing_image_uploads + if params[:back_to_edit] == "true" + redirect_to edit_responsible_person_notification_path(@notification.responsible_person, @notification) + elsif params[:after_save] == "upload_another" + rerender_current_step + else + render_next_step @notification + end + end + def notification_params params.fetch(:notification, {}) .permit( :product_name, :industry_reference, :under_three_years, + :still_on_the_market, + :shades, + :import_country, + :cpnp_notification_date, + :was_notified_before_eu_exit, image_uploads_attributes: [file: []], ) end @@ -191,4 +241,81 @@ def single_or_multi_component_values def model @notification end + + def rerender_current_step + render step + end + + def selected_notification_columns + %i[ + id + reference_number + product_name + state + notification_complete_at + responsible_person_id + industry_reference + under_three_years + components_are_mixed + previous_state + routing_questions_answers + cpnp_reference + source_notification_id + archive_reason + import_country + shades + cpnp_notification_date + was_notified_before_eu_exit + still_on_the_market + ph_min_value + ph_max_value + csv_cache + deleted_at + ] + end + + def responsible_person_columns + [ + Arel.sql("responsible_persons.id as responsible_person_id"), + Arel.sql("responsible_persons.name as responsible_person_name"), + ] + end + + def update_column_or_fallback + direct_update_columns = { + product_name: true, + under_three_years: true, + industry_reference: ->(_params) { step != :add_internal_reference }, + still_on_the_market: true, + shades: true, + import_country: true, + cpnp_notification_date: true, + was_notified_before_eu_exit: true, + } + + direct_update_columns.each do |column, condition| + next unless notification_params.key?(column) + next unless condition.is_a?(Proc) ? condition.call(notification_params) : condition + + return perform_direct_column_update(column) + end + + @notification.transaction do + if @notification.update_with_context(notification_params, step) + render_next_step @notification + else + rerender_current_step + end + end + end + + def perform_direct_column_update(column) + column_updated = @notification.update_column(column, notification_params[column]) + if column_updated + render_next_step(@notification) + else + @notification.errors.add(column, "could not be updated") + rerender_current_step + end + end end diff --git a/cosmetics-web/spec/controllers/responsible_persons/notifications/product_controller_spec.rb b/cosmetics-web/spec/controllers/responsible_persons/notifications/product_controller_spec.rb index 69ad3ea918..0ff6fa5f80 100644 --- a/cosmetics-web/spec/controllers/responsible_persons/notifications/product_controller_spec.rb +++ b/cosmetics-web/spec/controllers/responsible_persons/notifications/product_controller_spec.rb @@ -1,10 +1,12 @@ require "rails_helper" RSpec.describe ResponsiblePersons::Notifications::ProductController, :with_stubbed_antivirus, type: :controller do + let(:user) { create(:submit_user) } let(:responsible_person) { create(:responsible_person, :with_a_contact_person) } let(:notification) { create(:notification, responsible_person:) } let(:image_file) { fixture_file_upload("testImage.png", "image/png") } let(:text_file) { fixture_file_upload("testText.txt", "application/text") } + let(:request_env) { { "warden" => warden } } let(:params) do { @@ -13,12 +15,48 @@ } end + let(:warden) do + instance_double(Warden::Proxy).tap do |warden| + allow(warden).to receive_messages(authenticate!: user, authenticate?: true, authenticated?: true, user: user, session: {}) + end + end + before do - sign_in_as_member_of_responsible_person(responsible_person) + request.env["warden"] = warden + allow(controller).to receive_messages( + warden: warden, + submit_domain?: true, + user_signed_in?: true, + current_user: user, + ) + allow(user).to receive(:account_security_completed?).and_return(true) + + responsible_person.add_user(user) unless user.responsible_persons.include?(responsible_person) + session[:current_responsible_person_id] = responsible_person.id end after do - sign_out(:submit_user) + request.env["warden"] = nil + end + + shared_context "when counting database queries" do + def count_relevant_queries(&block) + queries = [] + callback = lambda { |*, payload| + unless payload[:name] == "SCHEMA" || payload[:sql].match?(/\A\s*BEGIN|\A\s*COMMIT|\A\s*ROLLBACK/) + queries << payload[:sql] + end + } + + ActiveSupport::Notifications.subscribed(callback, "sql.active_record", &block) + + queries.reject do |sql| + sql.include?("users") || + sql.include?("responsible_person_users") || + sql.include?("contact_persons") || + (sql.include?("responsible_persons") && !sql.include?("notifications")) + end + end end describe "GET #new" do @@ -63,6 +101,33 @@ context "when on the completed step" do let(:params_with_completed) { params.merge(id: :completed) } + describe "performance" do + include_context "when counting database queries" + + def perform_show_request + get(:show, params: params_with_completed) + end + + let!(:queries) do + perform_show_request + ActiveRecord::Base.connection.clear_query_cache + count_relevant_queries { perform_show_request } + end + + it "executes limited number of queries" do + expect(queries.length).to be_between(1, 3), + "Expected 1-3 queries but got #{queries.length}:\n#{queries.join("\n")}" + end + + it "includes notification query" do + expect(queries).to include(a_string_matching(/SELECT.*FROM "notifications"/)) + end + + it "includes component or nanomaterial query" do + expect(queries).to include(a_string_matching(/SELECT.*FROM "(nano_materials|components)"/)) + end + end + context "when notification has nanomaterials" do let(:nano_material) { create(:nano_material, notification: notification) } @@ -147,6 +212,33 @@ end describe "POST #update" do + describe "performance" do + include_context "when counting database queries" + + def perform_update_request + post(:update, params: params.merge( + id: :add_product_name, + notification: { product_name: "Super Shampoo" }, + )) + end + + let!(:queries) do + notification.update_column(:product_name, "Old Name") + perform_update_request + ActiveRecord::Base.connection.clear_query_cache + count_relevant_queries { perform_update_request } + end + + it "executes limited number of queries" do + expect(queries.length).to be_between(1, 3), + "Expected 1-3 queries but got #{queries.length}:\n#{queries.join("\n")}" + end + + it "includes notification update query" do + expect(queries).to include(a_string_matching(/UPDATE.*notifications/i)) + end + end + it "assigns the correct notification" do post(:update, params: params.merge(id: :add_product_name, notification: { product_name: "Super Shampoo" })) expect(assigns(:notification)).to eq(notification) diff --git a/cosmetics-web/spec/rails_helper.rb b/cosmetics-web/spec/rails_helper.rb index d5a3284181..c27de59de8 100644 --- a/cosmetics-web/spec/rails_helper.rb +++ b/cosmetics-web/spec/rails_helper.rb @@ -8,7 +8,6 @@ ENV["RAILS_ENV"] ||= "test" require File.expand_path("../config/environment", __dir__) -# Prevent database truncation if the environment is production abort("The Rails environment is running in production mode!") if Rails.env.production? # Add additional requires below this line. Rails is not loaded until this point! require "rspec/rails" @@ -81,6 +80,7 @@ config.include Matchers config.include ResponsiblePersonHelpers config.include ActionDispatch::TestProcess::FixtureFile + config.include DatabaseQueryCounter # Reset search indexes before each test if using a search service like Elasticsearch config.before do diff --git a/cosmetics-web/spec/support/database_query_counter.rb b/cosmetics-web/spec/support/database_query_counter.rb new file mode 100644 index 0000000000..8aa5dd703e --- /dev/null +++ b/cosmetics-web/spec/support/database_query_counter.rb @@ -0,0 +1,74 @@ +module DatabaseQueryCounter + def make_database_queries(count: nil, matching: nil, includes: nil, exact: nil) + QueryCounter.new(count: count, matching: matching, includes: includes, exact: exact) + end + + class QueryCounter + def initialize(count: nil, matching: nil, includes: nil, exact: nil) + @count = count + @matching = matching + @includes = includes + @exact = exact + @queries = [] + end + + def matches?(event_proc) + @queries = [] + subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload| + @queries << payload[:sql] unless payload[:name] == "SCHEMA" || payload[:sql].match?(/\A\s*BEGIN|\A\s*COMMIT|\A\s*ROLLBACK/) + end + + event_proc.call + ActiveSupport::Notifications.unsubscribe(subscriber) + + return matches_count?(@queries) if @count + return matches_exact?(@queries) if @exact + return matches_includes?(@queries) if @includes + return matches_matching?(@queries) if @matching + + true + end + + def failure_message + "expected database queries to match #{expectation}, but got #{@queries.length} queries:\n#{@queries.join("\n")}" + end + + def supports_block_expectations? + true + end + + private + + def matches_count?(queries) + case @count + when Range + @count.include?(queries.length) + when Integer + queries.length == @count + else + queries.length == @count + end + end + + def matches_exact?(queries) + queries == Array(@exact) + end + + def matches_includes?(queries) + Array(@includes).all? { |included| queries.any? { |q| q.include?(included) } } + end + + def matches_matching?(queries) + Array(@matching).all? { |pattern| queries.any? { |q| q.match?(pattern) } } + end + + def expectation + return "count #{@count}" if @count + return "exact #{@exact}" if @exact + return "includes #{@includes}" if @includes + return "matching #{@matching}" if @matching + + "unknown expectation" + end + end +end