search

rubyforgood / human-essentials

Human Essentials is an inventory management system for diaper, incontinence, and period-supply banks. It supports them in distributing to partners, tracking inventory, and reporting stats and analytics.
https://humanessentials.app
MIT License
447 stars 478 forks source link

Address pickup email issues #4486

Closed cielf closed 2 weeks ago

cielf commented 3 months ago

Summary

Restrict the pickup person email on the partner profile to valid email formats.

Why

We get errors on the distribution confirmation email send which goes to the pickup person, among others. Investigation shows that there are at least 20 partners with multiple emails in the pickup person email

Details

Allow properly formatted (a@b.c) emails, comma-separated, with optional whitespace. Split the pickup email into its components before sending. Also limits the number of pickup emails to 3.

Caveat

Before this can go into production, we will need to test against production data - whether it will fail softly on approval of existing partners with pickup person emails that do not match the new format(i.e. gives an actionable error rather than a 500) If it doesn't fail gently, we will need to address the data or make sure an actionable error appears in that case.

This is also going to need special communication, potentially, if we have to go the migration route.

Criteria for completion

Naraveni commented 3 months ago

Hey , I would to like to pick this up

cielf commented 3 months ago

@Naraveni -- We've got it marked as core team only , because we are going to have to do a bunch of testing against production data, and it will probably be held up awhile we do that. The change itself is a beginner difficulty.

cielf commented 3 months ago

@Naraveni If you are looking for something to pick up -- the issues with the label "Rubyforgood DC 2024" are now available, since the event is over -- we haven't marked the difficulty on them, but I think #4399 is beginner-friendly -- though you'll need to know about flipper, we can have a discussion on how to do that on that issue.

dorner commented 3 months ago

@cielf can we not just allow comma-separated e-mails and store that as a string? We can update the validation to allow for this and separate them out when the e-mail is sent rather than make model changes. This should minimize the number of issues with existing accounts.

cielf commented 3 months ago

@dorner That's pretty much what we're talking about. The current data is a mess, however. So that we'd get failures on things like requesting recertification, which are done from other screens.

dorner commented 3 months ago

ah I see. I can do a quick look to see which addresses wouldn't match the validation.

cielf commented 3 months ago

Now, the pickup email is not used for the emails at request recertification, or inviting. So maybe we can work around that? (That we would only care at the times when people are actually editing the profile?)

cielf commented 3 months ago

But/and we would want to notify the banks involved so they can get the info legitimately cleaned up, so the emails can actually go out... Thoughts?

cielf commented 3 months ago

Wait... Is the status (recertification requested, etc) on the partner itself rather than the profile (It has to do with the profile from a business pov -- you look at the profile to decide to approve, etc .) ? If it is.... maybe we can just put the verification in and it will eventually come out in the wash. Put the fix in, then let the banks with partners with problems know.

dorner commented 3 months ago

Yep, the status is on the partner, not the profile. So we would probably be OK just adding in the validation.

cielf commented 3 months ago

Well, then @Naraveni -- are you still interested in this one? It looks like it's less complicated than I thought.

DariusPirvulescu commented 3 weeks ago

Hello, since it's been more than a month, I can take a look into it. To clarify, do we need to prevent having multiple email addresses on the pick-up person?

The Primary Contact & Executive Director can also have multiple email addresses. Wouldn't this also be a problem?

If so, do you need to add the validation on this controller? Or should it be on the profile model?

cielf commented 3 weeks ago

I think the original ask was to limit it to 3 emails. I don't think we send anything to the Executive Director -- it's an information only field. We probably should look at the primary contact, though.

cielf commented 3 weeks ago

We usually put validations on the model if we can.

cielf commented 3 weeks ago

Please go ahead.