Stephen Finucane <step...@that.guru> writes:
> On Fri, 2021-08-27 at 13:51 +1000, Daniel Axtens wrote:
>> Stephen Finucane <step...@that.guru> writes:
>>
>> > Allow submitters to indicate that their comment is something that needs
>> > to be addressed.
>>
>> Hmm, do we have any evidence that any of our existing mail headers are
>> used by anything?
>
> I've no idea. A quick scan through an old Patchwork archive mbox I have
> locally
> suggests no but I can't speak for other lists. That said, part of the issue
> here
> could simply be lack of awareness of the feature as much as anything else.
> Perhaps we should try to make this feature more prominent in the docs?
>
>> Also, I'm not confident I know how to set a header on a comment and I
>> write my email in emacs, notoriously one of the more configurable
>> platforms for any given task.
>
> Evolution does make it pretty easy [1], as does mutt [1]. We could add these
> links to the docs also, if it would help? I'll admit I haven't used either
> myself though since I'm happy enough with 'git-pw' for my day-to-day work.
>
> Stephen
>
> [1]
> https://help.gnome.org/users/evolution/stable//mail-composer-custom-header-lines
> [2] http://www.mutt.org/doc/manual/#ex-my-hdr
Under the proposed model where the patch submitter gets to decide about
the display of un/addressed, does it make sense to have email header
support?
The email header support moves the un/addressed setting to the comment
submitter which sits oddly with it being usually owned by the patch
submitter.
Kind regards,
Daniel
>
>>
>> >
>> > Some minors issues are addressed in the docs while we're here.
>> >
>> > Signed-off-by: Stephen Finucane <step...@that.guru>
>> > ---
>> > docs/usage/headers.rst | 17 ++++---
>> > docs/usage/overview.rst | 7 +++
>> > patchwork/parser.py | 10 +++-
>> > patchwork/tests/test_parser.py | 89 ++++++++++++++++++++++++++++++++--
>> > 4 files changed, 111 insertions(+), 12 deletions(-)
>> >
>> > diff --git docs/usage/headers.rst docs/usage/headers.rst
>> > index 26e97c0a..b2b4b2d6 100644
>> > --- docs/usage/headers.rst
>> > +++ docs/usage/headers.rst
>> > @@ -5,8 +5,7 @@ Patchwork provides a number of special email headers to
>> > control how a patch is
>> > handled when it is received. The examples provided below use
>> > `git-send-email`,
>> > but custom headers can also be set when using tools like `mutt`.
>> >
>> > -`X-Patchwork-Hint`
>> > -
>> > +``X-Patchwork-Hint``
>> > Valid values: ignore
>> >
>> > When set, this header will ensure the provided email is not parsed
>> > @@ -16,8 +15,7 @@ but custom headers can also be set when using tools like
>> > `mutt`.
>> >
>> > $ git send-email --add-header="X-Patchwork-Hint: ignore" master
>> >
>> > -`X-Patchwork-Delegate`
>> > -
>> > +``X-Patchwork-Delegate``
>> > Valid values: An email address associated with a Patchwork user
>> >
>> > If set and valid, the user corresponding to the provided email address
>> > will
>> > @@ -28,8 +26,7 @@ but custom headers can also be set when using tools like
>> > `mutt`.
>> >
>> > $ git send-email --add-header="X-Patchwork-Delegate:
>> > a...@example.com" master
>> >
>> > -`X-Patchwork-State`
>> > -
>> > +``X-Patchwork-State``
>> > Valid values: Varies between deployments. This can usually be one of
>> > "Accepted", "Rejected", "RFC" or "Awaiting Upstream", among others.
>> >
>> > @@ -39,3 +36,11 @@ but custom headers can also be set when using tools
>> > like `mutt`.
>> > .. code-block:: shell
>> >
>> > $ git send-email --add-header="X-Patchwork-State: RFC" master
>> > +
>> > +``X-Patchwork-Action-Required``
>> > + Valid values: <none, value is ignored>
>> > +
>> > + When set on a reply to an existing cover letter or patch, this header
>> > will
>> > + mark the comment as "unaddressed". The comment can then be addressed
>> > using
>> > + the API or web UI. For more details, refer to
>> > + :ref:`overview-comment-metadata`.
>> > diff --git docs/usage/overview.rst docs/usage/overview.rst
>> > index a58acfa5..297569ec 100644
>> > --- docs/usage/overview.rst
>> > +++ docs/usage/overview.rst
>> > @@ -181,6 +181,8 @@ system to test patches. Checks have a number of fields
>> > associated with them:
>> > to Patchwork.
>> >
>> >
>> > +.. _overview-comment-metadata:
>> > +
>> > Comment Metadata
>> > ----------------
>> >
>> > @@ -198,6 +200,11 @@ the cover letters. Once the submitter has provided
>> > the required information,
>> > either the submitter or a maintainer can mark the comment as "addressed".
>> > This
>> > provides a more granular way of tracking work items than patch states.
>> >
>> > +.. note::
>> > +
>> > + Users can indicate that a comment requires an action using a custom
>> > mail
>> > + header. For more information, refer to :doc:`/usage/headers`.
>> > +
>> >
>> > Collections
>> > -----------
>> > diff --git patchwork/parser.py patchwork/parser.py
>> > index 61a81246..e6e1a7fb 100644
>> > --- patchwork/parser.py
>> > +++ patchwork/parser.py
>> > @@ -1019,6 +1019,12 @@ def find_delegate_by_header(mail):
>> > return None
>> >
>> >
>> > +def find_comment_addressed_by_header(mail):
>> > + """Determine whether a comment is actionable or not."""
>> > + # we dispose of the value - it's irrelevant
>> > + return False if 'X-Patchwork-Action-Required' in mail else None
>> > +
>> > +
>> > def parse_mail(mail, list_id=None):
>> > """Parse a mail and add to the database.
>> >
>> > @@ -1278,6 +1284,7 @@ def parse_mail(mail, list_id=None):
>> > patch = find_patch_for_comment(project, refs)
>> > if patch:
>> > author = get_or_create_author(mail, project)
>> > + addressed = find_comment_addressed_by_header(mail)
>> >
>> > with transaction.atomic():
>> > if PatchComment.objects.filter(patch=patch, msgid=msgid):
>> > @@ -1288,7 +1295,8 @@ def parse_mail(mail, list_id=None):
>> > date=date,
>> > headers=headers,
>> > submitter=author,
>> > - content=message)
>> > + content=message,
>> > + addressed=addressed)
>> >
>> > logger.debug('Comment saved')
>> >
>> > diff --git patchwork/tests/test_parser.py patchwork/tests/test_parser.py
>> > index eaf6599c..d0c5c2d7 100644
>> > --- patchwork/tests/test_parser.py
>> > +++ patchwork/tests/test_parser.py
>> > @@ -18,6 +18,7 @@ from django.db.transaction import atomic
>> > from django.db import connection
>> >
>> > from patchwork.models import Cover
>> > +from patchwork.models import CoverComment
>> > from patchwork.models import Patch
>> > from patchwork.models import PatchComment
>> > from patchwork.models import Person
>> > @@ -68,22 +69,42 @@ def read_mail(filename, project=None):
>> > return mail
>> >
>> >
>> > -def _create_email(msg, msgid=None, sender=None, listid=None,
>> > in_reply_to=None):
>> > +def _create_email(
>> > + msg,
>> > + msgid=None,
>> > + subject=None,
>> > + sender=None,
>> > + listid=None,
>> > + in_reply_to=None,
>> > + headers=None,
>> > +):
>> > msg['Message-Id'] = msgid or make_msgid()
>> > - msg['Subject'] = 'Test subject'
>> > + msg['Subject'] = subject or 'Test subject'
>> > msg['From'] = sender or 'Test Author <test-aut...@example.com>'
>> > msg['List-Id'] = listid or 'test.example.com'
>> > +
>> > if in_reply_to:
>> > msg['In-Reply-To'] = in_reply_to
>> >
>> > + for header in headers or {}:
>> > + msg[header] = headers[header]
>> > +
>> > return msg
>> >
>> >
>> > -def create_email(content, msgid=None, sender=None, listid=None,
>> > - in_reply_to=None):
>> > +def create_email(
>> > + content,
>> > + msgid=None,
>> > + subject=None,
>> > + sender=None,
>> > + listid=None,
>> > + in_reply_to=None,
>> > + headers=None,
>> > +):
>> > msg = MIMEText(content, _charset='us-ascii')
>> >
>> > - return _create_email(msg, msgid, sender, listid, in_reply_to)
>> > + return _create_email(
>> > + msg, msgid, subject, sender, listid, in_reply_to, headers)
>> >
>> >
>> > def parse_mail(*args, **kwargs):
>> > @@ -821,6 +842,64 @@ class DelegateRequestTest(TestCase):
>> > self.assertDelegate(None)
>> >
>> >
>> > +class CommentActionRequiredTest(TestCase):
>> > +
>> > + fixtures = ['default_tags']
>> > +
>> > + def setUp(self):
>> > + self.project = create_project(listid='test.example.com')
>> > +
>> > + def _create_submission_and_comments(self, submission_email):
>> > + comment_a_email = create_email(
>> > + 'test comment\n',
>> > + in_reply_to=submission_email['Message-Id'],
>> > + listid=self.project.listid,
>> > + headers={},
>> > + )
>> > + comment_b_email = create_email(
>> > + 'another test comment\n',
>> > + in_reply_to=submission_email['Message-Id'],
>> > + listid=self.project.listid,
>> > + headers={'X-Patchwork-Action-Required': ''},
>> > + )
>> > + parse_mail(submission_email)
>> > + parse_mail(comment_a_email)
>> > + parse_mail(comment_b_email)
>> > +
>> > + comment_a_msgid = comment_a_email.get('Message-ID')
>> > + comment_b_msgid = comment_b_email.get('Message-ID')
>> > +
>> > + return comment_a_msgid, comment_b_msgid
>> > +
>> > + def test_patch_comment(self):
>> > + body = read_patch('0001-add-line.patch')
>> > + patch_email = create_email(body, listid=self.project.listid)
>> > + comment_a_msgid, comment_b_msgid = \
>> > + self._create_submission_and_comments(patch_email)
>> > +
>> > + self.assertEqual(1, Patch.objects.count())
>> > + self.assertEqual(2, PatchComment.objects.count())
>> > + comment_a = PatchComment.objects.get(msgid=comment_a_msgid)
>> > + self.assertIsNone(comment_a.addressed)
>> > + comment_b = PatchComment.objects.get(msgid=comment_b_msgid)
>> > + self.assertFalse(comment_b.addressed)
>> > +
>> > + def test_cover_comment(self):
>> > + cover_email = create_email(
>> > + 'test cover letter',
>> > + subject='[0/2] A cover letter',
>> > + listid=self.project.listid)
>> > + comment_a_msgid, comment_b_msgid = \
>> > + self._create_submission_and_comments(cover_email)
>> > +
>> > + self.assertEqual(1, Cover.objects.count())
>> > + self.assertEqual(2, CoverComment.objects.count())
>> > + comment_a = CoverComment.objects.get(msgid=comment_a_msgid)
>> > + self.assertIsNone(comment_a.addressed)
>> > + comment_b = CoverComment.objects.get(msgid=comment_b_msgid)
>> > + self.assertFalse(comment_b.addressed)
>> > +
>> > +
>> > class InitialPatchStateTest(TestCase):
>> >
>> > patch_filename = '0001-add-line.patch'
>> > --
>> > 2.31.1
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
