[server, dashboard] Refactor User.getPrimaryEmail to return "string | undefined" instead of throwing an error #9446
Conversation
… undefined" instead of throwing an error
|
/werft run 👍 started the job as gitpod-build-gpl-8359-primary.2 |
There was a problem hiding this comment.
It feels like in various places, we treat an empty or undefined email differently. Sometimes we use empty string, sometimes we use "---", sometimes we keep it as undefined. Is it at all possible to choose one? In practice, we could encode that logic inside getPrimaryEmail() and not have to cast or convert in so many places.
I think in general it is not possible, because depending on your context, this has different meaning. Wrt to the different options to render in the dashboard("---" vs ""): I basically kept the previous decisions in place, scoping this PR solely on the reliability parts to never break the renderning. |
| @@ -50,7 +50,7 @@ export default function UserDetail(p: { user: User }) { | |||
| }, [p.user]); | |||
|
|
|||
| const email = User.getPrimaryEmail(p.user); | |||
| const emailDomain = email.split("@")[email.split("@").length - 1]; | |||
| const emailDomain = email ? email.split("@")[email.split("@").length - 1] : undefined; | |||
There was a problem hiding this comment.
[nit] Nothing to fix here, just a comment. This is in general quite fishy, but oh well. In practice, it's extremely hard to parse email addresses well as the spec is so very broad, the best we can often do is check the email is valid by sending an email.
There was a problem hiding this comment.
Absolutely agree in theory. Currently the only assumptions we make is that a) it has a '@' and b) non-empty domain part.
I know that both are not guaranteed by the spec, but held in practice well enough.
And then there is the other point that we should not be doing this randomly in the frontend, but should try to keep in a central place (or library).... 👍
roboquat
added
deployed: webapp
Description
Related Issue(s)
Fixes #8359
How to test
Release Notes
Documentation