Skip to content

Commit

Permalink
GetActivityLogEvents returns application_choice_id
Browse files Browse the repository at this point in the history 
  Some Audits cannot store a reference to an application choice. We must
  return the application choice id from the join when we query.

  When the query returns Audits for ApplicationForm, we only want to
  return those Audits whose audit_changes are a subset of a whitelist of
  columns. The app is only tracking audits where personal_information,
  contact_information, disability_disclosure and interview_preferences
  are changed.
  • Loading branch information
@inulty-dfe
inulty-dfe committed Oct 24, 2023
1 parent 7755b97 commit 27493f4
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 57 deletions.
37 changes: 17 additions & 20 deletions app/queries/get_activity_log_events.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
Expand Down
118 changes: 89 additions & 29 deletions spec/queries/get_activity_log_events_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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([])
Expand All @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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(
Expand All @@ -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
Expand Down
22 changes: 14 additions & 8 deletions spec/system/provider_interface/see_activity_log_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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

0 comments on commit 27493f4

Please sign in to comment.