Skip to content
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

[ENG-4903] Fixes issue with email confirmation links failing due to database congestion #10662

4 changes: 2 additions & 2 deletions api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from website.profile.views import update_osf_help_mails_subscription, update_mailchimp_subscription
from api.nodes.serializers import NodeSerializer, RegionRelationshipField
from api.base.schemas.utils import validate_user_json, from_json
from framework.auth.views import send_confirm_email
from framework.auth.views import send_confirm_email_async
from api.base.versioning import get_kebab_snake_case_field


Expand Down Expand Up @@ -596,7 +596,7 @@ def create(self, validated_data):
token = user.add_unconfirmed_email(address)
user.save()
if CONFIRM_REGISTRATIONS_BY_EMAIL:
send_confirm_email(user, email=address)
send_confirm_email_async(user, email=address)
user.email_last_sent = timezone.now()
user.save()
except ValidationError as e:
Expand Down
4 changes: 2 additions & 2 deletions api/users/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
from django.http import JsonResponse
from django.utils import timezone
from framework.auth.core import get_user
from framework.auth.views import send_confirm_email
from framework.auth.views import send_confirm_email_async
from framework.auth.oauth_scopes import CoreScopes, normalize_scopes
from framework.auth.exceptions import ChangePasswordError
from framework.utils import throttle_period_expired
Expand Down Expand Up @@ -900,7 +900,7 @@ def get_object(self):
if self.request.method == 'GET' and is_truthy(self.request.query_params.get('resend_confirmation')):
if not confirmed and settings.CONFIRM_REGISTRATIONS_BY_EMAIL:
if throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE):
send_confirm_email(user, email=address, renew=True)
send_confirm_email_async(user, email=address, renew=True)
user.email_last_sent = timezone.now()
user.save()

Expand Down
4 changes: 2 additions & 2 deletions api_tests/institutions/views/test_institution_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

from framework.auth import signals, Auth
from framework.auth.core import get_user
from framework.auth.views import send_confirm_email
from framework.auth.views import send_confirm_email_async

from osf.models import OSFUser, InstitutionAffiliation, InstitutionStorageRegion
from osf.models.institution import SsoFilterCriteriaAction
Expand Down Expand Up @@ -454,7 +454,7 @@ def test_user_external_unconfirmed(self, app, institution, url_auth_institution)
assert user.external_identity

# Send confirm email in order to add new email verifications
send_confirm_email(
send_confirm_email_async(
user,
user.username,
external_id_provider=external_id_provider,
Expand Down
20 changes: 10 additions & 10 deletions api_tests/users/views/test_user_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ def test_unconfirmed_email_included(self, app, url, payload, user_one, unconfirm
assert res.status_code == 200
assert unconfirmed_address in [result['attributes']['email_address'] for result in res.json['data']]

@mock.patch('api.users.serializers.send_confirm_email')
@mock.patch('api.users.serializers.send_confirm_email_async')
def test_create_new_email_current_user(self, mock_send_confirm_mail, user_one, user_two, app, url, payload):
new_email = 'hhh@wwe.test'
payload['data']['attributes']['email_address'] = new_email
Expand All @@ -229,7 +229,7 @@ def test_create_new_email_current_user(self, mock_send_confirm_mail, user_one, u
assert new_email in user_one.unconfirmed_emails
assert mock_send_confirm_mail.called

@mock.patch('api.users.serializers.send_confirm_email')
@mock.patch('api.users.serializers.send_confirm_email_async')
def test_create_new_email_not_current_user(self, mock_send_confirm_mail, app, url, payload, user_one, user_two):
new_email = 'HHH@wwe.test'
payload['data']['attributes']['email_address'] = new_email
Expand All @@ -239,7 +239,7 @@ def test_create_new_email_not_current_user(self, mock_send_confirm_mail, app, ur
assert new_email not in user_one.unconfirmed_emails
assert not mock_send_confirm_mail.called

@mock.patch('api.users.serializers.send_confirm_email')
@mock.patch('api.users.serializers.send_confirm_email_async')
def test_create_email_already_exists(self, mock_send_confirm_mail, app, url, payload, user_one):
new_email = 'hello@email.test'
Email.objects.create(address=new_email, user=user_one)
Expand Down Expand Up @@ -578,28 +578,28 @@ def test_updating_verified_for_merge(self, app, user_one, user_two, payload):
assert res.json['data']['attributes']['confirmed'] is True
assert res.json['data']['attributes']['is_merge'] is False

@mock.patch('api.users.views.send_confirm_email')
def test_resend_confirmation_email(self, mock_send_confirm_email, app, user_one, unconfirmed_url, confirmed_url):
@mock.patch('api.users.views.send_confirm_email_async')
def test_resend_confirmation_email(self, mock_send_confirm_email_async, app, user_one, unconfirmed_url, confirmed_url):
url = '{}?resend_confirmation=True'.format(unconfirmed_url)
res = app.get(url, auth=user_one.auth)
assert res.status_code == 202
assert mock_send_confirm_email.called
call_count = mock_send_confirm_email.call_count
assert mock_send_confirm_email_async.called
call_count = mock_send_confirm_email_async.call_count

# make sure setting false does not send confirm email
url = '{}?resend_confirmation=False'.format(unconfirmed_url)
res = app.get(url, auth=user_one.auth)
# should return 200 instead of 202 because nothing has been done
assert res.status_code == 200
assert mock_send_confirm_email.call_count
assert mock_send_confirm_email_async.call_count

# make sure normal GET request does not re-send confirmation email
res = app.get(unconfirmed_url, auth=user_one.auth)
assert mock_send_confirm_email.call_count == call_count
assert mock_send_confirm_email_async.call_count == call_count
assert res.status_code == 200

# resend confirmation with confirmed email address does not send confirmation email
url = '{}?resend_confirmation=True'.format(confirmed_url)
res = app.get(url, auth=user_one.auth)
assert mock_send_confirm_email.call_count == call_count
assert mock_send_confirm_email_async.call_count == call_count
assert res.status_code == 200
14 changes: 8 additions & 6 deletions framework/auth/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from website.util.metrics import CampaignClaimedTags, CampaignSourceTags
from website.ember_osf_web.decorators import ember_flag_is_active
from osf import features
from framework.postcommit_tasks.handlers import enqueue_postcommit_task
# from osf.models import PreprintProvider


Expand Down Expand Up @@ -800,7 +801,6 @@ def unconfirmed_email_add(auth=None):
'removed_email': json_body['address']
}, 200


def send_confirm_email(user, email, renew=False, external_id_provider=None, external_id=None, destination=None):
cslzchen marked this conversation as resolved.
Show resolved Hide resolved
"""
Sends `user` a confirmation to the given `email`.
Expand All @@ -815,7 +815,6 @@ def send_confirm_email(user, email, renew=False, external_id_provider=None, exte
:return:
:raises: KeyError if user does not have a confirmation token for the given email.
"""

confirmation_url = user.get_confirmation_url(
email,
external=True,
Expand Down Expand Up @@ -872,6 +871,9 @@ def send_confirm_email(user, email, renew=False, external_id_provider=None, exte
logo=logo if logo else settings.OSF_LOGO
)

def send_confirm_email_async(user, email, renew=False, external_id_provider=None, external_id=None, destination=None):
enqueue_postcommit_task(send_confirm_email, (user, email, renew, external_id_provider, external_id, destination), {})


def register_user(**kwargs):
"""
Expand Down Expand Up @@ -942,7 +944,7 @@ def register_user(**kwargs):
)

if settings.CONFIRM_REGISTRATIONS_BY_EMAIL:
send_confirm_email(user, email=user.username)
send_confirm_email_async(user, email=user.username)
message = language.REGISTRATION_SUCCESS.format(email=user.username)
return {'message': message}
else:
Expand Down Expand Up @@ -989,7 +991,7 @@ def resend_confirmation_post(auth):
if user:
if throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE):
try:
send_confirm_email(user, clean_email, renew=True)
send_confirm_email_async(user, clean_email, renew=True)
except KeyError:
# already confirmed, redirect to dashboard
status_message = 'This email {0} has already been confirmed.'.format(clean_email)
Expand Down Expand Up @@ -1094,7 +1096,7 @@ def external_login_email_post():
# 2. add unconfirmed email and send confirmation email
user.add_unconfirmed_email(clean_email, external_identity=external_identity)
user.save()
send_confirm_email(
send_confirm_email_async(
user,
clean_email,
external_id_provider=external_id_provider,
Expand Down Expand Up @@ -1124,7 +1126,7 @@ def external_login_email_post():
# TODO: [#OSF-6934] update social fields, verified social fields cannot be modified
user.save()
# 3. send confirmation email
send_confirm_email(
send_confirm_email_async(
user,
user.username,
external_id_provider=external_id_provider,
Expand Down
8 changes: 4 additions & 4 deletions tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3325,7 +3325,7 @@ def test_register_email_without_accepted_tos(self, _):
user = OSFUser.objects.get(username=email)
assert_equal(user.accepted_terms_of_service, None)

@mock.patch('framework.auth.views.send_confirm_email')
@mock.patch('framework.auth.views.send_confirm_email_async')
def test_register_scrubs_username(self, _):
url = api_url_for('register_user')
name = "<i>Eunice</i> O' \"Cornwallis\"<script type='text/javascript' src='http://www.cornify.com/js/cornify.js'></script><script type='text/javascript'>cornify_add()</script>"
Expand Down Expand Up @@ -3509,8 +3509,8 @@ def test_register_after_being_invited_as_unreg_contributor(self, mock_update_sea
assert_true(new_user.check_password(password))
assert_equal(new_user.fullname, real_name)

@mock.patch('framework.auth.views.send_confirm_email')
def test_register_sends_user_registered_signal(self, mock_send_confirm_email):
@mock.patch('framework.auth.views.send_confirm_email_async')
def test_register_sends_user_registered_signal(self, mock_send_confirm_email_async):
url = api_url_for('register_user')
name, email, password = fake.name(), fake_email(), 'underpressure'
with capture_signals() as mock_signals:
Expand All @@ -3525,7 +3525,7 @@ def test_register_sends_user_registered_signal(self, mock_send_confirm_email):
)
assert_equal(mock_signals.signals_sent(), set([auth.signals.user_registered,
auth.signals.unconfirmed_user_created]))
assert_true(mock_send_confirm_email.called)
assert_true(mock_send_confirm_email_async.called)

@mock.patch('framework.auth.views.mails.send_mail')
def test_resend_confirmation(self, send_mail):
Expand Down
6 changes: 3 additions & 3 deletions website/profile/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from framework.auth.decorators import must_be_logged_in
from framework.auth.decorators import must_be_confirmed
from framework.auth.exceptions import ChangePasswordError
from framework.auth.views import send_confirm_email
from framework.auth.views import send_confirm_email_async
from framework.auth.signals import (
user_account_merged,
user_account_deactivated,
Expand Down Expand Up @@ -84,7 +84,7 @@ def resend_confirmation(auth):

# TODO: This setting is now named incorrectly.
if settings.CONFIRM_REGISTRATIONS_BY_EMAIL:
send_confirm_email(user, email=address)
send_confirm_email_async(user, email=address)
user.email_last_sent = timezone.now()

user.save()
Expand Down Expand Up @@ -167,7 +167,7 @@ def update_user(auth):
if not throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE):
raise HTTPError(http_status.HTTP_400_BAD_REQUEST,
data={'message_long': 'Too many requests. Please wait a while before adding an email to your account.'})
send_confirm_email(user, email=address)
send_confirm_email_async(user, email=address)

############
# Username #
Expand Down
Loading