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 #4138 fix delivery address #4540

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jimmyli97
Copy link
Contributor

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

* group examples together with variables
* rubocop -A passes on this file without needing to disable ArrayAlignment
* Compare generated against expected PDFs
* Add helper test to regenerate expected PDFs
* Add helper module
…ethod

* Address output prints delivery address if filled in, otherwise partner address
* Only does this for delivery/shipped method
@cielf
Copy link
Collaborator

cielf commented Jul 19, 2024

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.

@jimmyli97
Copy link
Contributor Author

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

@cielf
Copy link
Collaborator

cielf commented Jul 19, 2024

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

@cielf
Copy link
Collaborator

cielf commented Jul 19, 2024

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 cielf requested a review from dorner July 19, 2024 15:10
cielf
cielf previously requested changes Jul 19, 2024
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hrm. I tried blanking out the two addresses, and got a mess. This shouldn't happen in real life (if it's being delivered, there really should be a delivery address, but I could see it happening in a situation where the bank is very familiar with a new partner, and some staff drops by the partner with the goods on their way home or something ). I'd still like to see it fixed. (You could suppress the "Delivery address" label in this case too.)
Screenshot 2024-07-19 at 11 22 54 AM

@jimmyli97 jimmyli97 requested a review from cielf July 23, 2024 03:39
@cielf cielf dismissed their stale review July 23, 2024 18:09

My concerns have been addressed. Over to @dorner for a technical look-see.

@cielf
Copy link
Collaborator

cielf commented Aug 25, 2024

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

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Overall looks OK - I had some concerns with the skipped helper method.

.gitignore Outdated
@@ -49,6 +49,7 @@ dump.rdb
.DS_Store
.ruby-gemset
*.pdf
!spec/fixtures/files/*.pdf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you adding this if you've added these files? Shouldn't they be tracked?

Copy link
Contributor Author

@jimmyli97 jimmyli97 Sep 12, 2024

Choose a reason for hiding this comment

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

line 51 tells git to ignore all .pdf files so line 52 specifies an exception (by prefixing an exclamation mark) to track the pdf files in spec/fixtures/files

added a comment to clarify in 0d53723

compare_pdf(create_dist(:pick_up), expected_pickup_file)
end
end
# this test is a helper function to regenerate expected PDFs, only commit with it skipped
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's really confusing. If you want a helper method, why not define it as a method, and people can run it from the Rails console?

Copy link
Contributor Author

@jimmyli97 jimmyli97 Sep 12, 2024

Choose a reason for hiding this comment

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

to generate the pdf files correctly, the helper method generating the file needs to have the same test environment setup as the actual test itself. This way you don't have to repeat code setting up the test environment. I did research but I could not figure out a better way to do it without duplicating code.

you can call this method from the terminal by calling the test's line number e.g. bundle exec rspec spec/pdfs/distribution_pdf_spec.rb:227.

I added a comment clarifying this in 0d53723

alternatively I could delete the method if you want, but I found it super helpful because I could just run that line whenever I made changes to the pdf generation and regenerate the test files. so if anyone in the future makes changes they could do the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should definitely be a way to do this without duplicating code. Maybe it's the setup that needs to be extracted to a base method that's called from both places. And worse comes to worst, if you do need to duplicate a few lines of code, it's still a lot less messy than a test that's not actually a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I came up with a solution which checks for an environment variable and if it's set to true to overwrite the expected PDFs in 4516951 . I think it's a lot cleaner, let me know if that works.

@cielf
Copy link
Collaborator

cielf commented Sep 28, 2024

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

@dorner
Copy link
Collaborator

dorner commented Sep 30, 2024

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

@jimmyli97
Copy link
Contributor Author

@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

let(:item3) { create(:item, name: "Item 3", value_in_cents: 300) }
let(:item4) { create(:item, name: "Item 4", package_size: 25, value_in_cents: 400) }
before(:each) do
@organization, @storage_location, @item1, @item2, @item3, @item4 = create_organization_storage_items
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no need to do this via instance variables. Here's how I'd do it to keep the RSpec looking the same:

let(:storage_creation) { create_organization_storage_items }
let(:organization) { storage_creation.organization }
let(:item1) { storage_creation.items[0] }
# etc.

See below where you can change this method to make it easier to work with.

item3 = FactoryBot.create(:item, name: "Item 3", value_in_cents: 300)
item4 = FactoryBot.create(:item, name: "Item 4", package_size: 25, value_in_cents: 400)

[org, storage_location, item1, item2, item3, item4]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd define a struct above, something like

StorageCreation = Data.define(:organization, :storage_location, :items)

and here you could return

StorageCreation.new(org, storage_location, [item1, item2, item3, item4])

private def create_profile(program_address1, program_address2, program_city, program_state, program_zip)
create(:partner_profile,
def create_organization_storage_items
org = FactoryBot.create(:organization,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this setup is being used outside RSpec, it shouldn't be in spec/support and it shouldn't use FactoryBot. It should work more like the seeds file where you create data manually. Probably should live in its own folder under the /lib directory.

organization: organization)
end

def create_file_paths
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you actually creating file paths here? Or just returning the ones you have specified?

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

Successfully merging this pull request may close these issues.

Add delivery address to distribution "receipt"
3 participants