Refactor email confirmation pathways to use common error handling #5891
Conversation
hackerkid
changed the title
There was a problem hiding this comment.
This looks great, thanks @hackerkid! Left a few comments. Before this is merged, we'll also have to add something like
try:
obj = get_object_from_key
except ConfirmationKeyException:
obj = False
or similar everywhere else that get_object_from_key is called.
confirmation/models.py
Outdated
| @@ -34,6 +36,14 @@ def __init__(self, error_type): | |||
| super(Exception, self).__init__() | |||
| self.error_type = error_type | |||
|
|
|||
| def render_confirmation_key_error(request, ctx, exception): | |||
There was a problem hiding this comment.
Let's use context here instead of ctx. The ctx elsewhere in the code is legacy from pre-2014.
| <hr/> | ||
| <p class="lead">Whoops. We couldn't find your confirmation link in the system!</p> | ||
|
|
||
| {% if verbose_support_offers %} |
There was a problem hiding this comment.
We can just set verbose_support_offers to True, and remove the False branch here and in the other confirmation templates.
|
|
||
| <div class="pitch"> | ||
| <hr/> | ||
| <p class="lead">Whoops. The confirmation link has expired.</p> |
There was a problem hiding this comment.
After this,
<p>If you're not sure how to generate a new one, shoot us a line at
<a href="mailto:{{ support_email }}">{{ support_email }}</a>
and we'll get this resolved shortly.
</p>
confirmation/models.py
Outdated
| @@ -52,14 +52,16 @@ def generate_key(): | |||
|
|
|||
| def get_object_from_key(confirmation_key): | |||
| # type: (str) -> Union[bool, PreregistrationUser, EmailChangeStatus] | |||
| if len(confirmation_key) != 24: | |||
There was a problem hiding this comment.
it can be 24 or 40. Before this line, we can add a comment like # confirmation keys used to be 40 characters
zerver/views/user_settings.py
Outdated
| try: | ||
| obj = get_object_from_key(confirmation_key) | ||
| except ConfirmationKeyException as exception: | ||
| ctx = {'verbose_support_offers': settings.VERBOSE_SUPPORT_OFFERS} |
There was a problem hiding this comment.
this can be removed (and possibly the ctx argument from render_confirmation_key_error can be removed entirely?)
hackerkid
force-pushed
the
confirmation-email
branch
from
93e5746
to
a6a0517
Compare
hackerkid
force-pushed
the
confirmation-email
branch
5 times, most recently
from
e82d104
to
521864c
Compare
|
@rishig Updated. |
There was a problem hiding this comment.
Great, thanks @hackerkid! Noticed a few spelling things, once those are resolved this will be ready for merge.
confirmation/models.py
Outdated
| @@ -24,6 +26,24 @@ | |||
| import string | |||
| from typing import Any, Dict, Optional, Text, Union | |||
|
|
|||
| class ConfirmationKeyException(Exception): | |||
| WORNG_LENGTH = 1 | |||
There was a problem hiding this comment.
oops, just noticed this should be WRONG, instead of WORNG
|
|
||
| <div class="pitch"> | ||
| <hr/> | ||
| <p class="lead">Whoops. We couldn't find your confirmation link in the system!</p> |
There was a problem hiding this comment.
I would make this end in a . instead of !
| <p class="lead">Whoops. The confirmation link is malformed.</p> | ||
|
|
||
| <p>Make sure you copied the link correctly in to your browser. If you're | ||
| still encountering this page, its probably our fault. We're sorry.</p> |
hackerkid
force-pushed
the
confirmation-email
branch
from
521864c
to
ed3ce72
Compare
hackerkid
force-pushed
the
confirmation-email
branch
from
ed3ce72
to
e40adc9
Compare
|
@rishig Fixed :) |
|
Thanks @hackerkid! |
|
Merged, thanks @hackerkid! |
Related issue #5739
@rishig Thoughts?