[Patchew-devel] [PATCH v2 00/12] Queue, MAINTAINERS and watched query

Fam Zheng posted 12 patches 5 years, 4 months ago
Failed in applying to current master (apply log)
api/migrations/0042_review_to_queue.py     |  19 ++
api/migrations/0043_auto_20181120_0636.py  |  40 ++++
api/migrations/0044_watchedquery.py        |  26 +++
api/migrations/0045_message_maintainers.py |  22 ++
api/models.py                              |  20 +-
api/search.py                              |  31 ++-
mods/footer.py                             |   2 +-
mods/git.py                                |   4 +-
mods/maintainer.py                         | 233 +++++++++++++++++----
patchew-cli                                |  31 ++-
scripts/developer-setup                    |   2 +-
www/templates/base.html                    |   3 +
www/templates/my-queues.html               |  82 ++++++++
www/templates/series-detail.html           |  30 ++-
www/templates/series-list.html             |   8 +
www/views.py                               |   2 +-
16 files changed, 493 insertions(+), 62 deletions(-)
create mode 100644 api/migrations/0042_review_to_queue.py
create mode 100644 api/migrations/0043_auto_20181120_0636.py
create mode 100644 api/migrations/0044_watchedquery.py
create mode 100644 api/migrations/0045_message_maintainers.py
create mode 100644 www/templates/my-queues.html
[Patchew-devel] [PATCH v2 00/12] Queue, MAINTAINERS and watched query
Posted by Fam Zheng 5 years, 4 months ago
Paolo: I switched the email provider for my personal email address, migrated
data before you replied to v1, but the MX records were bogus until later, so my
new INBOX lost your replies. But I'm replying here:

> > 4) An event is generated upon the addition, which is configured as a source for
> >    email notifications just like testing reports. In the template, To/Cc addr
> >    of the original message is tested. Depending on if or not the user's email
> >    is in it, an email from no-reply patchew org to the user's address is sent,
> >    in reply to the original message, to catch attention from the users INBOX.
>
> Can you explain how this part would be configured?  Especially the
> "depending if or not the user's email is in it" part.

It's possible to be done using the template markups I believe. In an email
config for MessageQueued event, something like this:

```

{% if queue.name == "watched" and queue.user.email not in message.recipients %}

Hi, this message ({{ message.subject }} by {{ message.sender }})just landed in
your watched queue on Patchew but your address is not in the recepient list.

{% else %}

{{ cancel }}

{% endif %}

```

> > What is missing is the isolation between the existing maintainers role that
> > takes care of the testings etc, and the new maintainer role that is only
> > interested in receiving above notifications.
>
> Why is there a conflict?  I understand that the new functionality is for
> any logged-in user, including getting email.  As long as there Maybe
> it's just a naming issue?

The email/git/testing config things are visible to maintainers, and are too
much information and too detailed for people who only want to configure and
receive such reminder emails.

>
> Perhaps one possibility is removing merged patches from the special
> queues (accept/reject/watch), using a MessageMerged event.  Also adding
> an "empty queue" button to the "my queues" page.  Otherwise looks great.

What does the "empty queue" button do?

>
> > Do we want the applier to create and maintain a git tree/tag for each queue?
>
> Maybe later, but it's not required.
>
> > What kind of user facing commands do we want on patchew-cli? Downloading and
> > applying a queue?
>
> As a start, even just listing the git tags should be enough for the user
> to script it.  Maybe a generic "formatted search" like "git log
> --pretty=format:..."?

More on this in reply to your /my-queues page comment below.

> > Every user can have a record in WatchedQuery. The query in this record
> > is going to be run against every new series, and if matched, the patches
> > will be added to the 'watched' queue, making this the 3rd specially
> > purposed queue.
>
> Why not one-to-many?  (That is a user can have >1 record in
> WatchedQuery).  This can come in handy when people go on vacation...

It makes sense, but for now I want to limit it to 1 query per user for a few
reasons:

1) UI is simpler. The added button just overwrites the existing one everytime,
so there's no watched query list page needed. (Oh yeah, this reminds me we need
a delete button).

2) No need to write code to limit the total number of queries allowed per user
and write documentation or HTML to inform people about the limit (we don't want
to support unlimited watched queries because of resources).

So I've changed the model field to ForeignKey to make it one-to-many, but the
code still only maintains at most one record.

> > +
> > +    <div class="col-lg-10">
> > +        {% for qn, msgs in queues.items %}
> > +            <h3>Queue: {{ qn }} [{{ p }}]</h3>
> > +            <ul class="panel" id="patches">
> > +            {% for patch in msgs %}
> > +                <li><a href="/{{ p }}/{{ patch.get_series_head.message_id }}/">
> > +                  <span class="fa fa-lg {% if patch.has_replies %}fa-comment-o{% else %}fa-ellipsis-v{% endif %}"></span>
> > +                  {{ patch.subject }}
> > +                  </a></li>
> > +            {% endfor %}
> > +            </ul>
> > +        {% endfor %}
>
> Since this is actually per series, maybe we want to format it like the
> series-list page?

Ultimately this feature is not per series, we want it more flexible.
Maintainers do tweak things after applying a series: dropping individual
patches that are not mergable, fixing typos in commit messages or doing small
changes to the diff.  Sure they can do it locally, and we can be simplistic on
patchew.org, but these are also requested by the downstream maintainer tasks in
Red Hat which is currently possible with patchwork. So I want to make it
supported on patchew as well. The idea is adding a "drop" and an "edit" button at
each patch in /my-queues. "Drop" is obvious, for "edit", it may give the user a
page to edit the patch and a patch-to-patch is generated and saved as
Queue.maintainer_tweak.

v2:
  - Rebase to current next
  - Put emit_event in the right patch. [Paolo]
  - Use OneToMany relation between User and WatchedQuery. [Paolo]
  - Squash the query_test_message patch. [Paolo]

I refreshed my queue branch from a year ago, in a slightly different way. This
more or less makes it possible to realize what Markus asked for: watch for
patches touching certain files (as indicated by MAINTAINERS file) and
automatically notify him if not already in Cc.

The user story basically consists of four parts.

1) A logged in user creates a "watched query", by accessing (once we merge this)
   http://next.patchew.org/search?q=maint%3Ame+%21cc%3Ame and clicking the "Watch
   query" button. The search query will be saved for the user in the database.

2) Once a new patchset is uploaded by the importer, and post-processed by the
   applier, which also collects the associated maintainers by invoking
   .scripts/get_maintainer.pl.

3) The watched query is run against this new patchset. If it is included in the
   query, it is automatically added to the user's queue named 'watched'.

4) An event is generated upon the addition, which is configured as a source for
   email notifications just like testing reports. In the template, To/Cc addr
   of the original message is tested. Depending on if or not the user's email
   is in it, an email from no-reply@patchew.org to the user's address is sent,
   in reply to the original message, to catch attention from the users INBOX.

What is missing is the isolation between the existing maintainers role that
takes care of the testings etc, and the new maintainer role that is only
interested in receiving above notifications. Any comments on how we should do
that? Of course the role separation and fine-grained permission is only a small
part of the problem, the bigger one is how to provide better interface and hide
the email config and templating details.

Maybe the solution is to package the feature (into that "watch query" button)
and just send the notifications when it makes sense?

Do we want the applier to create and maintain a git tree/tag for each queue?

What kind of user facing commands do we want on patchew-cli? Downloading and
applying a queue?

Other comments, ideas, feedback?

Fam Zheng (12):
  scripts: User download.patchew.org for example DB
  mod: Pass request in render_page_hook
  Generalize Review model to Queue
  maintainer: Queue operations in extra ops
  model: Introduce WatchedQuery
  Add "Watch query" button to search result page
  maintainer: Update watched queue when getting new message
  www: Add /my-queues page
  maintainer: Add MessageQueued and MessageDropping events
  model: Add and populate Message.maintainers
  search: Introduce maintained-by:NAME term
  maintainer: Add maintainers status to series details

 api/migrations/0042_review_to_queue.py     |  19 ++
 api/migrations/0043_auto_20181120_0636.py  |  40 ++++
 api/migrations/0044_watchedquery.py        |  26 +++
 api/migrations/0045_message_maintainers.py |  22 ++
 api/models.py                              |  20 +-
 api/search.py                              |  31 ++-
 mods/footer.py                             |   2 +-
 mods/git.py                                |   4 +-
 mods/maintainer.py                         | 233 +++++++++++++++++----
 patchew-cli                                |  31 ++-
 scripts/developer-setup                    |   2 +-
 www/templates/base.html                    |   3 +
 www/templates/my-queues.html               |  82 ++++++++
 www/templates/series-detail.html           |  30 ++-
 www/templates/series-list.html             |   8 +
 www/views.py                               |   2 +-
 16 files changed, 493 insertions(+), 62 deletions(-)
 create mode 100644 api/migrations/0042_review_to_queue.py
 create mode 100644 api/migrations/0043_auto_20181120_0636.py
 create mode 100644 api/migrations/0044_watchedquery.py
 create mode 100644 api/migrations/0045_message_maintainers.py
 create mode 100644 www/templates/my-queues.html

-- 
2.17.2


_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH v2 00/12] Queue, MAINTAINERS and watched query
Posted by Paolo Bonzini 5 years, 4 months ago
On 28/11/18 15:34, Fam Zheng wrote:
>>> What is missing is the isolation between the existing maintainers role that
>>> takes care of the testings etc, and the new maintainer role that is only
>>> interested in receiving above notifications.
>>
>> Why is there a conflict?  I understand that the new functionality is for
>> any logged-in user, including getting email.  As long as there Maybe
>> it's just a naming issue?
> 
> The email/git/testing config things are visible to maintainers, and are too
> much information and too detailed for people who only want to configure and
> receive such reminder emails.

Got it.  Perhaps we should add a per-user setting for "Receive
maintainer notifications" (or even tristate: yes, no, only if not in
recipients) and use it in the reminder email hook.

It shouldn't be hard to add a basic "user settings" form, but it can be
done later.  In the DB this can be a UserProperty table with one-to-many
relation to User.

>> Perhaps one possibility is removing merged patches from the special
>> queues (accept/reject/watch), using a MessageMerged event.  Also adding
>> an "empty queue" button to the "my queues" page.  Otherwise looks great.
> 
> What does the "empty queue" button do?

Remove all patches from the queue.  For example you could use it to
clear the "acked" queue after sending a pull request.

>>> Every user can have a record in WatchedQuery. The query in this record
>>> is going to be run against every new series, and if matched, the patches
>>> will be added to the 'watched' queue, making this the 3rd specially
>>> purposed queue.
>>
>> Why not one-to-many?  (That is a user can have >1 record in
>> WatchedQuery).  This can come in handy when people go on vacation...
> 
> It makes sense, but for now I want to limit it to 1 query per user for a few
> reasons:
> 
> 1) UI is simpler. The added button just overwrites the existing one everytime,
> so there's no watched query list page needed. (Oh yeah, this reminds me we need
> a delete button).
> 
> 2) No need to write code to limit the total number of queries allowed per user
> and write documentation or HTML to inform people about the limit (we don't want
> to support unlimited watched queries because of resources).

True, on the other hand we can sort of trust people on this I guess.
But still I am okay with this.  It just makes it more important to add
an OR syntax.

>>> +
>>> +    <div class="col-lg-10">
>>> +        {% for qn, msgs in queues.items %}
>>> +            <h3>Queue: {{ qn }} [{{ p }}]</h3>
>>> +            <ul class="panel" id="patches">
>>> +            {% for patch in msgs %}
>>> +                <li><a href="/{{ p }}/{{ patch.get_series_head.message_id }}/">
>>> +                  <span class="fa fa-lg {% if patch.has_replies %}fa-comment-o{% else %}fa-ellipsis-v{% endif %}"></span>
>>> +                  {{ patch.subject }}
>>> +                  </a></li>
>>> +            {% endfor %}
>>> +            </ul>
>>> +        {% endfor %}
>>
>> Since this is actually per series, maybe we want to format it like the
>> series-list page?
> 
> Ultimately this feature is not per series, we want it more flexible.
> Maintainers do tweak things after applying a series: dropping individual
> patches that are not mergable, fixing typos in commit messages or doing small
> changes to the diff.  Sure they can do it locally, and we can be simplistic on
> patchew.org, but these are also requested by the downstream maintainer tasks in
> Red Hat which is currently possible with patchwork. So I want to make it
> supported on patchew as well. The idea is adding a "drop" and an "edit" button at
> each patch in /my-queues. "Drop" is obvious, for "edit", it may give the user a
> page to edit the patch and a patch-to-patch is generated and saved as
> Queue.maintainer_tweak.

I think this is independent from making the queue display per series,
however.  That is, I would rename the Queue model to QueuedSeries, and
then we could add a QueuedPatch model and a new /my-queues/MESSAGE-ID/
detail view to do these "tweaks".

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel