human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

[BUG]: Organization editing raises 500 errors when there is a partner validation issue

Open awwaiid opened this issue 1 year ago • 4 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Current Behavior

When a bank saves their organization details (in My Organization -> Edit) we validate not only the organization, but also the Partners. Some partners have legacy data which is invalid, so the whole thing ends up with a 500 error.

For an example from bugsnag, ((direct link example)[https://app.bugsnag.com/ruby-for-good/human-essentials/errors/6703ebb0dab091357e9a2e6e]):

ActiveRecord::RecordInvalid 
organizations#update
Validation failed: Pick up email Invalid email address for '/'., Pick up email Invalid email address for '[email protected]/[email protected]'.

  | # github.com/rubyforgood/human-essentials/issues/3264
-- | --
  | next if organization.send(field)
  | organization.partners.each do \|partner\|
  | partner.profile.update!(field => organization.send(field))
  | end
  | end
  | end

So the error is that the partner has an invalid email address, since it is actually two email addresses separated by a slash, and we've increased our validation since the data was entered.

This is tricky to reproduce since new Partner data is now validated! So you'll need to force invalid data into a partner to test, and that's how you can do unit test as well.

Expected Behavior

Instead of getting a 500 error on saving the organization details with invalid partner data, show the user the validation error like we would for other organization-level validation errors.

So a flash message with the details of the validation issue.

Steps To Reproduce

  • Force invalid partner email via the rails console, like partner.update_columns(email: 'not/an/email')
  • Use the running UI to edit the bank that the partner is associated with
  • Try to save
  • Get a 500!!!

Environment

No response

Criteria for Completion

  • [ ] User gets a flash error instead of a 500 when saving an organization with invalid partner data
  • [ ] A unit or request spec that validates this behavior

Anything else?

No response

Code of Conduct

  • [X] I've read the Code of Conduct and understand my responsibilities as a member of the Ruby for Good community

awwaiid avatar Oct 13 '24 14:10 awwaiid

I'd like to help with this!

jp524 avatar Oct 17 '24 23:10 jp524

Please do!

cielf avatar Oct 20 '24 03:10 cielf

I was able to reproduce the issue by setting the partner's profile with an invalid email, like: partner.profile.update_columns(pick_up_email: 'not/an/email').

Setting the partner email to be invalid actually doesn't raise any errors. Currently we're only doing validation during the creation process, not when updating: https://github.com/rubyforgood/human-essentials/blob/64d20c1d4b76338080abd60a6710d41c7dc56629/app/models/partner.rb#L51-L52 Is this intentional? If not, I'll go ahead and fix it as part of my PR.

jp524 avatar Oct 20 '24 21:10 jp524

Hey @jp524 -- For this problem I think you should be looking at the pickup person email, not the partner's email. So that's on the partner profile.

That being said, I can't think of a reason that we would want no validation check of the email on updating the partner.

cielf avatar Oct 20 '24 23:10 cielf