[PATCH 0/3] Add patch relations

[Daniel Axtens]

Hi Mete,

> From: Mete Polat <metepolat2000 at gmail.com>
>
> This patch introduces the ability to view relations between patches by
> creating and updating patch relations via the REST API. Setting
> relations allows users to browse related patches like other revisions of
> the same patch.

I talked to Lukas about this at Linux Plumbers. As I said then, I'm OK
with the general idea of adding a related patches model to the database,
and I think that's worth attempting as a way to identify things like
series revisions. There's a lot of talk about this at both Plumbers and
the Maintainers Summit, so you've picked a good time to be working on it! :)


I have a few medium-sized questions and things I'd like you to change
please.

 1) I was a bit concerned about you being the first to use granular
    permissions, as I had wanted to be able to introduce them all at
    once rather than just for this. (In part because I'd like
    maintainers to be able to set them, not just people with access to
    the django admin console, and that requires some UI work.) However,
    as setting relations cuts across projects, I'm not sure I see a
    better way - maintainers can't grant rights outside their project
    anyway.

    Maybe it's worth allowing a maintainer to set relations within their
    project only, so that this can be rolled out on a per-list basis to
    begin with, rather than starting with instance wide. How tricky
    would that be to do?

 2) What's the utility of allowing the API caller to be able to set the
    ID of the relation? Is it just for allowing bulk operations, or are
    you hoping that the ID will be somehow meaningful?

    I'm not sure about the merits of bulk operations other than perhaps
    bulk creation, but I'm open to being convinced. (I am particularly
    nervous about people accidentally bulk-updating over data they
    didn't intend to, especially in the potential presence of multiple
    authorised users.) I am very dubious about having meaningful
    numerical IDs.

 3) Your model only does patch relations. Is there a concept of a
    cover-letter relation? Would it be better to tie into Submission
    rather than Patch? (noting also that the Patch model and the
    CoverLetter model are getting flattened into the one Submission
    model in a future patchwork version)

 4) You support and display both related patches within a project and
    related patches across projects. That's fine, but I'm a bit worried
    that if you consider something like the lockdown series for the
    linux kernel that hit version 40 and was sent to at least 3 lists,
    the display might get a bit unmanageable. I think users are probably
    most interested in relations with a project, maybe that should be
    the default and a separate click expands to relations across
    projects... It's not a deal-breaker, I'm just trying to think about
    the future uses.

On the more minor front:

 1) Please could you split up your UI changes. You do 2 things - you
    change how the existing series display shows, and you add a related
    display. I'm not sure that I like the changes to the series display,
    but I think I like the related patches display.

 2) If I call PUT with no IDs, it 500s rather than throwing a good
    error:

http -v --json PUT http://localhost:8000/api/1.2/relations/ Authorization:"Token e18a9f836bde4e2fec3f1c6199b389c3dc1db57e"|head -100
...
File "/home/patchwork/patchwork/patchwork/api/relation.py" in put
  71.             if queryset.filter(pk=d['id']).exists():


 3) Your series no longer quite works with the change to message-id
    based URLs. In particular the submission template needs this fix:

--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -123,7 +123,7 @@ function toggle_div(link_id, headers_id)
     {% for sibling in submission.related.patches.all %}
      <li>
       {% if sibling != submission %}
-       <a href="{% url 'patch-detail' patch_id=sibling.id %}">
+       <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}">
          {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
        </a>
        {% if sibling.project != submission.project %}

Beyond that, thank you very much for including tests, and I think this
is a very good start.

Regards,
Daniel


>
> Mete Polat (3):
>   ui: Retain table header position on size changes
>   models, templates: Add relations between patches
>   REST: Add patch relations
>
>  docs/api/schemas/latest/patchwork.yaml        | 149 +++++++++++++++++
>  docs/api/schemas/patchwork.j2                 | 157 ++++++++++++++++++
>  docs/api/schemas/v1.2/patchwork.yaml          | 149 +++++++++++++++++
>  htdocs/css/style.css                          |   4 +-
>  patchwork/api/index.py                        |   1 +
>  patchwork/api/relation.py                     |  95 +++++++++++
>  patchwork/migrations/0037_patch_relations.py  |  28 ++++
>  patchwork/models.py                           |  13 ++
>  patchwork/templates/patchwork/submission.html |  47 ++++--
>  patchwork/tests/api/test_relation.py          | 154 +++++++++++++++++
>  patchwork/tests/utils.py                      |  24 ++-
>  patchwork/urls.py                             |  11 ++
>  .../add-patch-relations-c96bb6c567b416d8.yaml |   9 +
>  requirements-dev.txt                          |   1 +
>  requirements-prod.txt                         |   1 +
>  tox.ini                                       |   3 +-
>  16 files changed, 830 insertions(+), 16 deletions(-)
>  create mode 100644 patchwork/api/relation.py
>  create mode 100644 patchwork/migrations/0037_patch_relations.py
>  create mode 100644 patchwork/tests/api/test_relation.py
>  create mode 100644 releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml
>
> -- 
> 2.20.1 (Apple Git-117)
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork