From 9e9b14320229f4b80f68ecc0f6693c35c7be32da Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 16:00:47 +0100 Subject: [PATCH 01/12] Hide Browse button unless in edit mode --- .../src/app/annotate/annotate.component.html | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) 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 - } From c067633d8b6cc2db3d36238f349e0c1fb29ce106 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 16:28:24 +0100 Subject: [PATCH 02/12] Hide "Manage labels" button based on new user.can_add_label_annotations --- backend/user/models.py | 11 +++++++++-- backend/user/serializers.py | 2 ++ .../problem-labels.component.html | 18 ++++++++++-------- .../problem-labels/problem-labels.component.ts | 11 +++++++++-- .../menu/user-menu/user-menu.component.spec.ts | 6 ++++-- frontend/src/app/user/models/user.ts | 2 ++ .../user-settings.component.spec.ts | 1 + frontend/src/app/user/utils.spec.ts | 1 + frontend/src/app/user/utils.ts | 1 + 9 files changed, 39 insertions(+), 14 deletions(-) diff --git a/backend/user/models.py b/backend/user/models.py index 7b37efb..37271ed 100644 --- a/backend/user/models.py +++ b/backend/user/models.py @@ -68,9 +68,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/serializers.py b/backend/user/serializers.py index 4f2e8a4..d2cdbbc 100644 --- a/backend/user/serializers.py +++ b/backend/user/serializers.py @@ -12,6 +12,7 @@ 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") class Meta(UserDetailsSerializer.Meta): @@ -26,5 +27,6 @@ class Meta(UserDetailsSerializer.Meta): "canEditProblem", "canCreateProblem", "canEditKb", + "canAddLabelAnnotations", ) read_only_fields = ["isStaff", "id", "email"] 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..5fe8984 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,8 @@ const fakeUserResponse: UserResponse = { role: UserRole.VISITOR, canCreateProblem: false, canEditProblem: false, - canEditKb: false + canEditKb: false, + canAddLabelAnnotations: false, }; const fakeAdminResponse: UserResponse = { @@ -36,7 +37,8 @@ const fakeAdminResponse: UserResponse = { role: UserRole.MASTER_ANNOTATOR, canCreateProblem: true, canEditProblem: true, - canEditKb: false + canEditKb: true, + canAddLabelAnnotations: true, }; describe("UserMenuComponent", () => { diff --git a/frontend/src/app/user/models/user.ts b/frontend/src/app/user/models/user.ts index c3a2c8e..36abb61 100644 --- a/frontend/src/app/user/models/user.ts +++ b/frontend/src/app/user/models/user.ts @@ -9,6 +9,7 @@ export interface UserResponse { canEditProblem: boolean; canCreateProblem: boolean; canEditKb: boolean; + canAddLabelAnnotations: boolean; } // Corresponds to frontend user type. @@ -31,6 +32,7 @@ export class User { public canEditProblem: boolean, public canCreateProblem: boolean, public canEditKb: boolean, + public canAddLabelAnnotations: 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..47ae289 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,7 @@ const fakeUser: User = { canCreateProblem: false, canEditProblem: false, canEditKb: false, + canAddLabelAnnotations: false, }; @Injectable({ providedIn: "root" }) diff --git a/frontend/src/app/user/utils.spec.ts b/frontend/src/app/user/utils.spec.ts index 1079800..4dd427c 100644 --- a/frontend/src/app/user/utils.spec.ts +++ b/frontend/src/app/user/utils.spec.ts @@ -43,6 +43,7 @@ describe("User utils", () => { canCreateProblem: false, canEditProblem: false, canEditKb: false, + canAddLabelAnnotations: 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..7361864 100644 --- a/frontend/src/app/user/utils.ts +++ b/frontend/src/app/user/utils.ts @@ -33,6 +33,7 @@ export const parseUserData = (result: UserResponse | null): User | null => { result.canEditProblem, result.canCreateProblem, result.canEditKb, + result.canAddLabelAnnotations, ); }; From dbff84f56675ca3df062409aa8c41f31f116cd0f Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 16:29:40 +0100 Subject: [PATCH 03/12] Remove change_labelannotation permission We only need "ADD" and "DELETE". --- backend/annotation/views.py | 6 ++---- backend/user/permissions.py | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) 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/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"), ] From 7a9d5add840036b0685b7e08c7b3d3e7b92d3973 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 16:30:16 +0100 Subject: [PATCH 04/12] Deduplicate logic for LabelAnnotation.createdBy --- backend/annotation/serializers.py | 14 +++++++++++++- .../manage-labels-modal.component.html | 4 ++-- .../manage-labels-modal.component.spec.ts | 2 +- .../manage-labels-modal.component.ts | 7 ++----- frontend/src/app/util.ts | 15 ++++++++------- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/backend/annotation/serializers.py b/backend/annotation/serializers.py index fde805b..e4f9edb 100644 --- a/backend/annotation/serializers.py +++ b/backend/annotation/serializers.py @@ -17,7 +17,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 +45,18 @@ 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: + """ + Return the full name of the user who created the annotation or a + default string if not available. + """ + default = "unknown user" + if annotation.created_by is None: + return default + + created_by_name = f"{annotation.created_by.first_name} {annotation.created_by.last_name}".strip() + return created_by_name if created_by_name else default + class KnowledgeBaseAnnotationSerializer(AnnotationBaseSerializer): """ 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/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 }; From a4dd4105eacb131b6e507d4e639b2f0f91e4326d Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 17:02:28 +0100 Subject: [PATCH 05/12] Add user.canCopyProblems and use in frontend to hide buttons --- backend/user/models.py | 7 +++++++ backend/user/serializers.py | 2 ++ backend/user/tests/test_user_views.py | 1 + .../annotation-input/annotation-input.component.html | 2 +- .../annotation-input/annotation-input.component.ts | 4 ++++ .../src/app/menu/user-menu/user-menu.component.spec.ts | 2 ++ frontend/src/app/user/models/user.ts | 2 ++ .../app/user/user-settings/user-settings.component.spec.ts | 1 + frontend/src/app/user/utils.ts | 2 ++ 9 files changed, 22 insertions(+), 1 deletion(-) diff --git a/backend/user/models.py b/backend/user/models.py index 37271ed..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: """ diff --git a/backend/user/serializers.py b/backend/user/serializers.py index d2cdbbc..ba3f4a4 100644 --- a/backend/user/serializers.py +++ b/backend/user/serializers.py @@ -13,6 +13,7 @@ class CustomUserDetailsSerializer(UserDetailsSerializer): ) 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): @@ -28,5 +29,6 @@ class Meta(UserDetailsSerializer.Meta): "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..b8f2195 100644 --- a/backend/user/tests/test_user_views.py +++ b/backend/user/tests/test_user_views.py @@ -15,6 +15,7 @@ def test_user_details(user_client, user_data): "canCreateProblem": False, "canEditProblem": False, "canEditKb": False, + "canCopyProblem": False, } 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 2764894..61f948a 100644 --- a/frontend/src/app/annotate/annotation-input/annotation-input.component.html +++ b/frontend/src/app/annotate/annotation-input/annotation-input.component.html @@ -18,7 +18,7 @@ label="Parse and proof" (click)="startParse()" /> - @if (appMode === 'browse') { + @if (appMode === 'browse' && (canCopyProblem$ | async)) { 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/menu/user-menu/user-menu.component.spec.ts b/frontend/src/app/menu/user-menu/user-menu.component.spec.ts index 5fe8984..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 @@ -25,6 +25,7 @@ const fakeUserResponse: UserResponse = { canEditProblem: false, canEditKb: false, canAddLabelAnnotations: false, + canCopyProblem: false, }; const fakeAdminResponse: UserResponse = { @@ -39,6 +40,7 @@ const fakeAdminResponse: UserResponse = { canEditProblem: true, 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 36abb61..daf5b32 100644 --- a/frontend/src/app/user/models/user.ts +++ b/frontend/src/app/user/models/user.ts @@ -10,6 +10,7 @@ export interface UserResponse { canCreateProblem: boolean; canEditKb: boolean; canAddLabelAnnotations: boolean; + canCopyProblem: boolean; } // Corresponds to frontend user type. @@ -33,6 +34,7 @@ export class User { 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 47ae289..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 @@ -28,6 +28,7 @@ const fakeUser: User = { canEditProblem: false, canEditKb: false, canAddLabelAnnotations: false, + canCopyProblem: false, }; @Injectable({ providedIn: "root" }) diff --git a/frontend/src/app/user/utils.ts b/frontend/src/app/user/utils.ts index 7361864..a13f33e 100644 --- a/frontend/src/app/user/utils.ts +++ b/frontend/src/app/user/utils.ts @@ -34,6 +34,7 @@ export const parseUserData = (result: UserResponse | null): User | null => { result.canCreateProblem, result.canEditKb, result.canAddLabelAnnotations, + result.canCopyProblem, ); }; @@ -53,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( From 187030bbb0f77927ef025830740093ac34a72749 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 17:02:50 +0100 Subject: [PATCH 06/12] Always show Save Problem button in editing mode --- .../annotate/annotation-input/annotation-input.component.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 61f948a..19b92cb 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) {
@@ -34,7 +35,7 @@ Copy problem } - @if (this.form.dirty) { + @if (this.form.dirty || editingOrAdding) { Date: Mon, 23 Feb 2026 17:03:01 +0100 Subject: [PATCH 07/12] Update README with latest permission overview --- README.md | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) 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 From a5de9e40cef09c49cebef2aeadc72973c205d532 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Mon, 23 Feb 2026 18:56:48 +0100 Subject: [PATCH 08/12] Fix tests --- backend/user/tests/test_user_views.py | 1 + frontend/src/app/user/utils.spec.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/backend/user/tests/test_user_views.py b/backend/user/tests/test_user_views.py index b8f2195..e6cf88e 100644 --- a/backend/user/tests/test_user_views.py +++ b/backend/user/tests/test_user_views.py @@ -16,6 +16,7 @@ def test_user_details(user_client, user_data): "canEditProblem": False, "canEditKb": False, "canCopyProblem": False, + "canAddLabelAnnotations": False, } diff --git a/frontend/src/app/user/utils.spec.ts b/frontend/src/app/user/utils.spec.ts index 4dd427c..b6a2d78 100644 --- a/frontend/src/app/user/utils.spec.ts +++ b/frontend/src/app/user/utils.spec.ts @@ -44,6 +44,7 @@ describe("User utils", () => { canEditProblem: false, canEditKb: false, canAddLabelAnnotations: false, + canCopyProblem: false, }; const user = parseUserData(result); expect(user).toBeInstanceOf(User); From 9fab13a4371d228e61368d3a5bf1af8c3e4cd914 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Thu, 26 Feb 2026 17:12:43 +0100 Subject: [PATCH 09/12] Simplify createdBy --- backend/annotation/serializers.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/backend/annotation/serializers.py b/backend/annotation/serializers.py index e4f9edb..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, @@ -46,16 +47,9 @@ def get_removable(self, annotation) -> bool: raise NotImplementedError("Subclasses must implement get_removable method.") def get_createdBy(self, annotation) -> str | None: - """ - Return the full name of the user who created the annotation or a - default string if not available. - """ - default = "unknown user" - if annotation.created_by is None: - return default - - created_by_name = f"{annotation.created_by.first_name} {annotation.created_by.last_name}".strip() - return created_by_name if created_by_name else default + """Returns the full name of the user who created the annotation.""" + return annotation.created_by.get_full_name() + class KnowledgeBaseAnnotationSerializer(AnnotationBaseSerializer): From 650592c918b92b25b56d2c48e84fd747db4b95e8 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Fri, 27 Feb 2026 09:43:10 +0100 Subject: [PATCH 10/12] Move permission checks to view --- backend/problem/serializers.py | 61 +++++++++-------------------- backend/problem/serializers_test.py | 25 ++++++------ backend/problem/views/problem.py | 44 +++++++++++++++------ 3 files changed, 66 insertions(+), 64 deletions(-) diff --git a/backend/problem/serializers.py b/backend/problem/serializers.py index 5fec960..15a8b21 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) - 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..b60d38a 100644 --- a/backend/problem/serializers_test.py +++ b/backend/problem/serializers_test.py @@ -130,7 +130,7 @@ def test_kb_create_no_permission(user_problem, visitor): ] serializer = input_serializer_with_user(visitor) - serializer._handle_kb_annotations(user_problem, kb_input) # type: ignore + serializer.handle_kb_annotations(user_problem, kb_input) # type: ignore assert ( KnowledgeBaseAnnotation.objects.filter(problem=user_problem).count() == 0 @@ -159,7 +159,7 @@ def test_kb_update_no_permission(user_problem, kb_annotation, visitor): assert original_entity1 != updated_entity_1 serializer = input_serializer_with_user(visitor) - serializer._handle_kb_annotations(user_problem, kb_input) # type: ignore + serializer.handle_kb_annotations(user_problem, kb_input) # type: ignore # Verify KB annotation was not updated kb_annotation.refresh_from_db() @@ -178,7 +178,7 @@ def test_kb_mark_removed_no_permission(user_problem, kb_annotation, visitor): 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 + serializer.handle_kb_annotations(user_problem, kb_input) # type: ignore # Verify KB annotation was not removed kb_annotation.refresh_from_db() @@ -200,7 +200,7 @@ def test_create_single_kb_annotation(user_problem, annotator): ] serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer.handle_kb_annotations(user_problem, kb_input) kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=user_problem) assert kb_annotations.count() == 1, "One KB annotation should have been created." @@ -231,7 +231,7 @@ 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.handle_kb_annotations(user_problem, kb_input) # Verify KB annotation was updated kb_annotation.refresh_from_db() @@ -251,7 +251,7 @@ 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.handle_kb_annotations(user_problem, kb_input) # Verify KB annotation was marked as removed kb_annotation.refresh_from_db() @@ -287,7 +287,7 @@ def test_create_and_update_multiple_kb_annotations( ] serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer.handle_kb_annotations(user_problem, kb_input) kb_annotations = KnowledgeBaseAnnotation.objects.filter( problem=user_problem, removed_at__isnull=True @@ -353,7 +353,7 @@ def test_create_update_and_remove_kb_annotations( ] serializer = input_serializer_with_user(annotator) - serializer._handle_kb_annotations(user_problem, kb_input) + serializer.handle_kb_annotations(user_problem, kb_input) # Verify kb1 was updated. kb1.refresh_from_db() @@ -504,10 +504,14 @@ def test_update_non_user_problem_adds_kb_only(non_user_problem, annotator): } # Preconditions - assert KnowledgeBaseAnnotation.objects.filter(problem=non_user_problem).count() == 0, "Non_user_problem should have no KB annotations to begin with." + 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) + serializer = input_serializer_with_user( + annotator, data=data, instance=non_user_problem + ) assert serializer.is_valid(raise_exception=True) updated_problem = serializer.save() @@ -518,4 +522,3 @@ def test_update_non_user_problem_adds_kb_only(non_user_problem, annotator): # Verify KB annotation was added kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=updated_problem) assert kb_annotations.count() == 1 - diff --git a/backend/problem/views/problem.py b/backend/problem/views/problem.py index ea607b8..6ee1724 100644 --- a/backend/problem/views/problem.py +++ b/backend/problem/views/problem.py @@ -2,7 +2,7 @@ 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 +15,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 +29,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 +148,38 @@ 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, context={"user": user}) 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 + else: - problem_instance = get_object_or_404( - Problem, id=problem_id - ) - problem: Problem = serializer.update(problem_instance, validated_input) - status = HTTP_200_OK + problem_instance = get_object_or_404(Problem, id=problem_id) + problem: Problem = serializer.update(problem_instance, validated_input) if user.can_edit_problem else problem_instance + + kb_items = validated_input.get("kbItems", []) + if kb_items: + serializer.handle_kb_annotations(problem=problem, kb_items=kb_items, user=user) return Response({"id": problem.pk}, status=status) From 8b739102d91779462443d672f63e498092f9ea85 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Fri, 27 Feb 2026 11:21:29 +0100 Subject: [PATCH 11/12] Move permission checks to view (and tests) --- backend/problem/serializers.py | 2 +- backend/problem/serializers_test.py | 224 +---------------------- backend/problem/views/problem.py | 26 ++- backend/problem/views/problem_test.py | 250 +++++++++++++++++++++++++- 4 files changed, 268 insertions(+), 234 deletions(-) diff --git a/backend/problem/serializers.py b/backend/problem/serializers.py index 15a8b21..76a05ca 100644 --- a/backend/problem/serializers.py +++ b/backend/problem/serializers.py @@ -100,7 +100,7 @@ def create(self, validated_data: dict) -> Problem: """ Create a new Problem instance from validated input data. """ - new_user_problem = Problem(dataset=Problem.Dataset.USER) + new_user_problem = Problem(dataset=Problem.Dataset.USER, extra_data={}) return self._update_core_problem_fields(new_user_problem, validated_data) diff --git a/backend/problem/serializers_test.py b/backend/problem/serializers_test.py index b60d38a..ecbf44c 100644 --- a/backend/problem/serializers_test.py +++ b/backend/problem/serializers_test.py @@ -117,76 +117,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.""" @@ -200,7 +130,7 @@ def test_create_single_kb_annotation(user_problem, annotator): ] serializer = input_serializer_with_user(annotator) - serializer.handle_kb_annotations(user_problem, kb_input) + serializer.handle_kb_annotations(user_problem, kb_input, annotator) kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=user_problem) assert kb_annotations.count() == 1, "One KB annotation should have been created." @@ -231,7 +161,7 @@ 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.handle_kb_annotations(user_problem, kb_input, annotator) # Verify KB annotation was updated kb_annotation.refresh_from_db() @@ -251,7 +181,7 @@ 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.handle_kb_annotations(user_problem, kb_input, annotator) # Verify KB annotation was marked as removed kb_annotation.refresh_from_db() @@ -287,7 +217,7 @@ def test_create_and_update_multiple_kb_annotations( ] serializer = input_serializer_with_user(annotator) - serializer.handle_kb_annotations(user_problem, kb_input) + serializer.handle_kb_annotations(user_problem, kb_input, annotator) kb_annotations = KnowledgeBaseAnnotation.objects.filter( problem=user_problem, removed_at__isnull=True @@ -337,7 +267,6 @@ def test_create_update_and_remove_kb_annotations( created_by=annotator, ) - # request = Mock(user=annotator) kb_input = [ { "id": kb1.pk, @@ -353,7 +282,7 @@ def test_create_update_and_remove_kb_annotations( ] serializer = input_serializer_with_user(annotator) - serializer.handle_kb_annotations(user_problem, kb_input) + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # Verify kb1 was updated. kb1.refresh_from_db() @@ -379,146 +308,3 @@ def test_create_update_and_remove_kb_annotations( 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/views/problem.py b/backend/problem/views/problem.py index 6ee1724..8e0b885 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, HTTP_401_UNAUTHORIZED, HTTP_403_FORBIDDEN +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 @@ -173,13 +178,22 @@ def _handle_update_create_problem( ) 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) if user.can_edit_problem else problem_instance - + 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 kb_items: - serializer.handle_kb_annotations(problem=problem, kb_items=kb_items, user=user) + 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.""" From 1a763d9829c5b07f4671a756c328e26249828305 Mon Sep 17 00:00:00 2001 From: Xander Vertegaal Date: Fri, 27 Feb 2026 11:32:38 +0100 Subject: [PATCH 12/12] Remove unnecessary context passing to serializer --- backend/problem/serializers_test.py | 22 ++++++++++------------ backend/problem/serializers_test_utils.py | 16 ---------------- backend/problem/views/problem.py | 2 +- 3 files changed, 11 insertions(+), 29 deletions(-) delete mode 100644 backend/problem/serializers_test_utils.py diff --git a/backend/problem/serializers_test.py b/backend/problem/serializers_test.py index ecbf44c..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 @@ -129,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, annotator) + 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." @@ -160,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, annotator) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore # Verify KB annotation was updated kb_annotation.refresh_from_db() @@ -180,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, annotator) + 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() @@ -216,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, annotator) + 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 @@ -281,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, annotator) + serializer = ProblemInputSerializer() + serializer.handle_kb_annotations(user_problem, kb_input, annotator) # type: ignore # Verify kb1 was updated. kb1.refresh_from_db() @@ -307,4 +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" - 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 8e0b885..fa4f223 100644 --- a/backend/problem/views/problem.py +++ b/backend/problem/views/problem.py @@ -164,7 +164,7 @@ def _handle_update_create_problem( input_data = request.data - serializer = ProblemInputSerializer(data=input_data, context={"user": user}) + serializer = ProblemInputSerializer(data=input_data) serializer.is_valid(raise_exception=True) validated_input: dict = serializer.validated_data # type: ignore