Review feedback: exception handling

This commit is contained in:
Neil Martinsen-Burrell 2023-04-04 14:50:36 -05:00
parent ce9c8abd60
commit e4233870d6
No known key found for this signature in database
GPG key ID: 6A3C818CC10D0184
3 changed files with 38 additions and 13 deletions

View file

@ -1,5 +1,7 @@
"""People are invited by email to administer domains.""" """People are invited by email to administer domains."""
import logging
from django.contrib.auth import get_user_model from django.contrib.auth import get_user_model
from django.db import models, IntegrityError from django.db import models, IntegrityError
@ -9,6 +11,9 @@ from .utility.time_stamped_model import TimeStampedModel
from .user_domain_role import UserDomainRole from .user_domain_role import UserDomainRole
logger = logging.getLogger(__name__)
class DomainInvitation(TimeStampedModel): class DomainInvitation(TimeStampedModel):
INVITED = "invited" INVITED = "invited"
RETRIEVED = "retrieved" RETRIEVED = "retrieved"
@ -39,7 +44,11 @@ class DomainInvitation(TimeStampedModel):
@transition(field="status", source=INVITED, target=RETRIEVED) @transition(field="status", source=INVITED, target=RETRIEVED)
def retrieve(self): def retrieve(self):
"""When an invitation is retrieved, create the corresponding permission.""" """When an invitation is retrieved, create the corresponding permission.
Raises:
RuntimeError if no matching user can be found.
"""
# get a user with this email address # get a user with this email address
User = get_user_model() User = get_user_model()
@ -54,12 +63,12 @@ class DomainInvitation(TimeStampedModel):
# and create a role for that user on this domain # and create a role for that user on this domain
try: try:
UserDomainRole.objects.create( role = UserDomainRole.objects.get(
user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN
) )
except IntegrityError: except UserDomainRole.DoesNotExist:
# should not happen because this user shouldn't retrieve this invitation UserDomainRole.objects.create(user=user, domain=self.domain, role=UserDomainRole.Roles.ADMIN)
# more than once. else:
raise RuntimeError( # something strange happened and this role already existed when
"Invitation would create a role that already exists for this user." # the invitation was retrieved. Log that this occurred.
) logger.warn("Invitation %s was retrieved for a role that already exists.", self)

View file

@ -1,3 +1,5 @@
import logging
from django.contrib.auth.models import AbstractUser from django.contrib.auth.models import AbstractUser
from django.db import models from django.db import models
@ -6,6 +8,9 @@ from .domain_invitation import DomainInvitation
from phonenumber_field.modelfields import PhoneNumberField # type: ignore from phonenumber_field.modelfields import PhoneNumberField # type: ignore
logger = logging.getLogger(__name__)
class User(AbstractUser): class User(AbstractUser):
""" """
A custom user model that performs identically to the default user model A custom user model that performs identically to the default user model
@ -43,5 +48,11 @@ class User(AbstractUser):
for invitation in DomainInvitation.objects.filter( for invitation in DomainInvitation.objects.filter(
email=self.email, status=DomainInvitation.INVITED email=self.email, status=DomainInvitation.INVITED
): ):
try:
invitation.retrieve() invitation.retrieve()
invitation.save() invitation.save()
except RuntimeError:
# retrieving should not fail because of a missing user, but
# if it does fail, log the error so a new user can continue
# logging in
logger.warn("Failed to retrieve invitation %s", invitation, exc_info=True)

View file

@ -177,6 +177,9 @@ class TestInvitations(TestCase):
) )
self.user, _ = User.objects.get_or_create(email=self.email) self.user, _ = User.objects.get_or_create(email=self.email)
# clean out the roles each time
UserDomainRole.objects.all().delete()
def test_retrieval_creates_role(self): def test_retrieval_creates_role(self):
self.invitation.retrieve() self.invitation.retrieve()
self.assertTrue(UserDomainRole.objects.get(user=self.user, domain=self.domain)) self.assertTrue(UserDomainRole.objects.get(user=self.user, domain=self.domain))
@ -187,11 +190,13 @@ class TestInvitations(TestCase):
with self.assertRaises(RuntimeError): with self.assertRaises(RuntimeError):
self.invitation.retrieve() self.invitation.retrieve()
def test_retrieve_existing_role_error(self): def test_retrieve_existing_role_no_error(self):
# make the overlapping role # make the overlapping role
UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain) UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.ADMIN)
with self.assertRaises(RuntimeError): # this is not an error but does produce a console warning
with less_console_noise():
self.invitation.retrieve() self.invitation.retrieve()
self.assertEqual(self.invitation.status, DomainInvitation.RETRIEVED)
def test_retrieve_on_first_login(self): def test_retrieve_on_first_login(self):
"""A new user's first_login callback retrieves their invitations.""" """A new user's first_login callback retrieves their invitations."""