Directory: Fix directory migration crash #2265
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
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.
src/onegov/directory/migration.py
Outdated
| def detect_changed_options(self) -> None: | ||
| self.renamed_options: list[tuple[str, str]] = [] | ||
|
|
||
| for old_id, old_field in self.old.items(): | ||
| if isinstance(old_field, OptionsField) and old_id in self.new: | ||
| new_field = self.new[old_id] | ||
| if isinstance(new_field, OptionsField): | ||
| old_labels = [r.label for r in old_field.choices] | ||
| new_labels = [r.label for r in new_field.choices] | ||
|
|
||
| for o, n in zip(old_labels, new_labels): | ||
| if o != n: | ||
| self.renamed_options.append((o, n)) |
There was a problem hiding this comment.
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:
- reorder options without changing the options
- rename a single option without changing anything else
- 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)
- 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
Directory: Fix directory migration crash when renaming option labels
TYPE: Bugfix
LINK: ogc-2353