odoo fixes #2295
odoo fixes #2295
Conversation
WalkthroughAdds per-item discount support, session invalidation, and a return_all product filter to POS flows. Exposes a new POST /invalidate/session endpoint, extends models to store discounts, and updates the processor to handle discounts, dynamic partner lookup/creation, and optional product filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as POS API Router
participant Processor as POS Processor
participant Odoo as Odoo Backend
Client->>Router: POST /pos/order (session_id, products, partner_id?)
Router->>Processor: create_pos_order(session_id, products, partner_id)
alt partner_id provided
Processor->>Odoo: use provided partner_id
else partner_id not provided
Processor->>Odoo: search_read partner "POS Customer"
alt found
Odoo-->>Processor: return partner_id
else not found
Processor->>Odoo: create partner "POS Customer"
Odoo-->>Processor: return new partner_id
end
end
Processor->>Processor: For each product: read product.product, discount = p.get("discount",0), compute subtotal_excl = price_unit * qty * (1 - discount/100)
Processor->>Odoo: create POS order with discount-aware lines
Odoo-->>Processor: order created response
Processor-->>Router: order details
Router-->>Client: Success response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kairon/api/app/routers/pos.py(1 hunks)kairon/shared/pos/models.py(1 hunks)kairon/shared/pos/processor.py(5 hunks)tests/integration_test/services_test.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
kairon/api/app/routers/pos.py (1)
kairon/shared/pos/processor.py (2)
get_pos_products(360-379)invalidate_session(381-390)
kairon/shared/pos/processor.py (1)
kairon/api/app/routers/pos.py (1)
get_pos_products(123-129)
🪛 Ruff (0.14.8)
kairon/api/app/routers/pos.py
126-126: Unused function argument: current_user
(ARG001)
126-126: Do not perform function call Security in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
138-138: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
kairon/shared/pos/models.py (1)
20-20: LGTM!The discount field addition is clean and integrates well with the downstream processing logic in
processor.py.kairon/shared/pos/processor.py (1)
360-379: LGTM!The filtering logic is correct: an empty domain returns all products, while a non-empty domain filters by
available_in_pos. The exception handling is appropriate.tests/integration_test/services_test.py (1)
1935-1936: LGTM! Mock responses correctly support the new partner lookup flow.The added mock responses returning
{"result": []}appropriately simulate the "no existing partner found" scenario before partner creation. The test mock sequence now correctly reflects the updated POS order creation flow: partner search → partner creation → subsequent operations.Also applies to: 1967-1968, 2009-2010, 2053-2054, 2123-2124
| return_all: bool = Query(default=False, description="Flag to return all products or not"), | ||
| current_user: User = Security(Authentication.get_current_user_and_bot, scopes=ADMIN_ACCESS) | ||
| ): | ||
| products = pos_processor.get_pos_products(session_id) | ||
| products = pos_processor.get_pos_products(session_id, return_all) | ||
| return Response(data={"count": len(products), "data": products}) |
There was a problem hiding this comment.
Default value mismatch between router and processor.
The router defines return_all with default=False, but the processor's get_pos_products method has return_all: bool = True as its default. While this doesn't cause a runtime issue (since the router always passes the parameter), the inconsistency can lead to confusion.
Consider aligning the defaults:
- return_all: bool = Query(default=False, description="Flag to return all products or not"),
+ return_all: bool = Query(default=True, description="Flag to return all products or not"),Or update the processor to match:
-def get_pos_products(self, session_id: str, return_all: bool = True):
+def get_pos_products(self, session_id: str, return_all: bool = False):📝 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.
| return_all: bool = Query(default=False, description="Flag to return all products or not"), | |
| current_user: User = Security(Authentication.get_current_user_and_bot, scopes=ADMIN_ACCESS) | |
| ): | |
| products = pos_processor.get_pos_products(session_id) | |
| products = pos_processor.get_pos_products(session_id, return_all) | |
| return Response(data={"count": len(products), "data": products}) | |
| return_all: bool = Query(default=True, description="Flag to return all products or not"), | |
| current_user: User = Security(Authentication.get_current_user_and_bot, scopes=ADMIN_ACCESS) | |
| ): | |
| products = pos_processor.get_pos_products(session_id, return_all) | |
| return Response(data={"count": len(products), "data": products}) |
🧰 Tools
🪛 Ruff (0.14.8)
126-126: Unused function argument: current_user
(ARG001)
126-126: Do not perform function call Security in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
In kairon/api/app/routers/pos.py around lines 125 to 129, the Query parameter
return_all is declared with default=False while pos_processor.get_pos_products
uses default=True; make the defaults consistent by changing the router Query
default to True (return_all: bool = Query(default=True, ...)) so the router and
processor share the same default, or alternatively change the processor
signature to default=False — pick one option and update both function signature
and any related docs/comments to reflect the chosen default.
| @router.post("/invalidate/session", response_model=Response) | ||
| def invalidate_session_api(session_id: str = Query(..., description="Odoo session_id")): | ||
| """ | ||
| Invalidate an Odoo session_id by calling /web/session/destroy. | ||
| """ | ||
|
|
||
| result = pos_processor.invalidate_session(session_id) | ||
| return Response(message="Session invalidated successfully") |
There was a problem hiding this comment.
Critical: Missing authentication on session invalidation endpoint.
This endpoint lacks authentication, allowing anyone to invalidate any Odoo session without authorization. All other endpoints in this router require authentication via current_user: User = Security(Authentication.get_current_user_and_bot, scopes=ADMIN_ACCESS).
Apply this diff to add authentication:
@router.post("/invalidate/session", response_model=Response)
-def invalidate_session_api(session_id: str = Query(..., description="Odoo session_id")):
+def invalidate_session_api(
+ session_id: str = Query(..., description="Odoo session_id"),
+ current_user: User = Security(Authentication.get_current_user_and_bot, scopes=ADMIN_ACCESS)
+):
"""
Invalidate an Odoo session_id by calling /web/session/destroy.
"""
-
result = pos_processor.invalidate_session(session_id)
return Response(message="Session invalidated successfully")Additionally, the result variable is assigned but never used. You can either use it or remove the assignment:
- result = pos_processor.invalidate_session(session_id)
+ pos_processor.invalidate_session(session_id)
return Response(message="Session invalidated successfully")📝 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.
| @router.post("/invalidate/session", response_model=Response) | |
| def invalidate_session_api(session_id: str = Query(..., description="Odoo session_id")): | |
| """ | |
| Invalidate an Odoo session_id by calling /web/session/destroy. | |
| """ | |
| result = pos_processor.invalidate_session(session_id) | |
| return Response(message="Session invalidated successfully") | |
| @router.post("/invalidate/session", response_model=Response) | |
| def invalidate_session_api( | |
| session_id: str = Query(..., description="Odoo session_id"), | |
| current_user: User = Security(Authentication.get_current_user_and_bot, scopes=ADMIN_ACCESS) | |
| ): | |
| """ | |
| Invalidate an Odoo session_id by calling /web/session/destroy. | |
| """ | |
| pos_processor.invalidate_session(session_id) | |
| return Response(message="Session invalidated successfully") |
🧰 Tools
🪛 Ruff (0.14.8)
138-138: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🤖 Prompt for AI Agents
In kairon/api/app/routers/pos.py around lines 132 to 139, the
invalidate_session_api endpoint is missing authentication and assigns an unused
result; add the authenticated dependency parameter to the function signature
(current_user: User = Security(Authentication.get_current_user_and_bot,
scopes=ADMIN_ACCESS)) to require ADMIN access, and remove the unused result
assignment (or use it in the Response) so there are no unused variables; ensure
any required imports (User, Security, Authentication, ADMIN_ACCESS) remain
available in the file.
kairon/shared/pos/processor.py
Outdated
| def invalidate_session(self, session_id): | ||
| url = f"{self.__base_url}/web/session/destroy" | ||
| cookies = {"session_id": session_id} | ||
| payload = { | ||
| "jsonrpc": "2.0", | ||
| "method": "call", | ||
| "params": {} | ||
| } | ||
| response = requests.post(url, json=payload, cookies=cookies, timeout=30).json() | ||
| return response |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use existing error handling helper for consistency.
The invalidate_session method lacks error handling and doesn't follow the established pattern used by other methods in this class. The existing _raise_if_error helper (lines 23-38) should be used for consistent error handling and better user feedback.
Apply this diff to improve error handling:
def invalidate_session(self, session_id):
url = f"{self.__base_url}/web/session/destroy"
cookies = {"session_id": session_id}
payload = {
"jsonrpc": "2.0",
"method": "call",
"params": {}
}
- response = requests.post(url, json=payload, cookies=cookies, timeout=30).json()
- return response
+ resp = requests.post(url, json=payload, cookies=cookies, timeout=30)
+ data = self._raise_if_error(resp, "Session invalidation")
+ return data🤖 Prompt for AI Agents
In kairon/shared/pos/processor.py around lines 381 to 390, invalidate_session
currently posts and returns response JSON without using the class's error
helper; update it to call the existing _raise_if_error(response) after parsing
the JSON and before returning, and wrap the request in the same try/except (or
allow same exception flow) pattern used by other methods so network errors are
propagated consistently; preserve the URL, cookies, payload and timeout, parse
response = requests.post(...).json(), call self._raise_if_error(response) and
then return the response.
| existing = self.jsonrpc_call( | ||
| session_id=session_id, | ||
| model="res.partner", | ||
| method="create", | ||
| args=[{"name": "POS Customer", "customer_rank": 1}] | ||
| method="search_read", | ||
| args=[[["name", "=", "POS Customer"]]], | ||
| kwargs={"fields": ["id"], "limit": 1} | ||
| ) | ||
|
|
||
| if existing: | ||
| partner_id = existing[0]["id"] | ||
| else: | ||
| partner_id = self.jsonrpc_call( | ||
| session_id=session_id, | ||
| model="res.partner", | ||
| method="create", | ||
| args=[{ | ||
| "name": "POS Customer", | ||
| "customer_rank": 1 | ||
| }] | ||
| ) |
There was a problem hiding this comment.
Potential race condition in partner creation.
When partner_id is not provided, the code searches for an existing "POS Customer" partner and creates one if none exists. However, if multiple concurrent requests arrive without partner_id, both might execute the search before either creates the partner, resulting in duplicate "POS Customer" entries.
This is unlikely to occur frequently and would only result in duplicate partners (not data corruption), but it's worth noting.
To eliminate the race condition, consider using Odoo's get_or_create pattern or adding a unique constraint on the partner name, though this may require database-level changes beyond the scope of this PR.
🤖 Prompt for AI Agents
In kairon/shared/pos/processor.py around lines 515 to 534, the current
search-then-create flow for the "POS Customer" partner has a race condition
where concurrent requests can each find no partner and both create duplicates;
modify this to perform an atomic get-or-create: first attempt to search for the
partner, if not found attempt to create it inside a try/except that catches a
unique constraint or creation conflict and, on exception, re-query for the
existing partner and return that id; alternatively, if Odoo offers a
get_or_create helper, call that instead; ensure the final logic always returns a
single partner id and handles exceptions gracefully.
| discount = p.get("discount", 0) | ||
|
|
There was a problem hiding this comment.
Add validation for discount range.
The discount value is extracted from the product data (line 542) and applied to pricing calculations (line 583), but there's no validation to ensure the discount is within a reasonable range (e.g., 0-100). Invalid discount values (negative or > 100) could result in incorrect pricing, including negative subtotals or prices exceeding the original amount.
Add validation after extracting the discount:
for p in products:
product_id = p["product_id"]
qty = p["qty"]
discount = p.get("discount", 0)
+
+ if discount < 0 or discount > 100:
+ raise HTTPException(
+ status_code=400,
+ detail=f"Discount must be between 0 and 100, got {discount}"
+ )Also applies to: 583-600
🤖 Prompt for AI Agents
In kairon/shared/pos/processor.py around lines 542 to 543 (and similarly 583 to
600), the discount value is read from product data without validation; add a
validation step right after extracting discount to ensure it's numeric and
within 0-100 (inclusive). If discount is missing default to 0, convert/validate
type (int/float), and either clamp out-of-range values to the nearest bound or
raise/log and skip the product; ensure downstream pricing calculations use the
validated/clamped discount to prevent negative subtotals or prices greater than
the original amount. Also apply the same validation before any later
recalculation blocks in lines ~583-600 so all uses of discount are safe and
consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
kairon/shared/pos/processor.py (3)
381-394: Use existing error handling helper for consistency.The method doesn't follow the established error handling pattern used by other methods in this class (e.g.,
pos_loginat lines 74-75). The existing_raise_if_errorhelper should be used for consistent error handling.Apply this diff:
def invalidate_session(self, session_id): url = f"{self.__base_url}/web/session/destroy" cookies = {"session_id": session_id} payload = { "jsonrpc": "2.0", "method": "call", "params": {} } - try: - response = requests.post(url, json=payload, cookies=cookies, timeout=30).json() - return response - except Exception as e: - raise HTTPException(400, detail=f"Odoo error: {e}") + resp = requests.post(url, json=payload, cookies=cookies, timeout=30) + data = self._raise_if_error(resp, "Session invalidation") + return data
518-539: Potential race condition in partner creation.When
partner_idis not provided, concurrent requests could both execute the search before either creates the partner, resulting in duplicate "POS Customer" entries. This is unlikely and would only create duplicates (not data corruption).To eliminate the race, consider using an atomic get-or-create pattern or adding a unique constraint on partner name, though this may require database-level changes beyond the scope of this PR.
546-546: Add validation for discount range.The discount value is extracted without validation. Invalid values (negative or > 100) could result in incorrect pricing—negative discounts increase the price, while discounts > 100 produce negative subtotals.
Add validation after extracting the discount:
for p in products: product_id = p["product_id"] qty = p["qty"] discount = p.get("discount", 0) + + if discount < 0 or discount > 100: + raise HTTPException( + status_code=400, + detail=f"Discount must be between 0 and 100, got {discount}" + )
🧹 Nitpick comments (1)
tests/integration_test/services_test.py (1)
2638-2663: Remove debug print statement.Line 2657 contains a
print(data)statement that appears to be a debug artifact and should be removed before merging.Apply this diff:
data = response.json() - print(data) assert data["success"] is True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/shared/pos/processor.py(5 hunks)tests/integration_test/services_test.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration_test/services_test.py (1)
kairon/shared/utils.py (1)
Utility(93-2369)
kairon/shared/pos/processor.py (1)
kairon/api/app/routers/pos.py (1)
get_pos_products(123-129)
🪛 Ruff (0.14.8)
kairon/shared/pos/processor.py
391-391: Consider moving this statement to an else block
(TRY300)
392-392: Do not catch blind exception: Exception
(BLE001)
393-393: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
kairon/shared/pos/processor.py (1)
360-362: LGTM! Clean parameter addition for filtering.The
return_allparameter is correctly implemented with appropriate default behavior and domain filtering logic.tests/integration_test/services_test.py (1)
2665-2695: Remove debug print statement and clarify error handling semantics for Odoo responses.This test expects
success=Trueand "Session invalidated successfully" even when Odoo's/web/session/destroyendpoint returns an error. If this implements idempotent delete semantics (treating "already expired" as success), document this behavior in a comment. However, this contradictstest_invalidate_session_request_failurewhich expectssuccess=Falseon failure—ensure both tests align with the actual endpoint behavior.Additionally, remove the debug print statement at line 2689.
| @pytest.mark.asyncio | ||
| @responses.activate | ||
| def test_invalidate_session_request_failure(): | ||
| import requests | ||
| base = Utility.environment["pos"]["odoo"]["odoo_url"] | ||
| url = f"{base}/web/session/destroy" | ||
|
|
||
| responses.add( | ||
| responses.POST, | ||
| url, | ||
| body=requests.exceptions.ConnectionError("Connection refused"), | ||
| status=400 | ||
| ) | ||
|
|
||
| response = client.post( | ||
| f"/api/bot/{pytest.bot}/pos/odoo/invalidate/session?session_id={pytest.session_id}", | ||
| headers={"Authorization": pytest.token_type + " " + pytest.access_token}, | ||
| ) | ||
|
|
||
| data = response.json() | ||
| print(data) | ||
|
|
||
| assert not data["success"] | ||
| assert "Connection refused" in data["message"] | ||
| assert not data["data"] | ||
| assert data["error_code"] == 400 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move import to module level and remove debug print statement.
Line 2700 imports requests inside the test function. This import should be at the top of the file with other module-level imports for consistency and to avoid repeated imports.
Additionally, remove the debug print statement at line 2717.
Apply this diff:
@pytest.mark.asyncio
@responses.activate
def test_invalidate_session_request_failure():
- import requests
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/session/destroy"
responses.add(
responses.POST,
url,
body=requests.exceptions.ConnectionError("Connection refused"),
status=400
)
response = client.post(
f"/api/bot/{pytest.bot}/pos/odoo/invalidate/session?session_id={pytest.session_id}",
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)
data = response.json()
- print(data)
assert not data["success"]Note: Ensure requests is imported at the top of the file if not already present.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/integration_test/services_test.py around lines 2697 to 2723, the test
function imports requests locally and contains a debug print; move the "import
requests" statement to the module-level imports at the top of the file (adding
it if not already present) and remove the debug print statement inside the test
function so the test uses the top-level import and does not print debug output.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.