Email verification #3405

macqueen · 2016-06-03T22:50:00Z

Not super fancy -- just based it on how we do password reset.

@ckj let me know if you have suggestions for making pages with very little text look less silly.

@ehfeng @getsentry/python

dcramer · 2016-06-04T21:05:30Z

A couple of thoughts:

We're going to want to support multiple email addresses, each of which would need verified
I'm not sure of the consequences of reusing LostPasswordHash here, and due to the need for multiple email addresses it might be worthwhile to set this up differently

macqueen · 2016-06-06T17:24:08Z

@dcramer ok, do you think it's a better idea to just hold off on this then? or just make it more flexible by making a separate model that isn't tied to the user (or at least doesn't have a unique constraint on the user)?

dcramer · 2016-06-06T17:26:55Z

@macqueen yeah I think it'd be best to get both birds here, and probably just have like an Email model (or LinkedEmail or something similar). Effectively we want to take a similar approach to how GitHub does email validation. I do think we can at least reuse some of the password code (even if we copy paste it).

ckj · 2016-06-06T23:41:30Z

@macqueen Looks good! I think I would try the .narrow or .auth body classes for this template. I'd also do something like <a class="btn btn-primary">Sign in</a> on the "has confirmed" message. Maybe something similar on the error view.

macqueen · 2016-06-07T21:16:17Z

Now this looks like:

ckj · 2016-06-07T21:31:50Z

Woah, just now noticing that sweet 'fi' ligature.

@macqueen I think this is much better. Two final suggestions:

Let's emphasize the email address: <strong>jess+s@getsentry.com</strong>
Can we change the heading based on the state? I'm thinking something like "Confirmation Successful," "Oops, something happened," and "Confirmation Sent" respective to the order your screenshots

ckj · 2016-06-08T06:10:25Z

😍

macqueen · 2016-06-09T23:41:01Z

@dcramer and/or @mattrobenolt do you have any other suggestions/feedback?

mattrobenolt · 2016-06-09T23:47:02Z

src/sentry/models/email.py

+
+
+class Email(Model):
+    __core__ = True


This isn't needed. The default is implied True.

macqueen · 2016-06-13T20:01:31Z

I think this is ready to merge unless anyone has any other suggestions.

dcramer · 2016-06-13T20:02:33Z

src/sentry/models/useremail.py

+
+    def set_hash(self):
+        self.date_hash_added = timezone.now()
+        characters = u'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'


would be worthwhile to define this at module level rather than here

JTCunning · 2016-06-15T22:23:32Z

src/sentry/models/user.py

+            }
+            msg = MessageBuilder(
+                subject='%sConfirm Email' % (options.get('mail.subject-prefix'),),
+                template='sentry/emails/confirm_email.txt',


Add type='user.confirm_email', below this line to get sweet sweet mail.{queued,sent} logging events.

If you run tests and they wig out or something doesn't work, rebasing off of origin/master will fix it.

done. thanks!

get initial email verification flow working add user is_verified field add tests for email verification flow linting, a few tweaks to make workflow smoother fix bug where verification email went to old email when user changed email address remove unnecessary change use a separate email model to make validation more flexible fix tests and migrations ui pr feedback, get rid of race condition when creating new email models ui/header feedback matt's pr feedback use signals to create user email model (instead of save method) add type to MessageBuilder

macqueen force-pushed the email-verification branch from e6fff74 to 0a52c77 Compare June 6, 2016 22:51

macqueen force-pushed the email-verification branch from 0b7007f to ce9da3e Compare June 9, 2016 22:37

mattrobenolt reviewed Jun 9, 2016
View reviewed changes

src/sentry/models/email.py Outdated

class Email(Model):

__core__ = True

Copy link

Contributor

mattrobenolt Jun 9, 2016

Choose a reason for hiding this comment

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

This isn't needed. The default is implied True.

macqueen force-pushed the email-verification branch 2 times, most recently from 6a551f7 to ee5d87b Compare June 13, 2016 19:06

dcramer reviewed Jun 13, 2016
View reviewed changes

macqueen force-pushed the email-verification branch from 0c73ac7 to 88ab53d Compare June 13, 2016 21:52

JTCunning reviewed Jun 15, 2016
View reviewed changes

macqueen force-pushed the email-verification branch from 88ab53d to cf2255e Compare June 16, 2016 00:10

macqueen force-pushed the email-verification branch from cf2255e to 9d3bd32 Compare June 16, 2016 17:34

macqueen merged commit 64f0db6 into master Jun 16, 2016

macqueen deleted the email-verification branch June 16, 2016 17:43

github-actions bot locked and limited conversation to collaborators Dec 23, 2020

Uh oh!

Email verification #3405

Email verification #3405

Uh oh!

Conversation

macqueen commented Jun 3, 2016

Uh oh!

dcramer commented Jun 4, 2016

Uh oh!

macqueen commented Jun 6, 2016

Uh oh!

dcramer commented Jun 6, 2016

Uh oh!

ckj commented Jun 6, 2016

Uh oh!

macqueen commented Jun 7, 2016

Uh oh!

ckj commented Jun 7, 2016

Uh oh!

ckj commented Jun 8, 2016

Uh oh!

macqueen commented Jun 9, 2016

Uh oh!

mattrobenolt Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

macqueen commented Jun 13, 2016

Uh oh!

dcramer Jun 13, 2016

Choose a reason for hiding this comment

Uh oh!

JTCunning Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

macqueen Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

JTCunning Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants