From 5057f02f3cfe78c493de1e65f004cce3ebc8310f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20P=C3=A9ters?= Date: Wed, 19 Jul 2017 20:42:33 +0200 Subject: [PATCH 1/2] formdef: add internal identifier attribute, separated from url_name (#15663) This will allow changing url name behaviour without impacting on storage matters. --- tests/test_admin_pages.py | 18 ++++++++----- tests/test_formdef.py | 66 +++++++++++++++++++++++++++++++++-------------- tests/test_sql.py | 2 +- wcs/admin/forms.py | 5 ++-- wcs/formdef.py | 58 +++++++++++++++++++++++++++++++---------- wcs/sql.py | 2 +- 6 files changed, 106 insertions(+), 45 deletions(-) diff --git a/tests/test_admin_pages.py b/tests/test_admin_pages.py index a65fc3bf..c9c99e5b 100644 --- a/tests/test_admin_pages.py +++ b/tests/test_admin_pages.py @@ -328,7 +328,8 @@ def test_forms_edit(pub): assert resp.location == 'http://example.net/backoffice/forms/1/' resp = resp.follow() assert FormDef.get(1).name == 'new title' - assert FormDef.get(1).url_name == 'new-title' + assert FormDef.get(1).url_name == 'form-title' + assert FormDef.get(1).internal_identifier == 'new-title' def test_form_category(pub): create_superuser(pub) @@ -745,8 +746,8 @@ def test_form_import(pub): resp = resp.forms[0].submit() assert FormDef.count() == 1 - # import the same formdef a second time, make sure the urlname is not - # reused + # import the same formdef a second time, make sure url name and internal + # identifier are not reused resp = app.get('/backoffice/forms/') resp = resp.click(href='import') resp.forms[0]['file'] = Upload('formdef.wcs', formdef_xml) @@ -754,9 +755,11 @@ def test_form_import(pub): assert FormDef.count() == 2 assert FormDef.get(1).url_name == 'form-title' assert FormDef.get(2).url_name == 'form-title-1' + assert FormDef.get(1).internal_identifier == 'form-title' + assert FormDef.get(2).internal_identifier == 'form-title-1' - # import a formdef with an url_name that doesn't match its title, - # it should be updated to match. + # import a formdef with an url name that doesn't match its title, + # it should be kept intact. formdef.url_name = 'xxx-other-form-title' formdef_xml = ET.tostring(formdef.export_to_xml(include_id=True)) @@ -764,7 +767,8 @@ def test_form_import(pub): resp = resp.click(href='import') resp.forms[0]['file'] = Upload('formdef.wcs', formdef_xml) resp = resp.forms[0].submit() - assert FormDef.get(3).url_name == 'form-title-2' + assert FormDef.get(3).url_name == 'xxx-other-form-title' + assert FormDef.get(3).internal_identifier == 'form-title-2' # import an invalid file resp = app.get('/backoffice/forms/') @@ -1418,7 +1422,7 @@ def test_form_overwrite(pub): app = login(get_app(pub)) resp = app.get('/backoffice/forms/%s/' % formdef_id) - resp = resp.click(href='overwrite') + resp = resp.click(href='overwrite', index=0) resp.forms[0]['file'] = Upload('formdef.wcs', formdef_xml) resp = resp.forms[0].submit() assert 'The form removes and changes fields' in resp.body diff --git a/tests/test_formdef.py b/tests/test_formdef.py index 8c017e6a..389e059c 100644 --- a/tests/test_formdef.py +++ b/tests/test_formdef.py @@ -1,3 +1,4 @@ +import cPickle import datetime import json import sys @@ -9,24 +10,31 @@ import pytest from quixote import cleanup from wcs import formdef from wcs.formdef import FormDef +from wcs.qommon.http_request import HTTPRequest from wcs.workflows import Workflow from wcs.fields import StringField, FileField, DateField, ItemField -from utilities import create_temporary_pub +from utilities import create_temporary_pub, clean_temporary_pub -def setup_module(module): - cleanup() +def pytest_generate_tests(metafunc): + if 'pub' in metafunc.fixturenames: + metafunc.parametrize('pub', ['pickle', 'sql'], indirect=True) - global pub - - pub = create_temporary_pub() +@pytest.fixture +def pub(request): + pub = create_temporary_pub(sql_mode=(request.param == 'sql')) + req = HTTPRequest(None, {'SCRIPT_NAME': '/', 'SERVER_NAME': 'example.net'}) + pub.set_app_dir(req) pub.cfg['language'] = {'language': 'en'} + pub.write_cfg() + + return pub def teardown_module(module): - shutil.rmtree(pub.APP_DIR) + clean_temporary_pub() -def test_is_disabled(): +def test_is_disabled(pub): formdef = FormDef() assert not formdef.is_disabled() @@ -34,7 +42,7 @@ def test_is_disabled(): assert formdef.is_disabled() -def test_is_disabled_publication_date(): +def test_is_disabled_publication_date(pub): formdef = FormDef() formdef.publication_date = '%s-%02d-%02d' % (datetime.datetime.today() - datetime.timedelta(1)).timetuple()[:3] @@ -44,7 +52,7 @@ def test_is_disabled_publication_date(): assert formdef.is_disabled() -def test_is_disabled_expiration_date(): +def test_is_disabled_expiration_date(pub): formdef = FormDef() formdef.expiration_date = '%s-%02d-%02d' % (datetime.datetime.today() - datetime.timedelta(1)).timetuple()[:3] @@ -54,7 +62,7 @@ def test_is_disabled_expiration_date(): assert not formdef.is_disabled() -def test_is_disabled_publication_datetime(): +def test_is_disabled_publication_datetime(pub): formdef = FormDef() formdef.publication_date = '%s-%02d-%02d %02d:%02d' % ( @@ -66,7 +74,7 @@ def test_is_disabled_publication_datetime(): assert formdef.is_disabled() -def test_is_disabled_expiration_datetime(): +def test_is_disabled_expiration_datetime(pub): formdef = FormDef() formdef.expiration_date = '%s-%02d-%02d %02d:%02d' % ( @@ -77,26 +85,28 @@ def test_is_disabled_expiration_datetime(): datetime.datetime.now() + datetime.timedelta(hours=1)).timetuple()[:5] assert not formdef.is_disabled() -def test_title_change(): +def test_title_change(pub): formdef = FormDef() formdef.name = 'foo' formdef.store() assert FormDef.get(formdef.id).name == 'foo' assert FormDef.get(formdef.id).url_name == 'foo' + assert FormDef.get(formdef.id).internal_identifier == 'foo' formdef.name = 'bar' formdef.store() assert FormDef.get(formdef.id).name == 'bar' - assert FormDef.get(formdef.id).url_name == 'bar' + assert FormDef.get(formdef.id).url_name == 'foo' + assert FormDef.get(formdef.id).internal_identifier == 'bar' - # makes sure the url_name doesn't change if there are submitted forms + # makes sure the internal_name doesn't change if there are submitted forms formdef.data_class()().store() formdef.name = 'baz' formdef.store() assert FormDef.get(formdef.id).name == 'baz' - assert FormDef.get(formdef.id).url_name == 'bar' # didn't change + assert FormDef.get(formdef.id).internal_identifier == 'bar' # didn't change -def test_substitution_variables(): +def test_substitution_variables(pub): formdef = FormDef() formdef.name = 'foo' formdef.store() @@ -121,7 +131,7 @@ def test_substitution_variables(): assert formdef.get_substitution_variables()['form_option_bar'] == 'Bar' assert formdef.get_substitution_variables()['form_option_bar_raw'] == 'bar' -def test_schema_with_date_variable(): +def test_schema_with_date_variable(pub): FormDef.wipe() formdef = FormDef() formdef.name = 'foo' @@ -137,7 +147,7 @@ def test_schema_with_date_variable(): formdef.workflow_options = {'foo': time.gmtime(time.mktime((2016, 4, 2, 0, 0, 0, 0, 0, 0)))} assert json.loads(formdef.export_to_json())['options']['foo'].startswith('2016-04') -def test_substitution_variables_object(): +def test_substitution_variables_object(pub): formdef = FormDef() formdef.name = 'foo' formdef.store() @@ -157,7 +167,7 @@ def test_substitution_variables_object(): with pytest.raises(AttributeError): assert substs.foobar -def test_file_field_migration(): +def test_file_field_migration(pub): pub.cfg['filetypes'] = {1: {'mimetypes': [ 'application/pdf', @@ -185,3 +195,19 @@ def test_file_field_migration(): assert formdef.fields[0].document_type['mimetypes'] == ['image/*', 'application/pdf,application/vnd.oasis.opendocument.text,application/msword,application/vnd.openxmlformats-officedocument.wordprocessingml.document,application/vnd.oasis.opendocument.spreadsheet,application/vnd.ms-excel,application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'] assert formdef.fields[1].document_type['label'] == 'Image files' assert formdef.fields[0].document_type['label'] == 'Image files, Documents' + +def test_internal_identifier_migration(pub): + FormDef.wipe() + formdef = FormDef() + formdef.name = 'foo' + formdef.fields = [] + formdef.store() + + obj = cPickle.load(open(formdef.get_object_filename())) + del obj.internal_identifier + cPickle.dump(obj, open(formdef.get_object_filename(), 'w')) + assert cPickle.load(open(formdef.get_object_filename())).internal_identifier is None + assert FormDef.get(formdef.id, ignore_migration=True).internal_identifier is None + + formdef = FormDef.get(formdef.id) + assert formdef.internal_identifier == 'foo' diff --git a/tests/test_sql.py b/tests/test_sql.py index f8251456..cb3a6204 100644 --- a/tests/test_sql.py +++ b/tests/test_sql.py @@ -569,7 +569,7 @@ def test_urlname_change(): formdef.name = 'tests2' formdef.store() - assert formdef.url_name == 'tests2' + assert formdef.url_name == 'tests' formdef.name = 'tests' formdef.store() diff --git a/wcs/admin/forms.py b/wcs/admin/forms.py index 1b415a8d..1cd19d94 100644 --- a/wcs/admin/forms.py +++ b/wcs/admin/forms.py @@ -949,8 +949,9 @@ class FormDefPage(Directory): return redirect('.') def overwrite_by_formdef(self, new_formdef): - # keep current formdef id, url_name and sql table name + # 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 new_formdef.url_name = self.formdef.url_name new_formdef.table_name = self.formdef.table_name # keep currently assigned category and workflow @@ -1552,7 +1553,7 @@ class FormsDirectory(AccessControlled, Directory): form.set_error('file', msg) raise ValueError() - formdef.url_name = None # a new one will be set in .store() + formdef.internal_identifier = None # a new one will be set in .store() formdef.disabled = True formdef.store() get_session().message = ('info', diff --git a/wcs/formdef.py b/wcs/formdef.py index 24f82312..fa459832 100644 --- a/wcs/formdef.py +++ b/wcs/formdef.py @@ -69,6 +69,7 @@ class FormDef(StorableObject): description = None keywords = None url_name = None + internal_identifier = None # mostly for pickle table_name = None # for SQL only fields = None category_id = None @@ -101,7 +102,7 @@ class FormDef(StorableObject): # declarations for serialization TEXT_ATTRIBUTES = ['name', 'url_name', 'description', 'keywords', - 'publication_date', 'expiration_date', + 'publication_date', 'expiration_date', 'internal_identifier', 'disabled_redirection',] BOOLEAN_ATTRIBUTES = ['discussion', 'detailed_emails', 'disabled', 'only_allow_one', 'enable_tracking_codes', 'confirmation', @@ -204,6 +205,10 @@ class FormDef(StorableObject): changed = True break + if not self.internal_identifier: + self.internal_identifier = self.url_name + changed = True + for f in self.fields or []: changed |= f.migrate() @@ -238,7 +243,7 @@ class FormDef(StorableObject): actions = sql.do_formdef_tables(self) else: cls = new.classobj(self.url_name.title(), (FormData,), - {'_names': 'form-%s' % self.url_name, + {'_names': 'form-%s' % self.internal_identifier, '_formdef': self}) actions = [] setattr(sys.modules['formdef'], self.url_name.title(), cls) @@ -264,15 +269,30 @@ class FormDef(StorableObject): suffix_no = 0 while True: try: - formdef = self.get_by_urlname(new_url_name, ignore_migration=True) + obj = self.get_on_index(new_url_name, 'url_name', ignore_migration=True) except KeyError: break - if formdef.id == self.id: + if obj.id == self.id: break suffix_no += 1 new_url_name = '%s-%s' % (base_new_url_name, suffix_no) return new_url_name + def get_new_internal_identifier(self): + new_internal_identifier = simplify(self.name) + base_new_internal_identifier = new_internal_identifier + suffix_no = 0 + while True: + try: + formdef = self.get_by_urlname(new_internal_identifier, ignore_migration=True) + except KeyError: + break + if formdef.id == self.id: + break + suffix_no += 1 + new_internal_identifier = '%s-%s' % (base_new_internal_identifier, suffix_no) + return new_internal_identifier + @classmethod def get_new_id(cls, create=False): keys = cls.keys() @@ -303,16 +323,18 @@ class FormDef(StorableObject): sql.formdef_wipe() def store(self): - new_url_name = self.get_new_url_name() - if not self.url_name: - self.url_name = new_url_name - if new_url_name != self.url_name: - # title changed, url will be changed only if the formdef is - # currently being imported (self.id is None) or if there are not - # yet any submitted forms - data_class = self.data_class() - if self.id is None or data_class().count() == 0: - self.url_name = new_url_name + if self.url_name is None: + # set url name if it's not yet there + self.url_name = self.get_new_url_name() + new_internal_identifier = self.get_new_internal_identifier() + if not self.internal_identifier: + self.internal_identifier = new_internal_identifier + if new_internal_identifier != self.internal_identifier: + # title changed, internal identifier will be changed only if + # the formdef is currently being imported (self.id is None) + # or if there are not yet any submitted forms + if self.id is None or self.data_class().count() == 0: + self.internal_identifier = new_internal_identifier self.last_modification_time = time.localtime() if get_request() and get_request().user: self.last_modification_user_id = str(get_request().user.id) @@ -846,6 +868,14 @@ class FormDef(StorableObject): formdef = cls.import_from_xml_tree(tree, charset=charset, include_id=include_id) + if formdef.url_name: + try: + obj = cls.get_on_index(formdef.url_name, 'url_name', ignore_migration=True) + except KeyError: + pass + else: + formdef.url_name = formdef.get_new_url_name() + # fix max_field_id if necessary if formdef.max_field_id is not None: max_field_id = max([lax_int(x.id) for x in formdef.fields]) diff --git a/wcs/sql.py b/wcs/sql.py index 3639b912..d51c3cab 100644 --- a/wcs/sql.py +++ b/wcs/sql.py @@ -762,7 +762,7 @@ def drop_global_views(conn, cur): def do_global_views(conn, cur): # recreate global views from wcs.formdef import FormDef - view_names = [get_formdef_view_name(x) for x in FormDef.select()] + view_names = [get_formdef_view_name(x) for x in FormDef.select(ignore_migration=True)] cur.execute('''SELECT table_name FROM information_schema.views WHERE table_schema = 'public' -- 2.13.3