Fix display behavior of metadata-only collections #480

VivianChu · 2018-07-11T16:00:28Z

Fixes #466

Local Checklist

What does this PR do?

Fix display behavior for both simple and complex metadata-only objects.
Also fix the search results generic icon for the metadata-only objects in public collections.

Why are we doing this? Any context of related work?

Currently on staging, these collections and objects are not behaving as expected. If I am on campus, I am seeing metadata-only behavior.

References #466

@ucsdlib/developers - please review

mcritchlow

This generally looks good to me 👍 , though I defer to @lsitu for review on the various use cases, since I know you all have been working closely on this together.

VivianChu · 2018-07-11T16:32:55Z

Thanks @mcritchlow for reviewing. I forgot to submit other tests for search results. I just submitted them now.

Fix display behavior for simple metadata-only object Remove un-used code Code cleanup Add tests for complex object Remove debug message Add tests for metadata-only object search Add tests for metadata-only public collection

coveralls · 2018-07-11T18:43:01Z

Coverage decreased (-0.08%) to 65.887% when pulling 0eb6ed2 on feature/metadata-only into d1c5b19 on develop.

houndci-bot · 2018-07-12T19:34:02Z

lib/dams/controller_helper.rb

@@ -903,6 +903,16 @@ def metadata_display?(data)
      data.any? { |t| t.include?('localDisplay') || t.include?('metadataDisplay') }
    end

+    def local_user_public_col?(document)
+      return false unless !current_user.nil?


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

VivianChu · 2018-07-13T14:31:59Z

@ucsdlib/developers - The code has been updated and more tests have been added. Please review. Thanks

lsitu

@VivianChu It looks good. Just a few questions regarding ability check and access control to see whether there are some gaps there or not.

lsitu · 2018-07-13T15:29:35Z

app/helpers/dams_objects_helper.rb

@@ -788,7 +788,7 @@ def grabRestrictedText(data)
  def grab_access_text(document)
    out = []
    data = rights_data document
-    return nil unless metadata_display?(data) && cannot?(:read, document)
+    return nil unless metadata_display?(data) && (current_user.nil? || local_user_public_col?(@document))


I seems like using ability check should be more efficient and reliable here. I think if ability check is an issue, then we need to fix it in the access control level since it's really a matter to control object access.
What issue do you see with ability check cannot?(:read, document)?

@lsitu - I already tried different ability check including cannot?(:read, document) for all type of users and they all return false. I couldn't use it to check for local or public user access local or public collection.

@lsitu - here are my test results to print out that ability check
local user - public collection : cannot? read - returns false
local user - local collection : cannot? read - returns false
public user - local collection : cannot? read - returns false
public user - public collection : cannot? read - returns false
I need a check for local user to access public collection with metadatadisplay object.

@lsitu - You can try that condition for this test https://github.com/ucsdlib/damspas/pull/480/files#diff-bff6a4c6341ffbdd45f5cabab7685905R1037 and https://github.com/ucsdlib/damspas/pull/480/files#diff-bff6a4c6341ffbdd45f5cabab7685905R1121 Thanks

@VivianChu But either rspec or manual tests seems to be correct for localDisplay objects. So I am wondering whether there is an issue with setting up the test as what I'd seen before. Let me run your tests to see how it goes.

@VivianChu I see the problem. Your test at https://github.com/ucsdlib/damspas/pull/480/files#diff-bff6a4c6341ffbdd45f5cabab7685905R1037 is failed with ability check cannot?(:read, document) because the object you setup for the test is a public object (copyright_attributes: [{status: 'Public domain'}]: https://github.com/ucsdlib/damspas/pull/480/files#diff-bff6a4c6341ffbdd45f5cabab7685905R997). With the rules we'd setup in the past for access control, this is always a public object with no access control overriding. I don't know whether we'll have such a case in the future or not, but we may need a ticket to fix it at the access control level if this will become a real case. For now, I think you can change it to something like copyright_attributes: [{status: 'Unknown'}]. In this case, access override will take it's role to override it for metadataDisplay and your tests for public metadata-only accesses will pass.

@lsitu - I'll update it to use other copyright_attributes "Under copyright" and change it to use cannot? read. Thanks

lsitu · 2018-07-13T15:33:25Z

app/views/dams_objects/_complex_object_viewer.html.erb

@@ -15,7 +15,7 @@
    </div>
  </div>
 <% end %>
-<% access_notice = grab_access_text(@document) if cannot? :update, @document %>
+<% access_notice = grab_access_text(@document) if ( current_user.nil? || local_user_public_col?(@document)) %>


Can we try to use ability check cannot? :read, @document instead?

I tried that before and it failed

@lsitu - You can try again in your local to see if the tests pass for you.

@VivianChu I think setup the test object with copyright_attributes: [{status: 'Unknown'}] will fix the puzzle. See my comment above.

@lsitu - I tried both copyright_attributes: [{status: 'Under copyright.'}] and copyright_attributes: [{status: 'Unknown'}] and the test passed.

lsitu · 2018-07-13T15:36:39Z

app/views/dams_objects/_simple_object_viewer.html.erb

@@ -8,7 +8,9 @@
 	    </div>
    </div>
  <% end %>
-<% if cannot? :update, @document then %>
+
+<% if current_user.nil? || local_user_public_col?(@document) then %>


Same as above.

lsitu · 2018-07-13T15:38:41Z

app/views/dams_objects/metadata.html.erb

@@ -30,7 +30,7 @@
          <div class="simple-object"></div>
        <% end %>
 <%# END RESTRICTED NOTICE %>
-    <% if cannot? :update, @document then %>
+    <% if current_user.nil? || local_user_public_col?(@document) then %>


Same as above.

lsitu · 2018-07-13T15:51:23Z

app/controllers/catalog_controller.rb

@@ -320,7 +320,7 @@ def collection_search

  def metadata_only_collections
    meta_colls = []
-    params = { q: 'visibility_tesim:local', fq: 'type_tesim:Collection' }
+    params = { q: 'visibility_tesim:local OR visibility_tesim:public', fq: 'type_tesim:Collection' }


It seems like adding OR visibility_tesim:public could bring up most of the collections if not all, which is close to 200 now. Is there a way to avoid scooping the public collections up or not? Just curios.

@lsitu - let me try to filter the result. It's because they also want the support for public collection with metadataDisplay object.

@lsitu - I updated it to use sub-query. Could you review it? Thanks

@VivianChu LGTM 👍

hweng · 2018-07-13T16:37:05Z

@VivianChu Have a question for your updates - In the case that a record have both “dams:note => Culturally sensitive…”(or “dams:note =>Embargoed content”) AND “dams:otherRights => metadataDisplay” in the record(for example bb60078423, bb3789396) , Which text display would take the priority in the curator viewer?

VivianChu · 2018-07-13T16:42:16Z

@hweng - I don't know. Let ask Gabriela.
@gamontoya - What do you think about Huawei's question above? Thanks

hweng · 2018-07-13T17:56:43Z

@VivianChu The current behavior on production is that the Culturally sensitive or Embargoed content would take the priority in the curator viewer. But not sure about after your updates, that why I asked.

lsitu · 2018-07-13T18:41:17Z

👍

gamontoya · 2018-07-13T20:23:35Z

@VivianChu @hweng For Culturally Sensitive or Embargo w/metadata-only visibility, curators will never see the banners when logged in. Only non-curators will see the relevant banners.

hweng · 2018-07-13T21:39:31Z

@gamontoya Get it, Thanks!

hweng · 2018-07-13T21:39:51Z

👍

VivianChu · 2018-07-13T22:14:52Z

Thanks everyone for reviewing and @hweng for merging.

mcritchlow reviewed Jul 11, 2018

View reviewed changes

Update metadata-only behavior for public collection

42175f3

Fix display behavior for simple metadata-only object Remove un-used code Code cleanup Add tests for complex object Remove debug message Add tests for metadata-only object search Add tests for metadata-only public collection

VivianChu force-pushed the feature/metadata-only branch from 274fce4 to 42175f3 Compare July 11, 2018 17:12

VivianChu changed the title ~~Fix display behavior of metadata-only collections~~ [WIP] Fix display behavior of metadata-only collections Jul 11, 2018

Update restricted text display behavior and add more tests

027fdaf

houndci-bot reviewed Jul 12, 2018

View reviewed changes

Fix hound issue

6c8be2a

lsitu reviewed Jul 13, 2018

View reviewed changes

VivianChu changed the title ~~[WIP] Fix display behavior of metadata-only collections~~ Fix display behavior of metadata-only collections Jul 13, 2018

Update condition to check for restricted text

7a32ced

Update metadata only collections query

0eb6ed2

hweng merged commit 795d902 into develop Jul 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix display behavior of metadata-only collections #480

Fix display behavior of metadata-only collections #480

VivianChu commented Jul 11, 2018

mcritchlow left a comment

VivianChu commented Jul 11, 2018

coveralls commented Jul 11, 2018 •

edited

Loading

houndci-bot Jul 12, 2018

VivianChu commented Jul 13, 2018

lsitu left a comment

lsitu Jul 13, 2018

VivianChu Jul 13, 2018

VivianChu Jul 13, 2018 •

edited

Loading

VivianChu Jul 13, 2018

lsitu Jul 13, 2018

lsitu Jul 13, 2018

VivianChu Jul 13, 2018

lsitu Jul 13, 2018

VivianChu Jul 13, 2018

VivianChu Jul 13, 2018

lsitu Jul 13, 2018

VivianChu Jul 13, 2018

lsitu Jul 13, 2018

lsitu Jul 13, 2018

lsitu Jul 13, 2018

VivianChu Jul 13, 2018

VivianChu Jul 13, 2018

lsitu Jul 13, 2018

hweng commented Jul 13, 2018

VivianChu commented Jul 13, 2018

hweng commented Jul 13, 2018

lsitu commented Jul 13, 2018

gamontoya commented Jul 13, 2018

hweng commented Jul 13, 2018

hweng commented Jul 13, 2018

VivianChu commented Jul 13, 2018

Fix display behavior of metadata-only collections #480

Fix display behavior of metadata-only collections #480

Conversation

VivianChu commented Jul 11, 2018

Local Checklist

What does this PR do?

Why are we doing this? Any context of related work?

mcritchlow left a comment

Choose a reason for hiding this comment

VivianChu commented Jul 11, 2018

coveralls commented Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

VivianChu commented Jul 13, 2018

lsitu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivianChu Jul 13, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hweng commented Jul 13, 2018

VivianChu commented Jul 13, 2018

hweng commented Jul 13, 2018

lsitu commented Jul 13, 2018

gamontoya commented Jul 13, 2018

hweng commented Jul 13, 2018

hweng commented Jul 13, 2018

VivianChu commented Jul 13, 2018

coveralls commented Jul 11, 2018 •

edited

Loading

VivianChu Jul 13, 2018 •

edited

Loading