Add Access note to browse by collection page for metadata-only collection #432

VivianChu · 2018-04-11T23:26:30Z

Fixes #409 ; refs #409

Display the access note for metadata-only view collections.

@ucsdlib/developers - please review

houndci-bot · 2018-04-11T23:26:42Z

app/helpers/catalog_helper.rb

    if accessGroup != nil

      if accessGroup.include?('public')
        viewAccess = nil
+      elsif accessGroup.include?('local') && metadata_colls.include?(document['id'])
+        viewAccess = 'Restricted View'


Naming/VariableName: Use snake_case for variable names.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

+    end
+    meta_colls    
+  end
+


Layout/TrailingWhitespace: Trailing whitespace detected.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

+      metadata_only = !collection_response.response['numFound'].zero?
+      meta_colls << doc['id_t'].to_s if metadata_only
+    end
+    meta_colls    


Layout/TrailingWhitespace: Trailing whitespace detected.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

+    other_rights_fquery = "(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay)"
+    response.docs.each do |doc|
+      collection_solr_params = { :q => "collections_tesim:#{doc['id'].to_s}", :fq => other_rights_fquery, :rows => 1, :spellcheck => "false" }
+      collection_response = raw_solr( collection_solr_params )


Layout/SpaceInsideParens: Space inside parentheses detected.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

+    response = raw_solr( params )
+    other_rights_fquery = "(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay)"
+    response.docs.each do |doc|
+      collection_solr_params = { :q => "collections_tesim:#{doc['id'].to_s}", :fq => other_rights_fquery, :rows => 1, :spellcheck => "false" }


Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

+    meta_colls = []
+    params = {:q => 'visibility_tesim:local', :fq => 'type_tesim:Collection', :spellcheck => "false"}
+    response = raw_solr( params )
+    other_rights_fquery = "(otherRights_tesim:localDisplay OR otherRights_tesim:metadataDisplay)"


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

+  def metadata_only_collections
+    meta_colls = []
+    params = {:q => 'visibility_tesim:local', :fq => 'type_tesim:Collection', :spellcheck => "false"}
+    response = raw_solr( params )


Layout/SpaceInsideParens: Space inside parentheses detected.

houndci-bot · 2018-04-11T23:26:42Z

app/controllers/catalog_controller.rb

@@ -362,6 +362,20 @@ def collection_search
    session[:search] = search
  end

+  def metadata_only_collections
+    meta_colls = []
+    params = {:q => 'visibility_tesim:local', :fq => 'type_tesim:Collection', :spellcheck => "false"}


Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

houndci-bot · 2018-04-11T23:44:17Z

app/helpers/catalog_helper.rb

        viewAccess = nil
-      elsif accessGroup.include?('local')
+      elsif access_group.include?('local') && metadata_colls.include?(document['id'])
+        viewAccess = 'Restricted View'


Naming/VariableName: Use snake_case for variable names.

houndci-bot · 2018-04-11T23:44:17Z

app/helpers/catalog_helper.rb

-    if accessGroup != nil
-
-      if accessGroup.include?('public')
+      if access_group.include?('public')


Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.

houndci-bot · 2018-04-11T23:44:17Z

app/helpers/catalog_helper.rb

    viewAccess = nil
+    if access_group != nil


Style/NonNilCheck: Prefer !expression.nil? over expression != nil.

houndci-bot · 2018-04-11T23:53:47Z

app/helpers/catalog_helper.rb

+  def display_access_control_level(document, metadata_colls)
+    access_group = document['read_access_group_ssim'] # "public" > "local" > "dams-curator" == "dams-rci" == default
+    view_access = nil
+    if !access_group.nil?


Style/NegatedIf: Favor unless over if for negative conditions.

+1, I think this could be if access_group, or simply an early return if it is nil.

coveralls · 2018-04-12T00:11:56Z

Coverage increased (+0.2%) to 66.242% when pulling 541ab00 on feature/browse_collection_metadataonly into 84da581 on develop.

VivianChu · 2018-04-26T16:08:15Z

@ucsdlib/developers - please review

mcritchlow

Overall, LGTM 👍

A couple questions, but the implementation looks good. I am curious how much these queries will slow things down over time, hopefully not too much and not much we can do about it at this point anyway :)

mcritchlow · 2018-04-26T16:38:01Z

app/helpers/catalog_helper.rb

-        viewAccess = 'Restricted to UC San Diego use only'
+  def display_access_control_level(document, metadata_colls)
+    access_group = document['read_access_group_ssim'] # "public" > "local" > "dams-curator" == "dams-rci" == default
+    view_access = nil


It looks like the fallback value here, from the else condition, is Curator Only. If that's true, could you possibly set the initial value of view_access to that instead of nil? Then you could drop the else entirely.

Somewhat related I'm curious why we use nil instead of just '' for public, but if it's working then I suppose it's not work messing with ;)

mcritchlow · 2018-04-26T16:41:21Z

app/helpers/catalog_helper.rb

+  def display_access_control_level(document, metadata_colls)
+    access_group = document['read_access_group_ssim'] # "public" > "local" > "dams-curator" == "dams-rci" == default
+    view_access = nil
+    if !access_group.nil?


+1, I think this could be if access_group, or simply an early return if it is nil.

mcritchlow · 2018-04-26T16:42:08Z

.rubocop.yml

 Style/Next:
  Enabled: true

+Style/NegatedIf:
+  Exclude:
+    - 'app/helpers/*'


If the negated condition noted by Hound is dropped (turned into an else or early return/guard clause) will this still be needed?

VivianChu · 2018-04-26T17:30:56Z

@mcritchlow - I updated the code and removed the negated condition from rubocop like you suggested and Hound didn't complain. The circleci build is still running. Thanks

…ion page Fix hound violations Fix hound violation Fix hound violation Fix hound issue Update rubocop Fix access control failed tests

VivianChu · 2018-04-26T22:04:58Z

@mcritchlow - The code has been updated and the tests passed. Could you review them again? Thanks

mcritchlow · 2018-04-26T22:06:49Z

LGTM 👍

hweng · 2018-04-26T22:17:25Z

👍

houndci-bot reviewed Apr 11, 2018

View reviewed changes

VivianChu force-pushed the feature/browse_collection_metadataonly branch from 1686261 to a0e9e46 Compare April 11, 2018 23:53

houndci-bot reviewed Apr 11, 2018

View reviewed changes

mcritchlow reviewed Apr 26, 2018

View reviewed changes

Add Access note for metadata-only collection to the browse by collect…

541ab00

…ion page Fix hound violations Fix hound violation Fix hound violation Fix hound issue Update rubocop Fix access control failed tests

VivianChu force-pushed the feature/browse_collection_metadataonly branch from 46f3e68 to 541ab00 Compare April 26, 2018 21:18

hweng merged commit 5438dca into develop Apr 26, 2018

hweng deleted the feature/browse_collection_metadataonly branch April 26, 2018 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Access note to browse by collection page for metadata-only collection #432

Add Access note to browse by collection page for metadata-only collection #432

VivianChu commented Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

houndci-bot Apr 11, 2018

mcritchlow Apr 26, 2018

coveralls commented Apr 12, 2018 •

edited

Loading

VivianChu commented Apr 26, 2018

mcritchlow left a comment

mcritchlow Apr 26, 2018

mcritchlow Apr 26, 2018

mcritchlow Apr 26, 2018

VivianChu commented Apr 26, 2018 •

edited

Loading

VivianChu commented Apr 26, 2018

mcritchlow commented Apr 26, 2018

hweng commented Apr 26, 2018

Add Access note to browse by collection page for metadata-only collection #432

Add Access note to browse by collection page for metadata-only collection #432

Conversation

VivianChu commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 12, 2018 • edited Loading

VivianChu commented Apr 26, 2018

mcritchlow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivianChu commented Apr 26, 2018 • edited Loading

VivianChu commented Apr 26, 2018

mcritchlow commented Apr 26, 2018

hweng commented Apr 26, 2018

coveralls commented Apr 12, 2018 •

edited

Loading

VivianChu commented Apr 26, 2018 •

edited

Loading