Conversation
vkrasnovyd
added
migration
luisa-beerboom
left a comment
luisa-beerboom
left a comment
There was a problem hiding this comment.
Generally looks good, i have some questions though.
| if instance.get("lead_motion_id") or datastore_instance.get("lead_motion_id"): | ||
| return [ | ||
| { | ||
| "type": (MotionErrorType.DIFF_VERSION), | ||
| "message": "You can define a diff_version only for the lead motion", | ||
| } | ||
| ] | ||
| return [] |
There was a problem hiding this comment.
Shouldn't there also be a requirement that normal motions need a diff version, or should it maybe be filled automatically?
There was a problem hiding this comment.
Valid point. @bastianjoel what is the desired backend behaviour in case no diff_version is provided for the normal motion?
There was a problem hiding this comment.
It can be left empty in that case. So no requirement to set that field needed.
There was a problem hiding this comment.
If you leave the possibility open to leave the field empty, what happens to lead motions that don't have the field set when the diff service is next updated?
Is there going to be a migration like in this PR every single time that happens?
Or are we just going to risk letting diffs for old motions break?
There was a problem hiding this comment.
If you leave the possibility open to leave the field empty, what happens to lead motions that don't have the field set when the diff service is next updated?
The compatibility layer will not be applied on these. I want to keep this option open for now.
Is there going to be a migration like in this PR every single time that happens?
No, if we decide that it needs to be required we can just set a default version in the client and apply the requirement check later.
bastianjoel
force-pushed
the
c-5355-diff-version
branch
from
be23578 to
f15042e
Compare
vkrasnovyd
force-pushed
the
c-5355-diff-version
branch
from
b9ed665 to
7bb5575
Compare
Reverts changes from merging main into this branch that have lead to some separate tests being merged together.
|
Client is now updated. You can merge after resolving the conflict |
3 participants
Needed for OpenSlides/openslides-client#5355
OpenSlides/openslides-meta#307