| Message ID | 20210826171831.547578-4-stephen@that.guru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Make addressed/unaddressed workflow opt-in | expand |
Stephen Finucane <stephen@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? 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. > > Some minors issues are addressed in the docs while we're here. > > Signed-off-by: Stephen Finucane <stephen@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-author@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
On Fri, 2021-08-27 at 13:51 +1000, Daniel Axtens wrote: > Stephen Finucane <stephen@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 > > > > > Some minors issues are addressed in the docs while we're here. > > > > Signed-off-by: Stephen Finucane <stephen@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-author@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
Stephen Finucane <stephen@that.guru> writes: > On Fri, 2021-08-27 at 13:51 +1000, Daniel Axtens wrote: >> Stephen Finucane <stephen@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 <stephen@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-author@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
czw., 26 sie 2021 o 19:19 Stephen Finucane <stephen@that.guru> napisał(a): > Allow submitters to indicate that their comment is something that needs > to be addressed. FWIW you shouldn't use "X-" prefix as it's discouraged. I'm not sure you it's the right moment to change the convention? https://stackoverflow.com/questions/3561381/custom-http-headers-naming-conventions https://datatracker.ietf.org/doc/html/rfc6648
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-author@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'
Allow submitters to indicate that their comment is something that needs to be addressed. Some minors issues are addressed in the docs while we're here. Signed-off-by: Stephen Finucane <stephen@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(-)