From d5c96032b0568cead5abb459f5b95c2b8cb78dcd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20P=C3=A9ters?= <fpeters@entrouvert.com>
Date: Sun, 6 Oct 2019 18:37:41 +0200
Subject: [PATCH] admin: revamp overriding forms (#36711)

* always display summary of changes if there are data
* -> skip summary of changes if there are no data (#13599)
* do not use scrolling in summary (#32177)
* ignore no-data fields when comparing types
* warn about data loss instead of data corruption/bugs
* remove data from incompatible columns (in SQL) (#15379)
---
 tests/test_admin_pages.py           |  79 +++++++++++----
 wcs/admin/forms.py                  | 145 +++++++++++++++-------------
 wcs/qommon/static/css/dc2/admin.css |  12 +--
 3 files changed, 142 insertions(+), 94 deletions(-)

diff --git a/tests/test_admin_pages.py b/tests/test_admin_pages.py
index 6cbcfbed7..904eb6955 100644
--- a/tests/test_admin_pages.py
+++ b/tests/test_admin_pages.py
@@ -1937,56 +1937,91 @@ def test_form_overwrite(pub):
     resp = resp.click(href='overwrite')
     resp.forms[0]['file'] = Upload('formdef.wcs', formdef_xml)
     resp = resp.forms[0].submit()
+    assert 'Overwrite - Summary of changes' in resp.body
+    resp = resp.forms[0].submit()
     assert FormDef.get(formdef_id).fields[0].label == '1st modified field'
     resp = resp.follow()
     assert 'The form has been successfully overwritten.' in resp.body
 
     # check with added/removed field
-    formdef = FormDef()
-    formdef.name = 'form test overwrite'
-    formdef.fields = [
+    new_formdef = FormDef()
+    new_formdef.name = 'form test overwrite'
+    new_formdef.fields = [
             fields.StringField(id='2', label='2nd field', type='string'),
             fields.StringField(id='3', label='3rd field', type='string')]
-    formdef_xml = ET.tostring(formdef.export_to_xml(include_id=True))
+    new_formdef_xml = ET.tostring(new_formdef.export_to_xml(include_id=True))
 
+    # and no data within
+    formdef.data_class().wipe()
     app = login(get_app(pub))
     resp = app.get('/backoffice/forms/%s/' % formdef_id)
     resp = resp.click(href='overwrite')
-    resp.forms[0]['file'] = Upload('formdef.wcs', formdef_xml)
+    resp.forms[0]['file'] = Upload('formdef.wcs', new_formdef_xml)
+    resp = resp.forms[0].submit()
+    assert FormDef.get(formdef_id).fields[0].id == '2'
+    assert FormDef.get(formdef_id).fields[0].label == '2nd field'
+    assert FormDef.get(formdef_id).fields[1].id == '3'
+    assert FormDef.get(formdef_id).fields[1].label == '3rd field'
+
+    # and data within
+    formdef.store()
+    formdata.data = {'1': 'foo', '2': 'bar'}
+    formdata.just_created()
+    formdata.store()
+
+    resp = app.get('/backoffice/forms/%s/' % formdef_id)
+    resp = resp.click(href='overwrite')
+    resp.forms[0]['file'] = Upload('formdef.wcs', new_formdef_xml)
     resp = resp.forms[0].submit()
-    assert 'The form removes and changes fields' in resp.body
-    assert not 'The form has incompatible fields, it may cause data corruption and bugs' in resp.body
+    assert 'The form removes or changes fields' in resp.body
+    assert resp.forms[0]['force'].checked is False
+    resp = resp.forms[0].submit()  # without checkbox (back to same form)
+    resp.forms[0]['force'].checked = True
     resp = resp.forms[0].submit()
+
     assert FormDef.get(formdef_id).fields[0].id == '2'
     assert FormDef.get(formdef_id).fields[0].label == '2nd field'
     assert FormDef.get(formdef_id).fields[1].id == '3'
     assert FormDef.get(formdef_id).fields[1].label == '3rd field'
 
     # check with a field of different type
-    formdef = FormDef()
-    formdef.name = 'form test overwrite'
-    formdef.fields = [
+    formdef = FormDef.get(formdef_id)
+    formdata = formdef.data_class()()
+    formdata.data = {'1': 'foo', '2': 'bar', '3': 'baz'}
+    formdata.just_created()
+    formdata.store()
+
+    new_formdef = FormDef()
+    new_formdef.name = 'form test overwrite'
+    new_formdef.fields = [
             fields.StringField(id='2', label='2nd field', type='string'),
-            fields.TextField(id='3', label='3rd field, text', type='text')]
-    formdef_xml = ET.tostring(formdef.export_to_xml(include_id=True))
+            fields.DateField(id='3', label='3rd field, date', type='date')]  # (string -> date)
+    new_formdef_xml = ET.tostring(new_formdef.export_to_xml(include_id=True))
 
     app = login(get_app(pub))
     resp = app.get('/backoffice/forms/%s/' % formdef_id)
     resp = resp.click(href='overwrite', index=0)
-    resp.forms[0]['file'] = Upload('formdef.wcs', formdef_xml)
+    resp.forms[0]['file'] = Upload('formdef.wcs', new_formdef_xml)
     resp = resp.forms[0].submit()
-    assert 'The form removes and changes fields' in resp.body
-    assert 'The form has incompatible fields, it may cause data corruption and bugs' in resp.body
-    resp.forms[0]['force'] = True
+    assert 'The form removes or changes fields' in resp.body
+    resp.forms[0]['force'].checked = True
     resp = resp.forms[0].submit()
     assert FormDef.get(formdef_id).fields[1].id == '3'
-    assert FormDef.get(formdef_id).fields[1].label == '3rd field, text'
-    assert FormDef.get(formdef_id).fields[1].type == 'text'
+    assert FormDef.get(formdef_id).fields[1].label == '3rd field, date'
+    assert FormDef.get(formdef_id).fields[1].type == 'date'
 
     # check we kept stable references
     assert FormDef.get(formdef_id).url_name == 'form-test'
     assert FormDef.get(formdef_id).table_name == 'xxx'
 
+    # check existing data
+    data = FormDef.get(formdef_id).data_class().get(formdata.id).data
+    assert data.get('2') == 'bar'
+    if pub.is_using_postgresql():
+        # in SQL, check data with different type has been removed
+        assert data.get('3') is None
+
+
 def test_form_export_import_export_overwrite(pub):
     create_superuser(pub)
     create_role()
@@ -2002,6 +2037,12 @@ def test_form_export_import_export_overwrite(pub):
     ]
     formdef.store()
 
+    # add data
+    formdata = formdef.data_class()()
+    formdata.data = {'1': 'foo'}
+    formdata.just_created()
+    formdata.store()
+
     formdef_xml = ET.tostring(formdef.export_to_xml(include_id=True))
 
     assert FormDef.count() == 1
@@ -2031,6 +2072,8 @@ def test_form_export_import_export_overwrite(pub):
     resp = resp.click(href='overwrite')
     resp.forms[0]['file'] = Upload('formdef.wcs', formdef2_xml)
     resp = resp.forms[0].submit()
+    assert 'Overwrite - Summary of changes' in resp.body
+    resp = resp.forms[0].submit()
     formdef_overwrited = FormDef.get(formdef.id)
     for i, field in enumerate(formdef2.fields):
         field_ow = formdef_overwrited.fields[i]
diff --git a/wcs/admin/forms.py b/wcs/admin/forms.py
index 61f65e671..f681d66ff 100644
--- a/wcs/admin/forms.py
+++ b/wcs/admin/forms.py
@@ -1008,36 +1008,20 @@ class FormDefPage(Directory):
                 form.set_error('file', msg)
             raise ValueError()
 
-        if form.get_widget('new_formdef').parse():
-            # it's been through the summary page.
-            if form.get_widget('force').parse():
-                # doing it!
-                return self.overwrite_by_formdef(new_formdef)
+        # it's been through the summary page, or there is no data yet
+        if not self.formdef.data_class().count() or form.get_widget('force').parse():
+            # doing it!
+            return self.overwrite_by_formdef(new_formdef)
 
-        # 1. map field id and types
-        current_fields = {}
-        new_fields = {}
-        for field in self.formdef.fields:
-            current_fields[field.id] = field.type
-        for field in new_formdef.fields:
-            new_fields[field.id] = field.type
-
-        # 2. compare with current fields
-        removed_fields = []
-        different_type_fields = []
-        for field_id, field_type in current_fields.items():
-            if field_id not in new_fields:
-                removed_fields.append(field_id)
-            elif new_fields.get(field_id) != field_type:
-                different_type_fields.append(field_id)
-
-        if removed_fields or different_type_fields:
-            return self.overwrite_warning_summary(new_formdef,
-                    removed_fields, different_type_fields)
-
-        return self.overwrite_by_formdef(new_formdef)
+        return self.overwrite_warning_summary(new_formdef)
 
     def overwrite_by_formdef(self, new_formdef):
+        incompatible_field_ids = self.get_incompatible_field_ids(new_formdef)
+        if incompatible_field_ids:
+            # if there are incompatible field ids, remove them first
+            self.formdef.fields = [x for x in self.formdef.fields if x.id not in incompatible_field_ids]
+            self.formdef.store()
+
         # keep current formdef id, url_name, internal identifier and sql table name
         new_formdef.id = self.formdef.id
         new_formdef.internal_identifier = self.formdef.internal_identifier
@@ -1056,17 +1040,25 @@ class FormDefPage(Directory):
         get_session().message = ('info', _(self.overwrite_success_message))
         return redirect('.')
 
-    def overwrite_warning_summary(self, new_formdef, removed_fields, different_type_fields):
+    def get_incompatible_field_ids(self, new_formdef):
+        incompatible_field_ids = []
+        current_fields = {}
+        for field in self.formdef.fields:
+            current_fields[field.id] = field
+
+        for field in new_formdef.fields:
+            current_field = current_fields.get(field.id)
+            if current_field and current_field.type != field.type:
+                incompatible_field_ids.append(field.id)
+
+        return incompatible_field_ids
+
+    def overwrite_warning_summary(self, new_formdef):
         self.html_top(title = _('Overwrite'))
         get_response().breadcrumb.append( ('overwrite', _('Overwrite')) )
         r = TemplateIO(html=True)
 
-        r += htmltext('<h2>%s</h2>') % _('Overwrite')
-        r += htmltext('<h3>%s</h3>') % _('Summary of changes')
-
-        r += htmltext('<p>%s</p>') % _(
-                'The form removes and changes fields, you should review the '
-                'changes carefully.')
+        r += htmltext('<h2>%s - %s</h2>') % (_('Overwrite'), _('Summary of changes'))
 
         current_fields_list = [str(x.id) for x in self.formdef.fields]
         new_fields_list = [str(x.id) for x in new_formdef.fields]
@@ -1078,52 +1070,68 @@ class FormDefPage(Directory):
         for field in new_formdef.fields:
             new_fields[field.id] = field
 
-        r += htmltext('<div id="form-diff">')
-        r += htmltext('<div>')
-        r += htmltext('<table id="table-diff">')
+        table = TemplateIO(html=True)
+        table += htmltext('<table id="table-diff">')
+
         def ellipsize_html(field):
             return misc.ellipsize(field.unhtmled_label, 60)
 
+        nodata_types = ('page', 'title', 'subtitle', 'comment')
+        display_warning = False
+
         for diffinfo in difflib.ndiff(current_fields_list, new_fields_list):
             if diffinfo[0] == '?':
                 # detail line, ignored
                 continue
             field_id = diffinfo[2:].split()[0]
+            current_field = current_fields.get(field_id)
+            new_field = new_fields.get(field_id)
+
+            current_label = ellipsize_html(current_field) if current_field else ''
+            new_label = ellipsize_html(new_field) if new_field else ''
+
             if diffinfo[0] == ' ':
                 # unchanged line
-                label1 = ellipsize_html(current_fields.get(field_id))
-                label2 = ellipsize_html(new_fields.get(field_id))
-                if current_fields.get(field_id) and new_fields.get(field_id) and \
-                        current_fields.get(field_id).type != new_fields.get(field_id).type:
-                    r += htmltext('<tr class="type-change"><td class="indicator">!</td>')
-                if current_fields.get(field_id) and new_fields.get(field_id) and \
-                        ET.tostring(current_fields.get(field_id).export_to_xml('utf-8')) != \
-                        ET.tostring(new_fields.get(field_id).export_to_xml('utf-8')):
-                    r += htmltext('<tr class="modified-field"><td class="indicator">~</td>')
+                if current_field and new_field and current_field.type != new_field.type:
+                    # different datatypes
+                    if current_field.type in nodata_types:
+                        # but current field doesn't hold data, not a problem
+                        table += htmltext('<tr class="added-field"><td class="indicator">+</td>')
+                        current_label = ''
+                    elif new_field.type in nodata_types:
+                        # new field won't hold data, but old data will be removed
+                        table += htmltext('<tr class="removed-field"><td class="indicator">-</td>')
+                        new_label = ''
+                        display_warning = True
+                    else:
+                        # and real incompatibility, data will need to be wiped out.
+                        table += htmltext('<tr class="type-change"><td class="indicator">!</td>')
+                        display_warning = True
+                elif current_field and new_field and \
+                        ET.tostring(current_field.export_to_xml('utf-8')) != \
+                        ET.tostring(new_field.export_to_xml('utf-8')):
+                    # same type, but changes within field
+                    table += htmltext('<tr class="modified-field"><td class="indicator">~</td>')
                 else:
-                    r += htmltext('<tr><td class="indicator"></td>')
-                r += htmltext('<td>%s</td> <td>%s</td></tr>') % (label1, label2)
+                    table += htmltext('<tr><td class="indicator"></td>')
             elif diffinfo[0] == '-':
                 # removed field
-                label1 = ellipsize_html(current_fields.get(field_id))
-                if current_fields.get(field_id) and new_fields.get(field_id) and \
-                        current_fields.get(field_id).type != new_fields.get(field_id).type:
-                    r += htmltext('<tr class="type-change"><td class="indicator">!</td>')
-                else:
-                    r += htmltext('<tr class="removed-field"><td class="indicator">-</td>')
-                r += htmltext('<td>%s</td> <td></td></tr>') % label1
+                table += htmltext('<tr class="removed-field"><td class="indicator">-</td>')
+                display_warning = True
             elif diffinfo[0] == '+':
                 # added field
-                label2 = ellipsize_html(new_fields.get(field_id))
-                if current_fields.get(field_id) and new_fields.get(field_id) and \
-                        current_fields.get(field_id).type != new_fields.get(field_id).type:
-                    r += htmltext('<tr class="type-change"><td class="indicator">!</td>')
-                else:
-                    r += htmltext('<tr class="added-field"><td class="indicator">+</td>')
-                r += htmltext('<td></td> <td>%s</td></tr>') % label2
+                table += htmltext('<tr class="added-field"><td class="indicator">+</td>')
+            table += htmltext('<td>%s</td> <td>%s</td></tr>') % (current_label, new_label)
+        table += htmltext('</table>')
 
-        r += htmltext('</table>')
-        r += htmltext('</div>')
+        if display_warning:
+            r += htmltext('<div class="errornotice"><p>%s</p></div>') % _(
+                    'The form removes or changes fields, you should review the '
+                    'changes carefully as some data will be lost.')
+
+        r += htmltext('<div class="section">')
+        r += htmltext('<div id="form-diff">')
+        r += table.getvalue()
 
         r += htmltext('<div id="legend">')
         r += htmltext('<table>')
@@ -1142,21 +1150,20 @@ class FormDefPage(Directory):
                 _('Incompatible field'))
         r += htmltext('</table>')
         r += htmltext('</div>') # .legend
-        r += htmltext('</div>')
 
         get_request().method = 'GET'
         get_request().form = {}
         form = Form(enctype='multipart/form-data', use_tokens=False)
-        if different_type_fields:
-            form.widgets.append(HtmlWidget('<div class="errornotice"><p>%s</p></div>' % _(
-                'The form has incompatible fields, it may cause data corruption and bugs.')))
-            form.add(CheckboxWidget, 'force', title=_('Overwrite nevertheless'))
+        if display_warning:
+            form.add(CheckboxWidget, 'force', title=_('Overwrite despite data loss'))
         else:
             form.add_hidden('force', 'ok')
         form.add_hidden('new_formdef', ET.tostring(new_formdef.export_to_xml(include_id=True)))
         form.add_submit('submit', _('Submit'))
         form.add_submit('cancel', _('Cancel'))
         r += form.render()
+        r += htmltext('</div>')  # #form-diff
+        r += htmltext('</div>')  # .section
 
         return r.getvalue()
 
diff --git a/wcs/qommon/static/css/dc2/admin.css b/wcs/qommon/static/css/dc2/admin.css
index 5833c5e8b..133fd56c4 100644
--- a/wcs/qommon/static/css/dc2/admin.css
+++ b/wcs/qommon/static/css/dc2/admin.css
@@ -1164,19 +1164,16 @@ ul.apps li.zone-no-icon a {
 	padding-top: 24px;
 }
 
-div#form-diff div {
-	max-height: 25em;
-	overflow-y: scroll;
-}
-
 div#form-diff td {
 	width: 50%;
 	padding: 0.5ex;
 }
 
 div#form-diff td.indicator {
-	width: 5px;
+	width: 20px;
+	min-width: 20px;
 	padding: 0 3px;
+	font-weight: bold;
 }
 
 div#form-diff tr td.indicator {
@@ -1214,7 +1211,8 @@ div#form-diff div#legend table {
 }
 
 div#form-diff div#legend table td.indicator {
-	width: 5px;
+	width: 20px;
+	min-width: 20px;
 }
 
 div#form-diff div#legend td {
-- 
2.23.0

