Skip to content

ETT-1840: Fixes for title summary table - #101

Merged
aelkiss merged 3 commits into
mainfrom
ETT-1840-pivot-facet-fixes
Sep 24, 2026
Merged

aelkiss merged 3 commits into
mainfrom
ETT-1840-pivot-facet-fixes

Conversation

@aelkiss

@aelkiss aelkiss commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

This addresses the reported issue of having extraneous format values, since the catalog extracts additional formats from various MARC fields in addition to the format extracted from the leader in a similar style to hathifiles.

  • Query by format: catalog currently includes many formats we don't want, and there's no way to facet on (say) the first value of the format field; we would need to index that separately, and may want to do so in the future

Along the way I also fixed a couple other issues:

  • Add facet.limit=-1 to ensure we're getting all values (the final count does differ and the tests will fail if that isn't there, subject to the vagaries of the sample catalog solr)

  • Add facet.missing=true so we get counts of items with one or more facets with null values (this we can test)

@aelkiss
aelkiss force-pushed the ETT-1840-pivot-facet-fixes branch from 35203d6 to 7ba265e Compare September 21, 2026 20:42
@coveralls

coveralls commented Sep 21, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 99.915% (+0.001%) from 99.914% — ETT-1840-pivot-facet-fixes into main

@aelkiss
aelkiss requested a review from moseshll September 21, 2026 20:43

@moseshll moseshll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I remove the binding.irb that is causing the tests to fail, I still get a failed test in solr_pivot_facets_spec.rb line 39 with 1319 not being in the 1400-200 range -- not sure if that's due to a change in the sample catalog or just to SolrPivotFacets fixes. I originally just wanted to capture that there would be "a lot" of rows, so maybe pushing the range to be_between(1000, 2000) would be supple enough.

@aelkiss

aelkiss commented Sep 23, 2026

Copy link
Copy Markdown
Member Author

If I remove the binding.irb that is causing the tests to fail, I still get a failed test in solr_pivot_facets_spec.rb line 39 with 1319 not being in the 1400-200 range -- not sure if that's due to a change in the sample catalog or just to SolrPivotFacets fixes. I originally just wanted to capture that there would be "a lot" of rows, so maybe pushing the range to be_between(1000, 2000) would be supple enough.

I thought I already changed this, but maybe the commit got lost. Fix incoming...

* Query by format: catalog currently includes many formats we don't
  want, and there's no way to facet on (say) the first value of the
  format field; we would need to index that separately, and may want to do
  so in the future

* Add facet.limit=-1 to ensure we're getting all values

* Add facet.missing=true so we get counts of items with one or more
  facets with null values
@aelkiss
aelkiss force-pushed the ETT-1840-pivot-facet-fixes branch from 7ba265e to a25a27b Compare September 23, 2026 13:40
@aelkiss

aelkiss commented Sep 23, 2026

Copy link
Copy Markdown
Member Author

Fixed & force-pushed. I'll re-request review once tests pass.

@aelkiss
aelkiss requested a review from moseshll September 23, 2026 13:49

@moseshll moseshll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good now. I would recommend following up with the change mentioned at the bottom of bin/refresh_title_summary_table.rb re SimpleCov since the updated version is complaining now, i.e., replace # :nocov: with a # simplecov:disable and # simplecov:enable pair.

@aelkiss
aelkiss merged commit 9422ba0 into main Sep 24, 2026
1 check passed
@aelkiss
aelkiss deleted the ETT-1840-pivot-facet-fixes branch September 24, 2026 15:37
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