-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add benefit plan configuration module with domain-driven architecture #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…d API layers Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a new Benefit Plan configuration module (domain/application/infrastructure/API) with async SQLAlchemy persistence, soft deletes, and supporting tests/docs/examples.
Changes:
- Introduces benefit plan domain enums + Pydantic schemas (create/update/read) with validation.
- Adds async repository + service layer and a FastAPI v1 router for CRUD + soft delete.
- Adds Alembic migration for
benefit_plansplus unit/integration-style tests and documentation.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_benefit_plan_service.py | Adds unit tests for service-layer business rules and error handling. |
| tests/unit/test_benefit_plan_repository.py | Adds repository tests (unit + DB-skipped integration tests). |
| tests/unit/test_benefit_plan_domain.py | Adds domain validation/enums tests for Pydantic models. |
| src/privaseeai_security/infrastructure/persistence/repositories/benefit_plan_repository.py | Implements async CRUD repository with soft delete and org-scoped reads. |
| src/privaseeai_security/infrastructure/persistence/repositories/init.py | Package init for repositories. |
| src/privaseeai_security/infrastructure/persistence/init.py | Package init for persistence layer. |
| src/privaseeai_security/infrastructure/init.py | Package init for infrastructure layer. |
| src/privaseeai_security/domain/benefit_plans/enums.py | Defines BenefitPlanType and NetworkType enums. |
| src/privaseeai_security/domain/benefit_plans/entities.py | Defines BenefitPlan Pydantic schemas + validation rules. |
| src/privaseeai_security/domain/benefit_plans/init.py | Re-exports domain benefit plan types/models. |
| src/privaseeai_security/domain/init.py | Package init for domain layer. |
| src/privaseeai_security/database/models.py | Adds SQLAlchemy ORM model for benefit_plans (indexes, soft delete fields). |
| src/privaseeai_security/database/init.py | Exposes BenefitPlan model via database package exports. |
| src/privaseeai_security/application/benefit_plans/services.py | Implements service orchestration + business rules for benefit plans. |
| src/privaseeai_security/application/benefit_plans/init.py | Re-exports BenefitPlanService. |
| src/privaseeai_security/application/init.py | Package init for application layer. |
| src/privaseeai_security/api/v1/benefit_plans.py | Adds FastAPI router with endpoints for create/list/get/update/delete. |
| src/privaseeai_security/api/v1/init.py | Package init for API v1. |
| src/privaseeai_security/api/init.py | Package init for API layer. |
| examples/benefit_plan_example.py | Adds runnable example demonstrating create/list/update/validation/soft-delete flows. |
| docs/BENEFIT_PLANS_MODULE.md | Adds module-level documentation and API usage reference. |
| docs/BENEFIT_PLANS_IMPLEMENTATION_SUMMARY.md | Adds implementation summary and checklist-style documentation. |
| alembic/versions/002_add_benefit_plans.py | Adds DB migration to create benefit_plans table + indexes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| existing = await self.repository.find_by_payer_and_name( | ||
| payer_id=plan_data.payer_id, | ||
| name=plan_data.name, | ||
| org_id=plan_data.organization_id, | ||
| ) | ||
| if existing: | ||
| raise ValueError( | ||
| f"Benefit plan '{plan_data.name}' with payer '{plan_data.payer_id}' " | ||
| f"already exists for this organization" |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate prevention checks for an existing plan by (org_id, payer_id, name), but the database enforces uniqueness on (organization_id, name) (see database/models.py index and migration 002_add_benefit_plans). This mismatch means two plans with the same name but different payer_id will pass the service check and then fail at commit with an unhandled IntegrityError (500). Align the service/repository duplicate check with the actual uniqueness rule (either check by org+name, or change the DB unique index to include payer_id).
| existing = await self.repository.find_by_payer_and_name( | |
| payer_id=plan_data.payer_id, | |
| name=plan_data.name, | |
| org_id=plan_data.organization_id, | |
| ) | |
| if existing: | |
| raise ValueError( | |
| f"Benefit plan '{plan_data.name}' with payer '{plan_data.payer_id}' " | |
| f"already exists for this organization" | |
| # Align with DB uniqueness on (organization_id, name). Prefer a repository | |
| # method that checks by org+name only, but fall back to the existing | |
| # payer-aware method if needed for backward compatibility. | |
| find_by_org_and_name = getattr(self.repository, "find_by_org_and_name", None) | |
| if callable(find_by_org_and_name): | |
| existing = await find_by_org_and_name( | |
| org_id=plan_data.organization_id, | |
| name=plan_data.name, | |
| ) | |
| else: | |
| existing = await self.repository.find_by_payer_and_name( | |
| payer_id=plan_data.payer_id, | |
| name=plan_data.name, | |
| org_id=plan_data.organization_id, | |
| ) | |
| if existing: | |
| raise ValueError( | |
| f"Benefit plan '{plan_data.name}' already exists for this organization" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Changed service to use new find_by_org_and_name() method that aligns with DB uniqueness constraint on (organization_id, name). This prevents IntegrityError 500s when duplicate names exist.
| def is_database_available(): | ||
| """Check if a test database is configured and available.""" | ||
| db_url = os.getenv("DATABASE_URL", "") | ||
| # Skip integration tests if no DATABASE_URL is set or if it's the default placeholder | ||
| if not db_url or "localhost:5432" in db_url: | ||
| return False | ||
| return True |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_database_available() treats any DATABASE_URL containing localhost:5432 as “not available”, which will skip integration tests even when a developer/CI is intentionally using a local Postgres instance. Consider skipping only when DATABASE_URL is unset or exactly equals the project’s default placeholder URL (or provide an explicit opt-in env var for DB tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Changed condition to only skip when DATABASE_URL is unset or empty, allowing local postgres testing with localhost URLs.
|
|
||
| from datetime import date | ||
| from decimal import Decimal | ||
| from unittest.mock import AsyncMock, MagicMock |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import: MagicMock is imported but not referenced in this test module. Removing it avoids lint noise and keeps test dependencies minimal.
| from unittest.mock import AsyncMock, MagicMock | |
| from unittest.mock import AsyncMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Removed unused MagicMock import.
| async def get_current_organization() -> UUID: | ||
| """ | ||
| Get current organization context from request. | ||
|
|
||
| This is a stub implementation. In production, this would: | ||
| - Extract organization from JWT token claims | ||
| - Verify user has access to the organization | ||
| - Return the organization UUID | ||
|
|
||
| For testing purposes, returns a fixed UUID. | ||
| """ | ||
| # TODO: Implement proper authentication and organization context | ||
| # Example: org_id = request.state.user.organization_id | ||
| from uuid import uuid4 | ||
|
|
||
| return uuid4() # Placeholder |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_current_organization() docstring says it returns a fixed UUID for testing, but it currently returns a new uuid4() on every call. That makes org-scoped endpoints inconsistent across requests (e.g., create/list/get won’t line up) and can break local manual testing. Consider returning a stable value (config/env-driven) or requiring the org id from auth context explicitly in this stub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Changed to return a stable test UUID (00000000-0000-0000-0000-000000000001) for consistent behavior across requests in local testing.
| # Validate date range if both are being updated or one is updated | ||
| update_dict = update_data.model_dump(exclude_unset=True) | ||
| effective = update_dict.get("effective_date", existing.effective_date) | ||
| termination = update_dict.get("termination_date", existing.termination_date) | ||
|
|
||
| if termination and effective and effective > termination: | ||
| raise ValueError("effective_date must be on or before termination_date") | ||
|
|
||
| # Perform update | ||
| updated_plan = await self.repository.update(plan_id, update_data) | ||
| if updated_plan: | ||
| return BenefitPlanRead.model_validate(updated_plan) |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_plan() does not guard against renaming a plan to a name that already exists within the organization. Because the DB has a unique (organization_id, name) constraint, such an update will raise an IntegrityError during repository.update() and currently surfaces as a 500. Add a duplicate-name check when update_data.name is set (scoped to org, excluding the current plan) and convert DB uniqueness violations into a user-facing 400/409.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Added duplicate name check when update_data.name is set, preventing IntegrityError and returning user-facing 400 error instead of 500.
| # Create indexes | ||
| op.create_index( | ||
| "idx_benefit_plans_organization_id", | ||
| "benefit_plans", | ||
| ["organization_id"], | ||
| ) |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index naming here uses the idx_... prefix (e.g., idx_benefit_plans_organization_id), but existing migrations use SQLAlchemy/Alembic’s default ix_... prefix (see 001_initial_threat_persistence.py). Because the ORM model also declares index=True on these columns (which generates ix_... names), this mismatch can cause Alembic autogenerate churn or duplicated indexes depending on how schema is created. Consider standardizing index names across migrations/models (either switch these to ix_... or define explicit indexes in the model with the same idx_... names and remove index=True).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Standardized all index names to use ix_ prefix in both migration and ORM model to match existing conventions and prevent autogenerate churn.
| async def create_benefit_plan( | ||
| plan_data: BenefitPlanCreate, | ||
| service: BenefitPlanService = Depends(get_benefit_plan_service), | ||
| ) -> BenefitPlanRead: |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The create endpoint accepts BenefitPlanCreate with a client-supplied organization_id and does not use the org context dependency. In a multi-tenant system this allows creating plans under any org id the caller provides. Derive organization_id from get_current_organization (or auth claims) and override/ignore any body-provided org id (or remove it from the request schema for this endpoint).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Create endpoint now derives organization_id from get_current_organization dependency and overrides any client-supplied value for proper multi-tenant isolation.
| with pytest.raises( | ||
| PydanticValidationError, match="termination_date must be on or after effective_date" | ||
| ): | ||
| invalid_plan = BenefitPlanCreate( |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable invalid_plan is not used.
| invalid_plan = BenefitPlanCreate( | |
| BenefitPlanCreate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Removed unused invalid_plan variable.
| """ | ||
|
|
||
| import os | ||
| from datetime import date, datetime |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'datetime' is not used.
| from datetime import date, datetime | |
| from datetime import date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Removed unused datetime import.
| """ | ||
|
|
||
| from datetime import date | ||
| from decimal import Decimal |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Decimal' is not used.
| from decimal import Decimal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a5d0ff3. Removed unused Decimal import.
|
@copilot apply changes based on the comments in this thread |
… type mismatches, and improve security Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
Co-authored-by: aurelianware <194855645+aurelianware@users.noreply.github.com>
|
@copilot This PR was created in the wrong repo, I wanted this PR for cloudhealthoffice |
I understand this PR was created in the wrong repository. The benefit plan configuration module was implemented for the PrivaseeAI.Security repository, but you intended it for the cloudhealthoffice repository. Since this is a repository mismatch rather than a code issue, the best approach would be to:
Would you like me to help you extract the changes so they can be applied to the correct repository, or would you prefer to close this PR and handle the migration yourself? |
|
Closing.@copilot implement this in Aurelianware/cloudhealthoffice instead |
Overview
Implements insurance benefit plan configuration for healthcare operations. Multi-tenant async CRUD system following domain-driven design with strict layer separation.
Architecture
Domain (
domain/benefit_plans/)Infrastructure (
infrastructure/persistence/)(organization_id, name) WHERE deleted_at IS NULLfind_by_org_and_name()Application (
application/benefit_plans/)API (
api/v1/benefit_plans.py)Key Changes
Security
org_idfor defense-in-depthData Model
Migration
002_add_benefit_planscreates table with indexespgcryptoextension setup forgen_random_uuid()ix_*prefix for consistency with existing schemaTesting
24 unit tests:
All tests passing. CodeQL: 0 vulnerabilities.
Original prompt
Here is a well-structured, detailed prompt you can give to GitHub Copilot, Copilot Chat, Claude, Cursor, or any other capable code-generation LLM to create a solid benefit plan configuration module for your cloudhealthoffice project.
You are an expert backend engineer building a configuration system for CloudHealthOffice — a modern healthcare SaaS platform that helps medical offices manage insurance benefits, patient eligibility, copays, deductibles, and billing rules.
Create a clean, production-ready benefit plan configuration module in Python using modern practices.
Project context:
Requirements for the benefit plan configuration module:
Core domain concept: BenefitPlan
Main benefit rules that should be configurable per plan:
Create the following pieces:
A. Pydantic models (schemas)
B. SQLAlchemy model
C. Repository pattern
D. Service layer (application/benefit_plans/services.py)
E. FastAPI router (api/v1/benefit_plans.py)
F. Enums
Desired code style & patterns:
model_config = ConfigDict(from_attributes=True)Please generate the complete folder structure and code for:
domain/benefit_plans/
├── entities.py # or models.py
├── enums.py
application/benefit_plans/
├── services.py
├── dto.py # if you prefer t...
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.