Refactoring GET endpoints to go through service layer and updating un…

…it tests to test out the service and api layers separately.

backend/ops_api/ops/resources/cans.py

@@ -1,23 +1,21 @@
 from dataclasses import dataclass
-from typing import List, Optional, cast
+from typing import List, Optional
 
 import desert
 from flask import Response, current_app, request
 from flask_jwt_extended import jwt_required
 from sqlalchemy import select
-from sqlalchemy.orm import InstrumentedAttribute
 
 from models import OpsEventType
 from models.base import BaseModel
 from models.cans import CAN
 from ops_api.ops.auth.auth_types import Permission, PermissionType
 from ops_api.ops.auth.decorators import is_authorized
 from ops_api.ops.base_views import BaseItemAPI, BaseListAPI
-from ops_api.ops.schemas.cans import CANSchema, CreateUpdateCANRequestSchema
+from ops_api.ops.schemas.cans import CANSchema, CreateUpdateCANRequestSchema, GetCANListRequestSchema
 from ops_api.ops.services.cans import CANService
 from ops_api.ops.utils.errors import error_simulator
 from ops_api.ops.utils.events import OpsEventHandler
-from ops_api.ops.utils.query_helpers import QueryHelper
 from ops_api.ops.utils.response import make_response_with_headers
 
 
@@ -29,18 +27,13 @@ class ListAPIRequest:
 class CANItemAPI(BaseItemAPI):
     def __init__(self, model):
         super().__init__(model)
+        self.can_service = CANService()
 
     @is_authorized(PermissionType.GET, Permission.CAN)
     def get(self, id: int) -> Response:
         schema = CANSchema()
-        item = self._get_item(id)
-
-        if item:
-            response = make_response_with_headers(schema.dump(item))
-        else:
-            response = make_response_with_headers({}, 404)
-
-        return response
+        item = self.can_service.get(id)
+        return make_response_with_headers(schema.dump(item))
 
     @is_authorized(PermissionType.PATCH, Permission.CAN)
     def patch(self, id: int) -> Response:
@@ -53,8 +46,7 @@ def patch(self, id: int) -> Response:
             schema = CreateUpdateCANRequestSchema(partial=True)
             serialized_request = schema.load(request_data)
 
-            can_service = CANService()
-            updated_can = can_service.update(serialized_request, id)
+            updated_can = self.can_service.update(serialized_request, id)
             serialized_can = schema.dump(updated_can)
             meta.metadata.update({"updated_can": serialized_can})
             return make_response_with_headers(schema.dump(updated_can))
@@ -70,8 +62,7 @@ def put(self, id: int) -> Response:
             schema = CreateUpdateCANRequestSchema()
             serialized_request = schema.load(request_data)
 
-            can_service = CANService()
-            updated_can = can_service.update(serialized_request, id)
+            updated_can = self.can_service.update(serialized_request, id)
             serialized_can = schema.dump(updated_can)
             meta.metadata.update({"updated_can": serialized_can})
             return make_response_with_headers(schema.dump(updated_can))
@@ -81,46 +72,25 @@ def delete(self, id: int) -> Response:
         """
         Delete a CAN with given id."""
         with OpsEventHandler(OpsEventType.DELETE_CAN) as meta:
-            can_service = CANService()
-            can_service.delete(id)
+            self.can_service.delete(id)
             meta.metadata.update({"Deleted BudgetLineItem": id})
             return make_response_with_headers({"message": "CAN deleted", "id": id}, 200)
 
 
 class CANListAPI(BaseListAPI):
     def __init__(self, model):
         super().__init__(model)
+        self.can_service = CANService()
         self._get_input_schema = desert.schema(ListAPIRequest)
 
-    @staticmethod
-    def _get_query(search=None):
-        stmt = select(CAN).order_by(CAN.id)
-
-        query_helper = QueryHelper(stmt)
-
-        if search is not None and len(search) == 0:
-            query_helper.return_none()
-        elif search:
-            query_helper.add_search(cast(InstrumentedAttribute, CAN.number), search)
-
-        stmt = query_helper.get_stmt()
-        current_app.logger.debug(f"SQL: {stmt}")
-
-        return stmt
-
     @jwt_required()
     @error_simulator
     def get(self) -> Response:
-        errors = self._get_input_schema.validate(request.args)
+        list_schema = GetCANListRequestSchema()
+        get_request = list_schema.load(request.args)
+        result = self.can_service.get_list(**get_request)
         can_schema = CANSchema()
-
-        if errors:
-            return make_response_with_headers(errors, 400)
-
-        request_data: ListAPIRequest = self._get_input_schema.load(request.args)
-        stmt = self._get_query(request_data.search)
-        result = current_app.db_session.execute(stmt).all()
-        return make_response_with_headers([can_schema.dump(i) for item in result for i in item])
+        return make_response_with_headers([can_schema.dump(can) for can in result])
 
     @is_authorized(PermissionType.POST, Permission.CAN)
     def post(self) -> Response:
@@ -132,8 +102,7 @@ def post(self) -> Response:
             schema = CreateUpdateCANRequestSchema()
             serialized_request = schema.load(request_data)
 
-            can_service = CANService()
-            created_can = can_service.create(serialized_request)
+            created_can = self.can_service.create(serialized_request)
 
             can_schema = CANSchema()
             serialized_can = can_schema.dump(created_can)

backend/ops_api/ops/schemas/cans.py

@@ -6,6 +6,10 @@
 from ops_api.ops.schemas.users import SafeUserSchema
 
 
+class GetCANListRequestSchema(Schema):
+    search = fields.String(allow_none=True)
+
+
 class CreateUpdateCANRequestSchema(Schema):
     nick_name = fields.String(load_default=None)
     number = fields.String(required=True)

backend/ops_api/ops/services/cans.py

@@ -1,9 +1,13 @@
+from typing import cast
+
 from flask import current_app
 from sqlalchemy import select
 from sqlalchemy.exc import NoResultFound
+from sqlalchemy.orm import InstrumentedAttribute
 from werkzeug.exceptions import NotFound
 
 from models import CAN
+from ops_api.ops.utils.query_helpers import QueryHelper
 
 
 class CANService:
@@ -30,7 +34,7 @@ def create(self, create_can_request) -> CAN:
         current_app.db_session.commit()
         return new_can
 
-    def update(self, updated_fields, id) -> CAN:
+    def update(self, updated_fields, id: int) -> CAN:
         """
         Update a CAN with only the provided values in updated_fields.
         """
@@ -47,7 +51,7 @@ def update(self, updated_fields, id) -> CAN:
             current_app.logger.exception(f"Could not find a CAN with id {id}")
             raise NotFound()
 
-    def delete(self, id):
+    def delete(self, id: int):
         """
         Delete a CAN with given id. Throw a NotFound error if no CAN corresponding to that ID exists."""
         try:
@@ -57,3 +61,43 @@ def delete(self, id):
         except NoResultFound:
             current_app.logger.exception(f"Could not find a CAN with id {id}")
             raise NotFound()
+
+    def get(self, id: int) -> CAN:
+        """
+        Get an individual CAN by id.
+        """
+        stmt = select(CAN).where(CAN.id == id).order_by(CAN.id)
+        can = current_app.db_session.scalar(stmt)
+
+        if can:
+            return can
+        else:
+            current_app.logger.exception(f"Could not find a CAN with id {id}")
+            raise NotFound()
+
+    def get_list(self, search=None) -> list[CAN]:
+        """
+        Get a list of CANs, optionally filtered by a search parameter.
+        """
+        search_query = self._get_query(search)
+        results = current_app.db_session.execute(search_query).all()
+        return [can for item in results for can in item]
+
+    @staticmethod
+    def _get_query(search=None):
+        """
+        Construct a search query that can be used to retrieve a list of CANs.
+        """
+        stmt = select(CAN).order_by(CAN.id)
+
+        query_helper = QueryHelper(stmt)
+
+        if search is not None and len(search) == 0:
+            query_helper.return_none()
+        elif search:
+            query_helper.add_search(cast(InstrumentedAttribute, CAN.number), search)
+
+        stmt = query_helper.get_stmt()
+        current_app.logger.debug(f"SQL: {stmt}")
+
+        return stmt

backend/ops_api/tests/ops/can/test_can.py

@@ -81,21 +81,39 @@ def test_can_is_inactive(loaded_db, mocker):
     assert can.status == CANStatus.INACTIVE
 
 
-def test_can_get_all(auth_client, loaded_db):
-    count = loaded_db.query(CAN).count()
-
+@pytest.mark.usefixtures("app_ctx")
+def test_can_get_all(auth_client, mocker, test_can):
+    mocker_get_can = mocker.patch("ops_api.ops.services.cans.CANService.get_list")
+    mocker_get_can.return_value = [test_can]
     response = auth_client.get("/api/v1/cans/")
     assert response.status_code == 200
-    assert len(response.json) == count
+    assert len(response.json) == 1
+    mocker_get_can.assert_called_once()
+
+
+def test_service_can_get_all(auth_client, loaded_db):
+    count = loaded_db.query(CAN).count()
+    can_service = CANService()
+    response = can_service.get_list()
+    assert len(response) == count
 
 
 @pytest.mark.usefixtures("app_ctx")
-def test_can_get_by_id(auth_client, loaded_db, test_can):
+def test_can_get_by_id(auth_client, mocker, test_can):
+    mocker_get_can = mocker.patch("ops_api.ops.services.cans.CANService.get")
+    mocker_get_can.return_value = test_can
     response = auth_client.get(f"/api/v1/cans/{test_can.id}")
     assert response.status_code == 200
     assert response.json["number"] == "G99HRF2"
 
 
+def test_can_service_get_by_id(test_can):
+    service = CANService()
+    can = service.get(test_can.id)
+    assert test_can.id == can.id
+    assert test_can.number == can.number
+
+
 @pytest.mark.usefixtures("app_ctx")
 def test_can_get_portfolio_cans(auth_client, loaded_db):
     response = auth_client.get("/api/v1/cans/portfolio/1")
@@ -121,6 +139,20 @@ def test_get_cans_search_filter(auth_client, loaded_db, test_can):
     assert len(response.json) == 0
 
 
+def test_service_get_cans_search_filter(test_can):
+    can_service = CANService()
+    response = can_service.get_list("XXX8")
+    assert len(response) == 1
+    assert response[0].id == 512
+
+    response = can_service.get_list("G99HRF2")
+    assert len(response) == 1
+    assert response[0].id == test_can.id
+
+    response = can_service.get_list("")
+    assert len(response) == 0
+
+
 # Testing CAN Creation
 @pytest.mark.usefixtures("app_ctx")
 def test_can_post_creates_can(budget_team_auth_client, mocker, loaded_db):
@@ -288,7 +320,7 @@ def test_basic_user_cannot_put_cans(basic_user_auth_client):
 
 
 @pytest.mark.usefixtures("app_ctx")
-def test_can_put_404(budget_team_auth_client, mocker, loaded_db, unadded_can):
+def test_can_put_404(budget_team_auth_client):
     test_can_id = 518
     update_data = {
         "number": "G123456",
@@ -350,11 +382,11 @@ def test_service_update_can_with_nones(loaded_db):
 def test_can_delete(budget_team_auth_client, mocker, unadded_can):
     test_can_id = 517
 
-    mocker_update_can = mocker.patch("ops_api.ops.services.cans.CANService.delete")
+    mocker_delete_can = mocker.patch("ops_api.ops.services.cans.CANService.delete")
     response = budget_team_auth_client.delete(f"/api/v1/cans/{test_can_id}")
 
     assert response.status_code == 200
-    mocker_update_can.assert_called_once_with(test_can_id)
+    mocker_delete_can.assert_called_once_with(test_can_id)
     assert response.json["message"] == "CAN deleted"
     assert response.json["id"] == test_can_id