From 05fcabe6587850fe1c714360f125acb6da05d774 Mon Sep 17 00:00:00 2001 From: "Brian \"Moses\" Hall" Date: Wed, 7 Jan 2026 12:11:53 -0500 Subject: [PATCH 1/3] ETT-1142 OTIS download report: filters with dropdown menus should include all possible values for that field - Proof of concept which does not update values based on other popup menu selections (will test for viability) - Add `HTDownload` method `all_values` called by the presenter to populate the menus --- app/models/ht_download.rb | 19 ++++++++++ app/presenters/ht_download_presenter.rb | 32 ++++++++++------ spec/models/ht_download_spec.rb | 12 ++++++ spec/presenters/ht_download_presenter_spec.rb | 37 +++++++++++++++++++ 4 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 spec/presenters/ht_download_presenter_spec.rb diff --git a/app/models/ht_download.rb b/app/models/ht_download.rb index 7789991..140344b 100644 --- a/app/models/ht_download.rb +++ b/app/models/ht_download.rb @@ -21,6 +21,25 @@ def self.ransackable_associations(auth_object = nil) ["ht_hathifile", "ht_institution"] end + # Needed by the presenter for assembling the data for dropdown menus + # that always have all possible values, not just the ones on the current page. + def self.all_values(field) + case field + when :rights_code, :content_provider_code, :digitization_agent_code, :rights_date_used + joins(:ht_hathifile).distinct.pluck(field) + when :role, :inst_code, :pages + distinct.pluck(field) + when :institution_name + joins(:ht_institution).distinct.pluck(:name) + # TODO: upcoming schema change in ETT-745 should simplify this. + # `map` to invert the Boolean to change partial -> full. + when :full_download + distinct.pluck(:is_partial).map { |value| !value } + else + raise "no all_values for field #{field}" + end + end + def institution_name institution&.name end diff --git a/app/presenters/ht_download_presenter.rb b/app/presenters/ht_download_presenter.rb index 207bd9f..65d41f7 100644 --- a/app/presenters/ht_download_presenter.rb +++ b/app/presenters/ht_download_presenter.rb @@ -57,20 +57,30 @@ def self.data_filter_control(field) DATA_FILTER_CONTROLS[field].to_s end - # Only needed for role because displayed value != database value. - # Maps display value and query values. + # Display all possible values for a given column that has a popup menu + # Maps display value and query values (which are the same except for role). # Should not be memoized, locale is variable. + # Returns a hash of { "search_value" => "Display Value", ... } def self.data_filter_data(field) - return unless field == :role - - map = {} - # TODO: this is a little clunky, could extract a class method from - # `ApplicationPresenter#field_value` to handle this kind of mucking - # about with scopes. - [:ssdproxy, :resource_sharing].each do |value| - map[value] = I18n.t(value_scope + ".#{field}.#{value}", raise: false) + return if DATA_FILTER_CONTROLS[field] != :select + + all_values = HTDownload.all_values(field) + all_values_map = all_values.to_h { |x| [x, x] } + # Postprocess role because displayed values are localized and anyway + # differ from the stored values. + if field == :role + all_values_map.each_key do |value| + all_values_map[value] = I18n.t(value_scope + ".#{field}.#{value}", raise: false) + end end - ("json:" + map.to_json).html_safe + if field == :full_download + # TODO: duplication with `show_full_download`, sidesteps localization. + # We have some overly complex ransacker plumbing in here just to support "yes" and "no" + # ETT-745 may give us the opportunity to return {0 => "no", 1 => "yes"} or {false => "no", true => "yes"} + # and get rid of the ransacker and some other goop. + all_values_map = all_values.to_h { |x| x ? ["yes", "yes"] : ["no", "no"] } + end + ("json:" + all_values_map.to_json).html_safe end # Some CSS in index.html.erb allows the title, imprint, and author fields to be a bit wider diff --git a/spec/models/ht_download_spec.rb b/spec/models/ht_download_spec.rb index 9fbe409..62b3171 100644 --- a/spec/models/ht_download_spec.rb +++ b/spec/models/ht_download_spec.rb @@ -30,6 +30,18 @@ end end + describe ".all_values" do + # Make sure we can get values for all of the columns that have a select control + HTDownloadPresenter::DATA_FILTER_CONTROLS.each_key do |field| + next if HTDownloadPresenter::DATA_FILTER_CONTROLS[field] != :select + context "with #{field}" do + it "returns an array of values" do + expect(described_class.all_values(field)).to be_a(Array) + end + end + end + end + describe ".ransackable_attributes" do it "returns an Array" do expect(described_class.ransackable_attributes).to be_a(Array) diff --git a/spec/presenters/ht_download_presenter_spec.rb b/spec/presenters/ht_download_presenter_spec.rb new file mode 100644 index 0000000..96121ef --- /dev/null +++ b/spec/presenters/ht_download_presenter_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +RSpec.describe HTDownloadPresenter do + around(:each) do |example| + HTDownload.delete_all + example.run + end + + describe ".data_filter_data" do + # Make sure we can get values for all of the columns that have a select control. + # TODO: this does not check that there is any particular content for any of the fields. + described_class::DATA_FILTER_CONTROLS.select { |_k, v| v == :select } + .each_key do |field| + context "with #{field}" do + it "returns a specially formatted JSON String" do + expect(described_class.data_filter_data(field)).to match(/^json:{.*?}$/) + end + end + end + + it "displays the correct value for role" do + create(:ht_download, role: :resource_sharing) do |download| + data = described_class.data_filter_data(:role) + data = JSON.parse(data.sub(/^json:/, "")) + expect(data).to eq({"resource_sharing" => "Resource Sharing"}) + end + end + + it "displays the correct value for full_download" do + create(:ht_download, is_partial: 1) do |download| + data = described_class.data_filter_data(:full_download) + data = JSON.parse(data.sub(/^json:/, "")) + expect(data).to eq({"no" => "no"}) + end + end + end +end From 04b1a79fd6d06e29adf4a809443ebbd7d74a32ec Mon Sep 17 00:00:00 2001 From: "Brian \"Moses\" Hall" Date: Wed, 7 Jan 2026 13:15:41 -0500 Subject: [PATCH 2/3] Additional tests --- spec/models/ht_download_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/models/ht_download_spec.rb b/spec/models/ht_download_spec.rb index 62b3171..88721cc 100644 --- a/spec/models/ht_download_spec.rb +++ b/spec/models/ht_download_spec.rb @@ -40,6 +40,14 @@ end end end + + context "with an unsupported field" do + it "raises" do + expect { + described_class.all_values("no such field") + }.to raise_error(StandardError) + end + end end describe ".ransackable_attributes" do @@ -113,4 +121,17 @@ expect(HTDownload.for_role("resource_sharing").size).to be(2) end end + + describe "ransacker" do + it "finds a record based on user selection" do + now = Time.now + create(:ht_download, datetime: now, is_partial: false) do |download| + downloads = described_class.ransack( + datetime_start: now.to_date.to_s, + full_download_eq: "yes" + ).result + expect(downloads.count).to eq(1) + end + end + end end From 65a1037919487eaa90523c9eb8b6880cf73fc04d Mon Sep 17 00:00:00 2001 From: "Brian \"Moses\" Hall" Date: Wed, 7 Jan 2026 13:58:34 -0500 Subject: [PATCH 3/3] Escape institutions name JSON so we can handle Queen's University --- app/views/ht_downloads/index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/ht_downloads/index.html.erb b/app/views/ht_downloads/index.html.erb index 0dfaf4c..b5eb2c0 100644 --- a/app/views/ht_downloads/index.html.erb +++ b/app/views/ht_downloads/index.html.erb @@ -123,7 +123,7 @@ data-filter-control="<%= control %>" <% if filter_data %> - <%= "data-filter-data='#{filter_data}'".html_safe %> + data-filter-data='<%= filter_data.gsub("'", "'").html_safe %>' <% end %> data-sortable="true"