fix: 404 instead of 300 for ambiguous email_or_person (#8004)

* fix: 404 on CommunityList name collision * fix: 404 on ambiuous person for photo() view * test: update tests --------- Co-authored-by: Robert Sparks <[email protected]>
ietf-tools · Oct 16, 2024 · a338df1 · a338df1
1 parent db4ff66
commit a338df1
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 45 deletions.
diff --git a/ietf/community/tests.py b/ietf/community/tests.py
@@ -95,9 +95,7 @@ def test_view_list_duplicates(self):
 
         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("[email protected]", r.content.decode())
-        self.assertIn("[email protected]", r.content.decode())
+        self.assertEqual(r.status_code, 404)
 
     def complex_person(self, *args, **kwargs):
         person = PersonFactory(*args, **kwargs)

diff --git a/ietf/community/views.py b/ietf/community/views.py
@@ -28,11 +28,14 @@
 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):
+    """Finds a CommunityList for a person or group
+    
+    Instantiates an unsaved CommunityList if one is not found.
+    
+    If the person or group cannot be found and uniquely identified, raises an Http404 exception
+    """
     assert email_or_name or acronym
 
     if acronym:
@@ -44,19 +47,14 @@ def lookup_community_list(request, email_or_name=None, acronym=None):
             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]))
+                raise Http404(f"Unable to identify the CommunityList for {email_or_name}")
         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)
-    except MultiplePersonError as err:
-        return HttpResponse(str(err), status=300)
-
+    clist = lookup_community_list(request, email_or_name)  # may raise Http404
     docs = docs_tracked_by_community_list(clist)
     docs, meta = prepare_document_table(request, docs, request.GET)
 
@@ -76,10 +74,7 @@ def view_list(request, email_or_name=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
-    try:
-        clist = lookup_community_list(request, email_or_name, acronym)
-    except MultiplePersonError as err:
-        return HttpResponse(str(err), status=300)
+    clist = lookup_community_list(request, email_or_name, acronym)  # may raise Http404
 
     if not can_manage_community_list(request.user, clist):
         permission_denied(request, "You do not have permission to access this view")
@@ -166,10 +161,7 @@ def track_document(request, name, email_or_name=None, acronym=None):
     doc = get_object_or_404(Document, name=name)
 
     if request.method == "POST":
-        try:
-            clist = lookup_community_list(request, email_or_name, acronym)
-        except MultiplePersonError as err:
-            return HttpResponse(str(err), status=300)
+        clist = lookup_community_list(request, email_or_name, acronym)  # may raise Http404
         if not can_manage_community_list(request.user, clist):
             permission_denied(request, "You do not have permission to access this view")
 
@@ -191,10 +183,7 @@ def track_document(request, name, email_or_name=None, acronym=None):
 @login_required
 def untrack_document(request, name, email_or_name=None, acronym=None):
     doc = get_object_or_404(Document, name=name)
-    try:
-        clist = lookup_community_list(request, email_or_name, acronym)
-    except MultiplePersonError as err:
-        return HttpResponse(str(err), status=300)
+    clist = lookup_community_list(request, email_or_name, acronym)  # may raise Http404
     if not can_manage_community_list(request.user, clist):
         permission_denied(request, "You do not have permission to access this view")
 
@@ -214,11 +203,7 @@ def untrack_document(request, name, email_or_name=None, acronym=None):
 
 @ignore_view_kwargs("group_type")
 def export_to_csv(request, email_or_name=None, acronym=None):
-    try:
-        clist = lookup_community_list(request, email_or_name, acronym)
-    except MultiplePersonError as err:
-        return HttpResponse(str(err), status=300)
-
+    clist = lookup_community_list(request, email_or_name, acronym)  # may raise Http404
     response = HttpResponse(content_type='text/csv')
 
     if clist.group:
@@ -259,11 +244,7 @@ def export_to_csv(request, email_or_name=None, acronym=None):
 
 @ignore_view_kwargs("group_type")
 def feed(request, email_or_name=None, acronym=None):
-    try:
-        clist = lookup_community_list(request, email_or_name, acronym)
-    except MultiplePersonError as err:
-        return HttpResponse(str(err), status=300)
-
+    clist = lookup_community_list(request, email_or_name, acronym)  # may raise Http404
     significant = request.GET.get('significant', '') == '1'
 
     documents = docs_tracked_by_community_list(clist).values_list('pk', flat=True)
@@ -299,12 +280,9 @@ def feed(request, email_or_name=None, acronym=None):
 @login_required
 @ignore_view_kwargs("group_type")
 def subscription(request, email_or_name=None, acronym=None):
-    try:
-        clist = lookup_community_list(request, email_or_name, acronym)
-        if clist.pk is None:
-            raise Http404
-    except MultiplePersonError as err:
-        return HttpResponse(str(err), status=300)
+    clist = lookup_community_list(request, email_or_name, acronym)  # may raise Http404
+    if clist.pk is None:
+        raise Http404
 
     person = request.user.person
 

diff --git a/ietf/person/tests.py b/ietf/person/tests.py
@@ -173,9 +173,7 @@ def test_person_photo_duplicates(self):
 
         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("[email protected]", r.content.decode())
-        self.assertIn("[email protected]", r.content.decode())
+        self.assertEqual(r.status_code, 404)
 
     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
@@ -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.user.username for p in persons]), status=300)
+        raise Http404("No photo found")
     person = persons[0]
     if not person.photo:
         raise Http404("No photo found")