Resolves #4818 Fix print feature for product drive with no participant #4831
Conversation
|
I see the It's a small fix, so I think you could add it to this PR. |
app/pdfs/donation_pdf.rb
Outdated
| @address = donation.product_drive_participant.address | ||
| @email = donation.product_drive_participant.email | ||
| else | ||
| @name = donation.product_drive.name |
There was a problem hiding this comment.
I think you are missing this requirement in your fix:
If it's a product drive, but without a product drive participant, put "Product Drive -- [the name of the product drive]" there instead.
If you just call name on the product drive, that prefix "Product Drive" will be missing.
There was a problem hiding this comment.
Updated! Added label for product drive
There was a problem hiding this comment.
I added the address attribute to the factory class but after that I was seeing this error: The error was being through by the Geocodable class which is included in the ProductDriveParticipant model class. It was not easy to track down and I think that might be why there are not any Rspec tests that have tested this specific attribute yet. I had to add a mock for Geocodable to resolve the error: Updating product drive participant factory and adding geocodable mock Please take a look and let me know if this solution looks good to you! |
|
Sorry for the trouble, I also didn't know that addresses are tied to the geocoder gem. Looks like it takes a address and then fetches the latitude/longitude. But since that requires an HTTP request (I think?), it is stubbed in the test suite. Check out this section in our Looks like if you use one of those three addresses, you shouldn't need that additional mocking line. |
|
Hey @gabeparra01. Thanks for this! Note for next time -- we use the syntax Resolves #4831 to indicate the issue that is resolved, or Partial #4831 if it's only a partial fix. Our automated workflow in gitbhub understands Resolves to be an indicator that when the PR is merged, it can close the issue. |
Anytime! Ok I will keep that in mind for next time! |
spec/pdfs/donation_pdf_spec.rb
Outdated
| @@ -20,6 +31,7 @@ | |||
| before(:each) do | |||
| create(:line_item, itemizable: donation, item: item1, quantity: 50) | |||
| create(:line_item, itemizable: donation, item: item2, quantity: 100) | |||
| allow_any_instance_of(ProductDriveParticipant).to receive(:geocode).and_return(true) | |||
There was a problem hiding this comment.
We shouldn't need this - see @coalest 's comment.
There was a problem hiding this comment.
Ok I removed the stub statement and replaced the address value: Using hardcoded address from stubbed class in rails_helper.rb
spec/pdfs/donation_pdf_spec.rb
Outdated
| it "renders correctly" do | ||
| pdf = described_class.new(organization, product_drive_donation) | ||
| pdf_test = PDF::Reader.new(StringIO.new(pdf.compute_and_render)) | ||
| expect(pdf_test.page(1).text).to include(product_drive_donation.product_drive_participant.business_name) |
There was a problem hiding this comment.
I much prefer explicit checks - since you've given the participant a specific business name, you should be checking against that string rather than going back to the model you created.
There was a problem hiding this comment.
…eparra01/human-essentials into 4818-fix-product-drive-print
…eparra01/human-essentials into 4818-fix-product-drive-print
Description
Resolves #4818
The description for the issue is here: #4818
The changes for fixing the nil error were straightforward and I followed the recommended solution in the issue description.
One obstacle that I faced was testing the address attribute for a product drive participant. It is listed as an attribute in the model class and it is referenced in donation_pdf.rb. But I couldn't find an example Rspec test where it is being tested. When I tried to create a product drive participant object with an address in an Rspec test, I received an unrecognized method/pattern error. Please let me know if I should open another issue to look into this further or if there is something I am missing in the database schema?
Type of change
How Has This Been Tested?
Screenshots
Product Drive with participant.pdf
Product Drive without a participant.pdf