[PATCH 0/3] Add patch relations
Mete Polat
metepolat2000 at gmail.com
Wed Oct 2 20:34:28 AEST 2019
Hi Daniel,
> > 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 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?
I think that should be possible. I designed that patch in a way that it
strongly favours setting relations by automation tools and that there is
just one single person managing them. But of course there is valid use
case for allowing maintainers maintain their own list and relations.
> 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?
It's easier for tools managing the relation ids on their own, but I will
change that. I won't make any sense with multiple users able to change
relations.
> 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.
I see your point but I think we are on the save side here. Maintainers
will probably use the JSON textbox in patchwork's browsable API for
updating multiple relations. The textbox's default entry is a JSON object.
I think everyone who replaces this object with multiple objects within a
JSON array (explicitly has to add squared brackets) understands the
consequences.
There is a good chance when automating things that we have to update
multiple patch relations. For example when patches in a series are
individualy related to patches of a previous revision or when there is
just a high volume mailing list in general. In these cases bulk-updates
increase efficiency.
> 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)
Good point. I will change that.
> 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.
Yes that's a good idea.
> 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.
Done.
> 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():
>
Good catch, thank you.
> 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 %}
Aaah I was rebasing just some hours before that got into the repo. Thanks
for pointing this out.
Best regards,
Mete
> >
> > 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
More information about the Patchwork
mailing list