Merge pull request #265 from DemocracyClub/fix-transaction-safety

Fixes for uses of transaction.atomic

candidates/management/commands/candidates_add_twitter_images_to_queue.py

@@ -94,5 +94,4 @@ def handle(self, *args, **options):
         # should add their Twitter avatar to the image moderation
         # queue:
         for person in Person.objects.select_related('extra').order_by('name'):
-            with transaction.atomic():
-                self.handle_person(person)
+            self.handle_person(person)

candidates/management/commands/candidates_update_twitter_usernames.py

@@ -140,7 +140,16 @@ def handle(self, *args, **options):
         self.twitter_data = TwitterAPIData()
         self.twitter_data.update_from_api()
         # Now go through every person in the database and check their
-        # Twitter details:
-        for person in Person.objects.select_related('extra').order_by('name'):
+        # Twitter details. This can take a long time, so use one
+        # transaction per person.
+        for person_id in Person.objects.order_by('name').values_list('pk', flat=True):
             with transaction.atomic():
+                # n.b. even though it's inefficient query-wise, we get
+                # each person from the database based on their ID
+                # within the transaction because the loop we're in
+                # takes a long time, other otherwise we might end up
+                # with out of date information (e.g. this has happened
+                # with the person.extra.versions field, with confusing
+                # results...)
+                person = Person.objects.select_related('extra').get(pk=person_id)
                 self.handle_person(person)

candidates/views/candidacies.py

@@ -39,12 +39,12 @@ class CandidacyView(ElectionMixin, LoginRequiredMixin, FormView):
 
     def form_valid(self, form):
         post_id = form.cleaned_data['post_id']
-        post = get_object_or_404(Post, extra__slug=post_id)
-        raise_if_locked(self.request, post, self.election_data)
-        change_metadata = get_change_metadata(
-            self.request, form.cleaned_data['source']
-        )
         with transaction.atomic():
+            post = get_object_or_404(Post, extra__slug=post_id)
+            raise_if_locked(self.request, post, self.election_data)
+            change_metadata = get_change_metadata(
+                self.request, form.cleaned_data['source']
+            )
             person = get_object_or_404(Person, id=form.cleaned_data['person_id'])
             LoggedAction.objects.create(
                 user=self.request.user,

candidates/views/constituencies.py

@@ -511,10 +511,10 @@ class ConstituencyRetractWinnerView(ElectionMixin, GroupRequiredMixin, View):
 
     def post(self, request, *args, **kwargs):
         post_id = self.kwargs['post_id']
-        post = get_object_or_404(Post, extra__slug=post_id)
-        constituency_name = post.extra.short_label
-
         with transaction.atomic():
+            post = get_object_or_404(Post, extra__slug=post_id)
+            constituency_name = post.extra.short_label
+
             all_candidacies = post.memberships.filter(
                 role=self.election_data.candidate_membership_role,
                 extra__election=self.election_data,

candidates/views/people.py

@@ -183,25 +183,25 @@ def post(self, request, *args, **kwargs):
         person_id = self.kwargs['person_id']
         source = self.request.POST['source']
 
-        person_extra = get_object_or_404(
-            PersonExtra.objects.select_related('base'),
-            base__id=person_id
-        )
-        person = person_extra.base
+        with transaction.atomic():
 
-        versions = json.loads(person_extra.versions)
+            person_extra = get_object_or_404(
+                PersonExtra.objects.select_related('base'),
+                base__id=person_id
+            )
+            person = person_extra.base
 
-        data_to_revert_to = None
-        for version in versions:
-            if version['version_id'] == version_id:
-                data_to_revert_to = version['data']
-                break
+            versions = json.loads(person_extra.versions)
 
-        if not data_to_revert_to:
-            message = _("Couldn't find the version {0} of person {1}")
-            raise Exception(message.format(version_id, person_id))
+            data_to_revert_to = None
+            for version in versions:
+                if version['version_id'] == version_id:
+                    data_to_revert_to = version['data']
+                    break
 
-        with transaction.atomic():
+            if not data_to_revert_to:
+                message = _("Couldn't find the version {0} of person {1}")
+                raise Exception(message.format(version_id, person_id))
 
             change_metadata = get_change_metadata(self.request, source)
 

elections/uk/management/commands/uk_create_joint_pseudo_parties.py

@@ -53,6 +53,7 @@ def create_or_update_party(joint_party_name, sub_parties):
 class Command(BaseCommand):
     help = "Create joint pseudo-parties based on parties with joint descriptions"
 
+    @transaction.atomic
     def handle(self, **options):
         joint_party_to_sub_parties = defaultdict(list)
         for party_extra in OrganizationExtra.objects.filter(
@@ -72,20 +73,18 @@ def handle(self, **options):
                     continue
                 joint_name = fix_joint_party_name(m.group('joint_name'))
                 joint_party_to_sub_parties[joint_name].append(party)
-        with transaction.atomic():
-            for joint_name, sub_parties in joint_party_to_sub_parties.items():
-                if len(sub_parties) < 2:
-                    message = 'Ignoring "{joint_name}"' \
-                        ' (only made up of one party: "{sub_party}")'
-                    print(message.format(
-                        joint_name=joint_name,
-                        sub_party=sub_parties[0].name
-                    ))
-                    continue
-                message = 'Creating "{joint_name}", made up of: {sub_parties}'
+        for joint_name, sub_parties in joint_party_to_sub_parties.items():
+            if len(sub_parties) < 2:
+                message = 'Ignoring "{joint_name}"' \
+                    ' (only made up of one party: "{sub_party}")'
                 print(message.format(
                     joint_name=joint_name,
-                    sub_parties=sub_parties,
+                    sub_party=sub_parties[0].name
                 ))
-
-                create_or_update_party(joint_name, sub_parties)
+                continue
+            message = 'Creating "{joint_name}", made up of: {sub_parties}'
+            print(message.format(
+                joint_name=joint_name,
+                sub_parties=sub_parties,
+            ))
+            create_or_update_party(joint_name, sub_parties)

uk_results/forms.py

@@ -262,33 +262,33 @@ def mark_candidates_as_winner(self, request, instance):
 
 
     def save(self, request):
-        instance = super(ResultSetForm, self).save(commit=False)
-        instance.review_status = CONFIRMED_STATUS
-        instance.post_election_result = self.post_election_result
-        instance.user = request.user if \
-            request.user.is_authenticated() else None
-        instance.ip_address = get_client_ip(request)
-        instance.save(request)
-
-        post_election = self.post_election_result.post_election
-        winner_count = post_election.winner_count
-
-        winners = dict(sorted(
-            [("{}-{}".format(self[y].value(), x.person.id), x)
-                for x, y in self.memberships],
-            reverse=True,
-            key=lambda votes: int(votes[0].split('-')[0])
-        )[:winner_count])
-
-        for membership, field_name in self.memberships:
-            instance.candidate_results.create(
-                membership=membership,
-                is_winner=bool(membership in winners.values()),
-                num_ballots_reported=self[field_name].value(),
-            )
+        with transaction.atomic():
 
+            instance = super(ResultSetForm, self).save(commit=False)
+            instance.review_status = CONFIRMED_STATUS
+            instance.post_election_result = self.post_election_result
+            instance.user = request.user if \
+                request.user.is_authenticated() else None
+            instance.ip_address = get_client_ip(request)
+            instance.save(request)
+
+            post_election = self.post_election_result.post_election
+            winner_count = post_election.winner_count
+
+            winners = dict(sorted(
+                [("{}-{}".format(self[y].value(), x.person.id), x)
+                    for x, y in self.memberships],
+                reverse=True,
+                key=lambda votes: int(votes[0].split('-')[0])
+            )[:winner_count])
+
+            for membership, field_name in self.memberships:
+                instance.candidate_results.create(
+                    membership=membership,
+                    is_winner=bool(membership in winners.values()),
+                    num_ballots_reported=self[field_name].value(),
+                )
 
-        with transaction.atomic():
             self.mark_candidates_as_winner(request, instance)
 
         return instance