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

Adding missing elements from physdesc #1482

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Conversation

randalldfloyd
Copy link
Contributor

@randalldfloyd randalldfloyd commented Dec 13, 2023

This PR adds individual indexing and display of the missing child elements of physdesc at both a collection and component level . In doing so, it carefully preserves the preexisting behaviors of the extents child element.

Assuming the test app is already running this commit level locally, this can be tested with:

cd .internal_test_app
export FILE=../spec/fixtures/ead/nlm/alphaomegaalpha.xml
export REPOSITORY_ID=nlm

bundle exec rake arclight:index

For the new document In the locally running test app ...

Summary should contain:

  • Extent: 15.0 linear feet (36 boxes + oversize folder) and 3 CDs

Background should contain:

  • Physical facet: Compact digital disc
  • Dimensions: 7.5 x 5.5 in.

This component in the document should contain:

  • Extent: 1.5 Linear Feet
  • Physical facet: Informational cards
  • Dimensions: various

@randalldfloyd randalldfloyd force-pushed the 1448_physdesc_elements branch 3 times, most recently from f3c30db to 20331c1 Compare December 13, 2023 13:29
@randalldfloyd randalldfloyd marked this pull request as ready for review December 13, 2023 17:15
@marlo-longley
Copy link
Contributor

marlo-longley commented Dec 13, 2023

Hi @randalldfloyd -- nice! This works great for me locally.

My only suggestion is maybe to add a test that captures the very helpful directions you laid out in your comment above. So, something like checking that "Physical facet: Compact digital disc
" is on the collection page for example in this file and one for the component page as well.

Your tests for the indexing are great but this would make sure the elements are appearing on the pages. Thoughts?

@randalldfloyd
Copy link
Contributor Author

@marlo-longley Glad you asked that because I had the same question myself.

I tried to get a feel for whether not I should test for display via the collection/component page feature specs, so I compared what's currently indexed/configured for display with what's actually being tested that way. As it turns out, there's a lengthy list of fields that aren't tested for display (and some have no indexing coverage), including the new ones from #1466, but it's especially true of component fields - there's not a lot of fielded data tested on the component page at all.

Based on that I wasn't sure how necessary it was to make everything track one-to-one. But I can definitely do it if we want to try to establish a pattern for doing so in the future.

@seanaery
Copy link
Contributor

I agree that feature specs would be helpful here (and for the other fields we have added in this sprint). We do have some existing that we can just supplement:

@randalldfloyd I'd recommend adding the feature tests for just the fields that this PR adds to the display. We can create another ticket for writing tests for the fields from #1466 and any others that may be missing.

@randalldfloyd
Copy link
Contributor Author

Thanks @seanaery ! Yeah the more I thought about it, my question was more for the missing elements still on the board than for this PR specifically, so having that consensus is great.

@randalldfloyd
Copy link
Contributor Author

@seanaery @marlo-longley The physdesc elements are now tested in the collection/component feature specs. I had to disable a rubocop metric for the collection page #background example, as the new elements bumped it over the limit of "Example has too many expectations [24/22]" . Could be why the tests stopped reflecting the fully configured list of fields.

If we don't like doing that I can find some arbitrary way to group those expectations into smaller examples, but either way it sounds like we plan on growing that list of tested elements.

@@ -73,7 +73,7 @@
end
end

it 'background has configured metadata' do
it 'background has configured metadata' do # rubocop:disable RSpec/MultipleExpectations
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this.

@seanaery
Copy link
Contributor

This is great @randalldfloyd . Thanks too for all the revisions to tests and fixtures, that helps a lot!

@marlo-longley marlo-longley merged commit 27f0f3e into main Dec 14, 2023
4 checks passed
@marlo-longley marlo-longley deleted the 1448_physdesc_elements branch December 14, 2023 22:31
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.

3 participants