samvera / hydra-works

A ruby gem implementation of the PCDM Works domain model based on the Samvera software stack
Other
24 stars 14 forks source link

using ordered_members puts parent-child relationship in both directions #361

Open elrayle opened 5 years ago

elrayle commented 5 years ago

Descriptive summary

Adding a child work to a parent work using ordered members causes the child to respond with parent works using both in_work_ids and member_of_work_ids, resulting in the parent work being listed twice.

Rationale

In the list of parent_works, the parents should not repeat.

Expected behavior

        parent_work1.ordered_members = [child_work]
        parent_work2.ordered_members = [child_work]
        child_work.save
        parent_work1.save
        parent_work2.save

        expect(child_work.parent_work_ids).to match_array [parent_work1.id, parent_work2.id]

Actual behavior

       expected collection contained:  ["wk1", "wk2"]
       actual collection contained:    ["wk1", "wk1", "wk2", "wk2"]

See PR #360 which includes a test that fails because of this bug.

Steps to reproduce the behavior

Use test provided in PR #360 to debug.

  1. Setup the relationships
        parent_work1.ordered_members = [child_work]
        parent_work2.ordered_members = [child_work]
        child_work.save
        parent_work1.save
        parent_work2.save
  2. Get list of parents
        child_work.parent_works

Expect parent_works to return parent_work1 and parent_work2, but you get the two parent works in the list twice.

Related Work

PR https://github.com/samvera/hyrax/pull/3697 - Orders members when converting to AF objects

elrayle commented 5 years ago
# Setup objects
parent_work1 = GenericWork.new(id: 'wk1')
parent_work2 = GenericWork.new(id: 'wk2')
child_work = GenericWork.new(id: 'wk3')

# Setup relationships
parent_work1.ordered_members = [child_work]
parent_work2.ordered_members = [child_work]
child_work.save
parent_work1.save
parent_work2.save

# test relationships
parent_work1.member_ids         # expected: [wk3] actual: [wk3]
parent_work2.member_ids         # expected: [wk3] actual: [wk3]
parent_work1.ordered_member_ids # expected: [wk3] actual: [wk3]
parent_work2.ordered_member_ids # expected: [wk3] actual: [wk3]
child_work.in_work_ids          # expected: [] actual: [wk1, wk2]
child_work.member_of_work_ids   # expected: [wk1, wk2] actual: [wk1, wk2]
child_work.parent_work_ids      # expected: [wk1, wk2] actual: [wk1, wk2, wk1, wk2]
elrayle commented 5 years ago

Implementation path of in_work_ids...

As written, in_work_ids seems to be using solr to find ordered members. I expected this method to use the members relationship to find all parents and then limit the results to only work parents.

Implementation path of member_of_work_ids...

I do expect this method to use the member_of relationship to find all parents for a work. I'm just not sure why it is defined. I wouldn't expect ordered_aggregation relationship to define this reverse relationship method.

elrayle commented 5 years ago

Ran same relationship checks when relationship was setup using members instead of ordered_members.

parent_work1.member_ids         # expected: [wk3] actual: [wk3]
parent_work2.member_ids         # expected: [wk3] actual: [wk3]
parent_work1.ordered_member_ids # expected: [] actual: []
parent_work2.ordered_member_ids # expected: [] actual: []
child_work.in_work_ids          # expected: [] actual: []
child_work.member_of_work_ids   # expected: [wk1, wk2] actual: [wk1, wk2]
child_work.parent_work_ids      # expected: [wk1, wk2] actual: [wk1, wk2]
elrayle commented 5 years ago

Relationship Expectations

Expected method behaviors:

Summary of Actual method behaviors:

As written, the methods are behaving as expected. But I don't believe this is what was originally intended.