Added additional tests for concordia.models

concordia/models.py

@@ -9,7 +9,7 @@
 from django.contrib.auth.models import User
 from django.core import signing
 from django.core.cache import cache
-from django.core.exceptions import ValidationError
+from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.core.validators import RegexValidator
 from django.db import models
 from django.db.models import Count, ExpressionWrapper, F, JSONField, Q
@@ -614,11 +614,13 @@ def __str__(self):
         return self.title
 
     def save(self, *args, **kwargs):
+        try:
+            self.campaign  # noqa: B018
+        except ObjectDoesNotExist:
+            self.campaign = self.item.project.campaign
         # This ensures all 'required' fields really are required
         # even when creating objects programmatically. Particularly,
         # we want to make sure we don't end up with an empty storage_image
-        if not self.campaign:
-            self.campaign = self.item.project.campaign
         self.full_clean()
         super().save(*args, **kwargs)
 
@@ -664,6 +666,10 @@ def turn_off_ocr(self):
         return self.disable_ocr or self.item.turn_off_ocr()
 
     def can_rollback(self):
+        # original_latest_transcription holds the actual latest transcription
+        # latest_transcription starts by holding the actual latest transcription,
+        # but if it's a rolled forward or backward transcription, we use it to
+        # find the most recent non-rolled transcription and store it instead
         original_latest_transcription = latest_transcription = (
             self.latest_transcription()
         )
@@ -675,43 +681,32 @@ def can_rollback(self):
                     "with no transcriptions"
                 ),
             )
+        # If the latest transcription has a source (i.e., is a rollback
+        # or rollforward transcription), we want the original transcription
+        # that it's based on, back to the original source
         while latest_transcription.source:
             latest_transcription = latest_transcription.source
 
-        transcriptions = (
+        # We look back from the latest non-rolled transcription,
+        # ignoring any rolled forward or sources of rolled forward
+        # transcriptions
+        transcription_to_rollback_to = (
             self.transcription_set.exclude(rolled_forward=True)
             .exclude(source_of__rolled_forward=True)
             .exclude(pk__gte=latest_transcription.pk)
             .order_by("-pk")
+            .first()
         )
-        if not transcriptions:
+        if transcription_to_rollback_to is None:
+            # We didn't find one, which means there's no eligible
+            # transcription to rollback to, because everything before
+            # is either a rollforward or the source of a rollforward
+            # (or there just isn't an earlier transcription at all)
             return (
                 False,
                 (
                     "Can not rollback transcription on an asset "
-                    "with no non-rollback transcriptions"
-                ),
-            )
-
-        transcription_to_rollback_to = None
-        for transcription in transcriptions:
-            if transcription.source:
-                transcription_to_check = transcription.source
-            else:
-                transcription_to_check = transcription
-            if (
-                latest_transcription.rolled_back is False
-                or latest_transcription.supersedes != transcription_to_check
-            ):
-                transcription_to_rollback_to = transcription
-                break
-
-        if not transcription_to_rollback_to:
-            return (
-                False,
-                (
-                    "Can not rollback transcription on an asset with "
-                    "no non-rollback transcriptions"
+                    "with no non-rollforward older transcriptions"
                 ),
             )
 
@@ -739,9 +734,9 @@ def rollback_transcription(self, user):
 
     def can_rollforward(self):
         # original_latest_transcription holds the actual latest transcription
-        # latest_transcription holds the most recent transcription. If it's a rolled
-        # forward transcription, we use it to find the most recent non-rolled-forward
-        # transcription
+        # latest_transcription starts by holding the actual latest transcription,
+        # but if it's a rolled forward transcription, we use it to find the most
+        # recent non-rolled-forward transcription and store that in latest_transcription
 
         original_latest_transcription = latest_transcription = (
             self.latest_transcription()
@@ -757,7 +752,6 @@ def can_rollforward(self):
             )
 
         if latest_transcription.rolled_forward:
-
             # We need to find the latest transcription that wasn't rolled forward
             rolled_forward_count = 0
             try:
@@ -779,8 +773,18 @@ def can_rollforward(self):
             try:
                 while rolled_forward_count >= 1:
                     latest_transcription = latest_transcription.supersedes
+                    if not latest_transcription:
+                        # We do this here to handle the error rather than letting
+                        # it be raised below when we try to process this
+                        # non-existent transcription
+                        raise AttributeError
                     rolled_forward_count -= 1
             except AttributeError:
+                # This error is raised manually if latest_transcription ends up
+                # being None at the end of the loop or automatically if it is None
+                # when the loop continues
+                # In either case, his should only happen if the transcription
+                # history was manually edited.
                 return (
                     False,
                     (
@@ -790,6 +794,9 @@ def can_rollforward(self):
                     ),
                 )
 
+        # If the latest_transcription we end up with is a rollback transcription,
+        # we want to rollforward to the transcription it replaced. If not,
+        # nothing can be rolled forward
         if latest_transcription.rolled_back:
             transcription_to_rollforward = latest_transcription.supersedes
         else:
@@ -801,12 +808,15 @@ def can_rollforward(self):
                 ),
             )
 
+        # If that replaced transcription doesn't exist, we can't do anything
+        # This shouldn't be possible normally, but if a transcription history
+        # is manually edited, you could end up in this state.
         if not transcription_to_rollforward:
             return (
                 False,
                 (
                     "Can not rollforward transcription on an asset if the latest "
-                    "non-transcription did not supersede a previous transcription"
+                    "rollback transcription did not supersede a previous transcription"
                 ),
             )
 

concordia/tests/test_models.py

@@ -2,22 +2,26 @@
 from secrets import token_hex
 from unittest import mock
 
-from django.core.exceptions import ValidationError
-from django.db.models.signals import post_save
+from django.conf import settings
+from django.core.exceptions import ObjectDoesNotExist, ValidationError
+from django.db.models import signals
 from django.test import TestCase
 from django.utils import timezone
 
 from concordia.models import (
+    Asset,
     AssetTranscriptionReservation,
     Campaign,
     CardFamily,
+    MediaType,
     Resource,
     Transcription,
     TranscriptionStatus,
     UserProfileActivity,
     resource_file_upload_path,
     validated_get_or_create,
 )
+from concordia.signals.handlers import create_user_profile
 from concordia.utils import get_anonymous_user
 
 from .utils import (
@@ -43,12 +47,12 @@
 class AssetTestCase(CreateTestUsers, TestCase):
     def setUp(self):
         self.asset = create_asset()
-        anon = get_anonymous_user()
-        create_transcription(asset=self.asset, user=anon)
+        self.anon = get_anonymous_user()
+        create_transcription(asset=self.asset, user=self.anon)
         create_transcription(
             asset=self.asset,
             user=self.create_test_user(username="tester"),
-            reviewed_by=anon,
+            reviewed_by=self.anon,
         )
 
     def test_get_ocr_transcript(self):
@@ -86,6 +90,62 @@ def test_get_storage_path(self):
             "test-campaign/test-project/testitem.0123456789/1.jpg",
         )
 
+    def test_saving_without_campaign(self):
+        try:
+            Asset.objects.create(
+                item=self.asset.item,
+                title="No campaign",
+                slug="no-campaign",
+                media_type=MediaType.IMAGE,
+                media_url="1.jpg",
+                storage_image="unittest1.jpg",
+            )
+        except (ValidationError, ObjectDoesNotExist):
+            self.fail("Creating an Asset without a campaign failed validation.")
+
+    def test_rollforward_with_only_rollforward_transcriptions(self):
+        asset = create_asset(slug="rollforward-test", item=self.asset.item)
+        create_transcription(asset=asset, user=self.anon, rolled_forward=True)
+        with self.assertRaisesMessage(
+            ValueError,
+            "Can not rollforward transcription on an asset with "
+            "no non-rollforward transcriptions",
+        ):
+            asset.rollforward_transcription(self.anon)
+
+    def test_rollforward_with_too_many_rollforward_transcriptions(self):
+        asset = create_asset(slug="rollforward-test", item=self.asset.item)
+        transcription1 = create_transcription(asset=asset, user=self.anon)
+        create_transcription(
+            asset=asset, user=self.anon, supersedes=transcription1, rolled_forward=True
+        )
+        create_transcription(
+            asset=asset, user=self.anon, supersedes=transcription1, rolled_forward=True
+        )
+        with self.assertRaisesMessage(
+            ValueError,
+            "More rollforward transcription exist than non-roll-forward "
+            "transcriptions, which shouldn't be possible. Possibly "
+            "incorrectly modified transcriptions for this asset.",
+        ):
+            asset.rollforward_transcription(self.anon)
+
+    def test_rollforward_with_no_superseded_transcription(self):
+        # This isn't a state that would happen normally, but could be created
+        # accidentally when manually editing transcription history
+        asset = create_asset(slug="rollforward-test", item=self.asset.item)
+        transcription1 = create_transcription(asset=asset, user=self.anon)
+        create_transcription(asset=asset, user=self.anon, supersedes=transcription1)
+        create_transcription(
+            asset=asset, user=self.anon, rolled_back=True, source=transcription1
+        )
+        with self.assertRaisesMessage(
+            ValueError,
+            "Can not rollforward transcription on an asset if the latest "
+            "rollback transcription did not supersede a previous transcription",
+        ):
+            asset.rollforward_transcription(self.anon)
+
 
 class TranscriptionManagerTestCase(CreateTestUsers, TestCase):
     def setUp(self):
@@ -328,6 +388,19 @@ def test_str(self):
         self.assertEqual(f"{activity.user} - {activity.campaign}", str(activity))
 
 
+class UserProfileTestCase(CreateTestUsers, TestCase):
+    def test_transcription_save_creating_userprofile(self):
+        signals.post_save.disconnect(
+            create_user_profile, sender=settings.AUTH_USER_MODEL
+        )
+        user = self.create_test_user()
+        self.assertFalse(hasattr(user, "profile"))
+        create_transcription(user=user)
+        self.assertTrue(hasattr(user, "profile"))
+        self.assertEqual(user.profile.transcribe_count, 1)
+        signals.post_save.connect(create_user_profile, sender=settings.AUTH_USER_MODEL)
+
+
 class CampaignTestCase(TestCase):
     def test_queryset(self):
         campaign = create_campaign(unlisted=True)
@@ -357,7 +430,7 @@ def test_str(self):
 
     def test_on_cardfamily_save(self):
         with mock.patch("concordia.models.on_cardfamily_save") as mocked_handler:
-            post_save.connect(mocked_handler, sender=CardFamily)
+            signals.post_save.connect(mocked_handler, sender=CardFamily)
             self.family1.save()
             self.assertTrue(mocked_handler.called)
             self.assertEqual(mocked_handler.call_count, 1)