From 6cf9eb8dd1393c5726ce708f3715398c36fb8cc2 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 May 2021 18:40:28 +0000 Subject: [PATCH] Allow secretariat to edit document author list. Fixes #3185. Commit ready for merge. - Legacy-Id: 18989 --- ietf/doc/factories.py | 3 + ietf/doc/forms.py | 25 +- ietf/doc/tests.py | 449 ++++++++++++++++++++++++- ietf/doc/tests_js.py | 144 ++++++++ ietf/doc/urls.py | 1 + ietf/doc/utils.py | 78 ++++- ietf/doc/views_doc.py | 80 ++++- ietf/person/ajax.py | 8 + ietf/person/tests.py | 24 ++ ietf/person/urls.py | 1 + ietf/submit/utils.py | 34 +- ietf/templates/doc/document_draft.html | 6 +- ietf/templates/doc/edit_authors.html | 160 +++++++++ ietf/utils/jstest.py | 6 + ietf/utils/test_utils.py | 12 +- 15 files changed, 1001 insertions(+), 30 deletions(-) create mode 100644 ietf/doc/tests_js.py create mode 100644 ietf/templates/doc/edit_authors.html diff --git a/ietf/doc/factories.py b/ietf/doc/factories.py index f3e525d1d..33954548e 100644 --- a/ietf/doc/factories.py +++ b/ietf/doc/factories.py @@ -373,6 +373,9 @@ class DocumentAuthorFactory(factory.DjangoModelFactory): document = factory.SubFactory(DocumentFactory) person = factory.SubFactory('ietf.person.factories.PersonFactory') email = factory.LazyAttribute(lambda obj: obj.person.email()) + affiliation = factory.Faker('company') + country = factory.Faker('country') + order = factory.LazyAttribute(lambda o: o.document.documentauthor_set.count() + 1) class WgDocumentAuthorFactory(DocumentAuthorFactory): document = factory.SubFactory(WgDraftFactory) diff --git a/ietf/doc/forms.py b/ietf/doc/forms.py index a0a76e0c0..d209eb0a2 100644 --- a/ietf/doc/forms.py +++ b/ietf/doc/forms.py @@ -11,7 +11,8 @@ from ietf.doc.fields import SearchableDocAliasesField, SearchableDocAliasField from ietf.doc.models import RelatedDocument, DocExtResource from ietf.iesg.models import TelechatDate from ietf.iesg.utils import telechat_page_count -from ietf.person.fields import SearchablePersonsField +from ietf.person.fields import SearchablePersonField, SearchablePersonsField +from ietf.person.models import Email, Person from ietf.name.models import ExtResourceName from ietf.utils.validators import validate_external_resource_value @@ -37,8 +38,28 @@ class TelechatForm(forms.Form): choice_display[d] += ' : WARNING - this may not leave enough time for directorate reviews!' self.fields['telechat_date'].choices = [("", "(not on agenda)")] + [(d, choice_display[d]) for d in dates] -from ietf.person.models import Person +class DocAuthorForm(forms.Form): + person = SearchablePersonField() + email = forms.ModelChoiceField(queryset=Email.objects.none(), required=False) + affiliation = forms.CharField(max_length=100, required=False) + country = forms.CharField(max_length=255, required=False) + + def __init__(self, *args, **kwargs): + super(DocAuthorForm, self).__init__(*args, **kwargs) + + person = self.data.get( + self.add_prefix('person'), + self.get_initial_for_field(self.fields['person'], 'person') + ) + if person: + self.fields['email'].queryset = Email.objects.filter(person=person) + +class DocAuthorChangeBasisForm(forms.Form): + basis = forms.CharField(max_length=255, + label='Reason for change', + help_text='What is the source or reasoning for the changes to the author list?') + class AdForm(forms.Form): ad = forms.ModelChoiceField(Person.objects.filter(role__name="ad", role__group__state="active", role__group__type='area').order_by('name'), label="Shepherding AD", empty_label="(None)", required=True) diff --git a/ietf/doc/tests.py b/ietf/doc/tests.py index 55e86e666..1cb40116b 100644 --- a/ietf/doc/tests.py +++ b/ietf/doc/tests.py @@ -10,6 +10,7 @@ import lxml import bibtexparser import mock import json +import copy from http.cookies import SimpleCookie from pyquery import PyQuery @@ -27,11 +28,12 @@ from tastypie.test import ResourceTestCaseMixin import debug # pyflakes:ignore from ietf.doc.models import ( Document, DocAlias, DocRelationshipName, RelatedDocument, State, - DocEvent, BallotPositionDocEvent, LastCallDocEvent, WriteupDocEvent, NewRevisionDocEvent, BallotType ) + DocEvent, BallotPositionDocEvent, LastCallDocEvent, WriteupDocEvent, NewRevisionDocEvent, BallotType, + EditedAuthorsDocEvent ) from ietf.doc.factories import ( DocumentFactory, DocEventFactory, CharterFactory, ConflictReviewFactory, WgDraftFactory, IndividualDraftFactory, WgRfcFactory, IndividualRfcFactory, StateDocEventFactory, BallotPositionDocEventFactory, - BallotDocEventFactory ) + BallotDocEventFactory, DocumentAuthorFactory ) from ietf.doc.fields import SearchableDocumentsField from ietf.doc.utils import create_ballot_if_not_open, uppercase_std_abbreviated_name from ietf.group.models import Group @@ -41,7 +43,7 @@ from ietf.meeting.models import Meeting, Session, SessionPresentation, Schedulin from ietf.meeting.factories import MeetingFactory, SessionFactory from ietf.name.models import SessionStatusName, BallotPositionName from ietf.person.models import Person -from ietf.person.factories import PersonFactory +from ietf.person.factories import PersonFactory, EmailFactory from ietf.utils.mail import outbox from ietf.utils.test_utils import login_testing_unauthorized, unicontent from ietf.utils.test_utils import TestCase @@ -786,6 +788,447 @@ Man Expires September 22, 2015 [Page 3] msg_prefix='Non-WG-like group %s (%s) should not include group type in link' % (group.acronym, group.type), ) + def login(self, username): + self.client.login(username=username, password=username + '+password') + + def test_edit_authors_permissions(self): + """Only the secretariat may edit authors""" + draft = WgDraftFactory(authors=PersonFactory.create_batch(3)) + RoleFactory(group=draft.group, name_id='chair') + RoleFactory(group=draft.group, name_id='ad', person=Person.objects.get(user__username='ad')) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + + # Relevant users not authorized to edit authors + unauthorized_usernames = [ + 'plain', + *[author.user.username for author in draft.authors()], + draft.group.get_chair().person.user.username, + 'ad' + ] + + # First, check that only the secretary can even see the edit page. + # Each call checks that currently-logged in user is refused, then logs in as the named user. + for username in unauthorized_usernames: + login_testing_unauthorized(self, username, url) + login_testing_unauthorized(self, 'secretary', url) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + self.client.logout() + + # Try to add an author via POST - still only the secretary should be able to do this. + orig_authors = draft.authors() + post_data = self.make_edit_authors_post_data( + basis='permission test', + authors=draft.documentauthor_set.all(), + ) + new_auth_person = PersonFactory() + self.add_author_to_edit_authors_post_data( + post_data, + dict( + person=str(new_auth_person.pk), + email=str(new_auth_person.email()), + affiliation='affil', + country='USA', + ), + ) + for username in unauthorized_usernames: + login_testing_unauthorized(self, username, url, method='post', request_kwargs=dict(data=post_data)) + draft = Document.objects.get(pk=draft.pk) + self.assertEqual(draft.authors(), orig_authors) # ensure draft author list was not modified + login_testing_unauthorized(self, 'secretary', url, method='post', request_kwargs=dict(data=post_data)) + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 302) + draft = Document.objects.get(pk=draft.pk) + self.assertEqual(draft.authors(), orig_authors + [new_auth_person]) + + def make_edit_authors_post_data(self, basis, authors): + """Helper to generate edit_authors POST data for a set of authors""" + def _add_prefix(s): + # The prefix here needs to match the formset prefix in the edit_authors() view + return 'author-{}'.format(s) + + data = { + 'basis': basis, + # management form + _add_prefix('TOTAL_FORMS'): '1', # just the empty form so far + _add_prefix('INITIAL_FORMS'): str(len(authors)), + _add_prefix('MIN_NUM_FORMS'): '0', + _add_prefix('MAX_NUM_FORMS'): '1000', + # empty form + _add_prefix('__prefix__-person'): '', + _add_prefix('__prefix__-email'): '', + _add_prefix('__prefix__-affiliation'): '', + _add_prefix('__prefix__-country'): '', + _add_prefix('__prefix__-ORDER'): '', + } + + for index, auth in enumerate(authors): + self.add_author_to_edit_authors_post_data( + data, + dict( + person=str(auth.person.pk), + email=auth.email, + affiliation=auth.affiliation, + country=auth.country + ) + ) + + return data + + def add_author_to_edit_authors_post_data(self, post_data, new_author, insert_order=-1, prefix='author'): + """Helper to insert an author in the POST data for the edit_authors view + + The insert_order parameter is 0-indexed (i.e., it's the Django formset ORDER field, not the + DocumentAuthor order property, which is 1-indexed) + """ + def _add_prefix(s): + return '{}-{}'.format(prefix, s) + + total_forms = int(post_data[_add_prefix('TOTAL_FORMS')]) - 1 # subtract 1 for empty form + if insert_order < 0: + insert_order = total_forms + else: + # Make a map from order to the data key that has that order value + order_key = dict() + for order in range(insert_order, total_forms): + key = _add_prefix(str(order) + '-ORDER') + order_key[int(post_data[key])] = key + # now increment all orders at or above where new element will be inserted + for order in range(insert_order, total_forms): + post_data[order_key[order]] = str(order + 1) + + form_index = total_forms # regardless of insert order, new data has next unused form index + total_forms += 1 # new form + + post_data[_add_prefix('TOTAL_FORMS')] = total_forms + 1 # add 1 for empty form + for prop in ['person', 'email', 'affiliation', 'country']: + post_data[_add_prefix(str(form_index) + '-' + prop)] = str(new_author[prop]) + post_data[_add_prefix(str(form_index) + '-ORDER')] = str(insert_order) + + def test_edit_authors_missing_basis(self): + draft = WgDraftFactory() + DocumentAuthorFactory.create_batch(3, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + + self.login('secretary') + post_data = self.make_edit_authors_post_data( + authors = draft.documentauthor_set.all(), + basis='delete me' + ) + post_data.pop('basis') + + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 200) + self.assertContains(r, 'This field is required.') + + def test_edit_authors_no_change(self): + draft = WgDraftFactory() + DocumentAuthorFactory.create_batch(3, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + change_reason = 'no change' + + before = list(draft.documentauthor_set.values('person', 'email', 'affiliation', 'country', 'order')) + + post_data = self.make_edit_authors_post_data( + authors = draft.documentauthor_set.all(), + basis=change_reason + ) + + self.login('secretary') + r = self.client.post(url, post_data) + + self.assertEqual(r.status_code, 302) + + draft = Document.objects.get(pk=draft.pk) + after = list(draft.documentauthor_set.values('person', 'email', 'affiliation', 'country', 'order')) + self.assertCountEqual(after, before, 'Unexpected change to an author') + self.assertEqual(EditedAuthorsDocEvent.objects.filter(basis=change_reason).count(), 0) + + def do_edit_authors_append_authors_test(self, new_author_count): + """Can add author at the end of the list""" + draft = WgDraftFactory() + starting_author_count = 3 + DocumentAuthorFactory.create_batch(starting_author_count, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + change_reason = 'add a new author' + + compare_props = 'person', 'email', 'affiliation', 'country', 'order' + before = list(draft.documentauthor_set.values(*compare_props)) + events_before = EditedAuthorsDocEvent.objects.count() + + post_data = self.make_edit_authors_post_data( + authors=draft.documentauthor_set.all(), + basis=change_reason + ) + + new_authors = PersonFactory.create_batch(new_author_count, default_emails=True) + new_author_data = [ + dict( + person=new_author.pk, + email=str(new_author.email()), + affiliation='University of Somewhere', + country='Botswana', + ) + for new_author in new_authors + ] + for index, auth_dict in enumerate(new_author_data): + self.add_author_to_edit_authors_post_data(post_data, auth_dict) + auth_dict['order'] = starting_author_count + index + 1 # for comparison later + + self.login('secretary') + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 302) + + draft = Document.objects.get(pk=draft.pk) + after = list(draft.documentauthor_set.values(*compare_props)) + + self.assertEqual(len(after), len(before) + new_author_count) + for b, a in zip(before + new_author_data, after): + for prop in compare_props: + self.assertEqual(a[prop], b[prop], + 'Unexpected change: "{}" was "{}", changed to "{}"'.format( + prop, b[prop], a[prop] + )) + + self.assertEqual(EditedAuthorsDocEvent.objects.count(), events_before + new_author_count) + change_events = EditedAuthorsDocEvent.objects.filter(basis=change_reason) + self.assertEqual(change_events.count(), new_author_count) + # The events are most-recent first, so first author added is last event in the list. + # Reverse the author list with [::-1] + for evt, auth in zip(change_events, new_authors[::-1]): + self.assertIn('added', evt.desc.lower()) + self.assertIn(auth.name, evt.desc) + + def test_edit_authors_append_author(self): + self.do_edit_authors_append_authors_test(1) + + def test_edit_authors_append_authors(self): + self.do_edit_authors_append_authors_test(3) + + def test_edit_authors_insert_author(self): + """Can add author in the middle of the list""" + draft = WgDraftFactory() + DocumentAuthorFactory.create_batch(3, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + change_reason = 'add a new author' + + compare_props = 'person', 'email', 'affiliation', 'country', 'order' + before = list(draft.documentauthor_set.values(*compare_props)) + events_before = EditedAuthorsDocEvent.objects.count() + + post_data = self.make_edit_authors_post_data( + authors = draft.documentauthor_set.all(), + basis=change_reason + ) + + new_author = PersonFactory(default_emails=True) + new_author_data = dict( + person=new_author.pk, + email=str(new_author.email()), + affiliation='University of Somewhere', + country='Botswana', + ) + self.add_author_to_edit_authors_post_data(post_data, new_author_data, insert_order=1) + + self.login('secretary') + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 302) + + draft = Document.objects.get(pk=draft.pk) + after = list(draft.documentauthor_set.values(*compare_props)) + + new_author_data['order'] = 2 # corresponds to insert_order == 1 + expected = copy.deepcopy(before) + expected.insert(1, new_author_data) + expected[2]['order'] = 3 + expected[3]['order'] = 4 + self.assertEqual(len(after), len(expected)) + for b, a in zip(expected, after): + for prop in compare_props: + self.assertEqual(a[prop], b[prop], + 'Unexpected change: "{}" was "{}", changed to "{}"'.format( + prop, b[prop], a[prop] + )) + + # 3 changes: new author, plus two order changes + self.assertEqual(EditedAuthorsDocEvent.objects.count(), events_before + 3) + change_events = EditedAuthorsDocEvent.objects.filter(basis=change_reason) + self.assertEqual(change_events.count(), 3) + + add_event = change_events.filter(desc__icontains='added').first() + reorder_events = change_events.filter(desc__icontains='changed order') + + self.assertIsNotNone(add_event) + self.assertEqual(reorder_events.count(), 2) + + def test_edit_authors_remove_author(self): + draft = WgDraftFactory() + DocumentAuthorFactory.create_batch(3, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + change_reason = 'remove an author' + + compare_props = 'person', 'email', 'affiliation', 'country', 'order' + before = list(draft.documentauthor_set.values(*compare_props)) + events_before = EditedAuthorsDocEvent.objects.count() + + post_data = self.make_edit_authors_post_data( + authors = draft.documentauthor_set.all(), + basis=change_reason + ) + + # delete the second author (index == 1) + deleted_author_data = before.pop(1) + post_data['author-1-DELETE'] = 'on' # delete box checked + + self.login('secretary') + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 302) + + draft = Document.objects.get(pk=draft.pk) + after = list(draft.documentauthor_set.values(*compare_props)) + + before[1]['order'] = 2 # was 3, but should have been decremented + self.assertEqual(len(after), len(before)) + for b, a in zip(before, after): + for prop in compare_props: + self.assertEqual(a[prop], b[prop], + 'Unexpected change: "{}" was "{}", changed to "{}"'.format( + prop, b[prop], a[prop] + )) + + # expect 2 events: one for removing author, another for reordering the later author + self.assertEqual(EditedAuthorsDocEvent.objects.count(), events_before + 2) + change_events = EditedAuthorsDocEvent.objects.filter(basis=change_reason) + self.assertEqual(change_events.count(), 2) + + removed_event = change_events.filter(desc__icontains='removed').first() + self.assertIsNotNone(removed_event) + deleted_person = Person.objects.get(pk=deleted_author_data['person']) + self.assertIn(deleted_person.name, removed_event.desc) + + reordered_event = change_events.filter(desc__icontains='changed order').first() + reordered_person = Person.objects.get(pk=after[1]['person']) + self.assertIsNotNone(reordered_event) + self.assertIn(reordered_person.name, reordered_event.desc) + + def test_edit_authors_reorder_authors(self): + draft = WgDraftFactory() + DocumentAuthorFactory.create_batch(3, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + change_reason = 'reorder the authors' + + compare_props = 'person', 'email', 'affiliation', 'country', 'order' + before = list(draft.documentauthor_set.values(*compare_props)) + events_before = EditedAuthorsDocEvent.objects.count() + + post_data = self.make_edit_authors_post_data( + authors = draft.documentauthor_set.all(), + basis=change_reason + ) + + # swap first two authors + post_data['author-0-ORDER'] = 1 + post_data['author-1-ORDER'] = 0 + + self.login('secretary') + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 302) + + draft = Document.objects.get(pk=draft.pk) + after = list(draft.documentauthor_set.values(*compare_props)) + + # swap the 'before' record order + tmp = before[0] + before[0] = before[1] + before[0]['order'] = 1 + before[1] = tmp + before[1]['order'] = 2 + for b, a in zip(before, after): + for prop in compare_props: + self.assertEqual(a[prop], b[prop], + 'Unexpected change: "{}" was "{}", changed to "{}"'.format( + prop, b[prop], a[prop] + )) + + # expect 2 events: one for each changed author + self.assertEqual(EditedAuthorsDocEvent.objects.count(), events_before + 2) + change_events = EditedAuthorsDocEvent.objects.filter(basis=change_reason) + self.assertEqual(change_events.count(), 2) + self.assertEqual(change_events.filter(desc__icontains='changed order').count(), 2) + + self.assertIsNotNone( + change_events.filter( + desc__contains=Person.objects.get(pk=before[0]['person']).name + ).first() + ) + self.assertIsNotNone( + change_events.filter( + desc__contains=Person.objects.get(pk=before[1]['person']).name + ).first() + ) + + def test_edit_authors_edit_fields(self): + draft = WgDraftFactory() + DocumentAuthorFactory.create_batch(3, document=draft) + url = urlreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + change_reason = 'reorder the authors' + + compare_props = 'person', 'email', 'affiliation', 'country', 'order' + before = list(draft.documentauthor_set.values(*compare_props)) + events_before = EditedAuthorsDocEvent.objects.count() + + post_data = self.make_edit_authors_post_data( + authors = draft.documentauthor_set.all(), + basis=change_reason + ) + + new_email = EmailFactory(person=draft.authors()[0]) + post_data['author-0-email'] = new_email.address + post_data['author-1-affiliation'] = 'University of Nowhere' + post_data['author-2-country'] = 'Chile' + + self.login('secretary') + r = self.client.post(url, post_data) + self.assertEqual(r.status_code, 302) + + draft = Document.objects.get(pk=draft.pk) + after = list(draft.documentauthor_set.values(*compare_props)) + + expected = copy.deepcopy(before) + expected[0]['email'] = new_email.address + expected[1]['affiliation'] = 'University of Nowhere' + expected[2]['country'] = 'Chile' + for b, a in zip(expected, after): + for prop in compare_props: + self.assertEqual(a[prop], b[prop], + 'Unexpected change: "{}" was "{}", changed to "{}"'.format( + prop, b[prop], a[prop] + )) + + # expect 3 events: one for each changed author + self.assertEqual(EditedAuthorsDocEvent.objects.count(), events_before + 3) + change_events = EditedAuthorsDocEvent.objects.filter(basis=change_reason) + self.assertEqual(change_events.count(), 3) + + email_event = change_events.filter(desc__icontains='changed email').first() + affiliation_event = change_events.filter(desc__icontains='changed affiliation').first() + country_event = change_events.filter(desc__icontains='changed country').first() + + self.assertIsNotNone(email_event) + self.assertIn(draft.authors()[0].name, email_event.desc) + self.assertIn(before[0]['email'], email_event.desc) + self.assertIn(after[0]['email'], email_event.desc) + + self.assertIsNotNone(affiliation_event) + self.assertIn(draft.authors()[1].name, affiliation_event.desc) + self.assertIn(before[1]['affiliation'], affiliation_event.desc) + self.assertIn(after[1]['affiliation'], affiliation_event.desc) + + self.assertIsNotNone(country_event) + self.assertIn(draft.authors()[2].name, country_event.desc) + self.assertIn(before[2]['country'], country_event.desc) + self.assertIn(after[2]['country'], country_event.desc) + @staticmethod def _pyquery_select_action_holder_string(q, s): """Helper to use PyQuery to find an action holder in the draft HTML""" diff --git a/ietf/doc/tests_js.py b/ietf/doc/tests_js.py new file mode 100644 index 000000000..1709e846c --- /dev/null +++ b/ietf/doc/tests_js.py @@ -0,0 +1,144 @@ +# Copyright The IETF Trust 2021, All Rights Reserved +# -*- coding: utf-8 -*- + +import debug # pyflakes:ignore + +from ietf.doc.factories import WgDraftFactory, DocumentAuthorFactory +from ietf.person.factories import PersonFactory +from ietf.person.models import Person +from ietf.utils.jstest import IetfSeleniumTestCase, ifSeleniumEnabled, selenium_enabled + +if selenium_enabled(): + from selenium.webdriver.common.by import By + from selenium.webdriver.support.ui import WebDriverWait + from selenium.webdriver.support import expected_conditions + + +class presence_of_element_child_by_css_selector: + """Wait for presence of a child of a WebElement matching a CSS selector + + This is a condition class for use with WebDriverWait. + """ + def __init__(self, element, child_selector): + self.element = element + self.child_selector = child_selector + + def __call__(self, driver): + child = self.element.find_element_by_css_selector(self.child_selector) + return child if child is not None else False + +@ifSeleniumEnabled +class EditAuthorsTests(IetfSeleniumTestCase): + def setUp(self): + super(EditAuthorsTests, self).setUp() + self.wait = WebDriverWait(self.driver, 2) + + def test_add_author_forms(self): + def _fill_in_author_form(form_elt, name, email, affiliation, country): + """Fill in an author form on the edit authors page + + The form_elt input should be an element containing all the relevant inputs. + """ + # To enter the person, type their name in the select2 search box, wait for the + # search to offer the result, then press 'enter' to accept the result and close + # the search input. + person_span = form_elt.find_element_by_class_name('select2-chosen') + self.scroll_to_element(person_span) + person_span.click() + input = self.driver.switch_to.active_element + input.send_keys(name) + result_selector = 'ul.select2-results > li > div.select2-result-label' + self.wait.until( + expected_conditions.text_to_be_present_in_element( + (By.CSS_SELECTOR, result_selector), + name + )) + input.send_keys('\n') # select the object + + # After the author is selected, the email select options will be populated. + # Wait for that, then click on the option corresponding to the requested email. + # This will only work if the email matches an address for the selected person. + email_select = form_elt.find_element_by_css_selector('select[name$="email"]') + email_option = self.wait.until( + presence_of_element_child_by_css_selector(email_select, 'option[value="{}"]'.format(email)) + ) + email_option.click() # select the email + + # Fill in the affiliation and country. Finally, simple text inputs! + affil_input = form_elt.find_element_by_css_selector('input[name$="affiliation"]') + affil_input.send_keys(affiliation) + country_input = form_elt.find_element_by_css_selector('input[name$="country"]') + country_input.send_keys(country) + + def _read_author_form(form_elt): + """Read values from an author form + + Note: returns the Person instance named in the person field, not just their name. + """ + hidden_person_input = form_elt.find_element_by_css_selector('input[type="text"][name$="person"]') + email_select = form_elt.find_element_by_css_selector('select[name$="email"]') + affil_input = form_elt.find_element_by_css_selector('input[name$="affiliation"]') + country_input = form_elt.find_element_by_css_selector('input[name$="country"]') + return ( + Person.objects.get(pk=hidden_person_input.get_attribute('value')), + email_select.get_attribute('value'), + affil_input.get_attribute('value'), + country_input.get_attribute('value'), + ) + + # Create testing resources + draft = WgDraftFactory() + DocumentAuthorFactory(document=draft) + authors = PersonFactory.create_batch(2) # authors we will add + orgs = ['some org', 'some other org'] # affiliations for the authors + countries = ['France', 'Uganda'] # countries for the authors + url = self.absreverse('ietf.doc.views_doc.edit_authors', kwargs=dict(name=draft.name)) + + # Star the test by logging in with appropriate permissions and retrieving the edit page + self.login('secretary') + self.driver.get(url) + + # The draft has one author to start with. Find the list and check the count. + authors_list = self.driver.find_element_by_id('authors-list') + author_forms = authors_list.find_elements_by_class_name('author-panel') + self.assertEqual(len(author_forms), 1) + + # get the "add author" button so we can add blank author forms + add_author_button = self.driver.find_element_by_id('add-author-button') + for index, auth in enumerate(authors): + self.scroll_to_element(add_author_button) # Can only click if it's in view! + add_author_button.click() # Create a new form. Automatically scrolls to it. + author_forms = authors_list.find_elements_by_class_name('author-panel') + authors_added = index + 1 + self.assertEqual(len(author_forms), authors_added + 1) # Started with 1 author, hence +1 + _fill_in_author_form(author_forms[index + 1], auth.name, str(auth.email()), orgs[index], countries[index]) + + # Check that the author forms have correct (and distinct) values + first_auth = draft.documentauthor_set.first() + self.assertEqual( + _read_author_form(author_forms[0]), + (first_auth.person, str(first_auth.email), first_auth.affiliation, first_auth.country), + ) + for index, auth in enumerate(authors): + self.assertEqual( + _read_author_form(author_forms[index + 1]), + (auth, str(auth.email()), orgs[index], countries[index]), + ) + + # Must provide a "basis" (change reason) + self.driver.find_element_by_id('id_basis').send_keys('change testing') + # Now click the 'submit' button and check that the update was accepted. + submit_button = self.driver.find_element_by_css_selector('button[type="submit"]') + self.scroll_to_element(submit_button) + submit_button.click() + # Wait for redirect to the document_main view + self.wait.until( + expected_conditions.url_to_be( + self.absreverse('ietf.doc.views_doc.document_main', kwargs=dict(name=draft.name)) + )) + # Just a basic check that the expected authors show up. Details of the updates + # are tested separately. + self.assertEqual( + list(draft.documentauthor_set.values_list('person', flat=True)), + [first_auth.person.pk] + [auth.pk for auth in authors] + ) \ No newline at end of file diff --git a/ietf/doc/urls.py b/ietf/doc/urls.py index 77acfb28c..fc4e0d959 100644 --- a/ietf/doc/urls.py +++ b/ietf/doc/urls.py @@ -113,6 +113,7 @@ urlpatterns = [ url(r'^%(name)s/edit/telechat/$' % settings.URL_REGEXPS, views_doc.telechat_date), url(r'^%(name)s/edit/iesgnote/$' % settings.URL_REGEXPS, views_draft.edit_iesg_note), url(r'^%(name)s/edit/ad/$' % settings.URL_REGEXPS, views_draft.edit_ad), + url(r'^%(name)s/edit/authors/$' % settings.URL_REGEXPS, views_doc.edit_authors), url(r'^%(name)s/edit/consensus/$' % settings.URL_REGEXPS, views_draft.edit_consensus), url(r'^%(name)s/edit/shepherd/$' % settings.URL_REGEXPS, views_draft.edit_shepherd), url(r'^%(name)s/edit/shepherdemail/$' % settings.URL_REGEXPS, views_draft.change_shepherd_email), diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 76aa40ea5..5854a6dcb 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -26,7 +26,7 @@ from ietf.community.utils import docs_tracked_by_community_list from ietf.doc.models import Document, DocHistory, State, DocumentAuthor, DocHistoryAuthor from ietf.doc.models import DocAlias, RelatedDocument, RelatedDocHistory, BallotType, DocReminder from ietf.doc.models import DocEvent, ConsensusDocEvent, BallotDocEvent, IRSGBallotDocEvent, NewRevisionDocEvent, StateDocEvent -from ietf.doc.models import TelechatDocEvent, DocumentActionHolder +from ietf.doc.models import TelechatDocEvent, DocumentActionHolder, EditedAuthorsDocEvent from ietf.name.models import DocReminderTypeName, DocRelationshipName from ietf.group.models import Role, Group from ietf.ietfauth.utils import has_role, is_authorized_in_doc_stream, is_individual_draft_author @@ -517,6 +517,82 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, ) +def update_documentauthors(doc, new_docauthors, by=None, basis=None): + """Update the list of authors for a document + + Returns an iterable of events describing the change. These must be saved by the caller if + they are to be kept. + + The new_docauthors argument should be an iterable containing objects that + have person, email, affiliation, and country attributes. An easy way to create + these objects is to use DocumentAuthor(), but e.g., a named tuple could be + used. These objects will not be saved, their attributes will be used to create new + DocumentAuthor instances. (The document and order fields will be ignored.) + """ + def _change_field_and_describe(auth, field, newval): + # make the change + oldval = getattr(auth, field) + setattr(auth, field, newval) + + was_empty = oldval is None or len(str(oldval)) == 0 + now_empty = newval is None or len(str(oldval)) == 0 + + # describe the change + if oldval == newval: + return None + else: + if was_empty and not now_empty: + return 'set {field} to "{new}"'.format(field=field, new=newval) + elif now_empty and not was_empty: + return 'cleared {field} (was "{old}")'.format(field=field, old=oldval) + else: + return 'changed {field} from "{old}" to "{new}"'.format( + field=field, old=oldval, new=newval + ) + + persons = [] + changes = [] # list of change descriptions + + for order, docauthor in enumerate(new_docauthors): + # If an existing DocumentAuthor matches, use that + auth = doc.documentauthor_set.filter(person=docauthor.person).first() + is_new_auth = auth is None + if is_new_auth: + # None exists, so create a new one (do not just use docauthor here because that + # will modify the input and might cause side effects) + auth = DocumentAuthor(document=doc, person=docauthor.person) + changes.append('Added "{name}" as author'.format(name=auth.person.name)) + + author_changes = [] + # Now fill in other author details + author_changes.append(_change_field_and_describe(auth, 'email', docauthor.email)) + author_changes.append(_change_field_and_describe(auth, 'affiliation', docauthor.affiliation)) + author_changes.append(_change_field_and_describe(auth, 'country', docauthor.country)) + author_changes.append(_change_field_and_describe(auth, 'order', order + 1)) + auth.save() + log.assertion('auth.email_id != "none"') + persons.append(docauthor.person) + if not is_new_auth: + all_author_changes = ', '.join([ch for ch in author_changes if ch is not None]) + if len(all_author_changes) > 0: + changes.append('Changed author "{name}": {changes}'.format( + name=auth.person.name, changes=all_author_changes + )) + + # Finally, remove any authors no longer in the list + removed_authors = doc.documentauthor_set.exclude(person__in=persons) + changes.extend(['Removed "{name}" as author'.format(name=auth.person.name) + for auth in removed_authors]) + removed_authors.delete() + + # Create change events - one event per author added/changed/removed. + # Caller must save these if they want them persisted. + return [ + EditedAuthorsDocEvent( + type='edited_authors', by=by, doc=doc, rev=doc.rev, desc=change, basis=basis + ) for change in changes + ] + def update_reminder(doc, reminder_type_slug, event, due_date): reminder_type = DocReminderTypeName.objects.get(slug=reminder_type_slug) diff --git a/ietf/doc/views_doc.py b/ietf/doc/views_doc.py index be858e16f..c1b4a59eb 100644 --- a/ietf/doc/views_doc.py +++ b/ietf/doc/views_doc.py @@ -54,20 +54,21 @@ import debug # pyflakes:ignore from ietf.doc.models import ( Document, DocAlias, DocHistory, DocEvent, BallotDocEvent, BallotType, ConsensusDocEvent, NewRevisionDocEvent, TelechatDocEvent, WriteupDocEvent, IanaExpertDocEvent, - IESG_BALLOT_ACTIVE_STATES, STATUSCHANGE_RELATIONS, DocumentActionHolder ) + IESG_BALLOT_ACTIVE_STATES, STATUSCHANGE_RELATIONS, DocumentActionHolder, DocumentAuthor ) from ietf.doc.utils import (add_links_in_new_revision_events, augment_events_with_revision, can_adopt_draft, can_unadopt_draft, get_chartering_type, get_tags_for_stream_id, 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, build_doc_meta_block, - augment_docs_and_user_with_user_info, irsg_needed_ballot_positions, add_action_holder_change_event, build_doc_supermeta_block, build_file_urls ) + augment_docs_and_user_with_user_info, irsg_needed_ballot_positions, add_action_holder_change_event, + build_doc_supermeta_block, build_file_urls, update_documentauthors ) from ietf.group.models import Role, Group from ietf.group.utils import can_manage_group_type, can_manage_materials, group_features_role_filter from ietf.ietfauth.utils import ( has_role, is_authorized_in_doc_stream, user_is_person, role_required, is_individual_draft_author) from ietf.name.models import StreamName, BallotPositionName from ietf.utils.history import find_history_active_at -from ietf.doc.forms import TelechatForm, NotifyForm, ActionHoldersForm +from ietf.doc.forms import TelechatForm, NotifyForm, ActionHoldersForm, DocAuthorForm, DocAuthorChangeBasisForm from ietf.doc.mails import email_comment, email_remind_action_holders from ietf.mailtrigger.utils import gather_relevant_expansions from ietf.meeting.models import Session @@ -185,6 +186,8 @@ def document_main(request, name, rev=None): irsg_state = doc.get_state("draft-stream-irtf") can_edit = has_role(request.user, ("Area Director", "Secretariat")) + can_edit_authors = has_role(request.user, ("Secretariat")) + stream_slugs = StreamName.objects.values_list("slug", flat=True) # For some reason, AnonymousUser has __iter__, but is not iterable, # which causes problems in the filter() below. Work around this: @@ -424,6 +427,7 @@ def document_main(request, name, rev=None): latest_revision=latest_revision, latest_rev=latest_rev, can_edit=can_edit, + can_edit_authors=can_edit_authors, can_change_stream=can_change_stream, can_edit_stream_info=can_edit_stream_info, can_edit_individual=can_edit_individual, @@ -1349,6 +1353,76 @@ def edit_notify(request, name): ) +@role_required('Secretariat') +def edit_authors(request, name): + """Edit the authors of a doc""" + class _AuthorsBaseFormSet(forms.BaseFormSet): + HIDE_FIELDS = ['ORDER'] + + def __init__(self, *args, **kwargs): + kwargs['prefix'] = 'author' + super(_AuthorsBaseFormSet, self).__init__(*args, **kwargs) + + def add_fields(self, form, index): + super(_AuthorsBaseFormSet, self).add_fields(form, index) + for fh in self.HIDE_FIELDS: + if fh in form.fields: + form.fields[fh].widget = forms.HiddenInput() + + AuthorFormSet = forms.formset_factory(DocAuthorForm, + formset=_AuthorsBaseFormSet, + can_delete=True, + can_order=True, + extra=0) + doc = get_object_or_404(Document, name=name) + + if request.method == 'POST': + change_basis_form = DocAuthorChangeBasisForm(request.POST) + author_formset = AuthorFormSet(request.POST) + if change_basis_form.is_valid() and author_formset.is_valid(): + docauthors = [] + for form in author_formset.ordered_forms: + if not form.cleaned_data['DELETE']: + docauthors.append( + DocumentAuthor( + # update_documentauthors() will fill in document and order for us + person=form.cleaned_data['person'], + email=form.cleaned_data['email'], + affiliation=form.cleaned_data['affiliation'], + country=form.cleaned_data['country'] + ) + ) + change_events = update_documentauthors( + doc, + docauthors, + request.user.person, + change_basis_form.cleaned_data['basis'] + ) + for event in change_events: + event.save() + return redirect('ietf.doc.views_doc.document_main', name=doc.name) + else: + change_basis_form = DocAuthorChangeBasisForm() + author_formset = AuthorFormSet( + initial=[{ + 'person': author.person, + 'email': author.email, + 'affiliation': author.affiliation, + 'country': author.country, + 'order': author.order, + } for author in doc.documentauthor_set.all()] + ) + return render( + request, + 'doc/edit_authors.html', + { + 'doc': doc, + 'change_basis_form': change_basis_form, + 'formset': author_formset, + 'titletext': doc_titletext(doc), + }) + + @role_required('Area Director', 'Secretariat') def edit_action_holders(request, name): """Change the set of action holders for a doc""" diff --git a/ietf/person/ajax.py b/ietf/person/ajax.py index 88bd24687..ee4cbb0ea 100644 --- a/ietf/person/ajax.py +++ b/ietf/person/ajax.py @@ -3,6 +3,7 @@ import json from django.shortcuts import get_object_or_404 from django.http import HttpResponse +from ietf.ietfauth.utils import role_required from ietf.person.models import Person def person_json(request, personid): @@ -12,3 +13,10 @@ def person_json(request, personid): sort_keys=True, indent=2), content_type="application/json") + +@role_required('Secretariat') +def person_email_json(request, personid): + person = get_object_or_404(Person, pk=personid) + addresses = person.email_set.order_by('-primary').values('address', 'primary') + + return HttpResponse(json.dumps(list(addresses)), content_type='application/json') \ No newline at end of file diff --git a/ietf/person/tests.py b/ietf/person/tests.py index 6dfa34c23..48fd715c9 100644 --- a/ietf/person/tests.py +++ b/ietf/person/tests.py @@ -3,6 +3,7 @@ import datetime +import json from io import StringIO, BytesIO from PIL import Image @@ -47,6 +48,29 @@ class PersonTests(TestCase): data = r.json() self.assertEqual(data[0]["id"], person.email_address()) + def test_ajax_person_email_json(self): + person = PersonFactory() + EmailFactory.create_batch(5, person=person) + primary_email = person.email() + primary_email.primary = True + primary_email.save() + + bad_url = urlreverse('ietf.person.ajax.person_email_json', kwargs=dict(personid=12345)) + url = urlreverse('ietf.person.ajax.person_email_json', kwargs=dict(personid=person.pk)) + + login_testing_unauthorized(self, 'secretary', bad_url) + r = self.client.get(bad_url) + self.assertEqual(r.status_code, 404) + self.client.logout() + + login_testing_unauthorized(self, 'secretary', url) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + self.assertCountEqual( + json.loads(r.content), + [dict(address=email.address, primary=email.primary) for email in person.email_set.all()], + ) + def test_default_email(self): person = PersonFactory() primary = EmailFactory(person=person, primary=True, active=True) diff --git a/ietf/person/urls.py b/ietf/person/urls.py index 5d5d113e7..ffcda989d 100644 --- a/ietf/person/urls.py +++ b/ietf/person/urls.py @@ -5,6 +5,7 @@ urlpatterns = [ url(r'^merge/$', views.merge), url(r'^search/(?P(person|email))/$', views.ajax_select2_search), url(r'^(?P[a-z0-9]+).json$', ajax.person_json), + url(r'^(?P[a-z0-9]+)/email.json$', ajax.person_email_json), url(r'^(?P[^/]+)$', views.profile), url(r'^(?P[^/]+)/photo/?$', views.photo), ] diff --git a/ietf/submit/utils.py b/ietf/submit/utils.py index 27a14420f..1f16a4e98 100644 --- a/ietf/submit/utils.py +++ b/ietf/submit/utils.py @@ -25,7 +25,8 @@ from ietf.doc.models import ( Document, State, DocAlias, DocEvent, SubmissionDoc from ietf.doc.models import NewRevisionDocEvent from ietf.doc.models import RelatedDocument, DocRelationshipName, DocExtResource from ietf.doc.utils import add_state_change_event, rebuild_reference_relations -from ietf.doc.utils import set_replaces_for_document, prettify_std_name, update_doc_extresources, can_edit_docextresources +from ietf.doc.utils import ( set_replaces_for_document, prettify_std_name, + update_doc_extresources, can_edit_docextresources, update_documentauthors ) from ietf.doc.mails import send_review_possibly_replaces_request, send_external_resource_change_request from ietf.group.models import Group from ietf.ietfauth.utils import has_role @@ -572,24 +573,21 @@ def ensure_person_email_info_exists(name, email, docname): return person, email def update_authors(draft, submission): - persons = [] - for order, author in enumerate(submission.authors): + docauthors = [] + for author in submission.authors: person, email = ensure_person_email_info_exists(author["name"], author.get("email"), submission.name) - - a = DocumentAuthor.objects.filter(document=draft, person=person).first() - if not a: - a = DocumentAuthor(document=draft, person=person) - - a.email = email - a.affiliation = author.get("affiliation") or "" - a.country = author.get("country") or "" - a.order = order - a.save() - log.assertion('a.email_id != "none"') - - persons.append(person) - - draft.documentauthor_set.exclude(person__in=persons).delete() + docauthors.append( + DocumentAuthor( + # update_documentauthors() will fill in document and order for us + person=person, + email=email, + affiliation=author.get("affiliation", ""), + country=author.get("country", "") + ) + ) + # The update_documentauthors() method returns a list of unsaved author edit events for the draft. + # Discard these because the existing logging is already adequate. + update_documentauthors(draft, docauthors) def cancel_submission(submission): submission.state = DraftSubmissionStateName.objects.get(slug="cancel") diff --git a/ietf/templates/doc/document_draft.html b/ietf/templates/doc/document_draft.html index 39b018383..a8e07efe4 100644 --- a/ietf/templates/doc/document_draft.html +++ b/ietf/templates/doc/document_draft.html @@ -75,7 +75,11 @@ Author{{doc.authors|pluralize}} - + + {% if can_edit_authors %} + Edit + {% endif %} + {# Implementation that uses the current primary email for each author #} {% for author in doc.authors %} diff --git a/ietf/templates/doc/edit_authors.html b/ietf/templates/doc/edit_authors.html new file mode 100644 index 000000000..57d230435 --- /dev/null +++ b/ietf/templates/doc/edit_authors.html @@ -0,0 +1,160 @@ +{% extends "base.html" %} +{# Copyright The IETF Trust 2021, All Rights Reserved #} +{% load origin %} +{% load static %} +{% load bootstrap3 %} + +{% block pagehead %} + + +{% endblock %} + +{% block morecss %} + + #empty-author-form { + display: none; + } +{% endblock %} + +{% block title %} + Edit authors for {{ titletext }} +{% endblock %} + +{% block content %} + {% origin %} +

Edit authors
{{ titletext }}

+ +
+ {% csrf_token %} + {% bootstrap_form change_basis_form %} + + {% buttons %} + + {% endbuttons %} + + {% bootstrap_form formset.management_form %} +
+ {% for form in formset %} +
+
+ +
+ {% bootstrap_form form layout='horizontal' %} +
+
+
+ {% endfor %} +
+
+
+
+ +
+ {% bootstrap_form formset.empty_form layout='horizontal' %} +
+
+
+
+ + {% buttons %} + + Back + {% endbuttons %} +
+ +{% endblock %} + +{% block js %} + + + + + +{% endblock %} \ No newline at end of file diff --git a/ietf/utils/jstest.py b/ietf/utils/jstest.py index b425dc965..bd6f53945 100644 --- a/ietf/utils/jstest.py +++ b/ietf/utils/jstest.py @@ -8,6 +8,7 @@ skip_selenium = False skip_message = "" try: from selenium import webdriver + from selenium.webdriver.common.action_chains import ActionChains except ImportError as e: skip_selenium = True skip_message = "Skipping selenium tests: %s" % e @@ -72,3 +73,8 @@ class IetfSeleniumTestCase(IetfLiveServerTestCase): self.driver.find_element_by_name('password').send_keys(password) self.driver.find_element_by_xpath('//button[@type="submit"]').click() + def scroll_to_element(self, element): + """Scroll an element into view""" + actions = ActionChains(self.driver) + actions.move_to_element(element).perform() + diff --git a/ietf/utils/test_utils.py b/ietf/utils/test_utils.py index 6409984c8..495ee77ad 100644 --- a/ietf/utils/test_utils.py +++ b/ietf/utils/test_utils.py @@ -63,8 +63,16 @@ def split_url(url): args = {} return url, args -def login_testing_unauthorized(test_case, username, url, password=None): - r = test_case.client.get(url) +def login_testing_unauthorized(test_case, username, url, password=None, method='get', request_kwargs=None): + """Test that a request is refused or redirected for login, then log in as the named user + + Defaults to making a 'get'. Set method to one of the other django.test.Client request method names + (e.g., 'post') to change that. If that request needs arguments, pass these in request_kwargs. + """ + request_method = getattr(test_case.client, method) + if request_kwargs is None: + request_kwargs = dict() + r = request_method(url, **request_kwargs) test_case.assertIn(r.status_code, (302, 403)) if r.status_code == 302: test_case.assertTrue("/accounts/login" in r['Location'])