Skip to content

Commit

Permalink
Add a second argument to #document_partial_name for the current kind …
Browse files Browse the repository at this point in the history
…of partial we're rendering

This allows downstream implementors to override the calculated
 #document_partial_name on a per-partial basis. E.g. if you had a
heterogeneous index with some records with MODS and MARC data, you could
choose the appropriate rendering format for those records independent of
the regular format.
  • Loading branch information
cbeer committed Mar 11, 2014
1 parent 8cf317b commit 3386f73
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 7 deletions.
20 changes: 13 additions & 7 deletions app/helpers/blacklight/blacklight_helper_behavior.rb
Original file line number Diff line number Diff line change
Expand Up @@ -428,19 +428,25 @@ def document_index_path_templates

##
# Return a normalized partial name for rendering a single document
#
#
# @param [SolrDocument]
# @param [Symbol] base name for the partial
# @return [String]
def document_partial_name(document)
display_type = document[blacklight_config.view_config(:show).display_type_field]
def document_partial_name(document, base_name = nil)

This comment has been minimized.

Copy link
@jrochkind

jrochkind Mar 12, 2014

Member

I frequently override this method in my own apps, so this is backward compat thing where I need to make sure to change the method signature in my override too.

(Even though the second arg is optional, if other code tries to call my custom override with two args, and my custom override only takes one, that'll be a runtime error).

Such is life, it happens. But make sure it's in the release notes, please?

Also, maybe (I'm not sure) consider making the second arg an options hash (document_partial_name(doc, :base_name => whatever)), so that the method signature will never need to be changed again, with a backwards compat issue. I believe this method should be considered part of the BL 'api' and it will be common for apps to override it locally to control partial templates used.

This comment has been minimized.

Copy link
@cbeer

cbeer Mar 12, 2014

Author Member

We can also check the arity where we're calling it. I can do that.

I thought about a hash, but couldn't think of another value we'd want to pass in. I think if we get to that point, we can re-evaluate.

This comment has been minimized.

Copy link
@jrochkind

jrochkind Mar 12, 2014

Member

Cool, I trust ya on the second argument hash or not.

I'm not sure if checking arrity is worth the code complexity, but I guess it is if we need to be able to not break backwards compat. Otherwise just a release note warning you to change signature of any local overrides of this method. your call.

This comment has been minimized.

Copy link
@cbeer

cbeer Mar 12, 2014

Author Member

check out #835. I think that should fix your concern.

view_config = blacklight_config.view_config(:show)

display_type = if action_name and view_config.has_key? :"#{base_name}_display_type_field"
document[view_config[:"#{base_name}_display_type_field"]]
end

display_type ||= document[view_config.display_type_field]

return 'default' unless display_type
display_type = display_type.join(" ") if display_type.respond_to?(:join)
display_type ||= 'default'

# .to_s is necessary otherwise the default return value is not always a string
# using "_" as sep. to more closely follow the views file naming conventions
# parameterize uses "-" as the default sep. which throws errors
"#{display_type.gsub("-"," ")}".parameterize("_").to_s
Array(display_type).join(" ").gsub("-","_").parameterize("_")
end

##
Expand All @@ -466,7 +472,7 @@ def render_document_partials(doc, partials = [], locals ={})
# @param [String] base name for the partial
# @param [Hash] locales to pass through to the partials
def render_document_partial(doc, base_name, locals = {})
format = document_partial_name(doc)
format = document_partial_name(doc, base_name)

document_partial_path_templates.each do |str|
# XXX rather than handling this logic through exceptions, maybe there's a Rails internals method
Expand Down
47 changes: 47 additions & 0 deletions spec/helpers/blacklight_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -445,4 +445,51 @@ def mock_document_app_helper_url *args
expect(helper.should_show_spellcheck_suggestions? response).to be_false
end
end

describe "#document_partial_name" do
let(:blacklight_config) { Blacklight::Configuration.new }

before do
helper.stub(blacklight_config: blacklight_config)
end

context "with a solr document with empty fields" do
let(:document) { SolrDocument.new }
it "should be the default value" do
expect(helper.document_partial_name(document)).to eq 'default'
end
end

context "with a solr document with the display type field set" do
let(:document) { SolrDocument.new 'my_field' => 'xyz'}

before do
blacklight_config.show.display_type_field = 'my_field'
end

it "should use the value in the configured display type field" do
expect(helper.document_partial_name(document)).to eq 'xyz'
end

it "should use the value in the configured display type field if the action-specific field is empty" do
expect(helper.document_partial_name(document, :some_action)).to eq 'xyz'
end
end

context "with a solr doucment with an action-specific field set" do

let(:document) { SolrDocument.new 'my_field' => 'xyz', 'other_field' => 'abc' }

before do
blacklight_config.show.media_display_type_field = 'my_field'
blacklight_config.show.metadata_display_type_field = 'other_field'
end

it "should use the value in the action-specific fields" do
expect(helper.document_partial_name(document, :media)).to eq 'xyz'
expect(helper.document_partial_name(document, :metadata)).to eq 'abc'
end

end
end
end

0 comments on commit 3386f73

Please sign in to comment.