Clean up handling of non-WG groups on the group edit page; restrict parent/child group relationships by type. Fixes ietf-tools#3253. Commit ready for merge.

 - Legacy-Id: 19075

Author: jennifer-richards

ietf/group/admin.py

@@ -21,6 +21,7 @@
 from ietf.group.models import (Group, GroupFeatures, GroupHistory, GroupEvent, GroupURL, GroupMilestone,
     GroupMilestoneHistory, GroupStateTransitions, Role, RoleHistory, ChangeStateGroupEvent,
     MilestoneGroupEvent, GroupExtResource, )
+from ietf.name.models import GroupTypeName
 
 from ietf.utils.validators import validate_external_resource_value
 from ietf.utils.response import permission_denied
@@ -139,10 +140,40 @@ def send_one_reminder(self, request, object_id):
 
 admin.site.register(Group, GroupAdmin)
 
+
+class GroupFeaturesAdminForm(forms.ModelForm):
+    def clean_default_parent(self):
+        # called before form clean() method -- cannot access other fields
+        parent_acro = self.cleaned_data['default_parent'].strip().lower()
+        if len(parent_acro) > 0:
+            if Group.objects.filter(acronym=parent_acro).count() == 0:
+                raise forms.ValidationError(
+                    'No group exists with acronym "%(acro)s"',
+                    params=dict(acro=parent_acro),
+                )
+        return parent_acro
+
+    def clean(self):
+        # cleaning/validation that requires multiple fields
+        parent_acro = self.cleaned_data['default_parent']
+        if len(parent_acro) > 0:
+            parent_type = GroupTypeName.objects.filter(group__acronym=parent_acro).first()
+            if parent_type not in self.cleaned_data['parent_types']:
+                self.add_error(
+                    'default_parent',
+                    forms.ValidationError(
+                        'Default parent group "%(acro)s" is type "%(gtype)s", which is not an allowed parent type.',
+                        params=dict(acro=parent_acro, gtype=parent_type),
+                    )
+                )
+
 class GroupFeaturesAdmin(admin.ModelAdmin):
+    form = GroupFeaturesAdminForm
     list_display = [
-
         'type',
+        'need_parent',
+        'default_parent',
+        'gf_parent_types',
         'has_milestones',
         'has_chartering_process',
         'has_documents',
@@ -165,8 +196,13 @@ class GroupFeaturesAdmin(admin.ModelAdmin):
         'groupman_roles',
         'matman_roles',
         'role_order',
-
     ]
+
+    def gf_parent_types(self, groupfeatures):
+        """Generate list of parent types; needed because many-to-many is not handled automatically"""
+        return ', '.join([gtn.slug for gtn in groupfeatures.parent_types.all()])
+    gf_parent_types.short_description = 'Parent Types'   # type: ignore # https://github.com/python/mypy/issues/2087
+
 admin.site.register(GroupFeatures, GroupFeaturesAdmin)
 
 class GroupHistoryAdmin(admin.ModelAdmin):

ietf/group/forms.py

@@ -18,10 +18,11 @@
 from ietf.group.models import Group, GroupHistory, GroupStateName, GroupFeatures 
 from ietf.name.models import ReviewTypeName, RoleName, ExtResourceName
 from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField
-from ietf.person.models import Person, Email
+from ietf.person.models import Email
 from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings
 from ietf.review.policies import get_reviewer_queue_policy
 from ietf.review.utils import close_review_request_states
+from ietf.utils import log
 from ietf.utils.textupload import get_cleaned_text_file_content
 #from ietf.utils.ordereddict import insert_after_in_ordered_dict
 from ietf.utils.fields import DatepickerDateField, MultiEmailField
@@ -60,7 +61,6 @@ class GroupForm(forms.Form):
     acronym = forms.CharField(max_length=40, label="Acronym", required=True)
     state = forms.ModelChoiceField(GroupStateName.objects.all(), label="State", required=True)
     # Note that __init__ will add role fields here
-    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=False)
 
     parent = forms.ModelChoiceField(Group.objects.filter(state="active").order_by('name'), empty_label="(None)", required=False)
     list_email = forms.CharField(max_length=64, required=False)
@@ -74,9 +74,25 @@ def __init__(self, *args, **kwargs):
         self.group = kwargs.pop('group', None)
         self.group_type = kwargs.pop('group_type', False)
         if self.group:
-            self.used_roles = self.group.used_roles or self.group.features.default_used_roles
+            group_features = self.group.features
+            self.used_roles = self.group.used_roles or group_features.default_used_roles
         else:
-            self.used_roles = GroupFeatures.objects.get(type=self.group_type).default_used_roles
+            group_features = GroupFeatures.objects.filter(type_id=self.group_type).first()
+
+        log.assertion('group_features is not None')
+        if group_features is not None:
+            self.used_roles = group_features.default_used_roles
+            parent_types = group_features.parent_types.all()
+            need_parent = group_features.need_parent
+            default_parent = group_features.default_parent
+        else:
+            # This should not happen, but in the absence of constraints that ensure it
+            # cannot, prevent the form from breaking if it does.
+            self.used_roles = []
+            parent_types = GroupFeatures.objects.none()
+            need_parent = False
+            default_parent = None
+
         if "field" in kwargs:
             field = kwargs["field"]
             del kwargs["field"]
@@ -109,22 +125,21 @@ def __init__(self, *args, **kwargs):
         if self.group_type == "rg":
             self.fields["state"].queryset = self.fields["state"].queryset.exclude(slug__in=("bof", "bof-conc"))
 
-        # if previous AD is now ex-AD, append that person to the list
-        ad_pk = self.initial.get('ad')
-        choices = self.fields['ad'].choices
-        if ad_pk and ad_pk not in [pk for pk, name in choices]:
-            self.fields['ad'].choices = list(choices) + [("", "-------"), (ad_pk, Person.objects.get(pk=ad_pk).plain_name())]
-
         if self.group:
             self.fields['acronym'].widget.attrs['readonly'] = ""
 
-        if self.group_type == "rg":
-            self.fields['ad'].widget = forms.HiddenInput()
-            self.fields['parent'].queryset = self.fields['parent'].queryset.filter(acronym="irtf")
-            self.fields['parent'].initial = self.fields['parent'].queryset.first()
-            self.fields['parent'].widget = forms.HiddenInput()
-        else:
-            self.fields['parent'].queryset = self.fields['parent'].queryset.filter(type="area")
+        # Sort out parent options
+        self.fields['parent'].queryset = self.fields['parent'].queryset.filter(type__in=parent_types)
+        if need_parent:
+            self.fields['parent'].required = True
+            self.fields['parent'].empty_label = None
+        # if this is a new group, fill in the default parent, if any
+        if self.group is None or (not hasattr(self.group, 'pk')):
+            self.fields['parent'].initial = self.fields['parent'].queryset.filter(
+                acronym=default_parent
+            ).first()
+        # label the parent field as 'IETF Area' if appropriate, for consistency with past behavior
+        if parent_types.count() == 1 and parent_types.first().pk == 'area':
             self.fields['parent'].label = "IETF Area"
 
         if field:

ietf/group/migrations/0043_add_groupfeatures_parent_type_fields.py

@@ -0,0 +1,29 @@
+# Generated by Django 2.2.19 on 2021-04-13 05:17
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('name', '0023_change_stream_descriptions'),
+        ('group', '0042_add_liaison_contact_roles_to_used_roles'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='groupfeatures',
+            name='parent_types',
+            field=models.ManyToManyField(blank=True, help_text='Group types allowed as parent of this group type', related_name='child_features', to='name.GroupTypeName'),
+        ),
+        migrations.AddField(
+            model_name='groupfeatures',
+            name='req_parent',
+            field=models.BooleanField(default=False, help_text='Does this group type require a parent group?', verbose_name='Need Parent'),
+        ),
+        migrations.AddField(
+            model_name='groupfeatures',
+            name='default_parent',
+            field=models.CharField(blank=True, default='', help_text='Default parent group acronym for this group type', max_length=40, verbose_name='Default Parent'),
+        ),
+    ]

ietf/group/migrations/0044_populate_groupfeatures_parent_type_fields.py

@@ -0,0 +1,91 @@
+# Generated by Django 2.2.19 on 2021-04-13 09:17
+
+from django.db import migrations
+
+def populate_parent_types(apps, schema_editor):
+    """Add default parent_types entries
+
+    Data were determined from existing groups via this query:
+    {t.slug: list(
+        Group.objects.filter(type=t, parent__isnull=False).values_list('parent__type', flat=True).distinct()
+    ) for t in GroupTypeName.objects.all()}
+    """
+    GroupFeatures = apps.get_model('group', 'GroupFeatures')
+    GroupTypeName = apps.get_model('name', 'GroupTypeName')
+    type_map = {
+        'adhoc': ['ietf'],
+        'admin': [],
+        'ag': ['area', 'ietf'],
+        'area': ['ietf'],
+        'dir': ['area'],
+        'iab': ['ietf'],
+        'iana': [],
+        'iesg': [],
+        'ietf': ['ietf'],
+        'individ': ['area'],
+        'irtf': ['irtf'],
+        'ise': [],
+        'isoc': ['isoc'],
+        'nomcom': ['area'],
+        'program': ['ietf'],
+        'rag': ['irtf'],
+        'review': ['area'],
+        'rfcedtyp': [],
+        'rg': ['irtf'],
+        'sdo': ['sdo', 'area'],
+        'team': ['area'],
+        'wg': ['area']
+    }
+    for type_slug, parent_slugs in type_map.items():
+        if len(parent_slugs) > 0:
+            features = GroupFeatures.objects.get(type__slug=type_slug)
+            features.parent_types.add(*GroupTypeName.objects.filter(slug__in=parent_slugs))
+
+    # validate
+    for gtn in GroupTypeName.objects.all():
+        slugs_in_db = set(type.slug for type in gtn.features.parent_types.all())
+        assert(slugs_in_db == set(type_map[gtn.slug]))
+
+
+def set_req_parent_values(apps, schema_editor):
+    """Set req_parent values
+
+    Data determined from existing groups using:
+
+    GroupTypeName.objects.exclude(pk__in=Group.objects.filter(parent__isnull=True).values('type'))
+
+    'iesg' has been removed because there are no groups of this type, so no parent types have
+    been made available to it.
+    """
+    GroupFeatures = apps.get_model('group', 'GroupFeatures')
+
+    GroupFeatures.objects.filter(
+        type_id__in=('area', 'dir', 'individ', 'review', 'rg',)
+    ).update(req_parent=True)
+
+
+def set_default_parents(apps, schema_editor):
+    GroupFeatures = apps.get_model('group', 'GroupFeatures')
+
+    # rg-typed groups are children of the irtf group
+    rg_features = GroupFeatures.objects.filter(type_id='rg').first()
+    if rg_features:
+        rg_features.default_parent = 'irtf'
+        rg_features.save()
+
+
+def empty_reverse(apps, schema_editor):
+    pass  # nothing to do, field will be dropped
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('group', '0043_add_groupfeatures_parent_type_fields'),
+    ]
+
+    operations = [
+        migrations.RunPython(populate_parent_types, empty_reverse),
+        migrations.RunPython(set_req_parent_values, empty_reverse),
+        migrations.RunPython(set_default_parents, empty_reverse),
+    ]

ietf/group/models.py

@@ -95,6 +95,9 @@ def area(self):
             return self.parent
         return None
 
+    def get_used_roles(self):
+        return self.used_roles if len(self.used_roles) > 0 else self.features.default_used_roles
+
     class Meta:
         abstract = True
 
@@ -250,6 +253,14 @@ def get_description(self):
 class GroupFeatures(models.Model):
     type = OneToOneField(GroupTypeName, primary_key=True, null=False, related_name='features')
     #history = HistoricalRecords()
+
+    #
+    need_parent = models.BooleanField("Need Parent", default=False, help_text="Does this group type require a parent group?")
+    parent_types = models.ManyToManyField(GroupTypeName, blank=True, related_name='child_features',
+                                          help_text="Group types allowed as parent of this group type")
+    default_parent = models.CharField("Default Parent", max_length=40, blank=True, default="",
+                                       help_text="Default parent group acronym for this group type")
+
     #
     has_milestones          = models.BooleanField("Milestones", default=False)
     has_chartering_process  = models.BooleanField("Chartering", default=False)