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

[#49081] PDF-Export: Add tests for pdf content #13238

Merged
merged 19 commits into from
Jul 31, 2023

Conversation

as-op
Copy link
Contributor

@as-op as-op commented Jul 25, 2023

This PR adds:

  • Best case tests for WorkPackage::PDFExport::WorkPackageListToPdf
  • Best case tests for WorkPackage::PDFExport::WorkPackageToPdf

https://community.openproject.org/work_packages/49081

@as-op as-op marked this pull request as ready for review July 27, 2023 11:29
Copy link
Contributor

@klaustopher klaustopher left a comment

Choose a reason for hiding this comment

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

Left 2 remarks inside, otherwise this is fine

Comment on lines +92 to +97
content = export_pdf.content
# File.binwrite('WorkPackageToPdf-test-preview.pdf', content)
{ strings: PDF::Inspector::Text.analyze(content).strings,
images: PDF::Inspector::XObject.analyze(content).page_xobjects.flat_map do |o|
o.values.select { |v| v.hash[:Subtype] == :Image }
end }
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ In the other test we are writing the PDF to disk, here we are checking the content directly. Can't we use the same procedure in the other file as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The (other) query based export does have a feature to batch exporting thousands of work packages (with images) to different pdfs and concats them in the end with an external tool. Granted, the batching is currently not triggered in the spec, but this change wouldn't have much benefits.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines 83 to 89
def column_title(column_name)
label_title(column_name).upcase
end

def label_title(column_name)
WorkPackage.human_attribute_name(column_name)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

It's only 2 methods, but they are the same here and in the other test. We could put them in a module in spec/support/ and include that module in both tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!

@as-op as-op merged commit 6468c7f into dev Jul 31, 2023
10 checks passed
@as-op as-op deleted the implementation/49081-add-tests-for-pdf-content branch July 31, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants