diff --git a/app/queries/get_activity_log_events.rb b/app/queries/get_activity_log_events.rb index 324ad07bdf6..173ab0cc1c7 100644 --- a/app/queries/get_activity_log_events.rb +++ b/app/queries/get_activity_log_events.rb @@ -15,27 +15,14 @@ class GetActivityLogEvents ], }.freeze - INCLUDE_APPLICATION_FORM_CHANGES_TO = [ - 'date_of_birth', - 'first_name', - 'last_name', - - # Contact Information - 'last_name', - 'phone_number', - 'address_line1', - 'address_line2', - 'address_line3', - 'address_line4', - 'country', - 'postcode', - - # Interview Preferences + INCLUDE_APPLICATION_FORM_CHANGES_TO = ApplicationForm::ColumnSectionMapping.by_section( + 'personal_information', + 'contact_information', 'interview_preferences', - - # Disability 'disability_disclosure', - ].freeze + ) + + DATABASE_CHANGE_KEYS = INCLUDE_APPLICATION_FORM_CHANGES_TO.map { |e| "'#{e}'" }.join(',') INCLUDE_APPLICATION_CHOICE_CHANGES_TO = %w[ reject_by_default_feedback_sent_at @@ -64,10 +51,20 @@ def self.call(application_choices:, since: nil) AND auditable_id = ac.application_form_id AND action = 'update' AND ( #{application_form_audits_filter_sql} ) + AND EXISTS ( + SELECT 1 + WHERE ARRAY[#{DATABASE_CHANGE_KEYS}] @> ( + SELECT ARRAY(SELECT jsonb_object_keys(a.audited_changes) + FROM audits a + WHERE a.id = audits.id + ) + ) + ) ) COMBINE_AUDITS_WITH_APPLICATION_CHOICES_SCOPE_AND_FILTER - Audited::Audit.includes(INCLUDES) + Audited::Audit.select('audits.id audit_id, audits.*, ac.id application_choice_id') + .includes(INCLUDES) .joins(application_choices_join_sql) .where('audits.created_at >= ?', since) .order('audits.created_at DESC') diff --git a/spec/queries/get_activity_log_events_spec.rb b/spec/queries/get_activity_log_events_spec.rb index a159fa763a5..b8bb2c06c17 100644 --- a/spec/queries/get_activity_log_events_spec.rb +++ b/spec/queries/get_activity_log_events_spec.rb @@ -21,6 +21,10 @@ def create_audit_for_application_choice(application_choice) create(:application_choice_audit, :with_offer, user: provider_user, application_choice:) end + def create_audit_for_application_form(application_choice) + create(:application_form_audit, user: provider_user, application_choice:, changes: { 'first_name' => %w[A B] }) + end + describe '#call' do it 'returns an empty array if no audits are found' do expect(service_call).to eq([]) @@ -32,7 +36,7 @@ def create_audit_for_application_choice(application_choice) result = service_call - expect(result.count).to eq(1) + expect(result.size).to eq(1) %i[created_at auditable].each do |attr| expect(result.first).to respond_to(attr) @@ -61,13 +65,13 @@ def create_audit_for_application_choice(application_choice) it 'defaults the query date range to the earliest scoped application form creation' do query_instance = instance_double(ActiveRecord::QueryMethods) - allow(Audited::Audit).to receive(:includes).and_return(query_instance) - %i[joins where order].each { |meth| allow(query_instance).to receive(meth).and_return(query_instance) } + allow(Audited::Audit).to receive(:select).and_return(query_instance) + %i[includes joins where order].each { |meth| allow(query_instance).to receive(meth).and_return(query_instance) } choice = create_application_choice_for_course course_provider_a another_choice = create_application_choice_for_course course_provider_a create_audit_for_application_choice choice - create_audit_for_application_choice another_choice + create_audit_for_application_form another_choice another_choice.application_form.update(created_at: 1.day.ago) @@ -85,10 +89,83 @@ def create_audit_for_application_choice(application_choice) expect(choice.audits.count).to eq(1) expect(choice.audits.first.action).to eq('create') - expect(result.count).to eq(0) + expect(result.size).to eq(0) + end + + it 'excludes events when not all keys are editable' do + choice = create_application_choice_for_course course_provider_a + audit = create( + :application_form_audit, + application_choice: choice, + changes: { + 'interview_preferences' => %w[This That], + 'subject_knowledge_completed' => [true, false], + }, + ) + + result = service_call + + expect(result).not_to include(audit) end - it 'includes events with a status change' do + it 'excludes events when ApplicationForm untracked attrs are changed' do + choice = create_application_choice_for_course course_provider_a + audit = create( + :application_form_audit, + application_choice: choice, + changes: { + 'date_of_birth' => %w[Hello Hi], + 'first_name' => %w[Hello Hi], + 'last_name' => %w[Hello Hi], + 'phone_number' => %w[Hello Hi], + 'address_line1' => %w[Hello Hi], + 'address_line2' => %w[Hello Hi], + 'address_line3' => %w[Hello Hi], + 'address_line4' => %w[Hello Hi], + 'country' => %w[Hello Hi], + 'postcode' => %w[Hello Hi], + 'region_code' => %w[Hello Hi], + 'interview_preferences' => %w[Hello Hi], + 'disability_disclosure' => %w[Hello Hi], + + # Untracked attribute + 'maths_gcse_completed' => %w[Hello Hi], + }, + ) + + result = service_call + + expect(result).not_to include(audit) + end + + it 'includes events when ApplicationForm attrs are changed' do + choice = create_application_choice_for_course course_provider_a + audit = create( + :application_form_audit, + application_choice: choice, + changes: { + 'date_of_birth' => %w[Hello Hi], + 'first_name' => %w[Hello Hi], + 'last_name' => %w[Hello Hi], + 'phone_number' => %w[Hello Hi], + 'address_line1' => %w[Hello Hi], + 'address_line2' => %w[Hello Hi], + 'address_line3' => %w[Hello Hi], + 'address_line4' => %w[Hello Hi], + 'country' => %w[Hello Hi], + 'postcode' => %w[Hello Hi], + 'region_code' => %w[Hello Hi], + 'interview_preferences' => %w[Hello Hi], + 'disability_disclosure' => %w[Hello Hi], + }, + ) + + result = service_call + + expect(result).to include(audit) + end + + it 'includes application_choice events with a status change' do choice = create_application_choice_for_course course_provider_a excluded = create( @@ -97,33 +174,16 @@ def create_audit_for_application_choice(application_choice) changes: { 'reject_by_default_at' => [nil, 40.days.from_now.iso8601] }, ) - included = [ - create( - :application_choice_audit, - application_choice: choice, - changes: { 'status' => %w[awaiting_provider_decision offer] }, - ), - create( - :application_form_audit, - application_choice: choice, - changes: { 'date_of_birth' => %w[01-01-2000 02-02-2002] }, - ), - create( - :application_form_audit, - application_choice: choice, - changes: { 'disability_disclosure' => %w[Hello Hi] }, - ), - create( - :application_form_audit, - application_choice: choice, - changes: { 'interview_preferences' => %w[This That] }, - ), - ] + included = create( + :application_choice_audit, + application_choice: choice, + changes: { 'status' => %w[awaiting_provider_decision offer] }, + ) result = service_call expect(result).not_to include(excluded) - expect(result).to match_array(included) + expect(result).to include(included) end it 'includes events with a reject_by_default_feedback_sent_at change' do diff --git a/spec/system/provider_interface/see_activity_log_spec.rb b/spec/system/provider_interface/see_activity_log_spec.rb index 4423edaa2e0..a2fa822de77 100644 --- a/spec/system/provider_interface/see_activity_log_spec.rb +++ b/spec/system/provider_interface/see_activity_log_spec.rb @@ -26,15 +26,17 @@ def and_i_have_a_manage_account end def and_my_organisation_has_applications - course1 = create(:course, provider: @provider1) - course2 = create(:course, provider: @provider2, accredited_provider: @provider1) - course3 = create(:course, provider: create(:provider), accredited_provider: @provider1) - course4 = create(:course, provider: create(:provider)) + course1 = create(:course, provider: @provider1, name: 'Course 1') + course2 = create(:course, provider: @provider2, accredited_provider: @provider1, name: 'Course 2') + course3 = create(:course, provider: create(:provider), accredited_provider: @provider1, name: 'Course 3') + course4 = create(:course, provider: create(:provider), name: 'Course 4') + course5 = create(:course, provider: @provider1, name: 'Course 5') course_option1 = create(:course_option, course: course1) course_option2 = create(:course_option, course: course2) course_option3 = create(:course_option, course: course3) course_option4 = create(:course_option, course: course4) + course_option5 = create(:course_option, course: course5) @choice1 = create(:application_choice, :awaiting_provider_decision, status: 'awaiting_provider_decision', course_option: course_option1) create(:application_choice_audit, :awaiting_provider_decision, application_choice: @choice1) @@ -47,6 +49,9 @@ def and_my_organisation_has_applications @choice4 = create(:application_choice, :offered, course_option: course_option4) create(:application_choice_audit, :with_offer, application_choice: @choice4) + + @choice5 = create(:application_choice, :offered, course_option: course_option5) + create(:application_form_audit, application_choice: @choice5, changes: { 'date_of_birth' => %w[01-01-2000 01-02-2002] }) end def and_the_provider_activity_log_feature_flag_is_on @@ -63,9 +68,10 @@ def when_i_click_on_the_activity_log_tab end def then_i_should_see_events_for_all_applications_belonging_to_my_providers - expect(page).to have_content @choice3.current_course.provider.name - expect(page).to have_content @choice2.current_course.provider.name - expect(page).to have_content @choice1.current_course.provider.name - expect(page).not_to have_content @choice4.current_course.provider.name + expect(page).to have_content @choice5.current_course.name + expect(page).to have_content @choice3.current_course.name + expect(page).to have_content @choice2.current_course.name + expect(page).to have_content @choice1.current_course.name + expect(page).not_to have_content @choice4.current_course.name end end