feat(portfolio): Implement Fyers portfolio fetching and complete QA#34
Merged
Conversation
This commit introduces the portfolio fetching functionality for the Fyers adapter. - Implements `get_portfolio` in the `FyersAdapter` to retrieve holdings and funds. - Adds `Portfolio`, `Holding`, and `Funds` models to standardize the portfolio data structure. - Includes unit and integration tests for the new functionality. - Completes the QA review for story 1.5, adding QA results to the story file and creating a quality gate file.
Contributor
WalkthroughImplements FyersAdapter.get_portfolio to fetch holdings and funds using a session access token, maps responses into new Pydantic models (Holding, Funds, Portfolio), introduces OrdoError and ApiException, adds tests covering success and API error paths, and includes QA/docs updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller
participant FA as FyersAdapter
participant Sess as SessionStore
participant FYH as Fyers /holdings
participant FYF as Fyers /funds
participant Models as PortfolioModels
Caller->>FA: get_portfolio()
FA->>Sess: read access_token
alt token present
FA->>FYH: GET /holdings (Authorization: Bearer <token>)
FYH-->>FA: 200 JSON (s="ok", holdings...)
FA->>FYF: GET /funds (Authorization: Bearer <token>)
FYF-->>FA: 200 JSON (funds...)
FA->>Models: map to Holding[], Funds, Portfolio
Models-->>FA: Portfolio instance
FA-->>Caller: Portfolio.model_dump()
else token missing/invalid
FA-->>Caller: ApiException(ApiError(token_missing))
end
alt API returns error / logical non-ok
FYH-->>FA: 4xx/5xx or 200 with s!="ok"
FA-->>Caller: ApiException(ApiError(code=BROKER_API_ERROR,...))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/qa/gates/1.5-fyers-adapter-portfolio.yml(1 hunks)docs/stories/story-1.5.md(3 hunks)src/ordo/adapters/fyers.py(2 hunks)src/ordo/exceptions.py(1 hunks)src/ordo/models/api/errors.py(2 hunks)src/ordo/models/api/portfolio.py(1 hunks)tests/adapters/test_fyers.py(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/stories/story-1.5.md
23-23: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (4)
src/ordo/models/api/errors.py (3)
22-26: LGTM! Clean exception wrapper implementation.The
ApiExceptioncorrectly wrapsApiErrorinstances, preserving both the structured error details (self.error) and standard exception message semantics viasuper().__init__(error.message). This design allows catching and inspecting the fullApiErrorpayload while maintaining Python exception conventions.
4-4: OrdoError import is valid. TheOrdoErrorclass is defined inordo.exceptions.
28-28: SecurityException inheritance change is safe OrdoError extends Exception, so this change remains backward compatible.src/ordo/models/api/portfolio.py (1)
1-2: LGTM! Standard Pydantic v2 imports.The imports are correct and follow Pydantic v2 best practices.
Comment on lines
+5
to
+17
| class Holding(BaseModel): | ||
| symbol: str = Field(..., description="Trading symbol of the instrument.") | ||
| quantity: int = Field(..., description="The quantity of the instrument held.") | ||
| ltp: float = Field(..., description="Last Traded Price of the instrument.") | ||
| avg_price: float = Field( | ||
| ..., description="Average acquisition price of the instrument." | ||
| ) | ||
| pnl: float = Field(..., description="Profit and Loss for the holding.") | ||
| day_pnl: float = Field(..., description="Profit and Loss for the current day.") | ||
| value: float = Field( | ||
| ..., description="Current market value of the holding (quantity * ltp)." | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating or computing derived fields.
The value field description states it should be "quantity * ltp", and pnl logically should be (ltp - avg_price) * quantity. However, both are required input fields with no validation or computation to enforce consistency.
Consider one of these approaches:
- Use Pydantic's
@computed_fieldto derivevalueandpnlfrom the base fields. - Add a
@model_validatorto verify the incoming values match the expected formulas. - If the broker API returns these pre-calculated values and they might legitimately differ (e.g., due to fees, splits, or rounding), document why manual values are needed.
Option 1: Make value and pnl computed fields (if broker doesn't return them):
+from pydantic import computed_field
class Holding(BaseModel):
symbol: str = Field(..., description="Trading symbol of the instrument.")
quantity: int = Field(..., description="The quantity of the instrument held.")
ltp: float = Field(..., description="Last Traded Price of the instrument.")
avg_price: float = Field(
..., description="Average acquisition price of the instrument."
)
- pnl: float = Field(..., description="Profit and Loss for the holding.")
day_pnl: float = Field(..., description="Profit and Loss for the current day.")
- value: float = Field(
- ..., description="Current market value of the holding (quantity * ltp)."
- )
+
+ @computed_field
+ @property
+ def value(self) -> float:
+ """Current market value of the holding (quantity * ltp)."""
+ return self.quantity * self.ltp
+
+ @computed_field
+ @property
+ def pnl(self) -> float:
+ """Profit and Loss for the holding."""
+ return (self.ltp - self.avg_price) * self.quantityOption 2: Add validation (if broker returns these values and you want to verify):
+from pydantic import model_validator
class Holding(BaseModel):
symbol: str = Field(..., description="Trading symbol of the instrument.")
quantity: int = Field(..., description="The quantity of the instrument held.")
ltp: float = Field(..., description="Last Traded Price of the instrument.")
avg_price: float = Field(
..., description="Average acquisition price of the instrument."
)
pnl: float = Field(..., description="Profit and Loss for the holding.")
day_pnl: float = Field(..., description="Profit and Loss for the current day.")
value: float = Field(
..., description="Current market value of the holding (quantity * ltp)."
)
+
+ @model_validator(mode='after')
+ def validate_computed_fields(self) -> 'Holding':
+ expected_value = self.quantity * self.ltp
+ expected_pnl = (self.ltp - self.avg_price) * self.quantity
+ if abs(self.value - expected_value) > 0.01:
+ raise ValueError(f"value {self.value} does not match quantity * ltp = {expected_value}")
+ if abs(self.pnl - expected_pnl) > 0.01:
+ raise ValueError(f"pnl {self.pnl} does not match (ltp - avg_price) * quantity = {expected_pnl}")
+ return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class Holding(BaseModel): | |
| symbol: str = Field(..., description="Trading symbol of the instrument.") | |
| quantity: int = Field(..., description="The quantity of the instrument held.") | |
| ltp: float = Field(..., description="Last Traded Price of the instrument.") | |
| avg_price: float = Field( | |
| ..., description="Average acquisition price of the instrument." | |
| ) | |
| pnl: float = Field(..., description="Profit and Loss for the holding.") | |
| day_pnl: float = Field(..., description="Profit and Loss for the current day.") | |
| value: float = Field( | |
| ..., description="Current market value of the holding (quantity * ltp)." | |
| ) | |
| from pydantic import BaseModel, Field, computed_field | |
| class Holding(BaseModel): | |
| symbol: str = Field(..., description="Trading symbol of the instrument.") | |
| quantity: int = Field(..., description="The quantity of the instrument held.") | |
| ltp: float = Field(..., description="Last Traded Price of the instrument.") | |
| avg_price: float = Field( | |
| ..., description="Average acquisition price of the instrument." | |
| ) | |
| day_pnl: float = Field(..., description="Profit and Loss for the current day.") | |
| @computed_field | |
| @property | |
| def value(self) -> float: | |
| """Current market value of the holding (quantity * ltp).""" | |
| return self.quantity * self.ltp | |
| @computed_field | |
| @property | |
| def pnl(self) -> float: | |
| """Profit and Loss for the holding.""" | |
| return (self.ltp - self.avg_price) * self.quantity |
Suggested change
| class Holding(BaseModel): | |
| symbol: str = Field(..., description="Trading symbol of the instrument.") | |
| quantity: int = Field(..., description="The quantity of the instrument held.") | |
| ltp: float = Field(..., description="Last Traded Price of the instrument.") | |
| avg_price: float = Field( | |
| ..., description="Average acquisition price of the instrument." | |
| ) | |
| pnl: float = Field(..., description="Profit and Loss for the holding.") | |
| day_pnl: float = Field(..., description="Profit and Loss for the current day.") | |
| value: float = Field( | |
| ..., description="Current market value of the holding (quantity * ltp)." | |
| ) | |
| from pydantic import BaseModel, Field, model_validator | |
| class Holding(BaseModel): | |
| symbol: str = Field(..., description="Trading symbol of the instrument.") | |
| quantity: int = Field(..., description="The quantity of the instrument held.") | |
| ltp: float = Field(..., description="Last Traded Price of the instrument.") | |
| avg_price: float = Field( | |
| ..., description="Average acquisition price of the instrument." | |
| ) | |
| pnl: float = Field(..., description="Profit and Loss for the holding.") | |
| day_pnl: float = Field(..., description="Profit and Loss for the current day.") | |
| value: float = Field( | |
| ..., description="Current market value of the holding (quantity * ltp)." | |
| ) | |
| @model_validator(mode='after') | |
| def validate_computed_fields(self) -> "Holding": | |
| expected_value = self.quantity * self.ltp | |
| expected_pnl = (self.ltp - self.avg_price) * self.quantity | |
| if abs(self.value - expected_value) > 0.01: | |
| raise ValueError(f"value {self.value} does not match quantity * ltp = {expected_value}") | |
| if abs(self.pnl - expected_pnl) > 0.01: | |
| raise ValueError(f"pnl {self.pnl} does not match (ltp - avg_price) * quantity = {expected_pnl}") | |
| return self |
🤖 Prompt for AI Agents
In src/ordo/models/api/portfolio.py around lines 5 to 17, the Holding model
currently requires externally-provided pnl and value though they are derivable
from quantity, ltp and avg_price; update the model so these fields are either
computed or validated: choose one of two fixes—(A) make pnl and value computed
fields using Pydantic's @computed_field (compute value = quantity * ltp and pnl
= (ltp - avg_price) * quantity) and remove them from required input, or (B) add
a @model_validator that checks incoming pnl and value match the computed
formulas (within a small epsilon for rounding) and raise a ValidationError if
they differ; implement the chosen approach and update field descriptions
accordingly (or add a comment explaining why manual values are accepted if
broker-provided).
Comment on lines
+19
to
+27
| class Funds(BaseModel): | ||
| available_balance: float = Field( | ||
| ..., description="The total available balance in the account." | ||
| ) | ||
| margin_used: float = Field(..., description="The total margin utilized for trades.") | ||
| total_balance: float = Field( | ||
| ..., description="The total balance in the account (available + margin)." | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider computing or validating total_balance.
The total_balance description states it should be "available + margin", but it's a required input field with no validation. This creates potential inconsistency if the provided value doesn't match available_balance + margin_used.
Consider:
- Making
total_balancea computed field. - Adding validation to ensure consistency.
- If the broker API returns a pre-calculated
total_balancethat might legitimately differ, document why.
Option 1: Make total_balance a computed field:
+from pydantic import computed_field
class Funds(BaseModel):
available_balance: float = Field(
..., description="The total available balance in the account."
)
margin_used: float = Field(..., description="The total margin utilized for trades.")
- total_balance: float = Field(
- ..., description="The total balance in the account (available + margin)."
- )
+
+ @computed_field
+ @property
+ def total_balance(self) -> float:
+ """The total balance in the account (available + margin)."""
+ return self.available_balance + self.margin_usedOption 2: Add validation:
+from pydantic import model_validator
class Funds(BaseModel):
available_balance: float = Field(
..., description="The total available balance in the account."
)
margin_used: float = Field(..., description="The total margin utilized for trades.")
total_balance: float = Field(
..., description="The total balance in the account (available + margin)."
)
+
+ @model_validator(mode='after')
+ def validate_total_balance(self) -> 'Funds':
+ expected_total = self.available_balance + self.margin_used
+ if abs(self.total_balance - expected_total) > 0.01:
+ raise ValueError(f"total_balance {self.total_balance} does not match available_balance + margin_used = {expected_total}")
+ return self🤖 Prompt for AI Agents
In src/ordo/models/api/portfolio.py around lines 19 to 27, the total_balance
field is declared as a required input but its description says it should equal
available_balance + margin_used; update the model so total_balance is either
computed or validated: either remove total_balance from input and implement a
property or @root_validator that sets total_balance = available_balance +
margin_used, or keep it but add a validator (or root_validator) that checks
provided total_balance equals available_balance + margin_used (and raises a
ValueError or logs a warning) and document any legitimate broker deviations in
the field docstring; choose one approach and implement the corresponding
Pydantic pattern.
Comment on lines
+29
to
+42
| class Portfolio(BaseModel): | ||
| holdings: List[Holding] = Field( | ||
| ..., description="List of all holdings in the portfolio." | ||
| ) | ||
| funds: Funds = Field(..., description="Details of the funds in the account.") | ||
| total_pnl: float = Field( | ||
| ..., description="Total Profit and Loss for the portfolio." | ||
| ) | ||
| total_day_pnl: float = Field( | ||
| ..., description="Total Profit and Loss for the current day for the portfolio." | ||
| ) | ||
| total_value: float = Field( | ||
| ..., description="Total current market value of the portfolio." | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider computing aggregate fields from holdings.
The total_pnl, total_day_pnl, and total_value fields logically should be aggregations over the holdings list (sum of each holding's pnl, day_pnl, and value respectively). However, they are required input fields with no validation or computation.
Consider:
- Making these computed fields that sum the corresponding values from
holdings. - Adding validation to ensure consistency.
- If the broker API returns pre-calculated totals that might differ (e.g., including cash positions or adjustments not reflected in holdings), document why.
Option 1: Make aggregates computed fields:
+from pydantic import computed_field
class Portfolio(BaseModel):
holdings: List[Holding] = Field(
..., description="List of all holdings in the portfolio."
)
funds: Funds = Field(..., description="Details of the funds in the account.")
- total_pnl: float = Field(
- ..., description="Total Profit and Loss for the portfolio."
- )
- total_day_pnl: float = Field(
- ..., description="Total Profit and Loss for the current day for the portfolio."
- )
- total_value: float = Field(
- ..., description="Total current market value of the portfolio."
- )
+
+ @computed_field
+ @property
+ def total_pnl(self) -> float:
+ """Total Profit and Loss for the portfolio."""
+ return sum(h.pnl for h in self.holdings)
+
+ @computed_field
+ @property
+ def total_day_pnl(self) -> float:
+ """Total Profit and Loss for the current day for the portfolio."""
+ return sum(h.day_pnl for h in self.holdings)
+
+ @computed_field
+ @property
+ def total_value(self) -> float:
+ """Total current market value of the portfolio."""
+ return sum(h.value for h in self.holdings)Option 2: Add validation:
+from pydantic import model_validator
class Portfolio(BaseModel):
holdings: List[Holding] = Field(
..., description="List of all holdings in the portfolio."
)
funds: Funds = Field(..., description="Details of the funds in the account.")
total_pnl: float = Field(
..., description="Total Profit and Loss for the portfolio."
)
total_day_pnl: float = Field(
..., description="Total Profit and Loss for the current day for the portfolio."
)
total_value: float = Field(
..., description="Total current market value of the portfolio."
)
+
+ @model_validator(mode='after')
+ def validate_totals(self) -> 'Portfolio':
+ expected_pnl = sum(h.pnl for h in self.holdings)
+ expected_day_pnl = sum(h.day_pnl for h in self.holdings)
+ expected_value = sum(h.value for h in self.holdings)
+
+ if abs(self.total_pnl - expected_pnl) > 0.01:
+ raise ValueError(f"total_pnl {self.total_pnl} does not match sum of holdings pnl = {expected_pnl}")
+ if abs(self.total_day_pnl - expected_day_pnl) > 0.01:
+ raise ValueError(f"total_day_pnl {self.total_day_pnl} does not match sum of holdings day_pnl = {expected_day_pnl}")
+ if abs(self.total_value - expected_value) > 0.01:
+ raise ValueError(f"total_value {self.total_value} does not match sum of holdings value = {expected_value}")
+ return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class Portfolio(BaseModel): | |
| holdings: List[Holding] = Field( | |
| ..., description="List of all holdings in the portfolio." | |
| ) | |
| funds: Funds = Field(..., description="Details of the funds in the account.") | |
| total_pnl: float = Field( | |
| ..., description="Total Profit and Loss for the portfolio." | |
| ) | |
| total_day_pnl: float = Field( | |
| ..., description="Total Profit and Loss for the current day for the portfolio." | |
| ) | |
| total_value: float = Field( | |
| ..., description="Total current market value of the portfolio." | |
| ) | |
| class Portfolio(BaseModel): | |
| holdings: List[Holding] = Field( | |
| ..., description="List of all holdings in the portfolio." | |
| ) | |
| funds: Funds = Field(..., description="Details of the funds in the account.") | |
| @computed_field | |
| @property | |
| def total_pnl(self) -> float: | |
| """Total Profit and Loss for the portfolio.""" | |
| return sum(h.pnl for h in self.holdings) | |
| @computed_field | |
| @property | |
| def total_day_pnl(self) -> float: | |
| """Total Profit and Loss for the current day for the portfolio.""" | |
| return sum(h.day_pnl for h in self.holdings) | |
| @computed_field | |
| @property | |
| def total_value(self) -> float: | |
| """Total current market value of the portfolio.""" | |
| return sum(h.value for h in self.holdings) |
Suggested change
| class Portfolio(BaseModel): | |
| holdings: List[Holding] = Field( | |
| ..., description="List of all holdings in the portfolio." | |
| ) | |
| funds: Funds = Field(..., description="Details of the funds in the account.") | |
| total_pnl: float = Field( | |
| ..., description="Total Profit and Loss for the portfolio." | |
| ) | |
| total_day_pnl: float = Field( | |
| ..., description="Total Profit and Loss for the current day for the portfolio." | |
| ) | |
| total_value: float = Field( | |
| ..., description="Total current market value of the portfolio." | |
| ) | |
| class Portfolio(BaseModel): | |
| holdings: List[Holding] = Field( | |
| ..., description="List of all holdings in the portfolio." | |
| ) | |
| funds: Funds = Field(..., description="Details of the funds in the account.") | |
| total_pnl: float = Field( | |
| ..., description="Total Profit and Loss for the portfolio." | |
| ) | |
| total_day_pnl: float = Field( | |
| ..., description="Total Profit and Loss for the current day for the portfolio." | |
| ) | |
| total_value: float = Field( | |
| ..., description="Total current market value of the portfolio." | |
| ) | |
| @model_validator(mode='after') | |
| def validate_totals(self) -> 'Portfolio': | |
| expected_pnl = sum(h.pnl for h in self.holdings) | |
| expected_day_pnl = sum(h.day_pnl for h in self.holdings) | |
| expected_value = sum(h.value for h in self.holdings) | |
| if abs(self.total_pnl - expected_pnl) > 0.01: | |
| raise ValueError( | |
| f"total_pnl {self.total_pnl} does not match sum of holdings pnl = {expected_pnl}" | |
| ) | |
| if abs(self.total_day_pnl - expected_day_pnl) > 0.01: | |
| raise ValueError( | |
| f"total_day_pnl {self.total_day_pnl} does not match sum of holdings day_pnl = {expected_day_pnl}" | |
| ) | |
| if abs(self.total_value - expected_value) > 0.01: | |
| raise ValueError( | |
| f"total_value {self.total_value} does not match sum of holdings value = {expected_value}" | |
| ) | |
| return self |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/adapters/test_fyers.py (1)
388-414: LGTM! Past review concern addressed.This test correctly implements the coverage requested in the previous review, handling the case where Fyers returns HTTP 200 but with
{"s": "error"}to indicate a logical failure. The assertion confirms thatApiExceptionwithBROKER_API_ERRORis raised as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/adapters/test_fyers.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/adapters/test_fyers.py (2)
src/ordo/models/api/errors.py (2)
CSRFError(34-37)ApiException(22-25)src/ordo/adapters/fyers.py (1)
get_portfolio(154-245)
🔇 Additional comments (2)
tests/adapters/test_fyers.py (2)
8-8: LGTM!The import of
ApiExceptionis necessary for the new portfolio tests and correctly references the error class introduced in this PR.
361-386: LGTM!This test correctly verifies that HTTP error responses (400) from the holdings endpoint are wrapped in
ApiExceptionwithBROKER_API_ERRORas expected by the adapter implementation.
Comment on lines
+267
to
+359
| @pytest.mark.asyncio | ||
| @pytest.mark.integration | ||
| @respx.mock | ||
| async def test_get_portfolio_success(mock_session_manager): | ||
| """ | ||
| Tests the success case for get_portfolio. | ||
| """ | ||
| adapter = FyersAdapter() | ||
| holdings_url = f"{adapter.base_url}/holdings" | ||
| funds_url = f"{adapter.base_url}/funds" | ||
|
|
||
| holdings_response = { | ||
| "s": "ok", | ||
| "code": 200, | ||
| "message": "", | ||
| "holdings": [ | ||
| { | ||
| "holdingType": "HLD", | ||
| "quantity": 1, | ||
| "costPrice": 1.55, | ||
| "marketVal": 3.75, | ||
| "remainingQuantity": 1, | ||
| "pl": 2.2, | ||
| "ltp": 3.75, | ||
| "id": 1, | ||
| "fyToken": 101000000011460, | ||
| "exchange": 10, | ||
| "symbol": "NSE:JPASSOCIAT-EQ", | ||
| "segment": 10, | ||
| "isin": "INE669E01016", | ||
| "qty_t1": 1, | ||
| "remainingPledgeQuantity": -1, | ||
| "collateralQuantity": 0, | ||
| } | ||
| ], | ||
| "overall": { | ||
| "count_total": 1, | ||
| "total_investment": 1.55, | ||
| "total_current_value": 3.75, | ||
| "total_pl": 2.2, | ||
| "pnl_perc": 141.94, | ||
| }, | ||
| } | ||
|
|
||
| funds_response = { | ||
| "code": 200, | ||
| "message": "", | ||
| "s": "ok", | ||
| "fund_limit": [ | ||
| { | ||
| "id": 1, | ||
| "title": "Total Balance", | ||
| "equityAmount": 10000, | ||
| "commodityAmount": 0, | ||
| }, | ||
| { | ||
| "id": 2, | ||
| "title": "Utilized Amount", | ||
| "equityAmount": 2000, | ||
| "commodityAmount": 0, | ||
| }, | ||
| { | ||
| "id": 10, | ||
| "title": "Available Balance", | ||
| "equityAmount": 8000, | ||
| "commodityAmount": 0, | ||
| }, | ||
| ], | ||
| } | ||
|
|
||
| respx.get(holdings_url).mock(return_value=Response(200, json=holdings_response)) | ||
| respx.get(funds_url).mock(return_value=Response(200, json=funds_response)) | ||
|
|
||
| mock_session_manager.get_session.return_value = "test_access_token" | ||
|
|
||
| session_data = { | ||
| "credentials": { | ||
| "app_id": "test_app_id", | ||
| "secret_id": "test_secret_id", | ||
| "redirect_uri": "http://localhost:8000/callback", | ||
| } | ||
| } | ||
|
|
||
| result = await adapter.get_portfolio(session_data) | ||
|
|
||
| assert result["total_pnl"] == 2.2 | ||
| assert result["total_value"] == 3.75 | ||
| assert result["funds"]["available_balance"] == 8000 | ||
| assert result["funds"]["margin_used"] == 2000 | ||
| assert result["funds"]["total_balance"] == 10000 | ||
| assert len(result["holdings"]) == 1 | ||
| assert result["holdings"][0]["symbol"] == "NSE:JPASSOCIAT-EQ" | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test coverage is good; consider verifying individual holding fields.
The test successfully covers the happy path for portfolio retrieval, mocking both endpoints and asserting the key aggregated values. However, to strengthen coverage of the data transformation logic, consider adding assertions for individual holding fields:
assert result["holdings"][0]["symbol"] == "NSE:JPASSOCIAT-EQ"
+ # Verify individual holding field mappings
+ holding = result["holdings"][0]
+ assert holding["quantity"] == 1
+ assert holding["ltp"] == 3.75
+ assert holding["avg_price"] == 1.55
+ assert holding["pnl"] == 2.2
+ assert holding["value"] == 3.75
+ assert holding["day_pnl"] == 0
+ assert result["total_day_pnl"] == 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.asyncio | |
| @pytest.mark.integration | |
| @respx.mock | |
| async def test_get_portfolio_success(mock_session_manager): | |
| """ | |
| Tests the success case for get_portfolio. | |
| """ | |
| adapter = FyersAdapter() | |
| holdings_url = f"{adapter.base_url}/holdings" | |
| funds_url = f"{adapter.base_url}/funds" | |
| holdings_response = { | |
| "s": "ok", | |
| "code": 200, | |
| "message": "", | |
| "holdings": [ | |
| { | |
| "holdingType": "HLD", | |
| "quantity": 1, | |
| "costPrice": 1.55, | |
| "marketVal": 3.75, | |
| "remainingQuantity": 1, | |
| "pl": 2.2, | |
| "ltp": 3.75, | |
| "id": 1, | |
| "fyToken": 101000000011460, | |
| "exchange": 10, | |
| "symbol": "NSE:JPASSOCIAT-EQ", | |
| "segment": 10, | |
| "isin": "INE669E01016", | |
| "qty_t1": 1, | |
| "remainingPledgeQuantity": -1, | |
| "collateralQuantity": 0, | |
| } | |
| ], | |
| "overall": { | |
| "count_total": 1, | |
| "total_investment": 1.55, | |
| "total_current_value": 3.75, | |
| "total_pl": 2.2, | |
| "pnl_perc": 141.94, | |
| }, | |
| } | |
| funds_response = { | |
| "code": 200, | |
| "message": "", | |
| "s": "ok", | |
| "fund_limit": [ | |
| { | |
| "id": 1, | |
| "title": "Total Balance", | |
| "equityAmount": 10000, | |
| "commodityAmount": 0, | |
| }, | |
| { | |
| "id": 2, | |
| "title": "Utilized Amount", | |
| "equityAmount": 2000, | |
| "commodityAmount": 0, | |
| }, | |
| { | |
| "id": 10, | |
| "title": "Available Balance", | |
| "equityAmount": 8000, | |
| "commodityAmount": 0, | |
| }, | |
| ], | |
| } | |
| respx.get(holdings_url).mock(return_value=Response(200, json=holdings_response)) | |
| respx.get(funds_url).mock(return_value=Response(200, json=funds_response)) | |
| mock_session_manager.get_session.return_value = "test_access_token" | |
| session_data = { | |
| "credentials": { | |
| "app_id": "test_app_id", | |
| "secret_id": "test_secret_id", | |
| "redirect_uri": "http://localhost:8000/callback", | |
| } | |
| } | |
| result = await adapter.get_portfolio(session_data) | |
| assert result["total_pnl"] == 2.2 | |
| assert result["total_value"] == 3.75 | |
| assert result["funds"]["available_balance"] == 8000 | |
| assert result["funds"]["margin_used"] == 2000 | |
| assert result["funds"]["total_balance"] == 10000 | |
| assert len(result["holdings"]) == 1 | |
| assert result["holdings"][0]["symbol"] == "NSE:JPASSOCIAT-EQ" | |
| assert result["holdings"][0]["symbol"] == "NSE:JPASSOCIAT-EQ" | |
| # Verify individual holding field mappings | |
| holding = result["holdings"][0] | |
| assert holding["quantity"] == 1 | |
| assert holding["ltp"] == 3.75 | |
| assert holding["avg_price"] == 1.55 | |
| assert holding["pnl"] == 2.2 | |
| assert holding["value"] == 3.75 | |
| assert holding["day_pnl"] == 0 | |
| assert result["total_day_pnl"] == 0 |
🤖 Prompt for AI Agents
tests/adapters/test_fyers.py around lines 267 to 359: add assertions that verify
the transformed fields of the single holding returned by get_portfolio (e.g.,
map costPrice -> cost_price, marketVal -> market_value or whichever keys adapter
uses, quantity, ltp, pl, isin, fyToken/id and exchange) to ensure the adapter
correctly maps and preserves individual holding values; update the test to
assert those individual fields from result["holdings"][0] (keep existing
aggregated asserts) so the transformation logic is validated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related Issue
Closes: #8
Type of change
How to test / QA steps
git checkout feature/8-fyers-portfolio-adapterpoetry installpoetry run pytesttests/adapters/test_fyers.pycover the new functionality.Screenshots (if UI)
N/A
Checklist — Definition of Done
Notes / Risks / Follow-ups
None
Summary by CodeRabbit
New Features
Documentation
Tests