Fix inaccurate access logging in timeline ops tool

The access log entries were being recorded as an after_action. In conjunction with other logic which redirected with default parameters if some parameters were unset, this led to extra log entries being recorded, with incorrect details.

This has been fixed by adjusting the sequencing of the access log recording, putting it inside the controller.

Author: samcoy3

app/controllers/inspect/timeline/patients_controller.rb

@@ -6,7 +6,6 @@ class Inspect::Timeline::PatientsController < ApplicationController
   skip_after_action :verify_policy_scoped
   before_action :ensure_ops_tools_feature_enabled
   before_action :set_patient
-  after_action :record_access_log_entry
 
   layout "full"
 
@@ -27,13 +26,12 @@ class Inspect::Timeline::PatientsController < ApplicationController
   def show
     set_pii_settings
 
-    params.reverse_merge!(event_names: DEFAULT_EVENT_NAMES)
     params[:audit_config] ||= {}
 
-    event_names = params[:event_names]
     compare_option = params[:compare_option] || nil
 
-    if params[:detail_config].blank?
+    # Set default values if none present
+    if params[:detail_config].blank? && params[:event_names].blank?
       default_details = TimelineRecords::DEFAULT_DETAILS_CONFIG
       new_params = params.to_unsafe_h.merge("detail_config" => default_details)
       redirect_to inspect_timeline_patient_path(new_params) and return
@@ -46,6 +44,13 @@ def show
         params[:audit_config][:include_filtered_audit_changes]
     }
 
+    @compare_patient = sample_patient(params[:compare_option]) if compare_option
+
+    record_access_log_entry
+
+    params.reverse_merge!(event_names: DEFAULT_EVENT_NAMES)
+    event_names = params[:event_names]
+
     @patient_timeline =
       TimelineRecords.new(
         @patient,
@@ -56,8 +61,6 @@ def show
 
     @no_events_message = true if @patient_timeline.empty?
 
-    @compare_patient = sample_patient(params[:compare_option]) if compare_option
-
     if @compare_patient == :invalid_patient
       @invalid_patient_id = true
     elsif @compare_patient
@@ -144,7 +147,7 @@ def pii_accessed?
   end
 
   def audit_pii_accessed?
-    true if @show_pii && params[:event_names].include?("audits")
+    true if @show_pii && params[:event_names]&.include?("audits")
   end
 
   def record_access_log_entry

spec/features/inspect_timeline_pii_access_logging_spec.rb

@@ -99,7 +99,11 @@ def when_i_login_as_a_support_user_with_pii_access
   end
 
   def and_i_visit_a_patient_timeline_with_pii_enabled
-    visit inspect_timeline_patient_path(id: @patient.id, show_pii: "true")
+    visit inspect_timeline_patient_path(
+            id: @patient.id,
+            show_pii: "true",
+            event_names: ["audits"]
+          )
     expect(page).to have_content("Customise timeline")
   end
 
@@ -108,15 +112,14 @@ def and_i_visit_a_patient_timeline_with_pii_enabled_and_a_comparison_patient
             id: @patient.id,
             show_pii: "true",
             compare_option: "manual_entry",
-            manual_patient_id: @compare_patient.id.to_s
+            manual_patient_id: @compare_patient.id.to_s,
+            event_names: ["audits"]
           )
     expect(page).to have_content("Customise timeline")
   end
 
   def then_an_access_log_entry_is_created_for_the_patient
-    # Two calls are made on first page load, so in this case (since we are visiting with show_pii: true),
-    # two logs are created
-    expect(@patient.access_log_entries.count).to eq(2)
+    expect(@patient.access_log_entries.count).to eq(1)
   end
 
   def and_the_access_log_entry_has_correct_attributes
@@ -125,7 +128,7 @@ def and_the_access_log_entry_has_correct_attributes
 
   def then_access_log_entries_are_created_for_both_patients
     # Check main patient log
-    expect(@patient.access_log_entries.count).to eq(2)
+    expect(@patient.access_log_entries.count).to eq(1)
     verify_log_entry(@patient.access_log_entries.last)
 
     # Check comparison patient log