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

fix(fonts): adds docs and tests for potentially missing language pack #9

Merged
merged 1 commit into from
May 10, 2019

Conversation

itsachen
Copy link
Contributor

@itsachen itsachen commented May 10, 2019

What

Why

Recently Quantum reached out with some letter IDs that contained non-standard un-embedded fonts. We ran the PDFs through Adobe preflight (see screenshot), and even pdffonts locally and confirmed that there were non-standard un-embedded fonts.
dotum

It turns out that these fonts required an additional poppler package poppler-data to properly detect them using pdffonts. Otherwise, those fonts would be ignored and a warning would be thrown:
Kibana

This isn't explicitly necessary on our dev laptops because the homebrew formula we use also installs poppler-data.

To be clear, the changes in this PR won't fix the bug in lob-api since the fix is dependent on a system-installed dependency, but these changes document this corner-case.

@itsachen itsachen requested review from a team and sjl2 May 10, 2019 00:39
Copy link

@sjl2 sjl2 left a comment

Choose a reason for hiding this comment

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

LGTM. So did you not have to bust the cache for the node modules in Circle?

@sjl2 sjl2 requested a review from a team May 10, 2019 00:51
@sjl2 sjl2 assigned itsachen and unassigned sjl2 May 10, 2019
@itsachen
Copy link
Contributor Author

itsachen commented May 10, 2019

@sjl2 Apparently this project doesn't use caching for CircleCI. We may need to bust the cache in lob-api.

@itsachen itsachen removed their assignment May 10, 2019
@itsachen itsachen requested a review from marklee22 May 10, 2019 00:54
Copy link

@marklee22 marklee22 left a comment

Choose a reason for hiding this comment

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

The PDF is blank when I view it. Is this because I'm missing the Language Pack fonts?

sudo sh -c 'echo "deb http://archive.debian.org/debian jessie-backports main" >> /etc/apt/sources.list'
sudo sh -c 'echo "deb-src http://archive.debian.org/debian jessie-backports main" >> /etc/apt/sources.list'
sudo apt-get update -o Acquire::Check-Valid-Until=false -o Acquire::CompressionTypes::Order::=gz
sudo apt-get install -y --no-install-recommends g++-4.9 libpoppler-private-dev poppler-data lcov

Choose a reason for hiding this comment

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

Do we need to also run these commands on our docker image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean on Lob API? Theres a PR for that here: https://github.com/lob/lob-api/pull/5270

Copy link

@marklee22 marklee22 May 10, 2019

Choose a reason for hiding this comment

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

Oh, my bad. I thought this was a lob-api fix PR. Ignore my comment.

@itsachen
Copy link
Contributor Author

itsachen commented May 10, 2019

@marklee22 Yeah the blank PDF is on purpose. I took the actual customer PDF that was causing the issue and removed all content from it for privacy reasons.

Whats interesting is that the PDF looks blank but it still contains empty text that uses these bad fonts. So...pretty messed up PDFs to begin with.

@marklee22 marklee22 assigned itsachen and unassigned marklee22 May 10, 2019
@itsachen itsachen merged commit 27ba8d7 into master May 10, 2019
@itsachen itsachen deleted the test-missing-languages branch May 10, 2019 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants