diff options
author | Mete Polat <metepolat2000@gmail.com> | 2020-02-27 23:29:32 +0000 |
---|---|---|
committer | Daniel Axtens <dja@axtens.net> | 2020-03-16 11:15:57 +1100 |
commit | 83f364aad66c35e31df8d0871ec2b62016eba337 (patch) | |
tree | 9a9c650ec0e11f18ca5b25145ec64ae9689ba378 | |
parent | 27c2acf56cd30e77c932a1dde87b6fc1de8eeb2c (diff) | |
download | patchwork-83f364aad66c35e31df8d0871ec2b62016eba337.tar patchwork-83f364aad66c35e31df8d0871ec2b62016eba337.tar.gz |
REST: Add patch relations
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. Because this is a writable many-many nested relationship, it
behaves a little unusually. 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
Signed-off-by: Mete Polat <metepolat2000@gmail.com>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Daniel Axtens <dja@axtens.net>
-rw-r--r-- | docs/api/schemas/latest/patchwork.yaml | 44 | ||||
-rw-r--r-- | docs/api/schemas/patchwork.j2 | 60 | ||||
-rw-r--r-- | docs/api/schemas/v1.1/patchwork.yaml | 18 | ||||
-rw-r--r-- | docs/api/schemas/v1.2/patchwork.yaml | 44 | ||||
-rw-r--r-- | patchwork/api/embedded.py | 25 | ||||
-rw-r--r-- | patchwork/api/event.py | 3 | ||||
-rw-r--r-- | patchwork/api/patch.py | 125 | ||||
-rw-r--r-- | patchwork/tests/api/test_patch.py | 2 | ||||
-rw-r--r-- | patchwork/tests/api/test_relation.py | 319 | ||||
-rw-r--r-- | patchwork/tests/utils.py | 11 | ||||
-rw-r--r-- | releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml | 11 |
11 files changed, 656 insertions, 6 deletions
diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml index 4cee151..13cdc9c 100644 --- a/docs/api/schemas/latest/patchwork.yaml +++ b/docs/api/schemas/latest/patchwork.yaml @@ -352,6 +352,7 @@ paths: - patch-created - patch-completed - patch-state-changed + - patch-relation-changed - patch-delegated - check-created - series-created @@ -390,6 +391,7 @@ paths: - $ref: '#/components/schemas/EventPatchCreated' - $ref: '#/components/schemas/EventPatchCompleted' - $ref: '#/components/schemas/EventPatchStateChanged' + - $ref: '#/components/schemas/EventPatchRelationChanged' - $ref: '#/components/schemas/EventPatchDelegated' - $ref: '#/components/schemas/EventCheckCreated' - $ref: '#/components/schemas/EventSeriesCreated' @@ -403,6 +405,8 @@ paths: '#/components/schemas/EventPatchCompleted' patch-state-changed: > '#/components/schemas/EventPatchStateChanged' + patch-relation-changed: > + '#/components/schemas/EventPatchRelationChanged' patch-delegated: > '#/components/schemas/EventPatchDelegated' check-created: '#/components/schemas/EventCheckCreated' @@ -552,6 +556,12 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' + '409': + description: Conflict + content: + application/json: + schema: + $ref: '#/components/schemas/Error' tags: - patches put: @@ -595,6 +605,12 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' + '409': + description: Conflict + content: + application/json: + schema: + $ref: '#/components/schemas/Error' tags: - patches /api/patches/{id}/comments/: @@ -1721,6 +1737,24 @@ components: current_state: title: Current state type: string + EventPatchRelationChanged: + allOf: + - $ref: '#/components/schemas/EventBase' + - type: object + properties: + category: + enum: + - patch-relation-changed + payload: + properties: + patch: + $ref: '#/components/schemas/PatchEmbedded' + previous_relation: + title: Previous relation + type: string + current_relation: + title: Current relation + type: string EventPatchDelegated: allOf: - $ref: '#/components/schemas/EventBase' @@ -1893,6 +1927,11 @@ components: items: type: string readOnly: true + related: + title: Relations + type: array + items: + type: string PatchDetail: allOf: - $ref: '#/components/schemas/PatchList' @@ -1943,6 +1982,11 @@ components: title: Delegate type: integer nullable: true + related: + title: Relations + type: array + items: + type: string Person: type: object properties: diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2 index c2f5ea6..bd714d5 100644 --- a/docs/api/schemas/patchwork.j2 +++ b/docs/api/schemas/patchwork.j2 @@ -359,6 +359,9 @@ paths: - patch-created - patch-completed - patch-state-changed +{% if version >= (1, 2) %} + - patch-relation-changed +{% endif %} - patch-delegated - check-created - series-created @@ -397,6 +400,9 @@ paths: - $ref: '#/components/schemas/EventPatchCreated' - $ref: '#/components/schemas/EventPatchCompleted' - $ref: '#/components/schemas/EventPatchStateChanged' +{% if version >= (1, 2) %} + - $ref: '#/components/schemas/EventPatchRelationChanged' +{% endif %} - $ref: '#/components/schemas/EventPatchDelegated' - $ref: '#/components/schemas/EventCheckCreated' - $ref: '#/components/schemas/EventSeriesCreated' @@ -410,6 +416,10 @@ paths: '#/components/schemas/EventPatchCompleted' patch-state-changed: > '#/components/schemas/EventPatchStateChanged' +{% if version >= (1, 2) %} + patch-relation-changed: > + '#/components/schemas/EventPatchRelationChanged' +{% endif %} patch-delegated: > '#/components/schemas/EventPatchDelegated' check-created: '#/components/schemas/EventCheckCreated' @@ -561,6 +571,14 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' +{% if version >= (1, 2) %} + '409': + description: Conflict + content: + application/json: + schema: + $ref: '#/components/schemas/Error' +{% endif %} tags: - patches put: @@ -604,6 +622,14 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' +{% if version >= (1, 2) %} + '409': + description: Conflict + content: + application/json: + schema: + $ref: '#/components/schemas/Error' +{% endif %} tags: - patches /api/{{ version_url }}patches/{id}/comments/: @@ -1777,6 +1803,26 @@ components: current_state: title: Current state type: string +{% if version >= (1, 1) %} + EventPatchRelationChanged: + allOf: + - $ref: '#/components/schemas/EventBase' + - type: object + properties: + category: + enum: + - patch-relation-changed + payload: + properties: + patch: + $ref: '#/components/schemas/PatchEmbedded' + previous_relation: + title: Previous relation + type: string + current_relation: + title: Current relation + type: string +{% endif %} EventPatchDelegated: allOf: - $ref: '#/components/schemas/EventBase' @@ -1955,6 +2001,13 @@ components: items: type: string readOnly: true +{% if version >= (1, 2) %} + related: + title: Relations + type: array + items: + type: string +{% endif %} PatchDetail: allOf: - $ref: '#/components/schemas/PatchList' @@ -2005,6 +2058,13 @@ components: title: Delegate type: integer nullable: true +{% if version >= (1, 2) %} + related: + title: Relations + type: array + items: + type: string +{% endif %} Person: type: object properties: diff --git a/docs/api/schemas/v1.1/patchwork.yaml b/docs/api/schemas/v1.1/patchwork.yaml index babc972..6b497ab 100644 --- a/docs/api/schemas/v1.1/patchwork.yaml +++ b/docs/api/schemas/v1.1/patchwork.yaml @@ -1551,6 +1551,24 @@ components: current_state: title: Current state type: string + EventPatchRelationChanged: + allOf: + - $ref: '#/components/schemas/EventBase' + - type: object + properties: + category: + enum: + - patch-relation-changed + payload: + properties: + patch: + $ref: '#/components/schemas/PatchEmbedded' + previous_relation: + title: Previous relation + type: string + current_relation: + title: Current relation + type: string EventPatchDelegated: allOf: - $ref: '#/components/schemas/EventBase' diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml index be79e38..db2ed12 100644 --- a/docs/api/schemas/v1.2/patchwork.yaml +++ b/docs/api/schemas/v1.2/patchwork.yaml @@ -352,6 +352,7 @@ paths: - patch-created - patch-completed - patch-state-changed + - patch-relation-changed - patch-delegated - check-created - series-created @@ -390,6 +391,7 @@ paths: - $ref: '#/components/schemas/EventPatchCreated' - $ref: '#/components/schemas/EventPatchCompleted' - $ref: '#/components/schemas/EventPatchStateChanged' + - $ref: '#/components/schemas/EventPatchRelationChanged' - $ref: '#/components/schemas/EventPatchDelegated' - $ref: '#/components/schemas/EventCheckCreated' - $ref: '#/components/schemas/EventSeriesCreated' @@ -403,6 +405,8 @@ paths: '#/components/schemas/EventPatchCompleted' patch-state-changed: > '#/components/schemas/EventPatchStateChanged' + patch-relation-changed: > + '#/components/schemas/EventPatchRelationChanged' patch-delegated: > '#/components/schemas/EventPatchDelegated' check-created: '#/components/schemas/EventCheckCreated' @@ -552,6 +556,12 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' + '409': + description: Conflict + content: + application/json: + schema: + $ref: '#/components/schemas/Error' tags: - patches put: @@ -595,6 +605,12 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' + '409': + description: Conflict + content: + application/json: + schema: + $ref: '#/components/schemas/Error' tags: - patches /api/1.2/patches/{id}/comments/: @@ -1721,6 +1737,24 @@ components: current_state: title: Current state type: string + EventPatchRelationChanged: + allOf: + - $ref: '#/components/schemas/EventBase' + - type: object + properties: + category: + enum: + - patch-relation-changed + payload: + properties: + patch: + $ref: '#/components/schemas/PatchEmbedded' + previous_relation: + title: Previous relation + type: string + current_relation: + title: Current relation + type: string EventPatchDelegated: allOf: - $ref: '#/components/schemas/EventBase' @@ -1893,6 +1927,11 @@ components: items: type: string readOnly: true + related: + title: Relations + type: array + items: + type: string PatchDetail: allOf: - $ref: '#/components/schemas/PatchList' @@ -1943,6 +1982,11 @@ components: title: Delegate type: integer nullable: true + related: + title: Relations + type: array + items: + type: string Person: type: object properties: diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py index de4f311..85a30ca 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,30 @@ 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 44c3452..d7a99c7 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 1a3ce90..efa38e1 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -1,27 +1,34 @@ # 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 import email.parser +from django.core.exceptions import ValidationError 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 +61,15 @@ 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 +83,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 +126,16 @@ 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 'related' in data: + 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 +143,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 +178,94 @@ 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 + ( @@ -174,7 +289,7 @@ class PatchList(ListAPIView): def get_queryset(self): return Patch.objects.all()\ - .prefetch_related('check_set')\ + .prefetch_related('check_set', 'related__patches__project')\ .select_related('project', 'state', 'submitter', 'delegate', 'series__project')\ .defer('content', 'diff', 'headers') @@ -196,6 +311,6 @@ class PatchDetail(RetrieveUpdateAPIView): def get_queryset(self): return Patch.objects.all()\ - .prefetch_related('check_set')\ + .prefetch_related('check_set', 'related__patches__project')\ .select_related('project', 'state', 'submitter', 'delegate', 'series') diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index 6e5e6a0..aba92b9 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -215,7 +215,7 @@ class TestPatchAPI(utils.APITestCase): series = create_series() create_patches(5, series=series) - with self.assertNumQueries(3): + with self.assertNumQueries(4): self.client.get(self.api_url()) @utils.store_samples('patch-detail') diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py new file mode 100644 index 0000000..d48c62b --- /dev/null +++ b/patchwork/tests/api/test_relation.py @@ -0,0 +1,319 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2020, IBM Corporation +# +# SPDX-License-Identifier: GPL-2.0-or-later + +import unittest + +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 5e6a1b1..7759c8f 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 0000000..44e704c --- /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. |