human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

Fix #4138 fix delivery address

Open jimmyli97 opened this issue 1 year ago • 8 comments

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

jimmyli97 avatar Jul 19 '24 01:07 jimmyli97

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 avatar Jul 19 '24 11:07 cielf

@cielf it automatically unassigned me, should I keep commenting every 7 days?

jimmyli97 avatar Jul 19 '24 12:07 jimmyli97

Oops - sorry, my bad (I somehow didn't see that you had been assigned and weren't anymore). Not to worry.

cielf avatar Jul 19 '24 15:07 cielf

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.

cielf avatar Jul 19 '24 15:07 cielf

Hey @dorner -- it looks like the ball on this one is currently in your court?

cielf avatar Aug 25 '24 01:08 cielf

@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.

cielf avatar Sep 28 '24 17:09 cielf

@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 avatar Sep 30 '24 00:09 dorner

@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

jimmyli97 avatar Oct 17 '24 22:10 jimmyli97

failing test is a known flaky one

jimmyli97 avatar Nov 28 '24 03:11 jimmyli97

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

jimmyli97 avatar Dec 03 '24 03:12 jimmyli97

(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

jimmyli97 avatar Dec 03 '24 03:12 jimmyli97

@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

jimmyli97 avatar Dec 03 '24 20:12 jimmyli97

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?

cielf avatar Dec 04 '24 00:12 cielf

fixed spacing in b00d1b0

jimmyli97 avatar Dec 04 '24 03:12 jimmyli97

Functionality works great! Thank you! Back to @dorner for final tech review.

cielf avatar Dec 04 '24 11:12 cielf

Still LGTM

cielf avatar Jan 14 '25 17:01 cielf

@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!

github-actions[bot] avatar Jan 19 '25 16:01 github-actions[bot]