Skip to content

feat: use new mail archive search API for review app lookups #8788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rjsparks merged 5 commits into from
Apr 23, 2025
Merged

feat: use new mail archive search API for review app lookups #8788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
538e1c8
feat: use new mail archive search API for review app lookups
rpcross Apr 8, 2025
037ffe0
fix: fix patch call
rpcross Apr 17, 2025
18b0c9e
fix: remove url from query_data
rpcross Apr 17, 2025
fae7a10
fix: force utc timezone on datetime from archive API
rpcross Apr 21, 2025
480e60a
fix: remove query_data['url'] from test
rpcross Apr 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 120 additions & 100 deletions ietf/doc/tests_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -650,112 +650,132 @@ 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('ietf.review.mailarch.requests.post')
def test_retrieve_messages(self, mock_post):
mock_data = {
"results": [
{
"from": "Alice <alice@example.com>",
"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",
}
]
}
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 <johndoe@example.com>"
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"] = "<https://www.example.com/testmessage>"
msg["Date"] = email.utils.formatdate()

mbox.add(msg)

# plain text + HTML
msg = email.mime.multipart.MIMEMultipart('alternative')
msg["From"] = "John Doe II <johndoe2@example.com>"
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"] = "<https://www.example.com/testmessage2>"

msg.attach(email.mime.text.MIMEText("Hi!,\r\nLooks OK!\r\n-John", "plain"))
msg.attach(email.mime.text.MIMEText("<html><body><p>Hi!,</p><p>Looks OK!</p><p>-John</p></body></html>", "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 <alice@example.com>')
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['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('ietf.doc.views_review.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 <alice@example.com>",
"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",
},
{
"from": "Joe <joe@example.com>",
"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",
}
]
}
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("<html>" 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<html><body><div class="xtr"><div class="xtd no-results">No results found</div></div>')
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("<html>" 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')
Expand Down
22 changes: 10 additions & 12 deletions ietf/doc/views_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand Down
Loading