Directory: Fix directory migration crash

[Tschuppi81]

Directory: Fix directory migration crash when renaming option labels

TYPE: Bugfix
LINK: ogc-2353

[linear]

OGC-2353 Crash in Verzeichnis-Migration wenn eine existierende Auswahl entfernt wird

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2604	1	2603	17

View the top 1 failed test(s) by shortest run time

tests/test_hierarchy.py::test_hierarchy

Stack Traces | 25.6s run time

def test_hierarchy() -> None:
        """ Originally, onegov.* modules were separated into separate repositories
        and deployed individually to PyPI.
    
        This meant that each module would list the dependencies it needed,
        including other onegov.* modules. As a side-effect, this ensured that
        a module like onegov.core would not import from onegov.org, creating
        an undesired dependency.
    
        With the move to a single repository and a container build, we lost this
        side-effect. It is now possible for onegov.core to import from onegov.org
        and that is not something we want, because things like the core should
        not import from modules higher up the chain.
    
        This test ensures that this restriction is still honored.
    
        Each module is put into a level. Modules may import from the same level
        or the levels below, but not from the levels above.
    
        The current list of levels is also used for the upgrade step order. It can
        be found in `onegov.core.__init__.py`.
    
        This is not exactly equivalent to what we had before, but it is good
        basic check to ensure that we do not add unwanted dependencies.
    
        """
    
        modules = level_by_module(LEVELS)
    
        # all modules must be defined
        for module in existing_modules():
            assert module in modules, f"module not defined in hierarchy: {module}"
    
        # graph all imports
        graph = ModuleGraph()
        graph.parsePathname(str(sources()))
    
        # ensure hierarchy
        for id, module in graph.modules.items():
            name = module_name(module.filename)
    
            if name is None:
                continue
    
            allowed = allowed_imports(LEVELS, name)
    
            for imported in module.imported_names:
                import_name = '.'.join(imported.name.split('.')[:2])
    
                if not import_name.startswith('onegov'):
                    continue
    
>               assert import_name in allowed, \
                    f"Invalid import {name} → {import_name} in {imported.filename}"
E               AssertionError: Invalid import onegov.directory → onegov.org in .../onegov/directory/migration.py
E               assert 'onegov.org' in {'onegov.activity', 'onegov.api', 'onegov.async_http', 'onegov.chat', 'onegov.core', 'onegov.directory', ...}

tests/test_hierarchy.py:68: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[Daverball]

This looks like a decent start, but we need to allow more operations than renaming a single option, there's many other legal changes, that will not result in form validation errors on existing entries.

[Daverball]

How do you distinguish between someone renaming an existing option, removing an existing option and inserting a new option between two existing options? Also someone might just rearrange the options into a new order, which should be allowed, since the underlying data doesn't have to change for that.

You can't just assume that every change in this list is intended to rename an existing option. I'd say the only time you could treat a change as renaming an option, is if the options are exactly the same as before, apart from a single options, that now has changed its label.

So legal operations:

1. reorder options without changing the options
2. rename a single option without changing anything else
3. remove an arbitrary number of options (potentially only if they haven't been selected before, but we could also display a warning instead, one case that definitely should be handled however is removing this option would turn a required field into a field with no value for an existing entry, which I believe is the original impetus for this change and the Sentry issue we got, since the form generated a validation error after the option was removed)
4. add an arbitrary number of new options without changing anything else (they can be inserted between existing options, but the existing options have to appear in the same order as before)

Any other change is ambiguous and we should disallow it on princriple