From 24cd6a7721f14fda8b13e0c8695472f2aaf3aa3a Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Tue, 20 Jun 2023 13:51:52 -0400 Subject: [PATCH 01/13] refactor: change references from User to Person (#5821) * refactor: Change CommunityList reference from User to Person * refactor: Convert more user references to person * refactor: Change augment_docs_and_user_with_user_info to person * refactor: Change Nomination and Feedback references from User to Person * refactor: Change a few test case function signatures to be more pythonic --- ietf/api/views.py | 2 +- ietf/community/admin.py | 4 +- ietf/community/forms.py | 5 +- .../migrations/0003_user_to_person.py | 54 +++++++++++ ietf/community/models.py | 11 +-- ietf/community/tests.py | 43 +++++---- ietf/community/utils.py | 6 +- ietf/community/views.py | 8 +- ietf/doc/utils.py | 31 +++---- ietf/doc/utils_search.py | 5 +- ietf/doc/views_doc.py | 5 +- ietf/nomcom/admin.py | 8 +- ietf/nomcom/factories.py | 6 +- ietf/nomcom/forms.py | 48 +++++----- ietf/nomcom/migrations/0004_user_to_person.py | 93 +++++++++++++++++++ ietf/nomcom/models.py | 5 +- ietf/nomcom/resources.py | 12 +-- ietf/nomcom/tests.py | 60 +++++------- ietf/nomcom/utils.py | 27 +++--- ietf/nomcom/views.py | 73 ++++++++------- ietf/person/tests.py | 33 +++---- ietf/person/utils.py | 30 +++--- ietf/secr/rolodex/views.py | 2 - ietf/templates/community/list_menu.html | 10 +- ietf/templates/community/view_list.html | 4 +- ietf/templates/doc/document_draft.html | 6 +- .../doc/search/search_result_row.html | 6 +- 27 files changed, 359 insertions(+), 238 deletions(-) create mode 100644 ietf/community/migrations/0003_user_to_person.py create mode 100644 ietf/nomcom/migrations/0004_user_to_person.py diff --git a/ietf/api/views.py b/ietf/api/views.py index b56544c436..a63caa255e 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -89,7 +89,7 @@ def get(self, request): 'sendqueue', 'nominee', 'topicfeedbacklastseen', 'alias', 'email', 'apikeys', 'personevent', 'reviewersettings', 'reviewsecretarysettings', 'unavailableperiod', 'reviewwish', 'nextreviewerinteam', 'reviewrequest', 'meetingregistration', 'submissionevent', 'preapproval', - 'user', 'user__communitylist', 'personextresource_set', ] + 'user', 'communitylist', 'personextresource_set', ] return self.json_view(request, filter={'id':person.id}, expand=expand) diff --git a/ietf/community/admin.py b/ietf/community/admin.py index 890819d9d9..4c947ad3f7 100644 --- a/ietf/community/admin.py +++ b/ietf/community/admin.py @@ -7,8 +7,8 @@ from ietf.community.models import CommunityList, SearchRule, EmailSubscription class CommunityListAdmin(admin.ModelAdmin): - list_display = ['id', 'user', 'group'] - raw_id_fields = ['user', 'group', 'added_docs'] + list_display = ['id', 'person', 'group'] + raw_id_fields = ['person', 'group', 'added_docs'] admin.site.register(CommunityList, CommunityListAdmin) class SearchRuleAdmin(admin.ModelAdmin): diff --git a/ietf/community/forms.py b/ietf/community/forms.py index 8d72ce0d70..e73b0d1ebf 100644 --- a/ietf/community/forms.py +++ b/ietf/community/forms.py @@ -106,14 +106,13 @@ def clean_text(self): class SubscriptionForm(forms.ModelForm): - def __init__(self, user, clist, *args, **kwargs): + def __init__(self, person, clist, *args, **kwargs): self.clist = clist - self.user = user super(SubscriptionForm, self).__init__(*args, **kwargs) self.fields["notify_on"].widget = forms.RadioSelect(choices=self.fields["notify_on"].choices) - self.fields["email"].queryset = self.fields["email"].queryset.filter(person__user=user, active=True).order_by("-primary") + self.fields["email"].queryset = self.fields["email"].queryset.filter(person=person, active=True).order_by("-primary") self.fields["email"].widget = forms.RadioSelect(choices=[t for t in self.fields["email"].choices if t[0]]) if self.fields["email"].queryset: diff --git a/ietf/community/migrations/0003_user_to_person.py b/ietf/community/migrations/0003_user_to_person.py new file mode 100644 index 0000000000..b626c23513 --- /dev/null +++ b/ietf/community/migrations/0003_user_to_person.py @@ -0,0 +1,54 @@ +# Generated by Django 4.2.2 on 2023-06-12 19:35 + +from django.conf import settings +from django.db import migrations +import django.db.models.deletion +import ietf.utils.models + + +def forward(apps, schema_editor): + CommunityList = apps.get_model('community', 'CommunityList') + for clist in CommunityList.objects.all(): + try: + clist.person = clist.user.person + except: + clist.person = None + clist.save() + +def reverse(apps, schema_editor): + CommunityList = apps.get_model('community', 'CommunityList') + for clist in CommunityList.objects.all(): + try: + clist.user = clist.person.user + except: + clist.user = None + clist.save() + +class Migration(migrations.Migration): + dependencies = [ + ("community", "0002_auto_20230320_1222"), + ("person", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="communitylist", + name="person", + field=ietf.utils.models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="person.Person", + ), + ), + migrations.RunPython(forward, reverse), + migrations.RemoveField( + model_name="communitylist", + name="user", + field=ietf.utils.models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to=settings.AUTH_USER_MODEL), + ), + ] diff --git a/ietf/community/models.py b/ietf/community/models.py index 9b2383f211..91629dcf46 100644 --- a/ietf/community/models.py +++ b/ietf/community/models.py @@ -2,7 +2,6 @@ # -*- coding: utf-8 -*- -from django.contrib.auth.models import User from django.db import models from django.db.models import signals from django.urls import reverse as urlreverse @@ -13,13 +12,13 @@ from ietf.utils.models import ForeignKey class CommunityList(models.Model): - user = ForeignKey(User, blank=True, null=True) + person = ForeignKey(Person, blank=True, null=True) group = ForeignKey(Group, blank=True, null=True) added_docs = models.ManyToManyField(Document) def long_name(self): - if self.user: - return 'Personal I-D list of %s' % self.user.username + if self.person: + return 'Personal I-D list of %s' % self.person.plain_name() elif self.group: return 'I-D list for %s' % self.group.name else: @@ -30,8 +29,8 @@ def __str__(self): def get_absolute_url(self): import ietf.community.views - if self.user: - return urlreverse(ietf.community.views.view_list, kwargs={ 'username': self.user.username }) + if self.person: + return urlreverse(ietf.community.views.view_list, kwargs={ 'username': self.person.user.username }) 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 3dd86f70e3..38e243f8d3 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -5,7 +5,6 @@ from pyquery import PyQuery from django.urls import reverse as urlreverse -from django.contrib.auth.models import User from django_webtest import WebTest @@ -38,7 +37,7 @@ def test_rule_matching(self): states=[('draft-iesg','lc'),('draft','active')], ) - clist = CommunityList.objects.create(user=User.objects.get(username="plain")) + clist = CommunityList.objects.create(person=plain) rule_group = SearchRule.objects.create(rule_type="group", group=draft.group, state=State.objects.get(type="draft", slug="active"), community_list=clist) rule_group_rfc = SearchRule.objects.create(rule_type="group_rfc", group=draft.group, state=State.objects.get(type="draft", slug="rfc"), community_list=clist) @@ -90,7 +89,7 @@ def test_rule_matching(self): self.assertTrue(draft in list(docs_matching_community_list_rule(rule_group_exp))) def test_view_list(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.view_list, kwargs={ "username": "plain" }) @@ -100,7 +99,7 @@ def test_view_list(self): self.assertEqual(r.status_code, 200) # with list - clist = CommunityList.objects.create(user=User.objects.get(username="plain")) + clist = CommunityList.objects.create(person=person) if not draft in clist.added_docs.all(): clist.added_docs.add(draft) SearchRule.objects.create( @@ -131,7 +130,7 @@ def test_manage_personal_list(self): 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(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.added_docs.filter(pk=draft.pk)) page = page.follow() @@ -142,7 +141,7 @@ def test_manage_personal_list(self): 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(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(not clist.added_docs.filter(pk=draft.pk)) page = page.follow() @@ -154,7 +153,7 @@ def test_manage_personal_list(self): "author_rfc-state": State.objects.get(type="draft", slug="rfc").pk, }) self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.searchrule_set.filter(rule_type="author_rfc")) # add name_contains rule @@ -165,7 +164,7 @@ def test_manage_personal_list(self): "name_contains-state": State.objects.get(type="draft", slug="active").pk, }) self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertTrue(clist.searchrule_set.filter(rule_type="name_contains")) # rule shows up on GET @@ -181,7 +180,7 @@ def test_manage_personal_list(self): "rule": rule.pk, }) - clist = CommunityList.objects.get(user__username="plain") + 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): @@ -222,7 +221,7 @@ def test_track_untrack_document(self): r = self.client.post(url) self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), [draft]) # untrack @@ -232,7 +231,7 @@ def test_track_untrack_document(self): r = self.client.post(url) self.assertEqual(r.status_code, 302) - clist = CommunityList.objects.get(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), []) def test_track_untrack_document_through_ajax(self): @@ -246,7 +245,7 @@ def test_track_untrack_document_through_ajax(self): 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(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), [draft]) # untrack @@ -254,11 +253,11 @@ def test_track_untrack_document_through_ajax(self): 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(user__username="plain") + clist = CommunityList.objects.get(person__user__username="plain") self.assertEqual(list(clist.added_docs.all()), []) def test_csv(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.export_to_csv, kwargs={ "username": "plain" }) @@ -268,7 +267,7 @@ def test_csv(self): self.assertEqual(r.status_code, 200) # with list - clist = CommunityList.objects.create(user=User.objects.get(username="plain")) + clist = CommunityList.objects.create(person=person) if not draft in clist.added_docs.all(): clist.added_docs.add(draft) SearchRule.objects.create( @@ -294,7 +293,7 @@ def test_csv_for_group(self): self.assertEqual(r.status_code, 200) def test_feed(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.feed, kwargs={ "username": "plain" }) @@ -304,7 +303,7 @@ def test_feed(self): self.assertEqual(r.status_code, 200) # with list - clist = CommunityList.objects.create(user=User.objects.get(username="plain")) + clist = CommunityList.objects.create(person=person) if not draft in clist.added_docs.all(): clist.added_docs.add(draft) SearchRule.objects.create( @@ -334,7 +333,7 @@ def test_feed_for_group(self): self.assertEqual(r.status_code, 200) def test_subscription(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() url = urlreverse(ietf.community.views.subscription, kwargs={ "username": "plain" }) @@ -346,7 +345,7 @@ def test_subscription(self): self.assertEqual(r.status_code, 404) # subscription with list - clist = CommunityList.objects.create(user=User.objects.get(username="plain")) + clist = CommunityList.objects.create(person=person) if not draft in clist.added_docs.all(): clist.added_docs.add(draft) SearchRule.objects.create( @@ -387,10 +386,10 @@ def test_subscription_for_group(self): self.assertEqual(r.status_code, 200) def test_notification(self): - PersonFactory(user__username='plain') + person = PersonFactory(user__username='plain') draft = WgDraftFactory() - clist = CommunityList.objects.create(user=User.objects.get(username="plain")) + clist = CommunityList.objects.create(person=person) if not draft in clist.added_docs.all(): clist.added_docs.add(draft) @@ -408,4 +407,4 @@ def test_notification(self): self.assertEqual(len(outbox), mailbox_before + 1) self.assertTrue(draft.name in outbox[-1]["Subject"]) - \ No newline at end of file + diff --git a/ietf/community/utils.py b/ietf/community/utils.py index 8130954b92..5beb2e2f67 100644 --- a/ietf/community/utils.py +++ b/ietf/community/utils.py @@ -37,7 +37,7 @@ def lookup_community_list(username=None, acronym=None): 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(user=user).first() or CommunityList(user=user) + clist = CommunityList.objects.filter(person__user=user).first() or CommunityList(person=user.person) return clist @@ -45,8 +45,8 @@ def can_manage_community_list(user, clist): if not user or not user.is_authenticated: return False - if clist.user: - return user == clist.user + if clist.person: + return user == clist.person.user elif clist.group: if has_role(user, 'Secretariat'): return True diff --git a/ietf/community/views.py b/ietf/community/views.py index 054bed3024..85f78d931e 100644 --- a/ietf/community/views.py +++ b/ietf/community/views.py @@ -255,12 +255,14 @@ def subscription(request, username=None, acronym=None, group_type=None): if clist.pk is None: raise Http404 - existing_subscriptions = EmailSubscription.objects.filter(community_list=clist, email__person__user=request.user) + person = request.user.person + + existing_subscriptions = EmailSubscription.objects.filter(community_list=clist, email__person=person) if request.method == 'POST': action = request.POST.get("action") if action == "subscribe": - form = SubscriptionForm(request.user, clist, request.POST) + form = SubscriptionForm(person, clist, request.POST) if form.is_valid(): subscription = form.save(commit=False) subscription.community_list = clist @@ -273,7 +275,7 @@ def subscription(request, username=None, acronym=None, group_type=None): return HttpResponseRedirect("") else: - form = SubscriptionForm(request.user, clist) + form = SubscriptionForm(person, clist) return render(request, 'community/subscription.html', { 'clist': clist, diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 6c94094759..e52d17d1ff 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -1047,29 +1047,26 @@ def build_file_urls(doc: Union[Document, DocHistory]): return file_urls, found_types -def augment_docs_and_user_with_user_info(docs, user): +def augment_docs_and_person_with_person_info(docs, person): """Add attribute to each document with whether the document is tracked - or has a review wish by the user or not, and the review teams the user is on.""" + or has a review wish by the person or not, and the review teams the person is on.""" tracked = set() review_wished = set() - - if user and user.is_authenticated: - user.review_teams = Group.objects.filter( - reviewteamsettings__isnull=False, role__person__user=user, role__name='reviewer') - doc_pks = [d.pk for d in docs] - clist = CommunityList.objects.filter(user=user).first() - if clist: - tracked.update( - docs_tracked_by_community_list(clist).filter(pk__in=doc_pks).values_list("pk", flat=True)) + # used in templates + person.review_teams = Group.objects.filter( + reviewteamsettings__isnull=False, role__person=person, role__name='reviewer') - try: - wishes = ReviewWish.objects.filter(person=Person.objects.get(user=user)) - wishes = wishes.filter(doc__pk__in=doc_pks).values_list("doc__pk", flat=True) - review_wished.update(wishes) - except Person.DoesNotExist: - pass + doc_pks = [d.pk for d in docs] + clist = CommunityList.objects.filter(person=person).first() + if clist: + tracked.update( + docs_tracked_by_community_list(clist).filter(pk__in=doc_pks).values_list("pk", flat=True)) + + wishes = ReviewWish.objects.filter(person=person) + wishes = wishes.filter(doc__pk__in=doc_pks).values_list("doc__pk", flat=True) + review_wished.update(wishes) for d in docs: d.tracked_in_personal_community_list = d.pk in tracked diff --git a/ietf/doc/utils_search.py b/ietf/doc/utils_search.py index 31aedda0d7..ee038bbfe1 100644 --- a/ietf/doc/utils_search.py +++ b/ietf/doc/utils_search.py @@ -11,7 +11,7 @@ from ietf.doc.models import Document, DocAlias, RelatedDocument, DocEvent, TelechatDocEvent, BallotDocEvent from ietf.doc.expire import expirable_drafts -from ietf.doc.utils import augment_docs_and_user_with_user_info +from ietf.doc.utils import augment_docs_and_person_with_person_info from ietf.meeting.models import SessionPresentation, Meeting, Session from ietf.review.utils import review_assignments_to_list_for_docs from ietf.utils.timezone import date_today @@ -200,7 +200,8 @@ def prepare_document_table(request, docs, query=None, max_results=200): docs = docs[:max_results] fill_in_document_table_attributes(docs) - augment_docs_and_user_with_user_info(docs, request.user) + if request.user.is_authenticated: + augment_docs_and_person_with_person_info(docs, request.user.person) augment_docs_with_related_docs_info(docs) meta = {} diff --git a/ietf/doc/views_doc.py b/ietf/doc/views_doc.py index b693f54e13..3891aaf769 100644 --- a/ietf/doc/views_doc.py +++ b/ietf/doc/views_doc.py @@ -63,7 +63,7 @@ needed_ballot_positions, nice_consensus, prettify_std_name, update_telechat, has_same_ballot, get_initial_notify, make_notify_changed_event, make_rev_history, default_consensus, add_events_message_info, get_unicode_document_content, - augment_docs_and_user_with_user_info, irsg_needed_ballot_positions, add_action_holder_change_event, + augment_docs_and_person_with_person_info, irsg_needed_ballot_positions, add_action_holder_change_event, build_file_urls, update_documentauthors, fuzzy_find_documents, bibxml_for_draft) from ietf.doc.utils_bofreq import bofreq_editors, bofreq_responsible @@ -459,7 +459,8 @@ def document_main(request, name, rev=None, document_html=False): elif can_edit_stream_info and (iesg_state_slug in ('idexists','watching')): actions.append(("Submit to IESG for Publication", urlreverse('ietf.doc.views_draft.to_iesg', kwargs=dict(name=doc.name)))) - augment_docs_and_user_with_user_info([doc], request.user) + if request.user.is_authenticated: + augment_docs_and_person_with_person_info([doc], request.user.person) published = doc.latest_event(type="published_rfc") started_iesg_process = doc.latest_event(type="started_iesg_process") diff --git a/ietf/nomcom/admin.py b/ietf/nomcom/admin.py index 4b18cc005c..1675d731a5 100644 --- a/ietf/nomcom/admin.py +++ b/ietf/nomcom/admin.py @@ -21,9 +21,9 @@ class NomComAdmin(admin.ModelAdmin): admin.site.register(NomCom, NomComAdmin) class NominationAdmin(admin.ModelAdmin): - list_display = ['id', 'position', 'candidate_name', 'candidate_email', 'candidate_phone', 'nominee', 'comments', 'nominator_email', 'user', 'time', 'share_nominator'] + list_display = ['id', 'position', 'candidate_name', 'candidate_email', 'candidate_phone', 'nominee', 'comments', 'nominator_email', 'person', 'time', 'share_nominator'] list_filter = ['time', 'share_nominator'] - raw_id_fields = ['nominee', 'comments', 'user'] + raw_id_fields = ['nominee', 'comments', 'person'] admin.site.register(Nomination, NominationAdmin) class NomineeAdmin(admin.ModelAdmin): @@ -51,9 +51,9 @@ def nominee(self, obj): return ", ".join(n.person.ascii for n in obj.nominees.all()) nominee.admin_order_field = 'nominees__person__ascii' # type: ignore # https://github.com/python/mypy/issues/2087 - list_display = ['id', 'nomcom', 'author', 'nominee', 'subject', 'type', 'user', 'time'] + list_display = ['id', 'nomcom', 'author', 'nominee', 'subject', 'type', 'person', 'time'] list_filter = ['nomcom', 'type', 'time', ] - raw_id_fields = ['positions', 'topics', 'user'] + raw_id_fields = ['positions', 'topics', 'person'] admin.site.register(Feedback, FeedbackAdmin) diff --git a/ietf/nomcom/factories.py b/ietf/nomcom/factories.py index 6fd6819b0b..9a37b4a188 100644 --- a/ietf/nomcom/factories.py +++ b/ietf/nomcom/factories.py @@ -9,7 +9,7 @@ from ietf.nomcom.models import NomCom, Position, Feedback, Nominee, NomineePosition, Nomination, Topic from ietf.group.factories import GroupFactory -from ietf.person.factories import PersonFactory, UserFactory +from ietf.person.factories import PersonFactory import debug # pyflakes:ignore @@ -196,7 +196,7 @@ class Meta: candidate_email = factory.LazyAttribute(lambda obj: obj.nominee.person.email()) candidate_phone = factory.Faker('phone_number') comments = factory.SubFactory(FeedbackFactory) - nominator_email = factory.LazyAttribute(lambda obj: obj.user.email) - user = factory.SubFactory(UserFactory) + nominator_email = factory.LazyAttribute(lambda obj: obj.person.user.email) + person = factory.SubFactory(PersonFactory) share_nominator = False diff --git a/ietf/nomcom/forms.py b/ietf/nomcom/forms.py index ad7bc67c23..2fb9ea2aec 100644 --- a/ietf/nomcom/forms.py +++ b/ietf/nomcom/forms.py @@ -15,7 +15,7 @@ from ietf.nomcom.models import ( NomCom, Nomination, Nominee, NomineePosition, Position, Feedback, ReminderDates, Topic, Volunteer ) from ietf.nomcom.utils import (NOMINATION_RECEIPT_TEMPLATE, FEEDBACK_RECEIPT_TEMPLATE, - get_user_email, validate_private_key, validate_public_key, + get_person_email, validate_private_key, validate_public_key, make_nomineeposition, make_nomineeposition_for_newperson, create_feedback_email) from ietf.person.models import Email @@ -256,7 +256,7 @@ class NominateForm(forms.ModelForm): def __init__(self, *args, **kwargs): self.nomcom = kwargs.pop('nomcom', None) - self.user = kwargs.pop('user', None) + self.person = kwargs.pop('person', None) self.public = kwargs.pop('public', None) super(NominateForm, self).__init__(*args, **kwargs) @@ -273,7 +273,7 @@ def __init__(self, *args, **kwargs): if not self.public: self.fields.pop('confirmation') - author = get_user_email(self.user) + author = get_person_email(self.person) if author: self.fields['nominator_email'].initial = author.address help_text = """(Nomcom Chair/Member: please fill this in. Use your own email address if the person making the @@ -303,7 +303,7 @@ def save(self, commit=True): author = None if self.public: - author = get_user_email(self.user) + author = get_person_email(self.person) else: if nominator_email: emails = Email.objects.filter(address=nominator_email) @@ -314,7 +314,7 @@ def save(self, commit=True): feedback = Feedback.objects.create(nomcom=self.nomcom, comments=self.nomcom.encrypt(qualifications), type=FeedbackTypeName.objects.get(slug='nomina'), - user=self.user) + person=self.person) feedback.positions.add(position) feedback.nominees.add(nominee) @@ -326,7 +326,7 @@ def save(self, commit=True): nomination.nominee = nominee nomination.comments = feedback nomination.share_nominator = share_nominator - nomination.user = self.user + nomination.person = self.person if commit: nomination.save() @@ -361,7 +361,7 @@ class NominateNewPersonForm(forms.ModelForm): def __init__(self, *args, **kwargs): self.nomcom = kwargs.pop('nomcom', None) - self.user = kwargs.pop('user', None) + self.person = kwargs.pop('person', None) self.public = kwargs.pop('public', None) super(NominateNewPersonForm, self).__init__(*args, **kwargs) @@ -375,7 +375,7 @@ def __init__(self, *args, **kwargs): if not self.public: self.fields.pop('confirmation') - author = get_user_email(self.user) + author = get_person_email(self.person) if author: self.fields['nominator_email'].initial = author.address help_text = """(Nomcom Chair/Member: please fill this in. Use your own email address if the person making the @@ -416,7 +416,7 @@ def save(self, commit=True): author = None if self.public: - author = get_user_email(self.user) + author = get_person_email(self.person) else: if nominator_email: emails = Email.objects.filter(address=nominator_email) @@ -429,7 +429,7 @@ def save(self, commit=True): feedback = Feedback.objects.create(nomcom=self.nomcom, comments=self.nomcom.encrypt(qualifications), type=FeedbackTypeName.objects.get(slug='nomina'), - user=self.user) + person=self.person) feedback.positions.add(position) feedback.nominees.add(nominee) @@ -441,7 +441,7 @@ def save(self, commit=True): nomination.nominee = nominee nomination.comments = feedback nomination.share_nominator = share_nominator - nomination.user = self.user + nomination.person = self.person if commit: nomination.save() @@ -476,7 +476,7 @@ class FeedbackForm(forms.ModelForm): def __init__(self, *args, **kwargs): self.nomcom = kwargs.pop('nomcom', None) - self.user = kwargs.pop('user', None) + self.person = kwargs.pop('person', None) self.public = kwargs.pop('public', None) self.position = kwargs.pop('position', None) self.nominee = kwargs.pop('nominee', None) @@ -484,7 +484,7 @@ def __init__(self, *args, **kwargs): super(FeedbackForm, self).__init__(*args, **kwargs) - author = get_user_email(self.user) + author = get_person_email(self.person) if self.public: self.fields.pop('nominator_email') @@ -514,7 +514,7 @@ def save(self, commit=True): author = None if self.public: - author = get_user_email(self.user) + author = get_person_email(self.person) else: nominator_email = self.cleaned_data['nominator_email'] if nominator_email: @@ -525,7 +525,7 @@ def save(self, commit=True): feedback.author = author.address feedback.nomcom = self.nomcom - feedback.user = self.user + feedback.person = self.person feedback.type = FeedbackTypeName.objects.get(slug='comment') feedback.comments = self.nomcom.encrypt(comment_text) feedback.save() @@ -578,7 +578,7 @@ class QuestionnaireForm(forms.ModelForm): def __init__(self, *args, **kwargs): self.nomcom = kwargs.pop('nomcom', None) - self.user = kwargs.pop('user', None) + self.person = kwargs.pop('person', None) super(QuestionnaireForm, self).__init__(*args, **kwargs) self.fields['nominee'] = PositionNomineeField(nomcom=self.nomcom, required=True) @@ -588,13 +588,13 @@ def save(self, commit=True): comment_text = self.cleaned_data['comment_text'] (position, nominee) = self.cleaned_data['nominee'] - author = get_user_email(self.user) + author = get_person_email(self.person) if author: feedback.author = author feedback.nomcom = self.nomcom - feedback.user = self.user + feedback.person = self.person feedback.type = FeedbackTypeName.objects.get(slug='questio') feedback.comments = self.nomcom.encrypt(comment_text) feedback.save() @@ -659,9 +659,9 @@ class Meta: model = Feedback fields = ('type', ) - def set_nomcom(self, nomcom, user): + def set_nomcom(self, nomcom, person): self.nomcom = nomcom - self.user = user + self.person = person #self.fields['nominee'] = MultiplePositionNomineeField(nomcom=self.nomcom, #required=True, #widget=forms.SelectMultiple, @@ -670,7 +670,7 @@ def set_nomcom(self, nomcom, user): def save(self, commit=True): feedback = super(PendingFeedbackForm, self).save(commit=False) feedback.nomcom = self.nomcom - feedback.user = self.user + feedback.person = self.person feedback.save() return feedback @@ -700,9 +700,9 @@ class Meta: model = Feedback fields = ('type', ) - def set_nomcom(self, nomcom, user, instances=None): + def set_nomcom(self, nomcom, person, instances=None): self.nomcom = nomcom - self.user = user + self.person = person instances = instances or [] self.feedback_type = None for i in instances: @@ -782,7 +782,7 @@ def save(self, commit=True): nominee=nominee, comments=feedback, nominator_email=nominator_email, - user=self.user) + person=self.person) return feedback else: feedback.save() diff --git a/ietf/nomcom/migrations/0004_user_to_person.py b/ietf/nomcom/migrations/0004_user_to_person.py new file mode 100644 index 0000000000..c76101d9b7 --- /dev/null +++ b/ietf/nomcom/migrations/0004_user_to_person.py @@ -0,0 +1,93 @@ +# Generated by Django 4.2.2 on 2023-06-14 19:47 + +from django.db import migrations +import django.db.models.deletion +import ietf.utils.models + + +def forward(apps, schema_editor): + Nomination = apps.get_model('nomcom', 'Nomination') + for nomination in Nomination.objects.all(): + try: + nomination.person = nomination.user.person + except: + nomination.person = None + nomination.save() + + Feedback = apps.get_model('nomcom', 'Feedback') + for feedback in Feedback.objects.all(): + try: + feedback.person = feedback.user.person + except: + feedback.person = None + feedback.save() + +def reverse(apps, schema_editor): + Nomination = apps.get_model('nomcom', 'Nomination') + for nomination in Nomination.objects.all(): + try: + nomination.user = nomination.person.user + except: + nomination.user = None + nomination.save() + + Feedback = apps.get_model('nomcom', 'Feedback') + for feedback in Feedback.objects.all(): + try: + feedback.user = feedback.person.user + except: + feedback.user = None + feedback.save() + +class Migration(migrations.Migration): + dependencies = [ + ("person", "0001_initial"), + ("nomcom", "0003_alter_nomination_share_nominator"), + ] + + operations = [ + migrations.AddField( + model_name="feedback", + name="person", + field=ietf.utils.models.ForeignKey( + blank=True, + editable=False, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="person.person", + ), + ), + migrations.AddField( + model_name="nomination", + name="person", + field=ietf.utils.models.ForeignKey( + editable=False, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="person.person", + ), + ), + migrations.RunPython(forward, reverse), + migrations.RemoveField( + model_name="feedback", + name="user", + field=ietf.utils.models.ForeignKey( + blank=True, + editable=False, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="user.user", + ), + ), + migrations.RemoveField( + model_name="nomination", + name="user", + field=ietf.utils.models.ForeignKey( + blank=True, + editable=False, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="user.user", + ), + ), + ] diff --git a/ietf/nomcom/models.py b/ietf/nomcom/models.py index ee2eea2cca..f02d3ef9c9 100644 --- a/ietf/nomcom/models.py +++ b/ietf/nomcom/models.py @@ -7,7 +7,6 @@ from django.db import models from django.db.models.signals import post_delete from django.conf import settings -from django.contrib.auth.models import User from django.template.loader import render_to_string from django.template.defaultfilters import linebreaks # type: ignore @@ -128,7 +127,7 @@ class Nomination(models.Model): nominee = ForeignKey('Nominee') comments = ForeignKey('Feedback') nominator_email = models.EmailField(verbose_name='Nominator Email', blank=True) - user = ForeignKey(User, editable=False, null=True, on_delete=models.SET_NULL) + person = ForeignKey(Person, editable=False, null=True, on_delete=models.SET_NULL) time = models.DateTimeField(auto_now_add=True) share_nominator = models.BooleanField(verbose_name='OK to share nominator\'s name with candidate', default=False, help_text='Check this box to allow the NomCom to let the ' @@ -299,7 +298,7 @@ class Feedback(models.Model): subject = models.TextField(verbose_name='Subject', blank=True) comments = models.BinaryField(verbose_name='Comments') type = ForeignKey(FeedbackTypeName, blank=True, null=True) - user = ForeignKey(User, editable=False, blank=True, null=True, on_delete=models.SET_NULL) + person = ForeignKey(Person, editable=False, blank=True, null=True, on_delete=models.SET_NULL) time = models.DateTimeField(auto_now_add=True) objects = FeedbackManager() diff --git a/ietf/nomcom/resources.py b/ietf/nomcom/resources.py index c87e72eae6..109a136419 100644 --- a/ietf/nomcom/resources.py +++ b/ietf/nomcom/resources.py @@ -115,11 +115,11 @@ class Meta: api.nomcom.register(NomineePositionResource()) from ietf.name.resources import FeedbackTypeNameResource -from ietf.utils.resources import UserResource +from ietf.person.resources import PersonResource class FeedbackResource(ModelResource): nomcom = ToOneField(NomComResource, 'nomcom') type = ToOneField(FeedbackTypeNameResource, 'type', null=True) - user = ToOneField(UserResource, 'user', null=True) + person = ToOneField(PersonResource, 'person', null=True) positions = ToManyField(PositionResource, 'positions', null=True) nominees = ToManyField(NomineeResource, 'nominees', null=True) class Meta: @@ -136,18 +136,18 @@ class Meta: "time": ALL, "nomcom": ALL_WITH_RELATIONS, "type": ALL_WITH_RELATIONS, - "user": ALL_WITH_RELATIONS, + "person": ALL_WITH_RELATIONS, "positions": ALL_WITH_RELATIONS, "nominees": ALL_WITH_RELATIONS, } api.nomcom.register(FeedbackResource()) -from ietf.utils.resources import UserResource +from ietf.person.resources import PersonResource class NominationResource(ModelResource): position = ToOneField(PositionResource, 'position') nominee = ToOneField(NomineeResource, 'nominee') comments = ToOneField(FeedbackResource, 'comments') - user = ToOneField(UserResource, 'user', null=True) + person = ToOneField(PersonResource, 'person', null=True) class Meta: cache = SimpleCache() queryset = Nomination.objects.all() @@ -164,7 +164,7 @@ class Meta: "position": ALL_WITH_RELATIONS, "nominee": ALL_WITH_RELATIONS, "comments": ALL_WITH_RELATIONS, - "user": ALL_WITH_RELATIONS, + "person": ALL_WITH_RELATIONS, } api.nomcom.register(NominationResource()) diff --git a/ietf/nomcom/tests.py b/ietf/nomcom/tests.py index 216984776c..fd6027687c 100644 --- a/ietf/nomcom/tests.py +++ b/ietf/nomcom/tests.py @@ -689,20 +689,16 @@ def test_public_nominate_with_automatic_questionnaire(self): self.assertIn('nominee@', outbox[1]['To']) - def nominate_view(self, *args, **kwargs): - public = kwargs.pop('public', True) - searched_email = kwargs.pop('searched_email', None) - nominee_email = kwargs.pop('nominee_email', 'nominee@example.com') + def nominate_view(self, public=True, searched_email=None, + nominee_email='nominee@example.com', + nominator_email=COMMUNITY_USER+EMAIL_DOMAIN, + position='IAOC', confirmation=False): + if not searched_email: - searched_email = Email.objects.filter(address=nominee_email).first() - if not searched_email: - searched_email = EmailFactory(address=nominee_email, primary=True, origin='test') + searched_email = Email.objects.filter(address=nominee_email).first() or EmailFactory(address=nominee_email, primary=True, origin='test') if not searched_email.person: searched_email.person = PersonFactory() searched_email.save() - nominator_email = kwargs.pop('nominator_email', "%s%s" % (COMMUNITY_USER, EMAIL_DOMAIN)) - position_name = kwargs.pop('position', 'IAOC') - confirmation = kwargs.pop('confirmation', False) if public: nominate_url = self.public_nominate_url @@ -726,7 +722,7 @@ def nominate_view(self, *args, **kwargs): q = PyQuery(response.content) self.assertEqual(len(q("#nominate-form")), 1) - position = Position.objects.get(name=position_name) + position = Position.objects.get(name=position) comment_text = 'Test nominate view. Comments with accents äöåÄÖÅ éáíóú âêîôû ü àèìòù.' candidate_phone = '123456' @@ -764,12 +760,9 @@ def nominate_view(self, *args, **kwargs): comments=feedback, nominator_email="%s%s" % (COMMUNITY_USER, EMAIL_DOMAIN)) - def nominate_newperson_view(self, *args, **kwargs): - public = kwargs.pop('public', True) - nominee_email = kwargs.pop('nominee_email', 'nominee@example.com') - nominator_email = kwargs.pop('nominator_email', "%s%s" % (COMMUNITY_USER, EMAIL_DOMAIN)) - position_name = kwargs.pop('position', 'IAOC') - confirmation = kwargs.pop('confirmation', False) + def nominate_newperson_view(self, public=True, nominee_email='nominee@example.com', + nominator_email=COMMUNITY_USER+EMAIL_DOMAIN, + position='IAOC', confirmation=False): if public: nominate_url = self.public_nominate_newperson_url @@ -793,7 +786,7 @@ def nominate_newperson_view(self, *args, **kwargs): q = PyQuery(response.content) self.assertEqual(len(q("#nominate-form")), 1) - position = Position.objects.get(name=position_name) + position = Position.objects.get(name=position) candidate_email = nominee_email candidate_name = 'nominee' comment_text = 'Test nominate view. Comments with accents äöåÄÖÅ éáíóú âêîôû ü àèìòù.' @@ -847,15 +840,13 @@ def test_add_questionnaire(self): self.access_chair_url(self.add_questionnaire_url) self.add_questionnaire() - def add_questionnaire(self, *args, **kwargs): - public = kwargs.pop('public', False) - nominee_email = kwargs.pop('nominee_email', 'nominee@example.com') - nominator_email = kwargs.pop('nominator_email', "%s%s" % (COMMUNITY_USER, EMAIL_DOMAIN)) - position_name = kwargs.pop('position', 'IAOC') + def add_questionnaire(self, public=False, nominee_email='nominee@example.com', + nominator_email=COMMUNITY_USER+EMAIL_DOMAIN, + position='IAOC'): self.nominate_view(public=public, nominee_email=nominee_email, - position=position_name, + position=position, nominator_email=nominator_email) response = self.client.get(self.add_questionnaire_url) @@ -874,7 +865,7 @@ def add_questionnaire(self, *args, **kwargs): self.assertEqual(response.status_code, 200) self.assertContains(response, "questionnnaireform") - position = Position.objects.get(name=position_name) + position = Position.objects.get(name=position) nominee = Nominee.objects.get(email__address=nominee_email) comment_text = 'Test add questionnaire view. Comments with accents äöåÄÖÅ éáíóú âêîôû ü àèìòù.' @@ -924,16 +915,13 @@ def test_private_feedback(self): self.access_member_url(self.private_feedback_url) self.feedback_view(public=False) - def feedback_view(self, *args, **kwargs): - public = kwargs.pop('public', True) - nominee_email = kwargs.pop('nominee_email', 'nominee@example.com') - nominator_email = kwargs.pop('nominator_email', "%s%s" % (COMMUNITY_USER, EMAIL_DOMAIN)) - position_name = kwargs.pop('position', 'IAOC') - confirmation = kwargs.pop('confirmation', False) + def feedback_view(self, public=True, nominee_email='nominee@example.com', + nominator_email=COMMUNITY_USER+EMAIL_DOMAIN, + position='IAOC', confirmation=False): self.nominate_view(public=public, nominee_email=nominee_email, - position=position_name, + position=position, nominator_email=nominator_email) feedback_url = self.public_feedback_url @@ -956,7 +944,7 @@ def feedback_view(self, *args, **kwargs): self.assertEqual(response.status_code, 200) self.assertNotContains(response, "feedbackform") - position = Position.objects.get(name=position_name) + position = Position.objects.get(name=position) nominee = Nominee.objects.get(email__address=nominee_email) feedback_url += "?nominee=%d&position=%d" % (nominee.id, position.id) @@ -972,7 +960,7 @@ def feedback_view(self, *args, **kwargs): comments = 'Test feedback view. Comments with accents äöåÄÖÅ éáíóú âêîôû ü àèìòù.' test_data = {'comment_text': comments, - 'position_name': position.name, + 'position': position.name, 'nominee_name': nominee.email.person.name, 'nominee_email': nominee.email.address, 'confirmation': confirmation} @@ -1168,7 +1156,7 @@ def setUp(self): feedback = Feedback.objects.create(nomcom=self.nomcom, comments=self.nomcom.encrypt('some non-empty comments'), type=FeedbackTypeName.objects.get(slug='questio'), - user=User.objects.get(username=CHAIR_USER)) + person=User.objects.get(username=CHAIR_USER).person) feedback.positions.add(gen) feedback.nominees.add(n) @@ -2133,7 +2121,7 @@ def test_public_accepting_feedback(self): self.assertIn('not currently accepting feedback', unicontent(response)) test_data = {'comment_text': 'junk', - 'position_name': pos.name, + 'position': pos.name, 'nominee_name': pos.nominee_set.first().email.person.name, 'nominee_email': pos.nominee_set.first().email.address, 'confirmation': False, diff --git a/ietf/nomcom/utils.py b/ietf/nomcom/utils.py index e009999e6d..b76c6f4d90 100644 --- a/ietf/nomcom/utils.py +++ b/ietf/nomcom/utils.py @@ -84,26 +84,21 @@ def get_year_by_nomcom(nomcom): return m.group(0) -def get_user_email(user): - # a user object already has an email field, but we don't want to - # overwrite anything that might be there, and we don't know that - # what's there is the right thing, so we cache the lookup results in a - # separate attribute - if not hasattr(user, "_email_cache"): - user._email_cache = None - if hasattr(user, "person"): - emails = user.person.email_set.filter(active=True).order_by('-time') - if emails: - user._email_cache = emails[0] - for email in emails: - if email.address.lower() == user.username.lower(): - user._email_cache = email +def get_person_email(person): + if not hasattr(person, "_email_cache"): + person._email_cache = None + emails = person.email_set.filter(active=True).order_by('-time') + if emails: + person._email_cache = emails[0] + for email in emails: + if email.address.lower() == person.user.username.lower(): + person._email_cache = email else: try: - user._email_cache = Email.objects.get(address=user.username) + person._email_cache = Email.objects.get(address=person.user.username) except ObjectDoesNotExist: pass - return user._email_cache + return person._email_cache def get_hash_nominee_position(date, nominee_position_id): return hmac.new(settings.NOMCOM_APP_SECRET, f"{date}{nominee_position_id}".encode('utf-8'), hashlib.sha256).hexdigest() diff --git a/ietf/nomcom/views.py b/ietf/nomcom/views.py index c077afce4c..a43454fc0f 100644 --- a/ietf/nomcom/views.py +++ b/ietf/nomcom/views.py @@ -458,23 +458,24 @@ def nominate(request, year, public, newperson): 'year': year, 'selected': 'nominate'}) + person = request.user.person if request.method == 'POST': if newperson: - form = NominateNewPersonForm(data=request.POST, nomcom=nomcom, user=request.user, public=public) + form = NominateNewPersonForm(data=request.POST, nomcom=nomcom, person=person, public=public) else: - form = NominateForm(data=request.POST, nomcom=nomcom, user=request.user, public=public) + form = NominateForm(data=request.POST, nomcom=nomcom, person=person, public=public) if form.is_valid(): form.save() messages.success(request, 'Your nomination has been registered. Thank you for the nomination.') if newperson: return redirect('ietf.nomcom.views.%s_nominate' % ('public' if public else 'private'), year=year) else: - form = NominateForm(nomcom=nomcom, user=request.user, public=public) + form = NominateForm(nomcom=nomcom, person=person, public=public) else: if newperson: - form = NominateNewPersonForm(nomcom=nomcom, user=request.user, public=public) + form = NominateNewPersonForm(nomcom=nomcom, person=person, public=public) else: - form = NominateForm(nomcom=nomcom, user=request.user, public=public) + form = NominateForm(nomcom=nomcom, person=person, public=public) return render(request, template, {'form': form, @@ -499,6 +500,7 @@ def feedback(request, year, public): nominee = None position = None topic = None + person = request.user.person if nomcom.group.state_id != 'conclude': selected_nominee = request.GET.get('nominee') selected_position = request.GET.get('position') @@ -510,7 +512,7 @@ def feedback(request, year, public): topic = get_object_or_404(Topic,id=selected_topic) if topic.audience_id == 'nomcom' and not nomcom.group.has_role(request.user, ['chair','advisor','liaison','member']): raise Http404() - if topic.audience_id == 'nominees' and not nomcom.nominee_set.filter(person=request.user.person).exists(): + if topic.audience_id == 'nominees' and not nomcom.nominee_set.filter(person=person).exists(): raise Http404() if public: @@ -522,12 +524,12 @@ def feedback(request, year, public): if not nomcom.group.has_role(request.user, ['chair','advisor','liaison','member']): topics = topics.exclude(audience_id='nomcom') - if not nomcom.nominee_set.filter(person=request.user.person).exists(): + if not nomcom.nominee_set.filter(person=person).exists(): topics = topics.exclude(audience_id='nominees') user_comments = Feedback.objects.filter(nomcom=nomcom, type='comment', - author__in=request.user.person.email_set.filter(active='True')) + author__in=person.email_set.filter(active='True')) counter = Counter(user_comments.values_list('positions','nominees')) counts = dict() for pos,nom in counter: @@ -580,11 +582,11 @@ def feedback(request, year, public): if request.method == 'POST': if nominee and position: form = FeedbackForm(data=request.POST, - nomcom=nomcom, user=request.user, + nomcom=nomcom, person=person, public=public, position=position, nominee=nominee) elif topic: form = FeedbackForm(data=request.POST, - nomcom=nomcom, user=request.user, + nomcom=nomcom, person=person, public=public, topic=topic) else: form = None @@ -603,10 +605,10 @@ def feedback(request, year, public): pass else: if nominee and position: - form = FeedbackForm(nomcom=nomcom, user=request.user, public=public, + form = FeedbackForm(nomcom=nomcom, person=person, public=public, position=position, nominee=nominee) elif topic: - form = FeedbackForm(nomcom=nomcom, user=request.user, public=public, + form = FeedbackForm(nomcom=nomcom, person=person, public=public, topic=topic) else: form = None @@ -672,6 +674,7 @@ def private_questionnaire(request, year): has_publickey = nomcom.public_key and True or False questionnaire_response = None template = 'nomcom/private_questionnaire.html' + person = request.user.person if not has_publickey: messages.warning(request, "This Nomcom is not yet accepting questionnaires.") @@ -692,14 +695,14 @@ def private_questionnaire(request, year): if request.method == 'POST': form = QuestionnaireForm(data=request.POST, - nomcom=nomcom, user=request.user) + nomcom=nomcom, person=person) if form.is_valid(): form.save() messages.success(request, 'The questionnaire response has been registered.') questionnaire_response = force_str(form.cleaned_data['comment_text']) - form = QuestionnaireForm(nomcom=nomcom, user=request.user) + form = QuestionnaireForm(nomcom=nomcom, person=person) else: - form = QuestionnaireForm(nomcom=nomcom, user=request.user) + form = QuestionnaireForm(nomcom=nomcom, person=person) return render(request, template, {'form': form, @@ -738,15 +741,13 @@ def process_nomination_status(request, year, nominee_position_id, state, date, h if form.cleaned_data['comments']: # This Feedback object is of type comment instead of nomina in order to not # make answering "who nominated themselves" harder. - who = request.user - if isinstance(who,AnonymousUser): - who = None + who = None if isinstance(request.user, AnonymousUser) else request.user.person f = Feedback.objects.create(nomcom = nomcom, author = nominee_position.nominee.email, subject = '%s nomination %s'%(nominee_position.nominee.name(),state), comments = nomcom.encrypt(form.cleaned_data['comments']), type_id = 'comment', - user = who, + person = who, ) f.positions.add(nominee_position.position) f.nominees.add(nominee_position.nominee) @@ -794,8 +795,9 @@ def nominee_staterank(nominee): sorted_nominees = sorted(nominees,key=lambda x:x.staterank) + reviewer = request.user.person for nominee in sorted_nominees: - last_seen = FeedbackLastSeen.objects.filter(reviewer=request.user.person,nominee=nominee).first() + last_seen = FeedbackLastSeen.objects.filter(reviewer=reviewer,nominee=nominee).first() nominee_feedback = [] for ft in nominee_feedback_types: qs = nominee.feedback_set.by_type(ft.slug) @@ -810,7 +812,7 @@ def nominee_staterank(nominee): nominees_feedback.append( {'nominee':nominee, 'feedback':nominee_feedback} ) independent_feedback = [ft.feedback_set.get_by_nomcom(nomcom).count() for ft in independent_feedback_types] for topic in nomcom.topic_set.all(): - last_seen = TopicFeedbackLastSeen.objects.filter(reviewer=request.user.person,topic=topic).first() + last_seen = TopicFeedbackLastSeen.objects.filter(reviewer=reviewer,topic=topic).first() topic_feedback = [] for ft in topic_feedback_types: qs = topic.feedback_set.by_type(ft.slug) @@ -857,6 +859,7 @@ def view_feedback_pending(request, year): except EmptyPage: feedback_page = paginator.page(paginator.num_pages) extra_step = False + person = request.user.person if request.method == 'POST' and request.POST.get('end'): extra_ids = request.POST.get('extra_ids', None) extra_step = True @@ -865,7 +868,7 @@ def view_feedback_pending(request, year): formset.absolute_max = 2000 formset.validate_max = False for form in formset.forms: - form.set_nomcom(nomcom, request.user) + form.set_nomcom(nomcom, person) if formset.is_valid(): formset.save() if extra_ids: @@ -877,7 +880,7 @@ def view_feedback_pending(request, year): extra.append(feedback) formset = FullFeedbackFormSet(queryset=Feedback.objects.filter(id__in=[i.id for i in extra])) for form in formset.forms: - form.set_nomcom(nomcom, request.user, extra) + form.set_nomcom(nomcom, person, extra) extra_ids = None else: messages.success(request, 'Feedback saved') @@ -885,7 +888,7 @@ def view_feedback_pending(request, year): elif request.method == 'POST': formset = FeedbackFormSet(request.POST) for form in formset.forms: - form.set_nomcom(nomcom, request.user) + form.set_nomcom(nomcom, person) if formset.is_valid(): extra = [] nominations = [] @@ -905,12 +908,12 @@ def view_feedback_pending(request, year): if nominations: formset = FullFeedbackFormSet(queryset=Feedback.objects.filter(id__in=[i.id for i in nominations])) for form in formset.forms: - form.set_nomcom(nomcom, request.user, nominations) + form.set_nomcom(nomcom, person, nominations) extra_ids = ','.join(['%s:%s' % (i.id, i.type.pk) for i in extra]) else: formset = FullFeedbackFormSet(queryset=Feedback.objects.filter(id__in=[i.id for i in extra])) for form in formset.forms: - form.set_nomcom(nomcom, request.user, extra) + form.set_nomcom(nomcom, person, extra) if moved: messages.success(request, '%s messages classified. You must enter more information for the following feedback.' % moved) else: @@ -919,7 +922,7 @@ def view_feedback_pending(request, year): else: formset = FeedbackFormSet(queryset=feedback_page.object_list) for form in formset.forms: - form.set_nomcom(nomcom, request.user) + form.set_nomcom(nomcom, person) type_dict = OrderedDict() for t in FeedbackTypeName.objects.all().order_by('pk'): rest = t.name @@ -964,13 +967,14 @@ def view_feedback_topic(request, year, topic_id): nomcom = get_nomcom_by_year(year) topic = get_object_or_404(Topic, id=topic_id) feedback_types = FeedbackTypeName.objects.filter(slug__in=['comment',]) + reviewer = request.user.person - last_seen = TopicFeedbackLastSeen.objects.filter(reviewer=request.user.person,topic=topic).first() + last_seen = TopicFeedbackLastSeen.objects.filter(reviewer=reviewer,topic=topic).first() last_seen_time = (last_seen and last_seen.time) or datetime.datetime(year=1, month=1, day=1, tzinfo=datetime.timezone.utc) if last_seen: last_seen.save() else: - TopicFeedbackLastSeen.objects.create(reviewer=request.user.person,topic=topic) + TopicFeedbackLastSeen.objects.create(reviewer=reviewer,topic=topic) return render(request, 'nomcom/view_feedback_topic.html', {'year': year, @@ -986,13 +990,14 @@ def view_feedback_nominee(request, year, nominee_id): nomcom = get_nomcom_by_year(year) nominee = get_object_or_404(Nominee, id=nominee_id) feedback_types = FeedbackTypeName.objects.filter(slug__in=settings.NOMINEE_FEEDBACK_TYPES) + reviewer = request.user.person - last_seen = FeedbackLastSeen.objects.filter(reviewer=request.user.person,nominee=nominee).first() + last_seen = FeedbackLastSeen.objects.filter(reviewer=reviewer,nominee=nominee).first() last_seen_time = (last_seen and last_seen.time) or datetime.datetime(year=1, month=1, day=1, tzinfo=datetime.timezone.utc) if last_seen: last_seen.save() else: - FeedbackLastSeen.objects.create(reviewer=request.user.person,nominee=nominee) + FeedbackLastSeen.objects.create(reviewer=reviewer,nominee=nominee) return render(request, 'nomcom/view_feedback_nominee.html', {'year': year, @@ -1243,15 +1248,15 @@ def configuration_help(request, year): @role_required("Nomcom Chair", "Nomcom Advisor") def edit_members(request, year): nomcom = get_nomcom_by_year(year) - if nomcom.group.state_id=='conclude': permission_denied(request, 'This nomcom is closed.') + person = request.user.person if request.method=='POST': form = EditMembersForm(nomcom, data=request.POST) if form.is_valid(): - update_role_set(nomcom.group, 'member', form.cleaned_data['members'], request.user.person) - update_role_set(nomcom.group, 'liaison', form.cleaned_data['liaisons'], request.user.person) + update_role_set(nomcom.group, 'member', form.cleaned_data['members'], person) + update_role_set(nomcom.group, 'liaison', form.cleaned_data['liaisons'], person) return HttpResponseRedirect(reverse('ietf.nomcom.views.private_index',kwargs={'year':year})) else: form = EditMembersForm(nomcom) diff --git a/ietf/person/tests.py b/ietf/person/tests.py index 11e1a5b663..a91527ea66 100644 --- a/ietf/person/tests.py +++ b/ietf/person/tests.py @@ -25,11 +25,11 @@ from ietf.nomcom.models import NomCom from ietf.nomcom.test_data import nomcom_test_data from ietf.nomcom.factories import NomComFactory, NomineeFactory, NominationFactory, FeedbackFactory, PositionFactory -from ietf.person.factories import EmailFactory, PersonFactory, UserFactory +from ietf.person.factories import EmailFactory, PersonFactory from ietf.person.models import Person, Alias from ietf.person.utils import (merge_persons, determine_merge_order, send_merge_notification, handle_users, get_extra_primary, dedupe_aliases, move_related_objects, merge_nominees, - handle_reviewer_settings, merge_users, get_dots) + handle_reviewer_settings, get_dots) from ietf.review.models import ReviewerSettings from ietf.utils.test_utils import TestCase, login_testing_unauthorized from ietf.utils.mail import outbox, empty_outbox @@ -373,13 +373,24 @@ def test_merge_persons(self): request.user = user source = PersonFactory() target = PersonFactory() + mars = RoleFactory(name_id='chair',group__acronym='mars').group source_id = source.pk source_email = source.email_set.first() source_alias = source.alias_set.first() source_user = source.user + communitylist = CommunityList.objects.create(person=source, group=mars) + nomcom = NomComFactory() + position = PositionFactory(nomcom=nomcom) + nominee = NomineeFactory(nomcom=nomcom, person=mars.get_chair().person) + feedback = FeedbackFactory(person=source, author=source.email().address, nomcom=nomcom) + feedback.nominees.add(nominee) + nomination = NominationFactory(nominee=nominee, person=source, position=position, comments=feedback) merge_persons(request, source, target, file=StringIO()) self.assertTrue(source_email in target.email_set.all()) self.assertTrue(source_alias in target.alias_set.all()) + self.assertIn(communitylist, target.communitylist_set.all()) + self.assertIn(feedback, target.feedback_set.all()) + self.assertIn(nomination, target.nomination_set.all()) self.assertFalse(Person.objects.filter(id=source_id)) self.assertFalse(source_user.is_active) @@ -399,24 +410,6 @@ def test_merge_persons_reviewer_settings(self): rs = target.reviewersettings_set.first() self.assertEqual(rs.min_interval, 7) - def test_merge_users(self): - person = PersonFactory() - source = person.user - target = UserFactory() - mars = RoleFactory(name_id='chair',group__acronym='mars').group - communitylist = CommunityList.objects.create(user=source, group=mars) - nomcom = NomComFactory() - position = PositionFactory(nomcom=nomcom) - nominee = NomineeFactory(nomcom=nomcom, person=mars.get_chair().person) - feedback = FeedbackFactory(user=source, author=person.email().address, nomcom=nomcom) - feedback.nominees.add(nominee) - nomination = NominationFactory(nominee=nominee, user=source, position=position, comments=feedback) - - merge_users(source, target) - self.assertIn(communitylist, target.communitylist_set.all()) - self.assertIn(feedback, target.feedback_set.all()) - self.assertIn(nomination, target.nomination_set.all()) - def test_dots(self): noroles = PersonFactory() self.assertEqual(get_dots(noroles),[]) diff --git a/ietf/person/utils.py b/ietf/person/utils.py index 942c2aaab2..a28aaaefa9 100755 --- a/ietf/person/utils.py +++ b/ietf/person/utils.py @@ -31,6 +31,20 @@ def merge_persons(request, source, target, file=sys.stdout, verbose=False): email.save() changes.append('EMAIL ACTION: {} no longer marked as primary'.format(email.address)) + # handle community list + for communitylist in source.communitylist_set.all(): + source.communitylist_set.remove(communitylist) + target.communitylist_set.add(communitylist) + + # handle feedback + for feedback in source.feedback_set.all(): + feedback.person = target + feedback.save() + # handle nominations + for nomination in source.nomination_set.all(): + nomination.person = target + nomination.save() + changes.append(handle_users(source, target)) reviewer_changes = handle_reviewer_settings(source, target) if reviewer_changes: @@ -103,7 +117,6 @@ def handle_users(source,target,check_only=False): if source.user and target.user: message = "DATATRACKER LOGIN ACTION: retaining login: {}, removing login: {}".format(target.user,source.user) if not check_only: - merge_users(source.user, target.user) syslog.syslog('merge-person-records: deactivating user {}'.format(source.user.username)) user = source.user source.user = None @@ -126,21 +139,6 @@ def move_related_objects(source, target, file, verbose=False): kwargs = { field_name:target } queryset.update(**kwargs) -def merge_users(source, target): - '''Move related objects from source user to target user''' - # handle community list - for communitylist in source.communitylist_set.all(): - source.communitylist_set.remove(communitylist) - target.communitylist_set.add(communitylist) - # handle feedback - for feedback in source.feedback_set.all(): - feedback.user = target - feedback.save() - # handle nominations - for nomination in source.nomination_set.all(): - nomination.user = target - nomination.save() - def dedupe_aliases(person): '''Check person for duplicate aliases and purge''' seen = [] diff --git a/ietf/secr/rolodex/views.py b/ietf/secr/rolodex/views.py index 7dd8201f0c..9fd4a8b107 100644 --- a/ietf/secr/rolodex/views.py +++ b/ietf/secr/rolodex/views.py @@ -7,7 +7,6 @@ from ietf.ietfauth.utils import role_required from ietf.person.models import Person, Email, Alias -from ietf.person.utils import merge_users from ietf.secr.rolodex.forms import EditPersonForm, EmailForm, NameForm, NewPersonForm, SearchForm @@ -179,7 +178,6 @@ def edit(request, id): if 'user' in person_form.changed_data and person_form.initial['user']: try: source = User.objects.get(username__iexact=person_form.initial['user']) - merge_users(source, person_form.cleaned_data['user']) source.is_active = False source.save() except User.DoesNotExist: diff --git a/ietf/templates/community/list_menu.html b/ietf/templates/community/list_menu.html index d54695775f..58269dccb4 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" username=clist.person.user.username %}{% endif %}"> All changes + href="{% if clist.group %}{% url "ietf.community.views.feed" acronym=clist.group.acronym %}{% else %}{% url "ietf.community.views.feed" username=clist.person.user.username %}{% 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" username=clist.person.user.username %}{% 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" username=clist.person.user.username %}{% endif %}"> Export as CSV - \ No newline at end of file + diff --git a/ietf/templates/community/view_list.html b/ietf/templates/community/view_list.html index 17650beb10..81f6d37454 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" username=clist.person.user.username %}"> Manage list @@ -22,4 +22,4 @@

{{ clist.long_name }}

{% endblock %} {% block js %} -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/ietf/templates/doc/document_draft.html b/ietf/templates/doc/document_draft.html index b38225dd4f..5db93a748c 100644 --- a/ietf/templates/doc/document_draft.html +++ b/ietf/templates/doc/document_draft.html @@ -726,7 +726,7 @@ Track {% endif %} - {% if user.review_teams %} + {% if user.person.review_teams %} @@ -734,7 +734,7 @@ Remove review wishes - @@ -820,4 +820,4 @@ - {% endblock %} \ No newline at end of file + {% endblock %} diff --git a/ietf/templates/doc/search/search_result_row.html b/ietf/templates/doc/search/search_result_row.html index d47dd4389c..5f956cb602 100644 --- a/ietf/templates/doc/search/search_result_row.html +++ b/ietf/templates/doc/search/search_result_row.html @@ -23,14 +23,14 @@
{% endif %} - {% if user.review_teams %} + {% if user.person.review_teams %} - @@ -151,4 +151,4 @@ {% endif %} {% endif %} - \ No newline at end of file + From ee2fd29795c38e4ccf80de7fd6405312504b955c Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 28 Jun 2023 12:42:12 -0400 Subject: [PATCH 02/13] 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 03/13] 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 @@ {% 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 04/13] 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 05/13] 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 06/13] 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 07/13] 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 08/13] 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 09/13] 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) From 309f083fa469634955ffd14fecd7b77efd220a45 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Tue, 8 Aug 2023 14:17:05 -0500 Subject: [PATCH 10/13] fix: case-insensitive person name or email matching (#6096) --- ietf/person/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ietf/person/utils.py b/ietf/person/utils.py index 5c97eda42e..eb2742ed30 100755 --- a/ietf/person/utils.py +++ b/ietf/person/utils.py @@ -249,11 +249,11 @@ def get_dots(person): return dots def lookup_persons(email_or_name): - aliases = Alias.objects.filter(name=email_or_name) + aliases = Alias.objects.filter(name__iexact=email_or_name) persons = set(a.person for a in aliases) if '@' in email_or_name: - emails = Email.objects.filter(address=email_or_name) + emails = Email.objects.filter(address__iexact=email_or_name) persons.update(e.person for e in emails) persons = [p for p in persons if p and p.id] From 962dbbb76ed042f74cb3418a662d7f2f429a6f1c Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 4 Jan 2024 11:59:50 -0400 Subject: [PATCH 11/13] chore: Renumber migrations --- .../{0003_user_to_person.py => 0004_user_to_person.py} | 2 +- .../{0004_user_to_person.py => 0005_user_to_person.py} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename ietf/community/migrations/{0003_user_to_person.py => 0004_user_to_person.py} (96%) rename ietf/nomcom/migrations/{0004_user_to_person.py => 0005_user_to_person.py} (97%) diff --git a/ietf/community/migrations/0003_user_to_person.py b/ietf/community/migrations/0004_user_to_person.py similarity index 96% rename from ietf/community/migrations/0003_user_to_person.py rename to ietf/community/migrations/0004_user_to_person.py index b626c23513..0e28cff077 100644 --- a/ietf/community/migrations/0003_user_to_person.py +++ b/ietf/community/migrations/0004_user_to_person.py @@ -26,7 +26,7 @@ def reverse(apps, schema_editor): class Migration(migrations.Migration): dependencies = [ - ("community", "0002_auto_20230320_1222"), + ("community", "0003_track_rfcs"), ("person", "0001_initial"), ] diff --git a/ietf/nomcom/migrations/0004_user_to_person.py b/ietf/nomcom/migrations/0005_user_to_person.py similarity index 97% rename from ietf/nomcom/migrations/0004_user_to_person.py rename to ietf/nomcom/migrations/0005_user_to_person.py index c76101d9b7..5ece380e15 100644 --- a/ietf/nomcom/migrations/0004_user_to_person.py +++ b/ietf/nomcom/migrations/0005_user_to_person.py @@ -42,7 +42,7 @@ def reverse(apps, schema_editor): class Migration(migrations.Migration): dependencies = [ ("person", "0001_initial"), - ("nomcom", "0003_alter_nomination_share_nominator"), + ("nomcom", "0004_volunteer_origin_volunteer_time_volunteer_withdrawn"), ] operations = [ From 960ed27b04bc6d00e8841e0deafb866e3dcec1e2 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 5 Jan 2024 13:15:46 -0400 Subject: [PATCH 12/13] fix: Update merged code so tests pass (#6887) * fix: Use refactored method * fix: Don't assume user has person * fix: Use new view param name --- ietf/doc/views_doc.py | 5 +++-- ietf/templates/doc/document_rfc.html | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ietf/doc/views_doc.py b/ietf/doc/views_doc.py index adbd245f99..64f9de4f58 100644 --- a/ietf/doc/views_doc.py +++ b/ietf/doc/views_doc.py @@ -287,7 +287,8 @@ def document_main(request, name, rev=None, document_html=False): presentations = doc.future_presentations() - augment_docs_and_user_with_user_info([doc], request.user) + if request.user.is_authenticated and hasattr(request.user, "person"): + augment_docs_and_person_with_person_info([doc], request.user.person) exp_comment = doc.latest_event(IanaExpertDocEvent,type="comment") iana_experts_comment = exp_comment and exp_comment.desc @@ -580,7 +581,7 @@ def document_main(request, name, rev=None, document_html=False): elif can_edit_stream_info and (iesg_state_slug in ('idexists','watching')): actions.append(("Submit to IESG for Publication", urlreverse('ietf.doc.views_draft.to_iesg', kwargs=dict(name=doc.name)))) - if request.user.is_authenticated: + if request.user.is_authenticated and hasattr(request.user, "person"): augment_docs_and_person_with_person_info([doc], request.user.person) published = doc.latest_event(type="published_rfc") # todo rethink this now that published_rfc is on rfc diff --git a/ietf/templates/doc/document_rfc.html b/ietf/templates/doc/document_rfc.html index 36c9babd79..9e889afaff 100644 --- a/ietf/templates/doc/document_rfc.html +++ b/ietf/templates/doc/document_rfc.html @@ -129,14 +129,14 @@ {% if user.is_authenticated %} Untrack From be2ec7b1e38d38aac836eda35b214c19b1fa09a4 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Mon, 8 Jan 2024 13:15:56 -0400 Subject: [PATCH 13/13] chore: Drop community lists w/o person; cleanup (#6896) * fix: Don't assume user has person * fix: user->person in update_community_list_index.py * feat: Remove CommunityLists without Person * refactor: Speed up nomcom migrations --- .../0004_delete_useless_community_lists.py | 26 +++++++++ ...er_to_person.py => 0005_user_to_person.py} | 2 +- ietf/doc/utils_search.py | 2 +- ietf/nomcom/migrations/0005_user_to_person.py | 55 +++++++++++-------- .../commands/update_community_list_index.py | 2 +- 5 files changed, 60 insertions(+), 27 deletions(-) create mode 100644 ietf/community/migrations/0004_delete_useless_community_lists.py rename ietf/community/migrations/{0004_user_to_person.py => 0005_user_to_person.py} (96%) diff --git a/ietf/community/migrations/0004_delete_useless_community_lists.py b/ietf/community/migrations/0004_delete_useless_community_lists.py new file mode 100644 index 0000000000..9f657a3c34 --- /dev/null +++ b/ietf/community/migrations/0004_delete_useless_community_lists.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.9 on 2024-01-05 21:28 + +from django.db import migrations + + +def forward(apps, schema_editor): + CommunityList = apps.get_model("community", "CommunityList") + # As of 2024-01-05, there are 570 personal CommunityLists with a user + # who has no associated Person. None of these has an EmailSubscription, + # so the lists are doing nothing and can be safely deleted. + personal_lists_no_person = CommunityList.objects.exclude( + user__isnull=True + ).filter( + user__person__isnull=True + ) + # Confirm the assumption that none of the lists to be deleted has an EmailSubscription + assert not personal_lists_no_person.filter(emailsubscription__isnull=False).exists() + personal_lists_no_person.delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("community", "0003_track_rfcs"), + ] + + operations = [migrations.RunPython(forward)] diff --git a/ietf/community/migrations/0004_user_to_person.py b/ietf/community/migrations/0005_user_to_person.py similarity index 96% rename from ietf/community/migrations/0004_user_to_person.py rename to ietf/community/migrations/0005_user_to_person.py index 0e28cff077..01d8950edb 100644 --- a/ietf/community/migrations/0004_user_to_person.py +++ b/ietf/community/migrations/0005_user_to_person.py @@ -26,7 +26,7 @@ def reverse(apps, schema_editor): class Migration(migrations.Migration): dependencies = [ - ("community", "0003_track_rfcs"), + ("community", "0004_delete_useless_community_lists"), ("person", "0001_initial"), ] diff --git a/ietf/doc/utils_search.py b/ietf/doc/utils_search.py index 7928efc93c..a4c9539fe4 100644 --- a/ietf/doc/utils_search.py +++ b/ietf/doc/utils_search.py @@ -199,7 +199,7 @@ def prepare_document_table(request, docs, query=None, max_results=200): docs = docs[:max_results] fill_in_document_table_attributes(docs) - if request.user.is_authenticated: + if request.user.is_authenticated and hasattr(request.user, "person"): augment_docs_and_person_with_person_info(docs, request.user.person) augment_docs_with_related_docs_info(docs) diff --git a/ietf/nomcom/migrations/0005_user_to_person.py b/ietf/nomcom/migrations/0005_user_to_person.py index 5ece380e15..66a6e99642 100644 --- a/ietf/nomcom/migrations/0005_user_to_person.py +++ b/ietf/nomcom/migrations/0005_user_to_person.py @@ -1,43 +1,50 @@ # Generated by Django 4.2.2 on 2023-06-14 19:47 from django.db import migrations +from django.db.models import OuterRef, Subquery import django.db.models.deletion import ietf.utils.models def forward(apps, schema_editor): Nomination = apps.get_model('nomcom', 'Nomination') - for nomination in Nomination.objects.all(): - try: - nomination.person = nomination.user.person - except: - nomination.person = None - nomination.save() + Person = apps.get_model("person", "Person") + Nomination.objects.exclude( + user__isnull=True + ).update( + person=Subquery( + Person.objects.filter(user_id=OuterRef("user_id")).values("pk")[:1] + ) + ) Feedback = apps.get_model('nomcom', 'Feedback') - for feedback in Feedback.objects.all(): - try: - feedback.person = feedback.user.person - except: - feedback.person = None - feedback.save() + Feedback.objects.exclude( + user__isnull=True + ).update( + person=Subquery( + Person.objects.filter(user_id=OuterRef("user_id")).values("pk")[:1] + ) + ) def reverse(apps, schema_editor): Nomination = apps.get_model('nomcom', 'Nomination') - for nomination in Nomination.objects.all(): - try: - nomination.user = nomination.person.user - except: - nomination.user = None - nomination.save() + Person = apps.get_model("person", "Person") + Nomination.objects.exclude( + person__isnull=True + ).update( + user_id=Subquery( + Person.objects.filter(pk=OuterRef("person_id")).values("user_id")[:1] + ) + ) Feedback = apps.get_model('nomcom', 'Feedback') - for feedback in Feedback.objects.all(): - try: - feedback.user = feedback.person.user - except: - feedback.user = None - feedback.save() + Feedback.objects.exclude( + person__isnull=True + ).update( + user_id=Subquery( + Person.objects.filter(pk=OuterRef("person_id")).values("user_id")[:1] + ) + ) class Migration(migrations.Migration): dependencies = [ diff --git a/ietf/utils/management/commands/update_community_list_index.py b/ietf/utils/management/commands/update_community_list_index.py index 609577763e..7f4951fd6e 100644 --- a/ietf/utils/management/commands/update_community_list_index.py +++ b/ietf/utils/management/commands/update_community_list_index.py @@ -32,7 +32,7 @@ def handle(self, *args, **options): person = rule.person if not person and not group: try: - person = rule.community_list.user.person + person = rule.community_list.person except: pass name = ((group and group.acronym) or (person and person.email_address())) or '?'