From 631eac6150924b593b16b3463feba6208fb66535 Mon Sep 17 00:00:00 2001
From: Justin Coyne <jcoyne@justincoyne.com>
Date: Wed, 3 Oct 2018 15:54:38 -0500
Subject: [PATCH 1/2] Respect the type specific show field config

If you add configuration like:
```ruby
    config.for_display_type 'Person' do
      config.add_show_field 'name_tsim', label: 'Name'
    end
```

Then, that config should be used to draw the pages that have documents
with that type.
---
 .rubocop_todo.yml                                |  6 ++----
 .../blacklight/configuration_helper_behavior.rb  |  8 +++++---
 app/presenters/blacklight/index_presenter.rb     |  5 +++++
 app/presenters/blacklight/show_presenter.rb      |  5 +++++
 app/views/catalog/_index.html.erb                |  6 +++---
 app/views/catalog/_show.html.erb                 |  2 +-
 app/views/catalog/index.json.jbuilder            |  2 +-
 app/views/catalog/show.json.jbuilder             |  2 +-
 lib/blacklight/configuration.rb                  | 12 ++++++++++++
 .../blacklight/templates/catalog_controller.rb   |  4 ++--
 spec/models/blacklight/configuration_spec.rb     | 16 ++++++++++++++--
 .../{ => blacklight}/index_presenter_spec.rb     | 14 ++++++++++++--
 .../{ => blacklight}/show_presenter_spec.rb      | 14 ++++++++++++--
 13 files changed, 75 insertions(+), 21 deletions(-)
 rename spec/presenters/{ => blacklight}/index_presenter_spec.rb (95%)
 rename spec/presenters/{ => blacklight}/show_presenter_spec.rb (97%)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 746626e848..405909813b 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -202,8 +202,6 @@ Lint/UselessAssignment:
     - 'spec/helpers/blacklight/configuration_helper_behavior_spec.rb'
     - 'spec/helpers/catalog_helper_spec.rb'
     - 'spec/models/blacklight/solr/search_builder_spec.rb'
-    - 'spec/presenters/index_presenter_spec.rb'
-    - 'spec/presenters/show_presenter_spec.rb'
     - 'spec/services/blacklight/search_service_spec.rb'
     - 'spec/support/features/session_helpers.rb'
 
@@ -399,7 +397,7 @@ RSpec/IteratedExpectation:
 RSpec/MessageChain:
   Exclude:
     - 'spec/controllers/bookmarks_controller_spec.rb'
-    - 'spec/presenters/show_presenter_spec.rb'
+    - 'spec/presenters/blacklight/show_presenter_spec.rb'
     - 'spec/views/shared/_user_util_links.html.erb_spec.rb'
 
 # Offense count: 52
@@ -428,7 +426,7 @@ RSpec/RepeatedDescription:
     - 'spec/helpers/blacklight/facets_helper_behavior_spec.rb'
     - 'spec/helpers/catalog_helper_spec.rb'
     - 'spec/models/blacklight/solr/search_builder_spec.rb'
-    - 'spec/presenters/show_presenter_spec.rb'
+    - 'spec/presenters/blacklight/show_presenter_spec.rb'
 
 # Offense count: 4
 RSpec/RepeatedExample:
diff --git a/app/helpers/blacklight/configuration_helper_behavior.rb b/app/helpers/blacklight/configuration_helper_behavior.rb
index 566e63eb54..6f7471bbed 100644
--- a/app/helpers/blacklight/configuration_helper_behavior.rb
+++ b/app/helpers/blacklight/configuration_helper_behavior.rb
@@ -6,6 +6,7 @@ module Blacklight::ConfigurationHelperBehavior
   # @param [SolrDocument] document
   # @return [Array<Blacklight::Configuration::Field>]
   def index_fields _document = nil
+    Deprecation.warn(self, "index_fields is deprecated and will be removed in Blacklight 8. Use IndexPresenter#fields instead")
     blacklight_config.index_fields
   end
 
@@ -27,8 +28,9 @@ def search_field_options_for_select
     end.compact
   end
 
-  # used in the catalog/_show/_default partial
+  # used in the catalog/_show partial
   def document_show_fields _document = nil
+    Deprecation.warn(self, "document_show_fields is deprecated and will be removed in Blacklight 8. Use ShowPresenter#fields instead")
     blacklight_config.show_fields
   end
 
@@ -53,7 +55,7 @@ def default_search_field?(selected_search_field)
   ##
   # Look up the label for the index field
   def index_field_label document, field
-    field_config = index_fields(document)[field]
+    field_config = blacklight_config.index_fields_for(document)[field]
     field_config ||= Blacklight::Configuration::NullField.new(key: field)
 
     field_config.display_label('index')
@@ -62,7 +64,7 @@ def index_field_label document, field
   ##
   # Look up the label for the show field
   def document_show_field_label document, field
-    field_config = document_show_fields(document)[field]
+    field_config = blacklight_config.show_fields_for(document)[field]
     field_config ||= Blacklight::Configuration::NullField.new(key: field)
 
     field_config.display_label('show')
diff --git a/app/presenters/blacklight/index_presenter.rb b/app/presenters/blacklight/index_presenter.rb
index 3cdf443d0b..11739140cf 100644
--- a/app/presenters/blacklight/index_presenter.rb
+++ b/app/presenters/blacklight/index_presenter.rb
@@ -16,6 +16,11 @@ def initialize(document, view_context, configuration = view_context.blacklight_c
       @view_config = configuration.view_config(view_context.document_index_view_type)
     end
 
+    # @return [Hash<String,Configuration::Field>]
+    def fields
+      configuration.index_fields_for(document)
+    end
+
     ##
     # Render the document index heading
     #
diff --git a/app/presenters/blacklight/show_presenter.rb b/app/presenters/blacklight/show_presenter.rb
index a8b07be4d6..891c807e23 100644
--- a/app/presenters/blacklight/show_presenter.rb
+++ b/app/presenters/blacklight/show_presenter.rb
@@ -12,6 +12,11 @@ def initialize(document, view_context, configuration = view_context.blacklight_c
       @configuration = configuration
     end
 
+    # @return [Hash<String,Configuration::Field>]
+    def fields
+      configuration.show_fields_for(document)
+    end
+
     ##
     # Create <link rel="alternate"> links from a documents dynamically
     # provided export formats. Returns empty string if no links available.
diff --git a/app/views/catalog/_index.html.erb b/app/views/catalog/_index.html.erb
index acce4b41cc..b170e907e5 100644
--- a/app/views/catalog/_index.html.erb
+++ b/app/views/catalog/_index.html.erb
@@ -1,8 +1,8 @@
-<% doc_presenter = index_presenter(document) %> 
+<% doc_presenter = index_presenter(document) %>
 <%# default partial to display solr document fields in catalog index view -%>
 <dl class="document-metadata dl-invert row">
-  
-  <% index_fields(document).each do |field_name, field| -%>
+
+  <% doc_presenter.fields.each do |field_name, field| -%>
     <% if should_render_index_field? document, field %>
 	    <dt class="blacklight-<%= field_name.parameterize %> col-md-3"><%= render_index_field_label document, field: field_name %></dt>
 	    <dd class="blacklight-<%= field_name.parameterize %> col-md-9"><%= doc_presenter.field_value field_name %></dd>
diff --git a/app/views/catalog/_show.html.erb b/app/views/catalog/_show.html.erb
index 67e4349eb9..b5588687be 100644
--- a/app/views/catalog/_show.html.erb
+++ b/app/views/catalog/_show.html.erb
@@ -1,7 +1,7 @@
 <% doc_presenter = show_presenter(document) %>
 <%# default partial to display solr document fields in catalog show view -%>
 <dl class="row dl-invert document-metadata">
-  <% document_show_fields(document).each do |field_name, field| -%>
+  <% doc_presenter.fields.each do |field_name, field| -%>
     <% if should_render_show_field? document, field %>
 	    <dt class="blacklight-<%= field_name.parameterize %> col-md-3"><%= render_document_show_field_label document, field: field_name %></dt>
 	    <dd class="blacklight-<%= field_name.parameterize %> col-md-9"><%= doc_presenter.field_value field_name %></dd>
diff --git a/app/views/catalog/index.json.jbuilder b/app/views/catalog/index.json.jbuilder
index 866433764d..60a67211fe 100644
--- a/app/views/catalog/index.json.jbuilder
+++ b/app/views/catalog/index.json.jbuilder
@@ -18,7 +18,7 @@ json.data do
     json.attributes do
       doc_presenter = index_presenter(document)
 
-      index_fields(document).each do |field_name, field|
+      doc_presenter.fields.each do |field_name, field|
         next unless should_render_index_field? document, field
         json.set!(field_name) do
           json.id "#{document_url}##{field_name}"
diff --git a/app/views/catalog/show.json.jbuilder b/app/views/catalog/show.json.jbuilder
index a53c818879..5ea7a94ef6 100644
--- a/app/views/catalog/show.json.jbuilder
+++ b/app/views/catalog/show.json.jbuilder
@@ -10,7 +10,7 @@ json.data do
   json.attributes do
     doc_presenter = show_presenter(@document)
 
-    document_show_fields(@document).each do |field_name, field|
+    doc_presenter.fields.each do |field_name, field|
       if should_render_show_field? @document, field
         json.set! field_name, doc_presenter.field_value(field_name)
       end
diff --git a/lib/blacklight/configuration.rb b/lib/blacklight/configuration.rb
index e4b277bdf0..fc8a7cc7e9 100644
--- a/lib/blacklight/configuration.rb
+++ b/lib/blacklight/configuration.rb
@@ -81,6 +81,7 @@ def default_values
           show: ViewConfig::Show.new(
             # document presenter class used by helpers and views
             document_presenter_class: nil,
+            display_type_field: 'format',
             # Default route parameters for 'show' requests.
             # Set this to a hash with additional arguments to merge into the route,
             # or set `controller: :current` to route to the current controller.
@@ -341,11 +342,22 @@ def for_display_type display_type, &block
       fields_for_type[display_type] ||= self.class.new(&block)
     end
 
+    ##
+    # Return a list of fields for the index display that should be used for the
+    # provided document.  This respects any configuration made using for_display_type
     def index_fields_for(document)
       display_type = document.first(index.display_type_field)
       for_display_type(display_type).index_fields.merge(index_fields)
     end
 
+    ##
+    # Return a list of fields for the show page that should be used for the
+    # provided document.  This respects any configuration made using for_display_type
+    def show_fields_for(document)
+      display_type = document.first(show.display_type_field)
+      for_display_type(display_type).show_fields.merge(show_fields)
+    end
+
     private
 
     def add_action(config_hash, name, opts)
diff --git a/lib/generators/blacklight/templates/catalog_controller.rb b/lib/generators/blacklight/templates/catalog_controller.rb
index 32766af236..a2ae7447e1 100644
--- a/lib/generators/blacklight/templates/catalog_controller.rb
+++ b/lib/generators/blacklight/templates/catalog_controller.rb
@@ -27,7 +27,7 @@ class <%= controller_name.classify %>Controller < ApplicationController
 
     # solr field configuration for search results/index views
     config.index.title_field = 'title_tsim'
-    config.index.display_type_field = 'format'
+    #config.index.display_type_field = 'format'
     #config.index.thumbnail_field = 'thumbnail_path_ss'
 
     config.add_results_document_tool(:bookmark, partial: 'bookmark_control', if: :render_bookmarks_control?)
@@ -194,7 +194,7 @@ class <%= controller_name.classify %>Controller < ApplicationController
     # Configuration for autocomplete suggestor
     config.autocomplete_enabled = true
     config.autocomplete_path = 'suggest'
-    # if the name of the solr.SuggestComponent provided in your solrcongig.xml is not the 
+    # if the name of the solr.SuggestComponent provided in your solrcongig.xml is not the
     # default 'mySuggester', uncomment and provide it below
     # config.autocomplete_suggester = 'mySuggester'
   end
diff --git a/spec/models/blacklight/configuration_spec.rb b/spec/models/blacklight/configuration_spec.rb
index 0b8829777a..8fb1740192 100644
--- a/spec/models/blacklight/configuration_spec.rb
+++ b/spec/models/blacklight/configuration_spec.rb
@@ -84,11 +84,10 @@
   end
 
   describe "for_display_type" do
-    let(:mock_field) { Blacklight::Configuration::IndexField.new }
     let(:image) { SolrDocument.new(format: 'Image') }
     let(:sound) { SolrDocument.new(format: 'Sound') }
 
-    it "adds fields just for a certain type" do
+    it "adds index fields just for a certain type" do
       config.for_display_type "Image" do |c|
         c.add_index_field :dimensions
       end
@@ -100,6 +99,19 @@
       expect(config.index_fields_for(image)).to have_key 'title'
       expect(config.index_fields).not_to have_key 'dimensions'
     end
+
+    it "adds show fields just for a certain type" do
+      config.for_display_type "Image" do |c|
+        c.add_show_field :dimensions
+      end
+      config.add_show_field :title
+
+      expect(config.show_fields_for(image)).to have_key 'dimensions'
+      expect(config.show_fields_for(image)).to have_key 'title'
+      expect(config.show_fields_for(sound)).not_to have_key 'dimensions'
+      expect(config.show_fields_for(image)).to have_key 'title'
+      expect(config.show_fields).not_to have_key 'dimensions'
+    end
   end
 
   describe "inheritable_copy" do
diff --git a/spec/presenters/index_presenter_spec.rb b/spec/presenters/blacklight/index_presenter_spec.rb
similarity index 95%
rename from spec/presenters/index_presenter_spec.rb
rename to spec/presenters/blacklight/index_presenter_spec.rb
index 35c367392d..bec537c337 100644
--- a/spec/presenters/index_presenter_spec.rb
+++ b/spec/presenters/blacklight/index_presenter_spec.rb
@@ -131,8 +131,6 @@
           args.first
         end
 
-        render_options = { a: 1 }
-
         options = subject.send(:field_values, field_config, a: 1)
 
         expect(options).to include :document, :field, :value, :config, :a
@@ -145,6 +143,18 @@
     end
   end
 
+  describe '#fields' do
+    let(:field) { instance_double(Blacklight::Configuration::Field) }
+
+    before do
+      allow(config).to receive(:index_fields_for).and_return(title: field)
+    end
+
+    it 'returns the list from the configs' do
+      expect(subject.fields).to eq(title: field)
+    end
+  end
+
   describe "#thumbnail" do
     subject { presenter.thumbnail }
 
diff --git a/spec/presenters/show_presenter_spec.rb b/spec/presenters/blacklight/show_presenter_spec.rb
similarity index 97%
rename from spec/presenters/show_presenter_spec.rb
rename to spec/presenters/blacklight/show_presenter_spec.rb
index 39074e3c28..6d216eb98d 100644
--- a/spec/presenters/show_presenter_spec.rb
+++ b/spec/presenters/blacklight/show_presenter_spec.rb
@@ -225,6 +225,18 @@ def mock_document_app_helper_url *args
     end
   end
 
+  describe '#fields' do
+    let(:field) { instance_double(Blacklight::Configuration::Field) }
+
+    before do
+      allow(config).to receive(:show_fields_for).and_return(title: field)
+    end
+
+    it 'returns the list from the configs' do
+      expect(subject.fields).to eq(title: field)
+    end
+  end
+
   describe "#heading" do
     it "falls back to an id" do
       allow(document).to receive(:[]).with('id').and_return "xyz"
@@ -282,8 +294,6 @@ def mock_document_app_helper_url *args
           args.first
         end
 
-        render_options = { a: 1 }
-
         options = subject.send(:field_values, field_config, a: 1)
 
         expect(options).to include :document, :field, :value, :config, :a

From f14f1f0ec975270734c5fa7710f4ae4940536e29 Mon Sep 17 00:00:00 2001
From: Justin Coyne <jcoyne@justincoyne.com>
Date: Wed, 3 Oct 2018 17:47:32 -0500
Subject: [PATCH 2/2] Always yield the config to the block passed to
 for_display_type

---
 lib/blacklight/configuration.rb              |  7 +++++--
 spec/models/blacklight/configuration_spec.rb | 12 ++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/blacklight/configuration.rb b/lib/blacklight/configuration.rb
index fc8a7cc7e9..eb2e2dbab1 100644
--- a/lib/blacklight/configuration.rb
+++ b/lib/blacklight/configuration.rb
@@ -337,9 +337,12 @@ def add_nav_action(name, opts = {})
 
     ##
     # Add a section of config that only applies to documents with a matching display type
-    def for_display_type display_type, &block
+    def for_display_type display_type, &_block
       self.fields_for_type ||= {}
-      fields_for_type[display_type] ||= self.class.new(&block)
+
+      (fields_for_type[display_type] ||= self.class.new).tap do |conf|
+        yield(conf) if block_given?
+      end
     end
 
     ##
diff --git a/spec/models/blacklight/configuration_spec.rb b/spec/models/blacklight/configuration_spec.rb
index 8fb1740192..233251c10a 100644
--- a/spec/models/blacklight/configuration_spec.rb
+++ b/spec/models/blacklight/configuration_spec.rb
@@ -112,6 +112,18 @@
       expect(config.show_fields_for(image)).to have_key 'title'
       expect(config.show_fields).not_to have_key 'dimensions'
     end
+
+    it 'calls the block on subsequent invocations' do
+      config.for_display_type "Image" do |c|
+        c.add_show_field :dimensions
+      end
+      config.for_display_type "Image" do |c|
+        c.add_show_field :photographer
+      end
+
+      expect(config.show_fields_for(image)).to have_key 'dimensions'
+      expect(config.show_fields_for(image)).to have_key 'photographer'
+    end
   end
 
   describe "inheritable_copy" do