Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow orphaned object report to find orphaned compound objects. #710

Merged
merged 7 commits into from Oct 9, 2018

Conversation

Projects
None yet
4 participants
@jonathangreen
Copy link
Member

commented Aug 24, 2018

JIRA Ticket

https://jira.duraspace.org/browse/ISLANDORA-2294

Was discussed in the committers meeting yesterday.

What does this Pull Request do?

The current orphan objects report does not look for orphaned compound objects. This adds a check to see if the compound solution pack exists, and if it does then it uses the compound solution pack variable to get the predicate the compound solution pack uses.

What's new?

Move the creation of the query components using parent predicates to its own logic based on an array of predicates. This allows us to optionally add the compound solution pack predicate.

How should this be tested?

  • Create some orphans (using the relationships for collection, book and compound)
  • Make sure the report finds them

Additional Notes:

This addition to the report will be more useful after Islandora/islandora_solution_pack_compound#146 is merged so that compounds no longer create parentless orphans.

Interested parties

@DiegoPino @Islandora/7-x-1-x-committers

@DiegoPino

This comment has been minimized.

Copy link

commented Aug 27, 2018

WOHO! @jonathangreen can review tomorrow. Reads fine. Thanks

}
!optionals
!filters
FILTER (!bound(?liveparent))
} ORDER BY ?object
EOQ;
$parent_relationships = array('<fedora-rels-ext:isMemberOfCollection>', '<fedora-rels-ext:isMemberOf>', '<islandora:isPageOf>');

This comment has been minimized.

Copy link
@DiegoPino

DiegoPino Sep 6, 2018

Hi, just wondering... could we eventually put a hook here? Altering that list seems like a pretty sweet idea. Would you like to give that a try? If not, ok too. Tested and works pretty well but wanted just to try my luck

This comment has been minimized.

Copy link
@jonathangreen

jonathangreen Sep 6, 2018

Author Member

I do agree perhaps a hook is a cleaner implementation.

I see 3 options really:

  1. Define something like hook_islandora_solution_pack_child_predicate and let each solution pack respond with its predicates.
  2. Leave these values here and also define an alter. Something like hook_islandora_orphan_child_predicate_alter and let people alter as they see fit.
  3. Leave it as it.

What option were you thinking of? I think option 1 is probably the best/cleanest option, it expands the scope of this PR a bit though, making it 4 PR instead. Option 2 and 3 are probably the easiest options.

This comment has been minimized.

Copy link
@DiegoPino

DiegoPino Sep 6, 2018

I agree with you, 1 is the best, but 2, low hanging fruit could lead to a future implementation (like a call of a call) of 1 if you are in a hurry. Totally depends on your energy/time.

@jonathangreen

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

Although I may come to regret it, I decided to give option 1 a try and implement a hook for this. I also made PRs to the other solution packs to implement the hook.

Testing for this should be the same as before.

jonathangreen added some commits Sep 7, 2018

@jonathangreen jonathangreen force-pushed the jonathangreen:ISLANDORA-2294 branch from dfc6470 to 163fd9d Sep 7, 2018

jonathangreen added some commits Sep 7, 2018

Show resolved Hide resolved islandora.module
@DiegoPino
Copy link

left a comment

Good stuff

@DiegoPino

This comment has been minimized.

Copy link

commented Sep 25, 2018

Ok this looks good to my old eyes and phpcs is happy. Will approve and then go through the other 4 related pull request,

Islandora/islandora_solution_pack_collection#208
Islandora/islandora_solution_pack_book#203
Islandora/islandora_solution_pack_compound#147
Islandora/islandora_solution_pack_newspaper#161

then i will let all this sit 48 hours until our next committers call(this week right?) so we get a good applause before merging. Sounds good @Islandora/7-x-1-x-committers ?

@DonRichards

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Ready to merge?

@DiegoPino DiegoPino merged commit f3b3ffc into Islandora:7.x Oct 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DiegoPino

This comment has been minimized.

Copy link

commented Oct 9, 2018

@jonathangreen let me know if all is OK with this set to close the ticket, or we can keep it open if any other SP needs same work? Fabulous work. If anything goes wrong its my fault because i pressed merge -=)

@jonathangreen jonathangreen deleted the jonathangreen:ISLANDORA-2294 branch Oct 9, 2018

@jonathangreen

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

🙏 Thanks so much Diego! Really appreciate all the reviews and feedback on this.

There is one other thing before we close the ticket, I believe this PR is on the same ticket:
Islandora/islandora_solution_pack_compound#146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.