Hi Mete,
> From: Mete Polat <metepolat2...@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
