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 ([email protected])
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.

[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. :sweat:

[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,[email protected],"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,[email protected],"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,[email protected],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,[email protected],"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,[email protected],"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,[email protected],,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!

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

[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. :pray:

[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 [email protected], I get

If I then check with [email protected], I get:

(Edit) However, if I run set up and then import with [email protected], I get the same as what you have.

[cielf]

Some thoughts on the situation above: 1/ I suspect that [email protected] doesn't have a default storage location set, whereas [email protected] might.

2/ That we didn't get any errors on importing as [email protected] after importing the same partners as [email protected] 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)

[github-actions[bot]]

Automatically unassigned after 7 days of inactivity.