-
Notifications
You must be signed in to change notification settings - Fork 0
fix(portfolio): return explicit 204 Response on position delete #2
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
Changes from all commits
69825b8
fd0a5ae
960a181
1866489
942c22c
95297d5
17b01fe
687bbf4
0048b17
1cc4fb0
b3feba8
2b6cee0
cec28ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,32 +52,37 @@ jobs: | |
| - name: Faithfulness demo | ||
| run: python testing_suite/calculate_faithfulness.py --demo | ||
|
|
||
| ai-evaluation: | ||
| name: DeepEval RAG / SQL checks | ||
| deepeval-optional: | ||
| runs-on: ubuntu-latest | ||
| needs: [python-tests] | ||
| # Optional: OpenAI quota / billing failures should not block the pipeline | ||
| continue-on-error: true | ||
| needs: [python-lint] | ||
| if: ${{ !secrets.OPENAI_API_KEY }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: ${{ env.PYTHON_VERSION }} | ||
| cache: pip | ||
| - name: Install | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements-local.txt | ||
| pip install -r testing_suite/requirements.txt | ||
| - name: DeepEval (requires OPENAI_API_KEY) | ||
| python-version: "3.12" | ||
| - run: pip install -e packages/ai-core[dev] deepeval | ||
| - run: pytest -q testing_suite/test_deepeval_sql_rag.py | ||
| env: | ||
| OPENAI_API_KEY: "" | ||
|
|
||
| deepeval-required: | ||
| runs-on: ubuntu-latest | ||
| continue-on-error: false | ||
| needs: [deepeval-optional] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| env: | ||
| HAS_KEY: ${{ secrets.OPENAI_API_KEY != '' }} | ||
| if: env.HAS_KEY == 'true' | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
| - run: pip install -e packages/ai-core[dev] deepeval | ||
| - run: pytest -q testing_suite/test_deepeval_sql_rag.py | ||
| env: | ||
| OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} | ||
| run: | | ||
| if [ -z "$OPENAI_API_KEY" ]; then | ||
| echo "OPENAI_API_KEY not set; skipping DeepEval" | ||
| exit 0 | ||
| fi | ||
| pytest -q testing_suite/test_deepeval_sql_rag.py | ||
|
|
||
| web-build: | ||
| name: Next.js production build | ||
|
|
@@ -114,8 +119,9 @@ jobs: | |
| uses: actions/setup-node@v4 | ||
| with: { node-version: '20' } | ||
| - name: Install and type-check | ||
| working-directory: apps/web | ||
| run: npm ci && npx tsc --noEmit | ||
| run: | | ||
| npm ci | ||
| npm run typecheck -w apps/web | ||
|
|
||
| python-lint: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,13 @@ | |
|
|
||
|
|
||
| def get_portfolio_graph(): | ||
| from aequitas_ai.agents.portfolio_agent import build_portfolio_agent | ||
| from aequitas_ai.agents.portfolio_agent import build_portfolio_agent, PortfolioAgentConfig | ||
| from langchain_openai import ChatOpenAI | ||
| from app.config import settings | ||
|
|
||
| return build_portfolio_agent() | ||
| analysis_llm = ChatOpenAI( | ||
| model=settings.synthesis_model, | ||
| temperature=0.0 | ||
|
Comment on lines
+11
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This constructs a Useful? React with 👍 / 👎. |
||
| ) | ||
| config = PortfolioAgentConfig(analysis_llm=analysis_llm) | ||
| return build_portfolio_agent(config) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| from decimal import Decimal | ||
| from uuid import UUID | ||
|
|
||
| from fastapi import APIRouter, Depends, HTTPException, Request, status | ||
| from fastapi import APIRouter, Depends, HTTPException, Request, Response, status | ||
| from fastapi.responses import StreamingResponse | ||
| from langchain_anthropic import ChatAnthropic | ||
| from langchain_core.language_models.chat_models import BaseChatModel | ||
|
|
@@ -188,13 +188,13 @@ async def list_positions( | |
| return [PositionOut.from_row(r) for r in rows] | ||
|
|
||
|
|
||
| @router.delete("/{portfolio_id}/positions/{position_id}", status_code=status.HTTP_204_NO_CONTENT) | ||
| @router.delete("/{portfolio_id}/positions/{position_id}") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Removing Useful? React with 👍 / 👎. |
||
| async def delete_position( | ||
| request: Request, | ||
| portfolio_id: UUID, | ||
| position_id: UUID, | ||
| session: AsyncSession = Depends(_get_session), | ||
| ) -> None: | ||
| ) -> Response: | ||
| ident = await get_identity(request) | ||
| user_id = _parse_user_id_or_400(ident.sub) | ||
| portfolio = await portfolio_svc.get_portfolio(session, portfolio_id, user_id) | ||
|
|
@@ -203,6 +203,7 @@ async def delete_position( | |
| deleted = await portfolio_svc.delete_position(session, position_id, portfolio_id) | ||
| if not deleted: | ||
| raise HTTPException(status_code=404, detail="Position not found") | ||
| return Response(status_code=status.HTTP_204_NO_CONTENT) | ||
|
|
||
|
|
||
| @router.get("/{portfolio_id}/pnl") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| """DELETE /v1/portfolio/.../positions/... returns explicit 204 Response.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import AsyncMock, patch | ||
| from uuid import uuid4 | ||
|
|
||
| import pytest | ||
| from fastapi import FastAPI | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from app.auth.identity import Identity | ||
| from app.routers.portfolio import _get_session, router | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def portfolio_id() -> str: | ||
| return str(uuid4()) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def position_id() -> str: | ||
| return str(uuid4()) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def user_id() -> str: | ||
| return str(uuid4()) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def client() -> TestClient: | ||
| app = FastAPI() | ||
| app.include_router(router) | ||
|
|
||
| async def _mock_session(): | ||
| yield AsyncMock() | ||
|
|
||
| app.dependency_overrides[_get_session] = _mock_session | ||
| return TestClient(app) | ||
|
|
||
|
|
||
| def test_delete_position_returns_204( | ||
| client: TestClient, | ||
| portfolio_id: str, | ||
| position_id: str, | ||
| user_id: str, | ||
| ) -> None: | ||
| ident = Identity(sub=user_id, role="analyst") | ||
| portfolio = object() | ||
|
|
||
| with ( | ||
| patch("app.routers.portfolio.get_identity", new_callable=AsyncMock, return_value=ident), | ||
| patch( | ||
| "app.routers.portfolio.portfolio_svc.get_portfolio", | ||
| new_callable=AsyncMock, | ||
| return_value=portfolio, | ||
| ), | ||
| patch( | ||
| "app.routers.portfolio.portfolio_svc.delete_position", | ||
| new_callable=AsyncMock, | ||
| return_value=True, | ||
| ), | ||
| ): | ||
| res = client.delete(f"/v1/portfolio/{portfolio_id}/positions/{position_id}") | ||
|
|
||
| assert res.status_code == 204 | ||
| assert res.content == b"" | ||
|
|
||
|
|
||
| def test_delete_position_404_when_portfolio_missing( | ||
| client: TestClient, | ||
| portfolio_id: str, | ||
| position_id: str, | ||
| user_id: str, | ||
| ) -> None: | ||
| ident = Identity(sub=user_id, role="analyst") | ||
|
|
||
| with ( | ||
| patch("app.routers.portfolio.get_identity", new_callable=AsyncMock, return_value=ident), | ||
| patch( | ||
| "app.routers.portfolio.portfolio_svc.get_portfolio", | ||
| new_callable=AsyncMock, | ||
| return_value=None, | ||
| ), | ||
| ): | ||
| res = client.delete(f"/v1/portfolio/{portfolio_id}/positions/{position_id}") | ||
|
|
||
| assert res.status_code == 404 | ||
| assert res.json()["detail"] == "Portfolio not found" | ||
|
|
||
|
|
||
| def test_delete_position_404_when_position_missing( | ||
| client: TestClient, | ||
| portfolio_id: str, | ||
| position_id: str, | ||
| user_id: str, | ||
| ) -> None: | ||
| ident = Identity(sub=user_id, role="analyst") | ||
| portfolio = object() | ||
|
|
||
| with ( | ||
| patch("app.routers.portfolio.get_identity", new_callable=AsyncMock, return_value=ident), | ||
| patch( | ||
| "app.routers.portfolio.portfolio_svc.get_portfolio", | ||
| new_callable=AsyncMock, | ||
| return_value=portfolio, | ||
| ), | ||
| patch( | ||
| "app.routers.portfolio.portfolio_svc.delete_position", | ||
| new_callable=AsyncMock, | ||
| return_value=False, | ||
| ), | ||
| ): | ||
| res = client.delete(f"/v1/portfolio/{portfolio_id}/positions/{position_id}") | ||
|
|
||
| assert res.status_code == 404 | ||
| assert res.json()["detail"] == "Position not found" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| "use client"; | ||
|
|
||
| import type { ReactNode } from "react"; | ||
| import { cn } from "@/lib/utils"; | ||
|
|
||
| type PageTemplateProps = { | ||
| title: string; | ||
| subtitle?: string; | ||
| actions?: ReactNode; | ||
| children: ReactNode; | ||
| maxWidthClassName?: string; | ||
| }; | ||
|
|
||
| export function PageTemplate({ | ||
| title, | ||
| subtitle, | ||
| actions, | ||
| children, | ||
| maxWidthClassName = "max-w-6xl", | ||
| }: PageTemplateProps) { | ||
| return ( | ||
| <section className={cn("mx-auto w-full animate-fade-in-up px-4 py-6", maxWidthClassName)}> | ||
| <header className="mb-5 flex flex-wrap items-start justify-between gap-3"> | ||
| <div> | ||
| <h1 className="text-2xl font-semibold tracking-tight text-foreground md:text-3xl">{title}</h1> | ||
| {subtitle ? <p className="mt-1 text-sm text-muted-foreground">{subtitle}</p> : null} | ||
| </div> | ||
| {actions ? <div className="flex items-center gap-2">{actions}</div> : null} | ||
| </header> | ||
| {children} | ||
| </section> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
|
|
||
| > @aequitas/web@0.1.0 typecheck | ||
| > tsc --noEmit | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import asyncio, json | ||
| from aequitas_ai.tools.market_data import fetch_market_price | ||
| from unittest.mock import patch, AsyncMock | ||
| from httpx import Response | ||
|
|
||
| async def test(): | ||
| mock_yahoo_data = {'chart': {'result': [{'meta': {'regularMarketPrice': 150.25}}]}} | ||
| mock_response = Response(200, json=mock_yahoo_data, request=None) | ||
| mock_get = AsyncMock(return_value=mock_response) | ||
| with patch('httpx.AsyncClient.get', mock_get): | ||
| result = await fetch_market_price('AAPL') | ||
| print("RESULT:") | ||
| print(result) | ||
|
|
||
| if __name__ == '__main__': | ||
| asyncio.run(test()) |
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.
This
ifusessecretsdirectly at the job level, which GitHub Actions does not support for conditionals; workflow validation fails before execution, so the DeepEval jobs never run in CI. Move the secret into an allowed context (for example viavars/needsoutputs or a preceding job output) and gate on that value instead.Useful? React with 👍 / 👎.