Directory: Fix directory migration crash #2265
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file 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.
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks pretty good. We're getting close, but I think we're now being a bit to laissez-faire with renaming options.
| def remove_old_options(self, values: dict[str, Any]) -> None: | ||
| for human_id, label in self.changes.removed_options: | ||
| id = as_internal_id(human_id) | ||
| if id in values: | ||
| if isinstance(values[id], list): | ||
| values[id] = [v for v in values[id] if v != label] | ||
| elif values[id] == label: | ||
| values[id] = None |
There was a problem hiding this comment.
It seems slightly controversial to delete existing selections. We might want to add an additional warning/confirmation step, if we detect that removing/renaming an option would modify the recorded data of existing entries (it would always be fine if none of the existing entries have selected this option).
There was a problem hiding this comment.
It is actually the same as with removing fields.
This is what you currently need to confirm:
Is that what you meant (A) or do you think of an extra step (B) where each entry would need to be changed in a separate window before an option can be removed?
There was a problem hiding this comment.
Yes, although it might be nice to be a bit more explicit that some data will be lost if there are existing entries that use the removed option.
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
| form.populate_obj(self.directory) | ||
|
|
||
| try: | ||
| form.populate_obj(self.directory) |
There was a problem hiding this comment.
Are we doing a good enough job of visualizing the ValidationError that can be emitted by Directory.update as part of DirectoryMigration.execute? E.g. when we add a required field to a directory with existing entries, are we getting a better error than "Eingabe erforderlich", do we see which field caused the issue? It might be worth detecting this common mistake, to provide a better error message, since you can never add a required field (other than a checkbox/radio field with a pre-selected option) to a directory that already has some entries, you will always need to make it optional first, fill in the field for all existing entries and then update the field to be required.
There was a problem hiding this comment.
The ValidationError pops up saying which field is required. But when opening the link to the entry the new field does not exist yet so we end up in a deadlock.
I will detect this mistake and through an error to the user preventing required fields in one step
Directory: Fix directory migration crash when renaming option labels
TYPE: Bugfix
LINK: ogc-2353