From ee2fd29795c38e4ccf80de7fd6405312504b955c Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 28 Jun 2023 12:42:12 -0400 Subject: [PATCH 1/8] refactor: Harmonize how profile and photo views look up email_or_name --- ietf/person/utils.py | 17 ++++++++++++++++- ietf/person/views.py | 26 +++++--------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/ietf/person/utils.py b/ietf/person/utils.py index a28aaaefa9..5c97eda42e 100755 --- a/ietf/person/utils.py +++ b/ietf/person/utils.py @@ -12,10 +12,11 @@ from django.core.cache import cache from django.core.exceptions import ObjectDoesNotExist from django.db.models import Q +from django.http import Http404 import debug # pyflakes:ignore -from ietf.person.models import Person +from ietf.person.models import Person, Alias, Email from ietf.utils.mail import send_mail def merge_persons(request, source, target, file=sys.stdout, verbose=False): @@ -246,3 +247,17 @@ def get_dots(person): if roles.filter(group__acronym__startswith='nomcom', name_id__in=('chair','member')).exists(): dots.append('nomcom') return dots + +def lookup_persons(email_or_name): + aliases = Alias.objects.filter(name=email_or_name) + persons = set(a.person for a in aliases) + + if '@' in email_or_name: + emails = Email.objects.filter(address=email_or_name) + persons.update(e.person for e in emails) + + persons = [p for p in persons if p and p.id] + if not persons: + raise Http404 + persons.sort(key=lambda p: p.id) + return persons diff --git a/ietf/person/views.py b/ietf/person/views.py index 4e3f7d8c30..7280992443 100644 --- a/ietf/person/views.py +++ b/ietf/person/views.py @@ -8,16 +8,16 @@ from django.contrib import messages from django.db.models import Q from django.http import HttpResponse, Http404 -from django.shortcuts import render, get_object_or_404, redirect +from django.shortcuts import render, redirect from django.utils import timezone import debug # pyflakes:ignore from ietf.ietfauth.utils import role_required -from ietf.person.models import Email, Person, Alias +from ietf.person.models import Email, Person from ietf.person.fields import select2_id_name_json from ietf.person.forms import MergeForm -from ietf.person.utils import handle_users, merge_persons +from ietf.person.utils import handle_users, merge_persons, lookup_persons def ajax_select2_search(request, model_name): @@ -69,28 +69,12 @@ def ajax_select2_search(request, model_name): def profile(request, email_or_name): - aliases = Alias.objects.filter(name=email_or_name) - persons = set(a.person for a in aliases) - - if '@' in email_or_name: - emails = Email.objects.filter(address=email_or_name) - persons.update(e.person for e in emails) - - persons = [p for p in persons if p and p.id] - if not persons: - raise Http404 - persons.sort(key=lambda p: p.id) + persons = lookup_persons(email_or_name) return render(request, 'person/profile.html', {'persons': persons, 'today': timezone.now()}) def photo(request, email_or_name): - if '@' in email_or_name: - persons = [ get_object_or_404(Email, address=email_or_name).person, ] - else: - aliases = Alias.objects.filter(name=email_or_name) - persons = list(set([ a.person for a in aliases ])) - if not persons: - raise Http404("No such person") + persons = lookup_persons(email_or_name) if len(persons) > 1: return HttpResponse(r"\r\n".join([p.email() for p in persons]), status=300) person = persons[0] From 485dfc5bebb4d382855fda71d93dbd6935a0fe09 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 28 Jun 2023 12:44:57 -0400 Subject: [PATCH 2/8] refactor: Rework community views to operate on Person instead of User (#5859) --- ietf/community/models.py | 2 +- ietf/community/tests.py | 24 ++++----- ietf/community/urls.py | 14 ++--- ietf/community/utils.py | 17 ++++-- ietf/community/views.py | 54 +++++++++++++------ ietf/templates/community/list_menu.html | 8 +-- ietf/templates/community/view_list.html | 2 +- ietf/templates/doc/document_draft.html | 4 +- .../doc/search/search_result_row.html | 4 +- 9 files changed, 79 insertions(+), 50 deletions(-) diff --git a/ietf/community/models.py b/ietf/community/models.py index 91629dcf46..2e40031768 100644 --- a/ietf/community/models.py +++ b/ietf/community/models.py @@ -30,7 +30,7 @@ def __str__(self): def get_absolute_url(self): import ietf.community.views if self.person: - return urlreverse(ietf.community.views.view_list, kwargs={ 'username': self.person.user.username }) + return urlreverse(ietf.community.views.view_list, kwargs={ 'email_or_name': self.person.email() }) elif self.group: return urlreverse("ietf.group.views.group_documents", kwargs={ 'acronym': self.group.acronym }) return "" diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 38e243f8d3..bd7029a749 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -92,7 +92,7 @@ def test_view_list(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.view_list, kwargs={ "username": "plain" }) + url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": person.email() }) # without list r = self.client.get(url) @@ -114,11 +114,11 @@ def test_view_list(self): def test_manage_personal_list(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') ad = Person.objects.get(user__username='ad') draft = WgDraftFactory(authors=[ad]) - url = urlreverse(ietf.community.views.manage_list, kwargs={ "username": "plain" }) + url = urlreverse(ietf.community.views.manage_list, kwargs={ "email_or_name": person.email() }) login_testing_unauthorized(self, "plain", url) page = self.app.get(url, user='plain') @@ -209,10 +209,10 @@ def test_manage_group_list(self): self.assertEqual(r.status_code, 200) def test_track_untrack_document(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.track_document, kwargs={ "username": "plain", "name": draft.name }) + url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) login_testing_unauthorized(self, "plain", url) # track @@ -225,7 +225,7 @@ def test_track_untrack_document(self): self.assertEqual(list(clist.added_docs.all()), [draft]) # untrack - url = urlreverse(ietf.community.views.untrack_document, kwargs={ "username": "plain", "name": draft.name }) + url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) r = self.client.get(url) self.assertEqual(r.status_code, 200) @@ -235,10 +235,10 @@ def test_track_untrack_document(self): self.assertEqual(list(clist.added_docs.all()), []) def test_track_untrack_document_through_ajax(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.track_document, kwargs={ "username": "plain", "name": draft.name }) + url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) login_testing_unauthorized(self, "plain", url) # track @@ -249,7 +249,7 @@ def test_track_untrack_document_through_ajax(self): self.assertEqual(list(clist.added_docs.all()), [draft]) # untrack - url = urlreverse(ietf.community.views.untrack_document, kwargs={ "username": "plain", "name": draft.name }) + url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') self.assertEqual(r.status_code, 200) self.assertEqual(r.json()["success"], True) @@ -260,7 +260,7 @@ def test_csv(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.export_to_csv, kwargs={ "username": "plain" }) + url = urlreverse(ietf.community.views.export_to_csv, kwargs={ "email_or_name": person.email() }) # without list r = self.client.get(url) @@ -296,7 +296,7 @@ def test_feed(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.feed, kwargs={ "username": "plain" }) + url = urlreverse(ietf.community.views.feed, kwargs={ "email_or_name": person.email() }) # without list r = self.client.get(url) @@ -336,7 +336,7 @@ def test_subscription(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.subscription, kwargs={ "username": "plain" }) + url = urlreverse(ietf.community.views.subscription, kwargs={ "email_or_name": person.email() }) login_testing_unauthorized(self, "plain", url) diff --git a/ietf/community/urls.py b/ietf/community/urls.py index f80547ffad..3ab132f2dc 100644 --- a/ietf/community/urls.py +++ b/ietf/community/urls.py @@ -4,11 +4,11 @@ from ietf.utils.urls import url urlpatterns = [ - url(r'^personal/(?P[^/]+)/$', views.view_list), - url(r'^personal/(?P[^/]+)/manage/$', views.manage_list), - url(r'^personal/(?P[^/]+)/trackdocument/(?P[^/]+)/$', views.track_document), - url(r'^personal/(?P[^/]+)/untrackdocument/(?P[^/]+)/$', views.untrack_document), - url(r'^personal/(?P[^/]+)/csv/$', views.export_to_csv), - url(r'^personal/(?P[^/]+)/feed/$', views.feed), - url(r'^personal/(?P[^/]+)/subscription/$', views.subscription), + url(r'^personal/(?P[^/]+)/$', views.view_list), + url(r'^personal/(?P[^/]+)/manage/$', views.manage_list), + url(r'^personal/(?P[^/]+)/trackdocument/(?P[^/]+)/$', views.track_document), + url(r'^personal/(?P[^/]+)/untrackdocument/(?P[^/]+)/$', views.untrack_document), + url(r'^personal/(?P[^/]+)/csv/$', views.export_to_csv), + url(r'^personal/(?P[^/]+)/feed/$', views.feed), + url(r'^personal/(?P[^/]+)/subscription/$', views.subscription), ] diff --git a/ietf/community/utils.py b/ietf/community/utils.py index 5beb2e2f67..79ada30fb3 100644 --- a/ietf/community/utils.py +++ b/ietf/community/utils.py @@ -13,8 +13,8 @@ from ietf.doc.models import Document, State from ietf.group.models import Role, Group from ietf.person.models import Person +from ietf.person.utils import lookup_persons from ietf.ietfauth.utils import has_role -from django.contrib.auth.models import User from django.shortcuts import get_object_or_404 from ietf.utils.mail import send_mail @@ -29,15 +29,22 @@ def states_of_significant_change(): Q(type="draft", slug__in=['rfc', 'dead']) ) -def lookup_community_list(username=None, acronym=None): - assert username or acronym +class MultiplePersonError(Exception): + """More than one Person record matches the given email or name""" + pass + +def lookup_community_list(email_or_name=None, acronym=None): + assert email_or_name or acronym if acronym: group = get_object_or_404(Group, acronym=acronym) clist = CommunityList.objects.filter(group=group).first() or CommunityList(group=group) else: - user = get_object_or_404(User, username__iexact=username) - clist = CommunityList.objects.filter(person__user=user).first() or CommunityList(person=user.person) + persons = lookup_persons(email_or_name) + if len(persons) > 1: + raise MultiplePersonError(r"\r\n".join([p.email() for p in persons])) + person = persons[0] + clist = CommunityList.objects.filter(person=person).first() or CommunityList(person=person) return clist diff --git a/ietf/community/views.py b/ietf/community/views.py index 85f78d931e..ce52b1e537 100644 --- a/ietf/community/views.py +++ b/ietf/community/views.py @@ -20,13 +20,17 @@ from ietf.community.utils import lookup_community_list, can_manage_community_list from ietf.community.utils import docs_tracked_by_community_list, docs_matching_community_list_rule from ietf.community.utils import states_of_significant_change, reset_name_contains_index_for_rule +from ietf.community.utils import MultiplePersonError from ietf.doc.models import DocEvent, Document from ietf.doc.utils_search import prepare_document_table from ietf.utils.http import is_ajax from ietf.utils.response import permission_denied -def view_list(request, username=None): - clist = lookup_community_list(username) +def view_list(request, email_or_name=None): + try: + clist = lookup_community_list(email_or_name) + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) docs = docs_tracked_by_community_list(clist) docs, meta = prepare_document_table(request, docs, request.GET) @@ -42,10 +46,13 @@ def view_list(request, username=None): }) @login_required -def manage_list(request, username=None, acronym=None, group_type=None): +def manage_list(request, email_or_name=None, acronym=None): # we need to be a bit careful because clist may not exist in the # database so we can't call related stuff on it yet - clist = lookup_community_list(username, acronym) + try: + clist = lookup_community_list(email_or_name, acronym) + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) if not can_manage_community_list(request.user, clist): permission_denied(request, "You do not have permission to access this view") @@ -129,11 +136,14 @@ def manage_list(request, username=None, acronym=None, group_type=None): @login_required -def track_document(request, name, username=None, acronym=None): +def track_document(request, name, email_or_name=None, acronym=None): doc = get_object_or_404(Document, docalias__name=name) if request.method == "POST": - clist = lookup_community_list(username, acronym) + try: + clist = lookup_community_list(email_or_name, acronym) + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) if not can_manage_community_list(request.user, clist): permission_denied(request, "You do not have permission to access this view") @@ -153,9 +163,12 @@ def track_document(request, name, username=None, acronym=None): }) @login_required -def untrack_document(request, name, username=None, acronym=None): +def untrack_document(request, name, email_or_name=None, acronym=None): doc = get_object_or_404(Document, docalias__name=name) - clist = lookup_community_list(username, acronym) + try: + clist = lookup_community_list(email_or_name, acronym) + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) if not can_manage_community_list(request.user, clist): permission_denied(request, "You do not have permission to access this view") @@ -173,8 +186,11 @@ def untrack_document(request, name, username=None, acronym=None): }) -def export_to_csv(request, username=None, acronym=None, group_type=None): - clist = lookup_community_list(username, acronym) +def export_to_csv(request, email_or_name=None, acronym=None): + try: + clist = lookup_community_list(email_or_name, acronym) + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) response = HttpResponse(content_type='text/csv') @@ -214,8 +230,11 @@ def export_to_csv(request, username=None, acronym=None, group_type=None): return response -def feed(request, username=None, acronym=None, group_type=None): - clist = lookup_community_list(username, acronym) +def feed(request, email_or_name=None, acronym=None): + try: + clist = lookup_community_list(email_or_name, acronym) + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) significant = request.GET.get('significant', '') == '1' @@ -250,10 +269,13 @@ def feed(request, username=None, acronym=None, group_type=None): @login_required -def subscription(request, username=None, acronym=None, group_type=None): - clist = lookup_community_list(username, acronym) - if clist.pk is None: - raise Http404 +def subscription(request, email_or_name=None, acronym=None): + try: + clist = lookup_community_list(email_or_name, acronym) + if clist.pk is None: + raise Http404 + except MultiplePersonError as err: + return HttpResponse(str(err), status=300) person = request.user.person diff --git a/ietf/templates/community/list_menu.html b/ietf/templates/community/list_menu.html index 58269dccb4..0552c76a45 100644 --- a/ietf/templates/community/list_menu.html +++ b/ietf/templates/community/list_menu.html @@ -3,18 +3,18 @@
+ href="{% if clist.group %}{% url "ietf.community.views.feed" acronym=clist.group.acronym %}{% else %}{% url "ietf.community.views.feed" email_or_name=clist.person.email_address %}{% endif %}"> All changes + href="{% if clist.group %}{% url "ietf.community.views.feed" acronym=clist.group.acronym %}{% else %}{% url "ietf.community.views.feed" email_or_name=clist.person.email_address %}{% endif %}?significant=1"> Significant
{% if clist.pk != None %} + href="{% if clist.group %}{% url "ietf.community.views.subscription" acronym=clist.group.acronym %}{% else %}{% url "ietf.community.views.subscription" email_or_name=clist.person.email_address %}{% endif %}"> {% if subscribed %} Change subscription @@ -24,7 +24,7 @@ {% endif %} + href="{% if clist.group %}{% url "ietf.community.views.export_to_csv" acronym=clist.group.acronym %}{% else %}{% url "ietf.community.views.export_to_csv" email_or_name=clist.person.email_address %}{% endif %}"> Export as CSV diff --git a/ietf/templates/community/view_list.html b/ietf/templates/community/view_list.html index 81f6d37454..7ca9f52748 100644 --- a/ietf/templates/community/view_list.html +++ b/ietf/templates/community/view_list.html @@ -12,7 +12,7 @@

{{ clist.long_name }}

{% bootstrap_messages %} {% if can_manage_list %} + href="{% url "ietf.community.views.manage_list" email_or_name=clist.person.email_address %}"> Manage list diff --git a/ietf/templates/doc/document_draft.html b/ietf/templates/doc/document_draft.html index 5db93a748c..a5d9ef7790 100644 --- a/ietf/templates/doc/document_draft.html +++ b/ietf/templates/doc/document_draft.html @@ -712,14 +712,14 @@ {% if user.is_authenticated %} Untrack diff --git a/ietf/templates/doc/search/search_result_row.html b/ietf/templates/doc/search/search_result_row.html index 5f956cb602..8b90934270 100644 --- a/ietf/templates/doc/search/search_result_row.html +++ b/ietf/templates/doc/search/search_result_row.html @@ -9,13 +9,13 @@ {% if user.is_authenticated %} - - From 4d003c9bc2c3ef866c2342c792b67bcb2aedc569 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Thu, 29 Jun 2023 10:58:09 -0400 Subject: [PATCH 3/8] test: Update tests to try all of the person's emails and aliases --- ietf/community/tests.py | 356 ++++++++++++++++++++++------------------ 1 file changed, 197 insertions(+), 159 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index bd7029a749..50fc02c033 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -18,12 +18,12 @@ from ietf.group.utils import setup_default_community_list_for_group from ietf.doc.models import State from ietf.doc.utils import add_state_change_event -from ietf.person.models import Person, Email +from ietf.person.models import Person, Email, Alias from ietf.utils.test_utils import login_testing_unauthorized from ietf.utils.mail import outbox from ietf.doc.factories import WgDraftFactory from ietf.group.factories import GroupFactory, RoleFactory -from ietf.person.factories import PersonFactory +from ietf.person.factories import PersonFactory, EmailFactory, AliasFactory class CommunityListTests(WebTest): def test_rule_matching(self): @@ -88,15 +88,26 @@ def test_rule_matching(self): # rule -> docs self.assertTrue(draft in list(docs_matching_community_list_rule(rule_group_exp))) + def complex_person(self, *args, **kwargs): + person = PersonFactory(*args, **kwargs) + EmailFactory(person=person) + AliasFactory(person=person) + return person + + def email_or_name_set(self, person): + return [e for e in Email.objects.filter(person=person)] + \ + [a for a in Alias.objects.filter(person=person)] + def test_view_list(self): - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": person.email() }) - # without list - r = self.client.get(url) - self.assertEqual(r.status_code, 200) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": id }) + #debug.show('url') + r = self.client.get(url) + self.assertEqual(r.status_code, 200) # with list clist = CommunityList.objects.create(person=person) @@ -108,80 +119,85 @@ def test_view_list(self): state=State.objects.get(type="draft", slug="active"), text="test", ) - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - self.assertContains(r, draft.name) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": id }) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + self.assertContains(r, draft.name) def test_manage_personal_list(self): - - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') ad = Person.objects.get(user__username='ad') draft = WgDraftFactory(authors=[ad]) url = urlreverse(ietf.community.views.manage_list, kwargs={ "email_or_name": person.email() }) login_testing_unauthorized(self, "plain", url) - page = self.app.get(url, user='plain') - self.assertEqual(page.status_int, 200) - - # add document - self.assertIn('add_document', page.forms) - form = page.forms['add_document'] - form['documents'].options=[(draft.pk, True, draft.name)] - page = form.submit('action',value='add_documents') - self.assertEqual(page.status_int, 302) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertTrue(clist.added_docs.filter(pk=draft.pk)) - page = page.follow() - - self.assertContains(page, draft.name) - - # remove document - self.assertIn('remove_document_%s' % draft.pk, page.forms) - form = page.forms['remove_document_%s' % draft.pk] - page = form.submit('action',value='remove_document') - self.assertEqual(page.status_int, 302) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertTrue(not clist.added_docs.filter(pk=draft.pk)) - page = page.follow() + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.manage_list, kwargs={ "email_or_name": id }) + #debug.show('id') + #debug.show('url') + page = self.app.get(url, user='plain') + self.assertEqual(page.status_int, 200) + + # add document + self.assertIn('add_document', page.forms) + form = page.forms['add_document'] + form['documents'].options=[(draft.pk, True, draft.name)] + page = form.submit('action',value='add_documents') + self.assertEqual(page.status_int, 302) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertTrue(clist.added_docs.filter(pk=draft.pk)) + page = page.follow() + + self.assertContains(page, draft.name) + + # remove document + self.assertIn('remove_document_%s' % draft.pk, page.forms) + form = page.forms['remove_document_%s' % draft.pk] + page = form.submit('action',value='remove_document') + self.assertEqual(page.status_int, 302) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertTrue(not clist.added_docs.filter(pk=draft.pk)) + page = page.follow() - # add rule - r = self.client.post(url, { - "action": "add_rule", - "rule_type": "author_rfc", - "author_rfc-person": Person.objects.filter(documentauthor__document=draft).first().pk, - "author_rfc-state": State.objects.get(type="draft", slug="rfc").pk, - }) - self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertTrue(clist.searchrule_set.filter(rule_type="author_rfc")) - - # add name_contains rule - r = self.client.post(url, { - "action": "add_rule", - "rule_type": "name_contains", - "name_contains-text": "draft.*mars", - "name_contains-state": State.objects.get(type="draft", slug="active").pk, - }) - self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertTrue(clist.searchrule_set.filter(rule_type="name_contains")) - - # rule shows up on GET - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - rule = clist.searchrule_set.filter(rule_type="author_rfc").first() - q = PyQuery(r.content) - self.assertEqual(len(q('#r%s' % rule.pk)), 1) + # add rule + r = self.client.post(url, { + "action": "add_rule", + "rule_type": "author_rfc", + "author_rfc-person": Person.objects.filter(documentauthor__document=draft).first().pk, + "author_rfc-state": State.objects.get(type="draft", slug="rfc").pk, + }) + self.assertEqual(r.status_code, 302) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertTrue(clist.searchrule_set.filter(rule_type="author_rfc")) + + # add name_contains rule + r = self.client.post(url, { + "action": "add_rule", + "rule_type": "name_contains", + "name_contains-text": "draft.*mars", + "name_contains-state": State.objects.get(type="draft", slug="active").pk, + }) + self.assertEqual(r.status_code, 302) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertTrue(clist.searchrule_set.filter(rule_type="name_contains")) + + # rule shows up on GET + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + rule = clist.searchrule_set.filter(rule_type="author_rfc").first() + q = PyQuery(r.content) + self.assertEqual(len(q('#r%s' % rule.pk)), 1) - # remove rule - r = self.client.post(url, { - "action": "remove_rule", - "rule": rule.pk, - }) + # remove rule + r = self.client.post(url, { + "action": "remove_rule", + "rule": rule.pk, + }) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertTrue(not clist.searchrule_set.filter(rule_type="author_rfc")) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertTrue(not clist.searchrule_set.filter(rule_type="author_rfc")) def test_manage_group_list(self): draft = WgDraftFactory(group__acronym='mars') @@ -203,83 +219,96 @@ def test_manage_group_list(self): p = PersonFactory() g.role_set.create(name_id={'area':'ad','program':'lead'}[gtype],person=p, email=p.email()) url = urlreverse(ietf.community.views.manage_list, kwargs={ "acronym": g.acronym }) + #debug.show('url') setup_default_community_list_for_group(g) self.client.login(username=p.user.username,password=p.user.username+"+password") r = self.client.get(url) self.assertEqual(r.status_code, 200) def test_track_untrack_document(self): - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) login_testing_unauthorized(self, "plain", url) - # track - r = self.client.get(url) - self.assertEqual(r.status_code, 200) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": id, "name": draft.name }) + #debug.show('url') + + # track + r = self.client.get(url) + self.assertEqual(r.status_code, 200) - r = self.client.post(url) - self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertEqual(list(clist.added_docs.all()), [draft]) + r = self.client.post(url) + self.assertEqual(r.status_code, 302) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertEqual(list(clist.added_docs.all()), [draft]) - # untrack - url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) - r = self.client.get(url) - self.assertEqual(r.status_code, 200) + # untrack + url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": id, "name": draft.name }) + #debug.show('url') + r = self.client.get(url) + self.assertEqual(r.status_code, 200) - r = self.client.post(url) - self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertEqual(list(clist.added_docs.all()), []) + r = self.client.post(url) + self.assertEqual(r.status_code, 302) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertEqual(list(clist.added_docs.all()), []) def test_track_untrack_document_through_ajax(self): - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) login_testing_unauthorized(self, "plain", url) - # track - r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') - self.assertEqual(r.status_code, 200) - self.assertEqual(r.json()["success"], True) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertEqual(list(clist.added_docs.all()), [draft]) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": id, "name": draft.name }) + #debug.show('url') - # untrack - url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": person.email(), "name": draft.name }) - r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') - self.assertEqual(r.status_code, 200) - self.assertEqual(r.json()["success"], True) - clist = CommunityList.objects.get(person__user__username="plain") - self.assertEqual(list(clist.added_docs.all()), []) + # track + r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') + self.assertEqual(r.status_code, 200) + self.assertEqual(r.json()["success"], True) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertEqual(list(clist.added_docs.all()), [draft]) + + # untrack + url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": id, "name": draft.name }) + #debug.show('url') + r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') + self.assertEqual(r.status_code, 200) + self.assertEqual(r.json()["success"], True) + clist = CommunityList.objects.get(person__user__username="plain") + self.assertEqual(list(clist.added_docs.all()), []) def test_csv(self): - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.export_to_csv, kwargs={ "email_or_name": person.email() }) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.export_to_csv, kwargs={ "email_or_name": id }) + #debug.show('url') - # without list - r = self.client.get(url) - self.assertEqual(r.status_code, 200) + # without list + r = self.client.get(url) + self.assertEqual(r.status_code, 200) - # with list - clist = CommunityList.objects.create(person=person) - if not draft in clist.added_docs.all(): - clist.added_docs.add(draft) - SearchRule.objects.create( - community_list=clist, - rule_type="name_contains", - state=State.objects.get(type="draft", slug="active"), - text="test", - ) - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - # this is a simple-minded test, we don't actually check the fields - self.assertContains(r, draft.name) + # with list + clist = CommunityList.objects.create(person=person) + if not draft in clist.added_docs.all(): + clist.added_docs.add(draft) + SearchRule.objects.create( + community_list=clist, + rule_type="name_contains", + state=State.objects.get(type="draft", slug="active"), + text="test", + ) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + # this is a simple-minded test, we don't actually check the fields + self.assertContains(r, draft.name) def test_csv_for_group(self): draft = WgDraftFactory() @@ -293,33 +322,35 @@ def test_csv_for_group(self): self.assertEqual(r.status_code, 200) def test_feed(self): - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') draft = WgDraftFactory() - url = urlreverse(ietf.community.views.feed, kwargs={ "email_or_name": person.email() }) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.feed, kwargs={ "email_or_name": id }) + #debug.show('url') - # without list - r = self.client.get(url) - self.assertEqual(r.status_code, 200) + # without list + r = self.client.get(url) + self.assertEqual(r.status_code, 200) - # with list - clist = CommunityList.objects.create(person=person) - if not draft in clist.added_docs.all(): - clist.added_docs.add(draft) - SearchRule.objects.create( - community_list=clist, - rule_type="name_contains", - state=State.objects.get(type="draft", slug="active"), - text="test", - ) - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - self.assertContains(r, draft.name) + # with list + clist = CommunityList.objects.create(person=person) + if not draft in clist.added_docs.all(): + clist.added_docs.add(draft) + SearchRule.objects.create( + community_list=clist, + rule_type="name_contains", + state=State.objects.get(type="draft", slug="active"), + text="test", + ) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + self.assertContains(r, draft.name) - # only significant - r = self.client.get(url + "?significant=1") - self.assertEqual(r.status_code, 200) - self.assertNotContains(r, '') + # only significant + r = self.client.get(url + "?significant=1") + self.assertEqual(r.status_code, 200) + self.assertNotContains(r, '') def test_feed_for_group(self): draft = WgDraftFactory() @@ -333,16 +364,19 @@ def test_feed_for_group(self): self.assertEqual(r.status_code, 200) def test_subscription(self): - person = PersonFactory(user__username='plain') + person = self.complex_person(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.subscription, kwargs={ "email_or_name": person.email() }) - login_testing_unauthorized(self, "plain", url) - # subscription without list - r = self.client.get(url) - self.assertEqual(r.status_code, 404) + for id in self.email_or_name_set(person): + url = urlreverse(ietf.community.views.subscription, kwargs={ "email_or_name": id }) + #debug.show('url') + + # subscription without list + r = self.client.get(url) + self.assertEqual(r.status_code, 404) # subscription with list clist = CommunityList.objects.create(person=person) @@ -354,22 +388,26 @@ def test_subscription(self): state=State.objects.get(type="draft", slug="active"), text="test", ) - r = self.client.get(url) - self.assertEqual(r.status_code, 200) - # subscribe - email = Email.objects.filter(person__user__username="plain").first() - r = self.client.post(url, { "email": email.pk, "notify_on": "significant", "action": "subscribe" }) - self.assertEqual(r.status_code, 302) + for email in Email.objects.filter(person=person): + url = urlreverse(ietf.community.views.subscription, kwargs={ "email_or_name": email }) + #debug.show('url') - subscription = EmailSubscription.objects.filter(community_list=clist, email=email, notify_on="significant").first() + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + + # subscribe + r = self.client.post(url, { "email": email.pk, "notify_on": "significant", "action": "subscribe" }) + self.assertEqual(r.status_code, 302) + + subscription = EmailSubscription.objects.filter(community_list=clist, email=email, notify_on="significant").first() - self.assertTrue(subscription) + self.assertTrue(subscription) - # delete subscription - r = self.client.post(url, { "subscription_id": subscription.pk, "action": "unsubscribe" }) - self.assertEqual(r.status_code, 302) - self.assertEqual(EmailSubscription.objects.filter(community_list=clist, email=email, notify_on="significant").count(), 0) + # delete subscription + r = self.client.post(url, { "subscription_id": subscription.pk, "action": "unsubscribe" }) + self.assertEqual(r.status_code, 302) + self.assertEqual(EmailSubscription.objects.filter(community_list=clist, email=email, notify_on="significant").count(), 0) def test_subscription_for_group(self): draft = WgDraftFactory(group__acronym='mars') @@ -384,7 +422,7 @@ def test_subscription_for_group(self): # test GET, rest is tested with personal list r = self.client.get(url) self.assertEqual(r.status_code, 200) - + def test_notification(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() From 4206a95611ce0b3ebdd8404b9e7941e1a72fd8a7 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Sun, 2 Jul 2023 18:03:59 -0400 Subject: [PATCH 4/8] fix: Recode a test case to avoid an exception if there's Unicode in the URL This only happens using the form-filling and submission feature of WebTest, which is only used in this one test case, so just it rip out. --- ietf/community/tests.py | 46 ++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 50fc02c033..7ca2ca9d7a 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -1,4 +1,4 @@ -# Copyright The IETF Trust 2016-2020, All Rights Reserved +# Copyright The IETF Trust 2016-2023, All Rights Reserved # -*- coding: utf-8 -*- @@ -6,8 +6,6 @@ from django.urls import reverse as urlreverse -from django_webtest import WebTest - import debug # pyflakes:ignore from ietf.community.models import CommunityList, SearchRule, EmailSubscription @@ -19,13 +17,13 @@ from ietf.doc.models import State from ietf.doc.utils import add_state_change_event from ietf.person.models import Person, Email, Alias -from ietf.utils.test_utils import login_testing_unauthorized +from ietf.utils.test_utils import TestCase, login_testing_unauthorized from ietf.utils.mail import outbox from ietf.doc.factories import WgDraftFactory from ietf.group.factories import GroupFactory, RoleFactory from ietf.person.factories import PersonFactory, EmailFactory, AliasFactory -class CommunityListTests(WebTest): +class CommunityListTests(TestCase): def test_rule_matching(self): plain = PersonFactory(user__username='plain') ad = Person.objects.get(user__username='ad') @@ -137,30 +135,36 @@ def test_manage_personal_list(self): url = urlreverse(ietf.community.views.manage_list, kwargs={ "email_or_name": id }) #debug.show('id') #debug.show('url') - page = self.app.get(url, user='plain') - self.assertEqual(page.status_int, 200) + r = self.client.get(url, user='plain') + self.assertEqual(r.status_code, 200) + + # We can't call post() with follow=True because that 404's if + # the url contains unicode. The view redirects to url='', so + # django.test.client._handle_redirects() is somehow failing to + # properly reconstruct the url. + # Whatever, follow the redirect manually. + def follow(r): + redirect_url = r.url or url + return self.client.get(redirect_url, user='plain') # add document - self.assertIn('add_document', page.forms) - form = page.forms['add_document'] - form['documents'].options=[(draft.pk, True, draft.name)] - page = form.submit('action',value='add_documents') - self.assertEqual(page.status_int, 302) + self.assertContains(r, 'add_document') + r = self.client.post(url, {'action': 'add_documents', 'documents': draft.pk}) + self.assertEqual(r.status_code, 302) clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.added_docs.filter(pk=draft.pk)) - page = page.follow() - - self.assertContains(page, draft.name) + r = follow(r) + self.assertContains(r, draft.name, status_code=200) # remove document - self.assertIn('remove_document_%s' % draft.pk, page.forms) - form = page.forms['remove_document_%s' % draft.pk] - page = form.submit('action',value='remove_document') - self.assertEqual(page.status_int, 302) + self.assertContains(r, 'remove_document_%s' % draft.pk) + r = self.client.post(url, {'action': 'remove_document', 'document': draft.pk}) + self.assertEqual(r.status_code, 302) clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(not clist.added_docs.filter(pk=draft.pk)) - page = page.follow() - + r = follow(r) + self.assertNotContains(r, draft.name, status_code=200) + # add rule r = self.client.post(url, { "action": "add_rule", From 4c22b7f91da7f4716dfd92100eac405241111bd4 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Tue, 4 Jul 2023 12:49:15 -0400 Subject: [PATCH 5/8] test: Add duplicate-person tests --- ietf/community/tests.py | 10 ++++++++++ ietf/community/utils.py | 2 +- ietf/person/tests.py | 10 ++++++++++ ietf/person/views.py | 2 +- 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 7ca2ca9d7a..daf34f2161 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -86,6 +86,16 @@ def test_rule_matching(self): # rule -> docs self.assertTrue(draft in list(docs_matching_community_list_rule(rule_group_exp))) + def test_view_list_duplicates(self): + person = PersonFactory(name="John Q. Public", user__username="bazquux@example.com") + PersonFactory(name="John Q. Public", user__username="foobar@example.com") + + url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": person.plain_name()}) + r = self.client.get(url) + self.assertEqual(r.status_code, 300) + self.assertIn("bazquux@example.com", r.content.decode()) + self.assertIn("foobar@example.com", r.content.decode()) + def complex_person(self, *args, **kwargs): person = PersonFactory(*args, **kwargs) EmailFactory(person=person) diff --git a/ietf/community/utils.py b/ietf/community/utils.py index 79ada30fb3..31b33c2e6c 100644 --- a/ietf/community/utils.py +++ b/ietf/community/utils.py @@ -42,7 +42,7 @@ def lookup_community_list(email_or_name=None, acronym=None): else: persons = lookup_persons(email_or_name) if len(persons) > 1: - raise MultiplePersonError(r"\r\n".join([p.email() for p in persons])) + raise MultiplePersonError(r"\r\n".join([p.user.username for p in persons])) person = persons[0] clist = CommunityList.objects.filter(person=person).first() or CommunityList(person=person) diff --git a/ietf/person/tests.py b/ietf/person/tests.py index a91527ea66..5d95a05965 100644 --- a/ietf/person/tests.py +++ b/ietf/person/tests.py @@ -157,6 +157,16 @@ def test_person_photo(self): img = Image.open(BytesIO(r.content)) self.assertEqual(img.width, 200) + def test_person_photo_duplicates(self): + person = PersonFactory(name="bazquux@example.com", user__username="bazquux@example.com", with_bio=True) + PersonFactory(name="bazquux@example.com", user__username="foobar@example.com", with_bio=True) + + url = urlreverse("ietf.person.views.photo", kwargs={ "email_or_name": person.plain_name()}) + r = self.client.get(url) + self.assertEqual(r.status_code, 300) + self.assertIn("bazquux@example.com", r.content.decode()) + self.assertIn("foobar@example.com", r.content.decode()) + def test_name_methods(self): person = PersonFactory(name="Dr. Jens F. Möller", ) diff --git a/ietf/person/views.py b/ietf/person/views.py index 7280992443..6d9daf4a81 100644 --- a/ietf/person/views.py +++ b/ietf/person/views.py @@ -76,7 +76,7 @@ def profile(request, email_or_name): def photo(request, email_or_name): persons = lookup_persons(email_or_name) if len(persons) > 1: - return HttpResponse(r"\r\n".join([p.email() for p in persons]), status=300) + return HttpResponse(r"\r\n".join([p.user.username for p in persons]), status=300) person = persons[0] if not person.photo: raise Http404("No photo found") From 9a8d332fe63c16bfc75869929f0a4728f606ca43 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Tue, 4 Jul 2023 17:44:48 -0400 Subject: [PATCH 6/8] fix: If there are multiple matching users, prefer the logged-in one. --- ietf/community/utils.py | 12 ++++++++---- ietf/community/views.py | 16 ++++++++-------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/ietf/community/utils.py b/ietf/community/utils.py index 31b33c2e6c..485a566418 100644 --- a/ietf/community/utils.py +++ b/ietf/community/utils.py @@ -1,4 +1,4 @@ -# Copyright The IETF Trust 2016-2020, All Rights Reserved +# Copyright The IETF Trust 2016-2023, All Rights Reserved # -*- coding: utf-8 -*- @@ -33,7 +33,7 @@ class MultiplePersonError(Exception): """More than one Person record matches the given email or name""" pass -def lookup_community_list(email_or_name=None, acronym=None): +def lookup_community_list(request, email_or_name=None, acronym=None): assert email_or_name or acronym if acronym: @@ -42,8 +42,12 @@ def lookup_community_list(email_or_name=None, acronym=None): else: persons = lookup_persons(email_or_name) if len(persons) > 1: - raise MultiplePersonError(r"\r\n".join([p.user.username for p in persons])) - person = persons[0] + if hasattr(request.user, 'person') and request.user.person in persons: + person = request.user.person + else: + raise MultiplePersonError("\r\n".join([p.user.username for p in persons])) + else: + person = persons[0] clist = CommunityList.objects.filter(person=person).first() or CommunityList(person=person) return clist diff --git a/ietf/community/views.py b/ietf/community/views.py index ce52b1e537..ae28243327 100644 --- a/ietf/community/views.py +++ b/ietf/community/views.py @@ -1,4 +1,4 @@ -# Copyright The IETF Trust 2012-2020, All Rights Reserved +# Copyright The IETF Trust 2012-2023, All Rights Reserved # -*- coding: utf-8 -*- @@ -28,7 +28,7 @@ def view_list(request, email_or_name=None): try: - clist = lookup_community_list(email_or_name) + clist = lookup_community_list(request, email_or_name) except MultiplePersonError as err: return HttpResponse(str(err), status=300) @@ -50,7 +50,7 @@ def manage_list(request, email_or_name=None, acronym=None): # we need to be a bit careful because clist may not exist in the # database so we can't call related stuff on it yet try: - clist = lookup_community_list(email_or_name, acronym) + clist = lookup_community_list(request, email_or_name, acronym) except MultiplePersonError as err: return HttpResponse(str(err), status=300) @@ -141,7 +141,7 @@ def track_document(request, name, email_or_name=None, acronym=None): if request.method == "POST": try: - clist = lookup_community_list(email_or_name, acronym) + clist = lookup_community_list(request, email_or_name, acronym) except MultiplePersonError as err: return HttpResponse(str(err), status=300) if not can_manage_community_list(request.user, clist): @@ -166,7 +166,7 @@ def track_document(request, name, email_or_name=None, acronym=None): def untrack_document(request, name, email_or_name=None, acronym=None): doc = get_object_or_404(Document, docalias__name=name) try: - clist = lookup_community_list(email_or_name, acronym) + clist = lookup_community_list(request, email_or_name, acronym) except MultiplePersonError as err: return HttpResponse(str(err), status=300) if not can_manage_community_list(request.user, clist): @@ -188,7 +188,7 @@ def untrack_document(request, name, email_or_name=None, acronym=None): def export_to_csv(request, email_or_name=None, acronym=None): try: - clist = lookup_community_list(email_or_name, acronym) + clist = lookup_community_list(request, email_or_name, acronym) except MultiplePersonError as err: return HttpResponse(str(err), status=300) @@ -232,7 +232,7 @@ def export_to_csv(request, email_or_name=None, acronym=None): def feed(request, email_or_name=None, acronym=None): try: - clist = lookup_community_list(email_or_name, acronym) + clist = lookup_community_list(request, email_or_name, acronym) except MultiplePersonError as err: return HttpResponse(str(err), status=300) @@ -271,7 +271,7 @@ def feed(request, email_or_name=None, acronym=None): @login_required def subscription(request, email_or_name=None, acronym=None): try: - clist = lookup_community_list(email_or_name, acronym) + clist = lookup_community_list(request, email_or_name, acronym) if clist.pk is None: raise Http404 except MultiplePersonError as err: From 1eb6011a21804c8564cbac04acb9d45106e31988 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 5 Jul 2023 10:00:01 -0400 Subject: [PATCH 7/8] chore: We no longer use WebTest, so don't include it. --- requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 86b4e8c6d8..f6d5bdc6f2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,7 +25,6 @@ django-simple-history>=3.0.0 django-stubs>=4.2.0 # The django-stubs version used determines the the mypy version indicated below django-tastypie>=0.14.5 # Version must be locked in sync with version of Django django-vite>=2.0.2 -django-webtest>=1.9.10 # Only used in tests django-widget-tweaks>=1.4.12 djlint>=1.0.0 # To auto-indent templates via "djlint --profile django --reformat" docutils>=0.18.1 # Used only by dbtemplates for RestructuredText From 9c5dd2a6eaf12ea47f4cf423fb865601a61c40f3 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Fri, 7 Jul 2023 14:16:02 -0400 Subject: [PATCH 8/8] fix: Address review comments --- ietf/community/tests.py | 58 ++++++++++++++++------------------------- ietf/community/utils.py | 27 +------------------ ietf/community/views.py | 30 ++++++++++++++++++--- 3 files changed, 50 insertions(+), 65 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index daf34f2161..8d6662d155 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -113,9 +113,8 @@ def test_view_list(self): # without list for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": id }) - #debug.show('url') r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") # with list clist = CommunityList.objects.create(person=person) @@ -130,7 +129,7 @@ def test_view_list(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.view_list, kwargs={ "email_or_name": id }) r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") self.assertContains(r, draft.name) def test_manage_personal_list(self): @@ -143,16 +142,12 @@ def test_manage_personal_list(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.manage_list, kwargs={ "email_or_name": id }) - #debug.show('id') - #debug.show('url') r = self.client.get(url, user='plain') - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") # We can't call post() with follow=True because that 404's if - # the url contains unicode. The view redirects to url='', so - # django.test.client._handle_redirects() is somehow failing to - # properly reconstruct the url. - # Whatever, follow the redirect manually. + # the url contains unicode, because the django test client + # apparently re-encodes the already-encoded url. def follow(r): redirect_url = r.url or url return self.client.get(redirect_url, user='plain') @@ -160,7 +155,7 @@ def follow(r): # add document self.assertContains(r, 'add_document') r = self.client.post(url, {'action': 'add_documents', 'documents': draft.pk}) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 302, msg=f"id='{id}', url='{url}'") clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.added_docs.filter(pk=draft.pk)) r = follow(r) @@ -169,7 +164,7 @@ def follow(r): # remove document self.assertContains(r, 'remove_document_%s' % draft.pk) r = self.client.post(url, {'action': 'remove_document', 'document': draft.pk}) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 302, msg=f"id='{id}', url='{url}'") clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(not clist.added_docs.filter(pk=draft.pk)) r = follow(r) @@ -182,7 +177,7 @@ def follow(r): "author_rfc-person": Person.objects.filter(documentauthor__document=draft).first().pk, "author_rfc-state": State.objects.get(type="draft", slug="rfc").pk, }) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 302, msg=f"id='{id}', url='{url}'") clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.searchrule_set.filter(rule_type="author_rfc")) @@ -193,13 +188,13 @@ def follow(r): "name_contains-text": "draft.*mars", "name_contains-state": State.objects.get(type="draft", slug="active").pk, }) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 302, msg=f"id='{id}', url='{url}'") clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.searchrule_set.filter(rule_type="name_contains")) # rule shows up on GET r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") rule = clist.searchrule_set.filter(rule_type="author_rfc").first() q = PyQuery(r.content) self.assertEqual(len(q('#r%s' % rule.pk)), 1) @@ -233,7 +228,6 @@ def test_manage_group_list(self): p = PersonFactory() g.role_set.create(name_id={'area':'ad','program':'lead'}[gtype],person=p, email=p.email()) url = urlreverse(ietf.community.views.manage_list, kwargs={ "acronym": g.acronym }) - #debug.show('url') setup_default_community_list_for_group(g) self.client.login(username=p.user.username,password=p.user.username+"+password") r = self.client.get(url) @@ -248,25 +242,23 @@ def test_track_untrack_document(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": id, "name": draft.name }) - #debug.show('url') # track r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") r = self.client.post(url) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 302, msg=f"id='{id}', url='{url}'") clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), [draft]) # untrack url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": id, "name": draft.name }) - #debug.show('url') r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") r = self.client.post(url) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 302, msg=f"id='{id}', url='{url}'") clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), []) @@ -279,20 +271,18 @@ def test_track_untrack_document_through_ajax(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.track_document, kwargs={ "email_or_name": id, "name": draft.name }) - #debug.show('url') # track r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") self.assertEqual(r.json()["success"], True) clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), [draft]) # untrack url = urlreverse(ietf.community.views.untrack_document, kwargs={ "email_or_name": id, "name": draft.name }) - #debug.show('url') r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") self.assertEqual(r.json()["success"], True) clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), []) @@ -303,11 +293,10 @@ def test_csv(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.export_to_csv, kwargs={ "email_or_name": id }) - #debug.show('url') # without list r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") # with list clist = CommunityList.objects.create(person=person) @@ -320,7 +309,7 @@ def test_csv(self): text="test", ) r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") # this is a simple-minded test, we don't actually check the fields self.assertContains(r, draft.name) @@ -341,11 +330,10 @@ def test_feed(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.feed, kwargs={ "email_or_name": id }) - #debug.show('url') # without list r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") # with list clist = CommunityList.objects.create(person=person) @@ -358,12 +346,12 @@ def test_feed(self): text="test", ) r = self.client.get(url) - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") self.assertContains(r, draft.name) # only significant r = self.client.get(url + "?significant=1") - self.assertEqual(r.status_code, 200) + self.assertEqual(r.status_code, 200, msg=f"id='{id}', url='{url}'") self.assertNotContains(r, '') def test_feed_for_group(self): @@ -386,11 +374,10 @@ def test_subscription(self): for id in self.email_or_name_set(person): url = urlreverse(ietf.community.views.subscription, kwargs={ "email_or_name": id }) - #debug.show('url') # subscription without list r = self.client.get(url) - self.assertEqual(r.status_code, 404) + self.assertEqual(r.status_code, 404, msg=f"id='{id}', url='{url}'") # subscription with list clist = CommunityList.objects.create(person=person) @@ -405,7 +392,6 @@ def test_subscription(self): for email in Email.objects.filter(person=person): url = urlreverse(ietf.community.views.subscription, kwargs={ "email_or_name": email }) - #debug.show('url') r = self.client.get(url) self.assertEqual(r.status_code, 200) diff --git a/ietf/community/utils.py b/ietf/community/utils.py index 485a566418..4de95c9e0b 100644 --- a/ietf/community/utils.py +++ b/ietf/community/utils.py @@ -11,11 +11,9 @@ from ietf.community.models import CommunityList, EmailSubscription, SearchRule from ietf.doc.models import Document, State -from ietf.group.models import Role, Group +from ietf.group.models import Role from ietf.person.models import Person -from ietf.person.utils import lookup_persons from ietf.ietfauth.utils import has_role -from django.shortcuts import get_object_or_404 from ietf.utils.mail import send_mail @@ -29,29 +27,6 @@ def states_of_significant_change(): Q(type="draft", slug__in=['rfc', 'dead']) ) -class MultiplePersonError(Exception): - """More than one Person record matches the given email or name""" - pass - -def lookup_community_list(request, email_or_name=None, acronym=None): - assert email_or_name or acronym - - if acronym: - group = get_object_or_404(Group, acronym=acronym) - clist = CommunityList.objects.filter(group=group).first() or CommunityList(group=group) - else: - persons = lookup_persons(email_or_name) - if len(persons) > 1: - if hasattr(request.user, 'person') and request.user.person in persons: - person = request.user.person - else: - raise MultiplePersonError("\r\n".join([p.user.username for p in persons])) - else: - person = persons[0] - clist = CommunityList.objects.filter(person=person).first() or CommunityList(person=person) - - return clist - def can_manage_community_list(user, clist): if not user or not user.is_authenticated: return False diff --git a/ietf/community/views.py b/ietf/community/views.py index ae28243327..737094e006 100644 --- a/ietf/community/views.py +++ b/ietf/community/views.py @@ -15,17 +15,41 @@ import debug # pyflakes:ignore -from ietf.community.models import SearchRule, EmailSubscription +from ietf.community.models import CommunityList, EmailSubscription, SearchRule from ietf.community.forms import SearchRuleTypeForm, SearchRuleForm, AddDocumentsForm, SubscriptionForm -from ietf.community.utils import lookup_community_list, can_manage_community_list +from ietf.community.utils import can_manage_community_list from ietf.community.utils import docs_tracked_by_community_list, docs_matching_community_list_rule from ietf.community.utils import states_of_significant_change, reset_name_contains_index_for_rule -from ietf.community.utils import MultiplePersonError +from ietf.group.models import Group from ietf.doc.models import DocEvent, Document from ietf.doc.utils_search import prepare_document_table +from ietf.person.utils import lookup_persons from ietf.utils.http import is_ajax from ietf.utils.response import permission_denied +class MultiplePersonError(Exception): + """More than one Person record matches the given email or name""" + pass + +def lookup_community_list(request, email_or_name=None, acronym=None): + assert email_or_name or acronym + + if acronym: + group = get_object_or_404(Group, acronym=acronym) + clist = CommunityList.objects.filter(group=group).first() or CommunityList(group=group) + else: + persons = lookup_persons(email_or_name) + if len(persons) > 1: + if hasattr(request.user, 'person') and request.user.person in persons: + person = request.user.person + else: + raise MultiplePersonError("\r\n".join([p.user.username for p in persons])) + else: + person = persons[0] + clist = CommunityList.objects.filter(person=person).first() or CommunityList(person=person) + + return clist + def view_list(request, email_or_name=None): try: clist = lookup_community_list(request, email_or_name)