Implement motion import #1923

luisa-beerboom · 2023-10-12T14:35:32Z

luisa-beerboom · 2023-10-27T14:12:17Z

Up until now the json_upload seems to work in all cases involving only the required fields (including reason, which is sometimes required), as well as the number field.

Submitter generation in case of omission seems to be working as well.

The categories have been tested in so far that the json_upload seems to work for most common cases (there are still some untested edge cases though)

There are some remaining TODOs and all other fields have not yet been tested at all.
The import too is neither finished, nor tested.

@r-peschke If you wish to have a look at and give feedback to the finished parts sometime next week, please feel free to do so, I'd appreciate that

openslides_backend/action/actions/motion/json_upload.py

openslides_backend/action/mixins/import_mixins.py

tests/system/action/motion/test_json_upload.py

r-peschke · 2023-11-24T15:03:04Z

openslides_backend/action/actions/motion/json_upload.py

+        if (category_name := entry.get("category_name")) and isinstance(
+            category_name, str
+        ):
+            category_prefix = entry.get("category_prefix") or None


You don't need the or None. The get-method returns the value, if the dictionary has the attribute or it returns None.

I need or None because I need this to ignore empty strings

Then please write a test for this edge case:-).

Already exists

Then there should be at minimum 1 failing test

What do you think should fail?

If the and None is necessary, there should be a reason why it is necessary. I would expect removing it, changes the behaviour. And this change should be manifested in a failing test

I removed the or None and it still works, seems it was unnecessary

openslides_backend/action/actions/motion/json_upload.py

r-peschke · 2023-11-24T15:34:03Z

openslides_backend/action/actions/motion/json_upload.py

+        mapped_fields: List[str] = [],
+        and_filters: List[Filter] = [],
+    ) -> Dict[str, List[Dict[str, Any]]]:
+        lookup: Dict[str, List[Dict[str, Any]]] = {}


instead of {} you should use defaultdict(List), the advantage see above

There is nothing above

There is nothing above

Sorry, I meant below

At what line of what file is that supposed to be? I can't find it

finally you found

I still have no idea what I am supposed to have "found"

openslides_backend/action/actions/motion/json_upload.py

r-peschke

Some small things.

r-peschke · 2023-11-28T13:51:10Z

openslides_backend/action/actions/motion/json_upload.py

+        if (category_name := entry.get("category_name")) and isinstance(
+            category_name, str
+        ):
+            category_prefix = entry.get("category_prefix") or None


Then there should be at minimum 1 failing test

r-peschke · 2023-11-28T14:01:14Z

openslides_backend/action/actions/motion/json_upload.py

+        mapped_fields: List[str] = [],
+        and_filters: List[Filter] = [],
+    ) -> Dict[str, List[Dict[str, Any]]]:
+        lookup: Dict[str, List[Dict[str, Any]]] = {}


finally you found

openslides_backend/action/actions/motion/json_upload.py

r-peschke

Reviewed parts of the user(submitter, supporter) validation and I think there are some helpful modifications.

openslides_backend/action/actions/motion/set_number_mixin.py

openslides_backend/action/actions/motion/json_upload.py

r-peschke · 2023-12-04T12:43:46Z

openslides_backend/action/actions/motion/json_upload.py

+        has_submitter_error: bool = False
+        for fieldname in ["submitter", "supporter"]:
+            if users := entry.get(f"{fieldname}s_username"):
+                if not isinstance(users, list):


From schema validation and import_mixin it's guaranteed to be a list. Write 1 test to check this in a new class and throw an exception, if it is not.

I removed the if clause and it still works. What should the test check for?

The test should check, whether the action codes recognizes the error, if the given submitters or supporters are not in a list.
The code could repair the problem, i.e make it a list or throw an exception, that it is no list.

I already have tests that check if it works when the submitters/supporters are not in a list.
It works, even after removing the if clauses. I surmise that the if clauses were unnecessary.
Does this address your concerns for this remark?

r-peschke · 2023-12-04T12:47:00Z

openslides_backend/action/actions/motion/json_upload.py

+                entry_list: list[Dict[str, Any]] = []
+                message_set = set()
+                lookup = (
+                    self.submitter_lookup


Why do you use 2 different lookups. You could join the data for all users in 1 lookup, because it doesn't matter, where you find the user.

Ah, that is a remainder from when I actually used the lookup class for this, I'll change it

Done in json_upload, still have to look at the import

Done in import as well

r-peschke · 2023-12-04T12:50:23Z

openslides_backend/action/actions/motion/json_upload.py

+                            entry_list.append(
+                                {"value": user, "info": ImportState.WARNING}
+                            )
+                            message_set.add(


You could add the multi-referenced usernames and put them finally into one message

Done for users

Done for tags

Done in import as well

r-peschke · 2023-12-04T12:51:23Z

openslides_backend/action/actions/motion/json_upload.py

+                            )
+                        else:
+                            username_set.add(user)
+                            found_users = lookup.get(user, [])


You cannot get more than one user, the username is unique. If you find duplicate users, you should throw an exception and finish the json_upload immediately,

r-peschke

Sorry, but there are some things.
Tomorrow I'll continue with import, ...

openslides_backend/action/actions/motion/update.py

tests/system/action/motion/test_create.py

openslides_backend/action/actions/motion/json_upload.py

r-peschke · 2024-02-15T17:12:49Z

openslides_backend/action/actions/motion/json_upload.py

+            )
+        return self._first_state_id
+
+    def _get_field_array(self, entry: dict[str, Any], fieldname: str) -> list[str]:


Seems, that this method is never used

I've deleted it

openslides_backend/action/actions/motion/json_upload.py

r-peschke · 2024-02-16T09:47:06Z

openslides_backend/action/actions/motion/payload_validation_mixin.py

+                )
+        recommendation_check = self._check_recommendation_and_state(instance)
+        if recommendation_check:
+            errors += recommendation_check


Could you write a test with an error in recommendation_checks?

Done (see motion/test_update.py test_only_motion_allowed and ..._2

r-peschke · 2024-02-16T09:51:00Z

openslides_backend/action/actions/motion/payload_validation_mixin.py

+                errors.append(
+                    {
+                        "type": MotionErrorType.MOTION_TYPE,
+                        "message": "You can't give both of lead_motion_id and statute_paragraph_id.",


Missing test for this error

r-peschke · 2024-02-16T09:54:46Z

openslides_backend/action/actions/motion/payload_validation_mixin.py

+                errors.append(
+                    {
+                        "type": MotionErrorType.AMENDMENT_PARAGRAPHS,
+                        "message": "You can't give amendment_paragraphs in this context",


Also missing a test for this error

r-peschke · 2024-02-16T10:07:51Z

openslides_backend/action/actions/motion/import_.py

+                        payload, meeting_id
+                    )
+                for err in errors:
+                    fieldname = ""


Maybe the problem will be resolved by other tests, but there is no test with errors passing this code

openslides_backend/action/actions/motion/import_.py

openslides_backend/action/actions/motion/json_upload.py

r-peschke · 2024-02-20T14:05:16Z

tests/system/action/motion/test_create.py

+            },
+        )
+        self.assert_status_code(response, 200)
+        motion = self.get_model("motion/1")


Here you should use the self.assert_model_exists-method

openslides_backend/action/actions/motion/update.py

* commit '7e10e3f80bc28f6a87cbb7ff426e84dd5fd3ab4f': (90 commits) Add cascade deletion to new motion fields (#2243) (#2284) Update all translations (#2282) Implement certain preventions for changing active speakers state (#2272) Update meta (#2269) Implement cascade delete for amendments (#2253) Automatically create PRs for commits on staging branches (#2256) Add staging branches to CI (#2255) Fix PoO for others (#2242) Make SLLoS ignore point-of-order time (#2240) Bump types-bleach in /requirements/partial (#2238) Update meta repository (#2235) Implement motion import (#1923) Enhance speaker creation (#2231) Add migration command to clear collectionfield tables (#2217) Allow initial_time to be changed regardless of speach status (#2220) Bump types-simplejson in /requirements/partial (#2223) Bump types-redis in /requirements/partial (#2221) Bump pypdf[crypto] from 4.0.1 to 4.0.2 in /requirements/partial (#2222) Bump pytest from 8.0.0 to 8.0.1 in /requirements/partial (#2224) Bump types-requests in /requirements/partial (#2225) ...

luisa-beerboom added the feature label Oct 12, 2023

luisa-beerboom added this to the 4.1 milestone Oct 12, 2023

luisa-beerboom self-assigned this Oct 12, 2023

luisa-beerboom force-pushed the 1713-motion-import branch from daa8d72 to c4ffa8e Compare October 26, 2023 14:08

luisa-beerboom force-pushed the 1713-motion-import branch 2 times, most recently from 740cd28 to e0270b9 Compare November 9, 2023 09:56

luisa-beerboom force-pushed the 1713-motion-import branch from f79517b to 1fbfbf4 Compare November 14, 2023 13:31

luisa-beerboom force-pushed the 1713-motion-import branch from dcf4413 to 94c77aa Compare November 22, 2023 10:46

luisa-beerboom mentioned this pull request Nov 23, 2023

Implement motion import via backend OpenSlides/openslides-client#3042

Closed

luisa-beerboom assigned r-peschke and unassigned luisa-beerboom Nov 23, 2023

luisa-beerboom requested a review from r-peschke November 23, 2023 16:39

luisa-beerboom marked this pull request as ready for review November 23, 2023 16:39

r-peschke requested changes Nov 24, 2023

View reviewed changes

r-peschke assigned luisa-beerboom and unassigned r-peschke Nov 28, 2023

luisa-beerboom requested a review from r-peschke November 28, 2023 13:15

luisa-beerboom assigned r-peschke and unassigned luisa-beerboom Nov 28, 2023

r-peschke requested changes Nov 28, 2023

View reviewed changes

r-peschke assigned luisa-beerboom and unassigned r-peschke Nov 28, 2023

luisa-beerboom requested a review from r-peschke November 28, 2023 14:47

luisa-beerboom assigned r-peschke and unassigned luisa-beerboom Nov 30, 2023

r-peschke requested changes Dec 4, 2023

View reviewed changes

r-peschke assigned luisa-beerboom and unassigned r-peschke Dec 4, 2023

luisa-beerboom force-pushed the 1713-motion-import branch from 9f89ca8 to 40cfc0a Compare December 5, 2023 15:29

Merge remote-tracking branch 'upstream/main' into 1713-motion-import

5576ffd

luisa-beerboom requested a review from r-peschke February 15, 2024 10:33

luisa-beerboom assigned r-peschke and unassigned luisa-beerboom Feb 15, 2024

luisa-beerboom added 2 commits February 15, 2024 12:36

cleanup

4d6148e

Maybe fix tests

6e5924c

r-peschke requested changes Feb 15, 2024

View reviewed changes

r-peschke requested changes Feb 16, 2024

View reviewed changes

Add som tests

cfdae3d

luisa-beerboom assigned luisa-beerboom and unassigned r-peschke Feb 19, 2024

luisa-beerboom added 2 commits February 19, 2024 14:35

Fix tests

944c74b

Merge remote-tracking branch 'upstream/main' into 1713-motion-import

d102dfd

luisa-beerboom requested a review from r-peschke February 19, 2024 13:59

luisa-beerboom assigned r-peschke and unassigned luisa-beerboom Feb 19, 2024

Fix meta reference

21ed7af

r-peschke approved these changes Feb 20, 2024

View reviewed changes

r-peschke assigned luisa-beerboom and unassigned r-peschke Feb 20, 2024

luisa-beerboom and others added 5 commits February 21, 2024 15:30

use assert_model_exists

79fc088

Update meta to main

ebddc9a

Update meta to main 2

9cb3bb5

Merge remote-tracking branch 'upstream/main' into 1713-motion-import

aa818a6

Merge branch 'main' into 1713-motion-import

201e85f

luisa-beerboom added this pull request to the merge queue Feb 21, 2024

Merged via the queue into OpenSlides:main with commit 8a1a78d Feb 21, 2024
5 checks passed

luisa-beerboom deleted the 1713-motion-import branch February 21, 2024 15:58

Implement motion import #1923

Implement motion import #1923

Conversation

luisa-beerboom commented Oct 12, 2023 • edited Loading

luisa-beerboom commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisa-beerboom Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-peschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-peschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-peschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisa-beerboom commented Oct 12, 2023 •

edited

Loading

luisa-beerboom Nov 28, 2023 •

edited

Loading