From 9b105264fdb52d8acee9b84b75b9f013e5805240 Mon Sep 17 00:00:00 2001 From: HelenWDTK <120410992+HelenWDTK@users.noreply.github.com> Date: Wed, 5 Apr 2023 05:21:55 +0100 Subject: [PATCH] Update admin general timeline Allow the timeline filters by time and event types at the same time. Improves parameters by using a single `start_date` param rather than separate ones for day/hour/week/... Refactors the loading of events to use `ActiveRecord` query where possible and where isn't possible use the query builder and call `#to_sql` in order to get query sanitization. --- app/controllers/admin_general_controller.rb | 78 ++++++------------- app/helpers/admin_general_timeline_helper.rb | 36 +++++++++ app/views/admin_general/timeline.html.erb | 25 +++--- .../admin_general_controller_spec.rb | 27 +++++-- 4 files changed, 88 insertions(+), 78 deletions(-) create mode 100644 app/helpers/admin_general_timeline_helper.rb diff --git a/app/controllers/admin_general_controller.rb b/app/controllers/admin_general_controller.rb index c51fd03c05..8d125e732c 100644 --- a/app/controllers/admin_general_controller.rb +++ b/app/controllers/admin_general_controller.rb @@ -5,6 +5,7 @@ # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ class AdminGeneralController < AdminController + include AdminGeneralTimelineHelper def index # Tasks to do @@ -170,46 +171,11 @@ def debug private - def get_date_back_to_utc - date_back_to = if params[:hour] - Time.zone.now - 1.hour - elsif params[:day] - Time.zone.now - 1.day - elsif params[:week] - Time.zone.now - 1.week - elsif params[:month] - Time.zone.now - 1.month - elsif params[:all] - Time.zone.now - 1000.years - else - Time.zone.now - 2.days - end - date_back_to.getutc - end - - def get_event_type - if params[:event_type] == 'authority_change' - 'Authority changes' - elsif params[:event_type] == 'info_request_event' - 'Request events' - else - "Events" - end - end - def get_events_title - title = if params[:hour] - "#{get_event_type} in last hour" - elsif params[:day] - "#{get_event_type} in last day" - elsif params[:week] - "#{get_event_type} in last week" - elsif params[:month] - "#{get_event_type} in last month" - elsif params[:all] - "#{get_event_type}, all time" + if current_time_filter == 'All time' + "#{current_event_type}, all time" else - "#{get_event_type} in last two days" + "#{current_event_type} in the last #{current_time_filter.downcase}" end end @@ -219,26 +185,26 @@ def get_timestamps # Note that the relevant date for InfoRequestEvents is creation, but # for PublicBodyVersions is update throughout connection = InfoRequestEvent.connection - authority_change_clause = "SELECT id, 'PublicBodyVersion', - updated_at AS timestamp - FROM #{PublicBody.versioned_class.table_name} - WHERE updated_at > '#{get_date_back_to_utc}'" - - info_request_event_clause = "SELECT id, 'InfoRequestEvent', - created_at AS timestamp - FROM info_request_events - WHERE created_at > '#{get_date_back_to_utc}'" - - timestamps = if params[:event_type] == 'authority_change' - connection.select_rows("#{authority_change_clause} - ORDER by timestamp desc") - elsif params[:event_type] == 'info_request_event' - connection.select_rows("#{info_request_event_clause} - ORDER by timestamp desc") + + authority_change_scope = PublicBody.versioned_class. + select("id, 'PublicBodyVersion', updated_at AS timestamp"). + where(updated_at: start_date...). + order(timestamp: :desc) + + info_request_event_scope = InfoRequestEvent. + select("id, 'InfoRequestEvent', created_at AS timestamp"). + where(created_at: start_date...). + order(timestamp: :desc) + + case params[:event_type] + when 'authority_change' + connection.select_rows(authority_change_scope.to_sql) + when 'info_request_event' + connection.select_rows(info_request_event_scope.to_sql) else - connection.select_rows("#{info_request_event_clause} + connection.select_rows("#{info_request_event_scope.unscope(:order).to_sql} UNION - #{authority_change_clause} + #{authority_change_scope.unscope(:order).to_sql} ORDER by timestamp desc") end end diff --git a/app/helpers/admin_general_timeline_helper.rb b/app/helpers/admin_general_timeline_helper.rb new file mode 100644 index 0000000000..c6fa3d70ef --- /dev/null +++ b/app/helpers/admin_general_timeline_helper.rb @@ -0,0 +1,36 @@ +## +# Helpers for rendering timeline filter buttons. Also used in the controller for +# generating the queries to load events. +# +module AdminGeneralTimelineHelper + def start_date + params[:start_date]&.to_datetime || 2.days.ago + end + + def time_filters + { + 'Hour' => 1.hour.ago, + 'Day' => 1.day.ago, + '2 days' => 2.days.ago, + 'Week' => 1.week.ago, + 'Month' => 1.month.ago, + 'All time' => Time.utc(1970, 1, 1) + } + end + + def current_time_filter + time_filters.min_by { |_, time| (time - start_date).abs }.first + end + + def event_types + { + authority_change: 'Authority changes', + info_request_event: 'Request events', + all: 'All events' + } + end + + def current_event_type + event_types[params[:event_type]&.to_sym] || event_types[:all] + end +end diff --git a/app/views/admin_general/timeline.html.erb b/app/views/admin_general/timeline.html.erb index 1983799f42..26bf5a5399 100644 --- a/app/views/admin_general/timeline.html.erb +++ b/app/views/admin_general/timeline.html.erb @@ -1,23 +1,18 @@ <% @title = "Timeline" %>
- <%= link_to "Hour", admin_timeline_path(:hour => 1), :class => "btn" %> - <%= link_to "Day", admin_timeline_path(:day => 1), :class => "btn" %> - <%= link_to "2 days", admin_timeline_path, :class => "btn" %> - <%= link_to "Week", admin_timeline_path(:week => 1), :class => "btn" %> - <%= link_to "Month", admin_timeline_path(:month => 1), :class => "btn" %> - <%= link_to "All time", admin_timeline_path(:all => 1), :class => "btn" %> + <% time_filters.each do |time_label, date| %> + <%= link_to time_label, + admin_timeline_path(start_date: date&.utc&.iso8601, event_type: params[:event_type]), + class: "btn #{'active' if current_time_filter == time_label}" %> + <% end %>
- <%= link_to 'Authority changes', - { event_type: 'authority_change' }, - class: 'btn' %> - <%= link_to 'Request events', - { event_type: 'info_request_event' }, - class: 'btn' %> - <%= link_to 'All', - { event_type: nil }, - class: 'btn' %> + <% event_types.each do |event_type, event_label| %> + <%= link_to event_label, + admin_timeline_path(start_date: params[:start_date], event_type: event_type), + class: "btn #{'active' if current_event_type == event_label}" %> + <% end %>
diff --git a/spec/controllers/admin_general_controller_spec.rb b/spec/controllers/admin_general_controller_spec.rb index 8fd21e3564..4558fd5434 100644 --- a/spec/controllers/admin_general_controller_spec.rb +++ b/spec/controllers/admin_general_controller_spec.rb @@ -339,16 +339,25 @@ end it 'sets the title appropriately' do - expect(assigns[:events_title]).to eq("Events, all time") + expect(assigns[:events_title]).to eq("All events in the last 2 days") + end + + context 'when start_date is set' do + + before do + get :timeline, params: { all: 1, start_date: Time.utc(1970, 1, 1) } + end + + it 'sets the title appropriately' do + expect(assigns[:events_title]).to eq("All events, all time") + end + end context 'when event_type is info_request_event' do before do - get :timeline, params: { - all: 1, - event_type: 'info_request_event' - } + get :timeline, params: { all: 1, event_type: 'info_request_event' } end it 'assigns an array of info request events in order of descending @@ -358,7 +367,9 @@ end it 'sets the title appropriately' do - expect(assigns[:events_title]).to eq("Request events, all time") + expect(assigns[:events_title]).to eq( + "Request events in the last 2 days" + ) end end @@ -375,7 +386,9 @@ end it 'sets the title appropriately' do - expect(assigns[:events_title]).to eq("Authority changes, all time") + expect(assigns[:events_title]).to eq( + "Authority changes in the last 2 days" + ) end end