Simple refactor and fixes in find client

portal/rest/login.py

-import logging
-
 from flask import request
 from flask_jwt_extended import create_access_token, create_refresh_token, get_jwt_identity, \
     jwt_refresh_token_required, jwt_required
 from flask_restplus import Namespace, Resource, fields
 
-from portal import jwt, logger
+from portal import logger
 from portal.database.models import Client
 from portal.service.auth import login_gitlab, login_username_password, login_secret
 from portal.service.errors import PortalAPIError, UnauthorizedError
-from portal.service.general import find_client, find_client_owner
 
 log = logger.get_logger(__name__)
 

portal/rest/roles.py

@@ -168,8 +168,8 @@ class RoleClient(Resource):
         course = general.find_course(cid)
         # authorization
         permissions.PermissionsService(course=course).require.write_roles()
-        client_type = request.args.get('type')
         role = general.find_role(course, rid)
+        client_type = request.args.get('type')
         client = general.find_client(clid, client_type=client_type)
         RoleService(role).add_client(client)
         return '', 204
@@ -180,6 +180,7 @@ class RoleClient(Resource):
         course = general.find_course(cid)
         permissions.PermissionsService(course=course).require.write_roles()
         role = general.find_role(course, rid)
-        client = general.find_client(clid)
+        client_type = request.args.get('type')
+        client = general.find_client(clid, client_type=client_type)
         RoleService(role).remove_client(client)
         return '', 204

portal/service/courses.py

@@ -94,8 +94,8 @@ class CourseService:
         """
         self.course.notes_access_token = token
         general.write_entity(self.course)
-        log.info(
-            f"[UPDATE] Notes access token ({self.course.codename}) [{self.course.id}]: {token}")
+        log.info(f"[UPDATE] Notes access token "
+                 f"({self.course.codename}) [{self.course.id}]: {token}")
         return self.course
 
     def find_all_courses(self) -> List[Course]:

portal/service/filters.py

@@ -4,7 +4,7 @@ Filters for the services
 
 import logging
 
-from portal.database.models import Course, ProjectState, User, Client
+from portal.database.models import Client, Course, ProjectState, User
 from portal.tools import time
 
 log = logging.getLogger(__name__)

portal/service/roles.py

@@ -36,18 +36,21 @@ class RoleService:
         Returns(Role): Copied role
         """
         source = self.role
-        log.info(
-            f"[COPY] Role: {self.role.id} to course {target.id} with config: {with_clients}")
-        new_name = get_new_name(source, target)
-        new_role = Role(target, codename=new_name)
-        new_role.set_permissions(**vars(source.permissions))
-        new_role.description = source.description
-        new_role.name = source.name
+        log.info(f"[COPY] Role: {self.role.id} to course {target.id} "
+                 f"with config: {with_clients}")
+        new_role = self.__copy_new_role(target)
         if with_clients == "with_users":
             for clients in source.clients:
                 clients.roles.append(new_role)
         return new_role
 
+    def __copy_new_role(self, target):
+        source = self.role
+        new_name = get_new_name(source, target)
+        new_role = Role(target, codename=new_name, description=source.description, name=source.name)
+        new_role.set_permissions(**vars(source.permissions))
+        return new_role
+
     def delete_role(self):
         """Deletes role for the DB
         """
@@ -81,27 +84,27 @@ class RoleService:
         log.info(f"[CREATE] Role for course {course.id}: {new_role}")
         return new_role
 
-    def update_permissions(self, permissions: dict) -> RolePermissions:
+    def update_permissions(self, perm: dict) -> RolePermissions:
         """Updates role permissions
         Args:
-            permissions(dict): Permissions dictionary
+            perm(dict): Permissions dictionary
 
         Returns(RolePermissions): Role Permissions instance
         """
-        self.role.set_permissions(**permissions)
+        self.role.set_permissions(**perm)
         write_entity(self.role)
         log.info(f"[UPDATE] Permissions for role {self.role.id} "
-                 f"in course {self.role.course.id}: {permissions}.")
+                 f"in course {self.role.course.id}: {perm}.")
         return self.role.permissions
 
     def find_clients(self, ids: List[str]) -> List[Client]:
         """Finds all clients based on their ids
 
-            Args:
-                ids(List[str]): List of client ids
+        Args:
+            ids(List[str]): List of client ids
 
-            Returns(List[Client]): List of clients
-            """
+        Returns(List[Client]): List of clients
+        """
         return [find_client(i) for i in ids]
 
     def update_clients_membership(self, data: dict) -> Role:
@@ -112,19 +115,27 @@ class RoleService:
         Returns(Role): Updated role
         """
         clients_old = set(self.role.clients)
-        clients_add = data.get('add')
+        clients = self.__add_users_to_role(clients_old, data)
+        clients = self.__remove_users_from_role(clients, clients_old, data)
+        self.role.clients = list(clients)
+        write_entity(self.role)
+        log.info(f"[UPDATE] Clients in Role {self.role.id}: {data}.")
+        return self.role
+
+    def __remove_users_from_role(self, clients, clients_old, data):
         clients_remove = data.get('remove')
+        if clients_remove:
+            to_remove = self.find_clients(data['remove'])
+            clients = clients_old.difference(to_remove)
+        return clients
+
+    def __add_users_to_role(self, clients_old, data):
+        clients_add = data.get('add')
         clients = set()
         if clients_add:
             to_add = self.find_clients(data['add'])
             clients = clients_old.union(to_add)
-        if clients_remove:
-            to_remove = self.find_clients(data['remove'])
-            clients = clients_old.difference(to_remove)
-        self.role.clients = list(clients)
-        write_entity(self.role)
-        log.info(f"[UPDATE] Clients in Role {self.role.id}: {data}.")
-        return self.role
+        return clients
 
     def add_client(self, client: Client):
         """Adds single client to the role
@@ -156,8 +167,8 @@ class RoleService:
             raise PortalAPIError(400, message=f"Could not remove client {client.id} "
                                               f"from role {self.role.id} in course {course.id}: "
                                               f"role does not contain client.")
-        log.info(
-            f"[REMOVE] Client {client.id} from role {self.role.id} in course {course.id}.")
+        log.info(f"[REMOVE] Client {client.id} from role "
+                 f"{self.role.id} in course {course.id}.")
         return self.role
 
     def list_roles(self, course: Course) -> List[Role]: