diff --git a/README.md b/README.md index 0099dcb..d3e61b6 100644 --- a/README.md +++ b/README.md @@ -36,23 +36,25 @@ The matrix below shows the permissions for each role. Not all permissions are cu (Last updated: November 14th, 2025) -| | Visitor | Annotator | Master Annotator | -| -------------------------- | ------- | --------- | ---------------- | -| Browse gold problems | Yes | Yes | Yes | -| Browse silver problems | No | Yes | Yes | -| Edit KB items | No | Yes | Yes | -| Add labels | No | Yes | Yes | -| Remove own labels | No | Yes | Yes | -| Remove other users' labels | No | No | Yes | -| Add problems | No | No | Yes | -| Copy problems | No | No | Yes | -| Update user problems | No | No | Yes | -| Delete problems | No | No | Yes | -| Edit existing problems | No | No | Yes | -| See hidden problems | No | No | Yes | -| Silver/gold problems | No | No | Yes | -| Hide/unhide problems | No | No | Yes | -| Manage users | No | No | Yes | +| | Unregistered users | Registered Users | Annotators | Master Annotators | +| ------------------------ | ------------------ | ---------------- | ---------- | ----------------- | +| Browse gold problems | Yes | Yes | Yes | Yes | +| Browse silver problems | No | Yes | Yes | Yes | +| Browse bronze problems | No | Yes | Yes | Yes | +| Add/edit/remove KB items | No | No | Yes | Yes | +| Add labels | No | No | Yes | Yes | +| Remove own labels | No | No | Yes | Yes | +| Remove others' labels | No | No | No | Yes | +| Copy problems | No | No | No | Yes | +| See hidden problems | No | No | No | Yes | +| Gold/ungold problems | No | No | No | Yes | +| Hide/unhide problems | No | No | No | Yes | +| Manage users | No | No | No | Yes | + +NB: +- 'Bronze' problems are problems that are 'untouched' (i.e., non-annotated) problems. These do not have knowledge base items or labels, and have no edits made to their syntactic parses and tableaus. +- 'Silver' problems are problems that have been annotated with labels and/or knowledge base items, but have not been marked as 'gold'. +- 'Gold' problems are problems that have been marked as 'gold' by a Master Annotator. ## Licence diff --git a/backend/annotation/serializers.py b/backend/annotation/serializers.py index fde805b..fcf67da 100644 --- a/backend/annotation/serializers.py +++ b/backend/annotation/serializers.py @@ -3,6 +3,7 @@ from django.contrib.auth.models import AnonymousUser from annotation.models import ( + BaseAnnotation, KnowledgeBaseAnnotation, Label, LabelAnnotation, @@ -17,7 +18,7 @@ class AnnotationBaseSerializer(serializers.ModelSerializer): """ createdAt = serializers.DateTimeField(source="created_at", read_only=True) - createdBy = serializers.PrimaryKeyRelatedField(source="created_by", read_only=True) + createdBy = serializers.SerializerMethodField(method_name="get_createdBy", read_only=True) removedAt = serializers.DateTimeField( source="removed_at", allow_null=True, read_only=True ) @@ -45,6 +46,11 @@ def get_removable(self, annotation) -> bool: """This should be overridden in subclasses.""" raise NotImplementedError("Subclasses must implement get_removable method.") + def get_createdBy(self, annotation) -> str | None: + """Returns the full name of the user who created the annotation.""" + return annotation.created_by.get_full_name() + + class KnowledgeBaseAnnotationSerializer(AnnotationBaseSerializer): """ diff --git a/backend/annotation/views.py b/backend/annotation/views.py index 013608a..2ae62bc 100644 --- a/backend/annotation/views.py +++ b/backend/annotation/views.py @@ -37,9 +37,7 @@ def has_permission(self, request, view): if user.is_superuser: return True - return user.has_perm("annotation.add_labelannotation") or user.has_perm( - "annotation.change_labelannotation" - ) + return user.has_perm("annotation.add_labelannotation") class LabelAnnotationView(ModelViewSet): @@ -126,7 +124,7 @@ def _update_label_annotations( def _mark_as_removed(self, label_annotation: LabelAnnotation, user: User) -> None: """Mark a label annotation as removed.""" - if not user.can_remove_label(label_annotation): + if not user.can_remove_label_annotation(label_annotation): logger.warning( f"User {user.username} attempted to remove label {label_annotation.label.pk} " f"attached by {label_annotation.created_by.username}" diff --git a/backend/problem/serializers.py b/backend/problem/serializers.py index 5fec960..76a05ca 100644 --- a/backend/problem/serializers.py +++ b/backend/problem/serializers.py @@ -8,6 +8,7 @@ ) from problem.services import FracasData, SNLIData, SickData from problem.models import Problem, Sentence +from user.models import User class ProblemSerializer(serializers.ModelSerializer): @@ -98,32 +99,10 @@ def validate_base(self, value): def create(self, validated_data: dict) -> Problem: """ Create a new Problem instance from validated input data. - Handles creation of related Sentence and KnowledgeBase objects. """ - premise_sentences = [ - Sentence.objects.get_or_create(text=premise)[0] - for premise in validated_data["premises"] - ] - - hypothesis_sentence = Sentence.objects.get_or_create( - text=validated_data["hypothesis"] - )[0] - - problem = Problem.objects.create( - base_id=validated_data.get("base", None), - hypothesis=hypothesis_sentence, - dataset=Problem.Dataset.USER, - # TODO: Determine entailment label based on LangPro parser output. - entailment_label=Problem.EntailmentLabel.UNKNOWN, - extra_data={}, - ) - - problem.premises.set(premise_sentences) + new_user_problem = Problem(dataset=Problem.Dataset.USER, extra_data={}) - kb_items = validated_data.get("kbItems", []) - self._handle_kb_annotations(problem, kb_items) - - return problem + return self._update_core_problem_fields(new_user_problem, validated_data) def _create_update_kb_annotation( self, kb_item: dict, problem: Problem, session: AnnotationSession @@ -164,11 +143,12 @@ def _mark_kb_not_in_input_as_removed( Marks KnowledgeBase annotations for a problem that are not included in the provided list of kb_items as removed. """ - kb_item_ids = {kb_item.get("id") for kb_item in kb_items if kb_item.get("id") is not None} + kb_item_ids = { + kb_item.get("id") for kb_item in kb_items if kb_item.get("id") is not None + } annotations_to_delete = KnowledgeBaseAnnotation.objects.filter( - problem=problem, - removed_at__isnull=True + problem=problem, removed_at__isnull=True ).exclude(id__in=kb_item_ids) current_time = timezone.now() @@ -178,18 +158,14 @@ def _mark_kb_not_in_input_as_removed( annotation.removed_by = session.user annotation.save() - def _handle_kb_annotations( - self, problem: Problem, kb_items: list[dict] + def handle_kb_annotations( + self, problem: Problem, kb_items: list[dict], user: User ) -> None: """ Creates, updates and deletes KnowledgeBase annotations for a problem. Creates an annotation session if it does not exist. """ - request = self.context.get("request", None) - if not request or not request.user.is_authenticated or not request.user.can_edit_kb: - return - - session = AnnotationSession.objects.create(user=request.user) + session = AnnotationSession.objects.create(user=user) self._mark_kb_not_in_input_as_removed(problem, kb_items, session) @@ -198,18 +174,19 @@ def _handle_kb_annotations( def update(self, instance: Problem, validated_data: dict) -> Problem: """ - Update an existing Problem instance from validated input data. - Handles updating of related Sentence and KnowledgeBase objects. + Updates Problem core fields from validated input data. """ - - # KB annotations can be made for all problems. - kb_items = validated_data.get("kbItems", []) - self._handle_kb_annotations(instance, kb_items) - - # Other fields can only be updated for user-created problems. + # Only USER-problems can be updated. if instance.dataset != Problem.Dataset.USER: return instance + return self._update_core_problem_fields(instance, validated_data) + + def _update_core_problem_fields(self, instance: Problem, validated_data: dict) -> Problem: + """ + Updates core Problem fields (premises, hypothesis, base) from validated + input data. + """ instance.hypothesis = Sentence.objects.get_or_create( text=validated_data["hypothesis"], )[0] diff --git a/backend/problem/serializers_test.py b/backend/problem/serializers_test.py index aec3156..d891c97 100644 --- a/backend/problem/serializers_test.py +++ b/backend/problem/serializers_test.py @@ -2,7 +2,6 @@ from rest_framework.exceptions import ValidationError from annotation.models import KnowledgeBaseAnnotation -from problem.serializers_test_utils import input_serializer_with_user from .serializers import ProblemInputSerializer from .models import Problem, Sentence @@ -117,76 +116,6 @@ def test_blank_hypothesis_invalid(): assert "hypothesis" in serializer.errors -@pytest.mark.django_db -def test_kb_create_no_permission(user_problem, visitor): - """Test that KB annotations cannot be created without permission.""" - kb_input = [ - { - "entity1": "cat", - "entity2": "feline", - "relationship": "equal", - "notes": "Test note", - } - ] - - serializer = input_serializer_with_user(visitor) - serializer._handle_kb_annotations(user_problem, kb_input) # type: ignore - - assert ( - KnowledgeBaseAnnotation.objects.filter(problem=user_problem).count() == 0 - ), "No KB annotations should be created without permission." - - -@pytest.mark.django_db -def test_kb_update_no_permission(user_problem, kb_annotation, visitor): - """Test that KB annotations cannot be updated without permission.""" - kb_annotation.problem = user_problem - kb_annotation.save() - original_entity1 = kb_annotation.entity1 - - updated_entity_1 = "updated_entity" - - kb_input = [ - { - "id": kb_annotation.pk, - "entity1": updated_entity_1, - "entity2": kb_annotation.entity2, - "relationship": kb_annotation.relationship, - } - ] - - # Preconditions - assert original_entity1 != updated_entity_1 - - serializer = input_serializer_with_user(visitor) - serializer._handle_kb_annotations(user_problem, kb_input) # type: ignore - - # Verify KB annotation was not updated - kb_annotation.refresh_from_db() - assert ( - kb_annotation.entity1 == original_entity1 - ), "KB annotation should not have been modified without permission." - assert updated_entity_1 != original_entity1 - - -@pytest.mark.django_db -def test_kb_mark_removed_no_permission(user_problem, kb_annotation, visitor): - """Test that KB annotations cannot be marked as removed without permission.""" - kb_annotation.problem = user_problem - kb_annotation.save() - - kb_input = [] # Empty list should mark any existing KB items as removed. - - serializer = input_serializer_with_user(visitor) - serializer._handle_kb_annotations(user_problem, kb_input) # type: ignore - - # Verify KB annotation was not removed - kb_annotation.refresh_from_db() - assert ( - kb_annotation.removed_at is None - ), "KB annotation should not have been marked as removed without permission." - - @pytest.mark.django_db def test_create_single_kb_annotation(user_problem, annotator): """Test creating a single KB annotation for a problem.""" @@ -199,8 +128,8 @@ def test_create_single_kb_annotation(user_problem, annotator): } ] - serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=user_problem) assert kb_annotations.count() == 1, "One KB annotation should have been created." @@ -230,8 +159,8 @@ def test_update_single_kb_annotation(user_problem, kb_annotation, annotator): } ] - serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore # Verify KB annotation was updated kb_annotation.refresh_from_db() @@ -250,8 +179,8 @@ def test_mark_kb_annotation_as_removed(user_problem, kb_annotation, annotator): kb_input = [] # Empty list should mark existing as removed - serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore # Verify KB annotation was marked as removed kb_annotation.refresh_from_db() @@ -286,8 +215,8 @@ def test_create_and_update_multiple_kb_annotations( }, ] - serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore kb_annotations = KnowledgeBaseAnnotation.objects.filter( problem=user_problem, removed_at__isnull=True @@ -337,7 +266,6 @@ def test_create_update_and_remove_kb_annotations( created_by=annotator, ) - # request = Mock(user=annotator) kb_input = [ { "id": kb1.pk, @@ -352,8 +280,8 @@ def test_create_update_and_remove_kb_annotations( }, ] - serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore # Verify kb1 was updated. kb1.refresh_from_db() @@ -378,144 +306,3 @@ def test_create_update_and_remove_kb_annotations( assert new_annotation.entity1 == "new_e1" assert new_annotation.entity2 == "new_e2" assert new_annotation.relationship == "superset" - - -@pytest.mark.django_db -def test_create_problem_with_kb_annotations(annotator): - """Test creating a problem with KB annotations through create().""" - data = { - "premises": ["A cat is running."], - "hypothesis": "A cat is moving.", - "kbItems": [ - { - "entity1": "cat", - "entity2": "feline", - "relationship": "equal", - "notes": "Test note", - }, - { - "entity1": "running", - "entity2": "moving", - "relationship": "subset", - }, - ], - } - - serializer = input_serializer_with_user(annotator, data=data) - assert serializer.is_valid(raise_exception=True) - problem = serializer.save() - - # Verify problem was created - assert problem.pk is not None - assert problem.dataset == Problem.Dataset.USER - assert problem.hypothesis.text == "A cat is moving." - assert problem.premises.count() == 1 - assert problem.premises.first().text == "A cat is running." - - # Verify KB annotations were created - kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=problem) - assert kb_annotations.count() == 2 - - kb1 = kb_annotations.get(entity1="cat") - assert kb1.entity2 == "feline" - assert kb1.relationship == "equal" - assert kb1.notes == "Test note" - assert kb1.created_by == annotator - - kb2 = kb_annotations.get(entity1="running") - assert kb2.entity2 == "moving" - assert kb2.relationship == "subset" - assert kb2.created_by == annotator - - -@pytest.mark.django_db -def test_create_problem_with_multiple_premises_and_kb(annotator): - """Test creating a problem with multiple premises.""" - data = { - "premises": ["Birds can fly.", "Penguins are birds."], - "hypothesis": "Penguins can fly.", - } - - serializer = input_serializer_with_user(annotator, data=data) - assert serializer.is_valid(raise_exception=True) - problem = serializer.save() - - assert problem.premises.count() == 2 - premise_texts = [p.text for p in problem.premises.all()] - assert "Birds can fly." in premise_texts - assert "Penguins are birds." in premise_texts - assert problem.hypothesis.text == "Penguins can fly." - - -@pytest.mark.django_db -def test_update_user_problem_with_new_kb_annotations(user_problem, annotator): - """Test updating a user problem adds new KB annotations through update().""" - data = { - "id": user_problem.pk, - "premises": ["Updated premise."], - "hypothesis": "Updated hypothesis.", - "kbItems": [ - { - "entity1": "new_entity1", - "entity2": "new_entity2", - "relationship": "subset", - } - ], - } - - assert ( - KnowledgeBaseAnnotation.objects.filter(problem=user_problem).count() == 0 - ), "Precondition: user_problem should have no KB annotations." - - serializer = input_serializer_with_user(annotator, data=data, instance=user_problem) - assert serializer.is_valid(raise_exception=True) - updated_problem = serializer.save() - - # Verify problem was updated - assert updated_problem.pk == user_problem.pk - assert updated_problem.hypothesis.text == "Updated hypothesis." - assert updated_problem.premises.first().text == "Updated premise." - - # Verify KB annotation was added - kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=updated_problem) - assert kb_annotations.count() == 1 - kb_annotation = kb_annotations.first() - assert kb_annotation is not None - assert kb_annotation.entity1 == "new_entity1" - - -@pytest.mark.django_db -def test_update_non_user_problem_adds_kb_only(non_user_problem, annotator): - """Test updating a non-user problem only adds KB annotations, not other fields.""" - original_hypothesis = non_user_problem.hypothesis.text - original_premise_count = non_user_problem.premises.count() - - data = { - "id": non_user_problem.pk, - "premises": ["This should be ignored."], - "hypothesis": "This should also be ignored.", - "kbItems": [ - { - "entity1": "entity1", - "entity2": "entity2", - "relationship": "equal", - } - ], - } - - # Preconditions - assert KnowledgeBaseAnnotation.objects.filter(problem=non_user_problem).count() == 0, "Non_user_problem should have no KB annotations to begin with." - assert original_hypothesis != data["hypothesis"] - - serializer = input_serializer_with_user(annotator, data=data, instance=non_user_problem) - assert serializer.is_valid(raise_exception=True) - updated_problem = serializer.save() - - # Verify problem fields were not updated - assert updated_problem.hypothesis.text == original_hypothesis - assert updated_problem.premises.count() == original_premise_count - - # Verify KB annotation was added - kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=updated_problem) - assert kb_annotations.count() == 1 - diff --git a/backend/problem/serializers_test_utils.py b/backend/problem/serializers_test_utils.py deleted file mode 100644 index c357c98..0000000 --- a/backend/problem/serializers_test_utils.py +++ /dev/null @@ -1,16 +0,0 @@ -from unittest.mock import Mock -from problem.serializers import ProblemInputSerializer -from user.models import User - - -def input_serializer_with_user( - user: User, data: dict | None = None, instance=None -) -> ProblemInputSerializer: - """ - Helper function to create a ProblemInputSerializer with a mock request - containing the specified user. The data argument can be used to provide - initial data for the serializer. - """ - return ProblemInputSerializer( - data=data, instance=instance, context={"request": Mock(user=user)} - ) diff --git a/backend/problem/views/problem.py b/backend/problem/views/problem.py index ea607b8..fa4f223 100644 --- a/backend/problem/views/problem.py +++ b/backend/problem/views/problem.py @@ -2,7 +2,12 @@ from rest_framework.request import Request from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet -from rest_framework.status import HTTP_201_CREATED, HTTP_200_OK +from rest_framework.status import ( + HTTP_201_CREATED, + HTTP_200_OK, + HTTP_401_UNAUTHORIZED, + HTTP_403_FORBIDDEN, +) from rest_framework.permissions import IsAuthenticated, IsAuthenticatedOrReadOnly from django.shortcuts import get_object_or_404 @@ -15,7 +20,11 @@ from problem.serializers import ProblemInputSerializer, ProblemSerializer from annotation.models import KnowledgeBaseAnnotation, LabelAnnotation -from annotation.serializers import KnowledgeBaseAnnotationSerializer, LabelAnnotationSerializer +from annotation.serializers import ( + KnowledgeBaseAnnotationSerializer, + LabelAnnotationSerializer, +) +from user.models import User class CreateProblemPermission(IsAuthenticated): @@ -25,7 +34,9 @@ def has_permission(self, request, view): class EditProblemPermission(IsAuthenticated): def has_permission(self, request, view): - return super().has_permission(request, view) and (request.user.can_edit_problem or request.user.can_edit_kb) + return super().has_permission(request, view) and ( + request.user.can_edit_problem or request.user.can_edit_kb + ) class ProblemView(ModelViewSet): @@ -142,22 +153,47 @@ def partial_update(self, request: Request, pk: int) -> Response: def _handle_update_create_problem( self, request: Request, problem_id: int | None ) -> Response: + user: User | None = request.user + + # Only authenticated users can create or update problems. + if not user or not user.is_authenticated: + return Response( + {"detail": "Authentication credentials were not provided."}, + status=HTTP_401_UNAUTHORIZED, + ) + input_data = request.data - serializer = ProblemInputSerializer( - data=input_data, context={"request": request} - ) + serializer = ProblemInputSerializer(data=input_data) serializer.is_valid(raise_exception=True) validated_input: dict = serializer.validated_data # type: ignore + status = HTTP_200_OK + if problem_id is None: + if not user.can_create_problem: + return Response( + {"detail": "User is not authorized to create problems."}, + status=HTTP_403_FORBIDDEN, + ) problem = serializer.create(validated_input) # type: ignore status = HTTP_201_CREATED + + elif not (user.can_edit_problem or user.can_edit_kb): + return Response( + { + "detail": "User is not authorized to edit problems or knowledge base annotations." + }, + status=HTTP_403_FORBIDDEN, + ) + else: - problem_instance = get_object_or_404( - Problem, id=problem_id - ) - problem: Problem = serializer.update(problem_instance, validated_input) - status = HTTP_200_OK + problem = get_object_or_404(Problem, id=problem_id) + if user.can_edit_problem: + problem: Problem = serializer.update(problem, validated_input) # type: ignore + + kb_items = validated_input.get("kbItems", []) + if user.can_edit_kb: + serializer.handle_kb_annotations(problem=problem, kb_items=kb_items, user=user) # type: ignore return Response({"id": problem.pk}, status=status) diff --git a/backend/problem/views/problem_test.py b/backend/problem/views/problem_test.py index 364fb67..56b1c93 100644 --- a/backend/problem/views/problem_test.py +++ b/backend/problem/views/problem_test.py @@ -1,6 +1,8 @@ import pytest from rest_framework import status +from problem.models import Problem + @pytest.fixture def problem_input_data(): @@ -9,7 +11,14 @@ def problem_input_data(): "premises": ["Test premise 1", "Test premise 2"], "hypothesis": "Test hypothesis", "entailmentLabel": "neutral", - "kbItems": [], + "kbItems": [ + { + "entity1": "dog", + "entity2": "canine", + "relationship": "equal", + "notes": "Test note", + } + ], } @@ -120,12 +129,12 @@ def test_master_annotator_can_create_problem( assert response.status_code == status.HTTP_201_CREATED assert "id" in response.json() - # Update + # Update core problem fields (hypothesis, premises, base) def test_unauthenticated_user_cannot_update_problem( self, client, sample_problem, problem_input_data ): - """Unauthenticated users should not be able to update problems.""" + """Unauthenticated users should not be able to update core problem fields.""" response = client.patch( f"/api/problem/{sample_problem.id}/", problem_input_data, @@ -136,7 +145,7 @@ def test_unauthenticated_user_cannot_update_problem( def test_visitor_cannot_update_problem( self, client, visitor, sample_problem, problem_input_data ): - """Visitors should not be able to update problems or KB items.""" + """Visitors should not be able to update core problem fields.""" client.force_login(user=visitor) response = client.patch( f"/api/problem/{sample_problem.id}/", @@ -145,30 +154,255 @@ def test_visitor_cannot_update_problem( ) assert response.status_code == status.HTTP_403_FORBIDDEN - def test_annotator_can_update_problem( + def test_annotators_cannot_update_problem( self, client, annotator, sample_problem, problem_input_data ): - """Annotators should be able to update KB items (but not problems).""" + """Annotators should not be able to update core problem fields.""" + + # Make sure the update would change something. + old_hypothesis = sample_problem.hypothesis.text + old_premise = sample_problem.premises.first().text + assert old_hypothesis != problem_input_data["hypothesis"] + assert old_premise != problem_input_data["premises"][0] + client.force_login(user=annotator) response = client.patch( f"/api/problem/{sample_problem.id}/", problem_input_data, content_type="application/json", ) + + # Status code 200 because KB items are updated in the same view. assert response.status_code == status.HTTP_200_OK - def test_master_annotator_can_update_problem( + sample_problem.refresh_from_db() + assert sample_problem.hypothesis.text == old_hypothesis + assert sample_problem.premises.first().text == old_premise + + def test_master_annotator_can_update_user_problem( self, client, master_annotator, sample_problem, problem_input_data ): - """Master annotators should be able to update user-created problems.""" + """Master annotators should be able to update core fields on user-created problems.""" + + old_hypothesis = sample_problem.hypothesis.text + old_premise = sample_problem.premises.first().text + + assert sample_problem.dataset == Problem.Dataset.USER + assert old_hypothesis != problem_input_data["hypothesis"] + assert old_premise != problem_input_data["premises"][0] + client.force_login(user=master_annotator) response = client.patch( f"/api/problem/{sample_problem.id}/", problem_input_data, content_type="application/json", ) + + # Status code 200 because KB items are updated in the same view. assert response.status_code == status.HTTP_200_OK + sample_problem.refresh_from_db() + assert sample_problem.hypothesis.text == problem_input_data["hypothesis"] + assert sample_problem.premises.first().text == problem_input_data["premises"][0] + + def test_master_annotator_cannot_update_non_user_problem( + self, client, master_annotator, sample_problem, problem_input_data + ): + """Master annotators should not be able to update core fields on non-user-created problems.""" + # Change the sample problem to be non-user-created. + sample_problem.dataset = Problem.Dataset.SNLI + sample_problem.save() + + old_hypothesis = sample_problem.hypothesis.text + old_premise = sample_problem.premises.first().text + + assert old_hypothesis != problem_input_data["hypothesis"] + assert old_premise != problem_input_data["premises"][0] + + client.force_login(user=master_annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + + # Status code 200 because KB items are updated in the same view. + assert response.status_code == status.HTTP_200_OK + + sample_problem.refresh_from_db() + # Core fields should not be updated since the problem is not user-created. + assert sample_problem.hypothesis.text == old_hypothesis + assert sample_problem.premises.first().text == old_premise + + # Update KB annotations + + def test_unauthenticated_user_cannot_update_kb_annotations( + self, client, sample_problem, problem_input_data + ): + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_visitor_cannot_update_kb_annotations( + self, client, visitor, sample_problem, problem_input_data + ): + client.force_login(user=visitor) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_annotator_can_create_kb_annotations( + self, client, annotator, sample_problem, problem_input_data + ): + assert sample_problem.knowledgebaseannotations.count() == 0 + + client.force_login(user=annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + sample_problem.refresh_from_db() + assert ( + sample_problem.knowledgebaseannotations.count() == 1 + ), "Annotator should be able to create a KB annotation." + kb_annotation = sample_problem.knowledgebaseannotations.first() + assert ( + kb_annotation.entity1 == problem_input_data["kbItems"][0]["entity1"] + ), "KB annotation entity1 should match input data." + + def test_annotator_can_update_kb_annotations( + self, client, annotator, sample_problem, problem_input_data, kb_annotation + ): + assert sample_problem.knowledgebaseannotations.count() == 1 + first_kb = sample_problem.knowledgebaseannotations.first() + assert first_kb.entity1 != problem_input_data["kbItems"][0]["entity1"] + + problem_input_data["kbItems"][0]["id"] = kb_annotation.id + + client.force_login(user=annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + sample_problem.refresh_from_db() + kb_annotation.refresh_from_db() + + assert ( + sample_problem.knowledgebaseannotations.count() == 1 + ), "Updating a KB annotation as an annotator should not create a new one." + assert ( + kb_annotation.entity1 == problem_input_data["kbItems"][0]["entity1"] + ), "Annotator should be able to update a KB annotation." + + def test_annotator_can_remove_kb_annotations( + self, client, annotator, sample_problem, problem_input_data, kb_annotation + ): + assert kb_annotation.removed_at is None + problem_input_data["kbItems"] = [] + + client.force_login(user=annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + kb_annotation.refresh_from_db() + assert ( + kb_annotation.removed_at is not None + ), "Annotator should be able to mark a KB annotation as removed." + + def test_master_annotator_can_create_kb_annotations( + self, client, master_annotator, sample_problem, problem_input_data + ): + assert sample_problem.knowledgebaseannotations.count() == 0 + + client.force_login(user=master_annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + sample_problem.refresh_from_db() + assert ( + sample_problem.knowledgebaseannotations.count() == 1 + ), "Master annotator should be able to create a KB annotation." + kb_annotation = sample_problem.knowledgebaseannotations.first() + assert ( + kb_annotation.entity1 == problem_input_data["kbItems"][0]["entity1"] + ), "KB annotation entity1 should match input data." + + def test_master_annotator_can_update_kb_annotations( + self, + client, + master_annotator, + sample_problem, + problem_input_data, + kb_annotation, + ): + assert sample_problem.knowledgebaseannotations.count() == 1 + first_kb = sample_problem.knowledgebaseannotations.first() + assert first_kb.entity1 != problem_input_data["kbItems"][0]["entity1"] + + problem_input_data["kbItems"][0]["id"] = kb_annotation.id + + client.force_login(user=master_annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + sample_problem.refresh_from_db() + kb_annotation.refresh_from_db() + + assert ( + sample_problem.knowledgebaseannotations.count() == 1 + ), "Updating a KB annotation as a master annotator should not create a new one." + assert ( + kb_annotation.entity1 == problem_input_data["kbItems"][0]["entity1"] + ), "Master annotator should be able to update a KB annotation." + + def test_master_annotator_can_remove_kb_annotations( + self, + client, + master_annotator, + sample_problem, + problem_input_data, + kb_annotation, + ): + assert kb_annotation.removed_at is None + problem_input_data["kbItems"] = [] + + client.force_login(user=master_annotator) + response = client.patch( + f"/api/problem/{sample_problem.id}/", + problem_input_data, + content_type="application/json", + ) + assert response.status_code == status.HTTP_200_OK + + kb_annotation.refresh_from_db() + assert ( + kb_annotation.removed_at is not None + ), "Master annotator should be able to mark a KB annotation as removed." + class TestUserRoleProperties: """Tests for user role property methods used by permissions.""" diff --git a/backend/user/models.py b/backend/user/models.py index 7b37efb..4578226 100644 --- a/backend/user/models.py +++ b/backend/user/models.py @@ -58,6 +58,13 @@ def can_create_problem(self) -> bool: """ return self.has_perm("problem.add_problem") + @property + def can_copy_problem(self) -> bool: + """ + Determines whether the user can copy existing problems. + """ + return self.has_perm("problem.copy_problems") + @property def can_edit_kb(self) -> bool: """ @@ -68,9 +75,16 @@ def can_edit_kb(self) -> bool: """ return self.has_perm("annotation.change_knowledgebaseannotation") - def can_remove_label(self, label_annotation: LabelAnnotation) -> bool: + @property + def can_add_label_annotations(self) -> bool: + """ + Determines whether the user can add label annotations. + """ + return self.has_perm("annotation.add_labelannotation") + + def can_remove_label_annotation(self, label_annotation: LabelAnnotation) -> bool: """ - Determines whether the user can remove a specific label (as part of an annotation). + Determines whether the user can mark a specific label annotation as removed. """ if self.is_superuser or self.has_perm("annotation.delete_any_labelannotation"): return True diff --git a/backend/user/permissions.py b/backend/user/permissions.py index cb37fc2..a15c4ce 100644 --- a/backend/user/permissions.py +++ b/backend/user/permissions.py @@ -3,7 +3,6 @@ ("problem", "view_silver_problems"), ("annotation", "change_knowledgebaseannotation"), ("annotation", "add_labelannotation"), - ("annotation", "change_labelannotation"), ("annotation", "delete_own_labelannotation"), ] diff --git a/backend/user/serializers.py b/backend/user/serializers.py index 4f2e8a4..ba3f4a4 100644 --- a/backend/user/serializers.py +++ b/backend/user/serializers.py @@ -12,6 +12,8 @@ class CustomUserDetailsSerializer(UserDetailsSerializer): read_only=True, source="can_create_problem" ) canEditKb = serializers.BooleanField(read_only=True, source="can_edit_kb") + canAddLabelAnnotations = serializers.BooleanField(read_only=True, source="can_add_label_annotations") + canCopyProblem = serializers.BooleanField(read_only=True, source="can_copy_problem") class Meta(UserDetailsSerializer.Meta): @@ -26,5 +28,7 @@ class Meta(UserDetailsSerializer.Meta): "canEditProblem", "canCreateProblem", "canEditKb", + "canAddLabelAnnotations", + "canCopyProblem", ) read_only_fields = ["isStaff", "id", "email"] diff --git a/backend/user/tests/test_user_views.py b/backend/user/tests/test_user_views.py index 013cd60..e6cf88e 100644 --- a/backend/user/tests/test_user_views.py +++ b/backend/user/tests/test_user_views.py @@ -15,6 +15,8 @@ def test_user_details(user_client, user_data): "canCreateProblem": False, "canEditProblem": False, "canEditKb": False, + "canCopyProblem": False, + "canAddLabelAnnotations": False, } diff --git a/frontend/src/app/annotate/annotate.component.html b/frontend/src/app/annotate/annotate.component.html index ce0e6a1..e195f85 100644 --- a/frontend/src/app/annotate/annotate.component.html +++ b/frontend/src/app/annotate/annotate.component.html @@ -38,6 +38,18 @@

This will update this problem in the database.

+ } @if (firstProblemId) { + + + Browse existing problems + } } @else { @@ -60,18 +72,6 @@ > Add new problem - } @else if (firstProblemId) { - - - Browse existing problems - } diff --git a/frontend/src/app/annotate/annotation-input/annotation-input.component.html b/frontend/src/app/annotate/annotation-input/annotation-input.component.html index 6b1ce9e..d4328fb 100644 --- a/frontend/src/app/annotate/annotation-input/annotation-input.component.html +++ b/frontend/src/app/annotate/annotation-input/annotation-input.component.html @@ -1,6 +1,7 @@ @let problem = problem$ | async; @let appMode = appMode$ | async; @let userProblem = isUserProblem$ | async; +@let editingOrAdding = appMode === 'edit' || appMode === 'add'; @if (problem) {
@@ -18,7 +19,7 @@ label="Parse and prove" (click)="startParse()" /> - @if (appMode === 'browse') { + @if (appMode === 'browse' && (canCopyProblem$ | async)) { Copy problem } - @if (this.form.dirty) { + @if (this.form.dirty || editingOrAdding) { user?.canEditProblem ?? false) ); + public canCopyProblem$ = this.authService.currentUser$.pipe( + map(user => user?.canCopyProblem ?? false) + ); + ngOnInit(): void { this.problemService.problem$ .pipe(takeUntilDestroyed(this.destroyRef)) diff --git a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.html b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.html index 1b60f39..b4a6467 100644 --- a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.html +++ b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.html @@ -38,9 +38,9 @@
Attached labels
{{ annotation.label.description }} - @if (getAttachedByText(annotation)) { + @if (getAttachedBy(annotation)) {
- {{ getAttachedByText(annotation) }} + {{ getAttachedBy(annotation) }}
} diff --git a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.spec.ts b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.spec.ts index f43b528..687e176 100644 --- a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.spec.ts +++ b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.spec.ts @@ -183,7 +183,7 @@ describe("ManageLabelsModalComponent", () => { createdAt: "2024-03-15T10:30:00Z" }; - const text = component.getAttachedByText(annotation); + const text = component.getAttachedBy(annotation); expect(text).toContain("you"); expect(text).toContain('15 March 2024'); diff --git a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.ts b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.ts index 19b321f..64fc212 100644 --- a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.ts +++ b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/manage-labels-modal/manage-labels-modal.component.ts @@ -7,7 +7,7 @@ import { toSignal } from '@angular/core/rxjs-interop'; import { AuthService } from '@/services/auth.service'; import { map, combineLatest, Subject, startWith, defer, Observable, filter } from 'rxjs'; import { AsyncPipe } from '@angular/common'; -import { formatDate } from '@/util'; +import { getAttachedByText } from '@/util'; import { Label, LabelAnnotation } from '@/types'; type SelectedLabelsForm = FormGroup<{ @@ -103,10 +103,7 @@ export class ManageLabelsModalComponent implements OnInit { this.activeModal.close(transformedValue); } - public getAttachedByText(annotation: LabelAnnotation): string { - const attachedUser = annotation.attachedByCurrentUser ? $localize`you` : annotation.createdBy; - return $localize`Attached by ${attachedUser} on ${formatDate(annotation.createdAt)}`; - } + public getAttachedBy = getAttachedByText; private currentUserName = toSignal( this.authService.currentUser$.pipe(map((user) => user?.username)) diff --git a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.html b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.html index f68b90f..a0558ee 100644 --- a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.html +++ b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.html @@ -13,12 +13,14 @@ } @empty { No labels attached to this problem yet } - + @if (canManageLabels$ | async) { + + }
diff --git a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.ts b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.ts index 464ca23..565c5e2 100644 --- a/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.ts +++ b/frontend/src/app/annotate/annotation-input/problem-details/problem-labels/problem-labels.component.ts @@ -4,16 +4,18 @@ import { FontAwesomeModule } from '@fortawesome/angular-fontawesome'; import { NgbTooltipModule, NgbModal } from '@ng-bootstrap/ng-bootstrap'; import { ManageLabelsModalComponent, ManageLabelsModalResult } from './manage-labels-modal/manage-labels-modal.component'; import { LabelAnnotation, SaveLabelsResponse } from '@/types'; -import { catchError, Subject, switchMap } from 'rxjs'; +import { catchError, map, Subject, switchMap } from 'rxjs'; import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; import { ToastService } from '@/services/toast.service'; import { HttpClient } from '@angular/common/http'; import { ProblemService } from '@/services/problem.service'; import { getLabelTooltipText, sortAnnotations } from '@/util'; +import { AuthService } from '@/services/auth.service'; +import { CommonModule } from '@angular/common'; @Component({ selector: 'la-problem-labels', - imports: [FontAwesomeModule, NgbTooltipModule], + imports: [FontAwesomeModule, NgbTooltipModule, CommonModule], templateUrl: './problem-labels.component.html', styleUrl: './problem-labels.component.scss' }) @@ -22,6 +24,7 @@ export class ProblemLabelsComponent implements OnInit { private toastService = inject(ToastService); private http = inject(HttpClient); private problemService = inject(ProblemService); + private authService = inject(AuthService); public problemId = input.required(); public labelAnnotations = input.required(); @@ -32,6 +35,10 @@ export class ProblemLabelsComponent implements OnInit { public faSearch = faSearch; + public canManageLabels$ = this.authService.currentUser$.pipe( + map(user => user?.canAddLabelAnnotations ?? false) + ); + private saveLabels$ = new Subject(); constructor(private modalService: NgbModal) { } diff --git a/frontend/src/app/menu/user-menu/user-menu.component.spec.ts b/frontend/src/app/menu/user-menu/user-menu.component.spec.ts index ee9275b..746a1dd 100644 --- a/frontend/src/app/menu/user-menu/user-menu.component.spec.ts +++ b/frontend/src/app/menu/user-menu/user-menu.component.spec.ts @@ -23,7 +23,9 @@ const fakeUserResponse: UserResponse = { role: UserRole.VISITOR, canCreateProblem: false, canEditProblem: false, - canEditKb: false + canEditKb: false, + canAddLabelAnnotations: false, + canCopyProblem: false, }; const fakeAdminResponse: UserResponse = { @@ -36,7 +38,9 @@ const fakeAdminResponse: UserResponse = { role: UserRole.MASTER_ANNOTATOR, canCreateProblem: true, canEditProblem: true, - canEditKb: false + canEditKb: true, + canAddLabelAnnotations: true, + canCopyProblem: true, }; describe("UserMenuComponent", () => { diff --git a/frontend/src/app/user/models/user.ts b/frontend/src/app/user/models/user.ts index c3a2c8e..daf5b32 100644 --- a/frontend/src/app/user/models/user.ts +++ b/frontend/src/app/user/models/user.ts @@ -9,6 +9,8 @@ export interface UserResponse { canEditProblem: boolean; canCreateProblem: boolean; canEditKb: boolean; + canAddLabelAnnotations: boolean; + canCopyProblem: boolean; } // Corresponds to frontend user type. @@ -31,6 +33,8 @@ export class User { public canEditProblem: boolean, public canCreateProblem: boolean, public canEditKb: boolean, + public canAddLabelAnnotations: boolean, + public canCopyProblem: boolean, ) { } } diff --git a/frontend/src/app/user/user-settings/user-settings.component.spec.ts b/frontend/src/app/user/user-settings/user-settings.component.spec.ts index dac6dcb..d62d549 100644 --- a/frontend/src/app/user/user-settings/user-settings.component.spec.ts +++ b/frontend/src/app/user/user-settings/user-settings.component.spec.ts @@ -27,6 +27,8 @@ const fakeUser: User = { canCreateProblem: false, canEditProblem: false, canEditKb: false, + canAddLabelAnnotations: false, + canCopyProblem: false, }; @Injectable({ providedIn: "root" }) diff --git a/frontend/src/app/user/utils.spec.ts b/frontend/src/app/user/utils.spec.ts index 1079800..b6a2d78 100644 --- a/frontend/src/app/user/utils.spec.ts +++ b/frontend/src/app/user/utils.spec.ts @@ -43,6 +43,8 @@ describe("User utils", () => { canCreateProblem: false, canEditProblem: false, canEditKb: false, + canAddLabelAnnotations: false, + canCopyProblem: false, }; const user = parseUserData(result); expect(user).toBeInstanceOf(User); diff --git a/frontend/src/app/user/utils.ts b/frontend/src/app/user/utils.ts index de47913..a13f33e 100644 --- a/frontend/src/app/user/utils.ts +++ b/frontend/src/app/user/utils.ts @@ -33,6 +33,8 @@ export const parseUserData = (result: UserResponse | null): User | null => { result.canEditProblem, result.canCreateProblem, result.canEditKb, + result.canAddLabelAnnotations, + result.canCopyProblem, ); }; @@ -52,6 +54,7 @@ export const encodeUserData = (data: Partial): Partial => { firstName: data.firstName, lastName: data.lastName, isStaff: data.isStaff, + canCopyProblem: data.canCopyProblem, }; // Remove undefined values from object. return Object.fromEntries( diff --git a/frontend/src/app/util.ts b/frontend/src/app/util.ts index 6a7eb59..51bbcb1 100644 --- a/frontend/src/app/util.ts +++ b/frontend/src/app/util.ts @@ -9,16 +9,17 @@ function sortAnnotations(labelAnnotations: LabelAnnotation[]): LabelAnnotation[] return [...otherLabels, ...userLabels]; } +function getAttachedByText(annotation: LabelAnnotation): string { + const attachedUser = annotation.attachedByCurrentUser ? $localize`you` : annotation.createdBy; + return $localize`Attached by ${attachedUser} on ${formatDate(annotation.createdAt)}`; +} + /** * Get the tooltip text for a problem label. */ function getLabelTooltipText(annotation: LabelAnnotation): string { - const label = annotation.label; - let tooltip = label.description; - const dateStr = formatDate(annotation.createdAt); - const attachedUser = annotation.attachedByCurrentUser ? $localize`you` : annotation.createdBy; - tooltip += `\n\nAttached by ${attachedUser} on ${dateStr}`; - return tooltip; + const attachedBy = getAttachedByText(annotation); + return `${annotation.label.description} -- ${attachedBy}`; } /** @@ -43,4 +44,4 @@ function formatDate(date: string | null | undefined): string { return Intl.DateTimeFormat('en-GB', { year: 'numeric', month: 'long', day: 'numeric' }).format(dateObj); } -export { sum, formatDate, sortAnnotations, getLabelTooltipText }; +export { sum, formatDate, sortAnnotations, getLabelTooltipText, getAttachedByText };