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

Add the distribution ID in the "receipt" (i.e. distribution printout) #4999

Open
3 tasks
cielf opened this issue Feb 9, 2025 · 7 comments
Open
3 tasks

Add the distribution ID in the "receipt" (i.e. distribution printout) #4999

cielf opened this issue Feb 9, 2025 · 7 comments
Assignees

Comments

@cielf
Copy link
Collaborator

cielf commented Feb 9, 2025

Summary

Add the distribution ID to the distribution printout

Why

It serves the same purpose as an invoice number -- will help the banks with their off-line record keeping of pickups.

Details

##To see a distribution printout,
sign in as [email protected]
Click Distributions
Then click "print" beside a distribution. This displays a pdf
##Change required:
Insert the field just above "Comments", on the left side of that pdf, with the heading "Distribution ID", (and show the distribution id there).

Criteria for completion

  • behaviour as described
  • automated tests to support the change
  • update user guide (relevant page at docs/user_guide/bank/essentials_distribution.md)
@cielf cielf added Difficulty—Beginner Help Wanted Groomed + open to all! labels Feb 9, 2025
@sunsheeppoplar
Copy link
Contributor

@cielf 👋 I've been working a solution this weekend. I think I'll be able to put up the fix soon, just working through some issues generating the test pdfs correctly. Any chance I could be assigned this?

@github-actions github-actions bot removed the Help Wanted Groomed + open to all! label Feb 18, 2025
@cielf
Copy link
Collaborator Author

cielf commented Feb 18, 2025

Yup! Done. Good to see you back.

@sunsheeppoplar
Copy link
Contributor

@cielf thanks, glad to be contributing again. I've run into a bit of a snag with tests. I'm not sure what the maintainers would like to see here.

I see that not too long ago there were some tests added to do some pdf comparisons which is pretty cool. By adding the requirement of rendering the distribution id, as it stands, it would mean that every variation would need to be accounted for in fixtures/files/*.pdf because every call in the spec of create_dist is incrementing an id causing failures in the compare_pdf comparison.

Right now there are 11 calls of the create_dist helper. I'm not sure if it's acceptable to commit all 11 fixtures or if there's something else more creative we could do. Over time I worry this wouldn't scale that well. Let me know what you think!

@cielf
Copy link
Collaborator Author

cielf commented Feb 19, 2025

I'm going to pass that question over to @dorner.

@dorner
Copy link
Collaborator

dorner commented Feb 20, 2025

Unfortunately you can't use the ID directly in the PDF, because the ID will be different depending on the order of the specs passing.

Here's my recommendation: Add the value {{distribution_id}} in the fixture PDFs where the actual distribution ID would go. Then update the comparison function to do a find/replace on {{distribution_id}} and replace with the actual distribution ID within the test. Then when you compare them it should pass.

@sunsheeppoplar
Copy link
Contributor

@dorner Thanks for your help. I think I understand the gist of what you're saying, but I'm struggling a little with the mechanics.

  1. Temporarily add {{distrubtion_id}} to the Prawn rendering code in distribution_pdf.rb
  2. Regenerate the fixtures
  3. In compare_pdf use IO.binread on the expected file

Assuming all that is correct, I wasn't sure how to go about finding and replacing. Would I need to convert the the string {{distrubtion_id}} into the format the binread and binwrite use, ASCII-8BIT?

@dorner
Copy link
Collaborator

dorner commented Feb 21, 2025

hmm... maybe? I'd try without it and if it doesn't work, you can convert the string. I'm not 100% sure either of those will work though...

You might need to hardcode the distribution ID when you generate them (e.g. set distribution.id = 456 before passing into the generator function)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants