From 87cd28ac24ce6ba214eae8be14b105c7012db5ef Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Fri, 21 Mar 2025 14:24:46 +0000 Subject: [PATCH 01/27] Create TimelineRecords module for patient event history visualisation Introduces a TimelineRecords class that collects and displays a chronological view of patient-related events across multiple record types. Features configurable detail display, audit trail integration, and privacy controls to ensure no PII is exposed. --- lib/timeline_records.rb | 292 +++++++++++++++ spec/lib/timeline_records_spec.rb | 573 ++++++++++++++++++++++++++++++ 2 files changed, 865 insertions(+) create mode 100644 lib/timeline_records.rb create mode 100644 spec/lib/timeline_records_spec.rb diff --git a/lib/timeline_records.rb b/lib/timeline_records.rb new file mode 100644 index 0000000000..bf0754f5a9 --- /dev/null +++ b/lib/timeline_records.rb @@ -0,0 +1,292 @@ +# frozen_string_literal: true + +class TimelineRecords + DEFAULT_DETAILS_CONFIG = { + cohort_imports: [], + class_imports: [], + sessions: %i[], + school_moves: %i[school_id source], + school_move_log_entries: %i[school_id user_id], + consents: %i[response route], + triages: %i[status performed_by_user_id], + vaccination_records: %i[outcome session_id] + }.freeze + + AVAILABLE_DETAILS_CONFIG = { + cohort_imports: %i[rows_count status uploaded_by_user_id], + class_imports: %i[rows_count year_groups uploaded_by_user_id location_id], + sessions: %i[], + school_moves: %i[source school_id home_educated], + school_move_log_entries: %i[user_id school_id home_educated], + consents: %i[ + programme_id + response + route + parent_id + withdrawn_at + invalidated_at + ], + triages: %i[status performed_by_user_id programme_id invalidated_at], + vaccination_records: %i[ + outcome + performed_by_user_id + programme_id + session_id + vaccine_id + ] + }.freeze + + DEFAULT_AUDITS_CONFIG = { + include_associated_audits: true, + include_filtered_audit_changes: false + }.freeze + + ALLOWED_AUDITED_CHANGES = %i[ + patient_id + session_id + location_id + programme_id + vaccine_id + organisation_id + team_id + school_id + gp_practice_id + uploaded_by_user_id + performed_by_user_id + user_id + parent_id + status + outcome + response + route + date_of_death_recorded_at + restricted_at + invalidated_at + withdrawn_at + rows_count + year_groups + home_educated + source + ].freeze + + def initialize(patient, detail_config: {}, audit_config: {}) + @patient = patient + @patient_id = @patient.id + @patient_events = patient_events(@patient) + @additional_events = additional_events(@patient) + @detail_config = extract_detail_config(detail_config) + @events = [] + @audit_config = audit_config + end + + def render_timeline(*event_names, truncate_columns: false) + load_grouped_events(event_names) + format_timeline_console(truncate_columns) + end + + def additional_events(patient) + patient_imports = patient_events(patient)[:class_imports] + patient_sessions = patient_events(patient)[:sessions] + patient_locations = + Location.joins(:sessions).where(sessions: { id: patient_sessions }) + class_imports = ClassImport.where(location_id: patient_locations) + class_imports = + class_imports.where.not(id: patient_imports) if patient_imports.present? + + { + class_imports: + class_imports + .group_by(&:location_id) + .transform_values { |imports| imports.map(&:id) }, + cohort_imports: + patient + .teams + .flat_map(&:cohort_imports) + .map(&:id) + .reject { |id| patient_events(patient)[:cohort_imports].include?(id) } + } + end + + def patient_events(patient) + { + class_imports: patient.class_imports.map(&:id), + cohort_imports: patient.cohort_imports.map(&:id), + sessions: patient.sessions.map(&:id) + } + end + + def extract_detail_config(detail_config) + detail_config.deep_symbolize_keys + end + + def details + @details ||= DEFAULT_DETAILS_CONFIG.merge(@detail_config) + end + + def audits + @audits ||= DEFAULT_AUDITS_CONFIG.merge(@audit_config) + end + + def load_events(event_names) + event_names.each do |event_name| + event_type = event_name.to_sym + + if details.key?(event_type) + fields = details[event_type] + records = @patient.send(event_type) + records = Array(records) + + records.each do |record| + event_details = + fields.each_with_object({}) do |field, hash| + field_value = record.send(field) + hash[field.to_s] = field_value.nil? ? "nil" : field_value + end + @events << { + event_type: record.class.name, + id: record.id, + details: event_details, + created_at: record.created_at + } + end + else + custom_event_handler(event_type) + end + end + @events.sort_by! { |event| event[:created_at] }.reverse! + end + + def load_grouped_events(event_names) + load_events(event_names) + @grouped_events = + @events.group_by do |event| + event[:created_at].strftime(Date::DATE_FORMATS[:long]) + end + end + + private + + def custom_event_handler(event_type) + case event_type + when :org_cohort_imports + @events += org_cohort_imports_events + when /^add_class_imports_\d+$/ # e.g. add_class_imports_123 + location_id = event_type.to_s.split("_").last + @events += add_class_imports_events(location_id) + when :audits + @events += audits_events + else + puts "No handler for event type: #{event_type}" + end + end + + def org_cohort_imports_events + CohortImport + .where(id: @additional_events[:cohort_imports]) + .map do |cohort_import| + { + event_type: "CohortImport", + id: cohort_import.id, + details: "excluding patient", + created_at: cohort_import.created_at + } + end + end + + def add_class_imports_events(location_id) + ClassImport + .where(id: @additional_events[:class_imports][location_id.to_i]) + .map do |class_import| + { + event_type: "ClassImport", + id: class_import.id, + details: { + location_id: "#{class_import.location_id}, excluding patient" + }, + created_at: class_import.created_at.to_time + } + end + end + + def audits_events + audit_events = + ( + if audits[:include_associated_audits] + @patient.own_and_associated_audits + else + @patient.audits + end + ).reject { it.audited_changes.keys == ["updated_from_pds_at"] } + + audit_events.map do |audit| + filtered_changes = + audit + .audited_changes + .each_with_object({}) do |(key, value), hash| + if ALLOWED_AUDITED_CHANGES.include?(key.to_sym) + hash[key] = value + elsif audits[:include_filtered_audit_changes] + hash[key] = "[FILTERED]" + end + end + + event_type = "#{audit.auditable_type}-Audit" + + event_details = { action: audit.action } + event_details[ + :audited_changes + ] = filtered_changes.deep_symbolize_keys unless filtered_changes.empty? + event_details[:auditable_id] = audit.auditable_id + + { + event_type: event_type, + id: audit.id, + details: event_details, + created_at: audit.created_at + } + end + end + + def format_timeline_console(truncate_columns) + event_type_width = 25 + details_width = 50 + header_format = + if truncate_columns + "%-12s %-10s %-#{event_type_width}s %-12s %-#{details_width}s" + else + "%-12s %-10s %-20s %-10s %-s" + end + puts sprintf( + header_format, + "DATE", + "TIME", + "EVENT_TYPE", + "EVENT-ID", + "DETAILS" + ) + puts "-" * 115 + + @grouped_events.each do |date, events| + puts "=== #{date} ===\n" + "-" * 115 + events.each do |event| + time = event[:created_at].strftime("%H:%M:%S") + event_type = event[:event_type].to_s + event_id = event[:id].to_s + details_string = + if event[:details].is_a?(Hash) + event[:details].map { |k, v| "#{k}: #{v}" }.join(", ") + else + event[:details].to_s + end + if truncate_columns + event_type = event_type.ljust(event_type_width)[0...event_type_width] + details = details_string.ljust(details_width)[0...details_width] + else + details = details_string + end + puts sprintf(header_format, "", time, event_type, event_id, details) + end + end + nil + end +end diff --git a/spec/lib/timeline_records_spec.rb b/spec/lib/timeline_records_spec.rb new file mode 100644 index 0000000000..178fb5b09a --- /dev/null +++ b/spec/lib/timeline_records_spec.rb @@ -0,0 +1,573 @@ +# frozen_string_literal: true + +describe TimelineRecords do + subject(:timeline) do + described_class.new(patient, detail_config: detail_config) + end + + let(:programme) { create(:programme, :hpv) } + let(:detail_config) { {} } + let(:team) { create(:team, programmes: [programme]) } + let(:session) { create(:session, team:, programmes: [programme]) } + let(:class_import) { create(:class_import, session:) } + let(:cohort_import) { create(:cohort_import, team:) } + let(:cohort_import_additional) { create(:cohort_import, team:) } + let(:class_import_additional) { create(:class_import, session:) } + let(:user) { create(:user) } + let(:patient) do + create( + :patient, + given_name: "Alex", + year_group: 8, + address_postcode: "SW1A 1AA", + class_imports: [class_import], + team: + ) + end + let(:school_move) { create(:school_move, :to_school, patient:) } + let(:school_move_log_entry) { create(:school_move_log_entry, patient:) } + let(:triage) do + create( + :triage, + patient:, + programme:, + status: :ready_to_vaccinate, + performed_by: user + ) + end + let(:consent) do + create( + :consent, + patient:, + response: :given, + programme:, + created_at: Date.new(2025, 1, 1) + ) + end + let(:second_consent) do + create( + :consent, + patient:, + response: :given, + programme:, + created_at: Date.new(2025, 2, 1) + ) + end + let(:third_consent) do + create( + :consent, + patient:, + response: :given, + programme:, + created_at: Date.new(2025, 3, 1) + ) + end + let(:fourth_consent) do + create( + :consent, + patient:, + response: :given, + programme:, + created_at: Date.new(2025, 4, 1) + ) + end + let(:consent_audit) do + consent.audits.create!( + audited_changes: { + response: %w[nil given] + }, + associated_type: Patient, + associated_id: patient.id + ) + end + let(:vaccination_record) do + create( + :vaccination_record, + patient:, + programme:, + session:, + created_at: Date.new(2025, 1, 1) + ) + end + let(:second_vaccination_record) do + create( + :vaccination_record, + patient:, + programme:, + session:, + created_at: Date.new(2025, 2, 1) + ) + end + let(:third_vaccination_record) do + create( + :vaccination_record, + patient:, + programme:, + session:, + created_at: Date.new(2025, 3, 1) + ) + end + let(:fourth_vaccination_record) do + create( + :vaccination_record, + patient:, + programme:, + session:, + created_at: Date.new(2025, 4, 1) + ) + end + + describe "#load_events" do + before do + patient.triages << triage + patient.consents << consent + create(:patient_location, patient:, location: session.location, session:) + patient.school_moves << school_move + patient.school_move_log_entries << school_move_log_entry + patient.vaccination_records << vaccination_record + patient.cohort_imports << cohort_import + end + + context "with default details configuration" do + it "loads consent events with default fields" do + timeline.send(:load_events, ["consents"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "Consent" + expect(event[:details]).to eq( + { "response" => consent.response, "route" => consent.route } + ) + end + + it "loads triage events with default fields" do + timeline.send(:load_events, ["triages"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "Triage" + expect(event[:details]).to eq( + { "status" => triage.status, "performed_by_user_id" => user.id } + ) + end + + it "loads cohort_import events with default fields" do + timeline.send(:load_events, ["cohort_imports"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "CohortImport" + expect(event[:details]).to eq({}) + end + + it "loads class_import events with default fields" do + timeline.send(:load_events, ["class_imports"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "ClassImport" + expect(event[:details]).to eq({}) + end + + it "loads school_move events with default fields" do + timeline.send(:load_events, ["school_moves"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "SchoolMove" + expect(event[:details]).to eq( + { + "school_id" => school_move.school_id, + "source" => school_move.source + } + ) + end + + it "loads school_move_log_entry events with default fields" do + timeline.send(:load_events, ["school_move_log_entries"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "SchoolMoveLogEntry" + expect(event[:details]).to eq( + { + "school_id" => school_move_log_entry.school_id, + "user_id" => school_move_log_entry.user_id + } + ) + end + + it "loads vaccination events with default fields" do + timeline.send(:load_events, ["vaccination_records"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "VaccinationRecord" + expect(event[:details]).to eq( + { + "outcome" => vaccination_record.outcome, + "session_id" => vaccination_record.session_id + } + ) + end + end + + context "with custom details configuration" do + let(:detail_config) { { consents: %i[route], triages: %i[status] } } + + before do + patient.consents << consent + patient.triages << triage + end + + it "loads consent events with custom fields" do + timeline.send(:load_events, ["consents"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "Consent" + expect(event[:details]).to eq({ "route" => consent.route }) + end + + it "loads triage events with custom fields" do + timeline.send(:load_events, ["triages"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "Triage" + expect(event[:details]).to eq({ "status" => triage.status }) + end + end + + context "with custom event handler" do + before do + patient.cohort_imports = [cohort_import] + additional_events = { + class_imports: { + session.id => [class_import_additional.id] + }, + cohort_imports: [cohort_import_additional.id] + } + timeline.instance_variable_set(:@additional_events, additional_events) + end + + it "calls custom event handler for add_class_imports" do + timeline.send(:load_events, ["add_class_imports_#{session.id}"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "ClassImport" + expect(event[:id]).to eq class_import_additional.id + expect(event[:details]).to eq( + { location_id: "#{session.location.id}, excluding patient" } + ) + end + + it "calls custom event handler for org_cohort_imports" do + timeline.send(:load_events, ["org_cohort_imports"]) + expect(timeline.instance_variable_get(:@events).size).to eq 1 + event = timeline.instance_variable_get(:@events).first + expect(event[:event_type]).to eq "CohortImport" + expect(event[:id]).to eq cohort_import_additional.id + expect(event[:details]).to eq "excluding patient" + end + end + end + + describe "#load_add_class_imports_events" do + before do + create(:patient_location, patient:, location: session.location, session:) + additional_events = { + class_imports: { + session.id => [class_import_additional.id] + } + } + timeline.instance_variable_set(:@additional_events, additional_events) + end + + it "returns an array of events" do + events = timeline.send(:load_events, ["add_class_imports_#{session.id}"]) + expect(events).to be_an Array + end + + it "includes the class import event" do + events = timeline.send(:load_events, ["add_class_imports_#{session.id}"]) + expect(events.size).to eq 1 + event = events.first + expect(event[:event_type]).to eq "ClassImport" + expect(event[:id]).to eq class_import_additional.id + expect(event[:details]).to eq( + { location_id: "#{session.location.id}, excluding patient" } + ) + expect(event[:created_at]).to eq class_import_additional.created_at + end + + it "handles multiple additional class imports" do + another_additional_class_import = + create(:class_import, session:, created_at: 1.minute.from_now) + additional_events = { + class_imports: { + session.id => [ + class_import_additional.id, + another_additional_class_import.id + ] + } + } + timeline.instance_variable_set(:@additional_events, additional_events) + events = timeline.send(:load_events, ["add_class_imports_#{session.id}"]) + expect(events.size).to eq 2 + expect(events.map { |event| event[:id] }).to contain_exactly( + class_import_additional.id, + another_additional_class_import.id + ) + end + + it "handles no additional class imports" do + additional_events = { class_imports: { session.id => [] } } + timeline.instance_variable_set(:@additional_events, additional_events) + events = timeline.send(:load_events, ["add_class_imports_#{session.id}"]) + expect(events).to be_empty + end + + it "handles a nil session id" do + events = timeline.send(:load_events, ["add_class_imports_nil"]) + expect(events).to be_empty + end + end + + describe "#audits_events" do + before do + create(:patient_location, patient:, location: session.location, session:) + end + + context "with default settings" do + let(:timeline) { described_class.new(patient) } + + it "returns an array of events" do + patient.audits.create!( + audited_changes: { + team_id: [nil, 1], + given_name: %w[Alessia Alice] + } + ) + events = timeline.load_events(["audits"]) + expect(events).to be_an Array + end + + it "includes the audit event" do + patient.audits.create!( + audited_changes: { + team_id: [nil, 1], + given_name: %w[Alessia Alice] + } + ) + events = timeline.load_events(["audits"]) + expect(events.size).to eq 3 # create is the first audit, PatientSession-Audit is the second + event = events.first + expect(event[:event_type]).to eq "Patient-Audit" + expect(event[:details][:audited_changes][:team_id]).to eq([nil, 1]) + expect(event[:created_at]).to eq patient.audits.second.created_at + end + + it "does not include audited changes that are not allowed" do + patient.audits.create!( + audited_changes: { + given_name: %w[Alessia Alice] + } + ) + events = timeline.load_events(["audits"]) + expect(events.size).to eq 3 + expect(events.first[:event_type]).to eq "Patient-Audit" + expect(events.first[:details]).to eq( + { action: nil, auditable_id: patient.id } + ) + end + + it "includes associated audits by default" do + consent.audits << consent_audit + events = timeline.load_events(["audits"]) + associated_event = events.find { |e| e[:id] == consent_audit.id } + expect(associated_event).to be_present + expect(associated_event[:event_type]).to eq "Consent-Audit" + end + + it "excludes audits with only updated_from_pds_at changes" do + patient.audits.create!( + audited_changes: { + updated_from_pds_at: [nil, Time.current] + } + ) + patient.audits.create!(audited_changes: { team_id: [nil, 1] }) + events = timeline.load_events(["audits"]) + expect(events.size).to eq 3 # create is the first audit, PatientSession-Audit is the second + event = events.first + expect(event[:details][:audited_changes]).not_to have_key( + :updated_from_pds_at + ) + expect(event[:details][:audited_changes][:team_id]).to eq([nil, 1]) + end + end + + context "with include_associated_audits: false" do + let(:timeline) do + described_class.new( + patient, + audit_config: { + include_associated_audits: false + } + ) + end + + it "does not include associated audits" do + consent.audits << consent_audit + events = timeline.load_events(["audits"]) + associated_event = events.find { |e| e[:id] == consent_audit.id } + expect(associated_event).to be_nil + end + + it "still includes patient audits" do + patient.audits.create!(audited_changes: { team_id: [nil, 1] }) + events = timeline.load_events(["audits"]) + expect(events.size).to eq 2 + expect(events.first[:event_type]).to eq "Patient-Audit" + end + end + + context "with include_filtered_audit_changes: true" do + let(:timeline) do + described_class.new( + patient, + audit_config: { + include_filtered_audit_changes: true + } + ) + end + + it "includes filtered changes with [FILTERED] value" do + patient.audits.create!( + audited_changes: { + given_name: %w[Alessia Alice] + } + ) + events = timeline.load_events(["audits"]) + expect(events.first[:details][:audited_changes][:given_name]).to eq( + "[FILTERED]" + ) + end + end + end + + describe "#additional_events" do + let(:cohort_imports_with_patient) do + create_list(:cohort_import, 2, team: session.team) + end + let(:cohort_imports_without_patient) do + create_list(:cohort_import, 1, team: session.team) + end + + before do + class_import_additional.location_id = session.id + create( + :patient_location, + patient:, + location: session.location, + session: session + ) + patient.class_imports = [class_import] + patient.cohort_imports = cohort_imports_with_patient + team.cohort_imports = + cohort_imports_with_patient + cohort_imports_without_patient + end + + context "with class imports" do + it "returns a hash with class imports and cohort imports" do + result = timeline.additional_events(patient) + expect(result).to be_a(Hash) + expect(result.keys).to eq(%i[class_imports cohort_imports]) + end + + it "returns class imports that the patient is not in, for sessions that the patient is in" do + result = timeline.additional_events(patient) + expect(result[:class_imports]).to be_a(Hash) + expect(result[:class_imports].keys).to eq([session.location.id]) + expect(result[:class_imports][session.location.id]).to eq( + [class_import_additional.id] + ) + end + end + + context "with cohort imports" do + it "returns cohort imports that the patient is not in, for teams that the patient is in" do + result = timeline.additional_events(patient) + expect(result[:cohort_imports]).to eq( + cohort_imports_without_patient.map(&:id) + ) + end + end + end + + describe "#patient_events" do + let(:class_imports) { create_list(:class_import, 3, session: session) } + let(:cohort_imports) { create_list(:cohort_import, 3, team: session.team) } + + before do + patient.class_imports = class_imports + patient.cohort_imports = cohort_imports + create( + :patient_location, + patient:, + location: session.location, + session: session + ) + end + + context "with class imports" do + it "returns a hash with class imports, cohort imports, and sessions" do + result = timeline.patient_events(patient) + expect(result).to be_a(Hash) + expect(result.keys).to eq(%i[class_imports cohort_imports sessions]) + end + + it "returns an array of class import IDs" do + result = timeline.patient_events(patient) + expect(result[:class_imports]).to eq(class_imports.map(&:id)) + end + end + + context "with cohort imports" do + it "returns an array of cohort import IDs" do + result = timeline.patient_events(patient) + expect(result[:cohort_imports]).to eq(cohort_imports.map(&:id)) + end + end + end + + describe "#load_grouped_events" do + before do + [consent, second_consent, third_consent, fourth_consent].each do |c| + patient.consents << c + end + [ + vaccination_record, + second_vaccination_record, + third_vaccination_record, + fourth_vaccination_record + ].each { |vr| patient.vaccination_records << vr } + create( + :patient_location, + patient:, + location: session.location, + session: session + ) + timeline.send(:load_events, %w[consents triages]) + timeline.send(:load_grouped_events, %w[consents triages]) + end + + it "groups events by date" do + grouped_events = timeline.instance_variable_get(:@grouped_events) + dates = grouped_events.keys + expect(grouped_events).to be_a(Hash) + expect(grouped_events.keys).to all(be_a(String)) + expect(grouped_events.values).to all(be_an(Array)) + expect(dates.uniq.size).to eq(dates.size) + end + + it "orders events by date in descending order" do + grouped_events = timeline.instance_variable_get(:@grouped_events) + dates = grouped_events.keys + expect(dates).to eq( + ["1 April 2025", "1 March 2025", "1 February 2025", "1 January 2025"] + ) + end + end +end From f07122406e611d6344b484025701368c4419b871 Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Fri, 21 Mar 2025 14:36:26 +0000 Subject: [PATCH 02/27] Add timeline UI with configurable filters for patient events Implements a front-end interface for the TimelineRecords module with: - Interactive filters for event types and detail levels - Tabular timeline view with chronological grouping - Controller and views in the Inspect::Timeline namespace - Routes restricted to test environments only - Comparison view to analyze events between patients --- app/components/app_timeline_component.rb | 123 +++++++++++-- .../app_timeline_filter_component.html.erb | 172 ++++++++++++++++++ .../app_timeline_filter_component.rb | 37 ++++ .../inspect/timeline/patients_controller.rb | 112 ++++++++++++ .../inspect/timeline/patients/show.html.erb | 78 ++++++++ config/routes.rb | 9 + lib/timeline_records.rb | 23 ++- spec/features/timeline_records_spec.rb | 45 +++++ 8 files changed, 579 insertions(+), 20 deletions(-) create mode 100644 app/components/app_timeline_filter_component.html.erb create mode 100644 app/components/app_timeline_filter_component.rb create mode 100644 app/controllers/inspect/timeline/patients_controller.rb create mode 100644 app/views/inspect/timeline/patients/show.html.erb create mode 100644 spec/features/timeline_records_spec.rb diff --git a/app/components/app_timeline_component.rb b/app/components/app_timeline_component.rb index 1fe80a1eac..d5671bb38d 100644 --- a/app/components/app_timeline_component.rb +++ b/app/components/app_timeline_component.rb @@ -6,27 +6,37 @@ class AppTimelineComponent < ViewComponent::Base <% @items.each do |item| %> <% next if item.blank? %> -
  • - <% if item[:active] || item[:is_past_item] %> - - <% else %> - - <% end %> - -
    -

    - <%= item[:heading_text].html_safe %> + <% if item[:type] == :section_header %> +
  • +

    + <%= content_tag(:strong, item[:date]) %>

    - - <% if item[:description].present? %> -

    <%= item[:description].html_safe %>

    +
    • + <% else %> +
  • + <% if item[:active] || item[:is_past_item] %> + + <% else %> + <% end %> -

    • -
    • + +
    +

    + <%= format_heading(item).html_safe %> +

    + + <% if item[:description].present? || item[:details].present? %> +
    + <%= format_description(item).html_safe %> +
    + <% end %> +
    + + <% end %> <% end %> ERB @@ -38,4 +48,79 @@ def initialize(items) def render? @items.present? end + + private + + EVENT_COLOUR_MAPPING = { + "CohortImport" => "blue", + "ClassImport" => "purple", + "PatientSession" => "green", + "Consent" => "yellow", + "Triage" => "red", + "VaccinationRecord" => "grey", + "SchoolMove" => "orange", + "SchoolMoveLogEntry" => "pink" + }.freeze + + def format_heading(item) + if item[:type] && item[:created_at] + time = format_time(item[:created_at]) + event_tag = + govuk_tag( + text: item[:event_type], + colour: tag_colour(item[:event_type]) + ) + "#{event_tag} at #{time}" + elsif item[:heading_text] + item[:heading_text] + end + end + + def format_description(item) + if item[:details].present? + formatted_details = format_event_details(item[:details]) + id_info = + item[:id] ? "

    id: #{item[:id]}

    " : "" + "#{id_info}#{formatted_details}" + elsif item[:description].present? + item[:description] + end + end + + def format_time(date_time) + date_time.strftime("%H:%M:%S") + end + + def format_event_details(details) + return "" if details.blank? + + if details.is_a?(Hash) + formatted = + details.map do |key, value| + if value.is_a?(Hash) + nested = + value.map do |sub_key, sub_value| + nested_value = + sub_value.is_a?(String) ? sub_value : sub_value.inspect + "
    #{sub_key}: #{nested_value}
    " + end + "
    #{key}:#{nested.join}
    " + else + "
    #{key}: #{value}
    " + end + end + formatted.join + else + details.to_s + end + end + + def tag_colour(type) + return "light-blue" if type.end_with?("-Audit") + EVENT_COLOUR_MAPPING.fetch(type, "grey") + end + + def govuk_tag(text:, colour:) + helpers.govuk_tag(text: text, colour: colour) + end end diff --git a/app/components/app_timeline_filter_component.html.erb b/app/components/app_timeline_filter_component.html.erb new file mode 100644 index 0000000000..122af27e6b --- /dev/null +++ b/app/components/app_timeline_filter_component.html.erb @@ -0,0 +1,172 @@ +<%= render AppCardComponent.new(filters: true) do |card| %> + <% card.with_heading { "Customise timeline" } %> + <%= form_with url: @url, + method: :get, + data: { module: "autosubmit", + turbo: "true", + turbo_action: "replace" }, + builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %> + <%= f.govuk_fieldset legend: { text: "Events to display:", size: "s" } do %> + <% event_options.keys.each do |value| %> + <%= f.govuk_check_boxes_fieldset :event_names, legend: { hidden: true } do %> + <%= f.govuk_check_box :event_names, + value, + label: { text: value.to_s.humanize }, + checked: value.to_s.in?(params[:event_names] || event_options.keys.map(&:to_s)), + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + + <% available_fields = timeline_fields[value.to_sym] || [] %> + <% if available_fields.any? && value.to_s.in?(params[:event_names]) %> +
    + <% available_fields.each do |field| %> + <%= f.govuk_check_box "detail_config[#{value}]", + field, + small: true, + label: { text: field }, + checked: field.to_s.in?(params.dig("detail_config", value) || []), + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% end %> +
    + <% end %> + <% end %> + <% end %> + + <%= f.govuk_check_boxes_fieldset :audit_config, legend: { hidden: true } do %> + <%= f.govuk_check_box :event_names, "audits", + label: { text: "Audits" }, + checked: "audits".in?(params[:event_names]), + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% if "audits".in?(params[:event_names]) %> +
    + <%= f.govuk_check_box "audit_config[include_associated_audits]", true, false, + multiple: false, + label: { text: "include associated audits" }, + checked: params.dig(:audit_config, :include_associated_audits) == "true", + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + + <%= f.govuk_check_box "audit_config[include_filtered_audit_changes]", true, false, + multiple: false, + label: { text: "include filtered audit changes" }, + checked: params.dig(:audit_config, :include_filtered_audit_changes) == "true", + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> +
    + <% end %> + <% end %> + + <%= f.govuk_check_box :event_names, "org_cohort_imports", + label: { text: "Cohort Imports for Team #{@teams.join(",")} excluding patient" }, + checked: "org_cohort_imports".in?(params[:event_names]), + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + + <% (@additional_class_imports).each do |location_id, import_ids| %> + <%= f.govuk_check_box :event_names, "add_class_imports_#{location_id}", + label: { text: "Class Imports for Location-#{location_id} excluding Patient" }, + checked: "add_class_imports_#{location_id}".in?(params[:event_names]), + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% end %> + + + <%= f.govuk_radio_buttons_fieldset :compare_option, legend: { text: "Compare with another patient:", size: "s" } do %> + <%= f.govuk_radio_button :compare_option, + nil, + label: { text: "Do not compare" }, + checked: params[:compare_option].blank?, + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + + <% if class_imports.present? %> + <%= f.govuk_radio_button :compare_option, + "class_import", + label: { text: "From a Class Import" }, + checked: params[:compare_option] == "class_import", + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" do %> + <% class_imports.each do |import| %> + <%= f.govuk_radio_button :compare_option_class_import, + import, + label: { text: "ClassImport-#{import}" }, + checked: params[:compare_option_class_import].to_s == import.to_s, + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% end %> + <% end %> + <% end %> + + <% if cohort_imports.present? %> + <%= f.govuk_radio_button :compare_option, + "cohort_import", + label: { text: "From a Cohort Import" }, + checked: params[:compare_option] == "cohort_import", + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" do %> + <% cohort_imports.each do |import| %> + <%= f.govuk_radio_button :compare_option_cohort_import, + import, + label: { text: "CohortImport-#{import}" }, + checked: params[:compare_option_cohort_import].to_s == import.to_s, + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% end %> + <% end %> + <% end %> + + <% if sessions.present? %> + <%= f.govuk_radio_button :compare_option, + "session", + label: { text: "In a Session" }, + checked: params[:compare_option] == "session" do %> + <% sessions.each do |session| %> + <%= f.govuk_radio_button :compare_option_session, + session, + label: { text: "Session-#{session}" }, + checked: params[:compare_option_session].to_s == session.to_s && params[:compare_option] == "session", + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% end %> + <% end %> + <% end %> + + <%= f.govuk_radio_button :compare_option, + "manual_entry", + label: { text: "With a specific Patient ID" }, + checked: params[:compare_option] == "manual_entry" do %> + <%= f.govuk_number_field :manual_patient_id, + label: { hidden: true }, + width: 10, + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% end %> + <% end %> + + <%= helpers.govuk_button_link_to "Reset filters", + @reset_url, + class: "govuk-button govuk-button--secondary nhsuk-u-display-block app-button--small", + secondary: true, + "data-autosubmit-target": "reset", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <%= f.govuk_submit "Filter" %> + <% end %> + <% end %> +<% end %> diff --git a/app/components/app_timeline_filter_component.rb b/app/components/app_timeline_filter_component.rb new file mode 100644 index 0000000000..4494fc4846 --- /dev/null +++ b/app/components/app_timeline_filter_component.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class AppTimelineFilterComponent < ViewComponent::Base + def initialize( + url:, + patient:, + teams:, + event_options:, + timeline_fields:, + additional_class_imports:, + class_imports:, + cohort_imports:, + sessions:, + reset_url: + ) + @url = url + @patient = patient + @teams = teams.map(&:id) + @event_options = event_options + @timeline_fields = timeline_fields + @additional_class_imports = additional_class_imports + @class_imports = class_imports + @cohort_imports = cohort_imports + @sessions = sessions + @reset_url = reset_url + end + + attr_reader :url, + :patient, + :event_options, + :timeline_fields, + :additional_class_imports, + :class_imports, + :cohort_imports, + :sessions, + :reset_url +end diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb new file mode 100644 index 0000000000..2c1f73a5a6 --- /dev/null +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +class Inspect::Timeline::PatientsController < ApplicationController + skip_after_action :verify_policy_scoped + skip_before_action :authenticate_user! + before_action :set_patient + + layout "full" + + DEFAULT_EVENT_NAMES = %w[ + consents + school_moves + school_move_log_entries + audits + sessions + triages + vaccination_records + class_imports + cohort_imports + ].freeze + + def show + 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? + 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 + end + + audit_config = { + include_associated_audits: + params[:audit_config][:include_associated_audits], + include_filtered_audit_changes: + params[:audit_config][:include_filtered_audit_changes] + } + + @patient_timeline = + TimelineRecords.new( + @patient, + detail_config: build_details_config, + audit_config: audit_config + ).load_timeline_events(event_names) + + @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 + @compare_patient_timeline = + TimelineRecords.new( + @compare_patient, + detail_config: build_details_config, + audit_config: audit_config + ).load_timeline_events(event_names) + + @no_events_compare_message = true if @compare_patient_timeline.empty? + end + end + + private + + def set_patient + @patient = Patient.find(params[:id]) + timeline = TimelineRecords.new(@patient) + @patient_events = timeline.patient_events(@patient) + @additional_events = timeline.additional_events(@patient) + end + + # TODO: Fix so that a new comparison patient isn't sampled every time + # a filter option is changed and the page is reloaded. + def sample_patient(compare_option) + case compare_option + when "class_import" + class_import = params[:compare_option_class_import] + class_import.patients.where.not(id: @patient.id).sample + when "cohort_import" + cohort_import = params[:compare_option_cohort_import] + cohort_import.patients.where.not(id: @patient.id).sample + when "session" + session = Session.find(params[:compare_option_session]) + session.patients.where.not(id: @patient.id).sample + when "manual_entry" + begin + Patient.find(params[:manual_patient_id]) + rescue ActiveRecord::RecordNotFound + :invalid_patient + end + else + raise ArgumentError, + "Invalid patient comparison option: #{compare_option}" + end + end + + def build_details_config + details_params = params[:detail_config] || {} + details_params = details_params.to_unsafe_h unless details_params.is_a?( + Hash + ) + + details_params.each_with_object({}) do |(event_type, fields), hash| + selected_fields = Array(fields).reject(&:blank?).map(&:to_sym) + hash[event_type.to_sym] = selected_fields + end + end +end diff --git a/app/views/inspect/timeline/patients/show.html.erb b/app/views/inspect/timeline/patients/show.html.erb new file mode 100644 index 0000000000..04e648ca83 --- /dev/null +++ b/app/views/inspect/timeline/patients/show.html.erb @@ -0,0 +1,78 @@ +<%= h1 page_title: @patient.initials do %> + <%= "Inspect Patient #{@patient.id}" %> +<% end %> + + + <%= "Patient.find(#{@patient.id})" %> + + +<% if params[:event_names].include?("audits") && @patient_timeline %> + <%= govuk_inset_text do %> +

    + Only audited changes that do not involve PII are included +

    + <% end %> +<% end %> + +
    + +
    + <%= render AppTimelineFilterComponent.new( + url: inspect_timeline_patient_path, + patient: @patient, + teams: @patient.teams, + event_options: TimelineRecords::DEFAULT_DETAILS_CONFIG, + timeline_fields: TimelineRecords::AVAILABLE_DETAILS_CONFIG, + class_imports: @patient_events[:class_imports], + cohort_imports: @patient_events[:cohort_imports], + sessions: @patient_events[:sessions], + additional_class_imports: @additional_events[:class_imports], + reset_url: inspect_timeline_patient_path( + event_names: Inspect::Timeline::PatientsController::DEFAULT_EVENT_NAMES, + detail_config: TimelineRecords::DEFAULT_DETAILS_CONFIG, + compare_option: nil, + ), + ) %> +
    + +
    + <% if @no_events_message %> + <%= render AppWarningCalloutComponent.new( + heading: "No events found", + description: "Patient-#{@patient.id} doesn't have the following events recorded: " + + "#{params[:event_names].map(&:humanize).join(", ")}".html_safe, + ) %> + <% else %> + <% if @compare_patient_timeline || @invalid_patient_id || @no_events_compare_message %> +
    +

    Patient <%= @patient.id %>

    + <%= render AppTimelineComponent.new(@patient_timeline) %> +
    + + <% if @invalid_patient_id %> +
    + <%= render AppWarningCalloutComponent.new( + heading: "Invalid patient ID", + description: "Patient-#{params[:manual_patient_id]} doesn't exist", + ) %> +
    + <% elsif @no_events_compare_message %> +
    + <%= render AppWarningCalloutComponent.new( + heading: "No events found for comparison patient", + description: "Patient-#{@compare_patient.id} doesn't have the following events recorded: " + + "#{params[:event_names].map(&:humanize).join(", ")}".html_safe, + ) %> +
    + <% elsif @compare_patient_timeline %> +

    Patient <%= @compare_patient.id %>

    +
    + <%= render AppTimelineComponent.new(@compare_patient_timeline) %> +
    + <% end %> + <% else %> + <%= render AppTimelineComponent.new(@patient_timeline) %> + <% end %> + <% end %> +
    +
    \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index 8955f7d728..fb57f2aebc 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -345,4 +345,13 @@ end get "/oidc/jwks", to: "jwks#jwks" + + constraints -> { Rails.env.local? } do + namespace :inspect do + get "graph/:object_type/:object_id", to: "graphs#show" + namespace :timeline do + resources :patients, only: [:show] + end + end + end end diff --git a/lib/timeline_records.rb b/lib/timeline_records.rb index bf0754f5a9..28aa21714a 100644 --- a/lib/timeline_records.rb +++ b/lib/timeline_records.rb @@ -164,7 +164,28 @@ def load_grouped_events(event_names) end end - private + def load_timeline_events(event_names) + load_grouped_events(event_names) + + formatted_items = [] + + @grouped_events.each do |date, events| + formatted_items << { type: :section_header, date: date } + + events.each do |event| + formatted_items << { + type: :event, + event_type: event[:event_type], + id: event[:id], + details: event[:details], + created_at: event[:created_at], + active: false, + is_past_item: true + } + end + end + formatted_items + end def custom_event_handler(event_type) case event_type diff --git a/spec/features/timeline_records_spec.rb b/spec/features/timeline_records_spec.rb new file mode 100644 index 0000000000..e323dfa35d --- /dev/null +++ b/spec/features/timeline_records_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +describe "TimelineRecords UI" do + before do + create(:session, id: 1) + create(:patient, id: 1, session: Session.first) + end + + scenario "Endpoint doesn't exist in prod" do + given_i_am_in_production + + when_i_visit_the_timeline_endpoint + then_i_should_see_a_404_error + end + + scenario "Endpoint is rendered in test" do + given_i_am_in_test + + when_i_visit_the_timeline_endpoint + then_i_should_see_the_page_rendered + end + + def given_i_am_in_production + allow(Rails).to receive(:env).and_return("production".inquiry) + end + + def given_i_am_in_test + # Already implicitly in test + end + + def when_i_visit_the_timeline_endpoint + visit "/inspect/timeline/patients/1" + end + + def then_i_should_see_a_404_error + expect(page.html).to include( + "If you entered a web address, check it is correct." + ) + end + + def then_i_should_see_the_page_rendered + expect(page.status_code).to eq(200) + expect(page.html).to include("Inspect Patient 1") + end +end From 2abcb6044ca448ce423a5abe965a44ea0ca25504 Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Wed, 10 Sep 2025 21:18:09 +0100 Subject: [PATCH 03/27] Update sample_patient function to return nil when no comparison option is selected Changed the default behavior of the sample_patient function to return nil when no comparison option is provided, instead of raising an error. Added a meaningful error message when an invalid comparison option is chosen. --- app/controllers/inspect/timeline/patients_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index 2c1f73a5a6..fba51c218e 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -76,6 +76,8 @@ def set_patient # TODO: Fix so that a new comparison patient isn't sampled every time # a filter option is changed and the page is reloaded. def sample_patient(compare_option) + return nil if compare_option.blank? || compare_option == "on" + case compare_option when "class_import" class_import = params[:compare_option_class_import] From d45fb4859aef1762d9e880a1bec369686712b1cb Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Wed, 10 Sep 2025 21:19:37 +0100 Subject: [PATCH 04/27] Add ability to show PII related information on the timeline A show_pii flag allows optional exclusion of personally-identifiable information from the event details and audited changes relating to a patient. This has been set to false by default in the controller, but can be toggled on or off with a check box in the UI. --- .../app_timeline_filter_component.html.erb | 9 + .../app_timeline_filter_component.rb | 7 +- .../inspect/timeline/patients_controller.rb | 13 +- .../inspect/timeline/patients/show.html.erb | 10 +- lib/timeline_records.rb | 162 +++++++++++++++--- spec/lib/timeline_records_spec.rb | 27 ++- 6 files changed, 193 insertions(+), 35 deletions(-) diff --git a/app/components/app_timeline_filter_component.html.erb b/app/components/app_timeline_filter_component.html.erb index 122af27e6b..1ae466d8ed 100644 --- a/app/components/app_timeline_filter_component.html.erb +++ b/app/components/app_timeline_filter_component.html.erb @@ -158,6 +158,15 @@ "data-turbo-permanent": "true" %> <% end %> <% end %> + +
    + <%= f.govuk_check_box :show_pii, true, + label: { text: "Show PII" }, + checked: @show_pii, + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> +
    <%= helpers.govuk_button_link_to "Reset filters", @reset_url, diff --git a/app/components/app_timeline_filter_component.rb b/app/components/app_timeline_filter_component.rb index 4494fc4846..0638412907 100644 --- a/app/components/app_timeline_filter_component.rb +++ b/app/components/app_timeline_filter_component.rb @@ -11,7 +11,8 @@ def initialize( class_imports:, cohort_imports:, sessions:, - reset_url: + reset_url:, + show_pii: ) @url = url @patient = patient @@ -23,6 +24,7 @@ def initialize( @cohort_imports = cohort_imports @sessions = sessions @reset_url = reset_url + @show_pii = show_pii end attr_reader :url, @@ -33,5 +35,6 @@ def initialize( :class_imports, :cohort_imports, :sessions, - :reset_url + :reset_url, + :show_pii end diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index fba51c218e..0416ca672e 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -7,6 +7,8 @@ class Inspect::Timeline::PatientsController < ApplicationController layout "full" + SHOW_PII = false + DEFAULT_EVENT_NAMES = %w[ consents school_moves @@ -20,6 +22,8 @@ class Inspect::Timeline::PatientsController < ApplicationController ].freeze def show + @show_pii = params[:show_pii] || SHOW_PII + params.reverse_merge!(event_names: DEFAULT_EVENT_NAMES) params[:audit_config] ||= {} @@ -43,7 +47,8 @@ def show TimelineRecords.new( @patient, detail_config: build_details_config, - audit_config: audit_config + audit_config: audit_config, + show_pii: @show_pii ).load_timeline_events(event_names) @no_events_message = true if @patient_timeline.empty? @@ -95,8 +100,10 @@ def sample_patient(compare_option) :invalid_patient end else - raise ArgumentError, - "Invalid patient comparison option: #{compare_option}" + raise ArgumentError, <<~MESSAGE + Invalid patient comparison option: #{compare_option}. + Supported options are: class_import, cohort_import, session, manual_entry + MESSAGE end end diff --git a/app/views/inspect/timeline/patients/show.html.erb b/app/views/inspect/timeline/patients/show.html.erb index 04e648ca83..d4f545b810 100644 --- a/app/views/inspect/timeline/patients/show.html.erb +++ b/app/views/inspect/timeline/patients/show.html.erb @@ -6,7 +6,7 @@ <%= "Patient.find(#{@patient.id})" %> -<% if params[:event_names].include?("audits") && @patient_timeline %> +<% if params[:event_names].include?("audits") && @patient_timeline && !@show_pii %> <%= govuk_inset_text do %>

    Only audited changes that do not involve PII are included @@ -16,13 +16,13 @@

    -
    +
    <%= render AppTimelineFilterComponent.new( url: inspect_timeline_patient_path, patient: @patient, teams: @patient.teams, event_options: TimelineRecords::DEFAULT_DETAILS_CONFIG, - timeline_fields: TimelineRecords::AVAILABLE_DETAILS_CONFIG, + timeline_fields: @show_pii ? TimelineRecords::AVAILABLE_DETAILS_CONFIG_WITH_PII : TimelineRecords::AVAILABLE_DETAILS_CONFIG, class_imports: @patient_events[:class_imports], cohort_imports: @patient_events[:cohort_imports], sessions: @patient_events[:sessions], @@ -31,11 +31,13 @@ event_names: Inspect::Timeline::PatientsController::DEFAULT_EVENT_NAMES, detail_config: TimelineRecords::DEFAULT_DETAILS_CONFIG, compare_option: nil, + show_pii: false, ), + show_pii: @show_pii, ) %>
    -
    +
    <% if @no_events_message %> <%= render AppWarningCalloutComponent.new( heading: "No events found", diff --git a/lib/timeline_records.rb b/lib/timeline_records.rb index 28aa21714a..127d8a8b9d 100644 --- a/lib/timeline_records.rb +++ b/lib/timeline_records.rb @@ -2,38 +2,102 @@ class TimelineRecords DEFAULT_DETAILS_CONFIG = { - cohort_imports: [], - class_imports: [], - sessions: %i[], - school_moves: %i[school_id source], - school_move_log_entries: %i[school_id user_id], consents: %i[response route], + sessions: %i[], + session_attendances: %i[], triages: %i[status performed_by_user_id], - vaccination_records: %i[outcome session_id] + vaccination_records: %i[outcome session_id], + cohort_imports: %i[], + class_imports: %i[], + parents: %i[], + gillick_assessments: %i[], + parent_relationships: %i[], + school_moves: %i[school_id source], + school_move_log_entries: %i[school_id user_id] }.freeze AVAILABLE_DETAILS_CONFIG = { - cohort_imports: %i[rows_count status uploaded_by_user_id], - class_imports: %i[rows_count year_groups uploaded_by_user_id location_id], - sessions: %i[], - school_moves: %i[source school_id home_educated], - school_move_log_entries: %i[user_id school_id home_educated], - consents: %i[ - programme_id - response - route - parent_id - withdrawn_at - invalidated_at + consents: %i[response route updated_at withdrawn_at invalidated_at], + sessions: %i[slug academic_year], + session_attendances: %i[attending updated_at], + triages: %i[status updated_at invalidated_at performed_by_user_id], + vaccination_records: %i[ + outcome + performed_at + updated_at + discarded_at + uuid + session_id + ], + cohort_imports: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count ], - triages: %i[status performed_by_user_id programme_id invalidated_at], + class_imports: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + year_groups + ], + parents: %i[], + gillick_assessments: %i[], + parent_relationships: %i[], + school_moves: %i[school_id source], + school_move_log_entries: %i[school_id user_id] + }.freeze + + AVAILABLE_DETAILS_CONFIG_WITH_PII = { + consents: %i[response route updated_at withdrawn_at invalidated_at], + sessions: %i[slug academic_year], + session_attendances: %i[attending updated_at], + triages: %i[status updated_at invalidated_at performed_by_user_id], vaccination_records: %i[ outcome - performed_by_user_id - programme_id + performed_at + updated_at + discarded_at + uuid session_id - vaccine_id - ] + ], + cohort_imports: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + ], + class_imports: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + year_groups + ], + parents: %i[full_name email phone], + gillick_assessments: %i[ + knows_vaccination + knows_disease + knows_consequences + knows_delivery + knows_side_effects + ], + parent_relationships: %i[type other_name], + school_moves: %i[school_id source], + school_move_log_entries: %i[school_id user_id] }.freeze DEFAULT_AUDITS_CONFIG = { @@ -69,7 +133,51 @@ class TimelineRecords source ].freeze - def initialize(patient, detail_config: {}, audit_config: {}) + ALLOWED_AUDITED_CHANGES_WITH_PII = %i[ + full_name + email + phone + nhs_number + given_name + family_name + date_of_birth + address_line_1 + address_line_2 + address_town + address_postcode + home_educated + updated_from_pds_at + restricted_at + date_of_death + pending_changes + patient_id + session_id + location_id + programme_id + vaccine_id + organisation_id + team_id + school_id + gp_practice_id + uploaded_by_user_id + performed_by_user_id + user_id + parent_id + status + outcome + response + route + date_of_death_recorded_at + restricted_at + invalidated_at + withdrawn_at + rows_count + year_groups + home_educated + source + ].freeze + + def initialize(patient, detail_config: {}, audit_config: {}, show_pii: false) @patient = patient @patient_id = @patient.id @patient_events = patient_events(@patient) @@ -77,6 +185,7 @@ def initialize(patient, detail_config: {}, audit_config: {}) @detail_config = extract_detail_config(detail_config) @events = [] @audit_config = audit_config + @show_pii = show_pii end def render_timeline(*event_names, truncate_columns: false) @@ -239,12 +348,15 @@ def audits_events end ).reject { it.audited_changes.keys == ["updated_from_pds_at"] } + allowed_changes = + @show_pii ? ALLOWED_AUDITED_CHANGES_WITH_PII : ALLOWED_AUDITED_CHANGES + audit_events.map do |audit| filtered_changes = audit .audited_changes .each_with_object({}) do |(key, value), hash| - if ALLOWED_AUDITED_CHANGES.include?(key.to_sym) + if allowed_changes.include?(key.to_sym) hash[key] = value elsif audits[:include_filtered_audit_changes] hash[key] = "[FILTERED]" diff --git a/spec/lib/timeline_records_spec.rb b/spec/lib/timeline_records_spec.rb index 178fb5b09a..04382190df 100644 --- a/spec/lib/timeline_records_spec.rb +++ b/spec/lib/timeline_records_spec.rb @@ -2,11 +2,16 @@ describe TimelineRecords do subject(:timeline) do - described_class.new(patient, detail_config: detail_config) + described_class.new( + patient, + detail_config: detail_config, + show_pii: show_pii + ) end let(:programme) { create(:programme, :hpv) } let(:detail_config) { {} } + let(:show_pii) { false } let(:team) { create(:team, programmes: [programme]) } let(:session) { create(:session, team:, programmes: [programme]) } let(:class_import) { create(:class_import, session:) } @@ -398,6 +403,26 @@ end end + context "with show_pii: true" do + let(:show_pii) { true } + + it "includes PII-based audited changes" do + patient.audits.create!( + audited_changes: { + given_name: %w[Alessia Alice], + organisation_id: [nil, 1] + } + ) + events = timeline.load_events(["audits"]) + expect(events.first[:details][:audited_changes]).to have_key( + :given_name + ) + expect(events.first[:details][:audited_changes]).to have_key( + :organisation_id + ) + end + end + context "with include_associated_audits: false" do let(:timeline) do described_class.new( From e9251bd1a0a92b38eb379fa391a2e53279dcd98e Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Tue, 30 Sep 2025 12:40:40 +0100 Subject: [PATCH 05/27] Refactor timeline config Refactor details and audits config for TimelineRecords to make it mroe readable. The PII fields are separated and merged into the base fields to created "with_pii" variables --- lib/timeline_records.rb | 209 +++++++++++++++++----------------------- 1 file changed, 86 insertions(+), 123 deletions(-) diff --git a/lib/timeline_records.rb b/lib/timeline_records.rb index 127d8a8b9d..ce20214292 100644 --- a/lib/timeline_records.rb +++ b/lib/timeline_records.rb @@ -2,181 +2,144 @@ class TimelineRecords DEFAULT_DETAILS_CONFIG = { - consents: %i[response route], - sessions: %i[], - session_attendances: %i[], - triages: %i[status performed_by_user_id], - vaccination_records: %i[outcome session_id], - cohort_imports: %i[], + changesets: %i[import_id import_type], class_imports: %i[], - parents: %i[], + cohort_imports: %i[], + consents: %i[response route], gillick_assessments: %i[], parent_relationships: %i[], + parents: %i[], + pds_search_results: %i[], + school_move_log_entries: %i[school_id user_id], school_moves: %i[school_id source], - school_move_log_entries: %i[school_id user_id] + sessions: %i[location_id], + teams: %i[name], + triages: %i[performed_by_user_id status], + vaccination_records: %i[outcome session_id] }.freeze AVAILABLE_DETAILS_CONFIG = { - consents: %i[response route updated_at withdrawn_at invalidated_at], - sessions: %i[slug academic_year], - session_attendances: %i[attending updated_at], - triages: %i[status updated_at invalidated_at performed_by_user_id], - vaccination_records: %i[ - outcome - performed_at - updated_at - discarded_at - uuid - session_id - ], - cohort_imports: %i[ + changesets: %i[import_id import_type], + class_imports: %i[ + changed_record_count csv_filename + exact_duplicate_record_count + new_record_count processed_at - status rows_count - new_record_count - exact_duplicate_record_count - changed_record_count + status + year_groups ], - class_imports: %i[ + cohort_imports: %i[ + changed_record_count csv_filename + exact_duplicate_record_count + new_record_count processed_at - status rows_count - new_record_count - exact_duplicate_record_count - changed_record_count - year_groups + status ], - parents: %i[], + consents: %i[invalidated_at response route updated_at withdrawn_at], gillick_assessments: %i[], parent_relationships: %i[], + parents: %i[], + pds_search_results: %i[step], + school_move_log_entries: %i[school_id user_id], school_moves: %i[school_id source], - school_move_log_entries: %i[school_id user_id] - }.freeze - - AVAILABLE_DETAILS_CONFIG_WITH_PII = { - consents: %i[response route updated_at withdrawn_at invalidated_at], - sessions: %i[slug academic_year], - session_attendances: %i[attending updated_at], - triages: %i[status updated_at invalidated_at performed_by_user_id], + sessions: %i[academic_year location_id slug], + teams: %i[name], + triages: %i[invalidated_at performed_by_user_id status updated_at], vaccination_records: %i[ + discarded_at + nhs_immunisations_api_synced_at outcome performed_at + protocol + session_id + source updated_at - discarded_at uuid - session_id - ], - cohort_imports: %i[ - csv_filename - processed_at - status - rows_count - new_record_count - exact_duplicate_record_count - changed_record_count - ], - class_imports: %i[ - csv_filename - processed_at - status - rows_count - new_record_count - exact_duplicate_record_count - changed_record_count - year_groups - ], - parents: %i[full_name email phone], + ] + }.freeze + + AVAILABLE_DETAILS_CONFIG_PII = { + changesets: %i[pds_nhs_number uploaded_nhs_number], gillick_assessments: %i[ - knows_vaccination - knows_disease knows_consequences knows_delivery + knows_disease knows_side_effects + knows_vaccination ], - parent_relationships: %i[type other_name], - school_moves: %i[school_id source], - school_move_log_entries: %i[school_id user_id] + parent_relationships: %i[other_name type], + parents: %i[email full_name phone], + pds_search_results: %i[nhs_number] }.freeze + AVAILABLE_DETAILS_CONFIG_WITH_PII = + AVAILABLE_DETAILS_CONFIG.merge( + AVAILABLE_DETAILS_CONFIG_PII + ) { |_, base_fields, pii_fields| (base_fields + pii_fields).uniq } + DEFAULT_AUDITS_CONFIG = { include_associated_audits: true, include_filtered_audit_changes: false }.freeze ALLOWED_AUDITED_CHANGES = %i[ - patient_id - session_id + date_of_death_recorded_at + gp_practice_id + home_educated + invalidated_at location_id - programme_id - vaccine_id organisation_id - team_id - school_id - gp_practice_id - uploaded_by_user_id - performed_by_user_id - user_id - parent_id - status outcome + parent_id + patient_id + performed_by_user_id + programme_id + registration_academic_year response - route - date_of_death_recorded_at restricted_at - invalidated_at - withdrawn_at rows_count - year_groups - home_educated + route + school_id + session_id source + status + team_id + uploaded_by_user_id + user_id + vaccine_id + withdrawn_at + year_groups ].freeze - ALLOWED_AUDITED_CHANGES_WITH_PII = %i[ - full_name - email - phone - nhs_number - given_name - family_name - date_of_birth + ALLOWED_AUDITED_CHANGES_PII = %i[ address_line_1 address_line_2 - address_town address_postcode - home_educated - updated_from_pds_at - restricted_at + address_town + birth_academic_year + date_of_birth date_of_death + email + family_name + full_name + gender_code + given_name + nhs_number pending_changes - patient_id - session_id - location_id - programme_id - vaccine_id - organisation_id - team_id - school_id - gp_practice_id - uploaded_by_user_id - performed_by_user_id - user_id - parent_id - status - outcome - response - route - date_of_death_recorded_at - restricted_at - invalidated_at - withdrawn_at - rows_count - year_groups - home_educated - source + phone + preferred_family_name + preferred_given_name + registration + updated_from_pds_at ].freeze + ALLOWED_AUDITED_CHANGES_WITH_PII = + (ALLOWED_AUDITED_CHANGES + ALLOWED_AUDITED_CHANGES_PII).uniq.freeze + def initialize(patient, detail_config: {}, audit_config: {}, show_pii: false) @patient = patient @patient_id = @patient.id From 461af24493c60310a46b0b8a8ea94cd91bfeca35 Mon Sep 17 00:00:00 2001 From: Alistair White-Horne Date: Tue, 15 Apr 2025 09:33:18 +0100 Subject: [PATCH 06/27] Improve graph_records.rb --- lib/graph_records.rb | 517 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 517 insertions(+) create mode 100644 lib/graph_records.rb diff --git a/lib/graph_records.rb b/lib/graph_records.rb new file mode 100644 index 0000000000..6947695975 --- /dev/null +++ b/lib/graph_records.rb @@ -0,0 +1,517 @@ +# frozen_string_literal: true + +require "digest" + +# Spit out a Mermaid-style graph of records. +# +# Usage: +# graph = GraphRecords.new.graph(patients: [patient]) +# puts graph +# +class GraphRecords + # TODO: put all these constants elsewhere? + BOX_STYLES = %w[ + fill:#e6194B,color:white + fill:#3cb44b,color:white + fill:#ffe119,color:black + fill:#4363d8,color:white + fill:#f58231,color:white + fill:#911eb4,color:white + fill:#42d4f4,color:black + fill:#f032e6,color:white + fill:#bfef45,color:black + fill:#fabed4,color:black + fill:#469990,color:white + fill:#dcbeff,color:black + fill:#9A6324,color:white + fill:#fffac8,color:black + fill:#800000,color:white + fill:#aaffc3,color:black + fill:#808000,color:white + fill:#ffd8b1,color:black + fill:#000075,color:white + fill:#a9a9a9,color:white + fill:#ffffff,color:black + fill:#000000,color:white + ].freeze + + DEFAULT_NODE_ORDER = %w[ + programme + organisation + team + location + session + session_date + class_import + cohort_import + patient_session + session_attendance + gillick_assessment + patient + vaccine + batch + vaccination_record + triage + user + consent + consent_form + parent_relationship + parent + ].freeze + + ALLOWED_TYPES = DEFAULT_NODE_ORDER + + DEFAULT_TRAVERSALS = { + patient: { + patient: %i[ + parents + consents + cohort_imports + class_imports + vaccination_records + patient_sessions + triages + school + ], + parent: %i[patients consents cohort_imports class_imports], + consent: %i[consent_form patient parent], + session: %i[location], + patient_session: %i[session session_attendances gillick_assessments], + vaccination_record: %i[session] + }, + parent: { + parent: %i[class_imports cohort_imports consents patients], + class_import: %i[session], + consent: %i[parent patient], + patient: %i[parents sessions consents], + session: %i[location] + }, + consent: { + consent: %i[consent_form parent patient programme], + parent: %i[patients], + patient: %i[parents] + }, + consent_form: { + consent_form: [:consent] + }, + vaccination_record: { + vaccination_record: %i[ + patient + programme + session + vaccine + performed_by_user + ], + patient: [:consents], + session: [:location], + consent: [:programme] + }, + location: { + location: %i[sessions team] + }, + session: { + session: %i[location programmes session_dates] + }, + session_attendance: { + session_attendance: %i[patient_session session_date], + patient_session: %i[session gillick_assessments patient], + session: %i[location], + session_date: %i[session] + }, + patient_session: { + patient_session: %i[ + session + patient + session_attendances + gillick_assessments + ], + session: %i[location] + }, + gillick_assessment: { + gillick_assessment: %i[patient_session performed_by programme], + patient_session: %i[session patient], + session: %i[location] + }, + triage: { + triage: %i[patient performed_by programme] + }, + programme: { + programme: %i[teams vaccines] + }, + organisation: { + organisation: %i[teams] + }, + team: { + team: %i[organisation programmes] + }, + vaccine: { + vaccine: %i[batches programme] + }, + batch: { + batch: %i[team vaccine], + vaccine: [:programme] + }, + user: { + user: %i[teams programmes], + team: [:programmes] + }, + session_date: { + session_date: [:session], + session: %i[location] + }, + cohort_import: { + cohort_import: %i[team uploaded_by] + }, + class_import: { + class_import: %i[team uploaded_by] + } + }.freeze + + DETAIL_WHITELIST = { + consent: %i[ + response + route + created_at + updated_at + withdrawn_at + invalidated_at + ], + session: %i[slug clinic? academic_year], + session_attendance: %i[attending created_at updated_at], + triage: %i[status created_at updated_at invalidated_at], + vaccination_record: %i[ + outcome + performed_at + created_at + updated_at + discarded_at + uuid + ], + programme: %i[type], + vaccine: %i[nivs_name], + organisation: %i[ods_code], + team: %i[name workgroup], + location: %i[name type year_groups], + cohort_import: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + ], + class_import: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + year_groups + ], + session_date: %i[value], + patient: %i[ + updated_from_pds_at + date_of_death_recorded_at + restricted_at + invalidated_at + ], + parent: %i[], + patient_session: %i[], + gillick_assessment: %i[created_at], + batch: %i[name expiry archived_at], + user: %i[fallback_role uid], + consent_form: %i[response recorded_at archived_at], + parent_relationship: %i[type] + }.freeze + + DETAIL_WHITELIST_WITH_PII = { + consent: %i[ + response + route + created_at + updated_at + withdrawn_at + invalidated_at + ], + session: %i[slug clinic? academic_year], + session_attendance: %i[attending created_at updated_at], + triage: %i[status created_at updated_at invalidated_at], + vaccination_record: %i[ + outcome + performed_at + created_at + updated_at + discarded_at + uuid + ], + programme: %i[type], + vaccine: %i[nivs_name], + organisation: %i[name ods_code], + location: %i[name address_postcode type year_groups], + cohort_import: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + ], + class_import: %i[ + csv_filename + processed_at + status + rows_count + new_record_count + exact_duplicate_record_count + changed_record_count + year_groups + ], + session_date: %i[value], + team: %i[name], + patient: %i[ + nhs_number + given_name + family_name + date_of_birth + address_line_1 + address_line_2 + address_town + address_postcode + home_educated + updated_from_pds_at + restricted_at + date_of_death + date_of_death_recorded_at + updated_from_pds_at + invalidated_at + pending_changes + ], + parent: %i[full_name email phone], + patient_session: %i[], + gillick_assessment: %i[ + knows_vaccination + knows_disease + knows_consequences + knows_delivery + knows_side_effects + created_at + ], + batch: %i[name expiry archived_at], + user: %i[given_name family_name email fallback_role uid], + consent_form: %i[ + response + recorded_at + archived_at + given_name + family_name + address_postcode + date_of_birth + ], + parent_relationship: %i[type other_name] + }.freeze + + # @param focus_config [Hash] Hash of model names to ids to focus on (make bold) + # @param node_order [Array] Array of model names in order to render nodes + # @param traversals_config [Hash] Hash of model names to arrays of associations to traverse + # @param node_limit [Integer] The maximum number of nodes which can be displayed + def initialize( + focus_config: {}, + node_order: DEFAULT_NODE_ORDER, + traversals_config: {}, + node_limit: 1000, + primary_type: nil, + clickable: false + ) + @focus_config = focus_config + @node_order = node_order + @traversals_config = traversals_config + @node_limit = node_limit + @primary_type = primary_type + @clickable = clickable + end + + # @param objects [Hash] Hash of model name to ids to be graphed + def graph(**objects) + @nodes = Set.new + @edges = Set.new + @inspected = Set.new + @focus = @focus_config.map { _1.to_s.classify.constantize.where(id: _2) } + + @primary_type ||= + if objects.keys.size >= 1 + objects.keys.first.to_s.singularize.to_sym + else + :patient + end + + objects.map do |klass, ids| + class_name = klass.to_s.singularize + associated_objects = + load_association(class_name.classify.constantize.where(id: ids)) + + @focus += associated_objects + + associated_objects.each do |obj| + @nodes << obj + introspect(obj) + end + end + ["flowchart TB"] + render_styles + render_nodes + render_edges + + (@clickable ? render_clicks : []) + rescue StandardError => e + if e.message.include?("Recursion limit") + # Create a Mermaid diagram with a red box containing the error message. + [ + "flowchart TB", + " error[#{e.message}]", + " style error fill:#f88,stroke:#f00,stroke-width:2px" + ] + else + raise e + end + end + + def traversals + @traversals ||= + (DEFAULT_TRAVERSALS[@primary_type] || {}).merge(@traversals_config) + end + + def render_styles + object_types = @nodes.map { |node| node.class.name.underscore.to_sym }.uniq + + styles = + object_types.each_with_object({}) do |type, hash| + color_index = + Digest::MD5.hexdigest(type.to_s).to_i(16) % BOX_STYLES.length + hash[type] = "#{BOX_STYLES[color_index]},stroke:#000" + end + + focused_styles = + styles.each_with_object({}) do |(type, style), hash| + hash["#{type}_focused"] = "#{style},stroke-width:3px" + end + + styles.merge!(focused_styles) + + styles + .with_indifferent_access + .slice(*@nodes.map { class_text_for_obj(it) }) + .map { |klass, style| " classDef #{klass} #{style}" } + end + + def render_nodes + @nodes.to_a.map { " #{node_with_class(it)}" } + end + + def render_edges + @edges.map { |from, to| " #{node_name(from)} --> #{node_name(to)}" } + end + + def render_clicks + @nodes.map { " click #{node_name(it)} \"#{node_link(it)}\"" } + end + + def introspect(obj) + associations_list = traversals[obj.class.name.underscore.to_sym] + return if associations_list.blank? + + return if @inspected.include?(obj) + @inspected << obj + + associations_list.each do + get_associated_objects(obj, it).each do + @nodes << it + @edges << order_nodes(obj, it) + + if @nodes.length > @node_limit + raise "Recursion limit of #{@node_limit} nodes has been exceeded. Try restricting the graph." + end + + introspect(it) + end + end + end + + def get_associated_objects(obj, association_name) + obj + .send(association_name) + .then { |associated_objects| load_association(associated_objects) } + end + + def load_association(associated_objects) + Array( + if associated_objects.is_a?(ActiveRecord::Relation) + associated_objects.strict_loading!(false) + else + associated_objects + end + ) + end + + def order_nodes(*nodes) + nodes.sort_by do |node| + @node_order.index(node.class.name.underscore) || Float::INFINITY + end + end + + def node_link(obj) + "/inspect/graph/#{obj.class.name.underscore.pluralize}/#{obj.id}" + end + + def node_name(obj) + klass = obj.class.name.underscore + "#{klass}-#{obj.id}" + end + + def class_text_for_obj(obj) + obj.class.name.underscore + (obj.in?(@focus) ? "_focused" : "") + end + + def node_display_name(obj) + klass = obj.class.name.underscore.humanize + "#{klass} #{obj.id}" + end + + def non_breaking_text(text) + # Insert non-breaking spaces and hyphens to prevent Mermaid from breaking the line + text.gsub(" ", " ").gsub("-", "#8209;") + end + + def escape_special_chars(text) + text.to_s.gsub("@", "\\@") + end + + def node_text(obj) + text = "\"#{node_display_name(obj)}" + + unless @clickable + command = "#{obj.class.to_s.classify}.find(#{obj.id})" + text += + "
    #{non_breaking_text(command)}" + command = + "puts GraphRecords.new.graph(#{obj.class.name.underscore}: #{obj.id})" + text += + "
    #{non_breaking_text(command)}" + end + + if DETAIL_WHITELIST.key?(obj.class.name.underscore.to_sym) + DETAIL_WHITELIST[obj.class.name.underscore.to_sym].each do |detail| + value = obj.send(detail) + name = detail.to_s + detail_text = "#{name}: #{escape_special_chars(value)}" + text += + "
    #{non_breaking_text(detail_text)}" + end + end + + "#{text}\"" + end + + def node_with_class(obj) + "#{node_name(obj)}[#{node_text(obj)}]:::#{class_text_for_obj(obj)}" + end +end From 5f27ba764c77fbb2cd6f9661506033a62c826551 Mon Sep 17 00:00:00 2001 From: Alistair White-Horne Date: Tue, 15 Apr 2025 09:36:49 +0100 Subject: [PATCH 07/27] Update functional test --- spec/lib/graph_records_spec.rb | 136 +++++++++++++++++++++++++++++---- 1 file changed, 122 insertions(+), 14 deletions(-) diff --git a/spec/lib/graph_records_spec.rb b/spec/lib/graph_records_spec.rb index d01887684e..168262b9d4 100644 --- a/spec/lib/graph_records_spec.rb +++ b/spec/lib/graph_records_spec.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true describe GraphRecords do - subject(:graph) { described_class.new.graph(patients: [patient]) } + subject(:graph) { described_class.new.graph(patient: patient.id) } + + around { |example| travel_to(Time.zone.local(2024, 2, 1)) { example.run } } let!(:programmes) { [create(:programme, :hpv)] } let!(:team) { create(:team, programmes:) } @@ -37,28 +39,134 @@ ) end + def non_breaking_text(text) + # Insert non-breaking spaces and hyphens to prevent Mermaid from breaking the line + text.gsub(" ", " ").gsub("-", "#8209;") + end + it { should start_with "flowchart TB" } it "generates the graph" do + # stree-ignore + session_details = + [ + "slug: #{session.slug}", + "clinic?: #{session.clinic?}", + "academic_year: #{session.academic_year}" + ].map { + "
    #{non_breaking_text(it)}" + } + .join + + consent_details = + [ + non_breaking_text("response: #{consent.response}"), + non_breaking_text("route: #{consent.route}"), + non_breaking_text("created_at: 2024-02-01 00:00:00 +0000"), + non_breaking_text("updated_at: 2024-02-01 00:00:00 +0000"), + non_breaking_text("withdrawn_at: "), + non_breaking_text("invalidated_at: ") + ].map { |d| "
    #{d}" }.join + + cohort_import_details = + [ + non_breaking_text("csv_filename: #{cohort_import.csv_filename}"), + non_breaking_text("processed_at: "), + non_breaking_text("status: #{cohort_import.status}"), + non_breaking_text("rows_count: #{cohort_import.rows_count}"), + non_breaking_text("new_record_count: "), + non_breaking_text("exact_duplicate_record_count: "), + non_breaking_text("changed_record_count: ") + ].map { |d| "
    #{d}" }.join + + class_import_details = + [ + non_breaking_text("csv_filename: #{class_import.csv_filename}"), + non_breaking_text("processed_at: "), + non_breaking_text("status: #{class_import.status}"), + non_breaking_text("rows_count: #{class_import.rows_count}"), + non_breaking_text("new_record_count: "), + non_breaking_text("exact_duplicate_record_count: "), + non_breaking_text("changed_record_count: "), + non_breaking_text("year_groups: #{class_import.year_groups}") + ].map { |d| "
    #{d}" }.join + + location_details = + [ + non_breaking_text("name: #{session.location.name}"), + non_breaking_text("type: #{session.location.type}"), + non_breaking_text("year_groups: #{session.location.year_groups}") + ].map { |d| "
    #{d}" }.join + expect(graph).to contain_exactly( "flowchart TB", - " classDef patient_focused fill:#c2e598,stroke:#000,stroke-width:3px", - " classDef parent fill:#faa0a0", - " classDef consent fill:#fffaa0", - " classDef class_import fill:#7fd7df", - " classDef cohort_import fill:#a2d2ff", - " patient-#{patient.id}:::patient_focused", - " parent-#{parent.id}:::parent", - " consent-#{consent.id}:::consent", - " class_import-#{class_import.id}:::class_import", - " cohort_import-#{cohort_import.id}:::cohort_import", + " classDef patient_focused fill:#469990,color:white,stroke:#000,stroke-width:3px", + " classDef parent fill:#e6194B,color:white,stroke:#000", + " classDef consent fill:#aaffc3,color:black,stroke:#000", + " classDef cohort_import fill:#4363d8,color:white,stroke:#000", + " classDef class_import fill:#000075,color:white,stroke:#000", + " classDef patient_session fill:#e6194B,color:white,stroke:#000", + " classDef session fill:#fabed4,color:black,stroke:#000", + " classDef location fill:#3cb44b,color:white,stroke:#000", + " patient-#{patient.id}[\"Patient #{patient.id}
    Patient.find(" \ + "#{patient.id})
    puts GraphRecords.new.graph(patient:" \ + " #{patient.id})
    updated_from_pds_at: 
    " \ + "date_of_death_recorded_at: 
    " \ + "restricted_at: 
    invalidated_at: \"]:::" \ + "patient_focused", + " parent-#{parent.id}[\"Parent #{parent.id}
    Parent.find(#{parent.id})" \ + "
    puts GraphRecords.new.graph(parent: #{parent.id})" \ + "\"]:::parent", + " consent-#{consent.id}[\"Consent #{consent.id}
    Consent.find(" \ + "#{consent.id})
    puts GraphRecords.new.graph(consent:" \ + " #{consent.id})#{consent_details}\"]:::consent", + " cohort_import-#{cohort_import.id}[\"Cohort import #{cohort_import.id}
    " \ + "CohortImport.find(#{cohort_import.id})
    puts " \ + "GraphRecords.new.graph(cohort_import: #{cohort_import.id})#{cohort_import_details}\"]:::" \ + "cohort_import", + " class_import-#{class_import.id}[\"Class import #{class_import.id}
    " \ + "ClassImport.find(#{class_import.id})
    puts GraphRecords." \ + "new.graph(class_import: #{class_import.id})#{class_import_details}\"]:::class_import", + " patient_session-#{patient.patient_sessions.first.id}[\"Patient session #{patient.patient_sessions.first.id}" \ + "
    PatientSession.find(#{patient.patient_sessions.first.id})" \ + "
    puts GraphRecords.new.graph(patient_session: " \ + "#{patient.patient_sessions.first.id})\"]:::patient_session", + " session-#{session.id}[\"Session #{session.id}
    Session.find(" \ + "#{session.id})
    puts GraphRecords.new.graph(session:" \ + " #{session.id})#{session_details}\"]:::session", + " location-#{session.location.id}[\"Location #{session.location.id}
    " \ + "Location.find(#{session.location.id})
    puts GraphRecords" \ + ".new.graph(location: #{session.location.id})#{location_details}\"]:::location", " patient-#{patient.id} --> parent-#{parent.id}", " consent-#{consent.id} --> parent-#{parent.id}", - " class_import-#{class_import.id} --> parent-#{parent.id}", - " cohort_import-#{cohort_import.id} --> parent-#{parent.id}", " patient-#{patient.id} --> consent-#{consent.id}", + " cohort_import-#{cohort_import.id} --> parent-#{parent.id}", + " class_import-#{class_import.id} --> parent-#{parent.id}", + " cohort_import-#{cohort_import.id} --> patient-#{patient.id}", " class_import-#{class_import.id} --> patient-#{patient.id}", - " cohort_import-#{cohort_import.id} --> patient-#{patient.id}" + " patient_session-#{patient.patient_sessions.first.id} --> patient-#{patient.id}", + " session-#{session.id} --> patient_session-#{patient.patient_sessions.first.id}", + " location-#{session.location.id} --> session-#{session.id}", + " location-#{session.location.id} --> patient-#{patient.id}" ) end + + context "when node limit is exceeded" do + subject(:graph_exceeded) do + described_class.new( + node_limit: 1 # A very low limit to trigger recursion limit early + ).graph(patients: [patient]) + end + + it "returns a fallback Mermaid diagram with the error message in a red box" do + error_message = + "Recursion limit of 1 nodes has been exceeded. Try restricting the graph." + expect(graph_exceeded).to include("flowchart TB") + # Assuming the error node is named `error` we check its content. + expect(graph_exceeded.join).to include("error[#{error_message}]") + expect(graph_exceeded.join).to include( + "style error fill:#f88,stroke:#f00,stroke-width:2px" + ) + end + end end From c27461f0c0bdebf166ef22d7b4fbafe40992b525 Mon Sep 17 00:00:00 2001 From: Alistair White-Horne Date: Tue, 15 Apr 2025 09:35:44 +0100 Subject: [PATCH 08/27] Add controller, route and view for the frontend --- app/controllers/inspect/graphs_controller.rb | 101 +++++++++++++++++++ app/views/inspect/graphs/show.html.erb | 62 ++++++++++++ lib/graph_records.rb | 9 +- 3 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 app/controllers/inspect/graphs_controller.rb create mode 100644 app/views/inspect/graphs/show.html.erb diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb new file mode 100644 index 0000000000..e67be7ad50 --- /dev/null +++ b/app/controllers/inspect/graphs_controller.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +module Inspect + class GraphsController < ApplicationController + skip_after_action :verify_policy_scoped + skip_before_action :authenticate_user! + + layout "full" + + SHOW_PII = false + + def show + @primary_type = safe_get_primary_type + if @primary_type.nil? + render plain: + "You don't have permission to view object type: #{params[:object_type].to_s.downcase.singularize}", + status: :bad_request and return + end + @primary_id = params[:object_id] + + # Set default relationships when loading a page for the first time + if params[:relationships].blank? && + GraphRecords::DEFAULT_TRAVERSALS.key?(@primary_type) + default_rels = GraphRecords::DEFAULT_TRAVERSALS[@primary_type] || {} + + new_params = params.to_unsafe_h.merge("relationships" => default_rels) + redirect_to inspect_path(new_params) and return + end + + @object = @primary_type.to_s.classify.constantize.find(@primary_id) + + @traversals_config = build_traversals_config + @graph_params = build_graph_params + + @mermaid = + GraphRecords + .new( + traversals_config: @traversals_config, + primary_type: @primary_type, + clickable: true, + show_pii: SHOW_PII + ) + .graph(**@graph_params) + .join("\n") + end + + private + + def build_traversals_config + traversals_config = {} + to_process = Set.new([@primary_type]) + processed = Set.new + + # Process types until we've explored all connected relationships + while (type = to_process.first) + to_process.delete(type) + processed.add(type) + + selected_rels = + Array(params.dig(:relationships, type)).reject(&:blank?).map(&:to_sym) + + traversals_config[type] = selected_rels + + # Add target types to process queue + klass = type.to_s.classify.constantize + selected_rels.each do |rel| + association = klass.reflect_on_association(rel) + next unless association + + target_type = association.klass.name.underscore.to_sym + to_process.add(target_type) unless processed.include?(target_type) + end + end + + traversals_config + end + + def build_graph_params + graph_params = { @primary_type => [@object.id] } + + if params[:additional_ids].present? + params[:additional_ids].each do |type, ids_string| + next if ids_string.blank? + additional_ids = ids_string.split(",").map { |s| s.strip.to_i } + next unless additional_ids.any? + type_sym = type.to_sym + graph_params[type_sym] ||= [] + graph_params[type_sym].concat(additional_ids) + end + end + + graph_params + end + + def safe_get_primary_type + singular_type = params[:object_type].downcase.singularize + return nil unless GraphRecords::ALLOWED_TYPES.include?(singular_type) + singular_type.to_sym + end + end +end diff --git a/app/views/inspect/graphs/show.html.erb b/app/views/inspect/graphs/show.html.erb new file mode 100644 index 0000000000..dd3242220a --- /dev/null +++ b/app/views/inspect/graphs/show.html.erb @@ -0,0 +1,62 @@ +
    "> + <%= h1 "Inspect #{@primary_type.to_s.classify} #{@primary_id}", size: "xl", style: "margin-bottom: 10px;" %> + + + <%= "#{@primary_type.to_s.classify}.find(#{@primary_id})" %> + +
    + +<% if @primary_type == :patient %> + <%= link_to "View timeline", inspect_timeline_patient_path(@primary_id), class: "nhsuk-button nhsuk-button--secondary" %> +<% end %> + +
    +  <%= @mermaid.html_safe %>
+
    + + +<%= render AppDetailsComponent.new(summary: "Graph options", expander: true, open: false) do %> + <%= form_with url: inspect_path(object_type: @primary_type.to_s.pluralize, object_id: @primary_id), method: :get, local: true do |form| %> +
    + <%= form.govuk_check_box :show_pii, 1, label: { text: "Show PII (Personally Identifiable Information)" }, checked: @show_pii, onchange: "this.form.submit();" %> +
    + + <% @traversals_config.keys.sort_by { |r| r.name.to_s.humanize }.each do |type| %> + <%= render AppDetailsComponent.new(summary: "#{type.to_s.humanize}", expander: true, open: false) do %> +
    + <%= form.label "additional_ids[#{type}]", safe_join(["Select relationships to include for ", tag.span(type, class: "nhsuk-u-font-weight-bold")]), class: "nhsuk-label" %> + <%= form.govuk_check_boxes_fieldset "relationships[#{type}]", legend: { hidden: true } do %> + <% type.to_s.classify.constantize.reflect_on_all_associations.sort_by { |r| + [ + params.dig(:relationships, type)&.include?(r.name.to_s) ? 0 : 1, + r.name.to_s.humanize, + ] + }.each do |r| %> + <%= form.govuk_check_box "relationships[#{type}]", r.name, label: { text: r.name.to_s.humanize }, checked: params.dig(:relationships, type)&.include?(r.name.to_s) %> + <% end %> + <% end %> + +
    + + <%= form.label "additional_ids[#{type}]", safe_join(["Additional ", tag.span(type, class: "nhsuk-u-font-weight-bold"), " IDs (comma separated)"]), class: "nhsuk-label" %> + <%= form.search_field "additional_ids[#{type}]", value: params.dig(:additional_ids, type), class: "nhsuk-input" %> +
    + <% end %> + <% end %> + + <%= form.govuk_submit "Update graph", name: nil %> + <% end %> +<% end %> + +<%= render AppDetailsComponent.new(summary: "Raw mermaid", expander: true, open: false) do %> + 
    <%= @mermaid %>
    +<% end %> diff --git a/lib/graph_records.rb b/lib/graph_records.rb index 6947695975..598bade44e 100644 --- a/lib/graph_records.rb +++ b/lib/graph_records.rb @@ -9,7 +9,6 @@ # puts graph # class GraphRecords - # TODO: put all these constants elsewhere? BOX_STYLES = %w[ fill:#e6194B,color:white fill:#3cb44b,color:white @@ -324,7 +323,8 @@ def initialize( traversals_config: {}, node_limit: 1000, primary_type: nil, - clickable: false + clickable: false, + show_pii: false ) @focus_config = focus_config @node_order = node_order @@ -332,6 +332,7 @@ def initialize( @node_limit = node_limit @primary_type = primary_type @clickable = clickable + @detail_whitelist = show_pii ? DETAIL_WHITELIST_WITH_PII : DETAIL_WHITELIST end # @param objects [Hash] Hash of model name to ids to be graphed @@ -498,8 +499,8 @@ def node_text(obj) "
    #{non_breaking_text(command)}" end - if DETAIL_WHITELIST.key?(obj.class.name.underscore.to_sym) - DETAIL_WHITELIST[obj.class.name.underscore.to_sym].each do |detail| + if @detail_whitelist.key?(obj.class.name.underscore.to_sym) + @detail_whitelist[obj.class.name.underscore.to_sym].each do |detail| value = obj.send(detail) name = detail.to_s detail_text = "#{name}: #{escape_special_chars(value)}" From f3535b9341ceef32cf52ba866642c85dbc9967aa Mon Sep 17 00:00:00 2001 From: Alistair White-Horne Date: Tue, 15 Apr 2025 09:37:24 +0100 Subject: [PATCH 09/27] Create feature test This checks that the correct endpoints exist/are hidden in certain environments --- spec/features/graph_records_spec.rb | 45 +++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 spec/features/graph_records_spec.rb diff --git a/spec/features/graph_records_spec.rb b/spec/features/graph_records_spec.rb new file mode 100644 index 0000000000..21db54bd06 --- /dev/null +++ b/spec/features/graph_records_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +describe "GraphRecord UI" do + scenario "Endpoint doesn't exist in prod" do + given_i_am_in_production + + when_i_visit_the_graph_endpoint + then_i_should_see_a_404_error + end + + scenario "Endpoint is rendered in test" do + given_i_am_in_test + given_a_patient_exists + + when_i_visit_the_graph_endpoint + then_i_should_see_the_page_rendered + end + + def given_i_am_in_production + allow(Rails).to receive(:env).and_return("production".inquiry) + end + + def given_i_am_in_test + # Already implicitly in test + end + + def given_a_patient_exists + create(:patient, id: 1) + end + + def when_i_visit_the_graph_endpoint + visit "/inspect/graph/patients/1" + end + + def then_i_should_see_a_404_error + expect(page.html).to include( + "If you entered a web address, check it is correct." + ) + end + + def then_i_should_see_the_page_rendered + expect(page.status_code).to eq(200) + expect(page.html).to include("Inspect Patient 1") + end +end From be8208cc183e1297c335906feca644c4e1c98fc0 Mon Sep 17 00:00:00 2001 From: Alistair White-Horne Date: Thu, 29 May 2025 08:58:49 +0100 Subject: [PATCH 10/27] Add toggle for `show_pii` mode in the UI --- app/controllers/inspect/graphs_controller.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb index e67be7ad50..d1880f7813 100644 --- a/app/controllers/inspect/graphs_controller.rb +++ b/app/controllers/inspect/graphs_controller.rb @@ -7,8 +7,6 @@ class GraphsController < ApplicationController layout "full" - SHOW_PII = false - def show @primary_type = safe_get_primary_type if @primary_type.nil? @@ -31,6 +29,7 @@ def show @traversals_config = build_traversals_config @graph_params = build_graph_params + @show_pii = params[:show_pii]&.first == "1" @mermaid = GraphRecords @@ -38,7 +37,7 @@ def show traversals_config: @traversals_config, primary_type: @primary_type, clickable: true, - show_pii: SHOW_PII + show_pii: @show_pii ) .graph(**@graph_params) .join("\n") From ba217da13b5b6937d17b344f4f48edd40a3eb83f Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Thu, 14 Aug 2025 14:22:04 +0100 Subject: [PATCH 11/27] Adjust display fields for new data structure The data structure has changed notably since this code was initially written. This now matches the new data structure. --- lib/graph_records.rb | 110 +++++++-------------------------- spec/lib/graph_records_spec.rb | 11 ++-- 2 files changed, 25 insertions(+), 96 deletions(-) diff --git a/lib/graph_records.rb b/lib/graph_records.rb index 598bade44e..c963a17ccb 100644 --- a/lib/graph_records.rb +++ b/lib/graph_records.rb @@ -43,7 +43,6 @@ class GraphRecords session_date class_import cohort_import - patient_session session_attendance gillick_assessment patient @@ -68,14 +67,13 @@ class GraphRecords cohort_imports class_imports vaccination_records - patient_sessions triages school + sessions ], parent: %i[patients consents cohort_imports class_imports], consent: %i[consent_form patient parent], session: %i[location], - patient_session: %i[session session_attendances gillick_assessments], vaccination_record: %i[session] }, parent: { @@ -112,23 +110,12 @@ class GraphRecords session: %i[location programmes session_dates] }, session_attendance: { - session_attendance: %i[patient_session session_date], - patient_session: %i[session gillick_assessments patient], + session_attendance: %i[session_date], session: %i[location], session_date: %i[session] }, - patient_session: { - patient_session: %i[ - session - patient - session_attendances - gillick_assessments - ], - session: %i[location] - }, gillick_assessment: { - gillick_assessment: %i[patient_session performed_by programme], - patient_session: %i[session patient], + gillick_assessment: %i[performed_by programme], session: %i[location] }, triage: { @@ -190,7 +177,8 @@ class GraphRecords vaccine: %i[nivs_name], organisation: %i[ods_code], team: %i[name workgroup], - location: %i[name type year_groups], + subteam: %i[name], + location: %i[name address_postcode type year_groups], cohort_import: %i[ csv_filename processed_at @@ -218,59 +206,21 @@ class GraphRecords invalidated_at ], parent: %i[], - patient_session: %i[], - gillick_assessment: %i[created_at], + gillick_assessment: %i[ + knows_vaccination + knows_disease + knows_consequences + knows_delivery + knows_side_effects + created_at + ], batch: %i[name expiry archived_at], user: %i[fallback_role uid], consent_form: %i[response recorded_at archived_at], parent_relationship: %i[type] }.freeze - DETAIL_WHITELIST_WITH_PII = { - consent: %i[ - response - route - created_at - updated_at - withdrawn_at - invalidated_at - ], - session: %i[slug clinic? academic_year], - session_attendance: %i[attending created_at updated_at], - triage: %i[status created_at updated_at invalidated_at], - vaccination_record: %i[ - outcome - performed_at - created_at - updated_at - discarded_at - uuid - ], - programme: %i[type], - vaccine: %i[nivs_name], - organisation: %i[name ods_code], - location: %i[name address_postcode type year_groups], - cohort_import: %i[ - csv_filename - processed_at - status - rows_count - new_record_count - exact_duplicate_record_count - changed_record_count - ], - class_import: %i[ - csv_filename - processed_at - status - rows_count - new_record_count - exact_duplicate_record_count - changed_record_count - year_groups - ], - session_date: %i[value], - team: %i[name], + EXTRA_DETAIL_WHITELIST_WITH_PII = { patient: %i[ nhs_number given_name @@ -281,38 +231,20 @@ class GraphRecords address_town address_postcode home_educated - updated_from_pds_at - restricted_at date_of_death - date_of_death_recorded_at - updated_from_pds_at - invalidated_at pending_changes ], parent: %i[full_name email phone], - patient_session: %i[], - gillick_assessment: %i[ - knows_vaccination - knows_disease - knows_consequences - knows_delivery - knows_side_effects - created_at - ], - batch: %i[name expiry archived_at], user: %i[given_name family_name email fallback_role uid], - consent_form: %i[ - response - recorded_at - archived_at - given_name - family_name - address_postcode - date_of_birth - ], - parent_relationship: %i[type other_name] + consent_form: %i[given_name family_name address_postcode date_of_birth], + parent_relationship: %i[other_name] }.freeze + DETAIL_WHITELIST_WITH_PII = + DETAIL_WHITELIST.merge( + EXTRA_DETAIL_WHITELIST_WITH_PII + ) { |_, base_fields, pii_fields| (base_fields + pii_fields).uniq } + # @param focus_config [Hash] Hash of model names to ids to focus on (make bold) # @param node_order [Array] Array of model names in order to render nodes # @param traversals_config [Hash] Hash of model names to arrays of associations to traverse diff --git a/spec/lib/graph_records_spec.rb b/spec/lib/graph_records_spec.rb index 168262b9d4..2eaf7e563a 100644 --- a/spec/lib/graph_records_spec.rb +++ b/spec/lib/graph_records_spec.rb @@ -94,6 +94,9 @@ def non_breaking_text(text) location_details = [ non_breaking_text("name: #{session.location.name}"), + non_breaking_text( + "address_postcode: #{session.location.address_postcode}" + ), non_breaking_text("type: #{session.location.type}"), non_breaking_text("year_groups: #{session.location.year_groups}") ].map { |d| "
    #{d}" }.join @@ -105,7 +108,6 @@ def non_breaking_text(text) " classDef consent fill:#aaffc3,color:black,stroke:#000", " classDef cohort_import fill:#4363d8,color:white,stroke:#000", " classDef class_import fill:#000075,color:white,stroke:#000", - " classDef patient_session fill:#e6194B,color:white,stroke:#000", " classDef session fill:#fabed4,color:black,stroke:#000", " classDef location fill:#3cb44b,color:white,stroke:#000", " patient-#{patient.id}[\"Patient #{patient.id}
    Patient.find(" \ @@ -127,10 +129,6 @@ def non_breaking_text(text) " class_import-#{class_import.id}[\"Class import #{class_import.id}
    " \ "ClassImport.find(#{class_import.id})
    puts GraphRecords." \ "new.graph(class_import: #{class_import.id})#{class_import_details}\"]:::class_import", - " patient_session-#{patient.patient_sessions.first.id}[\"Patient session #{patient.patient_sessions.first.id}" \ - "
    PatientSession.find(#{patient.patient_sessions.first.id})" \ - "
    puts GraphRecords.new.graph(patient_session: " \ - "#{patient.patient_sessions.first.id})\"]:::patient_session", " session-#{session.id}[\"Session #{session.id}
    Session.find(" \ "#{session.id})
    puts GraphRecords.new.graph(session:" \ " #{session.id})#{session_details}\"]:::session", @@ -138,14 +136,13 @@ def non_breaking_text(text) "Location.find(#{session.location.id})
    puts GraphRecords" \ ".new.graph(location: #{session.location.id})#{location_details}\"]:::location", " patient-#{patient.id} --> parent-#{parent.id}", + " session-#{session.id} --> patient-#{patient.id}", " consent-#{consent.id} --> parent-#{parent.id}", " patient-#{patient.id} --> consent-#{consent.id}", " cohort_import-#{cohort_import.id} --> parent-#{parent.id}", " class_import-#{class_import.id} --> parent-#{parent.id}", " cohort_import-#{cohort_import.id} --> patient-#{patient.id}", " class_import-#{class_import.id} --> patient-#{patient.id}", - " patient_session-#{patient.patient_sessions.first.id} --> patient-#{patient.id}", - " session-#{session.id} --> patient_session-#{patient.patient_sessions.first.id}", " location-#{session.location.id} --> session-#{session.id}", " location-#{session.location.id} --> patient-#{patient.id}" ) From f2488b11c9e98718278aa8500d585a2318014587 Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Tue, 29 Jul 2025 13:50:26 +0100 Subject: [PATCH 12/27] Add support user type to User model Add a support fallback role to the User model to enable controlled access to ops tools. --- app/models/cis2_info.rb | 15 +++++++++ app/models/user.rb | 11 +++++-- db/seeds.rb | 15 +++++++-- spec/factories/users.rb | 17 +++++++++- spec/models/user_spec.rb | 68 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 121 insertions(+), 5 deletions(-) diff --git a/app/models/cis2_info.rb b/app/models/cis2_info.rb index d01b2f2eaa..8adb1f64d6 100644 --- a/app/models/cis2_info.rb +++ b/app/models/cis2_info.rb @@ -5,11 +5,16 @@ class CIS2Info NURSE_ROLE = "S8000:G8000:R8001" MEDICAL_SECRETARY_ROLE = "S8000:G8001:R8006" + SUPPORT_ROLE = "S8001:G8005:R8015" + + SUPPORT_WORKGROUP = "mavissupport" + SUPPORT_ORGANISATION = "X26" ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE = "B1611" INDEPENDENT_PRESCRIBING_ACTIVITY_CODE = "B0420" LOCAL_SYSTEM_ADMINISTRATION_ACTIVITY_CODE = "B0062" PERSONAL_MEDICATION_ADMINISTRATION_ACTIVITY_CODE = "B0428" + VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE = "B0360" attribute :organisation_name attribute :organisation_code @@ -69,6 +74,16 @@ def can_perform_local_system_administration? activity_codes.include?(LOCAL_SYSTEM_ADMINISTRATION_ACTIVITY_CODE) end + def can_view_detailed_health_records? + activity_codes.include?(VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE) + end + + def is_support? + workgroups&.include?(SUPPORT_WORKGROUP) && role_code == SUPPORT_ROLE && + organisation_code == SUPPORT_ORGANISATION && + can_access_sensitive_flagged_records? && can_view_detailed_health_records? + end + private def request_session_key = "cis2_info" diff --git a/app/models/user.rb b/app/models/user.rb index 7f56214e51..471377d800 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -80,7 +80,8 @@ class User < ApplicationRecord medical_secretary: 1, superuser: 2, healthcare_assistant: 3, - prescriber: 4 + prescriber: 4, + support: 5 }, prefix: true, validate: { @@ -132,8 +133,10 @@ def role_description "Prescriber" elsif is_nurse? "Nurse" - else + elsif is_medical_secretary? "Medical secretary" + else + "Support" end is_superuser? ? "#{role} (Superuser)" : role @@ -159,6 +162,10 @@ def is_healthcare_assistant? end end + def is_support? + cis2_enabled? ? cis2_info.is_support? : fallback_role_support? + end + def is_prescriber? cis2_enabled? ? cis2_info.is_prescriber? : fallback_role_prescriber? end diff --git a/db/seeds.rb b/db/seeds.rb index 2bc6afb545..df82d1aeb1 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -19,8 +19,8 @@ def create_gp_practices FactoryBot.create_list(:gp_practice, 30) end -def create_team(ods_code:) - workgroup = ods_code.downcase +def create_team(ods_code:, workgroup: nil) + workgroup ||= ods_code.downcase Team.find_by(workgroup:) || FactoryBot.create( @@ -303,6 +303,17 @@ def create_team_sessions(user, team) team = create_team(ods_code: "A9A5A") user = create_user(:nurse, team:, uid: "555057896106") +support_team = + create_team( + ods_code: CIS2Info::SUPPORT_ORGANISATION, + workgroup: CIS2Info::SUPPORT_WORKGROUP + ) +create_user( + team: support_team, + email: "support@example.com", + fallback_role: "support" +) + attach_sample_of_schools_to(team) Audited.audit_class.as_user(user) { create_team_sessions(user, team) } diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 0d910ddf0d..d93024db93 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -44,6 +44,7 @@ team { Team.includes(:organisation).first || create(:team) } role_code { CIS2Info::NURSE_ROLE } + role_workgroups { [] } activity_codes { [] } cis2_info_hash do @@ -53,7 +54,7 @@ "role_code" => role_code, "activity_codes" => activity_codes, "team_workgroup" => team.workgroup, - "workgroups" => [team.workgroup] + "workgroups" => role_workgroups + [team.workgroup] } end end @@ -116,6 +117,19 @@ show_in_suppliers { false } end + trait :support do + role_code { CIS2Info::SUPPORT_ROLE } + sequence(:email) { |n| "support-#{n}@example.com" } + role_workgroups { [CIS2Info::SUPPORT_WORKGROUP] } + fallback_role { :support } + activity_codes do + [ + CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE, + CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE + ] + end + end + trait :signed_in do current_sign_in_at { Time.current } current_sign_in_ip { "127.0.0.1" } @@ -126,4 +140,5 @@ factory :medical_secretary, parent: :user, traits: %i[medical_secretary] factory :prescriber, parent: :user, traits: %i[prescriber] factory :superuser, parent: :user, traits: %i[superuser] + factory :support, parent: :user, traits: %i[support] end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d5300e2072..5363829519 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -295,4 +295,72 @@ end end end + + describe "#is_support?" do + subject(:is_support?) { user.is_support? } + + context "cis2 is enabled", cis2: :enabled do + context "when the user is support" do + let(:user) { build(:support) } + + it { should be true } + end + + context "when the user is a nurse" do + let(:user) { build(:nurse) } + + it { should be false } + end + + context "when the user is a nurse and superuser" do + let(:user) { build(:nurse, :superuser) } + + it { should be false } + end + + context "when the user is an admin" do + let(:user) { build(:admin) } + + it { should be false } + end + + context "when the user is an admin and superuser" do + let(:user) { build(:admin, :superuser) } + + it { should be false } + end + end + + context "cis2 is disabled", cis2: :disabled do + context "when the user is support" do + let(:user) { build(:support) } + + it { should be true } + end + + context "when the user is an admin" do + let(:user) { build(:admin) } + + it { should be false } + end + + context "when the user is an admin and superuser" do + let(:user) { build(:admin, :superuser) } + + it { should be false } + end + + context "when the user is a nurse" do + let(:user) { build(:nurse) } + + it { should be false } + end + + context "when the user is a nurse and superuser" do + let(:user) { build(:nurse, :superuser) } + + it { should be false } + end + end + end end From 1791b40f41b1b66e48e415f67b6c53ca11478d82 Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Tue, 12 Aug 2025 22:13:40 +0100 Subject: [PATCH 13/27] Limit access to ops tools to support users only This prevents any user without a "support" role from visiting the inspect endpoint. Support users have to belong to certain NHSE ODS code in CIS2, an appropriate support workgroup and CIS2 roles, which gives them the appropriate access. Activity codes are also checked. Support users cannot access any of the other endpoints in the service. They will be redirected to an operational support dashboard in these cases. --- .../concerns/authentication_concern.rb | 17 ++- .../inspect/dashboard_controller.rb | 13 ++ app/controllers/inspect/graphs_controller.rb | 1 - .../inspect/timeline/patients_controller.rb | 1 - app/views/inspect/dashboard/index.html.erb | 20 +++ config/routes.rb | 1 + ...authentication_inspect_non_support_spec.rb | 82 +++++++++++++ ...is2_authentication_inspect_support_spec.rb | 115 ++++++++++++++++++ 8 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 app/controllers/inspect/dashboard_controller.rb create mode 100644 app/views/inspect/dashboard/index.html.erb create mode 100644 spec/features/user_cis2_authentication_inspect_non_support_spec.rb create mode 100644 spec/features/user_cis2_authentication_inspect_support_spec.rb diff --git a/app/controllers/concerns/authentication_concern.rb b/app/controllers/concerns/authentication_concern.rb index a012797938..34f7a113e7 100644 --- a/app/controllers/concerns/authentication_concern.rb +++ b/app/controllers/concerns/authentication_concern.rb @@ -32,6 +32,11 @@ def authenticate_user! redirect_to users_organisation_not_found_path elsif !selected_cis2_workgroup_is_valid? redirect_to users_workgroup_not_found_path + elsif path_is_support? && !user_is_support? + raise ActionController::RoutingError, "Not found" + elsif !path_is_support? && user_is_support? && + request.path != new_users_teams_path + redirect_to inspect_dashboard_path end end end @@ -51,7 +56,15 @@ def selected_cis2_workgroup_is_valid? def selected_cis2_role_is_valid? cis2_info.is_medical_secretary? || cis2_info.is_nurse? || cis2_info.is_healthcare_assistant? || cis2_info.is_superuser? || - cis2_info.is_prescriber? + cis2_info.is_prescriber? || cis2_info.is_support? + end + + def user_is_support? + cis2_info.is_support? + end + + def path_is_support? + request.path.start_with?("/inspect") end def storable_location? @@ -108,6 +121,8 @@ def authenticate_basic def after_sign_in_path_for(scope) urls = [] + + urls << inspect_dashboard_path if user_is_support? if Flipper.enabled?(:reporting_api) urls << reporting_app_redirect_uri_with_auth_code_for(current_user) end diff --git a/app/controllers/inspect/dashboard_controller.rb b/app/controllers/inspect/dashboard_controller.rb new file mode 100644 index 0000000000..6ea6881619 --- /dev/null +++ b/app/controllers/inspect/dashboard_controller.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Inspect + class DashboardController < ApplicationController + skip_after_action :verify_policy_scoped + + layout "full" + + def index + @sample_patient_id = Patient.first.id + end + end +end diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb index d1880f7813..d8b54987c2 100644 --- a/app/controllers/inspect/graphs_controller.rb +++ b/app/controllers/inspect/graphs_controller.rb @@ -3,7 +3,6 @@ module Inspect class GraphsController < ApplicationController skip_after_action :verify_policy_scoped - skip_before_action :authenticate_user! layout "full" diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index 0416ca672e..9ddb7a4e2b 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -2,7 +2,6 @@ class Inspect::Timeline::PatientsController < ApplicationController skip_after_action :verify_policy_scoped - skip_before_action :authenticate_user! before_action :set_patient layout "full" diff --git a/app/views/inspect/dashboard/index.html.erb b/app/views/inspect/dashboard/index.html.erb new file mode 100644 index 0000000000..450abd51a0 --- /dev/null +++ b/app/views/inspect/dashboard/index.html.erb @@ -0,0 +1,20 @@ +<%= h1 "Operational support tools", size: "xl" %> + +

    These tools are only accessible to authorised operational support staff. Use them to inspect and troubleshoot individual records in the system.

    + +
      +
    • + <%= render AppCardComponent.new(link_to: "/inspect/graph/patient/#{@sample_patient_id}") do |card| %> + <% card.with_heading { "Object graphing tool" } %> +

      Explore the data graph for any object type (e.g., sessions, consents, patients) to trace relationships and links.

      +

      URL format: /inspect/graph/<object_type>/<ID>

      + <% end %> +
      • +
    • + <%= render AppCardComponent.new(link_to: "/inspect/timeline/patients/#{@sample_patient_id}") do |card| %> + <% card.with_heading { "Patient timeline tool" } %> +

      Use this tool to view a chronological timeline of key events for a specific patient.

      +

      URL format: /inspect/timeline/patients/<ID>

      + <% end %> +
      • +
    \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index fb57f2aebc..1754f59a26 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -348,6 +348,7 @@ constraints -> { Rails.env.local? } do namespace :inspect do + get "dashboard", to: "dashboard#index" get "graph/:object_type/:object_id", to: "graphs#show" namespace :timeline do resources :patients, only: [:show] diff --git a/spec/features/user_cis2_authentication_inspect_non_support_spec.rb b/spec/features/user_cis2_authentication_inspect_non_support_spec.rb new file mode 100644 index 0000000000..a24ee50652 --- /dev/null +++ b/spec/features/user_cis2_authentication_inspect_non_support_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +describe "Inspect tools", :cis2 do + scenario "Non-support user can't view timeline" do + given_a_test_team_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_nurse + + and_i_go_to_the_timeline_url_for_the_patient + then_i_see_an_error + end + + scenario "Non-support user can't view graph" do + given_a_test_team_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_nurse + + and_i_go_to_the_graph_url_for_the_patient + then_i_see_an_error + end + + def given_a_test_team_is_setup_in_mavis_and_cis2 + @user = create(:user, uid: "123") + @team = create(:team, users: [@user]) + + mock_cis2_auth( + uid: "123", + given_name: "Nurse", + family_name: "User", + org_code: @team.organisation.ods_code, + org_name: @team.name, + workgroups: [@team.workgroup] + ) + end + + def given_an_hpv_programme_is_underway + @programme = create(:programme, :hpv, teams: [@team]) + @session = + create( + :session, + date: Date.yesterday, + team: @team, + programmes: [@programme] + ) + + @patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "John", + family_name: "Smith", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + end + + def when_i_login_as_a_nurse + visit "/start" + click_button "Care Identity" + expect(page).to have_content("USER, Nurse") + expect(page).to have_button("Log out") + end + + def and_i_go_to_the_timeline_url_for_the_patient + visit inspect_timeline_patient_path(id: @patient.id) + end + + def and_i_go_to_the_graph_url_for_the_patient + visit inspect_path(object_type: "patient", object_id: @patient.id) + end + + def then_i_see_an_error + expect(page).to have_content( + "If you entered a web address, check it is correct." + ) + end +end diff --git a/spec/features/user_cis2_authentication_inspect_support_spec.rb b/spec/features/user_cis2_authentication_inspect_support_spec.rb new file mode 100644 index 0000000000..5f3982eb1d --- /dev/null +++ b/spec/features/user_cis2_authentication_inspect_support_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +describe "Inspect tools", :cis2 do + scenario "Support user can view timeline" do + given_a_test_support_organisation_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user + then_i_see_the_inspect_dashboard + + when_i_go_to_the_timeline_url_for_the_patient + then_i_see_the_timeline + end + + scenario "Support user can view graph" do + given_a_test_support_organisation_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user + then_i_see_the_inspect_dashboard + + when_i_go_to_the_graph_url_for_the_patient + then_i_see_the_graph + end + + scenario "Support user can't view confidential pages" do + given_a_test_support_organisation_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user + + and_i_go_to_a_confidential_page + then_i_see_the_inspect_dashboard + end + + def given_a_test_support_organisation_is_setup_in_mavis_and_cis2 + @organisation_support = create(:organisation, ods_code: "X26") + @team_support = + create( + :team, + organisation: @organisation_support, + workgroup: CIS2Info::SUPPORT_WORKGROUP + ) + @user = create(:user, :support, team: @team_support) + + mock_cis2_auth( + uid: "123", + given_name: "Support", + family_name: "Test", + org_code: @organisation_support.ods_code, + workgroups: [CIS2Info::SUPPORT_WORKGROUP], + role_code: CIS2Info::SUPPORT_ROLE, + activity_codes: [ + CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE, + CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE + ] + ) + end + + def given_an_hpv_programme_is_underway + @team = create(:team, :with_one_nurse) + @programme = create(:programme, :hpv, teams: [@team]) + @session = + create( + :session, + date: Date.yesterday, + team: @team, + programmes: [@programme] + ) + + @patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "John", + family_name: "Smith", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + end + + def when_i_login_as_a_support_user + visit "/start" + click_button "Care Identity" + expect(page).to have_content("TEST, Support") + expect(page).to have_button("Log out") + end + + def when_i_go_to_the_timeline_url_for_the_patient + visit inspect_timeline_patient_path(id: @patient.id) + end + + def when_i_go_to_the_graph_url_for_the_patient + visit inspect_path(object_type: "patient", object_id: @patient.id) + end + + def and_i_go_to_a_confidential_page + visit patients_path + end + + def then_i_see_the_timeline + expect(page).to have_content("Customise timeline") + end + + def then_i_see_the_graph + expect(page).to have_content("Graph options") + end + + def then_i_see_the_inspect_dashboard + expect(page).to have_content("Operational support tools") + end +end From d938d22803a7107a22014b7c7f1c421457fdd7be Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Tue, 12 Aug 2025 22:21:38 +0100 Subject: [PATCH 14/27] Remove production constraint on ops endpoints With CIS2 support user logins andd authentication, we no longer need to constrain the rendering of ops pages to non-prod envs only. --- config/routes.rb | 12 +++---- spec/features/graph_records_spec.rb | 45 -------------------------- spec/features/timeline_records_spec.rb | 45 -------------------------- 3 files changed, 5 insertions(+), 97 deletions(-) delete mode 100644 spec/features/graph_records_spec.rb delete mode 100644 spec/features/timeline_records_spec.rb diff --git a/config/routes.rb b/config/routes.rb index 1754f59a26..576d5c66e9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -346,13 +346,11 @@ get "/oidc/jwks", to: "jwks#jwks" - constraints -> { Rails.env.local? } do - namespace :inspect do - get "dashboard", to: "dashboard#index" - get "graph/:object_type/:object_id", to: "graphs#show" - namespace :timeline do - resources :patients, only: [:show] - end + namespace :inspect do + get "dashboard", to: "dashboard#index" + get "graph/:object_type/:object_id", to: "graphs#show" + namespace :timeline do + resources :patients, only: [:show] end end end diff --git a/spec/features/graph_records_spec.rb b/spec/features/graph_records_spec.rb deleted file mode 100644 index 21db54bd06..0000000000 --- a/spec/features/graph_records_spec.rb +++ /dev/null @@ -1,45 +0,0 @@ -# frozen_string_literal: true - -describe "GraphRecord UI" do - scenario "Endpoint doesn't exist in prod" do - given_i_am_in_production - - when_i_visit_the_graph_endpoint - then_i_should_see_a_404_error - end - - scenario "Endpoint is rendered in test" do - given_i_am_in_test - given_a_patient_exists - - when_i_visit_the_graph_endpoint - then_i_should_see_the_page_rendered - end - - def given_i_am_in_production - allow(Rails).to receive(:env).and_return("production".inquiry) - end - - def given_i_am_in_test - # Already implicitly in test - end - - def given_a_patient_exists - create(:patient, id: 1) - end - - def when_i_visit_the_graph_endpoint - visit "/inspect/graph/patients/1" - end - - def then_i_should_see_a_404_error - expect(page.html).to include( - "If you entered a web address, check it is correct." - ) - end - - def then_i_should_see_the_page_rendered - expect(page.status_code).to eq(200) - expect(page.html).to include("Inspect Patient 1") - end -end diff --git a/spec/features/timeline_records_spec.rb b/spec/features/timeline_records_spec.rb deleted file mode 100644 index e323dfa35d..0000000000 --- a/spec/features/timeline_records_spec.rb +++ /dev/null @@ -1,45 +0,0 @@ -# frozen_string_literal: true - -describe "TimelineRecords UI" do - before do - create(:session, id: 1) - create(:patient, id: 1, session: Session.first) - end - - scenario "Endpoint doesn't exist in prod" do - given_i_am_in_production - - when_i_visit_the_timeline_endpoint - then_i_should_see_a_404_error - end - - scenario "Endpoint is rendered in test" do - given_i_am_in_test - - when_i_visit_the_timeline_endpoint - then_i_should_see_the_page_rendered - end - - def given_i_am_in_production - allow(Rails).to receive(:env).and_return("production".inquiry) - end - - def given_i_am_in_test - # Already implicitly in test - end - - def when_i_visit_the_timeline_endpoint - visit "/inspect/timeline/patients/1" - end - - def then_i_should_see_a_404_error - expect(page.html).to include( - "If you entered a web address, check it is correct." - ) - end - - def then_i_should_see_the_page_rendered - expect(page.status_code).to eq(200) - expect(page.html).to include("Inspect Patient 1") - end -end From c6124a634e9bd06d19a8a01b03587fab2205ba25 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Thu, 7 Aug 2025 14:20:56 +0100 Subject: [PATCH 15/27] Add function to authenticate ops users with PII access --- .../concerns/authentication_concern.rb | 12 +++++++-- app/models/cis2_info.rb | 26 ++++++++++++++++--- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/app/controllers/concerns/authentication_concern.rb b/app/controllers/concerns/authentication_concern.rb index 34f7a113e7..785c713f15 100644 --- a/app/controllers/concerns/authentication_concern.rb +++ b/app/controllers/concerns/authentication_concern.rb @@ -63,6 +63,14 @@ def user_is_support? cis2_info.is_support? end + def user_is_support_without_pii_access? + cis2_info.is_support_without_pii_access? + end + + def user_is_support_with_pii_access? + cis2_info.is_support_with_pii_access? + end + def path_is_support? request.path.start_with?("/inspect") end @@ -113,8 +121,8 @@ def authenticate_basic unless authenticated request_http_basic_authentication "Application", <<~MESSAGE - Access is currently restricted to authorised users only. - MESSAGE + Access is currently restricted to authorised users only. + MESSAGE end end end diff --git a/app/models/cis2_info.rb b/app/models/cis2_info.rb index 8adb1f64d6..cf989ee055 100644 --- a/app/models/cis2_info.rb +++ b/app/models/cis2_info.rb @@ -15,6 +15,7 @@ class CIS2Info LOCAL_SYSTEM_ADMINISTRATION_ACTIVITY_CODE = "B0062" PERSONAL_MEDICATION_ADMINISTRATION_ACTIVITY_CODE = "B0428" VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE = "B0360" + VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE = "B1570" attribute :organisation_name attribute :organisation_code @@ -78,13 +79,32 @@ def can_view_detailed_health_records? activity_codes.include?(VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE) end + def can_view_shared_non_patient_identifiable_information? + activity_codes.include?( + VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE + ) + end + def is_support? - workgroups&.include?(SUPPORT_WORKGROUP) && role_code == SUPPORT_ROLE && - organisation_code == SUPPORT_ORGANISATION && - can_access_sensitive_flagged_records? && can_view_detailed_health_records? + is_support_without_pii_access? || is_support_with_pii_access? + end + + def is_support_without_pii_access? + is_support_without_activities? && + can_view_shared_non_patient_identifiable_information? + end + + def is_support_with_pii_access? + is_support_without_activities? && can_access_sensitive_flagged_records? && + can_view_detailed_health_records? end private + def is_support_without_activities? + workgroups&.include?(SUPPORT_WORKGROUP) && role_code == SUPPORT_ROLE && + organisation_code == SUPPORT_ORGANISATION + end + def request_session_key = "cis2_info" end From f72801409fda96ee71fd236473de347e1711375f Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Thu, 7 Aug 2025 14:25:49 +0100 Subject: [PATCH 16/27] Restrict PII access to eligible ops users --- app/controllers/inspect/graphs_controller.rb | 7 ++++++- .../inspect/timeline/patients_controller.rb | 15 ++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb index d8b54987c2..b002e8c8df 100644 --- a/app/controllers/inspect/graphs_controller.rb +++ b/app/controllers/inspect/graphs_controller.rb @@ -28,7 +28,7 @@ def show @traversals_config = build_traversals_config @graph_params = build_graph_params - @show_pii = params[:show_pii]&.first == "1" + @show_pii = get_show_pii @mermaid = GraphRecords @@ -44,6 +44,11 @@ def show private + def get_show_pii + return false unless user_is_support_with_pii_access? + params[:show_pii]&.first == "1" + end + def build_traversals_config traversals_config = {} to_process = Set.new([@primary_type]) diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index 9ddb7a4e2b..24ea7985fe 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -6,7 +6,7 @@ class Inspect::Timeline::PatientsController < ApplicationController layout "full" - SHOW_PII = false + SHOW_PII_BY_DEFAULT = false DEFAULT_EVENT_NAMES = %w[ consents @@ -21,7 +21,7 @@ class Inspect::Timeline::PatientsController < ApplicationController ].freeze def show - @show_pii = params[:show_pii] || SHOW_PII + @show_pii = get_show_pii params.reverse_merge!(event_names: DEFAULT_EVENT_NAMES) params[:audit_config] ||= {} @@ -70,6 +70,11 @@ def show private + def get_show_pii + return false unless user_is_support_with_pii_access? + params[:show_pii] || SHOW_PII_BY_DEFAULT + end + def set_patient @patient = Patient.find(params[:id]) timeline = TimelineRecords.new(@patient) @@ -100,9 +105,9 @@ def sample_patient(compare_option) end else raise ArgumentError, <<~MESSAGE - Invalid patient comparison option: #{compare_option}. - Supported options are: class_import, cohort_import, session, manual_entry - MESSAGE + Invalid patient comparison option: #{compare_option}. + Supported options are: class_import, cohort_import, session, manual_entry + MESSAGE end end From b9ffecf78e0775f70f24f8953e2eb4fe5cb43223 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Thu, 7 Aug 2025 15:29:02 +0100 Subject: [PATCH 17/27] Enforce timeline PII access controls on frontend --- .../app_timeline_filter_component.html.erb | 29 ++++++++++++------- .../app_timeline_filter_component.rb | 5 +++- .../inspect/timeline/patients_controller.rb | 10 ++++--- .../inspect/timeline/patients/show.html.erb | 23 +++++++-------- 4 files changed, 39 insertions(+), 28 deletions(-) diff --git a/app/components/app_timeline_filter_component.html.erb b/app/components/app_timeline_filter_component.html.erb index 1ae466d8ed..bf85a8d466 100644 --- a/app/components/app_timeline_filter_component.html.erb +++ b/app/components/app_timeline_filter_component.html.erb @@ -6,7 +6,25 @@ turbo: "true", turbo_action: "replace" }, builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %> - <%= f.govuk_fieldset legend: { text: "Events to display:", size: "s" } do %> + +
    + <% if @pii_access_allowed %> + <%= f.govuk_check_box :show_pii, true, + label: { text: "Show PII" }, + checked: @show_pii, + "data-autosubmit-target": "field", + "data-action": "autosubmit#submit", + "data-turbo-permanent": "true" %> + <% else %> + <%= f.govuk_check_box :show_pii, true, + label: { text: "Show PII (not allowed for this user)" }, + checked: false, + "disabled": "true" %> + <% end %> +
    + + + <%= f.govuk_fieldset legend: { text: "Events to display:", size: "s" } do %> <% event_options.keys.each do |value| %> <%= f.govuk_check_boxes_fieldset :event_names, legend: { hidden: true } do %> <%= f.govuk_check_box :event_names, @@ -159,15 +177,6 @@ <% end %> <% end %> -
    - <%= f.govuk_check_box :show_pii, true, - label: { text: "Show PII" }, - checked: @show_pii, - "data-autosubmit-target": "field", - "data-action": "autosubmit#submit", - "data-turbo-permanent": "true" %> -
    - <%= helpers.govuk_button_link_to "Reset filters", @reset_url, class: "govuk-button govuk-button--secondary nhsuk-u-display-block app-button--small", diff --git a/app/components/app_timeline_filter_component.rb b/app/components/app_timeline_filter_component.rb index 0638412907..e8a7b91497 100644 --- a/app/components/app_timeline_filter_component.rb +++ b/app/components/app_timeline_filter_component.rb @@ -12,7 +12,8 @@ def initialize( cohort_imports:, sessions:, reset_url:, - show_pii: + show_pii:, + pii_access_allowed: ) @url = url @patient = patient @@ -25,6 +26,7 @@ def initialize( @sessions = sessions @reset_url = reset_url @show_pii = show_pii + @pii_access_allowed = pii_access_allowed end attr_reader :url, @@ -37,4 +39,5 @@ def initialize( :sessions, :reset_url, :show_pii + :pii_access_allowed end diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index 24ea7985fe..70988dd8bd 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -21,7 +21,7 @@ class Inspect::Timeline::PatientsController < ApplicationController ].freeze def show - @show_pii = get_show_pii + set_pii_settings params.reverse_merge!(event_names: DEFAULT_EVENT_NAMES) params[:audit_config] ||= {} @@ -70,9 +70,11 @@ def show private - def get_show_pii - return false unless user_is_support_with_pii_access? - params[:show_pii] || SHOW_PII_BY_DEFAULT + def set_pii_settings + @user_is_allowed_to_access_pii = user_is_support_with_pii_access? + @show_pii = + @user_is_allowed_to_access_pii && + (params[:show_pii] || SHOW_PII_BY_DEFAULT) end def set_patient diff --git a/app/views/inspect/timeline/patients/show.html.erb b/app/views/inspect/timeline/patients/show.html.erb index d4f545b810..53774d10c5 100644 --- a/app/views/inspect/timeline/patients/show.html.erb +++ b/app/views/inspect/timeline/patients/show.html.erb @@ -1,18 +1,14 @@ -<%= h1 page_title: @patient.initials do %> - <%= "Inspect Patient #{@patient.id}" %> -<% end %> +
    + <%= h1 page_title: @patient.initials do %> + <%= "Inspect Patient #{@patient.id}" %> + <% end %> - - <%= "Patient.find(#{@patient.id})" %> - + + <%= "Patient.find(#{@patient.id})" %> + +
    -<% if params[:event_names].include?("audits") && @patient_timeline && !@show_pii %> - <%= govuk_inset_text do %> -

    - Only audited changes that do not involve PII are included -

    - <% end %> -<% end %> +<%= link_to "View graph", inspect_path(:patient, @patient.id), class: "nhsuk-button nhsuk-button--secondary" %>
    @@ -34,6 +30,7 @@ show_pii: false, ), show_pii: @show_pii, + pii_access_allowed: @user_is_allowed_to_access_pii, ) %>
    From 97c901cd74fc6b9b2217cb7600624036c77b6c31 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Thu, 7 Aug 2025 17:19:22 +0100 Subject: [PATCH 18/27] Enforce graph PII access controls on frontend --- app/controllers/inspect/graphs_controller.rb | 12 ++++++++---- app/views/inspect/graphs/show.html.erb | 17 ++++++++++++++--- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb index b002e8c8df..c7ba033d7a 100644 --- a/app/controllers/inspect/graphs_controller.rb +++ b/app/controllers/inspect/graphs_controller.rb @@ -6,6 +6,8 @@ class GraphsController < ApplicationController layout "full" + SHOW_PII_BY_DEFAULT = false + def show @primary_type = safe_get_primary_type if @primary_type.nil? @@ -28,7 +30,7 @@ def show @traversals_config = build_traversals_config @graph_params = build_graph_params - @show_pii = get_show_pii + set_pii_settings @mermaid = GraphRecords @@ -44,9 +46,11 @@ def show private - def get_show_pii - return false unless user_is_support_with_pii_access? - params[:show_pii]&.first == "1" + def set_pii_settings + @user_is_allowed_to_access_pii = user_is_support_with_pii_access? + @show_pii = + @user_is_allowed_to_access_pii && + (params[:show_pii] || SHOW_PII_BY_DEFAULT) end def build_traversals_config diff --git a/app/views/inspect/graphs/show.html.erb b/app/views/inspect/graphs/show.html.erb index dd3242220a..7e63ec5022 100644 --- a/app/views/inspect/graphs/show.html.erb +++ b/app/views/inspect/graphs/show.html.erb @@ -1,6 +1,7 @@
    "> - <%= h1 "Inspect #{@primary_type.to_s.classify} #{@primary_id}", size: "xl", style: "margin-bottom: 10px;" %> - + <%= h1 page_title: "Inspect #{@primary_type.to_s.classify} #{@primary_id}" do %> + <%= "Inspect #{@primary_type.to_s.classify} #{@primary_id}" %> + <% end %> <%= "#{@primary_type.to_s.classify}.find(#{@primary_id})" %> @@ -27,7 +28,17 @@ <%= render AppDetailsComponent.new(summary: "Graph options", expander: true, open: false) do %> <%= form_with url: inspect_path(object_type: @primary_type.to_s.pluralize, object_id: @primary_id), method: :get, local: true do |form| %>
    - <%= form.govuk_check_box :show_pii, 1, label: { text: "Show PII (Personally Identifiable Information)" }, checked: @show_pii, onchange: "this.form.submit();" %> + <% if @user_is_allowed_to_access_pii %> + <%= form.govuk_check_box :show_pii, true, + label: { text: "Show PII" }, + checked: @show_pii, + "onchange": "this.form.submit();" %> + <% else %> + <%= form.govuk_check_box :show_pii, true, + label: { text: "Show PII (not allowed for this user)" }, + checked: false, + "disabled": "true" %> + <% end %>
    <% @traversals_config.keys.sort_by { |r| r.name.to_s.humanize }.each do |type| %> From d2489dd1d11b1b3b835a26ff44afd5d3f148abf6 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Mon, 11 Aug 2025 13:52:19 +0100 Subject: [PATCH 19/27] Add feature tests for support access to tools --- ...entication_inspect_support_no_pii_spec.rb} | 36 ++++-- ...ntication_inspect_support_with_pii_spec.rb | 119 ++++++++++++++++++ 2 files changed, 142 insertions(+), 13 deletions(-) rename spec/features/{user_cis2_authentication_inspect_support_spec.rb => user_cis2_authentication_inspect_support_no_pii_spec.rb} (70%) create mode 100644 spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb diff --git a/spec/features/user_cis2_authentication_inspect_support_spec.rb b/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb similarity index 70% rename from spec/features/user_cis2_authentication_inspect_support_spec.rb rename to spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb index 5f3982eb1d..b75fec6a97 100644 --- a/spec/features/user_cis2_authentication_inspect_support_spec.rb +++ b/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb @@ -1,33 +1,33 @@ # frozen_string_literal: true describe "Inspect tools", :cis2 do - scenario "Support user can view timeline" do + scenario "Support user without PII access can view timeline but with PII checkbox disabled" do given_a_test_support_organisation_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway - when_i_login_as_a_support_user + when_i_login_as_a_support_user_without_pii_access then_i_see_the_inspect_dashboard when_i_go_to_the_timeline_url_for_the_patient - then_i_see_the_timeline + then_i_see_the_timeline_with_pii_checkbox_disabled end - scenario "Support user can view graph" do + scenario "Support user without PII access can view graph but with PII checkbox disabled" do given_a_test_support_organisation_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway - when_i_login_as_a_support_user + when_i_login_as_a_support_user_without_pii_access then_i_see_the_inspect_dashboard when_i_go_to_the_graph_url_for_the_patient - then_i_see_the_graph + then_i_see_the_graph_with_pii_checkbox_disabled end - scenario "Support user can't view confidential pages" do + scenario "Support user without PII access can't view confidential pages" do given_a_test_support_organisation_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway - when_i_login_as_a_support_user + when_i_login_as_a_support_user_without_pii_access and_i_go_to_a_confidential_page then_i_see_the_inspect_dashboard @@ -51,8 +51,7 @@ def given_a_test_support_organisation_is_setup_in_mavis_and_cis2 workgroups: [CIS2Info::SUPPORT_WORKGROUP], role_code: CIS2Info::SUPPORT_ROLE, activity_codes: [ - CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE, - CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE + CIS2Info::VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE ] ) end @@ -82,7 +81,7 @@ def given_an_hpv_programme_is_underway ) end - def when_i_login_as_a_support_user + def when_i_login_as_a_support_user_without_pii_access visit "/start" click_button "Care Identity" expect(page).to have_content("TEST, Support") @@ -101,12 +100,23 @@ def and_i_go_to_a_confidential_page visit patients_path end - def then_i_see_the_timeline + def then_i_see_the_timeline_with_pii_checkbox_disabled expect(page).to have_content("Customise timeline") + + expect(page).to have_field( + "Show PII (not allowed for this user)", + disabled: true + ) end - def then_i_see_the_graph + def then_i_see_the_graph_with_pii_checkbox_disabled expect(page).to have_content("Graph options") + + find("summary", text: "Graph options").click + expect(page).to have_field( + "Show PII (not allowed for this user)", + disabled: true + ) end def then_i_see_the_inspect_dashboard diff --git a/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb b/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb new file mode 100644 index 0000000000..e41aa050ec --- /dev/null +++ b/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +describe "Inspect tools with PII access", :cis2 do + scenario "Support user with PII access can view timeline with functional PII checkbox" do + given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user_with_pii_access + + and_i_go_to_the_timeline_url_for_the_patient + then_i_see_the_timeline_with_functional_pii_checkbox + end + + scenario "Support user with PII access can view graph with functional PII checkbox" do + given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user_with_pii_access + + and_i_go_to_the_graph_url_for_the_patient + then_i_see_the_graph_with_functional_pii_checkbox + end + + scenario "Support user with PII can't view confidential pages" do + given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user_with_pii_access + + and_i_go_to_a_confidential_page + then_i_see_the_inspect_dashboard + end + + def given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 + @organisation_support = create(:organisation, ods_code: "X26") + @team_support = + create( + :team, + organisation: @organisation_support, + workgroup: CIS2Info::SUPPORT_WORKGROUP + ) + @user = create(:user, :support, team: @team_support) + + mock_cis2_auth( + uid: "123", + given_name: "Support", + family_name: "Test", + org_code: @organisation_support.ods_code, + workgroups: [CIS2Info::SUPPORT_WORKGROUP], + role_code: CIS2Info::SUPPORT_ROLE, + activity_codes: [ + CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE, + CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE, + CIS2Info::VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE + ] + ) + end + + def given_an_hpv_programme_is_underway + @team = create(:team, :with_one_nurse) + @programme = create(:programme, :hpv, teams: [@team]) + @session = + create( + :session, + date: Date.yesterday, + team: @team, + programmes: [@programme] + ) + + @patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "John", + family_name: "Smith", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + end + + def when_i_login_as_a_support_user_with_pii_access + visit "/start" + click_button "Care Identity" + expect(page).to have_content("TEST, Support") + expect(page).to have_button("Log out") + end + + def and_i_go_to_the_timeline_url_for_the_patient + visit inspect_timeline_patient_path(id: @patient.id) + end + + def and_i_go_to_the_graph_url_for_the_patient + visit inspect_path(object_type: "patient", object_id: @patient.id) + end + + def and_i_go_to_a_confidential_page + visit patients_path + end + + def then_i_see_the_timeline_with_functional_pii_checkbox + expect(page).to have_content("Customise timeline") + + expect(page).to have_field("Show PII", disabled: false) + end + + def then_i_see_the_graph_with_functional_pii_checkbox + expect(page).to have_content("Graph options") + + find("summary", text: "Graph options").click + expect(page).to have_field("Show PII", disabled: false) + end + + def then_i_see_the_inspect_dashboard + expect(page).to have_content("Operational support tools") + end +end From e99ec30a3f8ec3ea1cf46e2d5b3bbaeef69b20b3 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Thu, 25 Sep 2025 17:56:05 +0100 Subject: [PATCH 20/27] Fix tests after rebase --- .../app_timeline_filter_component.html.erb | 12 ++++++------ .../app_timeline_filter_component.rb | 7 ++++--- db/seeds.rb | 6 +----- spec/factories/users.rb | 2 ++ ...thentication_inspect_support_no_pii_spec.rb | 10 +++------- spec/models/user_spec.rb | 18 +++++++++--------- 6 files changed, 25 insertions(+), 30 deletions(-) diff --git a/app/components/app_timeline_filter_component.html.erb b/app/components/app_timeline_filter_component.html.erb index bf85a8d466..82c7e9b373 100644 --- a/app/components/app_timeline_filter_component.html.erb +++ b/app/components/app_timeline_filter_component.html.erb @@ -1,6 +1,6 @@ <%= render AppCardComponent.new(filters: true) do |card| %> <% card.with_heading { "Customise timeline" } %> - <%= form_with url: @url, + <%= form_with url: url, method: :get, data: { module: "autosubmit", turbo: "true", @@ -8,10 +8,10 @@ builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
    - <% if @pii_access_allowed %> + <% if pii_access_allowed %> <%= f.govuk_check_box :show_pii, true, label: { text: "Show PII" }, - checked: @show_pii, + checked: show_pii, "data-autosubmit-target": "field", "data-action": "autosubmit#submit", "data-turbo-permanent": "true" %> @@ -82,13 +82,13 @@ <% end %> <%= f.govuk_check_box :event_names, "org_cohort_imports", - label: { text: "Cohort Imports for Team #{@teams.join(",")} excluding patient" }, + label: { text: "Cohort Imports for Team #{teams.join(",")} excluding patient" }, checked: "org_cohort_imports".in?(params[:event_names]), "data-autosubmit-target": "field", "data-action": "autosubmit#submit", "data-turbo-permanent": "true" %> - <% (@additional_class_imports).each do |location_id, import_ids| %> + <% (additional_class_imports).each do |location_id, import_ids| %> <%= f.govuk_check_box :event_names, "add_class_imports_#{location_id}", label: { text: "Class Imports for Location-#{location_id} excluding Patient" }, checked: "add_class_imports_#{location_id}".in?(params[:event_names]), @@ -178,7 +178,7 @@ <% end %> <%= helpers.govuk_button_link_to "Reset filters", - @reset_url, + reset_url, class: "govuk-button govuk-button--secondary nhsuk-u-display-block app-button--small", secondary: true, "data-autosubmit-target": "reset", diff --git a/app/components/app_timeline_filter_component.rb b/app/components/app_timeline_filter_component.rb index e8a7b91497..e0498dcd87 100644 --- a/app/components/app_timeline_filter_component.rb +++ b/app/components/app_timeline_filter_component.rb @@ -30,14 +30,15 @@ def initialize( end attr_reader :url, + :reset_url, :patient, + :teams, :event_options, :timeline_fields, :additional_class_imports, :class_imports, :cohort_imports, :sessions, - :reset_url, - :show_pii - :pii_access_allowed + :show_pii, + :pii_access_allowed end diff --git a/db/seeds.rb b/db/seeds.rb index df82d1aeb1..24bd18eec6 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -308,11 +308,7 @@ def create_team_sessions(user, team) ods_code: CIS2Info::SUPPORT_ORGANISATION, workgroup: CIS2Info::SUPPORT_WORKGROUP ) -create_user( - team: support_team, - email: "support@example.com", - fallback_role: "support" -) +create_user(:support, team: support_team, email: "support@example.com") attach_sample_of_schools_to(team) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index d93024db93..dfa3e51748 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -118,12 +118,14 @@ end trait :support do + team { create(:team, ods_code: "X26") } role_code { CIS2Info::SUPPORT_ROLE } sequence(:email) { |n| "support-#{n}@example.com" } role_workgroups { [CIS2Info::SUPPORT_WORKGROUP] } fallback_role { :support } activity_codes do [ + CIS2Info::VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE, CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE, CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE ] diff --git a/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb b/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb index b75fec6a97..1e67dd0371 100644 --- a/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb +++ b/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb @@ -34,20 +34,16 @@ end def given_a_test_support_organisation_is_setup_in_mavis_and_cis2 - @organisation_support = create(:organisation, ods_code: "X26") + @ods_code = "X26" @team_support = - create( - :team, - organisation: @organisation_support, - workgroup: CIS2Info::SUPPORT_WORKGROUP - ) + create(:team, ods_code: @ods_code, workgroup: CIS2Info::SUPPORT_WORKGROUP) @user = create(:user, :support, team: @team_support) mock_cis2_auth( uid: "123", given_name: "Support", family_name: "Test", - org_code: @organisation_support.ods_code, + org_code: @ods_code, workgroups: [CIS2Info::SUPPORT_WORKGROUP], role_code: CIS2Info::SUPPORT_ROLE, activity_codes: [ diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5363829519..20bffb59d3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -106,7 +106,7 @@ it { should be(true) } end - context "when the user is an admin and superuser" do + context "when the user is a medical secretary and superuser" do let(:user) { build(:medical_secretary, :superuser) } it { should be(false) } @@ -318,14 +318,14 @@ it { should be false } end - context "when the user is an admin" do - let(:user) { build(:admin) } + context "when the user is a medical secretary" do + let(:user) { build(:medical_secretary) } it { should be false } end - context "when the user is an admin and superuser" do - let(:user) { build(:admin, :superuser) } + context "when the user is a medical secretary and superuser" do + let(:user) { build(:medical_secretary, :superuser) } it { should be false } end @@ -338,14 +338,14 @@ it { should be true } end - context "when the user is an admin" do - let(:user) { build(:admin) } + context "when the user is a medical secretary" do + let(:user) { build(:medical_secretary) } it { should be false } end - context "when the user is an admin and superuser" do - let(:user) { build(:admin, :superuser) } + context "when the user is a medical secretary and superuser" do + let(:user) { build(:medical_secretary, :superuser) } it { should be false } end From 38284953e3588064f6d3dd3522deeb947a083a07 Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Fri, 30 May 2025 15:16:19 +0100 Subject: [PATCH 21/27] Add access logging for PII mode in timeline and graphs - Add new enum values to AccessLogEntry: timeline/graphs controllers and show_pii action --- app/models/access_log_entry.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/models/access_log_entry.rb b/app/models/access_log_entry.rb index 1f02a5edf4..1283c0678f 100644 --- a/app/models/access_log_entry.rb +++ b/app/models/access_log_entry.rb @@ -26,6 +26,9 @@ class AccessLogEntry < ApplicationRecord belongs_to :user belongs_to :patient - enum :controller, { patients: 0, patient_sessions: 1 }, validate: true - enum :action, { show: 0, log: 1 }, validate: true + enum :controller, + { patients: 0, patient_sessions: 1, timeline: 2, graph: 3 }, + validate: true + + enum :action, { show: 0, log: 1, show_pii: 2 }, validate: true end From 1fb8181db441e9634fc5b29049975724eef77088 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Tue, 29 Jul 2025 13:52:10 +0100 Subject: [PATCH 22/27] Add column to access log to store request details - Add new request_details column to AccessLogEntry --- app/models/access_log_entry.rb | 15 ++++++++------- ...321_add_request_details_to_access_log_entry.rb | 7 +++++++ db/schema.rb | 3 ++- spec/factories/access_log_entries.rb | 15 ++++++++------- spec/models/access_log_entry_spec.rb | 15 ++++++++------- 5 files changed, 33 insertions(+), 22 deletions(-) create mode 100644 db/migrate/20250926113321_add_request_details_to_access_log_entry.rb diff --git a/app/models/access_log_entry.rb b/app/models/access_log_entry.rb index 1283c0678f..81d138717a 100644 --- a/app/models/access_log_entry.rb +++ b/app/models/access_log_entry.rb @@ -4,13 +4,14 @@ # # Table name: access_log_entries # -# id :bigint not null, primary key -# action :integer not null -# controller :integer not null -# created_at :datetime not null -# updated_at :datetime not null -# patient_id :bigint not null -# user_id :bigint not null +# id :bigint not null, primary key +# action :integer not null +# controller :integer not null +# request_details :jsonb +# created_at :datetime not null +# updated_at :datetime not null +# patient_id :bigint not null +# user_id :bigint not null # # Indexes # diff --git a/db/migrate/20250926113321_add_request_details_to_access_log_entry.rb b/db/migrate/20250926113321_add_request_details_to_access_log_entry.rb new file mode 100644 index 0000000000..46ed8df316 --- /dev/null +++ b/db/migrate/20250926113321_add_request_details_to_access_log_entry.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddRequestDetailsToAccessLogEntry < ActiveRecord::Migration[8.0] + def change + add_column :access_log_entries, :request_details, :jsonb + end +end diff --git a/db/schema.rb b/db/schema.rb index d4e1326039..ec62009cd0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_09_22_201759) do +ActiveRecord::Schema[8.0].define(version: 2025_09_26_113321) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pg_trgm" @@ -22,6 +22,7 @@ t.integer "action", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.jsonb "request_details" t.index ["patient_id"], name: "index_access_log_entries_on_patient_id" t.index ["user_id"], name: "index_access_log_entries_on_user_id" end diff --git a/spec/factories/access_log_entries.rb b/spec/factories/access_log_entries.rb index e365db808c..55892c1dda 100644 --- a/spec/factories/access_log_entries.rb +++ b/spec/factories/access_log_entries.rb @@ -4,13 +4,14 @@ # # Table name: access_log_entries # -# id :bigint not null, primary key -# action :integer not null -# controller :integer not null -# created_at :datetime not null -# updated_at :datetime not null -# patient_id :bigint not null -# user_id :bigint not null +# id :bigint not null, primary key +# action :integer not null +# controller :integer not null +# request_details :jsonb +# created_at :datetime not null +# updated_at :datetime not null +# patient_id :bigint not null +# user_id :bigint not null # # Indexes # diff --git a/spec/models/access_log_entry_spec.rb b/spec/models/access_log_entry_spec.rb index f0cc2c53c1..c9861ccc51 100644 --- a/spec/models/access_log_entry_spec.rb +++ b/spec/models/access_log_entry_spec.rb @@ -4,13 +4,14 @@ # # Table name: access_log_entries # -# id :bigint not null, primary key -# action :integer not null -# controller :integer not null -# created_at :datetime not null -# updated_at :datetime not null -# patient_id :bigint not null -# user_id :bigint not null +# id :bigint not null, primary key +# action :integer not null +# controller :integer not null +# request_details :jsonb +# created_at :datetime not null +# updated_at :datetime not null +# patient_id :bigint not null +# user_id :bigint not null # # Indexes # From cd46718f559d1bc93b70872194a37c133884b652 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Tue, 29 Jul 2025 14:01:01 +0100 Subject: [PATCH 23/27] Log request details for PII accesses in timeline - Log fields accessed during a request which accessed PII - Log whether audits were enabled - Create an access log if a patient is being compared in the timeline view, only if the compared patient is valid. --- .../inspect/timeline/patients_controller.rb | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index 70988dd8bd..b575136d12 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -3,6 +3,7 @@ class Inspect::Timeline::PatientsController < ApplicationController skip_after_action :verify_policy_scoped before_action :set_patient + after_action :record_access_log_entry layout "full" @@ -61,7 +62,8 @@ def show TimelineRecords.new( @compare_patient, detail_config: build_details_config, - audit_config: audit_config + audit_config: audit_config, + show_pii: @show_pii ).load_timeline_events(event_names) @no_events_compare_message = true if @compare_patient_timeline.empty? @@ -124,4 +126,46 @@ def build_details_config hash[event_type.to_sym] = selected_fields end end + + def pii_accessed? + return false unless @show_pii + detail_config = build_details_config + detail_config.any? do |event_type, selected_fields| + pii_fields = + TimelineRecords::AVAILABLE_DETAILS_CONFIG_PII[event_type] || [] + (selected_fields & pii_fields).any? + end + end + + def audit_pii_accessed? + true if @show_pii && params[:event_names].include?("audits") + end + + def record_access_log_entry + return unless pii_accessed? || audit_pii_accessed? + + details_accessed = + build_details_config.reverse_merge( + params[:event_names].map { |key| [key.to_sym, []] }.to_h + ) + details_accessed[:audits] = :accessed if details_accessed.key?(:audits) + + # Log access for main patient + @patient.access_log_entries.create!( + user: current_user, + controller: "timeline", + action: "show_pii", + request_details: details_accessed + ) + + # Log access for compare patient if it exists and is valid + if @compare_patient && @compare_patient != :invalid_patient + @compare_patient.access_log_entries.create!( + user: current_user, + controller: "timeline", + action: "show_pii", + request_details: details_accessed + ) + end + end end From 5582534547f9186ef84eac4198d6bc8c68bd1e37 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Tue, 5 Aug 2025 13:20:33 +0100 Subject: [PATCH 24/27] Log PII access for graph view - Log PII access for all patients whose PII is present in the graph - Restrict what counts as PII (location information and vaccine batch information is not on its own sufficient) - Log additional information about main target of request - Do not display additional IDs in the graph if the type corresponding to the ID is not in traversal tree --- app/controllers/inspect/graphs_controller.rb | 80 +++++++++++++++++--- lib/graph_records.rb | 26 +++++++ 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb index c7ba033d7a..30af090347 100644 --- a/app/controllers/inspect/graphs_controller.rb +++ b/app/controllers/inspect/graphs_controller.rb @@ -3,6 +3,7 @@ module Inspect class GraphsController < ApplicationController skip_after_action :verify_policy_scoped + after_action :record_access_log_entry layout "full" @@ -32,16 +33,14 @@ def show @graph_params = build_graph_params set_pii_settings - @mermaid = - GraphRecords - .new( - traversals_config: @traversals_config, - primary_type: @primary_type, - clickable: true, - show_pii: @show_pii - ) - .graph(**@graph_params) - .join("\n") + @graph_record = + GraphRecords.new( + traversals_config: @traversals_config, + primary_type: @primary_type, + clickable: true, + show_pii: @show_pii + ) + @mermaid = @graph_record.graph(**@graph_params).join("\n") end private @@ -104,5 +103,66 @@ def safe_get_primary_type return nil unless GraphRecords::ALLOWED_TYPES.include?(singular_type) singular_type.to_sym end + + def pii_accessed? + return false unless @show_pii + + build_traversals_config.any? do |from_type, rels| + GraphRecords::EXTRA_DETAIL_WHITELIST_WITH_PII.key?( + from_type.name.underscore.to_sym + ) || + rels.any? do |rel| + from_class = from_type.to_s.classify.constantize + to_type = from_class.reflect_on_association(rel)&.klass + to_type && + GraphRecords::EXTRA_DETAIL_WHITELIST_WITH_PII.key?( + to_type.name.underscore.to_sym + ) + end + end + end + + def record_access_log_entry + if pii_accessed? + @graph_record.patients_with_pii_in_graph.each do |patient| + request_details = build_request_details + additional_ids = + params[:additional_ids] + &.to_unsafe_h + &.each_with_object({}) do |(type, ids_string), result| + result[type.to_sym] = ids_string if ids_string.present? + end + + patient.access_log_entries.create!( + user: current_user, + controller: "graph", + action: "show_pii", + request_details: { + primary_type: @primary_type, + primary_id: @primary_id, + additional_ids: additional_ids.presence, + visible_fields: request_details + } + ) + end + end + end + + def build_request_details + build_traversals_config.each_with_object( + {} + ) do |(_, relationships), details| + relationships.each { |rel| add_fields_to_details(details, rel) } + end + end + + def add_fields_to_details(details, type_sym) + fields = [] + fields.concat(GraphRecords::DETAIL_WHITELIST[type_sym] || []) + fields.concat( + GraphRecords::EXTRA_DETAIL_WHITELIST_WITH_PII[type_sym] || [] + ) + details[type_sym] = fields.uniq if fields.any? + end end end diff --git a/lib/graph_records.rb b/lib/graph_records.rb index c963a17ccb..9c0c36fe22 100644 --- a/lib/graph_records.rb +++ b/lib/graph_records.rb @@ -283,6 +283,11 @@ def graph(**objects) objects.map do |klass, ids| class_name = klass.to_s.singularize + class_sym = class_name.to_sym + + # Skip objects whose type is not in the traversal configuration + next unless traversals.key?(class_sym) + associated_objects = load_association(class_name.classify.constantize.where(id: ids)) @@ -409,6 +414,27 @@ def node_display_name(obj) "#{klass} #{obj.id}" end + def patients_with_pii_in_graph + result = Set.new(@nodes.select { |node| node.is_a?(Patient) }) + + @nodes.each do |node| + # Skip if not a type with potential PII + node_type = node.class.name.underscore.to_sym + next unless EXTRA_DETAIL_WHITELIST_WITH_PII.key?(node_type) + next if node.is_a?(Patient) # Already handled these + + if node.respond_to?(:patient) && node.patient + result << node.patient + elsif node.respond_to?(:consent) && node.consent&.patient + result << node.consent.patient + elsif node.respond_to?(:patients) + result.merge(node.patients.to_a) + end + end + + result.to_a + end + def non_breaking_text(text) # Insert non-breaking spaces and hyphens to prevent Mermaid from breaking the line text.gsub(" ", " ").gsub("-", "#8209;") From b57c2654cd72ada8cb33a20aaec0a72189424926 Mon Sep 17 00:00:00 2001 From: Sam Coy Date: Fri, 15 Aug 2025 14:10:37 +0100 Subject: [PATCH 25/27] Add unit tests for PII access logging - Adds tests for the graph and timeline view for auditing PII access --- .../inspect_graph_pii_access_logging_spec.rb | 199 ++++++++++++++++++ ...nspect_timeline_pii_access_logging_spec.rb | 139 ++++++++++++ 2 files changed, 338 insertions(+) create mode 100644 spec/features/inspect_graph_pii_access_logging_spec.rb create mode 100644 spec/features/inspect_timeline_pii_access_logging_spec.rb diff --git a/spec/features/inspect_graph_pii_access_logging_spec.rb b/spec/features/inspect_graph_pii_access_logging_spec.rb new file mode 100644 index 0000000000..68a36e21b0 --- /dev/null +++ b/spec/features/inspect_graph_pii_access_logging_spec.rb @@ -0,0 +1,199 @@ +# frozen_string_literal: true + +describe "Inspect graph PII access logging", :cis2 do + scenario "Support user with PII access visiting graph of patient" do + prepare_support_organisation_with_pii_access + prepare_hpv_programme_with_one_patient + + when_i_login_as_a_support_user_with_pii_access + and_i_visit_a_patient_graph_with_pii_enabled + + then_an_access_log_entry_is_created_for_the_patient + and_the_access_log_entry_has_correct_attributes + end + + scenario "Support user with PII visiting graph of session related to patient" do + prepare_support_organisation_with_pii_access + prepare_hpv_programme_with_one_patient + + when_i_login_as_a_support_user_with_pii_access + and_i_visit_a_parent_graph_with_pii_enabled + + then_an_access_log_entry_is_created_for_the_patient + and_the_access_log_entry_has_correct_attributes + end + + scenario "Support user with PII access visiting graph of patient with additional patients" do + prepare_support_organisation_with_pii_access + prepare_hpv_programme_with_two_patients + + when_i_login_as_a_support_user_with_pii_access + and_i_visit_a_patient_graph_with_additional_patient + + then_access_logs_are_created_for_both_patients + and_both_access_log_entries_have_correct_attributes + end + + # Setup methods + def prepare_support_organisation_with_pii_access + @organisation_support = create(:organisation, ods_code: "X26") + @team_support = + create( + :team, + organisation: @organisation_support, + workgroup: CIS2Info::SUPPORT_WORKGROUP + ) + + mock_cis2_auth( + uid: "123", + given_name: "Support", + family_name: "Test", + org_code: @organisation_support.ods_code, + workgroups: [CIS2Info::SUPPORT_WORKGROUP], + role_code: CIS2Info::SUPPORT_ROLE, + activity_codes: [ + CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE, + CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE, + CIS2Info::VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE + ] + ) + end + + def prepare_hpv_programme_with_one_patient + @team = create(:team, :with_one_nurse) + @programme = create(:programme, :hpv, teams: [@team]) + @session = + create( + :session, + date: Date.yesterday, + team: @team, + programmes: [@programme] + ) + + @patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "John", + family_name: "Smith", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + + @parent = create(:parent) + create(:parent_relationship, parent: @parent, patient: @patient) + end + + def prepare_hpv_programme_with_two_patients + @team = create(:team, :with_one_nurse) + @programme = create(:programme, :hpv, teams: [@team]) + @session = + create( + :session, + date: Date.yesterday, + team: @team, + programmes: [@programme] + ) + + @patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "John", + family_name: "Smith", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + + @additional_patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "Jane", + family_name: "Doe", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + end + + def when_i_login_as_a_support_user_with_pii_access + visit "/start" + click_button "Care Identity" + expect(page).to have_content("TEST, Support") + expect(page).to have_button("Log out") + end + + def and_i_visit_a_patient_graph_with_pii_enabled + visit inspect_path( + object_type: "patients", + object_id: @patient.id, + show_pii: ["true"], + relationships: GraphRecords::DEFAULT_TRAVERSALS[:patient], + additional_ids: { + patient: "" + } + ) + expect(page).to have_content("Graph options") + end + + def and_i_visit_a_parent_graph_with_pii_enabled + visit inspect_path( + object_type: "parents", + object_id: @parent.id, + show_pii: ["true"], + relationships: GraphRecords::DEFAULT_TRAVERSALS[:parent], + additional_ids: { + parent: "" + } + ) + expect(page).to have_content("Graph options") + end + + def and_i_visit_a_patient_graph_with_additional_patient + visit inspect_path( + object_type: "patient", + object_id: @patient.id, + show_pii: ["true"], + relationships: GraphRecords::DEFAULT_TRAVERSALS[:patient], + additional_ids: { + patient: @additional_patient.id.to_s + } + ) + expect(page).to have_content("Graph options") + end + + def then_an_access_log_entry_is_created_for_the_patient + # One log is created when visiting with show_pii: true + expect(@patient.access_log_entries.count).to eq(1) + end + + def then_access_logs_are_created_for_both_patients + # One log is created for each patient when visiting with show_pii: true + expect(@patient.access_log_entries.count).to eq(1) + expect(@additional_patient.access_log_entries.count).to eq(1) + end + + def and_the_access_log_entry_has_correct_attributes + verify_log_entry(@patient.access_log_entries.last) + end + + def and_both_access_log_entries_have_correct_attributes + verify_log_entry(@patient.access_log_entries.last) + verify_log_entry(@additional_patient.access_log_entries.last) + end + + def verify_log_entry(log_entry) + expect(User.find(log_entry.user_id).family_name).to eq("Test") + expect(log_entry.controller).to eq("graph") + expect(log_entry.action).to eq("show_pii") + end +end diff --git a/spec/features/inspect_timeline_pii_access_logging_spec.rb b/spec/features/inspect_timeline_pii_access_logging_spec.rb new file mode 100644 index 0000000000..9d959a5a58 --- /dev/null +++ b/spec/features/inspect_timeline_pii_access_logging_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +describe "Inspect timeline PII access logging", :cis2 do + scenario "Support user with PII access visiting timeline" do + prepare_support_organisation_with_pii_access + prepare_hpv_programme_with_one_patient + + when_i_login_as_a_support_user_with_pii_access + and_i_visit_a_patient_timeline_with_pii_enabled + + then_an_access_log_entry_is_created_for_the_patient + and_the_access_log_entry_has_correct_attributes + end + + scenario "Support user with PII access visiting timeline with comparison patient" do + prepare_support_organisation_with_pii_access + prepare_hpv_programme_with_two_patients + + when_i_login_as_a_support_user_with_pii_access + and_i_visit_a_patient_timeline_with_pii_enabled_and_a_comparison_patient + + then_access_log_entries_are_created_for_both_patients + end + + # Setup methods + def prepare_support_organisation_with_pii_access + @organisation_support = create(:organisation, ods_code: "X26") + @team_support = + create( + :team, + organisation: @organisation_support, + workgroup: CIS2Info::SUPPORT_WORKGROUP + ) + + mock_cis2_auth( + uid: "123", + given_name: "Support", + family_name: "Test", + org_code: @organisation_support.ods_code, + workgroups: [CIS2Info::SUPPORT_WORKGROUP], + role_code: CIS2Info::SUPPORT_ROLE, + activity_codes: [ + CIS2Info::ACCESS_SENSITIVE_FLAGGED_RECORDS_ACTIVITY_CODE, + CIS2Info::VIEW_DETAILED_HEALTH_RECORDS_ACTIVITY_CODE, + CIS2Info::VIEW_SHARED_NON_PATIENT_IDENTIFIABLE_INFORMATION_ACTIVITY_CODE + ] + ) + end + + def prepare_hpv_programme_with_one_patient + @team = create(:team, :with_one_nurse) + @programme = create(:programme, :hpv, teams: [@team]) + @session = + create( + :session, + date: Date.yesterday, + team: @team, + programmes: [@programme] + ) + + @patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "John", + family_name: "Smith", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + end + + def prepare_hpv_programme_with_two_patients + prepare_hpv_programme_with_one_patient + + @compare_patient = + create( + :patient, + :consent_given_triage_needed, + :triage_ready_to_vaccinate, + given_name: "Jane", + family_name: "Doe", + year_group: 8, + programmes: [@programme], + team: @team, + session: @session + ) + end + + def when_i_login_as_a_support_user_with_pii_access + visit "/start" + click_button "Care Identity" + expect(page).to have_content("TEST, Support") + expect(page).to have_button("Log out") + end + + def and_i_visit_a_patient_timeline_with_pii_enabled + visit inspect_timeline_patient_path(id: @patient.id, show_pii: "true") + expect(page).to have_content("Customise timeline") + end + + def and_i_visit_a_patient_timeline_with_pii_enabled_and_a_comparison_patient + visit inspect_timeline_patient_path( + id: @patient.id, + show_pii: "true", + compare_option: "manual_entry", + manual_patient_id: @compare_patient.id.to_s + ) + 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) + end + + def and_the_access_log_entry_has_correct_attributes + verify_log_entry(@patient.access_log_entries.last) + end + + def then_access_log_entries_are_created_for_both_patients + # Check main patient log + expect(@patient.access_log_entries.count).to eq(2) + verify_log_entry(@patient.access_log_entries.last) + + # Check comparison patient log + expect(@compare_patient.access_log_entries.count).to eq(1) + verify_log_entry(@compare_patient.access_log_entries.includes(:user).last) + end + + def verify_log_entry(log_entry) + expect(User.find(log_entry.user_id).family_name).to eq("Test") + expect(log_entry.controller).to eq("timeline") + expect(log_entry.action).to eq("show_pii") + end +end From bb4be45dadc4c55355db2fcf25fc14e8672ce778 Mon Sep 17 00:00:00 2001 From: Alistair White-Horne Date: Wed, 1 Oct 2025 17:14:17 +0100 Subject: [PATCH 26/27] Add feature flag for ops tools This will allow us to switch the operational tools on/off. This should let us get the code into production much sooner, and potentially allow an extended testing period if required. --- .../inspect_authentication_concern.rb | 15 ++++++++ .../inspect/dashboard_controller.rb | 3 ++ app/controllers/inspect/graphs_controller.rb | 3 ++ .../inspect/timeline/patients_controller.rb | 3 ++ config/feature_flags.yml | 2 + .../inspect_authentication_concern_spec.rb | 38 +++++++++++++++++++ ...hentication_inspect_support_no_pii_spec.rb | 28 ++++++++++++++ ...ntication_inspect_support_with_pii_spec.rb | 28 ++++++++++++++ 8 files changed, 120 insertions(+) create mode 100644 app/controllers/concerns/inspect_authentication_concern.rb create mode 100644 spec/controllers/concerns/inspect_authentication_concern_spec.rb diff --git a/app/controllers/concerns/inspect_authentication_concern.rb b/app/controllers/concerns/inspect_authentication_concern.rb new file mode 100644 index 0000000000..fc1f735965 --- /dev/null +++ b/app/controllers/concerns/inspect_authentication_concern.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module InspectAuthenticationConcern + extend ActiveSupport::Concern + + included do + private + + def ensure_ops_tools_feature_enabled + unless Flipper.enabled?("ops_tools") + raise ActionController::RoutingError, "Not Found" + end + end + end +end diff --git a/app/controllers/inspect/dashboard_controller.rb b/app/controllers/inspect/dashboard_controller.rb index 6ea6881619..740bef5f83 100644 --- a/app/controllers/inspect/dashboard_controller.rb +++ b/app/controllers/inspect/dashboard_controller.rb @@ -2,7 +2,10 @@ module Inspect class DashboardController < ApplicationController + include InspectAuthenticationConcern + skip_after_action :verify_policy_scoped + before_action :ensure_ops_tools_feature_enabled layout "full" diff --git a/app/controllers/inspect/graphs_controller.rb b/app/controllers/inspect/graphs_controller.rb index 30af090347..a7d0d6010c 100644 --- a/app/controllers/inspect/graphs_controller.rb +++ b/app/controllers/inspect/graphs_controller.rb @@ -2,7 +2,10 @@ module Inspect class GraphsController < ApplicationController + include InspectAuthenticationConcern + skip_after_action :verify_policy_scoped + before_action :ensure_ops_tools_feature_enabled after_action :record_access_log_entry layout "full" diff --git a/app/controllers/inspect/timeline/patients_controller.rb b/app/controllers/inspect/timeline/patients_controller.rb index b575136d12..033b6269a6 100644 --- a/app/controllers/inspect/timeline/patients_controller.rb +++ b/app/controllers/inspect/timeline/patients_controller.rb @@ -1,7 +1,10 @@ # frozen_string_literal: true class Inspect::Timeline::PatientsController < ApplicationController + include InspectAuthenticationConcern + skip_after_action :verify_policy_scoped + before_action :ensure_ops_tools_feature_enabled before_action :set_patient after_action :record_access_log_entry diff --git a/config/feature_flags.yml b/config/feature_flags.yml index 584de194ef..ca427620b3 100644 --- a/config/feature_flags.yml +++ b/config/feature_flags.yml @@ -16,6 +16,8 @@ import_choose_academic_year: Add an option to choose the academic year when import_low_pds_match_rate: Prevent processing uploads with a low pds match rate. +ops_tools: Enable the operational support tools; timeline and graph. + pds_lookup_during_import: Perform PDS lookups as part of the patient import processing. diff --git a/spec/controllers/concerns/inspect_authentication_concern_spec.rb b/spec/controllers/concerns/inspect_authentication_concern_spec.rb new file mode 100644 index 0000000000..b1e24710f4 --- /dev/null +++ b/spec/controllers/concerns/inspect_authentication_concern_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +describe InspectAuthenticationConcern do + let(:sample_class) do + c = + Class.new do + include InspectAuthenticationConcern + + public :ensure_ops_tools_feature_enabled + end + + c.new + end + + describe "#ensure_ops_tools_feature_enabled" do + context "when ops_tools feature flag is enabled" do + before { Flipper.enable(:ops_tools) } + after { Flipper.disable(:ops_tools) } + + it "does not raise an error" do + expect { + sample_class.ensure_ops_tools_feature_enabled + }.not_to raise_error + end + end + + context "when ops_tools feature flag is disabled" do + before { Flipper.disable(:ops_tools) } + + it "raises ActionController::RoutingError with 'Not Found' message" do + expect { sample_class.ensure_ops_tools_feature_enabled }.to raise_error( + ActionController::RoutingError, + "Not Found" + ) + end + end + end +end diff --git a/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb b/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb index 1e67dd0371..85b09722a2 100644 --- a/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb +++ b/spec/features/user_cis2_authentication_inspect_support_no_pii_spec.rb @@ -2,6 +2,7 @@ describe "Inspect tools", :cis2 do scenario "Support user without PII access can view timeline but with PII checkbox disabled" do + given_ops_tools_feature_flag_is_on given_a_test_support_organisation_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway @@ -13,6 +14,7 @@ end scenario "Support user without PII access can view graph but with PII checkbox disabled" do + given_ops_tools_feature_flag_is_on given_a_test_support_organisation_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway @@ -24,6 +26,7 @@ end scenario "Support user without PII access can't view confidential pages" do + given_ops_tools_feature_flag_is_on given_a_test_support_organisation_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway @@ -33,6 +36,27 @@ then_i_see_the_inspect_dashboard end + scenario "`ops_tools` feature flag is off" do + given_ops_tools_feature_flag_is_on + given_a_test_support_organisation_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user_without_pii_access + + given_ops_tools_feature_flag_is_off + when_i_go_to_the_timeline_url_for_the_patient + + then_a_page_not_found_error_is_displayed + end + + def given_ops_tools_feature_flag_is_on + Flipper.enable(:ops_tools) + end + + def given_ops_tools_feature_flag_is_off + Flipper.disable(:ops_tools) + end + def given_a_test_support_organisation_is_setup_in_mavis_and_cis2 @ods_code = "X26" @team_support = @@ -118,4 +142,8 @@ def then_i_see_the_graph_with_pii_checkbox_disabled def then_i_see_the_inspect_dashboard expect(page).to have_content("Operational support tools") end + + def then_a_page_not_found_error_is_displayed + expect(page).to have_content("Page not found") + end end diff --git a/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb b/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb index e41aa050ec..6ec3c7f71e 100644 --- a/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb +++ b/spec/features/user_cis2_authentication_inspect_support_with_pii_spec.rb @@ -2,6 +2,7 @@ describe "Inspect tools with PII access", :cis2 do scenario "Support user with PII access can view timeline with functional PII checkbox" do + given_ops_tools_feature_flag_is_on given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway @@ -12,6 +13,7 @@ end scenario "Support user with PII access can view graph with functional PII checkbox" do + given_ops_tools_feature_flag_is_on given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway @@ -22,6 +24,7 @@ end scenario "Support user with PII can't view confidential pages" do + given_ops_tools_feature_flag_is_on given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 given_an_hpv_programme_is_underway @@ -31,6 +34,27 @@ then_i_see_the_inspect_dashboard end + scenario "`ops_tools` feature flag is off" do + given_ops_tools_feature_flag_is_on + given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 + given_an_hpv_programme_is_underway + + when_i_login_as_a_support_user_with_pii_access + + given_ops_tools_feature_flag_is_off + and_i_go_to_the_timeline_url_for_the_patient + + then_a_page_not_found_error_is_displayed + end + + def given_ops_tools_feature_flag_is_on + Flipper.enable(:ops_tools) + end + + def given_ops_tools_feature_flag_is_off + Flipper.disable(:ops_tools) + end + def given_a_test_ops_organisation_with_pii_is_setup_in_mavis_and_cis2 @organisation_support = create(:organisation, ods_code: "X26") @team_support = @@ -116,4 +140,8 @@ def then_i_see_the_graph_with_functional_pii_checkbox def then_i_see_the_inspect_dashboard expect(page).to have_content("Operational support tools") end + + def then_a_page_not_found_error_is_displayed + expect(page).to have_content("Page not found") + end end From 73a5d1ef008ba8026313f574eb107751abf83cdf Mon Sep 17 00:00:00 2001 From: Jake Benilov Date: Fri, 3 Oct 2025 15:36:00 +0100 Subject: [PATCH 27/27] Ensure ops tools feature specs enable the feature flag first --- spec/features/identity_check_spec.rb | 2 ++ spec/features/inspect_graph_pii_access_logging_spec.rb | 2 ++ spec/features/inspect_timeline_pii_access_logging_spec.rb | 2 ++ 3 files changed, 6 insertions(+) diff --git a/spec/features/identity_check_spec.rb b/spec/features/identity_check_spec.rb index c863c7a190..6c4654308a 100644 --- a/spec/features/identity_check_spec.rb +++ b/spec/features/identity_check_spec.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true describe "HPV vaccination identity check" do + before { Flipper.enable(:ops_tools) } + scenario "Default identification, changed and edited" do given_i_am_signed_in diff --git a/spec/features/inspect_graph_pii_access_logging_spec.rb b/spec/features/inspect_graph_pii_access_logging_spec.rb index 68a36e21b0..b3ed2b5678 100644 --- a/spec/features/inspect_graph_pii_access_logging_spec.rb +++ b/spec/features/inspect_graph_pii_access_logging_spec.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true describe "Inspect graph PII access logging", :cis2 do + before { Flipper.enable(:ops_tools) } + scenario "Support user with PII access visiting graph of patient" do prepare_support_organisation_with_pii_access prepare_hpv_programme_with_one_patient diff --git a/spec/features/inspect_timeline_pii_access_logging_spec.rb b/spec/features/inspect_timeline_pii_access_logging_spec.rb index 9d959a5a58..2be64402bb 100644 --- a/spec/features/inspect_timeline_pii_access_logging_spec.rb +++ b/spec/features/inspect_timeline_pii_access_logging_spec.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true describe "Inspect timeline PII access logging", :cis2 do + before { Flipper.enable(:ops_tools) } + scenario "Support user with PII access visiting timeline" do prepare_support_organisation_with_pii_access prepare_hpv_programme_with_one_patient