Skip to content

ARC-184 - migrate PDF building - #107

Merged
respinos merged 13 commits into
mainfrom
ARC-184
Sep 15, 2026
Merged

respinos merged 13 commits into
mainfrom
ARC-184

Conversation

@respinos

@respinos respinos commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Tips for running the rake tass

  • clear out existing artifacts with find data/pdf -type f | xargs rm
  • run the generator with the following to see the details
RAILS_ENV=development FINDING_AID_DATA=$PWD/data EADID=umich-wcl-M-54ger bin/rails arclight:generate_pdf --trace
  • this adds some fields to the solr document, but those should be ignored if not present

Summary

Adds/updates Arclight fragment rendering and utility styling for component details/print views in:

  • _component_details.html.erb
  • _components.html.erb
  • _utility_styles.html.erb
  • fragment.html.erb

Adds a print stylesheet and asset manifest/config adjustments for the new UI output:

  • print.scss
  • manifest.js
  • assets.rb

Updates catalog/solr/EAD config plumbing for the new component/package behavior:

  • catalog_controller.rb
  • catalog.rb
  • solr_document.rb
  • ead2_config.rb
  • generate.rake

@respinos
respinos marked this pull request as ready for review September 14, 2026 15:13
@rshiggin

rshiggin commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

I'm working on testing this PR. First pass wasn't successful. Trying again with zeroed out data dir.

@ssciolla ssciolla left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I started going through this, but am not finished yet. Comments are mostly cleanup. Feel a little iffy on the platform-checking stuff -- maybe we just don't support generate when it's not on Linux/in a container?

Comment thread app/views/arclight/fragments/fragment.html.erb Outdated
Comment thread app/services/package/generator.rb Outdated
Comment thread app/services/package/generator.rb Outdated
Comment thread app/services/package/generator.rb Outdated

@rshiggin rshiggin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My testing was successful, once I'd zeroed out and recreated the XML and PDF derivatives.

@respinos

Copy link
Copy Markdown
Contributor Author

I started going through this, but am not finished yet. Comments are mostly cleanup. Feel a little iffy on the platform-checking stuff -- maybe we just don't support generate when it's not on Linux/in a container?

It's iffy in the long tradition of platform #ifdef ¯_(ツ)_/¯ This is fine and wkhtmltopdf should not be the long term solution.

Comment thread app/views/arclight/fragments/_components.html.erb Outdated

doc.css("#summary dl").first << fragment.css("dl#ead_author_block dt,dl#ead_author_block dd")
if (contents_el = doc.css("div.al-contents").first)
doc.css("html").first["class"] = ""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Was there a class that needed to be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OTOB there are no-js classes until javascript loads. Do the updated styles use this for anything? Undocumented, and early PDFs were turning out blank.

@ssciolla ssciolla left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry, more irksome comments. Going to run it now and then likely approve.

Comment thread app/views/arclight/fragments/_component_details.html.erb Outdated
Comment on lines +5 to +23
<% if component.collection_has_requestable_components? %>
<% if component.is_checkbox_requestable? %>
<div class="checkbox request-checkbox">
<label class="btn btn-outline-secondary btn-outline-request contents-action">
<input type="checkbox" name="Request" value="<%= component.id %>">
<input type="hidden" name="Request" value="<%= component.id %>">
<input type="hidden" name="ItemSubTitle_<%= component.id %>" value="<%= component.aeon_item_sub_title_value %>">
<input type="hidden" name="ItemVolume_<%= component.id %>" value="<%= component.aeon_item_volume_value %>">
<input type="hidden" name="ItemCitation_<%= component.id %>" value="<%= component.aeon_item_citation_value %>">
<% if component.aeon_item_sub_title_value.present? %>
<span class="request-checkbox-label" aria-hidden="true"><%= component.aeon_item_sub_title_value %></span>
<% end %>
<span class="visually-hidden">Request "<%= component.aeon_item_sub_title_visually_hidden %>"</span>
</label>
</div>
<% else %>
<%# <span style="padding: 0.375rem 1rem;"><i class="text-blended fas fa-circle"></i></span> %>
<% end %>
<% end %>

@ssciolla ssciolla Sep 15, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If fragment stuff is only ever used by the HTML and PDF derivatives, then do we need the Aeon stuff in there?

And can we remove the else if it's commented out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a great question, and IMHO beyond the scope of this PR. But I am team "time we spend on this wkhtmltopdf code is better spent figuring out what can replace it."

Comment thread lib/tasks/generate.rake
Comment on lines +18 to +29
# desc 'Generate packages for indexed finding aids via background jobs'
# task generate_enqueue: :environment do
# args = {}
# if ENV['REPOSITORY_ID']
# repository_config = Arclight::Repository.find_by(slug: ENV['REPOSITORY_ID'])
# args[:repository_ssm] = repository_config.name
# elsif ENV['EADID']
# args[:eadid] = ENV['EADID']
# end
# args[:format] = ENV['FORMAT'] || 'html'
# Package::Queue.new.setup(**args)
# end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# desc 'Generate packages for indexed finding aids via background jobs'
# task generate_enqueue: :environment do
# args = {}
# if ENV['REPOSITORY_ID']
# repository_config = Arclight::Repository.find_by(slug: ENV['REPOSITORY_ID'])
# args[:repository_ssm] = repository_config.name
# elsif ENV['EADID']
# args[:eadid] = ENV['EADID']
# end
# args[:format] = ENV['FORMAT'] || 'html'
# Package::Queue.new.setup(**args)
# end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I left this here in case this functionality would be useful in the future. ¯_(ツ)_/¯

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hm, okay, your call. By now you know I'm on team "no commented code."

Comment thread lib/tasks/generate.rake
Comment on lines +33 to +35
raise "Please specify your EAD ID, ex. EADID=<id>" unless ENV["EADID"]

identifier = ENV["EADID"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not very important, but you could just use a positional argument over an environment variable here and below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These were inherited from the original rake tasks; we're still doing this with index_dir, etc. aren't we?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, we are, but we probably won't use index_dir that much from now on, since we also want to create HTML and PDF. This is probably a matter of preference, but I find the positional argument a bit more explicit. Like the difference between passing a value into a function versus depending on a variable in global scope.

Comment on lines 203 to 205
to_field "has_online_content_ssim", extract_xpath(".//dao") do |_record, accumulator|
accumulator.replace([ accumulator.any? ])
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see below you added another to_field definition for has_online_content_ssim. Do we need both? Is this important for the PR? (not sure)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope. Removed, and simplified the total_digital_object_count_isim to match what has_online_content_ssim is currently doing.

Comment thread spec/services/package/generator_spec.rb Outdated
Comment on lines +56 to +57
expect(doc.xpath('//div[@id="summary"]//dl/dd[contains(., "Finding Aid written by E. A. Document")]').first).to be_truthy
# expect(doc.xpath('//div[@id="summary"]//dl/dd[contains(., "Finding Aid written by E. A. Document")]').first).to be_truthy
# expect(doc.css(".access-preview-snippet #toc").first).to be_truthy

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we restore this assertion somehow?

Comment thread spec/services/package/generator_spec.rb Outdated
Comment thread spec/services/package/generator_spec.rb

@ssciolla ssciolla left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! I can't say I thoroughly vetted all the styles, and I don't fully understand some parts of the code, but the output seems much better than what we had before.

Comment thread app/controllers/concerns/um_arclight/catalog.rb Outdated
@respinos
respinos merged commit 54a6553 into main Sep 15, 2026
6 checks passed

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.

Noticed the generated PDF has a duplicated title on each item, plus a checkbox at the beginning. I don't think we need the checkbox in the PDF — something related to the new print.scss?

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.

4 participants