[v3,5/5] REST: Add submission relations

View relations or add/update/delete them as a maintainer. Maintainers
can only create relations of submissions (patches/cover letters) which
are part of a project they maintain.

New REST API urls:
api/relations/
api/relations/<relation_id>/

Signed-off-by: Mete Polat <metepolat2000@gmail.com>
---
Previously it was possible to use the PatchSerializer. As we expanded the
support to submissions in general, it isn't a simple task anymore showing
hyperlinked submissions (as Patch and CoverLetter are not flattened into
one model yet). Right now only the submission ids are shown.

 docs/api/schemas/latest/patchwork.yaml        | 218 +++++++++++++++++
 docs/api/schemas/patchwork.j2                 | 230 ++++++++++++++++++
 docs/api/schemas/v1.2/patchwork.yaml          | 218 +++++++++++++++++
 patchwork/api/index.py                        |   1 +
 patchwork/api/relation.py                     |  73 ++++++
 patchwork/tests/api/test_relation.py          | 194 +++++++++++++++
 patchwork/tests/utils.py                      |  11 +
 patchwork/urls.py                             |  11 +
 ...submission-relations-c96bb6c567b416d8.yaml |  10 +
 9 files changed, 966 insertions(+)
 create mode 100644 patchwork/api/relation.py
 create mode 100644 patchwork/tests/api/test_relation.py
 create mode 100644 releasenotes/notes/add-submission-relations-c96bb6c567b416d8.yaml

Mete Polat <metepolat2000@gmail.com> writes:

> View relations or add/update/delete them as a maintainer. Maintainers
> can only create relations of submissions (patches/cover letters) which
> are part of a project they maintain.
>
> New REST API urls:
> api/relations/
> api/relations/<relation_id>/
>
> Signed-off-by: Mete Polat <metepolat2000@gmail.com>
> ---
> Previously it was possible to use the PatchSerializer. As we expanded the
> support to submissions in general, it isn't a simple task anymore showing
> hyperlinked submissions (as Patch and CoverLetter are not flattened into
> one model yet). Right now only the submission ids are shown.

Hmm, that's unfortunate. I didn't intend to lead you in to this problem,
sorry! Having said that, it'd be good to supply some common information.

How about this, which is super-gross but which I think is probably the
best we can do unless Stephen can chime in with something better...

diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index de4f31165ee7..8d44592b51f1 100644
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -102,6 +102,45 @@ class CheckSerializer(SerializedRelatedField):
             }
 
 
+def _upgrade_instance(instance):
+    if hasattr(instance, 'patch'):
+        return instance.patch
+    else:
+        return instance.coverletter
+
+
+class SubmissionSerializer(SerializedRelatedField):
+
+    class _Serializer(BaseHyperlinkedModelSerializer):
+        """We need to 'upgrade' or specialise the submission to the relevant
+        subclass, so we can't use the mixins. This is gross but can go away
+        once we flatten the models."""
+        url = SerializerMethodField()
+        web_url = SerializerMethodField()
+        mbox = SerializerMethodField()
+
+        def get_url(self, instance):
+            instance = _upgrade_instance(instance)
+            request = self.context.get('request')
+            return request.build_absolute_uri(instance.get_absolute_url())
+
+        def get_web_url(self, instance):
+            instance = _upgrade_instance(instance)
+            request = self.context.get('request')
+            return request.build_absolute_uri(instance.get_absolute_url())
+
+        def get_mbox(self, instance):
+            instance = _upgrade_instance(instance)
+            request = self.context.get('request')
+            return request.build_absolute_uri(instance.get_mbox_url())
+
+        class Meta:
+            model = models.Submission
+            fields = ('id', 'url', 'web_url', 'msgid', 'list_archive_url',
+                      'date', 'name', 'mbox')
+            read_only_fields = fields
+
+
 class CoverLetterSerializer(SerializedRelatedField):
 
     class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
index e7d002b9375a..c02a6fe67e2c 100644
--- a/patchwork/api/relation.py
+++ b/patchwork/api/relation.py
@@ -9,6 +9,7 @@ from rest_framework.generics import ListCreateAPIView
 from rest_framework.generics import RetrieveUpdateDestroyAPIView
 from rest_framework.serializers import ModelSerializer
 
+from patchwork.api.embedded import SubmissionSerializer
 from patchwork.models import SubmissionRelation
 
 
@@ -34,6 +35,8 @@ class MaintainerPermission(permissions.BasePermission):
 
 
 class SubmissionRelationSerializer(ModelSerializer):
+    submissions = SubmissionSerializer(many=True)
+
     class Meta:
         model = SubmissionRelation
         fields = ('id', 'url', 'submissions',)


>
>  docs/api/schemas/latest/patchwork.yaml        | 218 +++++++++++++++++
>  docs/api/schemas/patchwork.j2                 | 230 ++++++++++++++++++
>  docs/api/schemas/v1.2/patchwork.yaml          | 218 +++++++++++++++++
>  patchwork/api/index.py                        |   1 +
>  patchwork/api/relation.py                     |  73 ++++++
>  patchwork/tests/api/test_relation.py          | 194 +++++++++++++++
>  patchwork/tests/utils.py                      |  11 +
>  patchwork/urls.py                             |  11 +
>  ...submission-relations-c96bb6c567b416d8.yaml |  10 +
>  9 files changed, 966 insertions(+)
> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
> new file mode 100644
> index 0000000..e7d002b
> --- /dev/null
> +++ b/patchwork/api/relation.py
> @@ -0,0 +1,73 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +from django.db.models import Count
> +from rest_framework import permissions
> +from rest_framework.generics import ListCreateAPIView
> +from rest_framework.generics import RetrieveUpdateDestroyAPIView
> +from rest_framework.serializers import ModelSerializer
> +
> +from patchwork.models import SubmissionRelation
> +
> +
> +class MaintainerPermission(permissions.BasePermission):
> +
> +    def has_object_permission(self, request, view, submissions):
> +        if request.method in permissions.SAFE_METHODS:
> +            return True
> +
> +        user = request.user
> +        if not user.is_authenticated:
> +            return False
> +
> +        if isinstance(submissions, SubmissionRelation):
> +            submissions = list(submissions.submissions.all())
> +        maintaining = user.profile.maintainer_projects.all()
> +        return all(s.project in maintaining for s in submissions)

If I understand this correctly, you are saying that to have permissions,
for all patches, you must be a maintainer of that project.

That's correct, I think, but a comment spelling it out would be helpful:
perhaps it's because I don't do enough functional programming but it
took me a while to understand what you'd written.

Partially due to my lack of expertise with DRF, it wasn't clear to me
that this prevents the _creation_ (as opposed to modification) of a
relation where you're not the maintainer of all the involved projects,
but upon testing it would appear that it does, so that's good.

> +
> +    def has_permission(self, request, view):
> +        return request.method in permissions.SAFE_METHODS or \
> +               (request.user.is_authenticated and
> +                request.user.profile.maintainer_projects.count() > 0)
> +
> +
> +class SubmissionRelationSerializer(ModelSerializer):
> +    class Meta:
> +        model = SubmissionRelation
> +        fields = ('id', 'url', 'submissions',)
> +        read_only_fields = ('url',)
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-relation-detail'},
> +        }
> +
> +
> +class SubmissionRelationMixin:
> +    serializer_class = SubmissionRelationSerializer
> +    permission_classes = (MaintainerPermission,)
> +
> +    def get_queryset(self):
> +        return SubmissionRelation.objects.all() \
> +            .prefetch_related('submissions')

So prefetch_related always makes me perk up my ears.

Here, we end up doing an individual database query for every submission
that is in a relation (you can observe this with the Django Debug
Toolbar). That's probably not ideal: you could get potentially a large
number of relations per page with a large number of patches per
relation.

More interestingly, looking into it I think the way you've implemented
the model in patch 3 means that a patch can only have at most one
relation? Indeed, testing it shows that to be the case.

That's deeply counter-intuitive to me - shouldn't a patch be able to be
involved in multiple relations? Currently it can only be in one set of
relations (belong to one SubmissionRelation), it seems to me that it
ought to be able to be in multiple sets of relations.

I'm trying to think of a good example of where that makes sense, and I
think one is versions vs backports. So I say that v1 of patch A is
related to v2 and v3 of patch A, and it makes sense for that to be one
SubmissionRelation which v1, v2 and v3 all belong to. Then say that v3
is accepted, and then there's a stable backport of that to some old
trees. I would then say that v3 was related to the backport, but I'm not
sure I'd say that v1 was related to the backport in quite the same way.

So I would have thought there would be a many-to-many relationship
between submissions and relations. This would also facilitate relations
set by multiple tools where we want to be able to maintain some
separation.

Perhaps there's a good rationale for the current one-to-many
relationship, and I'm open to hearing it. If that's the case, then at the
very least, you shouldn't be able to silently _remove_ a patch from a
relation by adding it to another. You should be forced to first remove
the patch from the original relationship explictly, leaving it with
related=NULL, and then you can set another relationship.

Further to that, please could you add a field to SubmissionRelation with
some sort of comment/context/tool identification/etc so we can track
who's doing what. See e.g. the context field in the Check model.


> +
> +
> +class SubmissionRelationList(SubmissionRelationMixin, ListCreateAPIView):
> +    ordering = 'id'
> +    ordering_fields = ['id', 'submission_count']

What is the benefit of being able to order by submission_count?

It would be really nice to be able to filter by project, depending on
how feasible that is - it requires jumping though a JOIN which is
unpleasant...

> +
> +    def create(self, request, *args, **kwargs):
> +        serializer = self.get_serializer(data=request.data)
> +        serializer.is_valid(raise_exception=True)
> +        submissions = serializer.validated_data['submissions']
> +        self.check_object_permissions(request, submissions)
> +        return super().create(request, *args, **kwargs)
> +
> +    def get_queryset(self):
> +        return super().get_queryset() \
> +            .annotate(submission_count=Count('submission'))
> +
> +
> +class SubmissionRelationDetail(SubmissionRelationMixin,
> +                               RetrieveUpdateDestroyAPIView):
> +    pass
> diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py
> new file mode 100644
> index 0000000..296926d
> --- /dev/null
> +++ b/patchwork/tests/api/test_relation.py
> @@ -0,0 +1,194 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import unittest
> +from enum import Enum
> +from enum import auto
> +
> +import six
> +from django.conf import settings
> +from django.urls import reverse
> +
> +from patchwork.models import SubmissionRelation
> +from patchwork.tests.api import utils
> +from patchwork.tests.utils import create_maintainer
> +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
> +
> +
> +class UserType(Enum):
> +    ANONYMOUS = auto()
> +    NON_MAINTAINER = auto()
> +    MAINTAINER = auto()
> +
> +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestRelationAPI(utils.APITestCase):
> +    fixtures = ['default_tags']
> +
> +    @staticmethod
> +    def api_url(item=None):
> +        kwargs = {}
> +        if item is None:
> +            return reverse('api-relation-list', kwargs=kwargs)
> +        kwargs['pk'] = item
> +        return reverse('api-relation-detail', kwargs=kwargs)
> +
> +    def request_restricted(self, method, user_type: UserType):
> +        # setup
> +
> +        project = create_project()
> +
> +        if user_type == UserType.ANONYMOUS:
> +            expected_status = status.HTTP_403_FORBIDDEN
> +        elif user_type == UserType.NON_MAINTAINER:
> +            expected_status = status.HTTP_403_FORBIDDEN
> +            self.client.force_authenticate(user=create_user())
> +        elif user_type == UserType.MAINTAINER:
> +            if method == 'post':
> +                expected_status = status.HTTP_201_CREATED
> +            elif method == 'delete':
> +                expected_status = status.HTTP_204_NO_CONTENT
> +            else:
> +                expected_status = status.HTTP_200_OK
> +            user = create_maintainer(project)
> +            self.client.force_authenticate(user=user)
> +        else:
> +            raise ValueError
> +
> +        resource_id = None
> +        send = None
> +
> +        if method == 'delete':
> +            resource_id = create_relation(project=project).id
> +        elif method == 'post':
> +            patch_ids = [p.id for p in create_patches(2, project=project)]
> +            send = {'submissions': patch_ids}
> +        elif method == 'patch':
> +            resource_id = create_relation(project=project).id
> +            patch_ids = [p.id for p in create_patches(2, project=project)]
> +            send = {'submissions': patch_ids}
> +        else:
> +            raise ValueError
> +
> +        # request
> +
> +        resp = getattr(self.client, method)(self.api_url(resource_id), send)
> +
> +        # check
> +
> +        self.assertEqual(expected_status, resp.status_code)
> +
> +        if resp.status_code not in range(200, 202):
> +            return
> +
> +        if resource_id:
> +            self.assertEqual(resource_id, resp.data['id'])
> +
> +        send_ids = send['submissions']
> +        resp_ids = resp.data['submissions']
> +        six.assertCountEqual(self, resp_ids, send_ids)
> +
> +    def assertSerialized(self, obj: SubmissionRelation, resp: dict):
> +        self.assertEqual(obj.id, resp['id'])
> +        obj = [s.id for s in obj.submissions.all()]
> +        six.assertCountEqual(self, obj, resp['submissions'])
> +
> +    def test_list_empty(self):
> +        """List relation when none are present."""
> +        resp = self.client.get(self.api_url())
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertEqual(0, len(resp.data))
> +
> +    @utils.store_samples('relation-list')
> +    def test_list(self):
> +        """List relations."""
> +        relation = create_relation()
> +
> +        resp = self.client.get(self.api_url())
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertEqual(1, len(resp.data))
> +        self.assertSerialized(relation, resp.data[0])
> +
> +    def test_detail(self):
> +        """Show relation."""
> +        relation = create_relation()
> +
> +        resp = self.client.get(self.api_url(relation.id))
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertSerialized(relation, resp.data)
> +
> +    @utils.store_samples('relation-update-error-forbidden')
> +    def test_update_anonymous(self):
> +        """Update relation as anonymous user.
> +
> +        Ensure updates can be performed by maintainers.
> +        """
> +        self.request_restricted('patch', UserType.ANONYMOUS)
> +
> +    def test_update_non_maintainer(self):
> +        """Update relation as non-maintainer.
> +
> +        Ensure updates can be performed by maintainers.
> +        """
> +        self.request_restricted('patch', UserType.NON_MAINTAINER)
> +
> +    @utils.store_samples('relation-update')
> +    def test_update_maintainer(self):
> +        """Update relation as maintainer.
> +
> +        Ensure updates can be performed by maintainers.
> +        """
> +        self.request_restricted('patch', UserType.MAINTAINER)
> +
> +    @utils.store_samples('relation-delete-error-forbidden')
> +    def test_delete_anonymous(self):
> +        """Delete relation as anonymous user.
> +
> +        Ensure deletes can be performed by maintainers.
> +        """
> +        self.request_restricted('delete', UserType.ANONYMOUS)
> +
> +    def test_delete_non_maintainer(self):
> +        """Delete relation as non-maintainer.
> +
> +        Ensure deletes can be performed by maintainers.
> +        """
> +        self.request_restricted('delete', UserType.NON_MAINTAINER)
> +
> +    @utils.store_samples('relation-update')
> +    def test_delete_maintainer(self):
> +        """Delete relation as maintainer.
> +
> +        Ensure deletes can be performed by maintainers.
> +        """
> +        self.request_restricted('delete', UserType.MAINTAINER)
> +
> +    @utils.store_samples('relation-create-error-forbidden')
> +    def test_create_anonymous(self):
> +        """Create relation as anonymous user.
> +
> +        Ensure creates can be performed by maintainers.
> +        """
> +        self.request_restricted('post', UserType.ANONYMOUS)
> +
> +    def test_create_non_maintainer(self):
> +        """Create relation as non-maintainer.
> +
> +        Ensure creates can be performed by maintainers.
> +        """
> +        self.request_restricted('post', UserType.NON_MAINTAINER)
> +
> +    @utils.store_samples('relation-create')
> +    def test_create_maintainer(self):
> +        """Create relation as maintainer.
> +
> +        Ensure creates can be performed by maintainers.
> +        """
> +        self.request_restricted('post', UserType.MAINTAINER)
> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> index 577183d..47149de 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 SubmissionRelation
>  from patchwork.models import Person
>  from patchwork.models import Project
>  from patchwork.models import Series
> @@ -347,3 +348,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 = SubmissionRelation.objects.create()
> +    values = {
> +        'related': relation
> +    }
> +    values.update(kwargs)
> +    create_patches(count_patches, **values)
> +    return relation


I haven't looked at the tests in great detail, but they look very
comprehensive. You can get coverage data out of tox with -e coverage, so
just check that you've covered a reasonably amount of the new code
you're adding. (It doesn't necessarily have to be 100%.)


Lastly, you should probably also add a field on Patch and CoverLetter
that links to the related submissions if they exist. Otherwise I think
you have to enumerate all the relations in the API to determine if there
is one for the patch/cover letter that you're interested in?


Feel free to respin after you address these comments. Keep up the good
work!

Regards,
Daniel

Hi Daniel,

(sorry for the short delay)

On 06.11.19 16:12, Daniel Axtens wrote:
> Mete Polat <metepolat2000@gmail.com> writes:
> 
>> View relations or add/update/delete them as a maintainer. Maintainers
>> can only create relations of submissions (patches/cover letters) which
>> are part of a project they maintain.
>>
>> New REST API urls:
>> api/relations/
>> api/relations/<relation_id>/
>>
>> Signed-off-by: Mete Polat <metepolat2000@gmail.com>
>> ---
>> Previously it was possible to use the PatchSerializer. As we expanded the
>> support to submissions in general, it isn't a simple task anymore showing
>> hyperlinked submissions (as Patch and CoverLetter are not flattened into
>> one model yet). Right now only the submission ids are shown.
> 
> Hmm, that's unfortunate. I didn't intend to lead you in to this problem,
> sorry! Having said that, it'd be good to supply some common information.
> 
> How about this, which is super-gross but which I think is probably the
> best we can do unless Stephen can chime in with something better...
> 
> diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
> index de4f31165ee7..8d44592b51f1 100644
> --- a/patchwork/api/embedded.py
> +++ b/patchwork/api/embedded.py
> @@ -102,6 +102,45 @@ class CheckSerializer(SerializedRelatedField):
>              }
>  
>  
> +def _upgrade_instance(instance):
> +    if hasattr(instance, 'patch'):
> +        return instance.patch
> +    else:
> +        return instance.coverletter
> +
> +
> +class SubmissionSerializer(SerializedRelatedField):
> +
> +    class _Serializer(BaseHyperlinkedModelSerializer):
> +        """We need to 'upgrade' or specialise the submission to the relevant
> +        subclass, so we can't use the mixins. This is gross but can go away
> +        once we flatten the models."""
> +        url = SerializerMethodField()
> +        web_url = SerializerMethodField()
> +        mbox = SerializerMethodField()
> +
> +        def get_url(self, instance):
> +            instance = _upgrade_instance(instance)
> +            request = self.context.get('request')
> +            return request.build_absolute_uri(instance.get_absolute_url())
> +
> +        def get_web_url(self, instance):
> +            instance = _upgrade_instance(instance)
> +            request = self.context.get('request')
> +            return request.build_absolute_uri(instance.get_absolute_url())
> +

Nice, thank you. I think this should be sufficient. Just want to note
that get_url and get_web_url are showing the same url. I will change
that in the next revision.

> +        def get_mbox(self, instance):
> +            instance = _upgrade_instance(instance)
> +            request = self.context.get('request')
> +            return request.build_absolute_uri(instance.get_mbox_url())
> +
> +        class Meta:
> +            model = models.Submission
> +            fields = ('id', 'url', 'web_url', 'msgid', 'list_archive_url',
> +                      'date', 'name', 'mbox')
> +            read_only_fields = fields
> +
> +
>  class CoverLetterSerializer(SerializedRelatedField):
>  
>      class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
> index e7d002b9375a..c02a6fe67e2c 100644
> --- a/patchwork/api/relation.py
> +++ b/patchwork/api/relation.py
> @@ -9,6 +9,7 @@ from rest_framework.generics import ListCreateAPIView
>  from rest_framework.generics import RetrieveUpdateDestroyAPIView
>  from rest_framework.serializers import ModelSerializer
>  
> +from patchwork.api.embedded import SubmissionSerializer
>  from patchwork.models import SubmissionRelation
>  
>  
> @@ -34,6 +35,8 @@ class MaintainerPermission(permissions.BasePermission):
>  
>  
>  class SubmissionRelationSerializer(ModelSerializer):
> +    submissions = SubmissionSerializer(many=True)
> +
>      class Meta:
>          model = SubmissionRelation
>          fields = ('id', 'url', 'submissions',)
> 
> 
>>
>>  docs/api/schemas/latest/patchwork.yaml        | 218 +++++++++++++++++
>>  docs/api/schemas/patchwork.j2                 | 230 ++++++++++++++++++
>>  docs/api/schemas/v1.2/patchwork.yaml          | 218 +++++++++++++++++
>>  patchwork/api/index.py                        |   1 +
>>  patchwork/api/relation.py                     |  73 ++++++
>>  patchwork/tests/api/test_relation.py          | 194 +++++++++++++++
>>  patchwork/tests/utils.py                      |  11 +
>>  patchwork/urls.py                             |  11 +
>>  ...submission-relations-c96bb6c567b416d8.yaml |  10 +
>>  9 files changed, 966 insertions(+)
>> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
>> new file mode 100644
>> index 0000000..e7d002b
>> --- /dev/null
>> +++ b/patchwork/api/relation.py
>> @@ -0,0 +1,73 @@
>> +# Patchwork - automated patch tracking system
>> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
>> +#
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +from django.db.models import Count
>> +from rest_framework import permissions
>> +from rest_framework.generics import ListCreateAPIView
>> +from rest_framework.generics import RetrieveUpdateDestroyAPIView
>> +from rest_framework.serializers import ModelSerializer
>> +
>> +from patchwork.models import SubmissionRelation
>> +
>> +
>> +class MaintainerPermission(permissions.BasePermission):
>> +
>> +    def has_object_permission(self, request, view, submissions):
>> +        if request.method in permissions.SAFE_METHODS:
>> +            return True
>> +
>> +        user = request.user
>> +        if not user.is_authenticated:
>> +            return False
>> +
>> +        if isinstance(submissions, SubmissionRelation):
>> +            submissions = list(submissions.submissions.all())
>> +        maintaining = user.profile.maintainer_projects.all()
>> +        return all(s.project in maintaining for s in submissions)
>> If I understand this correctly, you are saying that to have permissions,
> for all patches, you must be a maintainer of that project.
> 
> That's correct, I think, but a comment spelling it out would be helpful:
> perhaps it's because I don't do enough functional programming but it
> took me a while to understand what you'd written.
> 
> Partially due to my lack of expertise with DRF, it wasn't clear to me
> that this prevents the _creation_ (as opposed to modification) of a
> relation where you're not the maintainer of all the involved projects,
> but upon testing it would appear that it does, so that's good.
> 
>> +
>> +    def has_permission(self, request, view):
>> +        return request.method in permissions.SAFE_METHODS or \
>> +               (request.user.is_authenticated and
>> +                request.user.profile.maintainer_projects.count() > 0)
>> +
>> +
>> +class SubmissionRelationSerializer(ModelSerializer):
>> +    class Meta:
>> +        model = SubmissionRelation
>> +        fields = ('id', 'url', 'submissions',)
>> +        read_only_fields = ('url',)
>> +        extra_kwargs = {
>> +            'url': {'view_name': 'api-relation-detail'},
>> +        }
>> +
>> +
>> +class SubmissionRelationMixin:
>> +    serializer_class = SubmissionRelationSerializer
>> +    permission_classes = (MaintainerPermission,)
>> +
>> +    def get_queryset(self):
>> +        return SubmissionRelation.objects.all() \
>> +            .prefetch_related('submissions')
> 
> So prefetch_related always makes me perk up my ears.
> 
> Here, we end up doing an individual database query for every submission
> that is in a relation (you can observe this with the Django Debug
> Toolbar). That's probably not ideal: you could get potentially a large
> number of relations per page with a large number of patches per
> relation.
> > More interestingly, looking into it I think the way you've implemented
> the model in patch 3 means that a patch can only have at most one
> relation? Indeed, testing it shows that to be the case.
> 
> That's deeply counter-intuitive to me - shouldn't a patch be able to be
> involved in multiple relations? Currently it can only be in one set of
> relations (belong to one SubmissionRelation), it seems to me that it
> ought to be able to be in multiple sets of relations.
> 
> I'm trying to think of a good example of where that makes sense, and I
> think one is versions vs backports. So I say that v1 of patch A is
> related to v2 and v3 of patch A, and it makes sense for that to be one
> SubmissionRelation which v1, v2 and v3 all belong to. Then say that v3
> is accepted, and then there's a stable backport of that to some old
> trees. I would then say that v3 was related to the backport, but I'm not
> sure I'd say that v1 was related to the backport in quite the same way.
> 
> So I would have thought there would be a many-to-many relationship
> between submissions and relations. This would also facilitate relations
> set by multiple tools where we want to be able to maintain some
> separation.
> 
> Perhaps there's a good rationale for the current one-to-many
> relationship, and I'm open to hearing it. If that's the case, then at the
> very least, you shouldn't be able to silently _remove_ a patch from a
> relation by adding it to another. You should be forced to first remove
> the patch from the original relationship explictly, leaving it with
> related=NULL, and then you can set another relationship.

I think you are right that there are different types of relations
however I am not sure how you exactly want to display them.

When viewing a submission, how do we decide which SubmissionRelation to
display under 'related'? Do we list the submissions of every
SubmissionRelation a specific submission is part of? Do we list them
separately or combined? Should users be able to mark a
SubmissionRelation as something like "backport-relation" which can then
be displayed in a separate row when viewing a submission?

I think a more complex relation model could possibly lead to some
confusion and unnecessary difficulties.

Furthermore  I am not sure whether a more complex model really
facilitates tools. As an example, PaStA, the analysis tool we are
currently using for finding relations and commit_refs for a specific
patch, does not differentiate between different types of relations.

Nevertheless I am open to be convinced. I just don't want to complicate
things if there is really no need.

> Further to that, please could you add a field to SubmissionRelation with
> some sort of comment/context/tool identification/etc so we can track
> who's doing what. See e.g. the context field in the Check model.
> 
> 
>> +
>> +
>> +class SubmissionRelationList(SubmissionRelationMixin, ListCreateAPIView):
>> +    ordering = 'id'
>> +    ordering_fields = ['id', 'submission_count']
> 
> What is the benefit of being able to order by submission_count?

We can remove this. I just used it to see how the UI looks like for
different relation counts.

> It would be really nice to be able to filter by project, depending on
> how feasible that is - it requires jumping though a JOIN which is
> unpleasant...
> 

I will see how manageable this is.

>> +
>> +    def create(self, request, *args, **kwargs):
>> +        serializer = self.get_serializer(data=request.data)
>> +        serializer.is_valid(raise_exception=True)
>> +        submissions = serializer.validated_data['submissions']
>> +        self.check_object_permissions(request, submissions)
>> +        return super().create(request, *args, **kwargs)
>> +
>> +    def get_queryset(self):
>> +        return super().get_queryset() \
>> +            .annotate(submission_count=Count('submission'))
>> +
>> +
>> +class SubmissionRelationDetail(SubmissionRelationMixin,
>> +                               RetrieveUpdateDestroyAPIView):
>> +    pass
>> diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py
>> new file mode 100644
>> index 0000000..296926d
>> --- /dev/null
>> +++ b/patchwork/tests/api/test_relation.py
>> @@ -0,0 +1,194 @@
>> +# Patchwork - automated patch tracking system
>> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
>> +#
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +import unittest
>> +from enum import Enum
>> +from enum import auto
>> +
>> +import six
>> +from django.conf import settings
>> +from django.urls import reverse
>> +
>> +from patchwork.models import SubmissionRelation
>> +from patchwork.tests.api import utils
>> +from patchwork.tests.utils import create_maintainer
>> +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
>> +
>> +
>> +class UserType(Enum):
>> +    ANONYMOUS = auto()
>> +    NON_MAINTAINER = auto()
>> +    MAINTAINER = auto()
>> +
>> +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
>> +class TestRelationAPI(utils.APITestCase):
>> +    fixtures = ['default_tags']
>> +
>> +    @staticmethod
>> +    def api_url(item=None):
>> +        kwargs = {}
>> +        if item is None:
>> +            return reverse('api-relation-list', kwargs=kwargs)
>> +        kwargs['pk'] = item
>> +        return reverse('api-relation-detail', kwargs=kwargs)
>> +
>> +    def request_restricted(self, method, user_type: UserType):
>> +        # setup
>> +
>> +        project = create_project()
>> +
>> +        if user_type == UserType.ANONYMOUS:
>> +            expected_status = status.HTTP_403_FORBIDDEN
>> +        elif user_type == UserType.NON_MAINTAINER:
>> +            expected_status = status.HTTP_403_FORBIDDEN
>> +            self.client.force_authenticate(user=create_user())
>> +        elif user_type == UserType.MAINTAINER:
>> +            if method == 'post':
>> +                expected_status = status.HTTP_201_CREATED
>> +            elif method == 'delete':
>> +                expected_status = status.HTTP_204_NO_CONTENT
>> +            else:
>> +                expected_status = status.HTTP_200_OK
>> +            user = create_maintainer(project)
>> +            self.client.force_authenticate(user=user)
>> +        else:
>> +            raise ValueError
>> +
>> +        resource_id = None
>> +        send = None
>> +
>> +        if method == 'delete':
>> +            resource_id = create_relation(project=project).id
>> +        elif method == 'post':
>> +            patch_ids = [p.id for p in create_patches(2, project=project)]
>> +            send = {'submissions': patch_ids}
>> +        elif method == 'patch':
>> +            resource_id = create_relation(project=project).id
>> +            patch_ids = [p.id for p in create_patches(2, project=project)]
>> +            send = {'submissions': patch_ids}
>> +        else:
>> +            raise ValueError
>> +
>> +        # request
>> +
>> +        resp = getattr(self.client, method)(self.api_url(resource_id), send)
>> +
>> +        # check
>> +
>> +        self.assertEqual(expected_status, resp.status_code)
>> +
>> +        if resp.status_code not in range(200, 202):
>> +            return
>> +
>> +        if resource_id:
>> +            self.assertEqual(resource_id, resp.data['id'])
>> +
>> +        send_ids = send['submissions']
>> +        resp_ids = resp.data['submissions']
>> +        six.assertCountEqual(self, resp_ids, send_ids)
>> +
>> +    def assertSerialized(self, obj: SubmissionRelation, resp: dict):
>> +        self.assertEqual(obj.id, resp['id'])
>> +        obj = [s.id for s in obj.submissions.all()]
>> +        six.assertCountEqual(self, obj, resp['submissions'])
>> +
>> +    def test_list_empty(self):
>> +        """List relation when none are present."""
>> +        resp = self.client.get(self.api_url())
>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>> +        self.assertEqual(0, len(resp.data))
>> +
>> +    @utils.store_samples('relation-list')
>> +    def test_list(self):
>> +        """List relations."""
>> +        relation = create_relation()
>> +
>> +        resp = self.client.get(self.api_url())
>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>> +        self.assertEqual(1, len(resp.data))
>> +        self.assertSerialized(relation, resp.data[0])
>> +
>> +    def test_detail(self):
>> +        """Show relation."""
>> +        relation = create_relation()
>> +
>> +        resp = self.client.get(self.api_url(relation.id))
>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>> +        self.assertSerialized(relation, resp.data)
>> +
>> +    @utils.store_samples('relation-update-error-forbidden')
>> +    def test_update_anonymous(self):
>> +        """Update relation as anonymous user.
>> +
>> +        Ensure updates can be performed by maintainers.
>> +        """
>> +        self.request_restricted('patch', UserType.ANONYMOUS)
>> +
>> +    def test_update_non_maintainer(self):
>> +        """Update relation as non-maintainer.
>> +
>> +        Ensure updates can be performed by maintainers.
>> +        """
>> +        self.request_restricted('patch', UserType.NON_MAINTAINER)
>> +
>> +    @utils.store_samples('relation-update')
>> +    def test_update_maintainer(self):
>> +        """Update relation as maintainer.
>> +
>> +        Ensure updates can be performed by maintainers.
>> +        """
>> +        self.request_restricted('patch', UserType.MAINTAINER)
>> +
>> +    @utils.store_samples('relation-delete-error-forbidden')
>> +    def test_delete_anonymous(self):
>> +        """Delete relation as anonymous user.
>> +
>> +        Ensure deletes can be performed by maintainers.
>> +        """
>> +        self.request_restricted('delete', UserType.ANONYMOUS)
>> +
>> +    def test_delete_non_maintainer(self):
>> +        """Delete relation as non-maintainer.
>> +
>> +        Ensure deletes can be performed by maintainers.
>> +        """
>> +        self.request_restricted('delete', UserType.NON_MAINTAINER)
>> +
>> +    @utils.store_samples('relation-update')
>> +    def test_delete_maintainer(self):
>> +        """Delete relation as maintainer.
>> +
>> +        Ensure deletes can be performed by maintainers.
>> +        """
>> +        self.request_restricted('delete', UserType.MAINTAINER)
>> +
>> +    @utils.store_samples('relation-create-error-forbidden')
>> +    def test_create_anonymous(self):
>> +        """Create relation as anonymous user.
>> +
>> +        Ensure creates can be performed by maintainers.
>> +        """
>> +        self.request_restricted('post', UserType.ANONYMOUS)
>> +
>> +    def test_create_non_maintainer(self):
>> +        """Create relation as non-maintainer.
>> +
>> +        Ensure creates can be performed by maintainers.
>> +        """
>> +        self.request_restricted('post', UserType.NON_MAINTAINER)
>> +
>> +    @utils.store_samples('relation-create')
>> +    def test_create_maintainer(self):
>> +        """Create relation as maintainer.
>> +
>> +        Ensure creates can be performed by maintainers.
>> +        """
>> +        self.request_restricted('post', UserType.MAINTAINER)
>> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
>> index 577183d..47149de 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 SubmissionRelation
>>  from patchwork.models import Person
>>  from patchwork.models import Project
>>  from patchwork.models import Series
>> @@ -347,3 +348,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 = SubmissionRelation.objects.create()
>> +    values = {
>> +        'related': relation
>> +    }
>> +    values.update(kwargs)
>> +    create_patches(count_patches, **values)
>> +    return relation
> 
> 
> I haven't looked at the tests in great detail, but they look very
> comprehensive. You can get coverage data out of tox with -e coverage, so
> just check that you've covered a reasonably amount of the new code
> you're adding. (It doesn't necessarily have to be 100%.)
> 
> 
> Lastly, you should probably also add a field on Patch and CoverLetter
> that links to the related submissions if they exist. Otherwise I think
> you have to enumerate all the relations in the API to determine if there
> is one for the patch/cover letter that you're interested in?

I am not entirely sure but I will have a closer look at this.

> 
> Feel free to respin after you address these comments. Keep up the good
> work!
> 
> Regards,
> Daniel
>

Thank you for reviewing!

Best regards,

Mete

Mete Polat <metepolat2000@gmail.com> writes:

> Hi Daniel,
>
> (sorry for the short delay)
>
> On 06.11.19 16:12, Daniel Axtens wrote:
>> Mete Polat <metepolat2000@gmail.com> writes:
>> 
>>> View relations or add/update/delete them as a maintainer. Maintainers
>>> can only create relations of submissions (patches/cover letters) which
>>> are part of a project they maintain.
>>>
>>> New REST API urls:
>>> api/relations/
>>> api/relations/<relation_id>/
>>>
>>> Signed-off-by: Mete Polat <metepolat2000@gmail.com>
>>> ---
>>> Previously it was possible to use the PatchSerializer. As we expanded the
>>> support to submissions in general, it isn't a simple task anymore showing
>>> hyperlinked submissions (as Patch and CoverLetter are not flattened into
>>> one model yet). Right now only the submission ids are shown.
>> 
>> Hmm, that's unfortunate. I didn't intend to lead you in to this problem,
>> sorry! Having said that, it'd be good to supply some common information.
>> 
>> How about this, which is super-gross but which I think is probably the
>> best we can do unless Stephen can chime in with something better...
>> 
>> diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
>> index de4f31165ee7..8d44592b51f1 100644
>> --- a/patchwork/api/embedded.py
>> +++ b/patchwork/api/embedded.py
>> @@ -102,6 +102,45 @@ class CheckSerializer(SerializedRelatedField):
>>              }
>>  
>>  
>> +def _upgrade_instance(instance):
>> +    if hasattr(instance, 'patch'):
>> +        return instance.patch
>> +    else:
>> +        return instance.coverletter
>> +
>> +
>> +class SubmissionSerializer(SerializedRelatedField):
>> +
>> +    class _Serializer(BaseHyperlinkedModelSerializer):
>> +        """We need to 'upgrade' or specialise the submission to the relevant
>> +        subclass, so we can't use the mixins. This is gross but can go away
>> +        once we flatten the models."""
>> +        url = SerializerMethodField()
>> +        web_url = SerializerMethodField()
>> +        mbox = SerializerMethodField()
>> +
>> +        def get_url(self, instance):
>> +            instance = _upgrade_instance(instance)
>> +            request = self.context.get('request')
>> +            return request.build_absolute_uri(instance.get_absolute_url())
>> +
>> +        def get_web_url(self, instance):
>> +            instance = _upgrade_instance(instance)
>> +            request = self.context.get('request')
>> +            return request.build_absolute_uri(instance.get_absolute_url())
>> +
>
> Nice, thank you. I think this should be sufficient. Just want to note
> that get_url and get_web_url are showing the same url. I will change
> that in the next revision.
>
>> +        def get_mbox(self, instance):
>> +            instance = _upgrade_instance(instance)
>> +            request = self.context.get('request')
>> +            return request.build_absolute_uri(instance.get_mbox_url())
>> +
>> +        class Meta:
>> +            model = models.Submission
>> +            fields = ('id', 'url', 'web_url', 'msgid', 'list_archive_url',
>> +                      'date', 'name', 'mbox')
>> +            read_only_fields = fields
>> +
>> +
>>  class CoverLetterSerializer(SerializedRelatedField):
>>  
>>      class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
>> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
>> index e7d002b9375a..c02a6fe67e2c 100644
>> --- a/patchwork/api/relation.py
>> +++ b/patchwork/api/relation.py
>> @@ -9,6 +9,7 @@ from rest_framework.generics import ListCreateAPIView
>>  from rest_framework.generics import RetrieveUpdateDestroyAPIView
>>  from rest_framework.serializers import ModelSerializer
>>  
>> +from patchwork.api.embedded import SubmissionSerializer
>>  from patchwork.models import SubmissionRelation
>>  
>>  
>> @@ -34,6 +35,8 @@ class MaintainerPermission(permissions.BasePermission):
>>  
>>  
>>  class SubmissionRelationSerializer(ModelSerializer):
>> +    submissions = SubmissionSerializer(many=True)
>> +
>>      class Meta:
>>          model = SubmissionRelation
>>          fields = ('id', 'url', 'submissions',)
>> 
>> 
>>>
>>>  docs/api/schemas/latest/patchwork.yaml        | 218 +++++++++++++++++
>>>  docs/api/schemas/patchwork.j2                 | 230 ++++++++++++++++++
>>>  docs/api/schemas/v1.2/patchwork.yaml          | 218 +++++++++++++++++
>>>  patchwork/api/index.py                        |   1 +
>>>  patchwork/api/relation.py                     |  73 ++++++
>>>  patchwork/tests/api/test_relation.py          | 194 +++++++++++++++
>>>  patchwork/tests/utils.py                      |  11 +
>>>  patchwork/urls.py                             |  11 +
>>>  ...submission-relations-c96bb6c567b416d8.yaml |  10 +
>>>  9 files changed, 966 insertions(+)
>>> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
>>> new file mode 100644
>>> index 0000000..e7d002b
>>> --- /dev/null
>>> +++ b/patchwork/api/relation.py
>>> @@ -0,0 +1,73 @@
>>> +# Patchwork - automated patch tracking system
>>> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +from django.db.models import Count
>>> +from rest_framework import permissions
>>> +from rest_framework.generics import ListCreateAPIView
>>> +from rest_framework.generics import RetrieveUpdateDestroyAPIView
>>> +from rest_framework.serializers import ModelSerializer
>>> +
>>> +from patchwork.models import SubmissionRelation
>>> +
>>> +
>>> +class MaintainerPermission(permissions.BasePermission):
>>> +
>>> +    def has_object_permission(self, request, view, submissions):
>>> +        if request.method in permissions.SAFE_METHODS:
>>> +            return True
>>> +
>>> +        user = request.user
>>> +        if not user.is_authenticated:
>>> +            return False
>>> +
>>> +        if isinstance(submissions, SubmissionRelation):
>>> +            submissions = list(submissions.submissions.all())
>>> +        maintaining = user.profile.maintainer_projects.all()
>>> +        return all(s.project in maintaining for s in submissions)
>>> If I understand this correctly, you are saying that to have permissions,
>> for all patches, you must be a maintainer of that project.
>> 
>> That's correct, I think, but a comment spelling it out would be helpful:
>> perhaps it's because I don't do enough functional programming but it
>> took me a while to understand what you'd written.
>> 
>> Partially due to my lack of expertise with DRF, it wasn't clear to me
>> that this prevents the _creation_ (as opposed to modification) of a
>> relation where you're not the maintainer of all the involved projects,
>> but upon testing it would appear that it does, so that's good.
>> 
>>> +
>>> +    def has_permission(self, request, view):
>>> +        return request.method in permissions.SAFE_METHODS or \
>>> +               (request.user.is_authenticated and
>>> +                request.user.profile.maintainer_projects.count() > 0)
>>> +
>>> +
>>> +class SubmissionRelationSerializer(ModelSerializer):
>>> +    class Meta:
>>> +        model = SubmissionRelation
>>> +        fields = ('id', 'url', 'submissions',)
>>> +        read_only_fields = ('url',)
>>> +        extra_kwargs = {
>>> +            'url': {'view_name': 'api-relation-detail'},
>>> +        }
>>> +
>>> +
>>> +class SubmissionRelationMixin:
>>> +    serializer_class = SubmissionRelationSerializer
>>> +    permission_classes = (MaintainerPermission,)
>>> +
>>> +    def get_queryset(self):
>>> +        return SubmissionRelation.objects.all() \
>>> +            .prefetch_related('submissions')
>> 
>> So prefetch_related always makes me perk up my ears.
>> 
>> Here, we end up doing an individual database query for every submission
>> that is in a relation (you can observe this with the Django Debug
>> Toolbar). That's probably not ideal: you could get potentially a large
>> number of relations per page with a large number of patches per
>> relation.
>> > More interestingly, looking into it I think the way you've implemented
>> the model in patch 3 means that a patch can only have at most one
>> relation? Indeed, testing it shows that to be the case.
>> 
>> That's deeply counter-intuitive to me - shouldn't a patch be able to be
>> involved in multiple relations? Currently it can only be in one set of
>> relations (belong to one SubmissionRelation), it seems to me that it
>> ought to be able to be in multiple sets of relations.
>> 
>> I'm trying to think of a good example of where that makes sense, and I
>> think one is versions vs backports. So I say that v1 of patch A is
>> related to v2 and v3 of patch A, and it makes sense for that to be one
>> SubmissionRelation which v1, v2 and v3 all belong to. Then say that v3
>> is accepted, and then there's a stable backport of that to some old
>> trees. I would then say that v3 was related to the backport, but I'm not
>> sure I'd say that v1 was related to the backport in quite the same way.
>> 
>> So I would have thought there would be a many-to-many relationship
>> between submissions and relations. This would also facilitate relations
>> set by multiple tools where we want to be able to maintain some
>> separation.
>> 
>> Perhaps there's a good rationale for the current one-to-many
>> relationship, and I'm open to hearing it. If that's the case, then at the
>> very least, you shouldn't be able to silently _remove_ a patch from a
>> relation by adding it to another. You should be forced to first remove
>> the patch from the original relationship explictly, leaving it with
>> related=NULL, and then you can set another relationship.
>
> I think you are right that there are different types of relations
> however I am not sure how you exactly want to display them.
>
> When viewing a submission, how do we decide which SubmissionRelation to
> display under 'related'? Do we list the submissions of every
> SubmissionRelation a specific submission is part of? Do we list them
> separately or combined? Should users be able to mark a
> SubmissionRelation as something like "backport-relation" which can then
> be displayed in a separate row when viewing a submission?
>
> I think a more complex relation model could possibly lead to some
> confusion and unnecessary difficulties.
>
> Furthermore  I am not sure whether a more complex model really
> facilitates tools. As an example, PaStA, the analysis tool we are
> currently using for finding relations and commit_refs for a specific
> patch, does not differentiate between different types of relations.
>
> Nevertheless I am open to be convinced. I just don't want to complicate
> things if there is really no need.

Sorry I missed this - December was crazy with the non-patchwork parts of
my job.

My vision was to list them all, but I agree that figuring out how to do
that is a bit tricky...

>> Further to that, please could you add a field to SubmissionRelation with
>> some sort of comment/context/tool identification/etc so we can track
>> who's doing what. See e.g. the context field in the Check model.

I was hoping a context field would provide space for something like
"backports" or "PaStA-bot" so people could identify what was creating
the relation. That would then provide headings for the list.

But I agree it's complex, so let's get the simple one deployed in 2.2
and see what people do with it. It _may_ limit people to ~1 bot per
mailing list, but let's see what happens.

Again, apologies for dropping this on the floor.

Regards,
Daniel

>> 
>> 
>>> +
>>> +
>>> +class SubmissionRelationList(SubmissionRelationMixin, ListCreateAPIView):
>>> +    ordering = 'id'
>>> +    ordering_fields = ['id', 'submission_count']
>> 
>> What is the benefit of being able to order by submission_count?
>
> We can remove this. I just used it to see how the UI looks like for
> different relation counts.
>
>> It would be really nice to be able to filter by project, depending on
>> how feasible that is - it requires jumping though a JOIN which is
>> unpleasant...
>> 
>
> I will see how manageable this is.
>
>>> +
>>> +    def create(self, request, *args, **kwargs):
>>> +        serializer = self.get_serializer(data=request.data)
>>> +        serializer.is_valid(raise_exception=True)
>>> +        submissions = serializer.validated_data['submissions']
>>> +        self.check_object_permissions(request, submissions)
>>> +        return super().create(request, *args, **kwargs)
>>> +
>>> +    def get_queryset(self):
>>> +        return super().get_queryset() \
>>> +            .annotate(submission_count=Count('submission'))
>>> +
>>> +
>>> +class SubmissionRelationDetail(SubmissionRelationMixin,
>>> +                               RetrieveUpdateDestroyAPIView):
>>> +    pass
>>> diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py
>>> new file mode 100644
>>> index 0000000..296926d
>>> --- /dev/null
>>> +++ b/patchwork/tests/api/test_relation.py
>>> @@ -0,0 +1,194 @@
>>> +# Patchwork - automated patch tracking system
>>> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +import unittest
>>> +from enum import Enum
>>> +from enum import auto
>>> +
>>> +import six
>>> +from django.conf import settings
>>> +from django.urls import reverse
>>> +
>>> +from patchwork.models import SubmissionRelation
>>> +from patchwork.tests.api import utils
>>> +from patchwork.tests.utils import create_maintainer
>>> +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
>>> +
>>> +
>>> +class UserType(Enum):
>>> +    ANONYMOUS = auto()
>>> +    NON_MAINTAINER = auto()
>>> +    MAINTAINER = auto()
>>> +
>>> +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
>>> +class TestRelationAPI(utils.APITestCase):
>>> +    fixtures = ['default_tags']
>>> +
>>> +    @staticmethod
>>> +    def api_url(item=None):
>>> +        kwargs = {}
>>> +        if item is None:
>>> +            return reverse('api-relation-list', kwargs=kwargs)
>>> +        kwargs['pk'] = item
>>> +        return reverse('api-relation-detail', kwargs=kwargs)
>>> +
>>> +    def request_restricted(self, method, user_type: UserType):
>>> +        # setup
>>> +
>>> +        project = create_project()
>>> +
>>> +        if user_type == UserType.ANONYMOUS:
>>> +            expected_status = status.HTTP_403_FORBIDDEN
>>> +        elif user_type == UserType.NON_MAINTAINER:
>>> +            expected_status = status.HTTP_403_FORBIDDEN
>>> +            self.client.force_authenticate(user=create_user())
>>> +        elif user_type == UserType.MAINTAINER:
>>> +            if method == 'post':
>>> +                expected_status = status.HTTP_201_CREATED
>>> +            elif method == 'delete':
>>> +                expected_status = status.HTTP_204_NO_CONTENT
>>> +            else:
>>> +                expected_status = status.HTTP_200_OK
>>> +            user = create_maintainer(project)
>>> +            self.client.force_authenticate(user=user)
>>> +        else:
>>> +            raise ValueError
>>> +
>>> +        resource_id = None
>>> +        send = None
>>> +
>>> +        if method == 'delete':
>>> +            resource_id = create_relation(project=project).id
>>> +        elif method == 'post':
>>> +            patch_ids = [p.id for p in create_patches(2, project=project)]
>>> +            send = {'submissions': patch_ids}
>>> +        elif method == 'patch':
>>> +            resource_id = create_relation(project=project).id
>>> +            patch_ids = [p.id for p in create_patches(2, project=project)]
>>> +            send = {'submissions': patch_ids}
>>> +        else:
>>> +            raise ValueError
>>> +
>>> +        # request
>>> +
>>> +        resp = getattr(self.client, method)(self.api_url(resource_id), send)
>>> +
>>> +        # check
>>> +
>>> +        self.assertEqual(expected_status, resp.status_code)
>>> +
>>> +        if resp.status_code not in range(200, 202):
>>> +            return
>>> +
>>> +        if resource_id:
>>> +            self.assertEqual(resource_id, resp.data['id'])
>>> +
>>> +        send_ids = send['submissions']
>>> +        resp_ids = resp.data['submissions']
>>> +        six.assertCountEqual(self, resp_ids, send_ids)
>>> +
>>> +    def assertSerialized(self, obj: SubmissionRelation, resp: dict):
>>> +        self.assertEqual(obj.id, resp['id'])
>>> +        obj = [s.id for s in obj.submissions.all()]
>>> +        six.assertCountEqual(self, obj, resp['submissions'])
>>> +
>>> +    def test_list_empty(self):
>>> +        """List relation when none are present."""
>>> +        resp = self.client.get(self.api_url())
>>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>>> +        self.assertEqual(0, len(resp.data))
>>> +
>>> +    @utils.store_samples('relation-list')
>>> +    def test_list(self):
>>> +        """List relations."""
>>> +        relation = create_relation()
>>> +
>>> +        resp = self.client.get(self.api_url())
>>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>>> +        self.assertEqual(1, len(resp.data))
>>> +        self.assertSerialized(relation, resp.data[0])
>>> +
>>> +    def test_detail(self):
>>> +        """Show relation."""
>>> +        relation = create_relation()
>>> +
>>> +        resp = self.client.get(self.api_url(relation.id))
>>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>>> +        self.assertSerialized(relation, resp.data)
>>> +
>>> +    @utils.store_samples('relation-update-error-forbidden')
>>> +    def test_update_anonymous(self):
>>> +        """Update relation as anonymous user.
>>> +
>>> +        Ensure updates can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('patch', UserType.ANONYMOUS)
>>> +
>>> +    def test_update_non_maintainer(self):
>>> +        """Update relation as non-maintainer.
>>> +
>>> +        Ensure updates can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('patch', UserType.NON_MAINTAINER)
>>> +
>>> +    @utils.store_samples('relation-update')
>>> +    def test_update_maintainer(self):
>>> +        """Update relation as maintainer.
>>> +
>>> +        Ensure updates can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('patch', UserType.MAINTAINER)
>>> +
>>> +    @utils.store_samples('relation-delete-error-forbidden')
>>> +    def test_delete_anonymous(self):
>>> +        """Delete relation as anonymous user.
>>> +
>>> +        Ensure deletes can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('delete', UserType.ANONYMOUS)
>>> +
>>> +    def test_delete_non_maintainer(self):
>>> +        """Delete relation as non-maintainer.
>>> +
>>> +        Ensure deletes can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('delete', UserType.NON_MAINTAINER)
>>> +
>>> +    @utils.store_samples('relation-update')
>>> +    def test_delete_maintainer(self):
>>> +        """Delete relation as maintainer.
>>> +
>>> +        Ensure deletes can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('delete', UserType.MAINTAINER)
>>> +
>>> +    @utils.store_samples('relation-create-error-forbidden')
>>> +    def test_create_anonymous(self):
>>> +        """Create relation as anonymous user.
>>> +
>>> +        Ensure creates can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('post', UserType.ANONYMOUS)
>>> +
>>> +    def test_create_non_maintainer(self):
>>> +        """Create relation as non-maintainer.
>>> +
>>> +        Ensure creates can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('post', UserType.NON_MAINTAINER)
>>> +
>>> +    @utils.store_samples('relation-create')
>>> +    def test_create_maintainer(self):
>>> +        """Create relation as maintainer.
>>> +
>>> +        Ensure creates can be performed by maintainers.
>>> +        """
>>> +        self.request_restricted('post', UserType.MAINTAINER)
>>> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
>>> index 577183d..47149de 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 SubmissionRelation
>>>  from patchwork.models import Person
>>>  from patchwork.models import Project
>>>  from patchwork.models import Series
>>> @@ -347,3 +348,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 = SubmissionRelation.objects.create()
>>> +    values = {
>>> +        'related': relation
>>> +    }
>>> +    values.update(kwargs)
>>> +    create_patches(count_patches, **values)
>>> +    return relation
>> 
>> 
>> I haven't looked at the tests in great detail, but they look very
>> comprehensive. You can get coverage data out of tox with -e coverage, so
>> just check that you've covered a reasonably amount of the new code
>> you're adding. (It doesn't necessarily have to be 100%.)
>> 
>> 
>> Lastly, you should probably also add a field on Patch and CoverLetter
>> that links to the related submissions if they exist. Otherwise I think
>> you have to enumerate all the relations in the API to determine if there
>> is one for the patch/cover letter that you're interested in?
>
> I am not entirely sure but I will have a closer look at this.
>
>> 
>> Feel free to respin after you address these comments. Keep up the good
>> work!
>> 
>> Regards,
>> Daniel
>>
>
> Thank you for reviewing!
>
> Best regards,
>
> Mete

On Sa., 11. Jan. 2020 at 12:54, Daniel Axtens <dja@axtens.net> wrote:

> Mete Polat <metepolat2000@gmail.com> writes:
>
> > Hi Daniel,
> >
> > (sorry for the short delay)
> >
> > On 06.11.19 16:12, Daniel Axtens wrote:
> >> Mete Polat <metepolat2000@gmail.com> writes:
> >>
> >>> View relations or add/update/delete them as a maintainer. Maintainers
> >>> can only create relations of submissions (patches/cover letters) which
> >>> are part of a project they maintain.
> >>>
> >>> New REST API urls:
> >>> api/relations/
> >>> api/relations/<relation_id>/
> >>>
> >>> Signed-off-by: Mete Polat <metepolat2000@gmail.com>
> >>> ---
> >>> Previously it was possible to use the PatchSerializer. As we expanded
> the
> >>> support to submissions in general, it isn't a simple task anymore
> showing
> >>> hyperlinked submissions (as Patch and CoverLetter are not flattened
> into
> >>> one model yet). Right now only the submission ids are shown.
> >>
> >> Hmm, that's unfortunate. I didn't intend to lead you in to this problem,
> >> sorry! Having said that, it'd be good to supply some common information.
> >>
> >> How about this, which is super-gross but which I think is probably the
> >> best we can do unless Stephen can chime in with something better...
> >>
> >> diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
> >> index de4f31165ee7..8d44592b51f1 100644
> >> --- a/patchwork/api/embedded.py
> >> +++ b/patchwork/api/embedded.py
> >> @@ -102,6 +102,45 @@ class CheckSerializer(SerializedRelatedField):
> >>              }
> >>
> >>
> >> +def _upgrade_instance(instance):
> >> +    if hasattr(instance, 'patch'):
> >> +        return instance.patch
> >> +    else:
> >> +        return instance.coverletter
> >> +
> >> +
> >> +class SubmissionSerializer(SerializedRelatedField):
> >> +
> >> +    class _Serializer(BaseHyperlinkedModelSerializer):
> >> +        """We need to 'upgrade' or specialise the submission to the
> relevant
> >> +        subclass, so we can't use the mixins. This is gross but can go
> away
> >> +        once we flatten the models."""
> >> +        url = SerializerMethodField()
> >> +        web_url = SerializerMethodField()
> >> +        mbox = SerializerMethodField()
> >> +
> >> +        def get_url(self, instance):
> >> +            instance = _upgrade_instance(instance)
> >> +            request = self.context.get('request')
> >> +            return
> request.build_absolute_uri(instance.get_absolute_url())
> >> +
> >> +        def get_web_url(self, instance):
> >> +            instance = _upgrade_instance(instance)
> >> +            request = self.context.get('request')
> >> +            return
> request.build_absolute_uri(instance.get_absolute_url())
> >> +
> >
> > Nice, thank you. I think this should be sufficient. Just want to note
> > that get_url and get_web_url are showing the same url. I will change
> > that in the next revision.
> >
> >> +        def get_mbox(self, instance):
> >> +            instance = _upgrade_instance(instance)
> >> +            request = self.context.get('request')
> >> +            return request.build_absolute_uri(instance.get_mbox_url())
> >> +
> >> +        class Meta:
> >> +            model = models.Submission
> >> +            fields = ('id', 'url', 'web_url', 'msgid',
> 'list_archive_url',
> >> +                      'date', 'name', 'mbox')
> >> +            read_only_fields = fields
> >> +
> >> +
> >>  class CoverLetterSerializer(SerializedRelatedField):
> >>
> >>      class _Serializer(MboxMixin, WebURLMixin,
> BaseHyperlinkedModelSerializer):
> >> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
> >> index e7d002b9375a..c02a6fe67e2c 100644
> >> --- a/patchwork/api/relation.py
> >> +++ b/patchwork/api/relation.py
> >> @@ -9,6 +9,7 @@ from rest_framework.generics import ListCreateAPIView
> >>  from rest_framework.generics import RetrieveUpdateDestroyAPIView
> >>  from rest_framework.serializers import ModelSerializer
> >>
> >> +from patchwork.api.embedded import SubmissionSerializer
> >>  from patchwork.models import SubmissionRelation
> >>
> >>
> >> @@ -34,6 +35,8 @@ class
> MaintainerPermission(permissions.BasePermission):
> >>
> >>
> >>  class SubmissionRelationSerializer(ModelSerializer):
> >> +    submissions = SubmissionSerializer(many=True)
> >> +
> >>      class Meta:
> >>          model = SubmissionRelation
> >>          fields = ('id', 'url', 'submissions',)
> >>
> >>
> >>>
> >>>  docs/api/schemas/latest/patchwork.yaml        | 218 +++++++++++++++++
> >>>  docs/api/schemas/patchwork.j2                 | 230 ++++++++++++++++++
> >>>  docs/api/schemas/v1.2/patchwork.yaml          | 218 +++++++++++++++++
> >>>  patchwork/api/index.py                        |   1 +
> >>>  patchwork/api/relation.py                     |  73 ++++++
> >>>  patchwork/tests/api/test_relation.py          | 194 +++++++++++++++
> >>>  patchwork/tests/utils.py                      |  11 +
> >>>  patchwork/urls.py                             |  11 +
> >>>  ...submission-relations-c96bb6c567b416d8.yaml |  10 +
> >>>  9 files changed, 966 insertions(+)
> >>> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
> >>> new file mode 100644
> >>> index 0000000..e7d002b
> >>> --- /dev/null
> >>> +++ b/patchwork/api/relation.py
> >>> @@ -0,0 +1,73 @@
> >>> +# Patchwork - automated patch tracking system
> >>> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft
> (BMW AG)
> >>> +#
> >>> +# SPDX-License-Identifier: GPL-2.0-or-later
> >>> +
> >>> +from django.db.models import Count
> >>> +from rest_framework import permissions
> >>> +from rest_framework.generics import ListCreateAPIView
> >>> +from rest_framework.generics import RetrieveUpdateDestroyAPIView
> >>> +from rest_framework.serializers import ModelSerializer
> >>> +
> >>> +from patchwork.models import SubmissionRelation
> >>> +
> >>> +
> >>> +class MaintainerPermission(permissions.BasePermission):
> >>> +
> >>> +    def has_object_permission(self, request, view, submissions):
> >>> +        if request.method in permissions.SAFE_METHODS:
> >>> +            return True
> >>> +
> >>> +        user = request.user
> >>> +        if not user.is_authenticated:
> >>> +            return False
> >>> +
> >>> +        if isinstance(submissions, SubmissionRelation):
> >>> +            submissions = list(submissions.submissions.all())
> >>> +        maintaining = user.profile.maintainer_projects.all()
> >>> +        return all(s.project in maintaining for s in submissions)
> >>> If I understand this correctly, you are saying that to have
> permissions,
> >> for all patches, you must be a maintainer of that project.
> >>
> >> That's correct, I think, but a comment spelling it out would be helpful:
> >> perhaps it's because I don't do enough functional programming but it
> >> took me a while to understand what you'd written.
> >>
> >> Partially due to my lack of expertise with DRF, it wasn't clear to me
> >> that this prevents the _creation_ (as opposed to modification) of a
> >> relation where you're not the maintainer of all the involved projects,
> >> but upon testing it would appear that it does, so that's good.
> >>
> >>> +
> >>> +    def has_permission(self, request, view):
> >>> +        return request.method in permissions.SAFE_METHODS or \
> >>> +               (request.user.is_authenticated and
> >>> +                request.user.profile.maintainer_projects.count() > 0)
> >>> +
> >>> +
> >>> +class SubmissionRelationSerializer(ModelSerializer):
> >>> +    class Meta:
> >>> +        model = SubmissionRelation
> >>> +        fields = ('id', 'url', 'submissions',)
> >>> +        read_only_fields = ('url',)
> >>> +        extra_kwargs = {
> >>> +            'url': {'view_name': 'api-relation-detail'},
> >>> +        }
> >>> +
> >>> +
> >>> +class SubmissionRelationMixin:
> >>> +    serializer_class = SubmissionRelationSerializer
> >>> +    permission_classes = (MaintainerPermission,)
> >>> +
> >>> +    def get_queryset(self):
> >>> +        return SubmissionRelation.objects.all() \
> >>> +            .prefetch_related('submissions')
> >>
> >> So prefetch_related always makes me perk up my ears.
> >>
> >> Here, we end up doing an individual database query for every submission
> >> that is in a relation (you can observe this with the Django Debug
> >> Toolbar). That's probably not ideal: you could get potentially a large
> >> number of relations per page with a large number of patches per
> >> relation.
> >> > More interestingly, looking into it I think the way you've implemented
> >> the model in patch 3 means that a patch can only have at most one
> >> relation? Indeed, testing it shows that to be the case.
> >>
> >> That's deeply counter-intuitive to me - shouldn't a patch be able to be
> >> involved in multiple relations? Currently it can only be in one set of
> >> relations (belong to one SubmissionRelation), it seems to me that it
> >> ought to be able to be in multiple sets of relations.
> >>
> >> I'm trying to think of a good example of where that makes sense, and I
> >> think one is versions vs backports. So I say that v1 of patch A is
> >> related to v2 and v3 of patch A, and it makes sense for that to be one
> >> SubmissionRelation which v1, v2 and v3 all belong to. Then say that v3
> >> is accepted, and then there's a stable backport of that to some old
> >> trees. I would then say that v3 was related to the backport, but I'm not
> >> sure I'd say that v1 was related to the backport in quite the same way.
> >>
> >> So I would have thought there would be a many-to-many relationship
> >> between submissions and relations. This would also facilitate relations
> >> set by multiple tools where we want to be able to maintain some
> >> separation.
> >>
> >> Perhaps there's a good rationale for the current one-to-many
> >> relationship, and I'm open to hearing it. If that's the case, then at
> the
> >> very least, you shouldn't be able to silently _remove_ a patch from a
> >> relation by adding it to another. You should be forced to first remove
> >> the patch from the original relationship explictly, leaving it with
> >> related=NULL, and then you can set another relationship.
> >
> > I think you are right that there are different types of relations
> > however I am not sure how you exactly want to display them.
> >
> > When viewing a submission, how do we decide which SubmissionRelation to
> > display under 'related'? Do we list the submissions of every
> > SubmissionRelation a specific submission is part of? Do we list them
> > separately or combined? Should users be able to mark a
> > SubmissionRelation as something like "backport-relation" which can then
> > be displayed in a separate row when viewing a submission?
> >
> > I think a more complex relation model could possibly lead to some
> > confusion and unnecessary difficulties.
> >
> > Furthermore  I am not sure whether a more complex model really
> > facilitates tools. As an example, PaStA, the analysis tool we are
> > currently using for finding relations and commit_refs for a specific
> > patch, does not differentiate between different types of relations.
> >
> > Nevertheless I am open to be convinced. I just don't want to complicate
> > things if there is really no need.
>
> Sorry I missed this - December was crazy with the non-patchwork parts of
> my job.
>
> My vision was to list them all, but I agree that figuring out how to do
> that is a bit tricky...
>
> >> Further to that, please could you add a field to SubmissionRelation with
> >> some sort of comment/context/tool identification/etc so we can track
> >> who's doing what. See e.g. the context field in the Check model.
>
> I was hoping a context field would provide space for something like
> "backports" or "PaStA-bot" so people could identify what was creating
> the relation. That would then provide headings for the list.
>
> But I agree it's complex, so let's get the simple one deployed in 2.2
> and see what people do with it. It _may_ limit people to ~1 bot per
> mailing list, but let's see what happens.
>
> Again, apologies for dropping this on the floor.
>

We share the same vision. We need a first few implementations of
scripts/heuristics that make use of this feature, though. Then, we will see
how users and projects really want to use this, and what we can implement
to support those use cases.

Lukas


> Regards,
> Daniel
>
> >>
> >>
> >>> +
> >>> +
> >>> +class SubmissionRelationList(SubmissionRelationMixin,
> ListCreateAPIView):
> >>> +    ordering = 'id'
> >>> +    ordering_fields = ['id', 'submission_count']
> >>
> >> What is the benefit of being able to order by submission_count?
> >
> > We can remove this. I just used it to see how the UI looks like for
> > different relation counts.
> >
> >> It would be really nice to be able to filter by project, depending on
> >> how feasible that is - it requires jumping though a JOIN which is
> >> unpleasant...
> >>
> >
> > I will see how manageable this is.
> >
> >>> +
> >>> +    def create(self, request, *args, **kwargs):
> >>> +        serializer = self.get_serializer(data=request.data)
> >>> +        serializer.is_valid(raise_exception=True)
> >>> +        submissions = serializer.validated_data['submissions']
> >>> +        self.check_object_permissions(request, submissions)
> >>> +        return super().create(request, *args, **kwargs)
> >>> +
> >>> +    def get_queryset(self):
> >>> +        return super().get_queryset() \
> >>> +            .annotate(submission_count=Count('submission'))
> >>> +
> >>> +
> >>> +class SubmissionRelationDetail(SubmissionRelationMixin,
> >>> +                               RetrieveUpdateDestroyAPIView):
> >>> +    pass
> >>> diff --git a/patchwork/tests/api/test_relation.py
> b/patchwork/tests/api/test_relation.py
> >>> new file mode 100644
> >>> index 0000000..296926d
> >>> --- /dev/null
> >>> +++ b/patchwork/tests/api/test_relation.py
> >>> @@ -0,0 +1,194 @@
> >>> +# Patchwork - automated patch tracking system
> >>> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft
> (BMW AG)
> >>> +#
> >>> +# SPDX-License-Identifier: GPL-2.0-or-later
> >>> +
> >>> +import unittest
> >>> +from enum import Enum
> >>> +from enum import auto
> >>> +
> >>> +import six
> >>> +from django.conf import settings
> >>> +from django.urls import reverse
> >>> +
> >>> +from patchwork.models import SubmissionRelation
> >>> +from patchwork.tests.api import utils
> >>> +from patchwork.tests.utils import create_maintainer
> >>> +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
> >>> +
> >>> +
> >>> +class UserType(Enum):
> >>> +    ANONYMOUS = auto()
> >>> +    NON_MAINTAINER = auto()
> >>> +    MAINTAINER = auto()
> >>> +
> >>> +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires
> ENABLE_REST_API')
> >>> +class TestRelationAPI(utils.APITestCase):
> >>> +    fixtures = ['default_tags']
> >>> +
> >>> +    @staticmethod
> >>> +    def api_url(item=None):
> >>> +        kwargs = {}
> >>> +        if item is None:
> >>> +            return reverse('api-relation-list', kwargs=kwargs)
> >>> +        kwargs['pk'] = item
> >>> +        return reverse('api-relation-detail', kwargs=kwargs)
> >>> +
> >>> +    def request_restricted(self, method, user_type: UserType):
> >>> +        # setup
> >>> +
> >>> +        project = create_project()
> >>> +
> >>> +        if user_type == UserType.ANONYMOUS:
> >>> +            expected_status = status.HTTP_403_FORBIDDEN
> >>> +        elif user_type == UserType.NON_MAINTAINER:
> >>> +            expected_status = status.HTTP_403_FORBIDDEN
> >>> +            self.client.force_authenticate(user=create_user())
> >>> +        elif user_type == UserType.MAINTAINER:
> >>> +            if method == 'post':
> >>> +                expected_status = status.HTTP_201_CREATED
> >>> +            elif method == 'delete':
> >>> +                expected_status = status.HTTP_204_NO_CONTENT
> >>> +            else:
> >>> +                expected_status = status.HTTP_200_OK
> >>> +            user = create_maintainer(project)
> >>> +            self.client.force_authenticate(user=user)
> >>> +        else:
> >>> +            raise ValueError
> >>> +
> >>> +        resource_id = None
> >>> +        send = None
> >>> +
> >>> +        if method == 'delete':
> >>> +            resource_id = create_relation(project=project).id
> >>> +        elif method == 'post':
> >>> +            patch_ids = [p.id for p in create_patches(2,
> project=project)]
> >>> +            send = {'submissions': patch_ids}
> >>> +        elif method == 'patch':
> >>> +            resource_id = create_relation(project=project).id
> >>> +            patch_ids = [p.id for p in create_patches(2,
> project=project)]
> >>> +            send = {'submissions': patch_ids}
> >>> +        else:
> >>> +            raise ValueError
> >>> +
> >>> +        # request
> >>> +
> >>> +        resp = getattr(self.client,
> method)(self.api_url(resource_id), send)
> >>> +
> >>> +        # check
> >>> +
> >>> +        self.assertEqual(expected_status, resp.status_code)
> >>> +
> >>> +        if resp.status_code not in range(200, 202):
> >>> +            return
> >>> +
> >>> +        if resource_id:
> >>> +            self.assertEqual(resource_id, resp.data['id'])
> >>> +
> >>> +        send_ids = send['submissions']
> >>> +        resp_ids = resp.data['submissions']
> >>> +        six.assertCountEqual(self, resp_ids, send_ids)
> >>> +
> >>> +    def assertSerialized(self, obj: SubmissionRelation, resp: dict):
> >>> +        self.assertEqual(obj.id, resp['id'])
> >>> +        obj = [s.id for s in obj.submissions.all()]
> >>> +        six.assertCountEqual(self, obj, resp['submissions'])
> >>> +
> >>> +    def test_list_empty(self):
> >>> +        """List relation when none are present."""
> >>> +        resp = self.client.get(self.api_url())
> >>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> >>> +        self.assertEqual(0, len(resp.data))
> >>> +
> >>> +    @utils.store_samples('relation-list')
> >>> +    def test_list(self):
> >>> +        """List relations."""
> >>> +        relation = create_relation()
> >>> +
> >>> +        resp = self.client.get(self.api_url())
> >>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> >>> +        self.assertEqual(1, len(resp.data))
> >>> +        self.assertSerialized(relation, resp.data[0])
> >>> +
> >>> +    def test_detail(self):
> >>> +        """Show relation."""
> >>> +        relation = create_relation()
> >>> +
> >>> +        resp = self.client.get(self.api_url(relation.id))
> >>> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> >>> +        self.assertSerialized(relation, resp.data)
> >>> +
> >>> +    @utils.store_samples('relation-update-error-forbidden')
> >>> +    def test_update_anonymous(self):
> >>> +        """Update relation as anonymous user.
> >>> +
> >>> +        Ensure updates can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('patch', UserType.ANONYMOUS)
> >>> +
> >>> +    def test_update_non_maintainer(self):
> >>> +        """Update relation as non-maintainer.
> >>> +
> >>> +        Ensure updates can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('patch', UserType.NON_MAINTAINER)
> >>> +
> >>> +    @utils.store_samples('relation-update')
> >>> +    def test_update_maintainer(self):
> >>> +        """Update relation as maintainer.
> >>> +
> >>> +        Ensure updates can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('patch', UserType.MAINTAINER)
> >>> +
> >>> +    @utils.store_samples('relation-delete-error-forbidden')
> >>> +    def test_delete_anonymous(self):
> >>> +        """Delete relation as anonymous user.
> >>> +
> >>> +        Ensure deletes can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('delete', UserType.ANONYMOUS)
> >>> +
> >>> +    def test_delete_non_maintainer(self):
> >>> +        """Delete relation as non-maintainer.
> >>> +
> >>> +        Ensure deletes can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('delete', UserType.NON_MAINTAINER)
> >>> +
> >>> +    @utils.store_samples('relation-update')
> >>> +    def test_delete_maintainer(self):
> >>> +        """Delete relation as maintainer.
> >>> +
> >>> +        Ensure deletes can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('delete', UserType.MAINTAINER)
> >>> +
> >>> +    @utils.store_samples('relation-create-error-forbidden')
> >>> +    def test_create_anonymous(self):
> >>> +        """Create relation as anonymous user.
> >>> +
> >>> +        Ensure creates can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('post', UserType.ANONYMOUS)
> >>> +
> >>> +    def test_create_non_maintainer(self):
> >>> +        """Create relation as non-maintainer.
> >>> +
> >>> +        Ensure creates can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('post', UserType.NON_MAINTAINER)
> >>> +
> >>> +    @utils.store_samples('relation-create')
> >>> +    def test_create_maintainer(self):
> >>> +        """Create relation as maintainer.
> >>> +
> >>> +        Ensure creates can be performed by maintainers.
> >>> +        """
> >>> +        self.request_restricted('post', UserType.MAINTAINER)
> >>> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> >>> index 577183d..47149de 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 SubmissionRelation
> >>>  from patchwork.models import Person
> >>>  from patchwork.models import Project
> >>>  from patchwork.models import Series
> >>> @@ -347,3 +348,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 = SubmissionRelation.objects.create()
> >>> +    values = {
> >>> +        'related': relation
> >>> +    }
> >>> +    values.update(kwargs)
> >>> +    create_patches(count_patches, **values)
> >>> +    return relation
> >>
> >>
> >> I haven't looked at the tests in great detail, but they look very
> >> comprehensive. You can get coverage data out of tox with -e coverage, so
> >> just check that you've covered a reasonably amount of the new code
> >> you're adding. (It doesn't necessarily have to be 100%.)
> >>
> >>
> >> Lastly, you should probably also add a field on Patch and CoverLetter
> >> that links to the related submissions if they exist. Otherwise I think
> >> you have to enumerate all the relations in the API to determine if there
> >> is one for the patch/cover letter that you're interested in?
> >
> > I am not entirely sure but I will have a closer look at this.
> >
> >>
> >> Feel free to respin after you address these comments. Keep up the good
> >> work!
> >>
> >> Regards,
> >> Daniel
> >>
> >
> > Thank you for reviewing!
> >
> > Best regards,
> >
> > Mete
>