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

Valkyrie Indexer Setup and Specs #4221

Closed
wants to merge 59 commits into from
Closed

Valkyrie Indexer Setup and Specs #4221

wants to merge 59 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 25, 2020

Note: includes a failing test that need a fix (see below).

Indexer:

  • Add :basic_metadata Indexer to ValkyrieWorkIndexer
  • Add indexing rules to config/metadata/basic_metadata.yml
  • Add a basic spec for a generated Indexer (MonographIndexer)

ActiveFedoraConverter

  • Fixes issue with creating a DefaultWork for Valkyrie native resources (ie. those generated via the generator)
  • Specs updated for ModelTransformer, ActiveFedoraConverter and Persister
  • This persister test now fails and needs fixing: Wings::Valkyrie::Persister When passing a Valkyrie::Resource that was never an ActiveFedora::Base can save nested resources with undefined method nested_resource_attributes=' for #Wings::CustomResource::GeneratedResourceSchema:xxxx`

@ghost ghost mentioned this pull request Jan 25, 2020
@no-reply
Copy link
Contributor

this is looking great! thanks for pushing this forward! i'll give a closer review when we get #4219 in and rebase.

@no-reply no-reply self-requested a review January 26, 2020 22:14
Tom Johnson and others added 11 commits January 26, 2020 20:21
`Hyrax::FilterByType` already includes infrastructure for filtering by
collection and work types. Rather than implement a new switch, use its existing
`only_works?` and `only_collections?` to filter by "generic type".
Adds tests for collection member search using Valkyrie `Hyrax::PcdmCollection`.
Use the configured storage adapter
* adjust approach to converting files
* add custom query find_file_metadata_by_use which is a valkyrie replacement for af object filter_files_by_type
* add helper methods original_file, extracted_text, and thumbnail in FileSet resource

Co-authored-by: cjcolvar <[email protected]>
Refactor file use:
* added methods to Hyrax::FileSet defining which Valkyrie::Vocab::PCDMUse URIs should be used in all cases for the primary uses of original_file, extracted_text, and thumbnail.
* updated all uses of Valkyrie::Vocab::PCDMUse URIs for the primary uses to use the methods defined in FileSet

Added tests for new methods
Instead of putting the use in a use attributes, conversion from AF File copies over all types as they exist in AF into the types attribute of FileMetadata.  To find the use, the :include? method is used to check for a specific use in the types.  This avoids data duplication by not having both type and use.  And it avoids data integrity issues when we aren’t able to correctly identify the use out of the array of types.

Co-authored-by: cjcolvar <[email protected]>
Supersedes #4208.
…xt, thumbnail

2 changes here…

### Parts of FileSet

AF behavior #original_file, #extracted_text, and #thumbnail each return a single PCDM::File,  So this now has the methods that get these relatiionships from FileMetadata to return `.first`

### Versioning

Versioning was looking through to ActiveFedora through the original_file method defined in wings/hydra/pcdm/pcdm_valkyrization_behavior.  Since that was method was removed with the addition of Hyrax::FileSet#original_file, it was no longer getting the lookthrough behavior.  The tests related to versioning have been marked pending.  Issue #3923 addresses FileMetadata and versioning.
Add indexer to work_resource generator
@straleyb
Copy link
Contributor

Since #4219 is in now, im going to go ahead and publish this PR. Its ready for review.

@straleyb straleyb marked this pull request as ready for review January 27, 2020 21:38
@ghost
Copy link
Author

ghost commented Jan 27, 2020

I think this is review-able, although note the one failing test.

@no-reply
Copy link
Contributor

@geekscruff thanks. i'm taking a look

Tom Johnson added 3 commits January 28, 2020 15:49
For valkyrie normalization, take advantage for the new
`Hyrax::FileMetadata::Use.uri_for` module method to avoid reproducing
logic. When the argument is already a URI, just return it to avoid casting
unnecessarily.

In the ActiveFedora normalizer, `RDF::URI` already implements equality
appropriately, so `#casecmp` isn't needed and we can flatten to a simple switch
statement.
The constant previously used an incorrect URI. `Thumbnail` should be
`ThumbnailImage`.
Avoid typing `Hyrax.query_service.custom_queries` everywhere. Provide a method
on the Hyrax level instead.
@no-reply
Copy link
Contributor

The issue is to do with inheritance of class variables, which impacts accepts_nested_attributes_for. i'm working on approaches, but may also just end up writing DefaultWork out of existence. it's probably time for that :)

@no-reply
Copy link
Contributor

seems connected to samvera/active_fedora#1343

straleyb and others added 21 commits January 29, 2020 06:40
Implement Valkyrie indexing for Collections
Transitions from using the instance variable to using the already included method that returns the resource indexer.
Prior to this change, specs reported the following warning:

```console
DEPRECATION WARNING: Delegating constraints to arel is deprecated and
will be removed in Rails 6.0.
```

I removed the deprecation warnings with this change (e.g. adding an
explicit call to #arel).
…n-warning

Removing Rails 6.0 deprecation warning
Create `FileSetDescription` to find characterization for FileSets
When constructing a description of a Valkyrie description, prefer to use URIs.
Support custom URI use/type in `FileSetDescription`
This is a bit of a stab in the dark. However, we had a failed Ruby
build with the following message:

```console
1) Hyrax::FileSetCSVService when specifying terms and separator csv
     Failure/Error:
       ::CSV.generate do |csv|
         csv << terms.map do |term|
           values = file_set.send(term)
           values = values.respond_to?(:to_a) ? values.to_a : [values] # make sure we have an array
           values.join(multi_value_separator)
         end
       end

     NameError:
       uninitialized constant CSV
```

What I suspect to be in play is that the corresponding spec does not
similarly namespace the `CSV` constant. The production ruby file uses
`::CSV` whereas the spec ruby file uses `CSV`. This commit seeks to use
the same namespacing, hopefully reducing an intermittent error.

See the following  CircleCI build for the failures:

-  https://circleci.com/gh/samvera/hyrax/17063#tests/containers/3
…s-for-csv

Ensuring CSV usage has same namespace consideration
@straleyb
Copy link
Contributor

Well, I rebased this onto master and it seems like it picked up on a whole slew of stuff. Not sure why that happened exactly. I can push a new one if needed though since I borked it-ish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants