Tighten validation of session conflicts in the session request tool,

preventing conflicts where source = target. Make sure the "use
previous session" button doesn't carry forward conflicts that are no
longer valid.

Also fix a bug with the group list in the dropdowns being computed
statically at module import time instead of being queried dynamically
upon each page request.

Commit ready for merge
 - Legacy-Id: 17217

ietf/secr/sreq/forms.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2013-2019, All Rights Reserved
+# Copyright The IETF Trust 2013-2020, All Rights Reserved
 # -*- coding: utf-8 -*-
 
 
@@ -20,20 +20,24 @@
 NUM_SESSION_CHOICES = (('','--Please select'),('1','1'),('2','2'))
 # LENGTH_SESSION_CHOICES = (('','--Please select'),('1800','30 minutes'),('3600','1 hour'),('5400','1.5 hours'), ('7200','2 hours'),('9000','2.5 hours'))
 LENGTH_SESSION_CHOICES = (('','--Please select'),('1800','30 minutes'),('3600','1 hour'),('5400','1.5 hours'), ('7200','2 hours'))
-WG_CHOICES = list( Group.objects.filter(type__in=('wg','rg','ag'),state__in=('bof','proposed','active')).values_list('acronym','acronym').order_by('acronym')) # type:ignore
-WG_CHOICES.insert(0,('','--Select WG(s)')) # type:ignore
 
 # -------------------------------------------------
 # Helper Functions
 # -------------------------------------------------
-def check_conflict(groups):
+def allowed_conflicting_groups():
+    return Group.objects.filter(type__in=['wg', 'ag', 'rg'], state__in=['bof', 'proposed', 'active'])
+
+def check_conflict(groups, source_group):
     '''
     Takes a string which is a list of group acronyms.  Checks that they are all active groups
     '''
     # convert to python list (allow space or comma separated lists)
     items = groups.replace(',',' ').split()
-    active_groups = Group.objects.filter(type__in=('wg','ag','rg'), state__in=('bof','proposed','active'))
+    active_groups = allowed_conflicting_groups()
     for group in items:
+        if group == source_group.acronym:
+            raise forms.ValidationError("Cannot declare a conflict with the same group: %s" % group)
+
         if not active_groups.filter(acronym=group):
             raise forms.ValidationError("Invalid or inactive group acronym: %s" % group)
 
@@ -67,28 +71,39 @@ class SessionForm(forms.Form):
     length_session2 = forms.ChoiceField(choices=LENGTH_SESSION_CHOICES,required=False)
     length_session3 = forms.ChoiceField(choices=LENGTH_SESSION_CHOICES,required=False)
     attendees = forms.IntegerField()
+    # FIXME: it would cleaner to have these be
+    # ModelMultipleChoiceField, and just customize the widgetry, that
+    # way validation comes for free
     conflict1 = forms.CharField(max_length=255,required=False)
     conflict2 = forms.CharField(max_length=255,required=False)
     conflict3 = forms.CharField(max_length=255,required=False)
     comments = forms.CharField(max_length=200,required=False)
-    wg_selector1 = forms.ChoiceField(choices=WG_CHOICES,required=False)
-    wg_selector2 = forms.ChoiceField(choices=WG_CHOICES,required=False)
-    wg_selector3 = forms.ChoiceField(choices=WG_CHOICES,required=False)
+    wg_selector1 = forms.ChoiceField(choices=[],required=False)
+    wg_selector2 = forms.ChoiceField(choices=[],required=False)
+    wg_selector3 = forms.ChoiceField(choices=[],required=False)
     third_session = forms.BooleanField(required=False)
     resources     = forms.MultipleChoiceField(widget=forms.CheckboxSelectMultiple,required=False)
     bethere       = SearchablePersonsField(label="Must be present", required=False)
 
-    def __init__(self, *args, **kwargs):
+    def __init__(self, group, *args, **kwargs):
         if 'hidden' in kwargs:
             self.hidden = kwargs.pop('hidden')
         else:
             self.hidden = False
+
+        self.group = group
+
         super(SessionForm, self).__init__(*args, **kwargs)
         self.fields['num_session'].widget.attrs['onChange'] = "stat_ls(this.selectedIndex);"
         self.fields['length_session1'].widget.attrs['onClick'] = "if (check_num_session(1)) this.disabled=true;"
         self.fields['length_session2'].widget.attrs['onClick'] = "if (check_num_session(2)) this.disabled=true;"
         self.fields['length_session3'].widget.attrs['onClick'] = "if (check_third_session()) { this.disabled=true;}"
         self.fields['comments'].widget = forms.Textarea(attrs={'rows':'6','cols':'65'})
+
+        group_acronym_choices = [('','--Select WG(s)')] + list(allowed_conflicting_groups().exclude(pk=group.pk).values_list('acronym','acronym').order_by('acronym'))
+        for i in range(1, 4):
+            self.fields['wg_selector{}'.format(i)].choices = group_acronym_choices
+
         # disabling handleconflictfield (which only enables or disables form elements) while we're hacking the meaning of the three constraints currently in use:
         #self.fields['wg_selector1'].widget.attrs['onChange'] = "document.form_post.conflict1.value=document.form_post.conflict1.value + ' ' + this.options[this.selectedIndex].value; return handleconflictfield(1);"
         #self.fields['wg_selector2'].widget.attrs['onChange'] = "document.form_post.conflict2.value=document.form_post.conflict2.value + ' ' + this.options[this.selectedIndex].value; return handleconflictfield(2);"
@@ -117,17 +132,17 @@ def __init__(self, *args, **kwargs):
 
     def clean_conflict1(self):
         conflict = self.cleaned_data['conflict1']
-        check_conflict(conflict)
+        check_conflict(conflict, self.group)
         return conflict
 
     def clean_conflict2(self):
         conflict = self.cleaned_data['conflict2']
-        check_conflict(conflict)
+        check_conflict(conflict, self.group)
         return conflict
 
     def clean_conflict3(self):
         conflict = self.cleaned_data['conflict3']
-        check_conflict(conflict)
+        check_conflict(conflict, self.group)
         return conflict
 
     def clean(self):

ietf/secr/sreq/tests.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2013-2019, All Rights Reserved
+# Copyright The IETF Trust 2013-2020, All Rights Reserved
 # -*- coding: utf-8 -*-
 
 
@@ -13,7 +13,7 @@
 
 from ietf.utils.test_utils import TestCase
 from ietf.group.factories import GroupFactory, RoleFactory
-from ietf.meeting.models import Session, ResourceAssociation, SchedulingEvent
+from ietf.meeting.models import Session, ResourceAssociation, SchedulingEvent, Constraint
 from ietf.meeting.factories import MeetingFactory, SessionFactory
 from ietf.person.models import Person
 from ietf.utils.mail import outbox, empty_outbox
@@ -154,6 +154,51 @@ def test_submit_request_invalid(self):
         self.assertEqual(len(q('#session-request-form')),1)
         self.assertContains(r, 'You must enter a length for all sessions')
 
+    def test_submit_request_check_constraints(self):
+        m1 = MeetingFactory(type_id='ietf', date=datetime.date.today() - datetime.timedelta(days=100))
+        MeetingFactory(type_id='ietf', date=datetime.date.today())
+        ad = Person.objects.get(user__username='ad')
+        area = RoleFactory(name_id='ad', person=ad, group__type_id='area').group
+        group = GroupFactory(parent=area)
+        still_active_group = GroupFactory(parent=area)
+        Constraint.objects.create(
+            meeting=m1,
+            source=group,
+            target=still_active_group,
+            name_id='conflict',
+        )
+        inactive_group = GroupFactory(parent=area, state_id='conclude')
+        inactive_group.save()
+        Constraint.objects.create(
+            meeting=m1,
+            source=group,
+            target=inactive_group,
+            name_id='conflict',
+        )
+        SessionFactory(group=group, meeting=m1)
+
+        self.client.login(username="secretary", password="secretary+password")
+
+        url = reverse('ietf.secr.sreq.views.new',kwargs={'acronym':group.acronym})
+        r = self.client.get(url + '?previous')
+        self.assertEqual(r.status_code, 200)
+        q = PyQuery(r.content)
+        conflict1 = q('[name="conflict1"]').val()
+        self.assertIn(still_active_group.acronym, conflict1)
+        self.assertNotIn(inactive_group.acronym, conflict1)
+
+        post_data = {'num_session':'1',
+                     'length_session1':'3600',
+                     'attendees':'10',
+                     'conflict1': group.acronym,
+                     'comments':'need projector',
+                     'submit': 'Continue'}
+        r = self.client.post(url,post_data)
+        self.assertEqual(r.status_code, 200)
+        q = PyQuery(r.content)
+        self.assertEqual(len(q('#session-request-form')),1)
+        self.assertContains(r, "Cannot declare a conflict with the same group")
+
     def test_request_notification(self):
         meeting = MeetingFactory(type_id='ietf', date=datetime.date.today())
         ad = Person.objects.get(user__username='ad')

ietf/secr/sreq/views.py

@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2013-2019, All Rights Reserved
+# Copyright The IETF Trust 2013-2020, All Rights Reserved
 # -*- coding: utf-8 -*-
 
 
@@ -21,7 +21,7 @@
 from ietf.meeting.helpers import get_meeting
 from ietf.meeting.utils import add_event_info_to_session_qs
 from ietf.name.models import SessionStatusName, ConstraintName
-from ietf.secr.sreq.forms import SessionForm, ToolStatusForm
+from ietf.secr.sreq.forms import SessionForm, ToolStatusForm, allowed_conflicting_groups
 from ietf.secr.utils.decorators import check_permissions
 from ietf.secr.utils.group import get_my_groups
 from ietf.utils.mail import send_mail
@@ -45,13 +45,13 @@ def check_app_locked(meeting=None):
         meeting = get_meeting()
     return bool(meeting.session_request_lock_message)
 
-def get_initial_session(sessions):
+def get_initial_session(sessions, clean_conflicts=False):
     '''
     This function takes a queryset of sessions ordered by 'id' for consistency.  It returns
     a dictionary to be used as the initial for a legacy session form
     '''
     initial = {}
-    if(len(sessions) == 0):
+    if len(sessions) == 0:
         return initial
 
     meeting = sessions[0].meeting
@@ -70,9 +70,13 @@ def get_initial_session(sessions):
     except IndexError:
         pass
     initial['attendees'] = sessions[0].attendees
-    initial['conflict1'] = ' '.join([ c.target.acronym for c in conflicts.filter(name__slug='conflict') ])
-    initial['conflict2'] = ' '.join([ c.target.acronym for c in conflicts.filter(name__slug='conflic2') ])
-    initial['conflict3'] = ' '.join([ c.target.acronym for c in conflicts.filter(name__slug='conflic3') ])
+
+    def valid_conflict(conflict):
+        return conflict.target != sessions[0].group and allowed_conflicting_groups().filter(pk=conflict.target_id).exists()
+
+    initial['conflict1'] = ' '.join(c.target.acronym for c in conflicts.filter(name__slug='conflict') if not clean_conflicts or valid_conflict(c))
+    initial['conflict2'] = ' '.join(c.target.acronym for c in conflicts.filter(name__slug='conflic2') if not clean_conflicts or valid_conflict(c))
+    initial['conflict3'] = ' '.join(c.target.acronym for c in conflicts.filter(name__slug='conflic3') if not clean_conflicts or valid_conflict(c))
     initial['comments'] = sessions[0].comments
     initial['resources'] = sessions[0].resources.all()
     initial['bethere'] = [x.person for x in sessions[0].constraints().filter(name='bethere').select_related("person")]
@@ -243,10 +247,10 @@ def confirm(request, acronym):
     to confirm for submission.
     '''
     # FIXME: this should be using form.is_valid/form.cleaned_data - invalid input will make it crash
-    form = SessionForm(request.POST, hidden=True)
+    group = get_object_or_404(Group,acronym=acronym)
+    form = SessionForm(group, request.POST, hidden=True)
     form.is_valid()
     meeting = get_meeting()
-    group = get_object_or_404(Group,acronym=acronym)
     login = request.user.person
 
     # check if request already exists for this group
@@ -376,7 +380,7 @@ def edit(request, acronym, num=None):
         if button_text == 'Cancel':
             return redirect('ietf.secr.sreq.views.view', acronym=acronym)
 
-        form = SessionForm(request.POST,initial=initial)
+        form = SessionForm(group, request.POST,initial=initial)
         if form.is_valid():
             if form.has_changed():
                 # might be cleaner to simply delete and rewrite all records (but maintain submitter?)
@@ -485,7 +489,7 @@ def edit(request, acronym, num=None):
     else:
         if not sessions:
             return redirect('ietf.secr.sreq.views.new', acronym=acronym)
-        form = SessionForm(initial=initial)
+        form = SessionForm(group, initial=initial)
 
     return render(request, 'sreq/edit.html', {
         'is_locked': is_locked,
@@ -583,7 +587,7 @@ def new(request, acronym):
         if button_text == 'Cancel':
             return redirect('ietf.secr.sreq.views.main')
 
-        form = SessionForm(request.POST)
+        form = SessionForm(group, request.POST)
         if form.is_valid():
             return confirm(request, acronym)
 
@@ -596,16 +600,16 @@ def new(request, acronym):
             messages.warning(request, 'This group did not meet at %s' % previous_meeting)
             return redirect('ietf.secr.sreq.views.new', acronym=acronym)
 
-        initial = get_initial_session(previous_sessions)
+        initial = get_initial_session(previous_sessions, clean_conflicts=True)
         add_essential_people(group,initial)
         if 'resources' in initial:
             initial['resources'] = [x.pk for x in initial['resources']]
-        form = SessionForm(initial=initial)
+        form = SessionForm(group, initial=initial)
 
     else:
         initial={}
         add_essential_people(group,initial)
-        form = SessionForm(initial=initial)
+        form = SessionForm(group, initial=initial)
 
     return render(request, 'sreq/new.html', {
         'meeting': meeting,