Add a review step when submitting milestones to try to help people

catch errors
 - Legacy-Id: 4543

Author: OleLaursen

ietf/templates/wginfo/edit_milestones.html

@@ -37,6 +37,8 @@
 {% load ietf_filters %}
 <h1>{{ title }}</h1>
 
+<noscript>This page depends on Javascript being enabled to work properly.</noscript>
+
 <p class="help">{% if forms %}Click a milestone to edit it.{% endif %}
 
 {% if needs_review %}
@@ -76,7 +78,8 @@ <h1>{{ title }}</h1>
 
 <div class="actions">
   <a href="{% if milestone_set == "charter" %}{% url doc_view name=group.charter.canonical_name %}{% else %}{% url wg_charter acronym=group.acronym %}{% endif %}">Back</a>
-  <input type="submit" value="Save"/>
+  <input type="submit" data-label-save="Save" data-label-review="Review changes" value="Save"/>
+  <input type="hidden" name="action" value="save">
 </div>
 
 </form>

ietf/templates/wginfo/milestone_form.html

@@ -1,7 +1,6 @@
 {# assumes group, form, needs_review are in the context #}
 <input type="hidden" name="prefix" value="{{ form.prefix|default:"" }}"/>
 {{ form.id }}
-{{ form.expanded_for_editing }}
 
 <table cellspacing="0" cellpadding="0">
   <tr>

ietf/wginfo/milestones.py

@@ -39,8 +39,6 @@ class MilestoneForm(forms.Form):
     accept = forms.ChoiceField(choices=(("accept", "Accept"), ("reject", "Reject and delete"), ("noaction", "No action")),
                                required=False, initial="noaction", widget=forms.RadioSelect)
 
-    expanded_for_editing = forms.BooleanField(required=False, initial=False, widget=forms.HiddenInput)
-
     def __init__(self, *args, **kwargs):
         m = self.milestone = kwargs.pop("instance", None)
 
@@ -119,141 +117,159 @@ def edit_milestones(request, acronym, milestone_set="current"):
 
     milestones_dict = dict((str(m.id), m) for m in milestones)
 
-    def add_event(m, desc):
-        if milestone_set == "charter":
-            DocEvent.objects.create(doc=group.charter, type="changed_charter_milestone",
-                                    by=login, desc=desc)
-        else:
-            MilestoneGroupEvent.objects.create(group=group, type="changed_milestone",
-                                               by=login, desc=desc, milestone=m)
+    def set_attributes_from_form(f, m):
+        c = f.cleaned_data
+        m.group = group
+        if milestone_set == "current":
+            if needs_review:
+                m.state = GroupMilestoneStateName.objects.get(slug="review")
+            else:
+                m.state = GroupMilestoneStateName.objects.get(slug="active")
+        elif milestone_set == "charter":
+            m.state = GroupMilestoneStateName.objects.get(slug="charter")
+        m.desc = c["desc"]
+        m.due = c["due"]
+        m.resolved = c["resolved"]
 
-    finished_milestone_text = "Done"
+    def save_milestone_form(f):
+        c = f.cleaned_data
 
-    form_errors = False
+        if f.milestone:
+            m = f.milestone
 
-    if request.method == 'POST':
-        for prefix in request.POST.getlist("prefix"):
-            if not prefix: # empty form
-                continue
+            named_milestone = 'milestone "%s"' % m.desc
+            if milestone_set == "charter":
+                named_milestone = "charter " + named_milestone
 
-            # new milestones have non-existing ids so instance end up as None
-            instance = milestones_dict.get(request.POST.get(prefix + "-id", ""), None)
-            f = MilestoneForm(request.POST, prefix=prefix, instance=instance,
-                              needs_review=needs_review)
-            forms.append(f)
+            if c["delete"]:
+                save_milestone_in_history(m)
 
-            form_errors = form_errors or not f.is_valid()
+                m.state_id = "deleted"
+                m.save()
 
-        if not form_errors:
-            for f in forms:
-                c = f.cleaned_data
+                return 'Deleted %s' % named_milestone
+
+            # compute changes
+            history = None
 
-                if f.milestone:
-                    m = f.milestone
+            changes = ['Changed %s' % named_milestone]
 
-                    named_milestone = 'milestone "%s"' % m.desc
-                    if milestone_set == "charter":
-                        named_milestone = "charter " + named_milestone
+            if m.state_id == "review" and not needs_review and c["accept"] != "noaction":
+                if not history:
+                    history = save_milestone_in_history(m)
 
-                    if c["delete"]:
-                        save_milestone_in_history(m)
+                if c["accept"] == "accept":
+                    m.state_id = "active"
+                    changes.append("changed state from review to active, accepting new milestone")
+                elif c["accept"] == "reject":
+                    m.state_id = "deleted"
+                    changes.append("changed state from review to deleted, rejecting new milestone")
 
-                        m.state_id = "deleted"
-                        m.save()
 
-                        add_event(m, 'Deleted %s' % named_milestone)
+            if c["desc"] != m.desc and not needs_review:
+                if not history:
+                    history = save_milestone_in_history(m)
+                m.desc = c["desc"]
+                changes.append('changed description to "%s"' % m.desc)
 
-                        continue
+            if c["due"] != m.due:
+                if not history:
+                    history = save_milestone_in_history(m)
+                m.due = c["due"]
+                changes.append('changed due date to %s' % m.due.strftime("%Y-%m-%d"))
 
-                    # compute changes
-                    history = None
+            resolved = c["resolved"]
+            if resolved != m.resolved:
+                if resolved and not m.resolved:
+                    changes.append('resolved as "%s"' % resolved)
+                elif not resolved and m.resolved:
+                    changes.append("reverted to not being resolved")
+                elif resolved and m.resolved:
+                    changes.append('changed resolution to "%s"' % resolved)
 
-                    changes = ['Changed %s' % named_milestone]
+                if not history:
+                    history = save_milestone_in_history(m)
 
-                    if m.state_id == "review" and not needs_review and c["accept"] != "noaction":
-                        if not history:
-                            history = save_milestone_in_history(m)
+                m.resolved = resolved
 
-                        if c["accept"] == "accept":
-                            m.state_id = "active"
-                            changes.append("changed state from review to active, accepting new milestone")
-                        elif c["accept"] == "reject":
-                            m.state_id = "deleted"
-                            changes.append("changed state from review to deleted, rejecting new milestone")
+            new_docs = set(c["docs"])
+            old_docs = set(m.docs.all())
+            if new_docs != old_docs:
+                added = new_docs - old_docs
+                if added:
+                    changes.append('added %s to milestone' % ", ".join(d.name for d in added))
 
+                removed = old_docs - new_docs
+                if removed:
+                    changes.append('removed %s from milestone' % ", ".join(d.name for d in removed))
 
-                    if c["desc"] != m.desc and not needs_review:
-                        if not history:
-                            history = save_milestone_in_history(m)
-                        m.desc = c["desc"]
-                        changes.append('changed description to "%s"' % m.desc)
+                if not history:
+                    history = save_milestone_in_history(m)
 
-                    if c["due"] != m.due:
-                        if not history:
-                            history = save_milestone_in_history(m)
-                        m.due = c["due"]
-                        changes.append('changed due date to %s' % m.due.strftime("%Y-%m-%d"))
+                m.docs = new_docs
 
-                    resolved = c["resolved"]
-                    if resolved != m.resolved:
-                        if resolved and not m.resolved:
-                            changes.append('resolved as "%s"' % resolved)
-                        elif not resolved and m.resolved:
-                            changes.append("reverted to not being resolved")
-                        elif resolved and m.resolved:
-                            changes.append('changed resolution to "%s"' % resolved)
+            if len(changes) > 1:
+                m.save()
 
-                        if not history:
-                            history = save_milestone_in_history(m)
+                return ", ".join(changes)
 
-                        m.resolved = resolved
+        else: # new milestone
+            m = f.milestone = GroupMilestone()
+            set_attributes_from_form(f, m)
+            m.save()
 
-                    new_docs = set(c["docs"])
-                    old_docs = set(m.docs.all())
-                    if new_docs != old_docs:
-                        added = new_docs - old_docs
-                        if added:
-                            changes.append('added %s to milestone' % ", ".join(d.name for d in added))
+            m.docs = c["docs"]
 
-                        removed = old_docs - new_docs
-                        if removed:
-                            changes.append('removed %s from milestone' % ", ".join(d.name for d in removed))
+            named_milestone = 'milestone "%s"' % m.desc
+            if milestone_set == "charter":
+                named_milestone = "charter " + named_milestone
 
-                        if not history:
-                            history = save_milestone_in_history(m)
+            if m.state_id in ("active", "charter"):
+                return 'Added %s, due %s' % (named_milestone, m.due.strftime("%Y-%m-%d"))
+            elif m.state_id == "review":
+                return 'Added %s for review, due %s' % (named_milestone, m.due.strftime("%Y-%m-%d"))
 
-                        m.docs = new_docs
+    finished_milestone_text = "Done"
+
+    form_errors = False
 
-                    if len(changes) > 1:
-                        add_event(m, ", ".join(changes))
+    if request.method == 'POST':
+        # parse out individual milestone forms
+        for prefix in request.POST.getlist("prefix"):
+            if not prefix: # empty form
+                continue
 
-                        m.save()
+            # new milestones have non-existing ids so instance end up as None
+            instance = milestones_dict.get(request.POST.get(prefix + "-id", ""), None)
+            f = MilestoneForm(request.POST, prefix=prefix, instance=instance,
+                              needs_review=needs_review)
+            forms.append(f)
 
-                else: # new milestone
-                    m = GroupMilestone()
-                    m.group = group
-                    if milestone_set == "current":
-                        if needs_review:
-                            m.state = GroupMilestoneStateName.objects.get(slug="review")
-                        else:
-                            m.state = GroupMilestoneStateName.objects.get(slug="active")
-                    elif milestone_set == "charter":
-                        m.state = GroupMilestoneStateName.objects.get(slug="charter")
-                    m.desc = c["desc"]
-                    m.due = c["due"]
-                    m.resolved = c["resolved"]
-                    m.save()
+            form_errors = form_errors or not f.is_valid()
 
-                    m.docs = c["docs"]
+        action = request.POST.get("action", "review")
+        if action == "review":
+            for f in forms:
+                if not f.is_valid():
+                    continue
+
+                # let's fill in the form milestone so we can output it in the template
+                if not f.milestone:
+                    f.milestone = GroupMilestone()
+                set_attributes_from_form(f, f.milestone)
+        elif action == "save" and not form_errors:
+            for f in forms:
+                change = save_milestone_form(f)
 
-                    named_milestone = 'milestone "%s"' % m.desc
-                    if milestone_set == "charter":
-                        named_milestone = "charter " + named_milestone
+                if not change:
+                    continue
 
-                    if m.state_id in ("active", "charter"):
-                        add_event(m, 'Added %s, due %s' % (named_milestone, m.due.strftime("%Y-%m-%d")))
-                    elif m.state_id == "review":
-                        add_event(m, 'Added %s for review, due %s' % (named_milestone, m.due.strftime("%Y-%m-%d")))
+                if milestone_set == "charter":
+                    DocEvent.objects.create(doc=group.charter, type="changed_charter_milestone",
+                                            by=login, desc=change)
+                else:
+                    MilestoneGroupEvent.objects.create(group=group, type="changed_milestone",
+                                                       by=login, desc=change, milestone=f.milestone)
 
             if milestone_set == "charter":
                 return redirect('doc_view', name=group.charter.canonical_name())
@@ -267,6 +283,8 @@ def add_event(m, desc):
 
     empty_form = MilestoneForm(needs_review=needs_review)
 
+    forms.sort(key=lambda f: f.milestone.due if f.milestone else datetime.date.max)
+
     return render_to_response('wginfo/edit_milestones.html',
                               dict(group=group,
                                    title=title,

ietf/wginfo/tests.py

@@ -316,6 +316,7 @@ def test_add_milestone(self):
                                     'm-1-due': due.strftime("%Y-%m-%d"),
                                     'm-1-resolved': "",
                                     'm-1-docs': ",".join(docs),
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 200)
         q = PyQuery(r.content)
@@ -329,6 +330,7 @@ def test_add_milestone(self):
                                     'm-1-due': due.strftime("%Y-%m-%d"),
                                     'm-1-resolved': "",
                                     'm-1-docs': ",".join(docs),
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 302)
         self.assertEquals(GroupMilestone.objects.count(), milestones_before + 1)
@@ -362,6 +364,7 @@ def test_add_milestone_as_chair(self):
                                     'm-1-due': due.strftime("%Y-%m-%d"),
                                     'm-1-resolved': "",
                                     'm-1-docs': "",
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 302)
         self.assertEquals(GroupMilestone.objects.count(), milestones_before + 1)
@@ -394,6 +397,7 @@ def test_accept_milestone(self):
                                     'm1-resolved': m1.resolved,
                                     'm1-docs': ",".join(m1.docs.values_list("name", flat=True)),
                                     'm1-accept': "accept",
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 302)
 
@@ -419,6 +423,7 @@ def test_delete_milestone(self):
                                     'm1-resolved': "",
                                     'm1-docs': ",".join(m1.docs.values_list("name", flat=True)),
                                     'm1-delete': "checked",
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 302)
         self.assertEquals(GroupMilestone.objects.count(), milestones_before)
@@ -447,6 +452,7 @@ def test_edit_milestone(self):
                                     'm1-due': due.strftime("%Y-%m-%d"),
                                     'm1-resolved': "",
                                     'm1-docs': ",".join(docs),
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 200)
         q = PyQuery(r.content)
@@ -463,6 +469,7 @@ def test_edit_milestone(self):
                                     'm1-resolved': "Done",
                                     'm1-resolved_checkbox': "checked",
                                     'm1-docs': ",".join(docs),
+                                    'action': "save",
                                     })
         self.assertEquals(r.status_code, 302)
         self.assertEquals(GroupMilestone.objects.count(), milestones_before)