Fix #4138 fix delivery address
Resolves #4138
Description
Prints no address if delivery_method is pick_up Prints partner address if partner has no program address and delivery_method is delivery or shipped Prints partner program address if partner has program address and delivery_method is delivery or shipped Tests output against expected PDFs Adds helper test to generate expected PDFs
Type of change
- Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
Passes test suite Manual testing
Hey @jimmyli97 As a matter of process, in the future please get yourself assigned to the issues when you are going to be working on them - or at least comment on them. This helps us avoid duplication of effort. Thanks.
@cielf it automatically unassigned me, should I keep commenting every 7 days?
Oops - sorry, my bad (I somehow didn't see that you had been assigned and weren't anymore). Not to worry.
But if it's going to lapse while you are still working on it, yeah - commenting makes sense -- otherwise someone else may pick it up. Once you've put in the pull request, it's not as big a deal.
Hey @dorner -- it looks like the ball on this one is currently in your court?
@jimmyli97 FYI: We had an urgent fix that required all the senior contributors this week , so we didn't get to look at this again. Hopefully this week will go better.
@jimmyli97 this helps a bit but not by much. Essentially:
- To generate the PDF, you need some set of environment to be set up.
- To run the RSpec, you need the same environment to be set up.
That does not imply that in order to generate the PDF, you should run an RSpec. It means you should extract the behavior that does the environment setup to shared code, and run that setup code both in the RSpec and in a separate script that can be used to generate the PDF.
@dorner I managed to figure out a way to separate out the setup code so you can call a Rails console method which calls that setup code, and also the Rspecs call that setup code. it's a bit messy though, let me know what you think
failing test is a known flaky one
If the partner should fill in an incomplete delivery address (maybe they just fill in the street address, because that's the part that is different), we get a mess like this
fixed in 39e8cb4
(6,1) failing test is known flake (6, 4) failing test is present on main branch also, unrelated to this PR, think it's flaky and added to list
@cielf I made a solution in 4c6cd65 where if there's no partner primary contact info it fills it in with a blank space, but I could also do it so that it uses the partner's name and email (which are both guaranteed to not be blank) if you want
Better! It does feel to me like there is a bit too much space between the Issued To and the Delivery Address, though. Is it possible to tighten that up without side effects?
fixed spacing in b00d1b0
Functionality works great! Thank you! Back to @dorner for final tech review.
Still LGTM
@jimmyli97: Your PR Fix #4138 fix delivery address is part of today's Human Essentials production release: 2025.01.19.
Thank you very much for your contribution!