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