Orders members when converting to AF objects. #3697

rotated8 · 2019-03-21T19:46:24Z

Adding members via #members= does not add the member to the ordered relation, so that after a save, #member_ids contains objects, but #ordered_member_ids is empty. Since many parts of the code use #ordered_members, the converter should order the members.

elrayle · 2019-03-21T20:05:53Z

spec/wings/active_fedora_converter_spec.rb

@@ -81,6 +81,10 @@
        it 'converts member_of_collection_ids back to af_object' do
          expect(converter.convert.members.map(&:id)).to match_array [work2.id, work3.id]


If you want to demonstrate the difference between members and ordered_members, you can use match_array here and reverse the order of the ids...

expect(converter.convert.members.map(&:id)).to match_array [work3.id, work2.id]

elrayle · 2019-03-21T20:06:36Z

spec/wings/active_fedora_converter_spec.rb

@@ -81,6 +81,10 @@
        it 'converts member_of_collection_ids back to af_object' do
          expect(converter.convert.members.map(&:id)).to match_array [work2.id, work3.id]
        end
+
+        it 'preserves order across conversion' do
+          expect(converter.convert.ordered_member_ids).to match_array [work2.id, work3.id]


And this should use .to eq

expect(converter.convert.ordered_member_ids).to eq [work2.id, work3.id]

This confirms the order is maintained.

elrayle

The change to members is a suggestion. The changed to ordered_members is needed to ensure that order was maintained.

elrayle · 2019-03-25T22:23:28Z

Exploring the fileset failures...

The failure is on calls to fileset.parent_works which does not exist for resources in Hyrax and therefore, converts the resource to an af_object and passes it to Hydra::Works through the missing_method override for Hyrax resources
The expected results are [wk1, wk2] and the actual results are [wk1, wk2, wk2]
This happens because parent_works in Hydra::Works combines in_works + member_of_works. in_works returns [wk2] and member_of_works returns [wk1, wk2]
Testing with the original af_object before it is converted to the resource, and it had the same problem. This implies that the problem my be at the Hydra::Works level.

What is not clear is why this did not happen before this branch and does with this branch. What is also not clear is why wk2 is repeated, but wk1 is not.

A simple fix would be to apply .uniq to parent_works in Hydra::Works, but I'd like to understand why changing from setting the members with members to setting them with ordered_members would cause this error.

elrayle · 2019-03-28T17:58:51Z

Since Hyrax is only interested in one direction, changed parent_works in Hyrax to not depend on parent_works in Hydra::Works. This fixes the problem where parent_works for works and filesets was returning repeated parents.

This solves issues with unordered members not being ordered at all after conversion.

Use a stricter test for the array returned from #ordered_member_ids

related to #3698

Otherwise, during the transformation back and forth, the members could be put on the transformed resource in the wrong order. This fixes a randomly failing condition. Changed wings tests to use ordered_members since that is the relationship used in hyrax.

jrgriffiniii · 2019-04-04T13:22:36Z

lib/wings/hydra/pcdm/models/concerns/pcdm_valkyrie_behavior.rb

+      end
+
+      # @param valkyrie [Boolean] Should the returned ids be for Valkyrie or AF objects?
+      # @return [Enumerable<String> | Enumerable<Valkerie::ID] The ids of the file sets this work contains


Comment should be rewritten # @return [Enumerable<String> | Enumerable<Valkyrie::ID>] The ids of the file sets this work contains

jrgriffiniii · 2019-04-04T13:23:16Z

lib/wings/hydra/works/models/concerns/work_valkyrie_behavior.rb

+      # # @return [Enumerable<String> | Enumerable<Valkerie::ID] The ids of the works this work is in
+      # def parent_work_ids(valkyrie: false)
+      #   parent_works(valkyrie: valkyrie).map(&:id)
+      # end


This commented block needs to be removed.

jrgriffiniii

I can clean the comments, but otherwise this seems to be an excellent solution.

…havior; Fixing a typo. error in the comment for Wings::Pcdm::PcdmValkyrieBehavior#parent_work_ids

I added code requiring review by others.

Fixes a random error in find_members test where sometimes the results are in the correct order and sometimes not.

laritakr · 2019-04-04T14:19:56Z

lib/wings/hydra/pcdm/models/concerns/pcdm_valkyrie_behavior.rb

@@ -139,7 +139,7 @@ def parent_works(valkyrie: false)
      end

      # @param valkyrie [Boolean] Should the returned ids be for Valkyrie or AF objects?
-      # @return [Enumerable<String> | Enumerable<Valkyrie::ID>] The ids of the file sets this work contains
+      # @return [Enumerable<String> | Enumerable<Valkerie::ID>] The ids of the works this work is contained in


…_ids

requested changes addressed

rotated8 added the wings label Mar 21, 2019

rotated8 self-assigned this Mar 21, 2019

rotated8 requested review from elrayle and lsat12357 March 21, 2019 19:46

mjgiarlo added the in progress label Mar 21, 2019

elrayle reviewed Mar 21, 2019

View reviewed changes

elrayle previously requested changes Mar 21, 2019

View reviewed changes

rotated8 force-pushed the wings-ordered-members branch 2 times, most recently from dff6a08 to f7dba2d Compare March 21, 2019 21:07

lsat12357 previously approved these changes Mar 22, 2019

View reviewed changes

elrayle dismissed lsat12357’s stale review via 6c30f5f March 22, 2019 20:43

elrayle force-pushed the wings-ordered-members branch 2 times, most recently from 6588fea to fcad364 Compare March 25, 2019 21:35

elrayle mentioned this pull request Mar 28, 2019

using ordered_members puts parent-child relationship in both directions samvera/hydra-works#361

Open

elrayle force-pushed the wings-ordered-members branch 2 times, most recently from 1850669 to 23784d7 Compare March 28, 2019 17:54

elrayle force-pushed the wings-ordered-members branch 3 times, most recently from 8c4d907 to dff2606 Compare April 2, 2019 16:21

rotated8 and others added 5 commits April 4, 2019 08:18

Orders members when converting to AF objects.

6cef9d8

This solves issues with unordered members not being ordered at all after conversion.

Update the tests

df837dc

Use a stricter test for the array returned from #ordered_member_ids

Clean the repo for the converter tests.

ef59589

related to #3698

add parent_works and parent_work_ids

6dc3627

elrayle force-pushed the wings-ordered-members branch from dff2606 to 6dc3627 Compare April 4, 2019 12:18

jrgriffiniii self-requested a review April 4, 2019 13:20

jrgriffiniii reviewed Apr 4, 2019

View reviewed changes

jrgriffiniii previously requested changes Apr 4, 2019

View reviewed changes

Cleaning the commented code block out of Wings::Works::WorkValkyrieBe…

7f979de

…havior; Fixing a typo. error in the comment for Wings::Pcdm::PcdmValkyrieBehavior#parent_work_ids

elrayle requested a review from jrgriffiniii April 4, 2019 13:51

make find_may_by_ids return results in same order as the passed in ids

0532405

Fixes a random error in find_members test where sometimes the results are in the correct order and sometimes not.

elrayle force-pushed the wings-ordered-members branch from a6163ac to 0532405 Compare April 4, 2019 14:02

laritakr reviewed Apr 4, 2019

View reviewed changes

Fixing a typo. error in Wings::Pcdm::PcdmValkyrieBehavior#parent_work…

ad16bf2

…_ids

laritakr approved these changes Apr 4, 2019

View reviewed changes

elrayle merged commit 9192ec0 into master Apr 4, 2019

elrayle deleted the wings-ordered-members branch April 4, 2019 15:10

elrayle removed the in progress label Apr 4, 2019

laritakr mentioned this pull request Apr 4, 2019

Persister can add and remove members #3737

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orders members when converting to AF objects. #3697

Orders members when converting to AF objects. #3697

rotated8 commented Mar 21, 2019

elrayle Mar 21, 2019

rotated8 Mar 21, 2019

elrayle Mar 21, 2019 •

edited

Loading

rotated8 Mar 21, 2019

elrayle left a comment

elrayle commented Mar 25, 2019

elrayle commented Mar 28, 2019

jrgriffiniii Apr 4, 2019

jrgriffiniii Apr 4, 2019

jrgriffiniii left a comment

laritakr Apr 4, 2019

		@@ -81,6 +81,10 @@
		it 'converts member_of_collection_ids back to af_object' do
		expect(converter.convert.members.map(&:id)).to match_array [work2.id, work3.id]

Orders members when converting to AF objects. #3697

Orders members when converting to AF objects. #3697

Conversation

rotated8 commented Mar 21, 2019

elrayle Mar 21, 2019

Choose a reason for hiding this comment

rotated8 Mar 21, 2019

Choose a reason for hiding this comment

elrayle Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

rotated8 Mar 21, 2019

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment

elrayle commented Mar 25, 2019

elrayle commented Mar 28, 2019

jrgriffiniii Apr 4, 2019

Choose a reason for hiding this comment

jrgriffiniii Apr 4, 2019

Choose a reason for hiding this comment

jrgriffiniii left a comment

Choose a reason for hiding this comment

laritakr Apr 4, 2019

Choose a reason for hiding this comment

elrayle Mar 21, 2019 •

edited

Loading