OpenSlides / openslides-backend

Backend service for OpenSlides 4+
MIT License
6 stars 26 forks source link

motion.number and category.prefix must be unique per meeting #1612

Closed r-peschke closed 1 year ago

r-peschke commented 1 year ago

See also documentation of motion.create, update and motion_category.number_motions.

reiterl commented 1 year ago

I have done the first point. What must be done in the second and the third point? As far as I know exists check_unique code in creation, etc. What is the problem with that code?

reiterl commented 1 year ago

In the motion code the set_number_mixin is used. It includes the function _check_if_unique() with a while-loop.

r-peschke commented 1 year ago

The check_if_unique seems okay for me. Look for tests and if there are no, write tests for duplicate-detections

reiterl commented 1 year ago

There is a test file for the set_number_mixin.

reiterl commented 1 year ago

I have added a pull request for the category.prefix part.

reiterl commented 1 year ago

Topic 2 & 3 need to be done -> Reopened.

reiterl commented 1 year ago

I talked to @rrenkert and @r-peschke about this issue. We need to check for topic 2 & 3 if the actions motion.create , motion.update and motion_category.number_motions are correct. Even in manuell setting of the number it should check on uniqueness.

reiterl commented 1 year ago

There are a lot of different tests in set_number_mixin. I think test_complex_example_manually_3 checks for not unique for manual in motion.create. Mmh, this number code has a lot of tests. But I don't see motion.update here. I added a test to motion.update and see a problem here, it seems like the action doesn't check on uniques.

reiterl commented 1 year ago

I think the motion.update is missing the check unique and the set_number functionality.

reiterl commented 1 year ago

Add a pull request for a check unique number in motion.update. Add a test.

reiterl commented 1 year ago

The motion_category.number_motion part is missing, and we should think about the motion.update set_number part.

jsangmeister commented 1 year ago

motion_category.number_motions already implements the check for uniqueness, as described in https://github.com/OpenSlides/OpenSlides/wiki/motion_category.number_motions#actions and tested in https://github.com/OpenSlides/openslides-backend/blob/d2006f6432a68af1d5d90d567903b68343c68d53/tests/system/action/motion_category/test_number_motions.py#L223

motion.{create,update} do as well with the SetNumberMixin, which is tested at least for motion.update in https://github.com/OpenSlides/openslides-backend/blob/3533cad1d29caccdac2022f846b14946719ad98e/tests/system/action/motion/test_update.py#L545

I'm not a fan of changing the behaviour of motion_category.number_motions, since the logic is very complex and tightly knotted with the other number-changing actions. Is this necessary or can this be closed? @r-peschke @emanuelschuetze

rrenkert commented 1 year ago

@r-peschke your opinion?

jsangmeister commented 1 year ago

No change neede atm.