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: [M3-7008] - Invoice and Payment id wrapping in generated PDFs #9702

Conversation

bnussman-akamai
Copy link
Member

Description 📝

Important

I recommend checking the internal ticket to see context of this 🪲

Fixes unnecessary wrap in invoice and payment PDFs that caused some id numbers to be partially cut off and improves general positioning

Major Changes 🔄

  • Do not enforce a maxWidth in the addTitle function
  • Replaced hard-coded table start position with dynamic start position 🎉

Preview 📷

Before After
Screenshot 2023-09-19 at 4 49 28 PM Screenshot 2023-09-19 at 5 01 32 PM
Screenshot 2023-09-19 at 4 49 17 PM Screenshot 2023-09-19 at 5 02 38 PM

How to test 🧪

  • Turn on the MSW
  • Go to http://localhost:3000/account/billing
  • Check the PDF of the invoice called Invoice with Large ID and verify the text Invoice: #123456789123456 is positioned correctly and is not overlapping anything
  • Check the PDF of the payment called Payment #123456789123456 and verify the text Receipt for Payment #123456789123456 is positioned correctly and is not overlapping anything
  • Continue to test on a variety of different invoices and payment PDFs

@bnussman-akamai bnussman-akamai added the Bug Fixes for regressions or bugs label Sep 19, 2023
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner September 19, 2023 21:13
@bnussman-akamai bnussman-akamai self-assigned this Sep 19, 2023
@bnussman-akamai bnussman-akamai requested review from mjac0bs and carrillo-erik and removed request for a team September 19, 2023 21:13
});
const titlePosition = addTitle(
doc,
Math.max(leftHeaderYPosition, rightHeaderYPosition) + 12,
Copy link
Member Author

@bnussman-akamai bnussman-akamai Sep 19, 2023

Choose a reason for hiding this comment

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

I changed 4 to 12 to match the payment PDFs. It helps make the PDF a bit more readable (see before and after photos above).

@@ -63,7 +64,7 @@ export const createPaymentsTable = (
headStyles: {
fillColor: '#444444',
},
startY: 165,
startY,
Copy link
Member Author

Choose a reason for hiding this comment

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

startY is now dynamic instead of hard-coded. This allows the table to be dynamically positioned so that the header text will not overlap with the table.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good choice, the dynamic value definitely improves the table's aesthetic.

@carrillo-erik
Copy link
Contributor

Looks good to me 👀 👍.

I was able to download and inspect both the Invoice and the Receipt PDFs. The long numbers do not overlap or cause any visual regressions and are positioned correctly within the PDF.

Screenshot 2023-09-19 at 10 01 55 PM

Screenshot 2023-09-19 at 10 01 41 PM

I did observe a discrepancy if I clicked the link Invoice with Large ID and downloaded the PDF from /billing/invoices/123456789123456, it's not the same invoice. This appears to be limited to the MSW, once I disabled it and navigated to an invoice I was able to download the appropriate PDF.

Screenshot 2023-09-19 at 10 08 20 PM

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Confirmed no wrapping or obscured confirmation numbers on the invoices and payments that I clicked through. Thanks for fixing! 🚢

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Sep 22, 2023
@bnussman-akamai bnussman-akamai merged commit a04069d into linode:develop Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Bug Fixes for regressions or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants