[rel-DB] Complete presenter tests#3175
[rel-DB] Complete presenter tests#3175vkrasnovyd merged 9 commits intoOpenSlides:feature/relational-dbfrom
Conversation
vkrasnovyd
force-pushed
the
rel-db-presenter-tests
branch
2 times, most recently
from
4e0dfa6 to
1ab27c3
Compare
vkrasnovyd
force-pushed
the
rel-db-presenter-tests
branch
from
1ab27c3 to
0663850
Compare
hjanott
left a comment
hjanott
left a comment
There was a problem hiding this comment.
Here are my change requests. A lot of great improvements though.
| def set_group_permissions( | ||
| self, group_id: int, permissions: list[Permission] | ||
| ) -> None: | ||
| self.update_model(f"group/{group_id}", {"permissions": permissions}) | ||
|
|
||
| def add_group_permissions( | ||
| self, group_id: int, permissions: list[Permission] | ||
| ) -> None: | ||
| group = self.get_model(f"group/{group_id}") | ||
| self.set_group_permissions(group_id, group.get("permissions", []) + permissions) | ||
|
|
||
| def remove_group_permissions( | ||
| self, group_id: int, permissions: list[Permission] | ||
| ) -> None: | ||
| group = self.get_model(f"group/{group_id}") | ||
| new_perms = [p for p in group.get("permissions", []) if p not in permissions] | ||
| self.set_group_permissions(group_id, new_perms) |
There was a problem hiding this comment.
Please try instead of fetching, altering and pushing back to database to create manual events with:
[Event(type=EventType.Update, fqid=fqid, list_fields={"add": {}, "remove":{})]
You can see in the update_model how those events can be send to database.
There was a problem hiding this comment.
Fetching step is required for add_group_permissions and remove_group_permissions.
set_group_permissions (also used in the methods mentioned above) already creates events by calling update_model.
There was a problem hiding this comment.
Solved with creating a helper method for generating list events. Also moved some duplicated logic from multiple methods into get_write_request and renamed it.
| self.create_meeting_for_two_users(1, user_id, 111) | ||
| self.create_meeting_for_two_users(4, user_id, 111) |
There was a problem hiding this comment.
The variable user_id is nice and all but could be more descriptive (long: logged_in_user_id) and the other user ids (111, 777) could also be in a descriptive variable. I vote for doing it for this test. (fe.: logged_in_id, admin_id, unalterable_id)
There was a problem hiding this comment.
Using logged_in_user_id now.
There was a problem hiding this comment.
Replaced also the ids of the other 2 users and added a data assertion.
| "meeting_user/14": {"meeting_id": 1, "user_id": 4}, | ||
| "meeting_user/16": {"meeting_id": 1, "user_id": 6}, | ||
| "meeting_user/17": {"meeting_id": 1, "user_id": 7}, | ||
| "meeting_user/47": {"meeting_id": 4, "user_id": 7}, | ||
| "meeting_user/19": {"meeting_id": 1, "user_id": 9}, | ||
| "meeting_user/49": {"meeting_id": 4, "user_id": 9}, | ||
| "group/1": {"meeting_user_ids": [14, 16, 17, 19]}, | ||
| "group/4": {"meeting_user_ids": [47, 49]}, |
There was a problem hiding this comment.
I would prefer set_user_groups.
There was a problem hiding this comment.
Or even extra parameters in create_user. Replaced.
There was a problem hiding this comment.
- As discussed, updated
create_userso it can also takecommittee_management_ids. Now user scope can be completely defined when calling this method. - Updated usage of
create_userin 2 user test cases to utilize this new setting. - Checked the tests where the method is being currently used to make sure that the new variable does not break anything.
vkrasnovyd
merged commit c9311d8
into
OpenSlides:feature/relational-db
2 participants
Needs OpenSlides/openslides-meta#343 and #3174.
Most of the base test methods were moved from
BaseActionTestCaseto its parentBaseSystemTestCaseto make them available to the presenter tests. Content of the base methods was not changed, but the order was updated, so these methods are now grouped by their purpose.As discussed, checker related tests will be handled separately. Skipped them for now.