Fixes #410 - Add support for metadata-only to collection level view #425
Conversation
| @@ -40,6 +40,9 @@ def show | |||
| # generate facet collection list for collection page only | |||
| models = @document["active_fedora_model_ssi"] | |||
| if models.include?("DamsAssembledCollection") || models.include?("DamsProvenanceCollection") || models.include?("DamsProvenanceCollectionPart") | |||
| @res, @doc = get_search_results(:q=>"(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay) AND collections_tesim:#{params[:id]}", :rows => 1 ) | |||
| @metadata_only = @res.response['numFound'] > 0 ? true : false | |||
There was a problem hiding this comment.
Style/NumericPredicate: Use (@res.response['numFound']).positive? instead of @res.response['numFound'] > 0.
Style/RedundantConditional: This conditional expression can just be replaced by @res.response['numFound'] > 0.
| @@ -40,6 +40,9 @@ def show | |||
| # generate facet collection list for collection page only | |||
| models = @document["active_fedora_model_ssi"] | |||
| if models.include?("DamsAssembledCollection") || models.include?("DamsProvenanceCollection") || models.include?("DamsProvenanceCollectionPart") | |||
| @res, @doc = get_search_results(:q=>"(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay) AND collections_tesim:#{params[:id]}", :rows => 1 ) | |||
There was a problem hiding this comment.
Layout/IndentationWidth: Use 2 (not 4) spaces for indentation.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
Layout/SpaceAroundOperators: Surrounding space missing for operator =>.
Metrics/LineLength: Line is too long. [167/150]
Layout/SpaceInsideParens: Space inside parentheses detected.
Layout/TrailingWhitespace: Trailing whitespace detected.
VivianChu
force-pushed
the
feature/metadata-only-collectiion
branch
2 times, most recently
from
77cdc25
to
6830b10
Compare
| @@ -40,6 +40,9 @@ def show | |||
| # generate facet collection list for collection page only | |||
| models = @document["active_fedora_model_ssi"] | |||
| if models.include?("DamsAssembledCollection") || models.include?("DamsProvenanceCollection") || models.include?("DamsProvenanceCollectionPart") | |||
| @res, @doc = get_search_results(:q=>"(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay) AND collections_tesim:#{params[:id]}", :rows => 1 ) | |||
There was a problem hiding this comment.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
Layout/SpaceAroundOperators: Surrounding space missing for operator =>.
Metrics/LineLength: Line is too long. [165/150]
Layout/SpaceInsideParens: Space inside parentheses detected.
Layout/TrailingWhitespace: Trailing whitespace detected.
VivianChu
force-pushed
the
feature/metadata-only-collectiion
branch
2 times, most recently
from
f822418
to
95efdf1
Compare
|
@ucsdlib/developers - I just ran the circleci build again and the tests passed. Please review, comment, etc.. Thanks |
| @@ -7,11 +7,13 @@ | |||
| if accessGroup != nil | |||
|
|
|||
| if accessGroup.include?('public') | |||
| viewAccess = nil | |||
| elsif accessGroup.include?('local') | |||
| viewAccess = nil | |||
There was a problem hiding this comment.
what if instead of nil we called this public, or even make it a symbol? It seems like that's what nil is representing here.
Then below for conditonals you could say if viewAccess != :public, for example, instead of a nil check. There may be other downstream issues with this, just a thought.
There was a problem hiding this comment.
@mcritchlow - I'm not sure why we use nil before instead of public. @lsitu - Do you know why we use nil instead? I'm fine with either value - nil or public. What do you all think?
There was a problem hiding this comment.
@VivianChu I have no idea how nil is used here but I believe the purpose is just not to show the label for the public view. So I think either way will be fine.
There was a problem hiding this comment.
@lsitu - I used the symbol way that Matt suggested and the tests still passed locally.
| @@ -40,6 +40,10 @@ def show | |||
| # generate facet collection list for collection page only | |||
| models = @document["active_fedora_model_ssi"] | |||
| if models.include?("DamsAssembledCollection") || models.include?("DamsProvenanceCollection") || models.include?("DamsProvenanceCollectionPart") | |||
| other_rights_query = "(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay) AND collections_tesim:#{params[:id]}" | |||
| @res, @doc = get_search_results(q: other_rights_query, rows: 1) | |||
| @metadata_only = @res.response['numFound'].zero? ? false : true | |||
There was a problem hiding this comment.
Doesn't .zero? return true or false already? Do we need the ternary part? (? false : true)
There was a problem hiding this comment.
Oh i see what you're doing here, you actually want the inverse. nevermind :) I suppose an alternative to the ternary is @metadata_only = !@res.response['numFound'].zero? but that's pretty subjective as far as what's more readable..
There was a problem hiding this comment.
@mcritchlow I think your alternative is better. I'll update it to use it.
…l record view Fixes hound issue Fixes indentation Fixes hound violation Use alternative way to check for metadataonly collection
VivianChu
force-pushed
the
feature/metadata-only-collectiion
branch
from
95efdf1
to
77a039e
Compare
|
@mcritchlow - The code has been updated to use symbol public and for @metadata_only. |
Fixes #410 ; refs #410
@ucsdlib/developers - please review