Baf 1097/missing order states for orders

fleet_management_api/api_impl/api_keys.py

@@ -61,7 +61,7 @@ def verify_key_and_return_key_info(
     except Exception as e:
         logger.error(f"Error while verifying key: {e}")
         return 500, "Internal server error."
-    if len(_key_db_models) == 0:
+    if not _key_db_models:
         return 401, "Invalid API key used."
     else:
         return 200, _key_db_models[0]

fleet_management_api/api_impl/controllers/car.py

@@ -124,7 +124,7 @@ def get_car(request: _ProcessedRequest, car_id: int, **kwargs) -> _Response:
         criteria={"id": lambda x: x == car_id},
         omitted_relationships=[_db_models.CarDB.orders],
     )
-    if len(db_cars) == 0:
+    if not db_cars:
         return _log_info_and_respond(
             f"Car with ID={car_id} was not found.", 404, title=_OBJ_NOT_FOUND
         )
@@ -140,8 +140,8 @@ def get_cars(request: _ProcessedRequest, **kwargs) -> _Response:  # noqa: E501
     db_cars = _db_access.get(
         request.tenants, _db_models.CarDB, omitted_relationships=[_db_models.CarDB.orders]
     )
-    cars: list[_models.Car] = list()
-    if len(db_cars) == 0:
+    cars: list[_models.Car] = []
+    if not db_cars:
         _log_info("Listing all cars: no cars found.")
     else:
         for db_car in db_cars:

fleet_management_api/api_impl/controllers/car_action.py

@@ -169,8 +169,8 @@ def check_for_invalid_car_action_state_transitions(
     states: list[CarActionState],
 ) -> dict[CarId, list[tuple[CarActionStatus, CarActionStatus]]]:
     """Return a dictionary of car ids with invalid state transitions."""
-    last_statuses: dict[CarId, CarActionStatus] = dict()
-    invalid_state_transitions: dict[CarId, list[tuple[CarActionStatus, CarActionStatus]]] = dict()
+    last_statuses: dict[CarId, CarActionStatus] = {}
+    invalid_state_transitions: dict[CarId, list[tuple[CarActionStatus, CarActionStatus]]] = {}
     for state in states:
         car_id = state.car_id
         if car_id not in last_statuses:

fleet_management_api/api_impl/controllers/order.py

@@ -63,7 +63,7 @@ def n_of_active_orders(car_id: int) -> int:
     if car_id not in _active_orders:
         response = get_car_orders(car_id)
         if response.status_code != 200:
-            _active_orders[car_id] = list()
+            _active_orders[car_id] = []
         else:
             orders: list[_models.Order] = response.body
             _active_orders[car_id] = [
@@ -80,7 +80,7 @@ def n_of_inactive_orders(car_id: int) -> int:
     if car_id not in _inactive_orders:
         response = get_car_orders(car_id)
         if response.status_code != 200:
-            _inactive_orders[car_id] = list()
+            _inactive_orders[car_id] = []
         else:
             orders: list[_models.Order] = response.body
             _inactive_orders[car_id] = [
@@ -251,8 +251,7 @@ def delete_order(request: _ProcessedRequest, car_id: int, order_id: int, **kwarg
         return _text_response(f"Order (ID={order_id})has been succesfully deleted.")
     else:
         msg = f"Order (ID={order_id}) could not be deleted. {response.body['detail']}"
-        _log_error(msg)
-        return _error(response.status_code, msg, response.body["title"])
+        return _log_warning_or_error_and_respond(msg, response.status_code, response.body["title"])
 
 
 def delete_oldest_inactive_order(car_id: int) -> _Response:
@@ -269,13 +268,17 @@ def get_order(request: _ProcessedRequest, car_id: int, order_id: int, **kwargs)
         base=_db_models.OrderDB,
         criteria={"id": lambda x: x == order_id, "car_id": lambda x: x == car_id},
     )
-    if len(order_db_models) == 0:
+    if not order_db_models:
         msg = f"Order with ID={order_id} assigned to car with ID={car_id} was not found."
         _log_info(msg)
         return _error(404, msg, _OBJ_NOT_FOUND)
     else:
         db_order = order_db_models[0]
         order = _get_order_with_last_state(request.tenants, db_order)
+        if not order:
+            msg = f"No valid order found for ID={order_id} for car with ID={car_id}."
+            _log_info(msg)
+            return _error(404, msg, _OBJ_NOT_FOUND)
         _log_info(f"Found order with ID={order_id} of car with ID={car_id}.")
         return _json_response(order)  # type: ignore
 
@@ -293,7 +296,7 @@ def get_car_orders(request: _ProcessedRequest, car_id: int, since: int = 0, **kw
         children_col_name="orders",
         criteria={"timestamp": lambda z: z >= since},
     )
-    orders: list[_models.Order] = list()
+    orders: list[_models.Order] = []
     for db_order in db_orders:
         order = _get_order_with_last_state(request.tenants, db_order)
         if order is not None:
@@ -309,7 +312,7 @@ def get_orders(request: _ProcessedRequest, since: int = 0, **kwargs) -> _Respons
     db_orders = _db_access.get(
         request.tenants, _db_models.OrderDB, criteria={"timestamp": lambda x: x >= since}
     )
-    orders: list[_models.Order] = list()
+    orders: list[_models.Order] = []
     for db_order in db_orders:
         order = _get_order_with_last_state(request.tenants, db_order)
         if order is not None:
@@ -326,6 +329,9 @@ def _get_order_with_last_state(
     order_db_model: _db_models.OrderDB,
 ) -> _models.Order | None:
     last_state = _get_last_order_state(tenants, order_db_model)
+    if not last_state:
+        _log_info(f"Order with ID={order_db_model.id} has no last state. Skipping.")
+        return None
     order = _obj_to_db.order_from_db_model(order_db_model=order_db_model, last_state=last_state)
     return order
 

fleet_management_api/api_impl/controllers/order_state.py

@@ -212,7 +212,7 @@ def _existing_orders(
     tenants: _AccessibleTenants, *order_ids: int
 ) -> dict[int, _db_models.OrderDB | None]:
     order_ids = tuple(dict.fromkeys(order_ids).keys())
-    orders: dict[int, _db_models.OrderDB | None] = dict()
+    orders: dict[int, _db_models.OrderDB | None] = {}
 
     for id_ in order_ids:
         orders_with_id = _db_access.get(
@@ -267,7 +267,7 @@ def _remove_old_states(tenants: _AccessibleTenants, order_id: int) -> _Response:
 def _trim_states_after_done_or_canceled(
     states: list[_models.OrderState],
 ) -> list[_models.OrderState]:
-    done_or_canceled_states: dict[OrderId, _models.OrderState] = dict()
+    done_or_canceled_states: dict[OrderId, _models.OrderState] = {}
     filtered_states: list[_models.OrderState] = []
     received_states = states.copy()
     for state in received_states:

fleet_management_api/api_impl/controllers/platform_hw.py

@@ -63,7 +63,7 @@ def get_hw(request: _ProcessedRequest, platform_hw_id: int, **kwargs) -> _Respon
         request.tenants, _db_models.PlatformHWDB, criteria={"id": lambda x: x == platform_hw_id}
     )
     hws = [_obj_to_db.hw_from_db_model(hw_id_model) for hw_id_model in hw_models]
-    if len(hws) == 0:
+    if not hws:
         return _log_info_and_respond(
             f"Platform HW with ID={platform_hw_id} was not found.",
             404,

fleet_management_api/api_impl/controllers/route.py

@@ -98,7 +98,7 @@ def get_route(request: _ProcessedRequest, route_id: int, **kwargs) -> _Route:
         request.tenants, _RouteDB, criteria={"id": lambda x: x == route_id}
     )
     routes = [_obj_to_db.route_from_db_model(route_db_model) for route_db_model in route_db_models]
-    if len(routes) == 0:
+    if not routes:
         return _log_info_and_respond(
             f"Route with ID={route_id} was not found.", 404, title=_OBJ_NOT_FOUND
         )
@@ -183,7 +183,7 @@ def check_id(x: int, id_: int) -> bool:
                 return x == id_
 
             _db_access.exists(tenants, _StopDB, criteria={"id": partial(check_id, id_=id_)})
-        existing_ids = set([stop_id.id for stop_id in _db_access.get(tenants, _StopDB)])
+        existing_ids = {stop_id.id for stop_id in _db_access.get(tenants, _StopDB)}
         nonexistent_stop_ids = checked_id_set.difference(existing_ids)
         if nonexistent_stop_ids:
             return _error(

fleet_management_api/api_impl/controllers/route_visualization.py

@@ -28,7 +28,7 @@ def get_route_visualization(request: _ProcessedRequest, route_id: int, **kwargs)
     rp_db_models = _db_access.get(
         request.tenants, _RouteVisDB, criteria={"route_id": lambda x: x == route_id}
     )
-    if len(rp_db_models) == 0:
+    if not rp_db_models:
         return _error(
             404,
             f"Route visualization (route ID={route_id}) was not found.",

fleet_management_api/api_impl/controllers/stop.py

@@ -82,7 +82,7 @@ def get_stop(request: _ProcessedRequest, stop_id: int, **kwargs) -> _Response:
         request.tenants, _db_models.StopDB, criteria={"id": lambda x: x == stop_id}
     )
     stops = [_obj_to_db.stop_from_db_model(stop_db_model) for stop_db_model in stop_db_models]
-    if len(stops) == 0:
+    if not stops:
         return _log_info_and_respond(f"Stop (ID={stop_id}) not found.", 404, title=_OBJ_NOT_FOUND)
     else:
         _log_info(f"Found {len(stops)} stop with ID={stop_id}")
@@ -131,7 +131,7 @@ def _get_routes_referencing_stop(tenants: _AccessibleTenants, stop_id: int) -> _
     route_db_models = [
         m for m in _db_access.get(tenants, _db_models.RouteDB) if stop_id in m.stop_ids
     ]
-    if len(route_db_models) > 0:
+    if route_db_models:
         return _log_info_and_respond(
             f"Stop with ID={stop_id} cannot be deleted because it is referenced by {len(route_db_models)} route(s).",
             400,

fleet_management_api/database/db_access.py

@@ -263,7 +263,11 @@ def add(
 
 @db_access_method
 def delete(tenants: Tenants, base: type[_Base], id_: Any) -> _Response:
-    """Delete a single object with `id_` from the database table correspoding to the mapped class `base`."""
+    """
+    Delete a single object with `id_` from the database table correspoding to the mapped class `base`.
+
+    There is only a single transaction for deleting the entity and its dependent entities.
+    """
     source = _get_current_connection_source()
     if base.owned_by_tenant():
         response = _check_and_set_tenant(tenants, base(id=id_))

fleet_management_api/database/wait.py

@@ -19,7 +19,7 @@ def __init__(self, timeout_ms: int = _class_default_timeout_ms) -> None:
         """
         WaitObjManager._check_nonnegative_timeout(timeout_ms)
         self._timeout_ms = timeout_ms
-        self._wait_dict: dict[str, list[WaitObject]] = dict()
+        self._wait_dict: dict[str, list[WaitObject]] = {}
 
     @property
     def timeout_ms(self) -> int:
@@ -66,7 +66,7 @@ def _new_wait_obj(
         if timeout_ms is None or timeout_ms < 0:
             timeout_ms = self._timeout_ms
         if key not in self._wait_dict:
-            self._wait_dict[key] = list()
+            self._wait_dict[key] = []
         wait_obj = WaitObject(timeout_ms, validation)
         self._wait_dict[key].append(wait_obj)
         return wait_obj
@@ -104,7 +104,7 @@ def __init__(
         - If `validation` is set, the WaitObject will only accept the data that passes the validation.
         - If `timeout_ms` is set to 0, the WaitObject will respond immediatelly.
         """
-        self._response_content: list[Any] = list()
+        self._response_content: list[Any] = []
         self._wait_condition = _threading.Condition()
         self._is_valid = validation
         self._timeout_ms = max(timeout_ms, 0)

fleet_management_api/openapi/openapi.yaml

@@ -9,7 +9,7 @@ info:
     name: AGPLv3
     url: https://www.gnu.org/licenses/agpl-3.0.en.html
   title: BringAuto Fleet Management v2 API
-  version: 4.1.2
+  version: 4.1.3
 servers:
 - url: /v2/management
 security:

openapi/openapi.yaml

@@ -2,7 +2,7 @@ openapi: 3.0.0
 info:
   title: BringAuto Fleet Management v2 API
   description: Specification for BringAuto fleet backend HTTP API
-  version: 4.1.2
+  version: 4.1.3
   contact:
     name: BringAuto s.r.o
     url: https://bringauto.com

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "fleet_management_api"
-version = "4.1.2"
+version = "4.1.3"
 
 [tool.setuptools.packages.find]
 include = ["fleet_management_api", "openapi", "config"]

tests/_utils/setup_utils.py

@@ -15,7 +15,7 @@ def unrestricted(self) -> bool:
         return not self.current and not self.all
 
     def is_accessible(self, tenant_name: str) -> bool:
-        return len(self.all) == 0 or tenant_name in self.all
+        return not self.all or tenant_name in self.all
 
 
 def create_platform_hws(

tests/controllers/car/test_car_action_controller.py

@@ -116,7 +116,7 @@ def test_no_consecutive_states_with_the_same_status_yield_unchanged_list(self):
             CarActionState(car_id=4, action_status=CarActionStatus.PAUSED),
         ]
         invalid_transitions = check_for_invalid_car_action_state_transitions(states)
-        self.assertDictEqual(invalid_transitions, dict())
+        self.assertDictEqual(invalid_transitions, {})
 
     def test_consecutive_states_with_identical_status_are_merged(self):
         states = [

tests/controllers/order/test_get_order_state.py

@@ -399,7 +399,7 @@ def setUp(self, *args) -> None:
         create_stops(app, 1)
         create_route(app, stop_ids=(1,))
 
-    def test_last_order_state_is_none_if_all_order_states_have_been_deleted(self):
+    def test_order_is_not_deleted_if_all_order_states_have_been_deleted(self):
         car = Car(name="Test Car", platform_hw_id=1, car_admin_phone=MobilePhone(phone="123456789"))
         app = _app.get_test_app(use_previous=True)
         with app.app.test_client() as c:
@@ -413,8 +413,7 @@ def test_last_order_state_is_none_if_all_order_states_have_been_deleted(self):
             # there are now no order states for order with ID=1
             self.assertEqual(response.json, [])
             response = c.get("/v2/management/order/1/1")
-            self.assertEqual(response.status_code, 200)
-            self.assertIsNone(Order.from_dict(response.json).last_state)
+            self.assertEqual(response.status_code, 404)
 
 
 if __name__ == "__main__":

tests/controllers/order/test_order_controller.py

@@ -1,6 +1,7 @@
 import unittest
 from unittest.mock import patch, Mock
 import os
+import threading
 
 import fleet_management_api.database.connection as _connection
 from fleet_management_api.models import Order, Car, MobilePhone, OrderState, OrderStatus
@@ -327,6 +328,43 @@ def test_deleting_nonexistent_order_yields_code_404(self):
             response = c.delete(f"/v2/management/order/1/{nonexistent_order_id}")
             self.assertEqual(response.status_code, 404)
 
+    def test_accessing_order_during_deletion_of_some_always_only_returns_orders_with_defined_last_state(
+        self,
+    ):
+        N_TO_TRY = 100
+        self.n_failed = 0
+        self.n_tried = 0
+        with self.app.app.test_client(TEST_TENANT_NAME) as c:
+
+            def check_states():
+                self.n_tried += 1
+                response = c.get("/v2/management/order")
+                self.assertEqual(response.status_code, 200)
+
+                for order in response.json:
+                    assert isinstance(order, dict)
+                    if "lastState" not in order.keys():
+                        self.n_failed += 1
+
+            for _ in range(N_TO_TRY):
+                response = c.post("/v2/management/order", json=[self.order])
+                self.assertEqual(response.status_code, 200)
+                self.assertEqual(len(response.json), 1)
+                order_id = response.json[0]["id"]
+                t1 = threading.Thread(target=c.delete, args=(f"/v2/management/order/1/{order_id}",))
+                t2 = threading.Thread(target=check_states)
+
+                t1.start()
+                t2.start()
+                t1.join()
+                t2.join()
+
+            if self.n_failed:
+                self.fail(
+                    "Some orders being deleted were accessible by GET method without having last status. "
+                    f"Number of such orders: {self.n_failed} out of {self.n_tried}."
+                )
+
     def tearDown(self) -> None:  # pragma: no cover
         if os.path.isfile("test.db"):
             os.remove("test.db")

tests/controllers/order/test_order_state_controller.py

@@ -393,7 +393,7 @@ def test_sending_single_order_state_after_DONE_status_has_been_received_yield_40
             response = c.get("/v2/management/orderstate/1")
             self.assertEqual(response.json[-1].get("status"), OrderStatus.DONE)
 
-    def test_sending_single_order_state_after_CANCELED_status_has_been_received_yield_403_code(
+    def test_sending_single_order_state_after_CANCELED_status_has_been_received_yields_403_code(
         self,
     ):
         canceled_state = OrderState(status=OrderStatus.CANCELED, order_id=1)
@@ -436,6 +436,40 @@ def test_sending_large_number_of_order_states_before_done_state(self):
             response = c.post("/v2/management/orderstate", json=[some_state])
             self.assertEqual(response.status_code, 403)
 
+    def test_receiving_states_after_final_state_does_not_trigger_autodeletion_of_oldest_states(
+        self,
+    ):
+        # set up the database
+        max_n = 3
+        _db_models.OrderStateDB.set_max_n_of_stored_states(max_n)
+
+        # push the done state, check there is the default and final state only
+        done_state = OrderState(status=OrderStatus.DONE, order_id=1)
+        with self.app.app.test_client(TEST_TENANT_NAME) as c:
+            response = c.post("/v2/management/orderstate", json=[done_state])
+            self.assertEqual(response.status_code, 200)
+
+            response = c.get("/v2/management/orderstate/1")
+            self.assertEqual(
+                [state["status"] for state in response.json],
+                [OrderStatus.TO_ACCEPT, OrderStatus.DONE],
+            )
+
+        # push more states
+        some_state = OrderState(status=OrderStatus.IN_PROGRESS, order_id=1)
+        with self.app.app.test_client(TEST_TENANT_NAME) as c:
+            for _ in range(max_n):
+                response = c.post("/v2/management/orderstate", json=[some_state])
+                self.assertEqual(response.status_code, 403)  # should be rejected
+
+        # check the states did not change
+        with self.app.app.test_client(TEST_TENANT_NAME) as c:
+            response = c.get("/v2/management/orderstate/1")
+            self.assertEqual(
+                [state["status"] for state in response.json],
+                [OrderStatus.TO_ACCEPT, OrderStatus.DONE],
+            )
+
     def tearDown(self) -> None:  # pragma: no cover
         if os.path.isfile("test.db"):
             os.remove("test.db")