Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update motion workflow export #2862

Merged

Conversation

luisa-beerboom
Copy link
Member

Close #2555

@bastianjoel bastianjoel removed their assignment Oct 6, 2023
Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allow_motion_forwarding and set_workflow_timestamp are only exported if it was activated once
It should always be exported even if it was never true

@luisa-beerboom
Copy link
Member Author

The reason why this happens is because all other workflow fields have defaults set in the backend. This causes the value to be automatically set in the backend upon workflow creation, meaning that those fields will have a value, even if they were never changed.

Therefore even the fields that are off by default technically have a value at the start, except for the two new fields which have no default set and will therefore initialized without a value (which will be interpreted as "false" in the client).
Since those two fields have no value, they will not be registered by the export.

There are multiple ways in which this may be changed.

  1. I could build an extra conditional into the client and add the two fields, if they are empty, or
  2. The backend could set defaults for these two fields (which may require a migration to fix this problem on legacy workflows)
  3. The backend could delete the defaults of all boolean fields that are false by default (which would at least create consistency in that this behaviour would now happen to all these fields)

Given that the workflow.import and all workflow-related functionality will still work perfectly fine with how the system currently is, however, I question whether it is really necessary to change anything here.

What do you think would be best @Elblinator @rrenkert

@rrenkert
Copy link
Member

Please implement option 2.

Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the new course of action the client PR is working fine. Before merging however the back-end should be adjusted

see OpenSlides/openslides-backend#1930

@Elblinator Elblinator added the waiting Waiting for some other PR/feature; more details in comments label Oct 18, 2023
@jsangmeister
Copy link
Contributor

Do we really want to add a migration for this? It feels like a huge overhead when the client could just fill in empty boolean fields with false by default, considering how long our migrations take, especially on big instances.

@Elblinator Elblinator removed the waiting Waiting for some other PR/feature; more details in comments label Nov 24, 2023
@Elblinator Elblinator merged commit 27a2531 into OpenSlides:main Nov 24, 2023
7 checks passed
m-schieder added a commit that referenced this pull request Dec 6, 2023
* commit 'e7432ec2412d0377a158ecee44f5964a31c767a3':
  Make router service consider ongoing navigation before login navigation (#3069)
  Remove translation marker (#3061)
  Display password change errors (#3056)
  Align Multiselect Text centered (#2976)
  Add tooltips (#2407)
  Fix reload cancel causing issues (#3053)
  Fix linenumber skip resulting in error in textNodeToLines (#3050)
  Update motion workflow export (#2862)
  Fix os-list filter bar slot (#3049)
  Fix linter warnings (#3035)
  Remove unused css classes (#3043)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the fields set_workflow_timestamp and allow_motion_forwarding to the motion workflow export
5 participants