Editing a work drops it from a collection if the collection is owned by a different user

[hackartisan]

Descriptive summary

Sufia 7.2

Expected behavior

Work should stay in a collection it was previously part of

Actual behavior

Collection relationship is lost

Steps to reproduce the behavior

1. Create 2 users; log in from different browsers or incognito window
2. User1 create a collection and a work
3. User1 add work to collection
4. User1 give edit access of work to User2
5. User2 edit work and click 'save' (you don't even have to make any changes)
6. When work refreshes (for either user) it is no longer part of the collection

[hackartisan]

Why are there two ways to check collections from a work? and why do they return different things?

[11] pry(#<CurationConcerns::Actors::GenericWorkActor>)> curation_concern.member_of_collections
=> []
[12] pry(#<CurationConcerns::Actors::GenericWorkActor>)> curation_concern.in_collections
  Load LDP (18.8ms) http://127.0.0.1:8080/fedora/rest/dev/1r/66/j1/12/1r66j112r Service: 70345058523360
=> [#<Collection id: "1r66j112r", depositor: "aheadley@chemheritage.org", title: ["Let's loose some data!"], date_uploaded: nil, date_modified: nil, head: [], tail: [], label: nil, relative_path: nil, import_url: nil, part_of: [], resource_type: [], creator: [], contributor: [], description: [], keyword: [], rights: [], rights_statement: [], publisher: [], date_created: [], subject: [], language: [], identifier: [], based_near: [], related_url: [], bibliographic_citation: [], source: [], access_control_id: "e77fc453-fe3c-4f75-8dd3-7ed78e58545a", representative_id: nil, thumbnail_id: nil>]
[13] pry(#<CurationConcerns::Actors::GenericWorkActor>)> show-source curation_concern.in_collections

From: /usr/local/lib/ruby/gems/2.2.0/gems/hydra-pcdm-0.9.0/lib/hydra/pcdm/models/concerns/pcdm_behavior.rb @ line 51:
Owner: Hydra::PCDM::PcdmBehavior
Visibility: public
Number of lines: 3

def in_collections
  member_of.select(&:pcdm_collection?).to_a
end
[14] pry(#<CurationConcerns::Actors::GenericWorkActor>)> show-source curation_concern.member_of_collections                                                                                             

From: /usr/local/lib/ruby/gems/2.2.0/gems/active-fedora-10.3.0/lib/active_fedora/associations/builder/association.rb @ line 95:
Owner: GenericWork::GeneratedAssociationMethods
Visibility: public
Number of lines: 3

def #{name}(*args)
  association(:#{name}).reader(*args)
end

[hackartisan]

Found the source of this bug.

Consider this case:

• WorkA belongs to CollectionA owned by UserA
• UserZ edits WorkA and saves it.

UserZ's edit form does not list CollectionA as an option, so when the save is submitted that collection is not sent with the form data. Therefore it is removed.

Source https://github.com/projecthydra/curation_concerns/blob/1-6-stable/app/actors/curation_concerns/actors/add_to_collection_actor.rb#L21

Side notes: [1], [2]

Assumption: The behavior commented on here (to remove from all collections before adding to the specified ones) was an easy way to delete from collections that were de-selected in the form.

However, we know that a work may belong to a collection that's not in the form.

Proposal: Have the actor check for collections IDs the work is a member of but the current user does not have edit permissions to.

[1] new_collection_ids is truthy when it's an empty array. I believe the intention was maybe to check for empty here, but that doesn't really solve the problem (e.g. if UserZ wants to add WorkA to CollectionZ then it's not empty and the work is again dropped from CollectionA) and I think will be extraneous once a better solution is reached.

[2] See equivalent code (which I haven't investigated, really) in CC master: https://github.com/projecthydra/curation_concerns/blob/master/app/actors/curation_concerns/actors/add_as_member_of_collections_actor.rb#L18

[hackartisan]

Alternate proposal: have the form contain a hidden field that sends all the IDs of collections that aren't displayed.

Also thinking it seems possible that there is similar logic for admin sets and we should check on that.

[jcoyne]

I like your initial proposal best. I suspect the form itself needs to be updated so that is shows the collections a work is a member of unioned with those collections that are editable by the current user.

[hackartisan]

@jcoyne cool thanks for the feedback. I'm looking into how to make this work and how to test it. I tried to replicate in curation concerns and I was limited by the UI so I'm not sure if it's really a bug there or whether I should override the actor in sufia. will probably take another stab at it.

Re: displaying on the form, I guess you'd also need to respect the visibility settings of the collection in question. that seems like a separate (enhancement) issue.

[hackartisan]

Okay I've got some logic that works locally; gonna turn back to CC and write a test there.

        def add_to_collections(new_collection_ids)
          return true unless new_collection_ids
          # remove from old collections
          (curation_concern.in_collection_ids - new_collection_ids).each do |old_id|
            # don't remove from collections current user doesn't have edit permissions for
            collection = ::Collection.find(old_id)
            if collection.edit_users.include? user.email
              collection.members.delete(curation_concern)
              collection.save
            end
          end
[...]