From 99b7508ad959b6311c7ce57c16e02de61ffca14f Mon Sep 17 00:00:00 2001 From: Adam Jazairi Date: Fri, 25 Feb 2022 17:45:46 -0500 Subject: [PATCH] Add holds by source report Why these changes are being introduced: We need to be able to see the holds for a given degree period, filtered by hold source. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/ETD-462 How this addresses that need: This adds a report of holds that allows filtering on thesis grad date and hold source. Side effects of this change: * Report#extract_terms now checks for the type of items in the collection, so as to accommodate non-thesis collections. * Performance may be slow due to possible N+1 queries. We do not have the test data to confirm this, but we will make changes as needed depending on its behavior in production. Co-Authored-By: Jeremy Prevost --- app/controllers/report_controller.rb | 12 ++ app/helpers/hold_helper.rb | 20 +++- app/models/ability.rb | 1 + app/models/report.rb | 8 +- app/views/report/_hold.html.erb | 14 +++ app/views/report/_hold_empty.html.erb | 3 + .../report/_holds_by_source_filter.html.erb | 31 +++++ app/views/report/holds_by_source.html.erb | 32 +++++ app/views/shared/_report_submenu.html.erb | 3 + config/routes.rb | 1 + test/controllers/report_controller_test.rb | 111 +++++++++++++++++- 11 files changed, 231 insertions(+), 5 deletions(-) create mode 100644 app/views/report/_hold.html.erb create mode 100644 app/views/report/_hold_empty.html.erb create mode 100644 app/views/report/_holds_by_source_filter.html.erb create mode 100644 app/views/report/holds_by_source.html.erb diff --git a/app/controllers/report_controller.rb b/app/controllers/report_controller.rb index b04a046d..957ffab5 100644 --- a/app/controllers/report_controller.rb +++ b/app/controllers/report_controller.rb @@ -5,6 +5,7 @@ class ReportController < ApplicationController protect_from_forgery with: :exception include ThesisHelper + include HoldHelper def empty_theses term = params[:graduation] ? params[:graduation].to_s : 'all' @@ -49,6 +50,17 @@ def student_submitted_theses @list = report.list_student_submitted_metadata subset end + def holds_by_source + term = params[:graduation] ? params[:graduation].to_s : 'all' + @this_term = 'all terms' + @this_term = term.in_time_zone('Eastern Time (US & Canada)').strftime('%b %Y') if term != 'all' + holds = Hold.all.includes(:thesis).includes(:hold_source).includes(thesis: :users).includes(thesis: :authors) + @terms = Report.new.extract_terms holds + @hold_sources = HoldSource.pluck(:source).uniq.sort + term_filtered = filter_holds_by_term holds + @list = filter_holds_by_source term_filtered + end + def index report = Report.new @terms = Thesis.pluck(:grad_date).uniq.sort diff --git a/app/helpers/hold_helper.rb b/app/helpers/hold_helper.rb index 3451d347..64aa634f 100644 --- a/app/helpers/hold_helper.rb +++ b/app/helpers/hold_helper.rb @@ -4,12 +4,28 @@ def render_hold_history_field(field_name, field_value) field_value = 'n/a' if field_value.nil? # For foreign key fields, return a link to the record - if field_name == 'thesis_id' && thesis = Thesis.find_by(id: field_value) + if field_name == 'thesis_id' && (thesis = Thesis.find_by(id: field_value)) field_value = link_to thesis.title, admin_thesis_path(field_value) - elsif field_name == 'hold_source_id' && hold_source = HoldSource.find_by(id: field_value) + elsif field_name == 'hold_source_id' && (hold_source = HoldSource.find_by(id: field_value)) field_value = link_to hold_source.source, admin_hold_source_path(field_value) end field_value end + + def filter_holds_by_term(holds) + if params[:graduation] && params[:graduation] != 'all' + return holds.where('theses.grad_date = ?', params[:graduation]).references(:thesis) + end + + holds + end + + def filter_holds_by_source(holds) + if params[:hold_source] && params[:hold_source] != 'all' + return holds.where('hold_sources.source = ?', params[:hold_source]).references(:thesis) + end + + holds + end end diff --git a/app/models/ability.rb b/app/models/ability.rb index ff31bd40..b335b5e8 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -59,6 +59,7 @@ def processor can :term, Report can :empty_theses, Report can :expired_holds, Report + can :holds_by_source, Report can :student_submitted_theses, Report can %i[read update], Thesis diff --git a/app/models/report.rb b/app/models/report.rb index 85a91454..dea155bf 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -290,8 +290,14 @@ def empty_theses_record(collection) result end + # This assumes a couple of things: first, that all items in a collection should be instances of the same model, and + # second, that the model of any non-thesis collections belongs_to the Thesis model. def extract_terms(collection) - collection.pluck(:grad_date).uniq.sort + if collection.first.is_a? Thesis + collection.pluck(:grad_date).uniq.sort + else + collection.includes(:thesis).pluck(:grad_date).uniq.sort + end end def record_empty_theses(collection) diff --git a/app/views/report/_hold.html.erb b/app/views/report/_hold.html.erb new file mode 100644 index 00000000..5d0efba1 --- /dev/null +++ b/app/views/report/_hold.html.erb @@ -0,0 +1,14 @@ + + <%= link_to title_helper(hold.thesis), edit_admin_thesis_path(hold.thesis.id) %> + + <% hold.thesis.authors.each do |author| %> + <%= author.user.display_name %>
+ <% end %> + + <%= hold.thesis.graduation_month[...3] %> <%= hold.thesis.graduation_year %> + <%= hold.hold_source.source %> + <%= hold.status %> + <%= hold.date_requested %> + <%= hold.date_start %> + <%= hold.date_end %> + diff --git a/app/views/report/_hold_empty.html.erb b/app/views/report/_hold_empty.html.erb new file mode 100644 index 00000000..c1b238ed --- /dev/null +++ b/app/views/report/_hold_empty.html.erb @@ -0,0 +1,3 @@ + + There are no holds for the given term. + diff --git a/app/views/report/_holds_by_source_filter.html.erb b/app/views/report/_holds_by_source_filter.html.erb new file mode 100644 index 00000000..44d3bcd9 --- /dev/null +++ b/app/views/report/_holds_by_source_filter.html.erb @@ -0,0 +1,31 @@ +<%= form_tag(nil, method: "get") do %> +
+ +
+ +
+ +
+ + <%= submit_tag('Apply filters', class: 'btn button-primary') %> +<% end %> diff --git a/app/views/report/holds_by_source.html.erb b/app/views/report/holds_by_source.html.erb new file mode 100644 index 00000000..9df55219 --- /dev/null +++ b/app/views/report/holds_by_source.html.erb @@ -0,0 +1,32 @@ +<%= content_for(:title, "Thesis Reporting | MIT Libraries") %> + +
+
+

Holds by source for <%= @this_term %>

+ + <%= render 'holds_by_source_filter' %> + + + + + + + + + + + + + + + + <%= render(partial: 'hold', collection: @list) || render('hold_empty') %> + +
TitleAuthor(s)Grad dateHold sourceStatusDate requestedStart dateEnd date
+
+ + +
diff --git a/app/views/shared/_report_submenu.html.erb b/app/views/shared/_report_submenu.html.erb index 07ab5805..81e345bf 100644 --- a/app/views/shared/_report_submenu.html.erb +++ b/app/views/shared/_report_submenu.html.erb @@ -20,6 +20,9 @@ <% if can?(:files, Report) %>
  • <%= link_to("Files without purpose", report_files_path) %>
    • <% end %> + <% if can?(:holds_by_source, Report) %> +
  • <%= link_to("Holds by source", report_holds_by_source_path) %>
    • + <% end %> <% if can?(:expired_holds, Report) %>
  • <%= link_to("Unreleased holds which have ended", report_expired_holds_path) %>
    • <% end %> diff --git a/config/routes.rb b/config/routes.rb index 1a68e659..cedaee42 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -24,6 +24,7 @@ get 'report/empty_theses', to: 'report#empty_theses', as: 'report_empty_theses' get 'report/expired_holds', to: 'report#expired_holds', as: 'report_expired_holds' get 'report/files', to: 'report#files', as: 'report_files' + get 'report/holds_by_source', to: 'report#holds_by_source', as: 'report_holds_by_source' get 'report/proquest_files', to: 'report#proquest_files', as: 'report_proquest_files' get 'report/student_submitted_theses', to: 'report#student_submitted_theses', as: 'report_student_submitted_theses' get 'report/term', to: 'report#term', as: 'report_term' diff --git a/test/controllers/report_controller_test.rb b/test/controllers/report_controller_test.rb index 74482004..4c1f72ed 100644 --- a/test/controllers/report_controller_test.rb +++ b/test/controllers/report_controller_test.rb @@ -342,7 +342,8 @@ def create_thesis_with_whodunnit(user) test 'default files report is empty' do sign_in users(:processor) get report_files_path - assert_select 'table tbody td', text: 'There are no files without an assigned purpose within the selected term.', count: 1 + assert_select 'table tbody td', text: 'There are no files without an assigned purpose within the selected term.', + count: 1 end test 'files have no default purpose, and appear on the files report' do @@ -362,7 +363,8 @@ def create_thesis_with_whodunnit(user) get report_files_path assert_select 'table tbody td', text: 'a_pdf.pdf', count: 2 get report_files_path, params: { graduation: '2018-09-01' } - assert_select 'table tbody td', text: 'There are no files without an assigned purpose within the selected term.', count: 1 + assert_select 'table tbody td', text: 'There are no files without an assigned purpose within the selected term.', + count: 1 end test 'files disappear from files report when a purpose is set' do @@ -561,4 +563,109 @@ def create_thesis_with_whodunnit(user) get report_student_submitted_theses_path, params: { graduation: '2018-09-01' } assert_select 'table tbody td', text: 'There are no student-submitted theses for the given term.', count: 1 end + + # ~~~~~~~~~~~~~~~ Holds by source report ~~~~~~~~~~~~~~~~ + test 'anonymous users cannot see holds by source report' do + # Note that nobody is signed in. + get report_holds_by_source_path + assert_response :redirect + end + + test 'basic users cannot see holds by source report' do + sign_in users(:basic) + get report_holds_by_source_path + assert_redirected_to '/' + follow_redirect! + assert_select 'div.alert', text: 'Not authorized.', count: 1 + end + + test 'submitters cannot see holds by source report' do + sign_in users(:transfer_submitter) + get report_holds_by_source_path + assert_redirected_to '/' + follow_redirect! + assert_select 'div.alert', text: 'Not authorized.', count: 1 + end + + test 'processors can see holds by sourcereport' do + sign_in users(:processor) + get report_holds_by_source_path + assert_response :success + end + + test 'thesis_admins can see holds by source report' do + sign_in users(:thesis_admin) + get report_holds_by_source_path + assert_response :success + end + + test 'admins can see holds by source report' do + sign_in users(:admin) + get report_holds_by_source_path + assert_response :success + end + + # ~~~~~~~~~~~~~~~ Holds by source report ~~~~~~~~~~~~~~~~ + test 'holds by source report shows holds with any source by default' do + hold_count = Hold.all.count + + sign_in users(:processor) + get report_holds_by_source_path + assert_select 'table tbody tr', count: hold_count + end + + test 'holds by source report allows filtering by term' do + hold_count = Hold.all.count + term_count = Hold.joins(:thesis).where('theses.grad_date = ?', '2017-09-01').count + assert_not_equal hold_count, term_count + + # Add 1 to the select counts for the 'All terms'/'All sources' options + term_select_count = Report.new.extract_terms(Hold.all).count + 1 + + sign_in users(:processor) + get report_holds_by_source_path + assert_select 'table tbody tr', count: hold_count + assert_select 'select[name="graduation"] option', count: term_select_count + + # Now request the queue with a filter applied, and see the correct record count + get report_holds_by_source_path, params: { graduation: '2017-09-01' } + assert_select 'table tbody tr', count: term_count + end + + test 'holds by source report allows filtering by hold source' do + hold_count = Hold.all.count + source_count = Hold.where(hold_source_id: "#{hold_sources(:tlo).id}").count + + # Add 1 to the select counts for the 'All terms'/'All sources' options + source_select_count = HoldSource.pluck(:source).uniq.count + 1 + + sign_in users(:processor) + get report_holds_by_source_path + assert_select 'table tbody tr', count: hold_count + assert_select 'select[name="hold_source"] option', count: source_select_count + + # Now request the queue with a filter applied, and the correct record count + get report_holds_by_source_path, params: { hold_source: 'technology licensing office' } + assert_select 'table tbody tr', count: source_count + end + + test 'holds by source report allows filtering by both term and publication status' do + hold_count = Hold.all.count + source_count = Hold.joins(:thesis).where('theses.grad_date = ?', '2017-09-01') + .where(hold_source_id: "#{hold_sources(:tlo).id}").count + + # Add 1 to the select counts for the 'All terms'/'All sources' options + term_select_count = Report.new.extract_terms(Hold.all).count + 1 + source_select_count = HoldSource.pluck(:source).uniq.count + 1 + + sign_in users(:processor) + get report_holds_by_source_path + assert_select 'table tbody tr', count: hold_count + assert_select 'select[name="graduation"] option', count: term_select_count + assert_select 'select[name="hold_source"] option', count: source_select_count + + # Now request the queue with both filters applied, and see the correct record count + get report_holds_by_source_path, params: { graduation: '2017-09-01', hold_source: 'technology licensing office' } + assert_select 'table tbody tr', count: source_count + end end