Add DB constraint to enforce hold/position mutual exclusion

Also make hold/position mutually exclusive in the API via a union

api/src/core/migrations/0017_betamove_hold_position_mutually_exclusive.py

@@ -0,0 +1,23 @@
+# Generated by Django 4.2.3 on 2023-07-29 02:14
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("core", "0016_remove_hold_boulder_remove_problem_holds_and_more"),
+    ]
+
+    operations = [
+        migrations.AddConstraint(
+            model_name="betamove",
+            constraint=models.CheckConstraint(
+                check=models.Q(
+                    ("hold_id__isnull", True),
+                    ("position__isnull", True),
+                    _connector="XOR",
+                ),
+                name="hold_position_mutually_exclusive",
+            ),
+        ),
+    ]

api/src/core/models.py

@@ -182,6 +182,10 @@ class Meta:
                 name="beta_order_unique",
                 fields=("beta", "order"),
                 deferrable=models.Deferrable.DEFERRED,
+            ),
+            models.CheckConstraint(
+                name="hold_position_mutually_exclusive",
+                check=Q(hold_id__isnull=True) ^ Q(position__isnull=True),
             )
             # TODO enforce that orders are always consecutive
         ]

api/src/core/schema/query.py

@@ -1,4 +1,4 @@
-from typing import Annotated, Iterable, Optional, Union
+from typing import Annotated, Iterable, Optional
 
 import strawberry
 from django.contrib.auth.models import User
@@ -265,26 +265,23 @@ class BetaMoveNode(relay.Node):
     annotation: strawberry.auto = strawberry.field(
         description="Informative text related to the move, created by the user"
     )
-    # TODO is there a better way to represent mutually exclusive fields?
-    hold: Optional[HoldNode] = strawberry.field(
-        description="The optional hold that this move is attached to. If null,"
-        " the move is 'free', e.g. a flag or smear."
-    )
 
     @strawberry.django.field(
-        description="Position of a free move. Null for attached moves.",
+        description="Where the move is going; either a hold or a free position",
         select_related=["beta__problem__boulder"],
         only=["beta__problem__boulder__image"],
     )
-    def position(self) -> Optional[SVGPosition]:
+    def target(self: BetaMove) -> HoldNode | SVGPosition:  # type: ignore[misc]
         # Note: You may be tempted to have this return the hold position when
         # available so the frontend doesn't have to handle that logic. But wait!
         # That doesn't work because then if the hold position is modified, Relay
         # doesn't know to update the associated move position(s) in local state,
         # so the data gets out of sync.
-        return self.position and SVGPosition.from_boulder_position(
-            self.position, self.beta.problem.boulder.image
-        )
+        if self.position:
+            return SVGPosition.from_boulder_position(
+                self.position, self.beta.problem.boulder.image
+            )
+        return self.hold
 
 
 @strawberry.type
@@ -339,7 +336,7 @@ def problems(
     # TODO rename return type to CurrentUser after
     # https://github.com/strawberry-graphql/strawberry/issues/2302
     @strawberry.field()
-    def current_user(self, info: Info) -> Union[UserNode, NoUser]:
+    def current_user(self, info: Info) -> UserNode | NoUser:
         """
         Get data on the requesting user (you). Null for unauthenticated users.
         Unauthenticated users who have performed a mutation will be logged in

api/src/core/tests/factories.py

@@ -1,11 +1,28 @@
 import factory
+import factory.random
 from django.contrib.auth.models import User
+from factory import Factory
 from factory.django import DjangoModelFactory
 from factory.faker import Faker
 from pytest_factoryboy import register
 
 from bs_auth.models import UserProfile
-from core.models import Boulder, Problem, Visibility
+from core.fields import BoulderPosition
+from core.models import (
+    Beta,
+    BetaMove,
+    Boulder,
+    Hold,
+    HoldAnnotationSource,
+    Problem,
+    Visibility,
+)
+
+
+def position_factory() -> str:
+    x = factory.random.random.random()
+    y = factory.random.random.random()
+    return f"{x},{y}"
 
 
 class UserFactory(DjangoModelFactory):
@@ -45,8 +62,58 @@ class Meta:
     visibility = Visibility.PUBLIC
 
 
+class BoulderPositionFactory(Factory):
+    class Meta:
+        model = BoulderPosition
+
+    x = factory.LazyFunction(factory.random.random.random)
+    y = factory.LazyFunction(factory.random.random.random)
+
+
+class HoldFactory(DjangoModelFactory):
+    class Meta:
+        model = Hold
+
+    problem = factory.SubFactory(ProblemFactory)
+    position = factory.SubFactory(BoulderPositionFactory)
+    source = HoldAnnotationSource.USER
+
+
+class BetaFactory(DjangoModelFactory):
+    class Meta:
+        model = Beta
+
+    problem = factory.SubFactory(ProblemFactory)
+    owner = factory.SubFactory(UserFactory)
+    name = Faker("name")
+
+
+class BetaMoveFactory(DjangoModelFactory):
+    class Meta:
+        model = BetaMove
+
+    class Params:
+        is_free = Faker("pybool")
+
+    beta = factory.SubFactory(BetaFactory)
+    hold = factory.Maybe(
+        "is_free",
+        yes_declaration=None,
+        no_declaration=factory.SubFactory(HoldFactory),
+    )
+    position = factory.Maybe(
+        "is_free",
+        yes_declaration=factory.SubFactory(BoulderPositionFactory),
+        no_declaration=None,
+    )
+
+
 # We have to register at the bottom, to avoid circular import issues
 register(UserFactory)
 register(UserProfileFactory)
 register(BoulderFactory)
 register(ProblemFactory)
+register(BoulderPositionFactory)
+register(HoldFactory)
+register(BetaFactory)
+register(BetaMoveFactory)

api/src/core/tests/models/test_beta_move.py

@@ -0,0 +1,41 @@
+from contextlib import AbstractContextManager, nullcontext
+
+import pytest
+from django.db import IntegrityError
+
+from core.fields import BoulderPosition
+from core.models import Beta, Hold
+from core.tests.factories import BetaMoveFactory
+
+pytestmark = pytest.mark.django_db
+
+
+@pytest.mark.parametrize(
+    "has_hold,has_position,expectation",
+    [
+        (False, False, pytest.raises(IntegrityError)),
+        (True, False, nullcontext()),
+        (True, False, nullcontext()),
+        (True, True, pytest.raises(IntegrityError)),
+    ],
+)
+def test_beta_move_hold_or_position(
+    beta: Beta,
+    hold: Hold,
+    boulder_position: BoulderPosition,
+    has_hold: bool,
+    has_position: bool,
+    expectation: AbstractContextManager,
+) -> None:
+    """
+    Test both failure and success when creating a beta move with hold/position.
+    Exactly one of these fields should be defined.
+    """
+    with expectation:
+        beta_move = BetaMoveFactory(
+            beta=beta,
+            hold=hold if has_hold else None,
+            position=boulder_position if has_position else None,
+        )
+        assert beta_move.hold == hold
+        assert beta_move.position is None

api/src/schema.graphql

@@ -31,13 +31,8 @@ type BetaMoveNode implements Node {
   """Informative text related to the move, created by the user"""
   annotation: String!
 
-  """
-  The optional hold that this move is attached to. If null, the move is 'free', e.g. a flag or smear.
-  """
-  hold: HoldNode
-
-  """Position of a free move. Null for attached moves."""
-  position: SVGPosition
+  """Where the move is going; either a hold or a free position"""
+  target: HoldNodeSVGPosition!
 }
 
 """A connection to a list of items."""
@@ -194,6 +189,8 @@ type HoldNodeEdge {
   node: HoldNode!
 }
 
+union HoldNodeSVGPosition = HoldNode | SVGPosition
+
 type Image {
   """Image access URL"""
   url: String!