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

normalize content types in pre-assembly to match current set #758

Merged
merged 6 commits into from
Mar 17, 2021

Conversation

peetucket
Copy link
Member

@peetucket peetucket commented Mar 15, 2021

Why was this change made?

Part of sul-dlss/argo#2427 and #753 and #759

Adds 'Map' as an option. We can't add Webarchive-seed, as pre-assembly does not know how to generate content-metadata right now for that kind of object.

UPDATE: Adding webarchive_seed as an option, which becomes dependent on the assembly-objectfile gem knowing how to create it.

UPDATE 2: adding reading order as a new book option, also dependent on some updates to assembly-objectfile gem

Todo:

  • update assembly-objectfile gem and make this dependent on the new gem
  • replace feature tests for type with a faster unit test

How was this change tested?

Updated tests and added new tests for books (both reading directions), map and webarchive-seed content metadata generation.

Which documentation and/or configurations were updated?

@@ -10,7 +10,7 @@
let(:bare_druid) { 'tx666tx9999' }
let(:object_staging_dir) { Rails.root.join(Settings.assembly_staging_dir, 'tx', '666', 'tx', '9999', bare_druid) }
let(:cocina_model_dark_access) { instance_double(Cocina::Models::Access, access: 'dark') }
let(:item) { instance_double(Cocina::Models::DRO, type: Cocina::Models::Vocab.media, access: cocina_model_dark_access) }
let(:item) { instance_double(Cocina::Models::DRO, type: Cocina::Models::Vocab.object, access: cocina_model_dark_access) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests should really use the correct object type, media happened to work, but its not really reflective of the test. Same for other tests below.

@@ -47,6 +47,7 @@ def content_md_creation_style
Cocina::Models::Vocab.object => :file,
Cocina::Models::Vocab.book => :simple_book,
Cocina::Models::Vocab.manuscript => :simple_book,
Cocina::Models::Vocab.document => :document,
Copy link
Member Author

@peetucket peetucket Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this mapping to contentMetadata type was missing, so I added it. Valid styles are here: https://github.com/sul-dlss/assembly-objectfile/blob/main/lib/assembly-objectfile/content_metadata.rb#L17

@peetucket peetucket marked this pull request as ready for review March 15, 2021 22:11
spec/features/map_spec.rb Outdated Show resolved Hide resolved
@peetucket peetucket marked this pull request as ready for review March 16, 2021 18:13
@@ -23,7 +23,9 @@ class BatchContext < ApplicationRecord
'file' => 3,
'media' => 4,
'3d' => 5,
'document' => 6
'document' => 6,
'maps' => 7,
Copy link
Member Author

@peetucket peetucket Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: it has to be maps because map is a reserved keyword for enums:

You tried to define an enum named "content_structure" on the model "BatchContext", but this will generate a class method "map", which is already defined by ActiveRecord::Relation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is it trying to eval the key?

Copy link
Member Author

@peetucket peetucket Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to happen when the class is required, the tests don't even run if you use a key of map:

$ bundle exec rspec

An error occurred while loading ./spec/models/batch_context_spec.rb.
Failure/Error:
  enum content_structure: {
    'simple_image' => 0,
    'simple_book' => 1,
    'book_as_image' => 2, # Deprecated
    'file' => 3,
    'media' => 4,
    '3d' => 5,
    'document' => 6,
    'map' => 7,
    'webarchive_seed' => 8,

ArgumentError:
  You tried to define an enum named "content_structure" on the model "BatchContext", but this will generate a class method "map", which is already defined by ActiveRecord::Relation.
# ./app/models/batch_context.rb:19:in `<class:BatchContext>'
# ./app/models/batch_context.rb:3:in `<top (required)>'
# /Users/petucket/.rvm/gems/ruby-2.7.1/gems/zeitwerk-2.4.2/lib/zeitwerk/kernel.rb:26:in `require'
# /Users/petucket/.rvm/gems/ruby-2.7.1/gems/zeitwerk-2.4.2/lib/zeitwerk/kernel.rb:26:in `require'
# ./spec/models/batch_context_spec.rb:3:in `<top (required)>'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, because it's the ActiveRecord enum method.

@@ -20,6 +20,8 @@
['File', 'file'],
['Media', 'media'],
['3D', '3d'],
['Map', 'maps'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, re: maps vs map

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed 'simple image' and 'simple book' to 'image' and 'book' in the UI to match what is in Argo.

@peetucket peetucket marked this pull request as draft March 16, 2021 20:39
@peetucket peetucket marked this pull request as ready for review March 16, 2021 20:44
@peetucket peetucket marked this pull request as draft March 17, 2021 00:31
@peetucket peetucket marked this pull request as ready for review March 17, 2021 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants