From fdedacf0c2eb512c31703ec18920ac02402b9f1b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20P=C3=A9ters?= <fpeters@entrouvert.com>
Date: Mon, 8 Apr 2019 19:19:35 +0200
Subject: [PATCH] misc: give unique identifiers to backoffice fields (#17375)

---
 tests/test_admin_pages.py | 11 ++++---
 wcs/sql.py                | 63 +++++++++++++++++++++------------------
 wcs/workflows.py          |  5 +---
 3 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/tests/test_admin_pages.py b/tests/test_admin_pages.py
index cf2705cdd..a74a8c448 100644
--- a/tests/test_admin_pages.py
+++ b/tests/test_admin_pages.py
@@ -2848,11 +2848,11 @@ def test_workflows_backoffice_fields(pub):
     resp = resp.follow()
     workflow = Workflow.get(workflow.id)
     assert len(workflow.backoffice_fields_formdef.fields) == 2
-    assert workflow.backoffice_fields_formdef.fields[0].id == 'bo1'
-    assert workflow.backoffice_fields_formdef.fields[1].id == 'bo2'
+    first_field_id = workflow.backoffice_fields_formdef.fields[0].id
+    assert workflow.backoffice_fields_formdef.fields[1].id != first_field_id
 
     # check there's no prefill field
-    resp = app.get('/backoffice/workflows/1/backoffice-fields/fields/bo2/')
+    resp = app.get('/backoffice/workflows/1/backoffice-fields/fields/%s/' % workflow.backoffice_fields_formdef.fields[1].id)
     assert not 'prefill$type' in resp.form.fields.keys()
 
     # add a title field
@@ -2862,7 +2862,6 @@ def test_workflows_backoffice_fields(pub):
     resp = resp.form.submit()
     workflow = Workflow.get(workflow.id)
     assert len(workflow.backoffice_fields_formdef.fields) == 3
-    assert workflow.backoffice_fields_formdef.fields[2].id == 'bo3'
 
     # check backoffice fields are available in set backoffice fields action
     resp = app.get('/backoffice/workflows/1/')
@@ -2874,11 +2873,11 @@ def test_workflows_backoffice_fields(pub):
     assert 'foobar2' in options
     assert 'foobar3' not in options
 
-    resp.form['fields$element0$field_id'] = 'bo1'
+    resp.form['fields$element0$field_id'] = first_field_id
     resp.form['fields$element0$value$value_text'] = 'Hello'
     resp = resp.form.submit('submit')
     workflow = Workflow.get(workflow.id)
-    assert workflow.possible_status[0].items[0].fields == [{'field_id': 'bo1', 'value': 'Hello'}]
+    assert workflow.possible_status[0].items[0].fields == [{'field_id': first_field_id, 'value': 'Hello'}]
 
 def test_workflows_functions(pub):
     create_superuser(pub)
diff --git a/wcs/sql.py b/wcs/sql.py
index 1af91ffbc..19a48563f 100644
--- a/wcs/sql.py
+++ b/wcs/sql.py
@@ -207,6 +207,9 @@ def get_name_as_sql_identifier(name):
     return name
 
 
+def get_field_id(field):
+    return 'f' + str(field.id).replace('-', '_')
+
 def parse_clause(clause):
     # returns a three-elements tuple with:
     #  - a list of SQL 'WHERE' clauses
@@ -450,22 +453,24 @@ def do_formdef_tables(formdef, conn=None, cur=None, rebuild_views=False, rebuild
         sql_type = SQL_TYPE_MAPPING.get(field.key, 'varchar')
         if sql_type is None:
             continue
-        needed_fields.add('f%s' % field.id)
-        if 'f%s' % field.id not in existing_fields:
+        needed_fields.add(get_field_id(field))
+        if get_field_id(field) not in existing_fields:
             cur.execute('''ALTER TABLE %s ADD COLUMN %s %s''' % (
                                     table_name,
-                                    'f%s' % field.id,
+                                    get_field_id(field),
                                     sql_type))
         if field.store_display_value:
-            needed_fields.add('f%s_display' % field.id)
-            if 'f%s_display' % field.id not in existing_fields:
+            needed_fields.add('%s_display' % get_field_id(field))
+            if '%s_display' % get_field_id(field) not in existing_fields:
                 cur.execute('''ALTER TABLE %s ADD COLUMN %s varchar''' % (
-                                        table_name, 'f%s_display' % field.id))
+                                        table_name,
+                                        '%s_display' % get_field_id(field)))
         if field.store_structured_value:
             needed_fields.add('f%s_structured' % field.id)
-            if 'f%s_structured' % field.id not in existing_fields:
+            if '%s_structured' % get_field_id(field) not in existing_fields:
                 cur.execute('''ALTER TABLE %s ADD COLUMN %s bytea''' % (
-                                        table_name, 'f%s_structured' % field.id))
+                                        table_name,
+                                        '%s_structured' % get_field_id(field)))
 
     for field in (formdef.geolocations or {}).keys():
         column_name = 'geoloc_%s' % field
@@ -563,22 +568,22 @@ def do_user_table():
         sql_type = SQL_TYPE_MAPPING.get(field.key, 'varchar')
         if sql_type is None:
             continue
-        needed_fields.add('f%s' % field.id)
-        if 'f%s' % field.id not in existing_fields:
+        needed_fields.add(get_field_id(field))
+        if get_field_id(field) not in existing_fields:
             cur.execute('''ALTER TABLE %s ADD COLUMN %s %s''' % (
                                     table_name,
-                                    'f%s' % field.id,
+                                    get_field_id(field),
                                     sql_type))
         if field.store_display_value:
-            needed_fields.add('f%s_display' % field.id)
-            if 'f%s_display' % field.id not in existing_fields:
+            needed_fields.add('%s_display' % get_field_id(field))
+            if '%s_display' % get_field_id(field) not in existing_fields:
                 cur.execute('''ALTER TABLE %s ADD COLUMN %s varchar''' % (
-                                        table_name, 'f%s_display' % field.id))
+                                        table_name, '%s_display' % get_field_id(field)))
         if field.store_structured_value:
-            needed_fields.add('f%s_structured' % field.id)
-            if 'f%s_structured' % field.id not in existing_fields:
+            needed_fields.add('%s_structured' % get_field_id(field))
+            if '%s_structured' % get_field_id(field) not in existing_fields:
                 cur.execute('''ALTER TABLE %s ADD COLUMN %s bytea''' % (
-                                        table_name, 'f%s_structured' % field.id))
+                                        table_name, '%s_structured' % get_field_id(field)))
 
     # migrations
     if not 'fts' in existing_fields:
@@ -742,7 +747,7 @@ def do_views(formdef, conn, cur, rebuild_global_views=True):
 
     column_names = {}
     for field in formdef.get_all_fields():
-        field_key = 'f%s' % field.id
+        field_key = get_field_id(field)
         if field.type in ('page', 'title', 'subtitle', 'comment'):
             continue
         if field.varname:
@@ -751,7 +756,7 @@ def do_views(formdef, conn, cur, rebuild_global_views=True):
             # doesn't contain invalid characters.
             field_name = 'f_%s' % get_name_as_sql_identifier(field.varname)[:50]
         else:
-            field_name = 'f%s_%s' % (field.id, get_name_as_sql_identifier(field.label))
+            field_name = '%s_%s' % (get_field_id(field), get_name_as_sql_identifier(field.label))
             field_name = field_name[:50]
         if field_name in column_names:
             # it may happen that the same varname is used on multiple fields
@@ -763,7 +768,7 @@ def do_views(formdef, conn, cur, rebuild_global_views=True):
         column_names[field_name] = 1
         view_fields.append((field_key, field_name))
         if field.store_display_value:
-            field_key = 'f%s_display' % field.id
+            field_key = '%s_display' % get_field_id(field)
             view_fields.append((field_key, field_name + '_display'))
 
     view_fields.append(('''ARRAY(SELECT status FROM %s_evolutions '''\
@@ -1088,11 +1093,11 @@ class SqlMixin(object):
                     value = bytearray(cPickle.dumps(value, protocol=2))
                 elif sql_type == 'boolean':
                     pass
-            sql_dict['f%s' % field.id] = value
+            sql_dict[get_field_id(field)] = value
             if field.store_display_value:
-                sql_dict['f%s_display' % field.id] = data.get('%s_display' % field.id)
+                sql_dict['%s_display' % get_field_id(field)] = data.get('%s_display' % field.id)
             if field.store_structured_value:
-                sql_dict['f%s_structured' % field.id] = bytearray(
+                sql_dict['%s_structured' % get_field_id(field)] = bytearray(
                         cPickle.dumps(data.get('%s_structured' % field.id), protocol=2))
         return sql_dict
 
@@ -1479,11 +1484,11 @@ class SqlFormData(SqlMixin, wcs.formdata.FormData):
             sql_type = SQL_TYPE_MAPPING.get(field.key, 'varchar')
             if sql_type is None:
                 continue
-            data_fields.append('f%s' % field.id)
+            data_fields.append(get_field_id(field))
             if field.store_display_value:
-                data_fields.append('f%s_display' % field.id)
+                data_fields.append('%s_display' % get_field_id(field))
             if field.store_structured_value:
-                data_fields.append('f%s_structured' % field.id)
+                data_fields.append('%s_structured' % get_field_id(field))
         return data_fields
 
     @classmethod
@@ -1722,11 +1727,11 @@ class SqlUser(SqlMixin, wcs.users.User):
             sql_type = SQL_TYPE_MAPPING.get(field.key, 'varchar')
             if sql_type is None:
                 continue
-            data_fields.append('f%s' % field.id)
+            data_fields.append(get_field_id(field))
             if field.store_display_value:
-                data_fields.append('f%s_display' % field.id)
+                data_fields.append('%s_display' % get_field_id(field))
             if field.store_structured_value:
-                data_fields.append('f%s_structured' % field.id)
+                data_fields.append('%s_structured' % get_field_id(field))
         return data_fields
 
     @classmethod
diff --git a/wcs/workflows.py b/wcs/workflows.py
index d1f6980f6..344f80e1e 100644
--- a/wcs/workflows.py
+++ b/wcs/workflows.py
@@ -267,14 +267,11 @@ class WorkflowBackofficeFieldsFormDef(FormDef):
         self.workflow = workflow
         if workflow.backoffice_fields_formdef and workflow.backoffice_fields_formdef.fields:
             self.fields = self.workflow.backoffice_fields_formdef.fields
-            self.max_field_id = max([int(x.id[len(self.field_prefix):]) for x in self.fields])
         else:
             self.fields = []
-            self.max_field_id = 0
 
     def get_new_field_id(self):
-        self.max_field_id += 1
-        return '%s%s' % (self.field_prefix, self.max_field_id)
+        return '%s%s' % (self.field_prefix, str(uuid.uuid4()))
 
     def store(self):
         self.workflow.backoffice_fields_formdef = self
-- 
2.20.1

