From ecea7964a11d62ca5a15de39231a30817633b92e Mon Sep 17 00:00:00 2001
From: Daniel Roschka <danielroschka@phoenitydawn.de>
Date: Tue, 4 Aug 2015 20:25:07 +0200
Subject: [PATCH] Improves the rendering of forms and refactors some of their
 logic.

Changes are:
* Display invalid input in editable input fields instead of adding it to the
  end of the error message. This should improve user experience in case of
  typos etc. where they now can simply edit there previously entered value.
* Use a single variable for form data and form errors, instead of using two.
  Makes the code a bit cleaner.
---
 .../bcommon/add_cname_record_form.html        | 41 ++++++++-------
 binder/templates/bcommon/add_record_form.html | 50 +++++++++----------
 binder/views.py                               | 22 ++++----
 3 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/binder/templates/bcommon/add_cname_record_form.html b/binder/templates/bcommon/add_cname_record_form.html
index 2edbeef..ed70b51 100644
--- a/binder/templates/bcommon/add_cname_record_form.html
+++ b/binder/templates/bcommon/add_cname_record_form.html
@@ -6,56 +6,55 @@
 <form class="form-horizontal" action="{% url "add_cname_result" %}" method="post">{% csrf_token %}
   <legend>Create CNAME record</legend>
 
-  <div class="form-group{% if form_errors.dns_server %} has-error{% endif %}">
+  <div class="form-group{% if form.dns_server.errors %} has-error{% endif %}">
     <label for="dns_server" class="col-sm-3 control-label">DNS Server:</label>
     <div class="col-sm-5 col-md-4">
       <input id="dns_server" name="dns_server" type="text" class="form-control" value="{{dns_server.hostname}}" readonly="readonly" />
     </div>
-    {% if form_errors.dns_server %}
+    {% if form.dns_server.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.dns_server|stringformat:"s"|striptags }}
-        {% if form_data.dns_server %} Previous Value: {{ form_data.dns_server }}{% endif %}
+        {{ form.dns_server.errors|stringformat:"s"|striptags }}
+        {% if form.dns_server.value %} Previous Value: {{ form.dns_server.value }}{% endif %}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.originating_record %} has-error{% endif %}">
+  <div class="form-group{% if form.originating_record.errors %} has-error{% endif %}">
     <label for="originating_record" class="col-sm-3 control-label">Originating Record: </label>
     <div class="col-sm-5 col-md-4">
       <input type="text" id="originating_record" name="originating_record" class="form-control" value="{{originating_record}}" readonly="readonly"/>
     </div>
-    {% if form_errors.originating_record %}
+    {% if form.originating_record.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.originating_record|stringformat:"s"|striptags }}
-        {% if form_data.originating_record %} Previous Value: {{ form_data.originating_record }}{% endif %}
+        {{ form.originating_record.errors|stringformat:"s"|striptags }}
+        {% if form.originating_record.value %} Previous Value: {{ form.originating_record.value }}{% endif %}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.cname %} has-error{% endif %}">
+  <div class="form-group{% if form.cname.errors %} has-error{% endif %}">
     <label for="cname" class="col-sm-3 control-label">CNAME: </label>
     <div class="col-sm-5 col-md-4">
       <div class="input-group">
-        <input id="cname" name="cname" type="text" class="form-control" />
+        <input id="cname" name="cname" type="text" class="form-control"{% if form.cname.value %} value="{{ form.cname.value }}"{% endif %}/>
         <span class="input-group-addon">.{{zone_name}}</span>
         <input type="hidden" name="zone_name" value="{{zone_name}}"/>
       </div>
     </div>
-    {% if form_errors.cname  %}
+    {% if form.cname.errors  %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        CNAME: {{ form_errors.cname|stringformat:"s"|striptags }}
-        {% if form_data.cnamr %} Previous Value: {{ form_data.cname }}{% endif %}
+        {{ form.cname.errors|stringformat:"s"|striptags }}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.ttl %} has-error{% endif %}">
+  <div class="form-group{% if form.ttl.errors %} has-error{% endif %}">
     <label for="ttl" class="col-sm-3 control-label">TTL: </label>
     <div class="col-sm-5 col-md-4">
       <select id="ttl" name="ttl" class="form-control">
@@ -66,17 +65,17 @@
         {% endfor %}
       </select>
     </div>
-    {% if form_errors.ttl %}
+    {% if form.ttl.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.ttl|stringformat:"s"|striptags }}
-        {% if form_data.ttl %} Previous Value: {{ form_data.ttl }}{% endif %}
+        {{ form.ttl.errors|stringformat:"s"|striptags }}
+        {% if form.ttl.value %} Previous choice: {{ form.ttl.value }}{% endif %}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.key_name %} has-error{% endif %}">
+  <div class="form-group{% if form.key_name.errors %} has-error{% endif %}">
     <label for="key_name" class="col-sm-3 control-label">TSIG Key:</label>
     <div class="col-sm-5 col-md-4">
       <select id="key_name" name="key_name" class="form-control">
@@ -87,11 +86,11 @@
         {% endfor %}
       </select>
     </div>
-    {% if form_errors.key_name %}
+    {% if form.key_name.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.key_name|stringformat:"s"|striptags }}
-        {% if form_data.key_name %} Previous Value: {{ form_data.key_name }}{% endif %}
+        {{ form.key_name.errors|stringformat:"s"|striptags }}
+        {% if form.key_name.value %} Previous choice: {{ form.key_name.value }}{% endif %}
       </div>
     </div>
     {% endif %}
diff --git a/binder/templates/bcommon/add_record_form.html b/binder/templates/bcommon/add_record_form.html
index cb384a2..4607429 100644
--- a/binder/templates/bcommon/add_record_form.html
+++ b/binder/templates/bcommon/add_record_form.html
@@ -6,41 +6,40 @@
 <form class="form-horizontal" action="{% url "add_record_result" %}" method="POST">{% csrf_token %}
   <legend>Create Record</legend>
 
-  <div class="form-group{% if form_errors.dns_server %} has-error{% endif %}">
+  <div class="form-group{% if form.dns_server.errors %} has-error{% endif %}">
     <label for="dns_server" class="col-sm-3 control-label">DNS Server:</label>
     <div class="col-sm-5 col-md-4">
       <input id="dns_server" name="dns_server" type="text" class="form-control" value="{{dns_server.hostname}}" readonly="readonly" />
     </div>
-    {% if form_errors.dns_server %}
+    {% if form.dns_server.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.dns_server|stringformat:"s"|striptags }}
-        {% if form_data.dns_server %}Previous Value: {{ form_data.dns_server }}{% endif %}
+        {{ form.dns_server.errors|stringformat:"s"|striptags }}
+        {% if form.dns_server.value %}Previous Value: {{ form.dns_server.value }}{% endif %}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.record_name %} has-error{% endif %}">
+  <div class="form-group{% if form.record_name.errors %} has-error{% endif %}">
     <label for="record_name" class="col-sm-3 control-label">Record Name:</label>
     <div class="col-sm-5 col-md-4">
       <div class="input-group">
-        <input id="record_name" name="record_name" type="text" class="form-control" />
+        <input id="record_name" name="record_name" type="text" class="form-control"{% if form.record_name.value %} value="{{ form.record_name.value }}"{% endif %} />
         <span class="input-group-addon">.{{zone_name}}</span>
         <input type="hidden" name="zone_name" value="{{zone_name}}" />
       </div>
     </div>
-    {% if form_errors.record_name %}
+    {% if form.record_name.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger form-control-static">
-        {{ form_errors.record_name|stringformat:"s"|striptags }}
-        {% if form_data.record_name %} Previous Value: {{ form_data.record_name }}{% endif %}
+        {{ form.record_name.errors|stringformat:"s"|striptags }}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.record_type %} has-error{% endif %}">
+  <div class="form-group{% if form.record_type.errors %} has-error{% endif %}">
     <label for="record_type" class="col-sm-3 control-label">Record Type:</label>
     <div class="col-sm-5 col-md-4">
       <select id="record_type" name="record_type" class="form-control">
@@ -53,32 +52,31 @@
       {% endif %}
       </select>
     </div>
-    {% if form_errors.record_type %}
+    {% if form.record_type.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.record_type|stringformat:"s"|striptags }}
-        {% if form_data.record_type %} Previous Value: {{ form_data.record_type }}{% endif %}
+        {{ form.record_type.errors|stringformat:"s"|striptags }}
+        {% if form.record_type.value %} Previous Value: {{ form.record_type.value }}{% endif %}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.record_data %} has-error{% endif %}">
+  <div class="form-group{% if form.record_data.errors %} has-error{% endif %}">
     <label for="record_data" class="col-sm-3 control-label">Record Data:</label>
     <div class="col-sm-5 col-md-4">
-      <input id="record_data" name="record_data" type="text" class="form-control" />
+      <input id="record_data" name="record_data" type="text" class="form-control"{% if form.record_data.value %} value="{{ form.record_data.value }}"{% endif %} />
     </div>
-    {% if form_errors.record_data %}
+    {% if form.record_data.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.record_data|stringformat:"s"|striptags }}
-        {% if form_data.record_data %} Previous Value: {{ form_data.record_data }}{% endif %}
+        {{ form.record_data.errors|stringformat:"s"|striptags }}
       </div>
     </div>
     {% endif %}
   </div>
 
-  <div class="form-group{% if form_errors.ttl %} has-error{% endif %}">
+  <div class="form-group{% if form.ttl.errors %} has-error{% endif %}">
     <label for="ttl" class="col-sm-3 control-label">TTL: </label>
     <div class="col-sm-5 col-md-4">
       <select id="ttl" name="ttl" class="form-control">
@@ -89,11 +87,11 @@
         {% endfor %}
       </select>
     </div>
-    {% if form_errors.ttl %}
+    {% if form.ttl.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.ttl|stringformat:"s"|striptags }}
-        {% if form_data.ttl %} Previous Value: {{ form_data.ttl }}{% endif %}
+        {{ form.ttl.errors|stringformat:"s"|striptags }}
+        {% if form.ttl.value %} Previous choice: {{ form.ttl.value }}{% endif %}
       </div>
     </div>
     {% endif %}
@@ -108,7 +106,7 @@
   </div>
   {% endif %}
 
-  <div class="form-group{% if form_errors.key_name %} has-error{% endif %}">
+  <div class="form-group{% if form.key_name.errors %} has-error{% endif %}">
     <label for="key_name" class="col-sm-3 control-label">TSIG Key:</label>
     <div class="col-sm-5 col-md-4">
       <select id="key_name" name="key_name" class="form-control">
@@ -119,11 +117,11 @@
         {% endfor %}
       </select>
     </div>
-    {% if form_errors.key_name %}
+    {% if form.key_name.errors %}
     <div class="col-sm-4 col-md-5">
       <div class="alert alert-danger">
-        {{ form_errors.key_name|stringformat:"s"|striptags }}
-        {% if form_data.key_name %} Previous Value: {{ form_data.key_name }}{% endif %}
+        {{ form.key_name.errors|stringformat:"s"|striptags }}
+        {% if form.key_name.value %} Previous choice: {{ form.key_name.value }}{% endif %}
       </div>
     </div>
     {% endif %}
diff --git a/binder/views.py b/binder/views.py
index f952ca6..5b85639 100644
--- a/binder/views.py
+++ b/binder/views.py
@@ -120,8 +120,7 @@ def view_add_record_result(request):
                    "tsig_keys": models.Key.objects.all(),
                    "ttl_choices": settings.TTL_CHOICES,
                    "record_type_choices": settings.RECORD_TYPE_CHOICES,
-                   "form_errors": form.errors,
-                   "form_data": request.POST})
+                   "form": form})
 
 
 def view_add_cname_record(request, dns_server, zone_name, record_name):
@@ -145,15 +144,15 @@ def view_add_cname_result(request):
     add_cname_response = ""
     form = forms.FormAddCnameRecord(request.POST)
     if form.is_valid():
-        cd = form.cleaned_data
+        form_cleaned = form.cleaned_data
         try:
             add_cname_response = helpers.add_cname_record(
-                cd["dns_server"],
-                cd["zone_name"],
-                cd["cname"],
-                str(cd["originating_record"]),
-                cd["ttl"],
-                cd["key_name"])
+                form_cleaned["dns_server"],
+                form_cleaned["zone_name"],
+                form_cleaned["cname"],
+                str(form_cleaned["originating_record"]),
+                form_cleaned["ttl"],
+                form_cleaned["key_name"])
         except exceptions.RecordException, err:
             errors = err
 
@@ -168,10 +167,9 @@ def view_add_cname_result(request):
                    "zone_name": request.POST["zone_name"],
                    "record_name": request.POST["cname"],
                    "originating_record": request.POST["originating_record"],
-                   "form_data": request.POST,
-                   "form_errors": form.errors,
                    "ttl_choices": settings.TTL_CHOICES,
-                   "tsig_keys": models.Key.objects.all()})
+                   "tsig_keys": models.Key.objects.all(),
+                   "form": form})
 
 
 def view_delete_record(request):