Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address pickup email issues #4486

Closed
3 tasks
cielf opened this issue Jun 30, 2024 · 15 comments · Fixed by #4648
Closed
3 tasks

Address pickup email issues #4486

cielf opened this issue Jun 30, 2024 · 15 comments · Fixed by #4648
Assignees

Comments

@cielf
Copy link
Collaborator

cielf commented Jun 30, 2024

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

  • may enter emails in the format above, only.
  • distribution emails properly cc'd to the indicated individuals
  • tests to confirm above behaviour works
@cielf cielf added Help Wanted Groomed + open to all! Difficulty—Beginner core team Groomed but likely needs expert knowledge and removed Help Wanted Groomed + open to all! Difficulty—Beginner labels Jun 30, 2024
@Naraveni
Copy link
Contributor

Naraveni commented Jul 3, 2024

Hey , I would to like to pick this up

@cielf cielf assigned Naraveni and unassigned Naraveni Jul 3, 2024
@cielf
Copy link
Collaborator Author

cielf commented Jul 3, 2024

@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
Copy link
Collaborator Author

cielf commented Jul 3, 2024

@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
Copy link
Collaborator

dorner commented Jul 5, 2024

@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
Copy link
Collaborator Author

cielf commented Jul 5, 2024

@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
Copy link
Collaborator

dorner commented Jul 5, 2024

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

@cielf
Copy link
Collaborator Author

cielf commented Jul 5, 2024

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
Copy link
Collaborator Author

cielf commented Jul 5, 2024

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
Copy link
Collaborator Author

cielf commented Jul 6, 2024

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
Copy link
Collaborator

dorner commented Jul 7, 2024

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

@cielf
Copy link
Collaborator Author

cielf commented Jul 7, 2024

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

@cielf cielf added Help Wanted Groomed + open to all! Difficulty—Beginner and removed core team Groomed but likely needs expert knowledge labels Jul 7, 2024
@DariusPirvulescu
Copy link
Contributor

DariusPirvulescu commented Sep 9, 2024

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
Copy link
Collaborator Author

cielf commented Sep 10, 2024

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
Copy link
Collaborator Author

cielf commented Sep 10, 2024

We usually put validations on the model if we can.

@github-actions github-actions bot removed the Help Wanted Groomed + open to all! label Sep 10, 2024
@cielf
Copy link
Collaborator Author

cielf commented Sep 10, 2024

Please go ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants