From 538e1c80d8c8cdf35727b8cbc747347bb8565d3b Mon Sep 17 00:00:00 2001 From: Ryan Cross Date: Tue, 8 Apr 2025 16:09:10 -0700 Subject: [PATCH 1/5] feat: use new mail archive search API for review app lookups --- ietf/doc/tests_review.py | 221 ++++++++++-------- ietf/doc/views_review.py | 22 +- ietf/review/mailarch.py | 130 ++++------- ietf/settings.py | 2 + .../templates/doc/review/complete_review.html | 4 +- 5 files changed, 181 insertions(+), 198 deletions(-) diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 13ddbc22ba3..1d31b076140 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -3,12 +3,12 @@ from pathlib import Path -import datetime, os, shutil +import datetime import io -import tarfile, tempfile, mailbox -import email.mime.multipart, email.mime.text, email.utils +import os +import shutil -from mock import patch +from mock import patch, Mock from requests import Response from django.apps import apps @@ -650,112 +650,133 @@ def test_accept_reviewer_assignment_after_reject(self): assignment = reload_db_objects(assignment) self.assertEqual(assignment.state_id, "accepted") - def make_test_mbox_tarball(self, review_req): - mbox_path = os.path.join(self.review_dir, "testmbox.tar.gz") - with tarfile.open(mbox_path, "w:gz") as tar: - with tempfile.NamedTemporaryFile(dir=self.review_dir, suffix=".mbox") as tmp: - mbox = mailbox.mbox(tmp.name) + @patch('requests.post') + def test_retrieve_messages(self, mock_post): + mock_data = { + "results": [ + { + "from": "Alice ", + "subject": "Hello", + "content": "Hi\n This is a really good document.\n", + "message_id": "abc123", + "url": "https://example.com/message", + "date": "2025-04-07T12:00:00+00:00", + } + ] + } + mock_post.return_value.json.return_value = mock_data - # plain text - msg = email.mime.text.MIMEText("Hello,\n\nI have reviewed the document and did not find any problems.\n\nJohn Doe") - msg["From"] = "John Doe " - msg["To"] = review_req.team.list_email - msg["Subject"] = "Review of {}-01".format(review_req.doc.name) - msg["Message-ID"] = email.utils.make_msgid() - msg["Archived-At"] = "" - msg["Date"] = email.utils.formatdate() - - mbox.add(msg) - - # plain text + HTML - msg = email.mime.multipart.MIMEMultipart('alternative') - msg["From"] = "John Doe II " - msg["To"] = review_req.team.list_email - msg["Subject"] = "Review of {}".format(review_req.doc.name) - msg["Message-ID"] = email.utils.make_msgid() - msg["Archived-At"] = "" - - msg.attach(email.mime.text.MIMEText("Hi!,\r\nLooks OK!\r\n-John", "plain")) - msg.attach(email.mime.text.MIMEText("

Hi!,

Looks OK!

-John

", "html")) - mbox.add(msg) - - tmp.flush() - - tar.add(os.path.relpath(tmp.name)) + doc = WgDraftFactory(group__acronym='mars', rev='01') + review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut")) + rev_role = RoleFactory(group=review_team, person__user__username='reviewer', person__user__email='reviewer@example.com', name_id='reviewer') + RoleFactory(group=review_team, person__user__username='reviewsecretary', person__user__email='reviewsecretary@example.com', name_id='secr') + review_req = ReviewRequestFactory(doc=doc, team=review_team, type_id='early', state_id='assigned', requested_by=rev_role.person, deadline=timezone.now() + datetime.timedelta(days=20)) + ReviewAssignmentFactory(review_request=review_req, reviewer=rev_role.person.email_set.first(), state_id='accepted') - mbox.close() + query_data = ietf.review.mailarch.construct_query_data(doc, review_team, query=None) + response = ietf.review.mailarch.retrieve_messages(query_data) - return mbox_path + self.assertEqual(len(response), 1) + self.assertEqual(response[0]['from'], 'Alice ') + self.assertEqual(response[0]['splitfrom'], ('Alice', 'alice@example.com')) + self.assertEqual(response[0]['subject'], 'Hello') + self.assertEqual(response[0]['content'], 'Hi\n This is a really good document.') + self.assertEqual(response[0]['message_id'], 'abc123') + self.assertEqual(response[0]['url'], 'https://example.com/message') + self.assertEqual(response[0]['utcdate'], ('2025-04-07', '12:00:00')) - def test_search_mail_archive(self): - doc = WgDraftFactory(group__acronym='mars',rev='01') + def test_construct_query_data(self): + doc = WgDraftFactory(group__acronym='mars', rev='01') review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut")) - rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',name_id='reviewer') - RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr') - review_req = ReviewRequestFactory(doc=doc,team=review_team,type_id='early',state_id='assigned',requested_by=rev_role.person,deadline=timezone.now()+datetime.timedelta(days=20)) + data = ietf.review.mailarch.construct_query_data(doc, review_team, query=None) + self.assertEqual(data['url'], 'https://mailarchive.ietf.org/api/v1/message/search/') + self.assertEqual(data['start_date'], (date_today() - datetime.timedelta(days=180)).isoformat()) + self.assertEqual(data['email_list'], 'reviewteam') + self.assertEqual(data['query_value'], doc.name) + self.assertEqual(data['query'], f'subject:({doc.name})') + self.assertEqual(data['limit'], '30') + + @patch('requests.post') + def test_search_mail_archive(self, mock_post): + doc = WgDraftFactory(group__acronym='mars', rev='01') + review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut")) + rev_role = RoleFactory(group=review_team, person__user__username='reviewer', person__user__email='reviewer@example.com', name_id='reviewer') + RoleFactory(group=review_team, person__user__username='reviewsecretary', person__user__email='reviewsecretary@example.com', name_id='secr') + review_req = ReviewRequestFactory(doc=doc, team=review_team, type_id='early', state_id='assigned', requested_by=rev_role.person, deadline=timezone.now() + datetime.timedelta(days=20)) assignment = ReviewAssignmentFactory(review_request=review_req, reviewer=rev_role.person.email_set.first(), state_id='accepted') + mock_data = { + "results": [ + { + "from": "Alice ", + "subject": "Review of {}-01".format(review_req.doc.name), + "content": "Hi\n This is a really good document.\n", + "message_id": "abc123", + "url": "https://example.com/message", + "date": "2025-04-07T12:00:00+00:00", + }, + { + "from": "Joe ", + "subject": "Review of {}".format(review_req.doc.name), + "content": "Hi\n I believe this is the best document.\n", + "message_id": "abc456", + "url": "https://example.com/message", + "date": "2025-04-07T12:00:00+00:00", + } + ] + } + response1 = Mock() + response1.json.return_value = mock_data + + response2 = Mock() + response2.json.return_value = mock_data + + response3 = Mock() + response3.json.return_value = {"results": []} + + mock_post.side_effect = [response1, response2, response3] + # test URL construction - query_urls = ietf.review.mailarch.construct_query_urls(doc, review_team) - self.assertTrue(review_req.doc.name in query_urls["query_data_url"]) + query_data = ietf.review.mailarch.construct_query_data(doc, review_team) + self.assertTrue(review_req.doc.name in query_data["query_value"]) - # test parsing - mbox_path = self.make_test_mbox_tarball(review_req) + url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={"name": doc.name, "assignment_id": assignment.pk}) + url2 = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={"name": doc.name, "acronym": review_team.acronym}) + login_testing_unauthorized(self, "reviewsecretary", url) - try: - # mock URL generator and point it to local file - for this - # to work, the module (and not the function) must be - # imported in the view - real_fn = ietf.review.mailarch.construct_query_urls - ietf.review.mailarch.construct_query_urls = lambda doc, team, query=None: { "query_data_url": "file://" + os.path.abspath(mbox_path) } - url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "assignment_id": assignment.pk }) - url2 = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "acronym": review_team.acronym }) - login_testing_unauthorized(self, "reviewsecretary", url) - - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - messages = r.json()["messages"] - self.assertEqual(len(messages), 2) - - r = self.client.get(url2) - self.assertEqual(r.status_code, 200) - messages = r.json()["messages"] - self.assertEqual(len(messages), 2) - - today = date_today(datetime.timezone.utc) - - self.assertEqual(messages[0]["url"], "https://www.example.com/testmessage") - self.assertTrue("John Doe" in messages[0]["content"]) - self.assertEqual(messages[0]["subject"], "Review of {}-01".format(review_req.doc.name)) - self.assertEqual(messages[0]["revision_guess"], "01") - self.assertEqual(messages[0]["splitfrom"], ["John Doe", "johndoe@example.com"]) - self.assertEqual(messages[0]["utcdate"][0], today.isoformat()) - - self.assertEqual(messages[1]["url"], "https://www.example.com/testmessage2") - self.assertTrue("Looks OK" in messages[1]["content"]) - self.assertTrue("" not in messages[1]["content"]) - self.assertEqual(messages[1]["subject"], "Review of {}".format(review_req.doc.name)) - self.assertFalse('revision_guess' in messages[1]) - self.assertEqual(messages[1]["splitfrom"], ["John Doe II", "johndoe2@example.com"]) - self.assertEqual(messages[1]["utcdate"][0], "") - - - # Test failure to return mailarch results - no_result_path = os.path.join(self.review_dir, "mailarch_no_result.html") - with io.open(no_result_path, "w") as f: - f.write('Content-Type: text/html\n\n
No results found
') - ietf.review.mailarch.construct_query_urls = lambda doc, team, query=None: { "query_data_url": "file://" + os.path.abspath(no_result_path) } - - url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={ "name": doc.name, "assignment_id": assignment.pk }) - - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - result = r.json() - self.assertNotIn('messages', result) - self.assertIn('No results found', result['error']) - - finally: - ietf.review.mailarch.construct_query_urls = real_fn + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + messages = r.json()["messages"] + self.assertEqual(len(messages), 2) + + r = self.client.get(url2) + self.assertEqual(r.status_code, 200) + messages = r.json()["messages"] + self.assertEqual(len(messages), 2) + + self.assertEqual(messages[0]["url"], "https://example.com/message") + self.assertTrue("Hi" in messages[0]["content"]) + self.assertEqual(messages[0]["subject"], "Review of {}-01".format(review_req.doc.name)) + self.assertEqual(messages[0]["revision_guess"], "01") + self.assertEqual(messages[0]["splitfrom"], ["Alice", "alice@example.com"]) + self.assertEqual(messages[0]["utcdate"], ['2025-04-07', '12:00:00']) + + self.assertEqual(messages[1]["url"], "https://example.com/message") + self.assertTrue("Hi" in messages[1]["content"]) + self.assertTrue("" not in messages[1]["content"]) + self.assertEqual(messages[1]["subject"], "Review of {}".format(review_req.doc.name)) + self.assertFalse('revision_guess' in messages[1]) + self.assertEqual(messages[1]["splitfrom"], ["Joe", "joe@example.com"]) + self.assertEqual(messages[1]["utcdate"], ['2025-04-07', '12:00:00']) + + # Test failure to return mailarch results + url = urlreverse('ietf.doc.views_review.search_mail_archive', kwargs={"name": doc.name, "assignment_id": assignment.pk}) + + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + result = r.json() + self.assertNotIn('messages', result) + self.assertIn('No results found', result['error']) def test_submit_unsolicited_review_choose_team(self): doc = WgDraftFactory(group__acronym='mars', rev='01') diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py index 1f23c435fa8..6ac0e17791e 100644 --- a/ietf/doc/views_review.py +++ b/ietf/doc/views_review.py @@ -958,14 +958,14 @@ def complete_review(request, name, assignment_id=None, acronym=None): form = CompleteReviewForm(assignment, doc, team, is_reviewer, initial=initial) - mail_archive_query_urls = mailarch.construct_query_urls(doc, team) + mail_archive_query_data = mailarch.construct_query_data(doc, team) return render(request, 'doc/review/complete_review.html', { 'doc': doc, 'team': team, 'assignment': assignment, 'form': form, - 'mail_archive_query_urls': mail_archive_query_urls, + 'mail_archive_query_data': mail_archive_query_data, 'revising_review': revising_review, 'review_to': to, 'review_cc': cc, @@ -987,27 +987,25 @@ def search_mail_archive(request, name, acronym=None, assignment_id=None): if not (is_reviewer or can_manage_request): permission_denied(request, "You do not have permission to perform this action") - res = mailarch.construct_query_urls(doc, team, query=request.GET.get("query")) - if not res: - return JsonResponse({ "error": "Couldn't do lookup in mail archive - don't know where to look"}) - - MAX_RESULTS = 30 + query_data = mailarch.construct_query_data(doc, team, query=request.GET.get("query")) + if not query_data: + return JsonResponse({"error": "Couldn't do lookup in mail archive - don't know where to look"}) try: - res["messages"] = mailarch.retrieve_messages(res["query_data_url"])[:MAX_RESULTS] - for message in res["messages"]: + query_data["messages"] = mailarch.retrieve_messages(query_data) + for message in query_data["messages"]: try: revision_guess = message["subject"].split(name)[1].split('-')[1] message["revision_guess"] = revision_guess if revision_guess.isnumeric() else None except IndexError: pass except KeyError as e: - res["error"] = "No results found (%s)" % str(e) + query_data["error"] = "No results found (%s)" % str(e) except Exception as e: - res["error"] = "Retrieval from mail archive failed: %s" % str(e) + query_data["error"] = "Retrieval from mail archive failed: %s" % str(e) # raise # useful when debugging - return JsonResponse(res) + return JsonResponse(query_data) class EditReviewRequestCommentForm(forms.ModelForm): comment = forms.CharField(widget=forms.Textarea, strip=False) diff --git a/ietf/review/mailarch.py b/ietf/review/mailarch.py index c34a6079ce7..867bcdf4ebd 100644 --- a/ietf/review/mailarch.py +++ b/ietf/review/mailarch.py @@ -6,25 +6,19 @@ # mailarchive.ietf.org import base64 -import contextlib import datetime import email.utils import hashlib -import mailbox -import tarfile -import tempfile - -from urllib.parse import urlencode -from urllib.request import urlopen +import json +import requests import debug # pyflakes:ignore -from pyquery import PyQuery from django.conf import settings from django.utils.encoding import force_bytes, force_str -from ietf.utils.mail import get_payload_text +from ietf.utils.log import log from ietf.utils.timezone import date_today @@ -43,7 +37,7 @@ def hash_list_message_id(list_name, msgid): sha.update(force_bytes(list_name)) return force_str(base64.urlsafe_b64encode(sha.digest()).rstrip(b"=")) -def construct_query_urls(doc, team, query=None): +def construct_query_data(doc, team, query=None): list_name = list_name_from_email(team.list_email) if not list_name: return None @@ -51,83 +45,51 @@ def construct_query_urls(doc, team, query=None): if not query: query = doc.name - encoded_query = "?" + urlencode({ - "qdr": "c", # custom time frame - "start_date": (date_today() - datetime.timedelta(days=180)).isoformat(), - "email_list": list_name, - "q": "subject:({})".format(query), - "as": "1", # this is an advanced search - }) - - return { - "query": query, - "query_url": settings.MAILING_LIST_ARCHIVE_URL + "/arch/search/" + encoded_query, - "query_data_url": settings.MAILING_LIST_ARCHIVE_URL + "/arch/export/mbox/" + encoded_query, + query_data = { + 'url': settings.MAILING_LIST_ARCHIVE_SEARCH_URL, + 'start_date': (date_today() - datetime.timedelta(days=180)).isoformat(), + 'email_list': list_name, + 'query_value': query, + 'query': f'subject:({query})', + 'limit': '30', } + return query_data def construct_message_url(list_name, msgid): return "{}/arch/msg/{}/{}".format(settings.MAILING_LIST_ARCHIVE_URL, list_name, hash_list_message_id(list_name, msgid)) -def retrieve_messages_from_mbox(mbox_fileobj): - """Return selected content in message from mbox from mailarch.""" - res = [] - with tempfile.NamedTemporaryFile(suffix=".mbox") as mbox_file: - # mailbox.mbox needs a path, so we need to put the contents - # into a file - mbox_data = mbox_fileobj.read() - mbox_file.write(mbox_data) - mbox_file.flush() - - mbox = mailbox.mbox(mbox_file.name, create=False) - for msg in mbox: - content = "" - - for part in msg.walk(): - if part.get_content_type() == "text/plain": - charset = part.get_content_charset() or "utf-8" - content += get_payload_text(part, default_charset=charset) - - # parse a couple of things for the front end - utcdate = None - d = email.utils.parsedate_tz(msg["Date"]) - if d: - utcdate = datetime.datetime.fromtimestamp(email.utils.mktime_tz(d), datetime.timezone.utc) - - res.append({ - "from": msg["From"], - "splitfrom": email.utils.parseaddr(msg["From"]), - "subject": msg["Subject"], - "content": content.replace("\r\n", "\n").replace("\r", "\n").strip("\n"), - "message_id": email.utils.unquote(msg["Message-ID"].strip()), - "url": email.utils.unquote(msg["Archived-At"].strip()), - "date": msg["Date"], - "utcdate": (utcdate.date().isoformat(), utcdate.time().isoformat()) if utcdate else ("", ""), - }) - mbox.close() - - return res - -def retrieve_messages(query_data_url): +def retrieve_messages(query_data): """Retrieve and return selected content from mailarch.""" - res = [] - - # This has not been rewritten to use requests.get() because get() does - # not handle file URLs out of the box, which we need for tesing - with contextlib.closing(urlopen(query_data_url, timeout=15)) as fileobj: - content_type = fileobj.info()["Content-type"] - if not content_type.startswith("application/x-tar"): - if content_type.startswith("text/html"): - r = fileobj.read(20000) - q = PyQuery(r) - div = q('div[class~="no-results"]') - if div: - raise KeyError("No results: %s -> %s" % (query_data_url, div.text(), )) - raise Exception("Export failed - this usually means no matches were found") - - with tarfile.open(fileobj=fileobj, mode='r|*') as tar: - for entry in tar: - if entry.isfile(): - mbox_fileobj = tar.extractfile(entry) - res.extend(retrieve_messages_from_mbox(mbox_fileobj)) - - return res + + headers = {'X-Api-Key': settings.MAILING_LIST_ARCHIVE_API_KEY, + 'Content-Type': 'application/json'} + post_data = query_data.copy() + del post_data['url'] + json_data = json.dumps(post_data) + try: + response = requests.post( + query_data['url'], + headers=headers, + data=json_data, + timeout=settings.DEFAULT_REQUESTS_TIMEOUT) + except requests.Timeout as exc: + log(f'POST request failed for [{query_data["url"]}]: {exc}') + raise RuntimeError(f'Timeout retrieving [{query_data["url"]}]') from exc + + results = [] + jresponse = response.json() + if 'results' not in jresponse or len(jresponse['results']) == 0: + raise KeyError(f'No results: {query_data["query"]}') + for msg in jresponse['results']: + utcdate = datetime.datetime.fromisoformat(msg['date']) # date is already UTC + results.append({ + "from": msg["from"], + "splitfrom": email.utils.parseaddr(msg["from"]), + "subject": msg["subject"], + "content": msg["content"].replace("\r\n", "\n").replace("\r", "\n").strip("\n"), + "message_id": msg["message_id"], + "url": msg["url"], + "utcdate": (utcdate.date().isoformat(), utcdate.time().isoformat()), + }) + + return results diff --git a/ietf/settings.py b/ietf/settings.py index 1fe5f482294..a13fd3df8ff 100644 --- a/ietf/settings.py +++ b/ietf/settings.py @@ -808,6 +808,8 @@ def skip_unreadable_post(record): # Mailing list info URL for lists hosted on the IETF servers MAILING_LIST_INFO_URL = "https://mailman3.%(domain)s/mailman3/lists/%(list_addr)s.%(domain)s" MAILING_LIST_ARCHIVE_URL = "https://mailarchive.ietf.org" +MAILING_LIST_ARCHIVE_SEARCH_URL = "https://mailarchive.ietf.org/api/v1/message/search/" +MAILING_LIST_ARCHIVE_API_KEY = "changeme" # Liaison Statement Tool settings (one is used in DOC_HREFS below) LIAISON_UNIVERSAL_FROM = 'Liaison Statement Management Tool ' diff --git a/ietf/templates/doc/review/complete_review.html b/ietf/templates/doc/review/complete_review.html index 091f30532e6..8aeb1486a4c 100644 --- a/ietf/templates/doc/review/complete_review.html +++ b/ietf/templates/doc/review/complete_review.html @@ -93,7 +93,7 @@

Back {% endif %}
- {% if mail_archive_query_urls %} + {% if mail_archive_query_data %}