Enable csv import of partners with six fields

[McEileen]

Resolves #5056

Description

• Importing partners will now include more fields.
• To manually test this:

1. log in as a bank organization admin (org_admin1@example.com)
2. navigate to /partners and click "Import partners"
3. then upload a csv file with the following fields: name,email,default_storage_location,send_reminders,quota,notes

• I would appreciate feedback on supporting the upload of a default storage location. I added error handling for scenarios where a user enters a default storage location that is associated with a different organization. I tried to add error handling for scenarios where a user enters a default storage location that doesn't exist, but I couldn't get the validation to work with the non-nullable default_storage_id column.
• Some of the new csv files specify that they include six fields. Thoughts on this naming? Is it helpful to be this explicit, or is it unnecessary?

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• I added unit and request specs.

[cielf]

Thanks for taking this on -- here's your Functional review, pass one. Mentioning @dorner re your above request for advice.

Review notes:
1/ I'd like to see a warning if the default storage location indicated does not exist as a storage location on the organization -- I think uploading, but with a warning would be the case of least surprise here.
2/ I left the send_reminders blank on a partner. I expected that to give a false send_reminders, but instead the partner failed to upload, with no explanation.
3/ I then added a few more items to the upload file, including blank default storage partner. I got an "ActiveModel::UnknownAttributeError at /partners/import_csv.
unknown attribute 'default_storage_location' for Partner." error on the blank one. If there is no default storage location indicated, the partner should be uploadable - we just don't set it.
4/ I like the case insensitive matching on the storage locations!

[dorner]

@McEileen when you say you couldn't get it working... what did you try, and what problems did you see?

[McEileen]

@dorner Good question. My initial approach was to search for the default storage location using the name that the user entered, and to then throw an error if StorageLocation.find_by(name: default_storage_location_name) returned nil, since that would indicate that the storage location doesn't exist. I thought it would be a good way to catch typos, like if someone types "Bluk Storage Location." The problem is that this approach would require the user to only import partners that have default storage locations associated with them. This contradicts the data model, which shows that default_storage_location_id could be nil.

To give a code example, if I change the default_storage_location_belongs_to_organization validator in the partner model to not allow a nil default_storage_location_id, like so

  def default_storage_location_belongs_to_organization
    location_ids = organization&.storage_locations&.pluck(:id)
    unless location_ids&.include?(default_storage_location_id)
      errors.add(:default_storage_location_id, "The default storage location is not a storage location for this partner's organization")
    end
  end

It causes 39 tests to fail. 😓

[McEileen]

@cielf Thank you for the thorough functional review!

1/ I'll work on this and reach out with any follow-up questions.

2/ Did the test csv file you upload have the send_reminders header and it was missing the field itself, as seen below?

name,email,default_storage_location,send_reminders,quota,notes
Partner 1,partner1@example.com,"Smithsonian Conservation Center",50,"great partner"

Or was it missing both the send_reminders header and field, as seen below?

name,email,default_storage_location,quota,notes
Partner 1,partner1@example.com,"Smithsonian Conservation Center",50,"great partner"

3/ Same question here. When there's a blank default storage partner, does that mean the header is present and the field is missing, as seen below?

name,email,default_storage_location,send_reminders,quota,notes
Partner 1,partner1@example.com,true,50,"great partner"

Or does it mean both the default_storage_location header and field are missing, as seen below?

name,email,default_storage_location,send_reminders,quota,notes
Partner 1,partner1@example.com,"Smithsonian Conservation Center",true,50,"great partner"

[cielf]

@McEileen -- 2/ I think it had the header, but for that row, it had no value. I was working with numbers and exporting... so it was showing blank rather than FALSE. Which should have produced something like
name,email,default_storage_location,send_reminders,quota,notes
Partner 1,partner1@example.com,"Smithsonian Conservation Center",,50,"great partner"

If that doesn't work, let me know and I'll reproduce an example.

3/ Similarly for this one
name,email,default_storage_location,send_reminders,quota,notes
Partner 1,partner1@example.com,,FALSE,50,"great partne

should do the trick

[McEileen]

@cielf @dorner I addressed outstanding feedback, and also implemented some changes that @awwaiid had suggested during a past Sunday office hours. Please let me know if there are other changes you all would like to see.

Thank you for the thorough reviews!

[cielf]

I'm down to 1 quibble, functionally:
If we have a partner with a misspelled storage location name, can we get that to appear as a warning rather than an error? (ie show it as black on yellow, rather than the white on red).

Otherwise looks very good!

[McEileen]

If we have a partner with a misspelled storage location name, can we get that to appear as a warning rather than an error? (ie show it as black on yellow, rather than the white on red).

@cielf Sure, I'll look into that this weekend.

[McEileen]

Hi @cielf, I made the warning change that you requested. Let me know what you think! (I am a little uneasy about how it required me to make changes to other models that also use the importable concern, however, I was able to get #import_csv tests passing.)

[McEileen]

@cielf I wanted to highlight this logic to you.

If a user attempts to upload a partner with an email and name that is already in use, in addition to a misspelled storage location name, both the email/name messages and the storage location message will display as errors. If you want, I could look into displaying the email/name error messages as errors and the storage location message as a warning. Let me know what you prefer.

[cielf]

My initial take is that if the partner is not getting added, having everything as errors won't be a big deal.

[cielf]

For that matter -- if it's easier, it would be ok if it just showed the errors in that case, without the warning?

[McEileen]

Hi @cielf, thanks for flagging this. I realize what I said in this comment was incorrect, apologies.

I pushed up another commit to make my previous commit actually accurate. If you want, I could also stick with the version where if both errors and warnings occur, only errors display. Let me know what you prefer!

[cielf]

Hey @McEileen - I ran the attached csv, and it mostly looks good.

However, the warnings aren't showing up. (I suspect you're only showing the warnings if there are no errors, but I haven't confirmed that.)

partners_import_test_20250602.csv

Results in:

[cielf]

Hey @McEileen -- don't know if this is supposed to be ready for review, but partner 9 in the earlier attached csv should show a warning, and isn't.

[cielf]

Hey @McEileen -- are you still working on this?

[McEileen]

Hi, I apologize for being out of touch. As I shared with greater detail in slack, I won't be able to contribute to Human Essentials for the next couple of months. Anyone who is interested is willing to work on the linked issue, #5056, and is also welcome to build off the code in this PR.
Thank you for all the valuable feedback I received in the review process. 🙏

[jonny5]

@cielf This has been updated to display the warnings. Using partners_import_test_20250602.csv locally shows the errors/warnings as below:

[cielf]

Hrm. I'm seeing different results.
If I run setup, then check with org_admin2@example.com, I get

If I then check with org_admin1@example.com, I get:

(Edit)
However, if I run set up and then import with org_admin1@example.com, I get the same as what you have.

[cielf]

Some thoughts on the situation above: 1/ I suspect that org_admin2@example.com doesn't have a default storage location set, whereas org_admin1@example.com might.

2/ That we didn't get any errors on importing as org_admin1@example.com after importing the same partners as org_admin2@example.com seems more than a little odd -- it also only imported one of the partners. I'm going to point out that the partner that it did import would have failed, after a successful import of an earlier one.

I feel like there's something going on here around checking partner names / emails across the system rather than within the bank.

[cielf]

But/and this raises bigger questions. (IIRC, we do not currently allow partners to send requests to multiple banks) so I'm ccing in @awwaiid.

[cielf]

Also ccing in @ruestitch
For this PR, it looks to me like we are failing silently if the partner to be imported already exists on another bank.
I suggest that until we can properly handle a 1 partner two bank situation, we should instead fail noisily (ie. give an error)

[scooter-dangle]

For consistence, do we actually want to add skip_blanks: true when we have a number of other places where we parse CSV data where we don't? It's easier to explain a rule on import to users that's true in all cases.

If we think we should allow+skip blanks on CSV import, can we leave that out here and do it in a separate PR updating every CSV.parse we're using?

[jonny5]

👍 consistency is good, I removed that change

[github-actions]

Automatically unassigned after 7 days of inactivity.

[cielf]

Note @ParsannaK is working on #5056 -- Not clear whether they are extending this PR -- so keep a look out for a PR from them instead.