Skip to content

Various forgotten fixups necessary for emails being nullable #667

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

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

myieye
Copy link
Contributor

@myieye myieye commented Mar 27, 2024

Fixes #657

FYI the form validation change (.nullable()) doesn't allow a user to delete an existing email address.
But even if they could the server would ignore it, because it ignores empty values.

@myieye myieye requested a review from rmunn March 27, 2024 13:57
@myieye myieye marked this pull request as draft March 27, 2024 13:57
Copy link

github-actions bot commented Mar 27, 2024

UI unit Tests

11 tests  ±0   11 ✅ ±0   0s ⏱️ ±0s
 3 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 8aba105. ± Comparison against base commit 85f4c8b.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Mar 27, 2024

C# Unit Tests

26 tests   26 ✅  4s ⏱️
 6 suites   0 💤
 1 files     0 ❌

Results for commit 8aba105.

♻️ This comment has been updated with latest results.

@myieye myieye marked this pull request as ready for review March 28, 2024 15:06
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable to me, I didn't test it though

Copy link
Contributor

@rmunn rmunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, and everything seems to work. One feature that's not in here is editing the username of a user with no email (can edit display name, but not username). That should probably be in a different PR, though. This one LGTM.

@rmunn
Copy link
Contributor

rmunn commented Apr 1, 2024

One issue revealed in testing: if you edit a user with no email, type something into the email box, then delete it, you can't submit the form. (Because the field went from undefined/null to '' (empty string) and now zod won't allow form submission).

I triggered this by accident when I was trying to edit the display name of a bulk-added user, and edited the email field by mistake.

Copy link
Contributor

@rmunn rmunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just turned up an issue I missed in earlier testing: if you accidentally type into the email field, then delete what you typed, you end up unable to submit the form.

image

Allowing that scenario would involve treating an empty string in the email column as if it was a null/undefined. If we add that, we'll also accidentally enable admins to delete a user's email address. That may or may not be desirable. Still, I think this scenario should be fixed somehow.

@myieye
Copy link
Contributor Author

myieye commented Apr 3, 2024

I think usernames can be read-only. I think that's fairly common.
Email addresses can't, because a user might (e.g.) want to migrate to a different provider.

@myieye
Copy link
Contributor Author

myieye commented Apr 3, 2024

@rmunn I mapped falsy emails to null if the user doesn't currently have an email address (8aba105)

@myieye myieye requested a review from rmunn April 3, 2024 14:35
Copy link
Contributor

@rmunn rmunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great; everything looks good now.

@myieye myieye merged commit 457a7e5 into develop Apr 4, 2024
@myieye myieye deleted the fixup/nullable-email-fixups branch April 4, 2024 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admins can't edit guest users who don't have email addresses
3 participants