Repository: carpentries/amy
Issue: Check constraints upon trying to add a new person via update request

There's an error when admin accepts a profile update which is not matched to any person, and a collision in usernames exists.



Changed files:
workshops/views.py, workshops/test/test_profile_update_request.py

diff --git a/workshops/test/test_profile_update_request.py b/workshops/test/test_profile_update_request.py
index af0ac2a4..8fda77a0 100644
--- a/workshops/test/test_profile_update_request.py
+++ b/workshops/test/test_profile_update_request.py
@@ -179,6 +179,70 @@ class TestProfileUpdateRequest(TestBase):
         self.assertEqual(set(person.lessons.all()),
                          set(Lesson.objects.all()[0:2]))
 
+    def test_database_constraints_when_updating(self):
+        """Ensure unique constraints are checked when processing a profile
+        update.
+
+        This is a regression test for #1237:
+        https://github.com/swcarpentry/amy/issues/1237
+        """
+
+        # Create a profile update for Ron but include Hermione's and Harry's
+        # data. Only Ron doesn't have a GitHub nor Twitter accounts.
+        pr1 = ProfileUpdateRequest.objects.create(
+            active=True,
+            personal='Ron', family='Weasley', airport_iata='AAA',
+            gender='M', github='hpotter',
+        )
+        pr2 = ProfileUpdateRequest.objects.create(
+            active=True,
+            personal='Ron', family='Weasley', airport_iata='AAA',
+            gender='M', twitter='herself',
+        )
+        pr3 = ProfileUpdateRequest.objects.create(
+            active=True,
+            personal='Ron', family='Weasley', airport_iata='AAA',
+            gender='M', email='ron@weasley.com',
+        )
+
+        # ensure no new person was created so far
+        self.assertEqual(
+            Person.objects.filter(username__startswith='weasley_ron').count(),
+            1
+        )
+
+        # now try to accept the profile updates for Ron Weasley
+        rv1 = self.client.get(reverse('profileupdaterequest_accept',
+                                      args=[pr1.pk, self.ron.pk]))
+        self.assertEqual(rv1.status_code, 302)
+        rv2 = self.client.get(reverse('profileupdaterequest_accept',
+                                      args=[pr2.pk, self.ron.pk]))
+        self.assertEqual(rv2.status_code, 302)
+
+        # force create a new person from profile update, ensure no fail
+        rv3 = self.client.get(reverse('profileupdaterequest_accept',
+                                      args=[pr3.pk]))
+        self.assertEqual(rv3.status_code, 302)
+
+        # no update to github or twitter handles
+        self.ron.refresh_from_db()
+        self.assertEqual(self.ron.github, None)
+        self.assertEqual(self.ron.twitter, None)
+
+        # update requests are still active
+        pr1.refresh_from_db()
+        pr2.refresh_from_db()
+        pr3.refresh_from_db()
+        self.assertEqual(pr1.active, True)
+        self.assertEqual(pr2.active, True)
+        self.assertEqual(pr3.active, False)
+
+        # make sure pr3 made a new person
+        self.assertEqual(
+            Person.objects.filter(username__startswith='weasley_ron').count(),
+            2
+        )
+
 
 class TestUpdateRequestWithLowercasedAirport(TestBase):
     """Regression tests for [#1109] bug -- airport IATA code should be case
diff --git a/workshops/views.py b/workshops/views.py
index 9e3d999e..c8faa8e5 100644
--- a/workshops/views.py
+++ b/workshops/views.py
@@ -18,7 +18,7 @@ from django.core.exceptions import (
     PermissionDenied,
 )
 from django.core.urlresolvers import reverse, reverse_lazy
-from django.db import IntegrityError
+from django.db import IntegrityError, transaction
 from django.db.models import (
     Case,
     When,
@@ -2351,30 +2351,51 @@ def profileupdaterequest_accept(request, request_id, person_id=None):
     person.gender = profileupdate.gender
     person.user_notes = profileupdate.notes
 
-    # we need person to exist in the database in order to set domains and
-    # lessons
-    if not person.id:
-        person.save()
-
-    person.domains = list(profileupdate.domains.all())
-    person.languages.set(profileupdate.languages.all())
-
-    # Since Person.lessons uses a intermediate model Qualification, we ought to
-    # operate on Qualification objects instead of using Person.lessons as a
-    # list.
+    with transaction.atomic():
+        # we need person to exist in the database in order to set domains and
+        # lessons
+        if not person.id:
+            try:
+                person.username = create_username(person.personal,
+                                                  person.family)
+                person.save()
+            except IntegrityError:
+                messages.error(
+                    request,
+                    'Cannot update profile: some database constraints weren\\'t'
+                    'fulfilled. Make sure that user name, GitHub user name,'
+                    'Twitter user name, or email address are unique.'
+                )
+                return redirect(profileupdate.get_absolute_url())
 
-    # erase old lessons
-    Qualification.objects.filter(person=person).delete()
-    # add new
-    Qualification.objects.bulk_create([
-        Qualification(person=person, lesson=L)
-        for L in profileupdate.lessons.all()
-    ])
+        person.domains = list(profileupdate.domains.all())
+        person.languages.set(profileupdate.languages.all())
 
-    person.save()
+        try:
+            person.save()
+        except IntegrityError:
+            messages.error(
+                request,
+                'Cannot update profile: some database constraints weren\\'t'
+                'fulfilled. Make sure that user name, GitHub user name,'
+                'Twitter user name, or email address are unique.'
+            )
+            return redirect(profileupdate.get_absolute_url())
+
+        # Since Person.lessons uses a intermediate model Qualification, we ought to
+        # operate on Qualification objects instead of using Person.lessons as a
+        # list.
+
+        # erase old lessons
+        Qualification.objects.filter(person=person).delete()
+        # add new
+        Qualification.objects.bulk_create([
+            Qualification(person=person, lesson=L)
+            for L in profileupdate.lessons.all()
+        ])
 
-    profileupdate.active = False
-    profileupdate.save()
+        profileupdate.active = False
+        profileupdate.save()
 
     if person_id is None:
         messages.success(request,