Feature/remove group service (#529)

* removed group service in favor of user service and messing around with ruff and pre-commeit w/ burnettk

* pyl can succeed now w/ burnettk

* removed debug code w/ burnettk

* pyl

---------

Co-authored-by: jasquat <jasquat@users.noreply.github.com>

.pre-commit-config.yaml

@@ -51,6 +51,8 @@ repos:
         language: system
         types: [python]
         require_serial: true
+        # this is also specified in spiffworkflow-backend/pyproject.toml but we run pre-commit
+        # with all-files which ignores that
         exclude: "/migrations/"
       - id: trailing-whitespace
         files: ^spiffworkflow-backend/

spiffworkflow-backend/bin/tests-par

@@ -18,4 +18,4 @@ if ! python -c "import xdist" &>/dev/null; then
   exit 1
 fi
 
-SPIFFWORKFLOW_BACKEND_DATABASE_TYPE=sqlite poet test -n auto -x --ff --random-order
+SPIFFWORKFLOW_BACKEND_DATABASE_TYPE=sqlite poet test -n auto -x --random-order

spiffworkflow-backend/pyproject.toml

@@ -197,6 +197,10 @@ line-length = 130
 # target python 3.10
 target-version = "py310"
 
+exclude = [
+  "migrations"
+]
+
 [tool.ruff.per-file-ignores]
 "migrations/versions/*.py" = ["E501"]
 "tests/**/*.py" = ["PLR2004", "S101"]  # PLR2004 is about magic vars, S101 allows assert

spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py

@@ -708,7 +708,7 @@ def task_save_draft(
 
     task_model = _get_task_model_from_guid_or_raise(task_guid, process_instance_id)
     full_bpmn_process_id_path = TaskService.full_bpmn_process_path(task_model.bpmn_process, "id")
-    task_definition_id_path = f"{':'.join(map(str,full_bpmn_process_id_path))}:{task_model.task_definition_id}"
+    task_definition_id_path = f"{':'.join(map(str, full_bpmn_process_id_path))}:{task_model.task_definition_id}"
     task_draft_data_dict: TaskDraftDataDict = {
         "process_instance_id": process_instance.id,
         "task_definition_id_path": task_definition_id_path,
@@ -974,7 +974,7 @@ def _prepare_form_data(
         wfe.add_note(f"Error in Json Form File '{form_file}'")
         api_error = ApiError.from_workflow_exception("instructions_error", str(wfe), exp=wfe)
         api_error.file_name = form_file
-        raise api_error
+        raise api_error from wfe
 
     try:
         # form_contents is a str

spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py

@@ -32,7 +32,6 @@ from spiffworkflow_backend.services.authentication_service import TokenExpiredEr
 from spiffworkflow_backend.services.authentication_service import TokenInvalidError
 from spiffworkflow_backend.services.authentication_service import TokenNotProvidedError
 from spiffworkflow_backend.services.authentication_service import UserNotLoggedInError
-from spiffworkflow_backend.services.group_service import GroupService
 from spiffworkflow_backend.services.user_service import UserService
 from sqlalchemy import and_
 from sqlalchemy import or_
@@ -146,7 +145,7 @@ class AuthorizationService:
         permission: str = "all",
         auth_token_properties: dict | None = None,
     ) -> None:
-        guest_user = GroupService.find_or_create_guest_user(username=username, group_identifier=group_identifier)
+        guest_user = UserService.find_or_create_guest_user(username=username, group_identifier=group_identifier)
         if permission_target is not None:
             cls.add_permission_from_uri_or_macro(group_identifier, permission=permission, target=permission_target)
         g.user = guest_user
@@ -508,14 +507,14 @@ class AuthorizationService:
                 )
             else:
                 for desired_group_identifier in desired_group_identifiers:
-                    GroupService.add_user_to_group(user_model, desired_group_identifier)
+                    UserService.add_user_to_group_by_group_identifier(user_model, desired_group_identifier)
                 current_group_identifiers = [g.identifier for g in user_model.groups]
                 groups_to_remove_from_user = [
                     item for item in current_group_identifiers if item not in desired_group_identifiers
                 ]
                 for gtrfu in groups_to_remove_from_user:
                     if gtrfu != current_app.config["SPIFFWORKFLOW_BACKEND_DEFAULT_USER_GROUP"]:
-                        GroupService.remove_user_from_group(user_model, gtrfu)
+                        UserService.remove_user_from_group(user_model, gtrfu)
 
         # this may eventually get too slow.
         # when it does, be careful about backgrounding, because
@@ -768,7 +767,7 @@ class AuthorizationService:
     def add_permission_from_uri_or_macro(
         cls, group_identifier: str, permission: str, target: str
     ) -> list[PermissionAssignmentModel]:
-        group = GroupService.find_or_create_group(group_identifier)
+        group = UserService.find_or_create_group(group_identifier)
         permissions_to_assign = cls.explode_permissions(permission, target)
         permission_assignments = []
         for permission_to_assign in permissions_to_assign:
@@ -833,12 +832,12 @@ class AuthorizationService:
         default_group = None
         default_group_identifier = current_app.config["SPIFFWORKFLOW_BACKEND_DEFAULT_USER_GROUP"]
         if default_group_identifier:
-            default_group = GroupService.find_or_create_group(default_group_identifier)
+            default_group = UserService.find_or_create_group(default_group_identifier)
             unique_user_group_identifiers.add(default_group_identifier)
 
         for group in group_permissions:
             group_identifier = group["name"]
-            GroupService.find_or_create_group(group_identifier)
+            UserService.find_or_create_group(group_identifier)
             if not group_permissions_only:
                 for username in group["users"]:
                     if user_model and username != user_model.username:
@@ -848,7 +847,7 @@ class AuthorizationService:
                         "group_identifier": group_identifier,
                     }
                     user_to_group_identifiers.append(user_to_group_dict)
-                    GroupService.add_user_to_group_or_add_to_waiting(username, group_identifier)
+                    UserService.add_user_to_group_or_add_to_waiting(username, group_identifier)
                     unique_user_group_identifiers.add(group_identifier)
         for group in group_permissions:
             group_identifier = group["name"]

spiffworkflow-backend/src/spiffworkflow_backend/services/group_service.py

@@ -1,63 +0,0 @@
-from spiffworkflow_backend.models.db import db
-from spiffworkflow_backend.models.group import SPIFF_GUEST_GROUP
-from spiffworkflow_backend.models.group import GroupModel
-from spiffworkflow_backend.models.user import SPIFF_GUEST_USER
-from spiffworkflow_backend.models.user import UserModel
-from spiffworkflow_backend.models.user_group_assignment import UserGroupAssignmentModel
-from spiffworkflow_backend.models.user_group_assignment import UserGroupAssignmentNotFoundError
-from spiffworkflow_backend.services.user_service import UserService
-from sqlalchemy import and_
-
-
-class GroupService:
-    @classmethod
-    def find_or_create_group(cls, group_identifier: str) -> GroupModel:
-        group: GroupModel | None = GroupModel.query.filter_by(identifier=group_identifier).first()
-        if group is None:
-            group = GroupModel(identifier=group_identifier)
-            db.session.add(group)
-            db.session.commit()
-            UserService.create_principal(group.id, id_column_name="group_id")
-        return group
-
-    @classmethod
-    def add_user_to_group_or_add_to_waiting(cls, username: str | UserModel, group_identifier: str) -> None:
-        group = cls.find_or_create_group(group_identifier)
-        user = UserModel.query.filter_by(username=username).first()
-        if user:
-            UserService.add_user_to_group(user, group)
-        else:
-            UserService.add_waiting_group_assignment(username, group)
-
-    @classmethod
-    def add_user_to_group(cls, user: UserModel, group_identifier: str) -> None:
-        group = cls.find_or_create_group(group_identifier)
-        UserService.add_user_to_group(user, group)
-
-    @classmethod
-    def remove_user_from_group(cls, user: UserModel, group_identifier: str) -> None:
-        user_group_assignment = (
-            UserGroupAssignmentModel.query.filter_by(user_id=user.id)
-            .join(
-                GroupModel,
-                and_(GroupModel.id == UserGroupAssignmentModel.group_id, GroupModel.identifier == group_identifier),
-            )
-            .first()
-        )
-        if user_group_assignment is None:
-            raise (UserGroupAssignmentNotFoundError(f"User ({user.username}) is not in group ({group_identifier})"))
-        db.session.delete(user_group_assignment)
-        db.session.commit()
-
-    @classmethod
-    def find_or_create_guest_user(
-        cls, username: str = SPIFF_GUEST_USER, group_identifier: str = SPIFF_GUEST_GROUP
-    ) -> UserModel:
-        guest_user: UserModel | None = UserModel.query.filter_by(
-            username=username, service="spiff_guest_service", service_id="spiff_guest_service_id"
-        ).first()
-        if guest_user is None:
-            guest_user = UserService.create_user(username, "spiff_guest_service", "spiff_guest_service_id")
-            GroupService.add_user_to_group_or_add_to_waiting(guest_user.username, group_identifier)
-
-        return guest_user

spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py

@@ -75,7 +75,6 @@ from spiffworkflow_backend.scripts.script import Script
 from spiffworkflow_backend.services.custom_parser import MyCustomParser
 from spiffworkflow_backend.services.element_units_service import ElementUnitsService
 from spiffworkflow_backend.services.file_system_service import FileSystemService
-from spiffworkflow_backend.services.group_service import GroupService
 from spiffworkflow_backend.services.jinja_service import JinjaHelpers
 from spiffworkflow_backend.services.process_instance_queue_service import ProcessInstanceQueueService
 from spiffworkflow_backend.services.process_instance_tmp_service import ProcessInstanceTmpService
@@ -815,7 +814,7 @@ class ProcessInstanceProcessor:
         lane_assignment_id = None
 
         if "allowGuest" in task.task_spec.extensions and task.task_spec.extensions["allowGuest"] == "true":
-            guest_user = GroupService.find_or_create_guest_user()
+            guest_user = UserService.find_or_create_guest_user()
             potential_owner_ids = [guest_user.id]
         elif re.match(r"(process.?)initiator", task_lane, re.IGNORECASE):
             potential_owner_ids = [self.process_instance_model.process_initiator_id]

spiffworkflow-backend/src/spiffworkflow_backend/services/task_service.py

@@ -594,7 +594,7 @@ class TaskService:
         cls, task_model: TaskModel, create_if_not_exists: bool = False
     ) -> TaskDraftDataModel | None:
         full_bpmn_process_id_path = cls.full_bpmn_process_path(task_model.bpmn_process, "id")
-        task_definition_id_path = f"{':'.join(map(str,full_bpmn_process_id_path))}:{task_model.task_definition_id}"
+        task_definition_id_path = f"{':'.join(map(str, full_bpmn_process_id_path))}:{task_model.task_definition_id}"
         task_draft_data: TaskDraftDataModel | None = TaskDraftDataModel.query.filter_by(
             process_instance_id=task_model.process_instance_id, task_definition_id_path=task_definition_id_path
         ).first()

spiffworkflow-backend/src/spiffworkflow_backend/services/user_service.py

@@ -4,14 +4,18 @@ from flask import current_app
 from flask import g
 from spiffworkflow_backend.exceptions.api_error import ApiError
 from spiffworkflow_backend.models.db import db
+from spiffworkflow_backend.models.group import SPIFF_GUEST_GROUP
 from spiffworkflow_backend.models.group import GroupModel
 from spiffworkflow_backend.models.human_task import HumanTaskModel
 from spiffworkflow_backend.models.human_task_user import HumanTaskUserModel
 from spiffworkflow_backend.models.principal import MissingPrincipalError
 from spiffworkflow_backend.models.principal import PrincipalModel
+from spiffworkflow_backend.models.user import SPIFF_GUEST_USER
 from spiffworkflow_backend.models.user import UserModel
 from spiffworkflow_backend.models.user_group_assignment import UserGroupAssignmentModel
+from spiffworkflow_backend.models.user_group_assignment import UserGroupAssignmentNotFoundError
 from spiffworkflow_backend.models.user_group_assignment_waiting import UserGroupAssignmentWaitingModel
+from sqlalchemy import and_
 
 
 class UserService:
@@ -57,7 +61,7 @@ class UserService:
                     message=f"Could not add user {username}",
                 ) from e
             cls.create_principal(user_model.id)
-            UserService().apply_waiting_group_assignments(user_model)
+            cls.apply_waiting_group_assignments(user_model)
             return user_model
 
         else:
@@ -77,9 +81,9 @@ class UserService:
     def has_user() -> bool:
         return hasattr(g, "authenticated") and g.authenticated is True and "user" in g and bool(g.user)
 
-    @staticmethod
+    @classmethod
-    def current_user() -> Any:
+    def current_user(cls) -> Any:
-        if not UserService.has_user():
+        if not cls.has_user():
             raise ApiError("logged_out", "You are no longer logged in.", status_code=401)
         return g.user
 
@@ -114,7 +118,7 @@ class UserService:
 
     @classmethod
     def add_user_to_group(cls, user: UserModel, group: GroupModel) -> None:
-        exists = UserGroupAssignmentModel().query.filter_by(user_id=user.id).filter_by(group_id=group.id).count()
+        exists = UserGroupAssignmentModel.query.filter_by(user_id=user.id).filter_by(group_id=group.id).count()
         if not exists:
             ugam = UserGroupAssignmentModel(user_id=user.id, group_id=group.id)
             db.session.add(ugam)
@@ -129,6 +133,8 @@ class UserService:
             wugam = UserGroupAssignmentWaitingModel(username=username, group_id=group.id)
             db.session.add(wugam)
             db.session.commit()
+
+        # to handle people who are already signed in
         if wugam.is_match_all():
             for user in UserModel.query.all():
                 cls.add_user_to_group(user, group)
@@ -208,3 +214,55 @@ class UserService:
             principals.append(group.principal)
 
         return principals
+
+    @classmethod
+    def find_or_create_group(cls, group_identifier: str) -> GroupModel:
+        group: GroupModel | None = GroupModel.query.filter_by(identifier=group_identifier).first()
+        if group is None:
+            group = GroupModel(identifier=group_identifier)
+            db.session.add(group)
+            db.session.commit()
+            cls.create_principal(group.id, id_column_name="group_id")
+        return group
+
+    @classmethod
+    def add_user_to_group_or_add_to_waiting(cls, username: str | UserModel, group_identifier: str) -> None:
+        group = cls.find_or_create_group(group_identifier)
+        user = UserModel.query.filter_by(username=username).first()
+        if user:
+            cls.add_user_to_group(user, group)
+        else:
+            cls.add_waiting_group_assignment(username, group)
+
+    @classmethod
+    def add_user_to_group_by_group_identifier(cls, user: UserModel, group_identifier: str) -> None:
+        group = cls.find_or_create_group(group_identifier)
+        cls.add_user_to_group(user, group)
+
+    @classmethod
+    def remove_user_from_group(cls, user: UserModel, group_identifier: str) -> None:
+        user_group_assignment = (
+            UserGroupAssignmentModel.query.filter_by(user_id=user.id)
+            .join(
+                GroupModel,
+                and_(GroupModel.id == UserGroupAssignmentModel.group_id, GroupModel.identifier == group_identifier),
+            )
+            .first()
+        )
+        if user_group_assignment is None:
+            raise (UserGroupAssignmentNotFoundError(f"User ({user.username}) is not in group ({group_identifier})"))
+        db.session.delete(user_group_assignment)
+        db.session.commit()
+
+    @classmethod
+    def find_or_create_guest_user(
+        cls, username: str = SPIFF_GUEST_USER, group_identifier: str = SPIFF_GUEST_GROUP
+    ) -> UserModel:
+        guest_user: UserModel | None = UserModel.query.filter_by(
+            username=username, service="spiff_guest_service", service_id="spiff_guest_service_id"
+        ).first()
+        if guest_user is None:
+            guest_user = cls.create_user(username, "spiff_guest_service", "spiff_guest_service_id")
+            cls.add_user_to_group_or_add_to_waiting(guest_user.username, group_identifier)
+
+        return guest_user

spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py

@@ -26,11 +26,11 @@ from spiffworkflow_backend.models.reference_cache import ReferenceCacheModel
 from spiffworkflow_backend.models.task import TaskModel  # noqa: F401
 from spiffworkflow_backend.models.user import UserModel
 from spiffworkflow_backend.services.file_system_service import FileSystemService
-from spiffworkflow_backend.services.group_service import GroupService
 from spiffworkflow_backend.services.process_caller_service import ProcessCallerService
 from spiffworkflow_backend.services.process_instance_processor import ProcessInstanceProcessor
 from spiffworkflow_backend.services.process_instance_service import ProcessInstanceService
 from spiffworkflow_backend.services.process_model_service import ProcessModelService
+from spiffworkflow_backend.services.user_service import UserService
 
 from tests.spiffworkflow_backend.helpers.base_test import BaseTest
 from tests.spiffworkflow_backend.helpers.test_data import load_test_spec
@@ -102,9 +102,9 @@ class TestProcessApi(BaseTest):
         with_db_and_bpmn_file_cleanup: None,
     ) -> None:
         user = self.find_or_create_user()
-        group = GroupService.find_or_create_group("test_group")
+        group = UserService.find_or_create_group("test_group")
         principal = group.principal
-        GroupService.add_user_to_group(user, group.identifier)
+        UserService.add_user_to_group(user, group)
         self.add_permissions_to_principal(principal, target_uri="/v1.0/process-groups/%", permission_names=["read"])
         request_body = {
             "requests_to_check": {
@@ -1304,7 +1304,7 @@ class TestProcessApi(BaseTest):
         )
 
         assert response.json is not None
-        assert type(response.json["updated_at_in_seconds"]) is int
+        assert isinstance(response.json["updated_at_in_seconds"], int)
         assert response.json["updated_at_in_seconds"] > 0
         assert response.json["status"] == "complete"
         assert response.json["process_model_identifier"] == process_model.id
@@ -1738,9 +1738,9 @@ class TestProcessApi(BaseTest):
         assert response.json["pagination"]["total"] == 1
 
         process_instance_dict = response.json["results"][0]
-        assert type(process_instance_dict["id"]) is int
+        assert isinstance(process_instance_dict["id"], int)
         assert process_instance_dict["process_model_identifier"] == process_model.id
-        assert type(process_instance_dict["start_in_seconds"]) is int
+        assert isinstance(process_instance_dict["start_in_seconds"], int)
         assert process_instance_dict["start_in_seconds"] > 0
         assert process_instance_dict["end_in_seconds"] is None
         assert process_instance_dict["status"] == "not_started"

spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py

@@ -5,7 +5,6 @@ from spiffworkflow_backend.models.group import GroupModel
 from spiffworkflow_backend.services.authorization_service import AuthorizationService
 from spiffworkflow_backend.services.authorization_service import GroupPermissionsDict
 from spiffworkflow_backend.services.authorization_service import InvalidPermissionError
-from spiffworkflow_backend.services.group_service import GroupService
 from spiffworkflow_backend.services.process_instance_processor import ProcessInstanceProcessor
 from spiffworkflow_backend.services.process_instance_service import ProcessInstanceService
 from spiffworkflow_backend.services.user_service import UserService
@@ -341,7 +340,7 @@ class TestAuthorizationService(BaseTest):
         with_db_and_bpmn_file_cleanup: None,
     ) -> None:
         user = self.find_or_create_user(username="user_one")
-        user_group = GroupService.find_or_create_group("group_one")
+        user_group = UserService.find_or_create_group("group_one")
         UserService.add_user_to_group(user, user_group)
         AuthorizationService.add_permission_from_uri_or_macro(user_group.identifier, "read", "PG:hey")
         self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey")
@@ -376,10 +375,10 @@ class TestAuthorizationService(BaseTest):
         admin_user = self.find_or_create_user(username="testadmin1")
 
         # this group is not mentioned so it will get deleted
-        GroupService.find_or_create_group("group_two")
+        UserService.find_or_create_group("group_two")
         assert GroupModel.query.filter_by(identifier="group_two").first() is not None
 
-        GroupService.find_or_create_group("group_three")
+        UserService.find_or_create_group("group_three")
         assert GroupModel.query.filter_by(identifier="group_three").first() is not None
 
         group_info: list[GroupPermissionsDict] = [

spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_user_service.py

@@ -2,7 +2,6 @@
 from flask.app import Flask
 from flask.testing import FlaskClient
 from spiffworkflow_backend.models.user_group_assignment_waiting import UserGroupAssignmentWaitingModel
-from spiffworkflow_backend.services.group_service import GroupService
 from spiffworkflow_backend.services.user_service import UserService
 
 from tests.spiffworkflow_backend.helpers.base_test import BaseTest
@@ -15,7 +14,7 @@ class TestUserService(BaseTest):
         client: FlaskClient,
         with_db_and_bpmn_file_cleanup: None,
     ) -> None:
-        a_test_group = GroupService.find_or_create_group("aTestGroup")
+        a_test_group = UserService.find_or_create_group("aTestGroup")
         UserService.add_waiting_group_assignment("initiator_user", a_test_group)
         initiator_user = self.find_or_create_user("initiator_user")
         assert initiator_user.groups[0] == a_test_group
@@ -26,7 +25,7 @@ class TestUserService(BaseTest):
         client: FlaskClient,
         with_db_and_bpmn_file_cleanup: None,
     ) -> None:
-        everybody_group = GroupService.find_or_create_group("everybodyGroup")
+        everybody_group = UserService.find_or_create_group("everybodyGroup")
         UserService.add_waiting_group_assignment(UserGroupAssignmentWaitingModel.MATCH_ALL_USERS, everybody_group)
         initiator_user = self.find_or_create_user("initiator_user")
         assert initiator_user.groups[0] == everybody_group
@@ -38,6 +37,6 @@ class TestUserService(BaseTest):
         with_db_and_bpmn_file_cleanup: None,
     ) -> None:
         initiator_user = self.find_or_create_user("initiator_user")
-        everybody_group = GroupService.find_or_create_group("everybodyGroup")
+        everybody_group = UserService.find_or_create_group("everybodyGroup")
         UserService.add_waiting_group_assignment(UserGroupAssignmentWaitingModel.MATCH_ALL_USERS, everybody_group)
         assert initiator_user.groups[0] == everybody_group