seanh
commented
6 years ago I'm still investigating and will flesh this out in more detail later. But based on a first pass, the list of tasks for making user emails optional should be something like:
-
[x] Remove the email not null constraint from the model, and do a DB migration for this.
Also
models.User.get_by_email()needs to never return users with null emails This should be quite straightforward. Pull requests: https://github.com/hypothesis/h/pull/5102, https://github.com/hypothesis/h/pull/5103. -
[ ] Enable the user create API to create users without emails. May require changes to the
UserSignupservice too? But the CLI and sign up web form should still require emails.This is straightforward: https://github.com/hypothesis/h/pull/5105.
The create user API is only available to people who have a client_credentials
AuthClient(eLife, and in the future the LMS app). Do we may want to further limit who can use the API to create users without emails, make it more restrictive than just anyone who has access to this API -
[x] Fix the JSON rendering of users in API responses to handle users with no emails.
On testing this appears to just work. If
User.emailisNonethen it gets rendered in JSON asemail: null. -
[x] Make sure the user update API can still be used with users that don't have emails.
They should still be able to use this API to change their display name, without having to set an email.
On testing this seems to just work, but this PR adds a couple of unit tests to make sure: https://github.com/hypothesis/h/pull/5117
-
[x] Don't send reply notifications to users without emails.
-
[x] Don't send flag notifications (moderation) if the group creator has no email.
Easy fix: https://github.com/hypothesis/h/pull/5121
Note: this means that the moderation flow is broken if the group creator (moderator) has no email.
-
[x] Make sure that users without email addresses can still edit their other account settings on the account settings pages, and don't keep getting error messages about email required or anything like that. LMS users won't have access to these account settings pages at first anyway, but we should probably make sure they work for users without emails.
I've tested and editing the display name, description etc just works.
-
[ ] Fix issues with the form for changing your email address, when the user has no email address.
This is the account settings form at </account/settings>. Currently it renders "None" in the email field, it should just show nothing. Other than that it seems to work: I can set a valid email address by entering one into this field and saving.
Perhaps add tests for this form when the user has no email.
Note that when you set your email address using this form we don't verify the email address in any way. This is an existing problem (even with users who do have email addresses, they can use this form to change their email address to another one without verifying the new one).
Also known issue with changing emails: https://github.com/hypothesis/h/issues/5123
-
[ ] Do something with the "Email me when someone replies to one of my annotations" setting on the notifications settings page. This setting makes no sense for users without emails. Again, LMS users won't have access to this page but we should probably make sure the page works for users without emails.
-
[x] Forgot password form.
This works fine - you cannot reset your password if you have no email because the form works by typing in your email then it searches for your account. There's already both client- and (if the user disables that by e.g. editing the HTML) server-side validation that the entered email is valid (and not null).
Various things should be tested after the changes, including:
- [ ] You still can't sign up without an email
- [ ] You can't edit your email to null
lyzadanger
For https://github.com/hypothesis/product-backlog/issues/597, we know that we cannot rely on trusted email address information in LTI data. As such, we need to find a way to establish third-party accounts for LMS users that don't have usable email addresses.
Ideally, we'd be able to remove the email non-nullable constraint from the database, while leaving it required at other levels of the application for now.
However, we need to cap this exercise. If it turns out to be heinously complex or not feasible, we'll want to find a fake-email-like workaround if we must.
This is done when: