Reverse Collection membership #901

escowles · 2016-08-16T19:03:39Z

Improve scalability of Collections by linking from Objects to Collections (instead of linking from Collections to their member Objects).

This PR removes the existing functionality to link from Collections to Objects. Objects are typically members of one or a small number of Collections, but some Collections can have 10,000 or more member Objects. While there is ongoing work to address the issue, Fedora performance for Collections that link to that many Objects with pcdm:hasMember is poor, so this PR dramatically improves performance by avoiding that bottleneck.

@projecthydra/sufia-code-reviewers

jeremyf · 2016-08-17T15:45:47Z

app/controllers/concerns/curation_concerns/curation_concern_controller.rb

@@ -115,6 +115,7 @@ def form_class

    def build_form
      @form = form_class.new(curation_concern, current_ability)
+      @collections = Collection.all


The return value of this method has changed, which may or may not be an issue. I'm also concerned about adding another instance variable for the view to use.

Suggestion:

Add form_class#collections_for_select method that can be referenced in the view

escowles · 2016-08-17T17:37:34Z

Screenshots demonstrating new functionality:

Selecting one or more collections on the Work create or edit form:

Listing collections a Work is a member of on the Work show page:

Collection membership is a facet in search results:

escowles · 2016-08-17T18:42:36Z

@jeremyf thanks for your comments — I've pushed another commit to address them.

pgwillia · 2016-08-26T21:38:23Z

This is still preliminary for us but in order to get this to work for our sufia 7.1.0 based app, in our project's Gemfile we had to include

gem 'hydra-pcdm', github:'projecthydra/hydra-pcdm', branch: :master
gem 'hydra-works', github:'projecthydra/hydra-works', branch: :master

and make one more modification to curation_concerns

diff --git a/app/models/concerns/curation_concerns/collection.rb b/app/models/concerns/curation_concerns/
index 2c948c9..5b4ab12 100644
--- a/app/models/concerns/curation_concerns/collection.rb
+++ b/app/models/concerns/curation_concerns/collection.rb
@@ -9,6 +9,11 @@ module CurationConcerns
     def add_members(new_member_ids)
       return if new_member_ids.nil? || new_member_ids.empty?
       members << ActiveFedora::Base.find(new_member_ids)
+      members.each do |m|
+        m.member_of_collections << self 
+        m.member_of_collection_ids << id
+        m.save!
+      end
     end
   end
 end

escowles · 2016-08-30T19:58:13Z

@pgwillia Thanks for bringing that up. I think it's better to leave the members/add_members alone (so Collections can still have child Collections if anyone wants to do that), but I've added some helper methods like what you outlined to add a batch of objects to a collection. See b6fa608

tpendragon · 2016-08-31T16:25:30Z

We need an automated data migration method to do this.

pgwillia · 2016-08-31T21:50:04Z

@escowles The problem that I was trying to solve is that Sufia doesn't use an Actor like your Plum does, so needed a place to populate member_of_collections. Where the collection added members seemed logical to me. Shouldn't the inverted collection membership apply to all members regardless of class?

Sufia also needs member_of_collection_ids set. Would you be agreeable to adding this to the add_member_objects helper method?

pgwillia · 2016-08-31T22:21:18Z

Sufia calls

# app/controllers/concerns/curation_concerns/collections_controller_behavior.rb
80     def create
81       @collection.apply_depositor_metadata(current_user.user_key)
82       add_members_to_collection unless batch.empty?
...
216       def add_members_to_collection(collection = nil)
217         collection ||= @collection
218         collection.add_members batch
219       end

So I think that member_of_collections should be set somewhere in curation_concerns.

pgwillia · 2016-09-01T21:44:54Z

Did @tpendragon last commit to this branch step on some of @escowles work? b6fa608 doesn't seem to exist.

tpendragon · 2016-09-01T21:45:43Z

Er, maybe? @escowles can you check? I thought I just got rid of the AF lock.

…ixes ualbertalib/HydraNorth#1126 * use latest fcrepo4 (4.6.0) * add debugging tools to Gemfile * still need to override curation_concerns for some missing behaviour * configure Collection faceting

tpendragon · 2016-09-07T21:33:01Z

@pgwillia Looks like @escowles pushed up the missing code.

escowles · 2016-09-12T19:14:40Z

Are there any outstanding issues with this PR? @pgwillia @jeremyf @projecthydra/sufia-code-reviewers ? If not, I can rebase it to address the merge conflicts.

mjgiarlo · 2016-09-13T15:16:24Z

@escowles that sounds like a no to me. Rebase away!

jeremyf · 2016-10-05T13:59:34Z

@escowles We have a broken build due to dependencies on RDF.

jcoyne · 2016-10-24T14:00:12Z

@escowles this last change (requiring RDF 2.1) is going to be a breaking change for folks who want the ordering behavior present in RDF 1.

escowles · 2016-10-24T14:06:04Z

@jcoyne I know — but I thought it was unavoidable because we're requiring a version of AF that requires RDF 2.x.

But I just checked and noticed that curation_concerns.gemspec is only requiring AF >= 10.3.0.rc1, whereas the test app Gemfile.extra is requiring 11.0.0.rc7. I'll see if I can relax the Gemfile.extra to match the gemspec and get it working with RDF 1.x.

tpendragon · 2016-11-02T15:47:18Z

@escowles Were those two tests really unused? They seem to be around the collection show page "remove member" functionality.

escowles · 2016-11-02T15:50:17Z

@tpendragon I removed the tests because this PR removes the underlying functionality, since collection membership is now managed through the work edit form instead of through the collection edit form.

I'm not opposed to reimplementing that functionality if people think it's useful. But this PR is already big enough I thought it was better to not add new stuff to it.

tpendragon · 2016-11-02T15:51:34Z

@escowles I think we should maintain the current UI's functionality - it feels unexpected that it would change.

val99erie · 2016-11-03T16:10:18Z

app/indexers/curation_concerns/collection_indexer.rb

@@ -10,6 +10,11 @@ def generate_solr_document
        # Index the size of the collection in bytes
        solr_doc[Solrizer.solr_name(:bytes, STORED_LONG)] = object.bytes
        solr_doc['thumbnail_path_ss'] = thumbnail_path
+
+        object.in_collections.each do |col|
+          solr_doc['member_of_collection_ids_ssim'] = col.id


Can collections only belong to one other collection? Is that why you are using = here instead of <<?

No, this should be using << instead of = to allow the object to be in multiple collections — I'll update that.

mjgiarlo

Just a few last changes to suggest. Thanks for working on this!

mjgiarlo · 2016-11-22T16:22:08Z

app/views/curation_concerns/base/_form_member_of_collections.html.erb

@@ -0,0 +1,6 @@
+<% if @form.respond_to? :member_of_collection_ids %>
+  <div class="row" with-headroom">


@escowles You've either got an extraneous or a missing " here.

mjgiarlo · 2016-11-22T16:25:17Z

curation_concerns.gemspec

@@ -45,6 +45,7 @@ Gem::Specification.new do |spec|
  spec.add_dependency 'dry-struct', '~> 0.1'
  spec.add_dependency 'flot-rails', '~> 0.0.7'
  spec.add_dependency 'highcharts-rails'
+  spec.add_dependency 'hydra-pcdm', '~> 0.9.0'


Instead of pinning hydra-pcdm in CC, should we update the hydra-works gemspec and let CC's dependence on HW pick up the right version of HP?

I've uddated samvera/hydra-works#303 to make HW require hydra-pcdm >= 0.9.0. I can update this PR to use that once it's released.

Merged, @escowles. Thanks! HW now needs a release and this PR can then point at the new release.

mjgiarlo · 2016-11-22T16:27:54Z

spec/test_app_templates/Gemfile.extra

@@ -1,4 +1,6 @@
 # placeholder to use for pinning against specific gem commit references
+gem 'hydra-pcdm', github:'projecthydra/hydra-pcdm', branch: 'master'


Are these still needed? I'd like to see these taken out before we merge this PR -- and then we can cut the 2.0 release soon thereafter.

mjgiarlo · 2016-11-22T16:32:42Z

@escowles Would you like to squash the commits, or do you think they're good logical chunks that should remain separate? I could go either way.

jcoyne · 2016-11-22T16:33:28Z

app/services/curation_concerns/actors/actor_factory.rb

@@ -9,6 +9,7 @@ def self.build(curation_concern, current_user)

      def self.stack_actors(curation_concern)
        [AddToCollectionActor,
+         AddAsMemberOfCollectionsActor,


Why are we making this a new actor and not replacing AddToCollectionActor?

My intention was to leave the existing membership functionality for Collections to continue to have member Collections. But I could just replace AddToCollectionActor if that makes more sense.

I don't think Collections use the ActorStack, so I think we can get rid of it, right?

escowles · 2016-12-01T14:38:32Z

@mjgiarlo I've rebased and squashed this PR, now depending on hydra-works 0.15.
@jcoyne Does my answer above address your concern, or would you like me to replace AddToCollectionActor?

jcoyne · 2016-12-01T15:30:43Z

app/services/curation_concerns/actors/actor_factory.rb

@@ -9,6 +9,7 @@ def self.build(curation_concern, current_user)

      def self.stack_actors(curation_concern)
        [AddToCollectionActor,
+         AddAsMemberOfCollectionsActor,


I don't think Collections use the ActorStack, so I think we can get rid of it, right?

…collections to link to Collections

escowles · 2016-12-07T15:57:22Z

@jcoyne I've removed the AddToCollectionActor to avoid that duplication.

jcoyne · 2016-12-07T16:24:24Z

app/models/concerns/curation_concerns/collection.rb

+
+    def add_member_objects(new_member_ids)
+      Array(new_member_ids).each do |member_id|
+        member = ActiveFedora::Base.find(member_id, cast: true)


The cast: true doesn't do anything (anymore). please remove that.

jcoyne · 2016-12-07T16:25:38Z

app/models/concerns/curation_concerns/collection.rb

@@ -10,5 +10,17 @@ def add_members(new_member_ids)
      return if new_member_ids.nil? || new_member_ids.empty?
      members << ActiveFedora::Base.find(new_member_ids)
    end
+
+    def add_member_objects(new_member_ids)


Add some documentation here about how this adds the association on the member side. If you wouldn't mind, please also add some documentation to add_members too.

jcoyne · 2016-12-07T16:29:55Z

app/presenters/curation_concerns/work_show_presenter.rb

+                                        *presenter_factory_arguments)
+    end
+
+    def member_of_collection_presenters


Add method documentation here.

jcoyne · 2016-12-07T16:31:57Z

app/presenters/curation_concerns/work_show_presenter.rb

@@ -84,8 +82,12 @@ def composite_presenter_class

    # @return [Array<CollectionPresenter>] presenters for the collections that this work is a member of
    def collection_presenters


Is this method still needed?

jcoyne · 2016-12-07T16:33:43Z

app/views/catalog/_action_menu_partials/_collection.html.erb

        <% end %>
      </li>
    <% end %>
-    <% if can? :collect, document %>
+    <% if can?(:collect, document) && document.collection? %>


Why is this check required. Isn't this used on a show page for a collection (which ensures that document is a Collection)?

Is this template used at all?

jcoyne · 2016-12-07T16:38:44Z

app/views/collections/edit.html.erb

@@ -1,7 +1,6 @@
 <% provide :page_title, curation_concern_page_title(@collection) %>

-<h1>Edit <%= @collection %> <span class="human_readable_type">(<%= @collection.human_readable_type %>)</span></h1>
-
+<h1>Edit <%= @collection.first_title %> <span class="human_readable_type">(<%= @collection.human_readable_type %>)</span></h1>


use @collection.to_s for the title rather than first_title.

jcoyne · 2016-12-07T16:39:18Z

app/views/curation_concerns/base/_form_member_of_collections.html.erb

@@ -0,0 +1,6 @@
+<% if @form.respond_to? :member_of_collection_ids %>


Why do we need to check this? Shouldn't it always be present?

jcoyne · 2016-12-07T16:40:19Z

app/views/curation_concerns/base/_member_of_collections.html.erb

@@ -0,0 +1,12 @@
+    <% if defined?(presenter.member_of_collection_presenters) && presenter.member_of_collection_presenters.size > 0 %>


Why do we need to check for defined?(presenter.member_of_collection_presenters)?

This is called from _attributes.html.erb for everything (including FileSets that don't have a member_of_collection_presenters method). We can move this logic into _attribute.html.erb if that seems better.

jcoyne · 2016-12-07T16:41:02Z

app/views/curation_concerns/base/_member_of_collections.html.erb

+        <td>
+          <ul class="member_of_collections">
+            <% presenter.member_of_collection_presenters.each do |p| %>
+              <li><%= link_to p.title.first, main_app.collection_path(p.id), class: 'collection-link' %></li>


Can we encapsulate p.title.first in a method (#to_s perhaps)?

escowles · 2016-12-08T21:54:50Z

@jcoyne I've added some docs and removed some unnecessary methods and checks leftover from when I was trying to support both the member and member_of membership modes.

jcoyne

Just one more thing. Then we can merge it.

jcoyne · 2016-12-08T23:37:59Z

app/views/curation_concerns/base/_member_of_collections.html.erb

@@ -0,0 +1,12 @@
+<% if defined?(presenter.member_of_collection_presenters) && presenter.member_of_collection_presenters.size > 0 %>


Yes. Let's get rid of this check here and put it in the _attributes partial.

OK, I've moved the check into the _attributes partial.

jcoyne · 2016-12-09T14:15:54Z

I'm dismissing @mjgiarlo's review because all those changes have been made.

All these changes have been made.

escowles added the in progress label Aug 16, 2016

jeremyf reviewed Aug 17, 2016
View reviewed changes

escowles force-pushed the member_of_replace branch from ca5e080 to ac1931a Compare August 17, 2016 18:41

escowles force-pushed the member_of_replace branch 3 times, most recently from 27ee033 to e18cb74 Compare August 23, 2016 11:15

tpendragon mentioned this pull request Aug 29, 2016

[WIP] Adding support for member_of_collections links from Works to Collections #888

Closed

escowles changed the title ~~[WIP] Reverse Collection membership~~ Reverse Collection membership Aug 29, 2016

tpendragon force-pushed the member_of_replace branch from b6fa608 to 4c3329d Compare August 31, 2016 16:45

escowles force-pushed the member_of_replace branch from e29f93c to c2fca95 Compare September 13, 2016 16:43

escowles force-pushed the member_of_replace branch from 54af92e to 082e146 Compare October 24, 2016 15:04

val99erie reviewed Nov 3, 2016

View reviewed changes

no-reply self-assigned this Nov 9, 2016

escowles force-pushed the member_of_replace branch from bcd7ab1 to c57f134 Compare November 15, 2016 18:41

mjgiarlo previously requested changes Nov 22, 2016

View reviewed changes

jcoyne requested changes Nov 22, 2016

View reviewed changes

escowles force-pushed the member_of_replace branch from fe94e88 to 96eb177 Compare November 30, 2016 17:48

jcoyne requested changes Dec 1, 2016

View reviewed changes

jcoyne mentioned this pull request Dec 3, 2016

Reverse collection membership samvera/hyrax#7

Closed

escowles force-pushed the member_of_replace branch from 96eb177 to ca7f07e Compare December 6, 2016 12:33

Reversing membership to improve performance: Works now use member_of_…

3f2cb49

…collections to link to Collections

escowles force-pushed the member_of_replace branch from ca7f07e to 3f2cb49 Compare December 6, 2016 13:56

jcoyne requested changes Dec 7, 2016

View reviewed changes

Addressing code review comments

96add13

jcoyne requested changes Dec 8, 2016

View reviewed changes

Moving member_of_collection_presenters check to parent view

ed6bd7a

jcoyne approved these changes Dec 9, 2016

View reviewed changes

jcoyne merged commit 613651e into master Dec 9, 2016

jcoyne removed the in progress label Dec 9, 2016

jcoyne deleted the member_of_replace branch December 9, 2016 14:17

jrochkind mentioned this pull request Apr 5, 2017

Collection facet sciencehistory/chf-sufia#349

Open

tpendragon unassigned no-reply Sep 19, 2018

		@@ -0,0 +1,6 @@
		<% if @form.respond_to? :member_of_collection_ids %>
		<div class="row" with-headroom">

		@@ -1,4 +1,6 @@
		# placeholder to use for pinning against specific gem commit references
		gem 'hydra-pcdm', github:'projecthydra/hydra-pcdm', branch: 'master'

		@@ -84,8 +82,12 @@ def composite_presenter_class

		# @return [Array<CollectionPresenter>] presenters for the collections that this work is a member of
		def collection_presenters

		@@ -0,0 +1,12 @@
		<% if defined?(presenter.member_of_collection_presenters) && presenter.member_of_collection_presenters.size > 0 %>

Reverse Collection membership #901

Reverse Collection membership #901

Conversation

escowles commented Aug 16, 2016 • edited

Choose a reason for hiding this comment

escowles commented Aug 17, 2016

escowles commented Aug 17, 2016

pgwillia commented Aug 26, 2016

escowles commented Aug 30, 2016

tpendragon commented Aug 31, 2016

pgwillia commented Aug 31, 2016 • edited

pgwillia commented Aug 31, 2016 • edited

pgwillia commented Sep 1, 2016 • edited

tpendragon commented Sep 1, 2016

tpendragon commented Sep 7, 2016

escowles commented Sep 12, 2016

mjgiarlo commented Sep 13, 2016

jeremyf commented Oct 5, 2016

jcoyne commented Oct 24, 2016

escowles commented Oct 24, 2016

tpendragon commented Nov 2, 2016 • edited

escowles commented Nov 2, 2016

tpendragon commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjgiarlo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjgiarlo commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escowles commented Dec 1, 2016

Choose a reason for hiding this comment

escowles commented Dec 7, 2016

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

escowles commented Dec 8, 2016

jcoyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne commented Dec 9, 2016

escowles commented Aug 16, 2016 •

edited

pgwillia commented Aug 31, 2016 •

edited

pgwillia commented Aug 31, 2016 •

edited

pgwillia commented Sep 1, 2016 •

edited

tpendragon commented Nov 2, 2016 •

edited