From patchwork Tue Jan 28 13:55:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 1230368 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 486Sqs2mVRz9sNx for ; Wed, 29 Jan 2020 00:58:01 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.a=rsa-sha256 header.s=google header.b=eCLMnjW5; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 486Sqs1y97zDqLM for ; Wed, 29 Jan 2020 00:58:01 +1100 (AEDT) X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=axtens.net (client-ip=2607:f8b0:4864:20::1030; helo=mail-pj1-x1030.google.com; envelope-from=dja@axtens.net; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.a=rsa-sha256 header.s=google header.b=eCLMnjW5; dkim-atps=neutral Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 486SnP59ZmzDqHW for ; Wed, 29 Jan 2020 00:55:53 +1100 (AEDT) Received: by mail-pj1-x1030.google.com with SMTP id gv17so1046326pjb.1 for ; Tue, 28 Jan 2020 05:55:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=w8fUD5E2qdAKhioeOKVYuIGv4Cp/gR7FoYC49NtsPEA=; b=eCLMnjW5M5v6sjxLUr6YvDpMF+BnCRt0IIp9fgypG2KrePSX+MKQfvJZ5ZtM5opff0 QJt8IKPCIjTYQQAQXOz+8Ht18VjnwafOlY0fOr1H6gx+vtBePvo8cZ4F7xppH882ZMTY YiMM0L9cxUOt5x/4/2BC6t7/UrAhuNBgLyL98= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=w8fUD5E2qdAKhioeOKVYuIGv4Cp/gR7FoYC49NtsPEA=; b=WNlZ5Liqb99/fD6KirfW5QIMAl613lk0r2dLyKo4EvjWR9tQt9UMdu0lXkc5ttH9ra PYq1ylf+AM/ym+ay9Me5FCKKqGyGT4YQBltUaaGqonA7Sdgb9NrutFOvN0+heG1OqXQc zJ/MGocpjT+QjZS3TTLXu9juBwOpwU9pqy7WqJYeJ2MTx63jeVn8c2unSlfQI3SL+1bE uieCTJOR8qG3tSAd1RjuhMc6IRaONDuk0H8CjVSMBDBrtSdxdaf8Zbn9CLJagQZFDBvM haB0MXQvm64tipjz33Cvtrr0H2CF1Y6tCdgWzPLzUfVqAI/VHVl3SDkbvfa4U/BmMkSJ wccg== X-Gm-Message-State: APjAAAUkOWEMmHqKJPCPKPrWUo/DZ4isZdka3/hwjZgE1/rwMc5qZ/aZ BHZkrSubl/OhhcNhoznq9PkajvvYO0s= X-Google-Smtp-Source: APXvYqy4d7zFPuTxEVkm9NR0g12981Pl5RoUvLdd2JWrmNRvBZt4OEZ5K4xSY9fcN5hMOtJmDFBniA== X-Received: by 2002:a17:90a:d081:: with SMTP id k1mr4994152pju.57.1580219749386; Tue, 28 Jan 2020 05:55:49 -0800 (PST) Received: from localhost (2001-44b8-111e-5c00-dc05-6a2b-858f-0270.static.ipv6.internode.on.net. [2001:44b8:111e:5c00:dc05:6a2b:858f:270]) by smtp.gmail.com with ESMTPSA id z130sm20960143pgz.6.2020.01.28.05.55.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Jan 2020 05:55:48 -0800 (PST) From: Daniel Axtens To: patchwork@lists.ozlabs.org Subject: [PATCH 4/4] REST: Add patch relations Date: Wed, 29 Jan 2020 00:55:23 +1100 Message-Id: <20200128135523.29808-5-dja@axtens.net> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200128135523.29808-1-dja@axtens.net> References: <20200128135523.29808-1-dja@axtens.net> MIME-Version: 1.0 X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" From: Mete Polat View relations and add/update/delete them as a maintainer. Maintainers can only create relations of patches which are part of a project they maintain. I've significantly rewritten this from Mete's original submission based on how Stephen wants the API to work. In short: - all operations use PATCH to the 'related' field of a patch - to relate a patch to another patch, say 7 to 19, either: PATCH /api/patch/7 related := [19] PATCH /api/patch/19 related := [7] - to delete a patch from a relation, say 1, 21 and 42 are related but we only want it to be 1 and 42: PATCH /api/patch/21 related := [] * You _cannot_ remove a patch from a relation by patching another patch in the relation: I'm trying to avoid read-modify-write loops. * Relations that would be left with just 1 patch are deleted. This is only ensured in the API - the admin interface will let you do this. - break-before-make: if you have [1, 12, 24] and [7, 15, 42] and you want to end up with [1, 12, 15, 42], you have to remove 15 from the second relation first: PATCH /api/patch/1 related := [15] will fail with 409 Conflict. Instead do: PATCH /api/patch/15 related := [] PATCH /api/patch/1 related := [15] -> 200 OK, [1, 12, 15, 42] and [7, 42] are the resulting relations I think this allows you to do things in a reasonably sensible way. I've preserved the BMW copyright header and moved it into api/patch.py, because I have taken some chunks of code verbatim. I have dropped it from the tests because I have completely rewritten them in the process of redoing the API. Signed-off-by: Mete Polat Signed-off-by: Daniel Axtens --- Todos: - j2 API docs - pep8, tox tests - optimise db queries: I haven't optimised the queries yet because I keep getting sidetracked by the random queries for a project linkname - general nitpickery --- patchwork/api/embedded.py | 23 ++ patchwork/api/event.py | 3 + patchwork/api/patch.py | 112 ++++++- patchwork/tests/api/test_relation.py | 304 ++++++++++++++++++ patchwork/tests/utils.py | 11 + .../add-patch-relations-c96bb6c567b416d8.yaml | 11 + 6 files changed, 461 insertions(+), 3 deletions(-) create mode 100644 patchwork/tests/api/test_relation.py create mode 100644 releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py index de4f31165ee7..506873c4adb6 100644 --- a/patchwork/api/embedded.py +++ b/patchwork/api/embedded.py @@ -14,6 +14,7 @@ from collections import OrderedDict from rest_framework.serializers import CharField from rest_framework.serializers import SerializerMethodField from rest_framework.serializers import PrimaryKeyRelatedField +from rest_framework.serializers import ValidationError from patchwork.api.base import BaseHyperlinkedModelSerializer from patchwork.api.base import CheckHyperlinkedIdentityField @@ -138,6 +139,28 @@ class PatchSerializer(SerializedRelatedField): } +class PatchRelationSerializer(BaseHyperlinkedModelSerializer): + """Hide the PatchRelation model, just show the list""" + patches = PatchSerializer(many=True) + + def to_internal_value(self, data): + if not isinstance(data, type([])): + raise ValidationError( + "Patch relations must be specified as a list of patch IDs" + ) + result = super(PatchRelationSerializer, self).to_internal_value({'patches': data}) + return result + + def to_representation(self, instance): + data = super(PatchRelationSerializer, self).to_representation(instance) + data = data['patches'] + return data + + class Meta: + model = models.PatchRelation + fields = ('patches',) + + class PersonSerializer(SerializedRelatedField): class _Serializer(BaseHyperlinkedModelSerializer): diff --git a/patchwork/api/event.py b/patchwork/api/event.py index d7685f4c138c..b8b535040116 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -13,6 +13,7 @@ from rest_framework.serializers import SlugRelatedField from patchwork.api.embedded import CheckSerializer from patchwork.api.embedded import CoverLetterSerializer from patchwork.api.embedded import PatchSerializer +from patchwork.api.embedded import PatchRelationSerializer from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer from patchwork.api.embedded import UserSerializer @@ -33,6 +34,8 @@ class EventSerializer(ModelSerializer): current_delegate = UserSerializer() created_check = SerializerMethodField() created_check = CheckSerializer() + previous_relation = PatchRelationSerializer(read_only=True) + current_relation = PatchRelationSerializer(read_only=True) _category_map = { Event.CATEGORY_COVER_CREATED: ['cover'], diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index a29a1ab0eb71..996547be1e96 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -1,5 +1,7 @@ # Patchwork - automated patch tracking system # Copyright (C) 2016 Linaro Corporation +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG) +# Copyright (C) 2020, IBM Corporation # # SPDX-License-Identifier: GPL-2.0-or-later @@ -7,21 +9,25 @@ import email.parser from django.utils.text import slugify from django.utils.translation import ugettext_lazy as _ +from rest_framework import status +from rest_framework.exceptions import APIException +from rest_framework.exceptions import PermissionDenied from rest_framework.generics import ListAPIView from rest_framework.generics import RetrieveUpdateAPIView from rest_framework.relations import RelatedField from rest_framework.reverse import reverse from rest_framework.serializers import SerializerMethodField -from rest_framework.serializers import ValidationError from patchwork.api.base import BaseHyperlinkedModelSerializer from patchwork.api.base import PatchworkPermission from patchwork.api.filters import PatchFilterSet +from patchwork.api.embedded import PatchRelationSerializer from patchwork.api.embedded import PersonSerializer from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer from patchwork.api.embedded import UserSerializer from patchwork.models import Patch +from patchwork.models import PatchRelation from patchwork.models import State from patchwork.parser import clean_subject @@ -54,6 +60,13 @@ class StateField(RelatedField): return State.objects.all() +class PatchConflict(APIException): + status_code = status.HTTP_409_CONFLICT + default_detail = 'At least one patch is already part of another ' \ + 'relation. You have to explicitly remove a patch ' \ + 'from its existing relation before moving it to this one.' + + class PatchListSerializer(BaseHyperlinkedModelSerializer): web_url = SerializerMethodField() @@ -67,6 +80,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): check = SerializerMethodField() checks = SerializerMethodField() tags = SerializerMethodField() + related = PatchRelationSerializer() def get_web_url(self, instance): request = self.context.get('request') @@ -109,6 +123,15 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): # will be removed in API v2 data = super(PatchListSerializer, self).to_representation(instance) data['series'] = [data['series']] if data['series'] else [] + + # stop the related serializer returning this patch in the list of + # related patches. Also make it return an empty list, not null/None + if data['related']: + data['related'] = [p for p in data['related'] + if p['id'] != instance.id] + else: + data['related'] = [] + return data class Meta: @@ -116,13 +139,13 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): fields = ('id', 'url', 'web_url', 'project', 'msgid', 'list_archive_url', 'date', 'name', 'commit_ref', 'pull_url', 'state', 'archived', 'hash', 'submitter', 'delegate', 'mbox', - 'series', 'comments', 'check', 'checks', 'tags') + 'series', 'comments', 'check', 'checks', 'tags', 'related',) read_only_fields = ('web_url', 'project', 'msgid', 'list_archive_url', 'date', 'name', 'hash', 'submitter', 'mbox', 'series', 'comments', 'check', 'checks', 'tags') versioned_fields = { '1.1': ('comments', 'web_url'), - '1.2': ('list_archive_url',), + '1.2': ('list_archive_url', 'related',), } extra_kwargs = { 'url': {'view_name': 'api-patch-detail'}, @@ -151,6 +174,89 @@ class PatchDetailSerializer(PatchListSerializer): def get_prefixes(self, instance): return clean_subject(instance.name)[1] + def update(self, instance, validated_data): + # d-r-f cannot handle writable nested models, so we handle that + # specifically ourselves and let d-r-f handle the rest + if 'related' not in validated_data: + return super(PatchDetailSerializer, self).update(instance, validated_data) + + related = validated_data.pop('related') + + # Validation rules + # ---------------- + # + # Permissions: to change a relation: + # for all patches in the relation, current and proposed, + # the user must be maintainer of the patch's project + # Note that this has a ratchet effect: if you add a cross-project + # relation, only you or another maintainer across both projects can + # modify that relationship in _any way_. + # + # Break before Make: a patch must be explicitly removed from a + # relation before being added to another + # + # No Read-Modify-Write for deletion: + # to delete a patch from a relation, clear _its_ related patch, + # don't modify one of the patches that are to remain. + # + # (As a consequence of those two, operations are additive: + # if 1 is in a relation with [1,2,3], then + # patching 1 with related=[2,4] gives related=[1,2,3,4]) + + # Permissions: + # Because we're in a serializer, not a view, this is a bit clunky + user = self.context['request'].user.profile + # Must be maintainer of: + # - current patch + self.check_user_maintains_all(user, [instance]) + # - all patches currently in relation + # - all patches proposed to be in relation + patches = set(related['patches']) if related else {} + if instance.related is not None: + patches = patches.union(instance.related.patches.all()) + self.check_user_maintains_all(user, patches) + + # handle deletion + if not related['patches']: + # do not allow relations with a single patch + if instance.related and instance.related.patches.count() == 2: + instance.related.delete() + instance.related = None + return super(PatchDetailSerializer, self).update(instance, validated_data) + + # break before make + relations = {patch.related for patch in patches if patch.related} + if len(relations) > 1: + raise PatchConflict() + if relations: + relation = relations.pop() + else: + relation = None + if relation and instance.related is not None: + if instance.related != relation: + raise PatchConflict() + + # apply + if relation is None: + relation = PatchRelation() + relation.save() + for patch in patches: + patch.related = relation + patch.save() + instance.related = relation + instance.save() + + return super(PatchDetailSerializer, self).update(instance, validated_data) + + @staticmethod + def check_user_maintains_all(user, patches): + maintains = user.maintainer_projects.all() + if any(s.project not in maintains for s in patches): + detail = 'At least one patch is part of a project you are ' \ + 'not maintaining.' + raise PermissionDenied(detail=detail) + return True + class Meta: model = Patch fields = PatchListSerializer.Meta.fields + ( diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py new file mode 100644 index 000000000000..fae9eeef5ada --- /dev/null +++ b/patchwork/tests/api/test_relation.py @@ -0,0 +1,304 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2020, IBM Corporation +# Author: Daniel Axtens +# +# SPDX-License-Identifier: GPL-2.0-or-later + +import unittest + +import six +from django.conf import settings +from django.urls import reverse + +from patchwork.models import Patch +from patchwork.models import PatchRelation + +from patchwork.tests.api import utils +from patchwork.tests.utils import create_maintainer +from patchwork.tests.utils import create_patch +from patchwork.tests.utils import create_patches +from patchwork.tests.utils import create_project +from patchwork.tests.utils import create_relation +from patchwork.tests.utils import create_user + +if settings.ENABLE_REST_API: + from rest_framework import status + + +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') +class TestRelationSimpleAPI(utils.APITestCase): + + @staticmethod + def api_url(item=None, version=None): + kwargs = {} + if version: + kwargs['version'] = version + + if item is None: + return reverse('api-patch-list', kwargs=kwargs) + kwargs['pk'] = item + return reverse('api-patch-detail', kwargs=kwargs) + + def setUp(self): + self.project = create_project() + self.normal_user = create_user() + self.maintainer = create_maintainer(self.project) + + def test_no_relation(self): + patch = create_patch(project=self.project) + resp = self.client.get(self.api_url(item=patch.pk)) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(resp.data['related'], []) + + @utils.store_samples('relation-list') + def test_list_two_patch_relation(self): + relation = create_relation(2, project=self.project) + patches = relation.patches.all() + + # nobody + resp = self.client.get(self.api_url(item=patches[0].pk)) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + + self.assertIn('related', resp.data) + self.assertEqual(len(resp.data['related']), 1) + self.assertEqual(resp.data['related'][0]['id'], patches[1].pk) + + resp = self.client.get(self.api_url(item=patches[1].pk)) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + + self.assertIn('related', resp.data) + self.assertEqual(len(resp.data['related']), 1) + self.assertEqual(resp.data['related'][0]['id'], patches[0].pk) + + @utils.store_samples('relation-create-forbidden') + def test_create_two_patch_relation_nobody(self): + patches = create_patches(2, project=self.project) + + resp = self.client.patch(self.api_url(item=patches[0].pk), + {'related': [patches[1].pk]}) + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + + def test_create_two_patch_relation_user(self): + patches = create_patches(2, project=self.project) + + self.client.force_authenticate(user=self.normal_user) + resp = self.client.patch(self.api_url(item=patches[0].pk), + {'related': [patches[1].pk]}) + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + + @utils.store_samples('relation-create-maintainer') + def test_create_two_patch_relation_maintainer(self): + patches = create_patches(2, project=self.project) + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patches[0].pk), + {'related': [patches[1].pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + + # reload and verify + patches = Patch.objects.all() + self.assertIsNotNone(patches[0].related) + self.assertIsNotNone(patches[1].related) + self.assertEqual(patches[1].related, patches[0].related) + + def test_delete_two_patch_relation_nobody(self): + relation = create_relation(project=self.project) + patch = relation.patches.all()[0] + + self.assertEqual(PatchRelation.objects.count(), 1) + + resp = self.client.patch(self.api_url(item=patch.pk), + {'related': []}) + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual(PatchRelation.objects.count(), 1) + + @utils.store_samples('relation-delete') + def test_delete_two_patch_relation_maintainer(self): + relation = create_relation(project=self.project) + patch = relation.patches.all()[0] + + self.assertEqual(PatchRelation.objects.count(), 1) + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patch.pk), + {'related': []}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + + self.assertEqual(PatchRelation.objects.count(), 0) + self.assertEqual(Patch.objects.filter(related__isnull=False).exists(), + False) + + def test_create_three_patch_relation(self): + patches = create_patches(3, project=self.project) + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patches[0].pk), + {'related': [patches[1].pk, patches[2].pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + + # reload and verify + patches = Patch.objects.all() + self.assertIsNotNone(patches[0].related) + self.assertIsNotNone(patches[1].related) + self.assertIsNotNone(patches[2].related) + self.assertEqual(patches[0].related, patches[1].related) + self.assertEqual(patches[1].related, patches[2].related) + + def test_delete_from_three_patch_relation(self): + relation = create_relation(3, project=self.project) + patch = relation.patches.all()[0] + + self.assertEqual(PatchRelation.objects.count(), 1) + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patch.pk), + {'related': []}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertIsNone(Patch.objects.get(id=patch.pk).related) + self.assertEqual(PatchRelation.objects.count(), 1) + self.assertEqual(PatchRelation.objects.first().patches.count(), 2) + + @utils.store_samples('relation-extend-through-new') + def test_extend_relation_through_new(self): + relation = create_relation(project=self.project) + existing_patch_a = relation.patches.first() + + new_patch = create_patch(project=self.project) + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=new_patch.pk), + {'related': [existing_patch_a.pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(relation, + Patch.objects.get(pk=new_patch.pk).related) + self.assertEqual(relation.patches.count(), 3) + + def test_extend_relation_through_old(self): + relation = create_relation(project=self.project) + existing_patch_a = relation.patches.first() + + new_patch = create_patch(project=self.project) + + # maintainer + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=existing_patch_a.pk), + {'related': [new_patch.pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(relation, + Patch.objects.get(id=new_patch.id).related) + self.assertEqual(relation.patches.count(), 3) + + def test_extend_relation_through_new_two(self): + relation = create_relation(project=self.project) + existing_patch_a = relation.patches.first() + + new_patch_a = create_patch(project=self.project) + new_patch_b = create_patch(project=self.project) + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=new_patch_a.pk), + {'related': [existing_patch_a.pk, + new_patch_b.pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(relation, + Patch.objects.get(id=new_patch_a.id).related) + self.assertEqual(relation, + Patch.objects.get(id=new_patch_b.id).related) + self.assertEqual(relation.patches.count(), 4) + + @utils.store_samples('relation-extend-through-old') + def test_extend_relation_through_old_two(self): + relation = create_relation(project=self.project) + existing_patch_a = relation.patches.first() + + new_patch_a = create_patch(project=self.project) + new_patch_b = create_patch(project=self.project) + + # maintainer + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=existing_patch_a.pk), + {'related': [new_patch_a.pk, new_patch_b.pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(relation, + Patch.objects.get(id=new_patch_a.id).related) + self.assertEqual(relation, + Patch.objects.get(id=new_patch_b.id).related) + self.assertEqual(relation.patches.count(), 4) + + def test_remove_one_patch_from_relation_bad(self): + relation = create_relation(3, project=self.project) + keep_patch_a = relation.patches.all()[1] + keep_patch_b = relation.patches.all()[2] + + # this should do nothing - it is interpreted as + # _adding_ keep_patch_b again which is a no-op. + + # maintainer + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=keep_patch_a.pk), + {'related': [keep_patch_b.pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(relation.patches.count(), 3) + + def test_remove_one_patch_from_relation_good(self): + relation = create_relation(3, project=self.project) + target_patch = relation.patches.all()[0] + + # maintainer + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=target_patch.pk), + {'related': []}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertIsNone(Patch.objects.get(id=target_patch.id).related) + self.assertEqual(relation.patches.count(), 2) + + @utils.store_samples('relation-forbid-moving-between-relations') + def test_forbid_moving_patch_between_relations(self): + """Test the break-before-make logic""" + relation_a = create_relation(project=self.project) + relation_b = create_relation(project=self.project) + + patch_a = relation_a.patches.first() + patch_b = relation_b.patches.first() + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patch_a.pk), + {'related': [patch_b.pk]}) + self.assertEqual(resp.status_code, status.HTTP_409_CONFLICT) + + resp = self.client.patch(self.api_url(item=patch_b.pk), + {'related': [patch_a.pk]}) + self.assertEqual(resp.status_code, status.HTTP_409_CONFLICT) + + def test_cross_project_different_maintainers(self): + patch_a = create_patch(project=self.project) + + project_b = create_project() + patch_b = create_patch(project=project_b) + + # maintainer a, patch in own project + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patch_a.pk), + {'related': [patch_b.pk]}) + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + + # maintainer a, patch in project b + resp = self.client.patch(self.api_url(item=patch_b.pk), + {'related': [patch_a.pk]}) + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + + def test_cross_project_relation_super_maintainer(self): + patch_a = create_patch(project=self.project) + + project_b = create_project() + patch_b = create_patch(project=project_b) + + project_b.maintainer_project.add(self.maintainer.profile) + project_b.save() + + self.client.force_authenticate(user=self.maintainer) + resp = self.client.patch(self.api_url(item=patch_a.pk), + {'related': [patch_b.pk]}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(Patch.objects.get(id=patch_a.id).related, + Patch.objects.get(id=patch_b.id).related) diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py index 5e6a1b130394..7759c8f37a30 100644 --- a/patchwork/tests/utils.py +++ b/patchwork/tests/utils.py @@ -16,6 +16,7 @@ from patchwork.models import Check from patchwork.models import Comment from patchwork.models import CoverLetter from patchwork.models import Patch +from patchwork.models import PatchRelation from patchwork.models import Person from patchwork.models import Project from patchwork.models import Series @@ -352,3 +353,13 @@ def create_covers(count=1, **kwargs): kwargs (dict): Overrides for various cover letter fields """ return _create_submissions(create_cover, count, **kwargs) + + +def create_relation(count_patches=2, **kwargs): + relation = PatchRelation.objects.create() + values = { + 'related': relation + } + values.update(kwargs) + create_patches(count_patches, **values) + return relation diff --git a/releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml b/releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml new file mode 100644 index 000000000000..44e704c85c20 --- /dev/null +++ b/releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml @@ -0,0 +1,11 @@ +--- +features: + - | + Patches can now be related to other patches (e.g. to cross-reference + revisions). Relations can be set via the REST API by maintainers + (currently only for patches of projects they maintain). Patches can + belong to at most one relation at a time. +api: + - | + Relations are available via ``/patches/{patchID}/`` endpoint, in + the ``related`` field.