From 608b8e16a4cf0dd5dca43159a3c8809cb98c6d15 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 26 Jul 2022 12:23:00 -0400 Subject: [PATCH] feat: only offer IAB/IESG members for bofreq responsible leadership (#4276) * refactor: avoid using select2 data-* attrs on html elements Using "data-ajax--url" shadows explicit configuration in our select2.js wrapper. Use "data-select2-ajax-url" to avoid this. Also add ability to omit the ajax setup entirely by returning None from ajax_url(). * chore: hook up a flag to disable ajax for SearchablePersonsField * refactor: send select2 prefetch data as array and allow config of min input length * feat: only offer IAB/IESG members for bofreq responsible leadership * test: area directors/IAB members should be options for bofreq responsible leaders * test: update tests to match changes to SearchableField * fix: clean up SearchablePersonsField breakage when searching by email address * chore: finish incomplete comment --- ietf/doc/tests.py | 6 +-- ietf/doc/tests_bofreq.py | 22 +++++++++++ ietf/doc/views_bofreq.py | 19 +++++++++- ietf/liaisons/tests.py | 2 +- ietf/person/fields.py | 38 ++++++++++++------- ietf/static/js/select2.js | 20 +++++++--- ietf/templates/base.html | 2 +- .../doc/status_change/edit_related_rows.html | 4 +- ietf/utils/fields.py | 13 +++++-- 9 files changed, 95 insertions(+), 31 deletions(-) diff --git a/ietf/doc/tests.py b/ietf/doc/tests.py index 4fbad71ef..e0e1204c2 100644 --- a/ietf/doc/tests.py +++ b/ietf/doc/tests.py @@ -2468,12 +2468,12 @@ class FieldTests(TestCase): decoded = json.loads(json_data) except json.JSONDecodeError as e: self.fail('data-pre contained invalid JSON data: %s' % str(e)) - decoded_ids = list(decoded.keys()) - self.assertCountEqual(decoded_ids, [str(doc.id) for doc in docs]) + decoded_ids = [item['id'] for item in decoded] + self.assertEqual(decoded_ids, [doc.id for doc in docs]) for doc in docs: self.assertEqual( dict(id=doc.pk, selected=True, url=doc.get_absolute_url(), text=escape(uppercase_std_abbreviated_name(doc.name))), - decoded[str(doc.pk)], + decoded[decoded_ids.index(doc.pk)], ) class MaterialsTests(TestCase): diff --git a/ietf/doc/tests_bofreq.py b/ietf/doc/tests_bofreq.py index aecfe5acb..4d02108d0 100644 --- a/ietf/doc/tests_bofreq.py +++ b/ietf/doc/tests_bofreq.py @@ -2,6 +2,7 @@ import datetime import debug # pyflakes:ignore +import json import os from pathlib import Path @@ -18,7 +19,9 @@ from ietf.group.factories import RoleFactory from ietf.doc.factories import BofreqFactory, NewRevisionDocEventFactory from ietf.doc.models import State, Document, DocAlias, NewRevisionDocEvent from ietf.doc.utils_bofreq import bofreq_editors, bofreq_responsible +from ietf.ietfauth.utils import has_role from ietf.person.factories import PersonFactory +from ietf.person.models import Person from ietf.utils.mail import outbox, empty_outbox from ietf.utils.test_utils import TestCase, reload_db_objects, unicontent, login_testing_unauthorized from ietf.utils.text import xslugify @@ -270,6 +273,25 @@ This test section has some text. for p in bad_batch: self.assertIn(p.plain_name(), error_text) + def test_change_responsible_options(self): + """Only valid options should be offered for responsible leadership field""" + doc = BofreqFactory() + url = urlreverse('ietf.doc.views_bofreq.change_responsible', kwargs={'name': doc.name}) + self.client.login(username='secretary', password='secretary+password') + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + q = PyQuery(r.content) + option_ids = [opt['id'] for opt in json.loads(q('#id_responsible').attr('data-pre'))] + ad_people = [p for p in Person.objects.all() if has_role(p.user, 'Area Director')] + iab_people = [p for p in Person.objects.all() if has_role(p.user, 'IAB')] + self.assertGreater(len(ad_people), 0, 'Need at least one AD') + self.assertGreater(len(iab_people), 0, 'Need at least one IAB member') + self.assertGreater(Person.objects.count(), len(ad_people) + len(iab_people), + 'Need at least one Person not an AD nor IAB member') + # Next line will fail if there's overlap between ad_people and iab_people. This is by design. + # If the test setup changes and overlap is expected, need to separately check that area directors + # and IAB members wind up in the options list. + self.assertCountEqual(option_ids, [p.pk for p in ad_people + iab_people]) def test_submit(self): doc = BofreqFactory() diff --git a/ietf/doc/views_bofreq.py b/ietf/doc/views_bofreq.py index 80baaf3d6..92a130efb 100644 --- a/ietf/doc/views_bofreq.py +++ b/ietf/doc/views_bofreq.py @@ -7,6 +7,7 @@ import io from django import forms from django.conf import settings from django.contrib.auth.decorators import login_required +from django.db.models import Q from django.shortcuts import get_object_or_404, redirect, render from django.template.loader import render_to_string from django.urls import reverse as urlreverse @@ -20,6 +21,7 @@ from ietf.doc.utils import add_state_change_event from ietf.doc.utils_bofreq import bofreq_editors, bofreq_responsible from ietf.ietfauth.utils import has_role, role_required from ietf.person.fields import SearchablePersonsField +from ietf.person.models import Person from ietf.utils import markdown from ietf.utils.response import permission_denied from ietf.utils.text import xslugify @@ -224,7 +226,22 @@ def change_editors(request, name): class ChangeResponsibleForm(forms.Form): - responsible = SearchablePersonsField(required=False) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # The queryset in extra_prefetch finds users for whom has_role(u, 'Area Director') or + # has_role(u, 'IAB') is True. It needs to match the queries in the has_role() method. + # Disable ajax requests because the SearchablePersonsField cannot enforce the queryset filter. + self.fields['responsible'] = SearchablePersonsField( + required=False, + extra_prefetch=Person.objects.filter( + Q(role__name='member', role__group__acronym='iab') + | Q(role__name__in=('pre-ad', 'ad'), role__group__type='area', role__group__state='active') + ).distinct(), + disable_ajax=True, # only use the prefetched options + min_search_length=0, # do not require typing to display options + ) + def clean_responsible(self): responsible = self.cleaned_data['responsible'] not_leadership = list() diff --git a/ietf/liaisons/tests.py b/ietf/liaisons/tests.py index 4e4eaa136..b08832ae5 100644 --- a/ietf/liaisons/tests.py +++ b/ietf/liaisons/tests.py @@ -1014,7 +1014,7 @@ class LiaisonManagementTests(TestCase): self.assertEqual(q('#id_from_groups').find('option:selected').val(),reply_from_group_id) self.assertEqual(q('#id_to_groups').find('option:selected').val(),reply_to_group_id) pre = json.loads(q('#id_related_to').attr("data-pre")) - self.assertEqual(pre[str(liaison.pk)]['id'], liaison.pk) + self.assertEqual(pre[0]['id'], liaison.pk) def test_search(self): # Statement 1 diff --git a/ietf/person/fields.py b/ietf/person/fields.py index 654508fb5..e15be489b 100644 --- a/ietf/person/fields.py +++ b/ietf/person/fields.py @@ -9,6 +9,7 @@ from urllib.parse import urlencode from typing import Type # pyflakes:ignore +import unidecode from django import forms from django.core.validators import validate_email from django.db import models # pyflakes:ignore @@ -62,6 +63,9 @@ class SearchablePersonsField(SearchableField): that may be added to the initial set should be included in the extra_prefetch list. These can then be added by updating val() and triggering the 'change' event on the select2 field in JavaScript. + + If disable_ajax is True, only objects that are prefetched can be selected. This + will be any currently selected items plus any in extra_prefetch. """ model = Person # type: Type[models.Model] default_hint_text = "Type name to search for person." @@ -69,12 +73,14 @@ class SearchablePersonsField(SearchableField): only_users=False, # only select persons who also have a user all_emails=False, # select only active email addresses extra_prefetch=None, # extra data records to include in prefetch + disable_ajax=False, # use ajax to search outside of prefetched set *args, **kwargs): super(SearchablePersonsField, self).__init__(*args, **kwargs) self.only_users = only_users self.all_emails = all_emails self.extra_prefetch = extra_prefetch or [] assert all([isinstance(obj, self.model) for obj in self.extra_prefetch]) + self.disable_ajax = disable_ajax def validate_pks(self, pks): """Validate format of PKs""" @@ -87,21 +93,27 @@ class SearchablePersonsField(SearchableField): # via the extra_prefetch property. prefetch_set = set(model_instances) if model_instances else set() prefetch_set = prefetch_set.union(set(self.extra_prefetch)) # eliminate duplicates - return select2_id_name(list(prefetch_set)) + return sorted( + select2_id_name(list(prefetch_set)), + key=lambda item: unidecode.unidecode(item['text']), + ) def ajax_url(self): - url = urlreverse( - "ietf.person.views.ajax_select2_search", - kwargs={ "model_name": self.model.__name__.lower() } - ) - query_args = {} - if self.only_users: - query_args["user"] = "1" - if self.all_emails: - query_args["a"] = "1" - if len(query_args) > 0: - url += '?%s' % urlencode(query_args) - return url + if self.disable_ajax: + return None + else: + url = urlreverse( + "ietf.person.views.ajax_select2_search", + kwargs={ "model_name": self.model.__name__.lower() } + ) + query_args = {} + if self.only_users: + query_args["user"] = "1" + if self.all_emails: + query_args["a"] = "1" + if len(query_args) > 0: + url += '?%s' % urlencode(query_args) + return url class SearchablePersonField(SearchablePersonsField): diff --git a/ietf/static/js/select2.js b/ietf/static/js/select2.js index 5b3b28235..5e282e6f8 100644 --- a/ietf/static/js/select2.js +++ b/ietf/static/js/select2.js @@ -22,12 +22,19 @@ function prettify_tz(x) { // Copyright The IETF Trust 2015-2021, All Rights Reserved // JS for ietf.utils.fields.SearchableField subclasses window.setupSelect2Field = function (e) { - var url = e.data("ajax--url"); - var maxEntries = e.data("max-entries"); - var result_key = e.data("result-key"); - var options = e.data("pre"); - for (var id in options) { - e.append(new Option(options[id].text, options[id].id, false, options[id].selected)); + // Avoid using data attributes that match any of the search2 option attributes. + // These will override settings in the options object that we use below. + // (see https://select2.org/configuration/data-attributes) + // Note: e.data('k') returns undefined if there is no data-k attribute. + let url = e.data("select2-ajax-url"); + let maxEntries = e.data("max-entries"); + let minSearchLength = e.data("min-search-length"); + let result_key = e.data("result-key"); + let options = e.data("pre"); + if (options) { + options.forEach( + opt => e.append(new Option(opt.text, opt.id, false, opt.selected)) + ); } template_modify = e.hasClass("tz-select") ? prettify_tz : undefined; @@ -35,6 +42,7 @@ window.setupSelect2Field = function (e) { e.select2({ multiple: maxEntries !== 1, maximumSelectionSize: maxEntries, + minimumInputLength: minSearchLength, templateResult: template_modify, templateSelection: template_modify, ajax: url ? { diff --git a/ietf/templates/base.html b/ietf/templates/base.html index 672eeb7b9..0ff81fd3a 100644 --- a/ietf/templates/base.html +++ b/ietf/templates/base.html @@ -61,7 +61,7 @@ diff --git a/ietf/templates/doc/status_change/edit_related_rows.html b/ietf/templates/doc/status_change/edit_related_rows.html index 837018db2..8912c99fd 100644 --- a/ietf/templates/doc/status_change/edit_related_rows.html +++ b/ietf/templates/doc/status_change/edit_related_rows.html @@ -5,7 +5,7 @@ {% for rfc,choice_slug in form.relations.items %}