From 45ff7781122502827a7004b5bcb850f7611c4789 Mon Sep 17 00:00:00 2001 From: jableader Date: Sun, 19 Oct 2025 13:51:27 +1100 Subject: [PATCH] Move inline API request/response models out of main.py into feature modules, Add enums/constants for participant roles --- api/meals.py | 42 +++------------------------- api/products.py | 34 +++++++++++++++++++++++ main.py | 30 ++------------------ meals/__init__.py | 2 ++ meals/db.py | 25 +++++++++-------- meals/roles.py | 6 ++++ meals/service.py | 54 ++++++++++++++++++++++++++++++++++++ refactor-project-strategy.md | 17 +++++++----- tests/test_main.py | 20 +++++++------ 9 files changed, 137 insertions(+), 93 deletions(-) create mode 100644 api/products.py create mode 100644 meals/roles.py create mode 100644 meals/service.py diff --git a/api/meals.py b/api/meals.py index f82f40b..8c67310 100644 --- a/api/meals.py +++ b/api/meals.py @@ -136,43 +136,9 @@ async def delete_meal( return meal -def get_duplicates(items: List[meals.Person]) -> set[str]: - seen: set[int] = set() - duplicates: set[str] = set() - for item in items: - if item.id in seen: - duplicates.add(item.name) - seen.add(item.id) - return duplicates - - def validate_meal(meal: meals.Meal, request: Optional[Request] = None) -> Optional[Request]: - if not meal.chefs: - return error_response(request, 400, "Meal must have at least one chef") - - if not meal.cleanup: - return error_response(request, 400, "Meal must have at least one cleanup person") - - if not meal.consumers: - return error_response(request, 400, "Meal must have at least one consumer") - - if len(meal.recipes) == 0 and len(meal.extra_ingredients) == 0: - return error_response(request, 400, "Meal must have at least one recipe or ingredient") - - duplicates = get_duplicates(meal.chefs) - if duplicates: - return error_response(request, 400, f'Duplicate chef: {", ".join(duplicates)}') - - duplicates = get_duplicates(meal.cleanup) - if duplicates: - return error_response(request, 400, f'Duplicate cleanup person: {", ".join(duplicates)}') - - duplicates = get_duplicates(meal.consumers) - if duplicates: - return error_response(request, 400, f'Duplicate consumer: {", ".join(duplicates)}') - - zero_servings = [r for r in meal.recipes if r.servings == 0] - if zero_servings: - return error_response(request, 400, "Recipe servings must be greater than 0") - + """HTTP-friendly wrapper that maps service validation to ProblemDetails.""" + msg = meals.validate_meal(meal) + if msg: + return error_response(request, 400, msg) return None diff --git a/api/products.py b/api/products.py new file mode 100644 index 0000000..36ec04e --- /dev/null +++ b/api/products.py @@ -0,0 +1,34 @@ +from __future__ import annotations + +from typing import List, Optional + +import aiosqlite +from fastapi import APIRouter, Depends, Response +from pydantic import Field + +import products +from common import ApiModel +from api.deps import get_db + + +router = APIRouter(prefix="/products", tags=["products"]) + + +class ProductUrl(ApiModel): + url: str + tags: List[str] = Field(default_factory=list) + + +@router.post( + "", + operation_id="createProduct", + summary="Create or fetch a product from a URL", + response_model=products.Product, +) +async def create_product( + url: ProductUrl, response: Response, conn: aiosqlite.Connection = Depends(get_db) +) -> Optional[products.Product]: + product = await products.get_or_create(conn, url.url, url.tags) + if product: + response.headers["Location"] = f"/api/v1/products/{product.id}" + return product diff --git a/main.py b/main.py index 367b38e..b80a0a3 100644 --- a/main.py +++ b/main.py @@ -160,25 +160,7 @@ def _extend_openapi_with_problem_responses(app: FastAPI) -> None: _extend_openapi_with_problem_responses(app) -class ProductUrl(ApiModel): - url: str - tags: List[str] = Field(default_factory=list) - - -@api_v1.post( - "/products", - operation_id="createProduct", - tags=["products"], - summary="Create or fetch a product from a URL", - response_model=products.Product, -) -async def create_product( - url: ProductUrl, response: Response, conn: aiosqlite.Connection = Depends(get_db) -) -> Optional[products.Product]: - product = await products.get_or_create(conn, url.url, url.tags) - if product: - response.headers["Location"] = f"/api/v1/products/{product.id}" - return product +from api import products as products_router # type: ignore @@ -188,7 +170,6 @@ from api import recipes as recipes_router # type: ignore from api import meals as meals_router # type: ignore -from api.meals import validate_meal as _validate_meal, get_duplicates as _get_duplicates from api import shopping as shopping_router # type: ignore from api import persons as persons_router # type: ignore from api import auth as auth_router # type: ignore @@ -252,6 +233,7 @@ async def request_validation_exc_handler(request: Request, exc: RequestValidatio # Mount versioned API router app.include_router(api_v1, prefix="/api/v1", tags=["v1"]) +app.include_router(products_router.router, prefix="/api/v1", tags=["v1"]) # extracted app.include_router(recipes_router.router, prefix="/api/v1", tags=["v1"]) # extracted app.include_router(meals_router.router, prefix="/api/v1", tags=["v1"]) # extracted app.include_router(shopping_router.router, prefix="/api/v1", tags=["v1"]) # extracted @@ -263,13 +245,7 @@ app.include_router(auth_router.router, prefix="/api/v1", tags=["v1"]) # extract async def healthz(): return {"status": "ok"} -# Backward-compatibility: expose helper functions expected by tests in main -def get_duplicates(items): - return _get_duplicates(items) - - -def validate_meal(meal, request: Request | None = None): - return _validate_meal(meal, request) +# Back-compat shims removed; tests should import helpers from feature modules if settings.prod: diff --git a/meals/__init__.py b/meals/__init__.py index 20e9086..8a997e4 100644 --- a/meals/__init__.py +++ b/meals/__init__.py @@ -20,3 +20,5 @@ from meals.db import ( update_meal as update_meal, ) from persons import Person as Person +from meals.service import get_duplicates as get_duplicates, validate_meal as validate_meal +from meals.roles import ROLE_CHEF as ROLE_CHEF, ROLE_CLEANUP as ROLE_CLEANUP, ROLE_CONSUMER as ROLE_CONSUMER diff --git a/meals/db.py b/meals/db.py index 61deea4..d1facf7 100644 --- a/meals/db.py +++ b/meals/db.py @@ -6,6 +6,7 @@ from common import ApiModel import persons from persons import get_by_ids as persons_get_by_ids +from .roles import ROLE_CHEF, ROLE_CLEANUP, ROLE_CONSUMER from ingredients import ( Ingredient, delete_ingredients_by_meal_id, @@ -132,9 +133,9 @@ async def insert_meal(conn, meal: Meal): ) as cursor: meal.id = cursor.lastrowid - await sync_meal_participants(conn, meal.id, meal.chefs, "chef") - await sync_meal_participants(conn, meal.id, meal.cleanup, "cleanup") - await sync_meal_participants(conn, meal.id, meal.consumers, "consumer") + await sync_meal_participants(conn, meal.id, meal.chefs, ROLE_CHEF) + await sync_meal_participants(conn, meal.id, meal.cleanup, ROLE_CLEANUP) + await sync_meal_participants(conn, meal.id, meal.consumers, ROLE_CONSUMER) for meal_recipe in meal.recipes: meal_recipe.meal_id = meal.id @@ -199,13 +200,13 @@ async def load_participants(conn, meal: Meal) -> None: for pid, role in links: person = people.get(pid) - if role == "chef": + if role == ROLE_CHEF: if person: meal.chefs.append(person) - elif role == "cleanup": + elif role == ROLE_CLEANUP: if person: meal.cleanup.append(person) - elif role == "consumer": + elif role == ROLE_CONSUMER: if person: meal.consumers.append(person) else: @@ -260,11 +261,11 @@ async def bulk_load_participants(conn, meals: List[Meal]) -> None: person = people.get(pid) if not person: continue - if role == "chef": + if role == ROLE_CHEF: meal.chefs.append(person) - elif role == "cleanup": + elif role == ROLE_CLEANUP: meal.cleanup.append(person) - elif role == "consumer": + elif role == ROLE_CONSUMER: meal.consumers.append(person) else: raise Exception(f"Unknown role: {role}") @@ -342,9 +343,9 @@ async def update_meal(conn, meal: Meal) -> None: (meal.suggested_date.isoformat(), meal.id), ) - await sync_meal_participants(conn, meal.id, meal.chefs, "chef") - await sync_meal_participants(conn, meal.id, meal.cleanup, "cleanup") - await sync_meal_participants(conn, meal.id, meal.consumers, "consumer") + await sync_meal_participants(conn, meal.id, meal.chefs, ROLE_CHEF) + await sync_meal_participants(conn, meal.id, meal.cleanup, ROLE_CLEANUP) + await sync_meal_participants(conn, meal.id, meal.consumers, ROLE_CONSUMER) await sync_extra_ingredients(conn, meal.id, meal.extra_ingredients) await sync_meal_recipes(conn, meal.id, meal.recipes) diff --git a/meals/roles.py b/meals/roles.py new file mode 100644 index 0000000..918342e --- /dev/null +++ b/meals/roles.py @@ -0,0 +1,6 @@ +from __future__ import annotations + +# Centralized participant role constants to avoid string duplication/typos +ROLE_CHEF = "chef" +ROLE_CLEANUP = "cleanup" +ROLE_CONSUMER = "consumer" diff --git a/meals/service.py b/meals/service.py new file mode 100644 index 0000000..1c0be01 --- /dev/null +++ b/meals/service.py @@ -0,0 +1,54 @@ +from __future__ import annotations + +from typing import List, Set + +from persons import Person +from meals.db import Meal + + +def get_duplicates(items: List[Person]) -> Set[str]: + """Return the set of duplicate person names based on repeated ids.""" + seen: set[int] = set() + duplicates: set[str] = set() + for item in items: + if item.id in seen: + duplicates.add(item.name) + seen.add(item.id) + return duplicates + + +def validate_meal(meal: Meal) -> str | None: + """Validate a Meal domain model. + + Returns: + None if valid, otherwise a human-readable error message. + """ + if not meal.chefs: + return "Meal must have at least one chef" + + if not meal.cleanup: + return "Meal must have at least one cleanup person" + + if not meal.consumers: + return "Meal must have at least one consumer" + + if len(meal.recipes) == 0 and len(meal.extra_ingredients) == 0: + return "Meal must have at least one recipe or ingredient" + + duplicates = get_duplicates(meal.chefs) + if duplicates: + return f"Duplicate chef: {', '.join(duplicates)}" + + duplicates = get_duplicates(meal.cleanup) + if duplicates: + return f"Duplicate cleanup person: {', '.join(duplicates)}" + + duplicates = get_duplicates(meal.consumers) + if duplicates: + return f"Duplicate consumer: {', '.join(duplicates)}" + + zero_servings = [r for r in meal.recipes if r.servings == 0] + if zero_servings: + return "Recipe servings must be greater than 0" + + return None diff --git a/refactor-project-strategy.md b/refactor-project-strategy.md index d9bbd45..e7e3285 100644 --- a/refactor-project-strategy.md +++ b/refactor-project-strategy.md @@ -105,13 +105,13 @@ Notes --- ## Phase 4 — Modeling and validation -- [ ] Move inline API request/response models out of main.py into feature modules - - [ ] ProductUrl - - [ ] CurrentShoppingList - - [ ] PurchasedShoppingList - - [ ] LoginBody -- [ ] Extract validation (e.g., validate_meal) into a service layer for reuse -- [ ] Add enums/constants for participant roles +- [X] Move inline API request/response models out of main.py into feature modules + - [x] ProductUrl (moved to api/products.py) + - [x] CurrentShoppingList (in api/shopping.py) + - [x] PurchasedShoppingList (in api/shopping.py) + - [x] LoginBody (in api/auth.py) +- [x] Extract validation (e.g., validate_meal) into a service layer for reuse +- [x] Add enums/constants for participant roles Acceptance criteria - Cleaner main.py; feature modules own their request/response contracts @@ -178,6 +178,9 @@ Note: We can adopt this structure gradually without moving DB code immediately; - 2025-10-19: Fixed SQLite error during test setup by creating the `MealRecipe` table before indexing it; corrected `update_meal` to call `get_meal` with explicit `(request, conn)` avoiding a Depends object leak. Full test suite now passes (100%). Batch-loading of recipe ingredients is in place; meal participant batching remains outstanding. -. - 2025-10-19: Implemented participant batch-loading (`meals.bulk_load_participants`) and updated `api/meals.get_upcoming_meals` to use it; re-ran the test suite (green). Phase 3 marked complete; Phase 4-5 next. + - 2025-10-19: Phase 4 (partial) — Extracted ProductUrl and product creation endpoint to `api/products.py`; wired new router; extracted `get_duplicates` to `meals/service.py` and re-exported via `meals.__init__`; tests still green. + - 2025-10-19: Phase 4 — Extracted `validate_meal` into `meals/service.py`, re-exported via `meals.__init__`, and updated `api/meals.validate_meal` wrapper to map to ProblemDetails. Full test suite remains green. + - 2025-10-19: Phase 4 — Added centralized role constants in `meals/roles.py` and replaced string literals in `meals/db.py`; re-exported constants via `meals.__init__`. Removed back-compat shims from `main.py` and updated tests to import helpers directly from modules. Tests remain green. --- diff --git a/tests/test_main.py b/tests/test_main.py index dbadb59..5e49c52 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -15,6 +15,8 @@ def reload_test_data(): from db import connect, create import main +from meals import get_duplicates +from api.meals import validate_meal import meals import meals.db as meals_db from meals.db import Meal, MealRecipe @@ -412,7 +414,7 @@ class TestMainHelperFunctions(unittest.TestCase): persons.Person(id=2, name="Ryan"), persons.Person(id=3, name="Ellie"), ] - duplicates = main.get_duplicates(persons_list) + duplicates = get_duplicates(persons_list) self.assertEqual(len(duplicates), 0) def test_get_duplicates_with_duplicates(self): @@ -423,7 +425,7 @@ class TestMainHelperFunctions(unittest.TestCase): persons.Person(id=1, name="Jacob"), # Duplicate persons.Person(id=3, name="Ellie"), ] - duplicates = main.get_duplicates(persons_list) + duplicates = get_duplicates(persons_list) self.assertEqual(len(duplicates), 1) self.assertIn("Jacob", duplicates) @@ -438,7 +440,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[MealRecipe(meal_id=1, recipe_id=1, servings=2.0)], extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNone(result) def test_validate_meal_no_chefs(self): @@ -452,7 +454,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[MealRecipe(meal_id=1, recipe_id=1, servings=2.0)], extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNotNone(result) self.assertEqual(result.status_code, 400) @@ -467,7 +469,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[MealRecipe(meal_id=1, recipe_id=1, servings=2.0)], extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNotNone(result) self.assertEqual(result.status_code, 400) @@ -482,7 +484,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[MealRecipe(meal_id=1, recipe_id=1, servings=2.0)], extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNotNone(result) self.assertEqual(result.status_code, 400) @@ -497,7 +499,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[], extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNotNone(result) self.assertEqual(result.status_code, 400) @@ -515,7 +517,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[MealRecipe(meal_id=1, recipe_id=1, servings=2.0)], extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNotNone(result) self.assertEqual(result.status_code, 400) @@ -530,7 +532,7 @@ class TestMainHelperFunctions(unittest.TestCase): recipes=[MealRecipe(meal_id=1, recipe_id=1, servings=0)], # Zero servings extra_ingredients=[], ) - result = main.validate_meal(meal) + result = validate_meal(meal) self.assertIsNotNone(result) self.assertEqual(result.status_code, 400)