#3067: add required info on csv partner export (partial solution) #4229
Conversation
- Also includes the updates on partner model test
callmarx
changed the title
There was a problem hiding this comment.
Looks pretty good! I've got a couple of minor functionality quibbles.
I'm also thinking that we should add the "other agency type" to the agency type column if the agency type is other. I can make that a separate issue in the spirit of avoiding scope creep -- but if you want to make the adjustment that would also be ok. What do you think?
| @@ -172,6 +172,12 @@ def self.csv_export_headers | |||
| [ | |||
| "Agency Name", | |||
| "Agency Email", | |||
| "Agency address", | |||
| "Agency City", | |||
There was a problem hiding this comment.
Minor quibble: inconsistent capitalization -- please caplitalize address.
| primary_contact_phone: contact_phone | ||
| primary_contact_phone: contact_phone, | ||
| address1: agency_address, | ||
| city: agency_city, |
There was a problem hiding this comment.
I would do a test with both address1 and address 2.
|
Hi @cielf, I fixed the minor functionality quibbles and implemented a solution to include |
app/models/partner.rb
Outdated
| return {} if profile.blank? | ||
|
|
||
| @agency_info = { | ||
| address: [profile.address1, profile.address2].reject(&:blank?).join(', '), |
There was a problem hiding this comment.
I usually prefer positive over negative statements, so I'd like this to be .select(&:present?)
There was a problem hiding this comment.
it's definitely make the code more readable. Thanks
spec/models/partner_spec.rb
Outdated
| expect(partner.csv_export_attributes).to include(agency_state) | ||
| expect(partner.csv_export_attributes).to include(agency_zipcode) | ||
| expect(partner.csv_export_attributes).to include(agency_website) | ||
| expect(partner.csv_export_attributes).to include("#{Partner::AGENCY_TYPES["OTHER"]}: #{other_agency_type}") |
There was a problem hiding this comment.
Isn't the order of columns important? We should be checking the exact array that's returned.
There was a problem hiding this comment.
Sure! I've changed the test for the exact value of csv_export_attributes and the title to "should has the info in the columns order"
… csv_export_attributes
|
@callmarx there's a failing test - can you fix it? |
|
@callmarx: Your PR |
Partial #3067
Description
This is a partial solution for the issue that includes only the following into the partner agencies export:
As this is my first PR and contact with the project, I intend to make another one to include the information "providing diapers" and "providing period supplies" when I got a better overview of it.
Type of change
How Has This Been Tested?
Successful passed on rspec test, which I included into the partner model test, and check it manually the download based on seeds data
Screenshots
unnecessary