Feature: Add Group and Group Membership data models to restapi #329
Conversation
| log.info("Request received", group_id=groupId) | ||
| group = self._group_service.get(groupId, error_if_not_found=True, log=log) | ||
|
|
||
| return cast(Group, group) |
There was a problem hiding this comment.
Not sure if necessary, service returns Group type already.
There was a problem hiding this comment.
mypy raises an error as it interprets the return from the query as Any.
| ) | ||
| name = fields.String( | ||
| attribute="name", | ||
| allow_none=True, # should we force the user to pick a name? |
There was a problem hiding this comment.
Leaving a note here for James to review. I think all groups should have names.
src/dioptra/restapi/group/service.py
Outdated
| class GroupService(object): | ||
| """The service methods for registering and managing groups by their unique id.""" | ||
|
|
||
| def get_all(self, **kwargs) -> List[Group]: |
There was a problem hiding this comment.
Change to returning -> list[Group]
src/dioptra/restapi/group/service.py
Outdated
| from __future__ import annotations | ||
|
|
||
| import datetime | ||
| from typing import Any, List, cast |
|
|
||
| log.info("Request received") | ||
|
|
||
| parsed_obj = request.parsed_obj # type: ignore |
There was a problem hiding this comment.
Might be worth defining default values for some of these parameters (not sure if read and write values will always be provided. I achieved this with request.args.get(), but that code still needs to be reviewed by James.
There was a problem hiding this comment.
For permissions, I think we should always force the creator to define what the added User can and cannot do explicitly. Otherwise we risk permissions being applied inappropriately, which may lead to further issues down the road. Maybe defaults can exist as part of the ui in terms of toggles? That way the user is at least fully aware of what is being set.
| """Retrieve a list of all group memberships. | ||
|
|
||
| Returns: | ||
| List[GroupMembership]: List of group memberships. |
There was a problem hiding this comment.
Typo, List instead of list.
There was a problem hiding this comment.
actually, the whole type should be removed from the docstring. My mistake for including it.
There was a problem hiding this comment.
Only file that needs major changes. Main takeaways are:
-Tests need to interact with the controller and not the service
-Tests need to be restructured to focus on user flow, rather than testing functionality individually.
-Tests need to be restructured according to the "actions", "assertions", and "tests" sections
-We might need to split up group and group_membership tests. As they are so closely related, maybe not if we can define tests which combine and cover them both (need James' input on this one).
Before going back over the file again, I would try to define some integrative user stories, get feedback, and then use them as a framework for reorganizing the tests.
Since the purpose of this PR is to get the group and group_membership src files merged, we can possibly separate out the tests into another issue and I can have a crack at them.
|
Closing due to REST V1 refactors and new database. |
resolves #287