diff --git a/app/controllers/concerns/member_concerns.rb b/app/controllers/concerns/member_concerns.rb index 1f4abffb6..8a8356f58 100644 --- a/app/controllers/concerns/member_concerns.rb +++ b/app/controllers/concerns/member_concerns.rb @@ -30,11 +30,11 @@ def set_member @member = current_user end - def how_you_found_us_selections_valid? - how_found_present = member_params[:how_you_found_us].present? - other_reason_present = member_params[:how_you_found_us_other_reason].present? - return false if member_params[:how_you_found_us] == 'other' && !other_reason_present - return true if member_params[:how_you_found_us] == 'other' && other_reason_present + def how_you_found_us_selections_valid?(attrs) + how_found_present = attrs[:how_you_found_us].present? + other_reason_present = attrs[:how_you_found_us_other_reason].present? + return false if attrs[:how_you_found_us] == 'other' && !other_reason_present + return true if attrs[:how_you_found_us] == 'other' && other_reason_present how_found_present != other_reason_present end diff --git a/app/controllers/member/details_controller.rb b/app/controllers/member/details_controller.rb index 848a01e10..9652a7f5d 100644 --- a/app/controllers/member/details_controller.rb +++ b/app/controllers/member/details_controller.rb @@ -14,22 +14,17 @@ def edit def update attrs = member_params - attrs[:how_you_found_us_other_reason] = nil if attrs[:how_you_found_us] != 'other' - unless how_you_found_us_selections_valid? + unless how_you_found_us_selections_valid?(attrs) @member.errors.add(:how_you_found_us, 'You must select one option') return render :edit end - attrs[:how_you_found_us] = params[:member][:how_you_found_us] if params[:member][:how_you_found_us].present? - if params[:member][:how_you_found_us_other_reason].present? && attrs[:how_you_found_us] == 'other' - attrs[:how_you_found_us_other_reason] = - params[:member][:how_you_found_us_other_reason] - end + attrs[:how_you_found_us_other_reason] = nil if attrs[:how_you_found_us] != 'other' return render :edit unless @member.update(attrs) - member_params[:newsletter] ? subscribe_to_newsletter(@member) : unsubscribe_from_newsletter(@member) + attrs[:newsletter] ? subscribe_to_newsletter(@member) : unsubscribe_from_newsletter(@member) redirect_to step2_member_path end end diff --git a/spec/controllers/member/details_controller_spec.rb b/spec/controllers/member/details_controller_spec.rb index 9c98c487b..128dbc6fd 100644 --- a/spec/controllers/member/details_controller_spec.rb +++ b/spec/controllers/member/details_controller_spec.rb @@ -97,6 +97,32 @@ expect(response.body).to include('You must select one option') end + + it 'validates efficiently by calling member_params only once' do + # This test verifies the fix for the performance issue where member_params + # was called multiple times (previously 6 times!): + # - Old: Line 16 called member_params, line 19 validation called it again, line 32 called it again + # - New: Line 16 calls member_params once, passes result to validation + + # Mock to track how many times member_params is called + call_count = 0 + allow(controller).to receive(:member_params).and_wrap_original do |method| + call_count += 1 + method.call + end + + patch :update, params: { + id: member.id, + member: { + how_you_found_us: 'social_media', + how_you_found_us_other_reason: '', # Empty is OK + newsletter: 'true' + } + } + + # After fix: member_params is called only once + expect(call_count).to eq(1) + end end end end