Skip to content

odoo fixes #2295

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.

Sign up for GitHub

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

Open
maheshsattala wants to merge 3 commits into
base: master
Choose a base branch
from
Open

odoo fixes #2295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb748c7
added test cases for the odoo apis.
Dec 12, 2025
5a4f60e
added test cases for the odoo apis.
Dec 12, 2025
d22d1bc
added test cases for the odoo apis.
Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion kairon/api/app/routers/pos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,18 @@ def reject_order(
@router.get("/product", response_model=Response)
async def get_pos_products(
session_id: str = Query(..., description="Odoo session_id"),
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})
Comment on lines +125 to 129
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.
"""

pos_processor.invalidate_session(session_id)
return Response(message="Session invalidated successfully")
1 change: 1 addition & 0 deletions kairon/shared/pos/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class ProductItem(BaseModel):
product_id: int
qty: int
unit_price: float
discount: float = 0


class POSOrderRequest(BaseModel):
Expand Down
44 changes: 37 additions & 7 deletions kairon/shared/pos/processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,9 @@ def jsonrpc_search_read(self, session_id: str, model: str, domain: List[list], f

return self.jsonrpc_call(session_id, model, "search_read", args=[domain], kwargs=kwargs)

def get_pos_products(self, session_id: str):
def get_pos_products(self, session_id: str, return_all: bool = True):
try:
domain = [["available_in_pos", "=", True]]
domain = [] if return_all else [["available_in_pos", "=", True]]

products = self.jsonrpc_call(
session_id=session_id,
Expand All @@ -378,6 +378,21 @@ def get_pos_products(self, session_id: str):
except Exception as e:
raise HTTPException(500, detail=f"Odoo error: {e}")

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}")


def toggle_product_in_pos(self, session_id: str, product_id: int) -> Dict[str, Any]:
"""
Toggle product.template.available_in_pos boolean using session.
Expand Down Expand Up @@ -501,19 +516,34 @@ def create_pos_order(self, session_id: str, products: list, partner_id: int = No
"""

if not partner_id:
partner_id = self.jsonrpc_call(
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
}]
)
Comment on lines +519 to +538
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


order_lines = []
total = 0.0

for p in products:
product_id = p["product_id"]
qty = p["qty"]
discount = p.get("discount", 0)

Comment on lines +546 to 547
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

product_data = self.jsonrpc_call(
session_id=session_id,
Expand Down Expand Up @@ -554,7 +584,7 @@ def create_pos_order(self, session_id: str, products: list, partner_id: int = No
if t["amount_type"] == "percent":
tax_total += (price_unit * qty) * (t["amount"] / 100)

subtotal_excl = price_unit * qty
subtotal_excl = (price_unit * qty) * (1 - (discount / 100))
subtotal_incl = subtotal_excl + tax_total

total += subtotal_incl
Expand All @@ -571,7 +601,7 @@ def create_pos_order(self, session_id: str, products: list, partner_id: int = No

"price_subtotal": subtotal_excl,
"price_subtotal_incl": subtotal_incl,
"discount": 0,
"discount": discount,
}])

pos_configs = self.jsonrpc_call(
Expand Down
97 changes: 97 additions & 0 deletions tests/integration_test/services_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1932,6 +1932,8 @@ def test_create_pos_order_product_not_found():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/dataset/call_kw"

responses.add(responses.POST, url, json={"result": []}, status=200)

responses.add(responses.POST, url, json={"result": 99}, status=200)

responses.add(responses.POST, url, json={"result": []}, status=200)
Expand Down Expand Up @@ -1962,6 +1964,8 @@ def test_create_pos_order_product_not_available():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/dataset/call_kw"

responses.add(responses.POST, url, json={"result": []}, status=200)

responses.add(responses.POST, url, json={"result": 98}, status=200)

responses.add(
Expand Down Expand Up @@ -2002,6 +2006,8 @@ def test_create_pos_order_no_pos_config():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/dataset/call_kw"

responses.add(responses.POST, url, json={"result": []}, status=200)

responses.add(responses.POST, url, json={"result": 99}, status=200)

responses.add(
Expand Down Expand Up @@ -2044,6 +2050,8 @@ def test_create_pos_order_no_payment_method():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/dataset/call_kw"

responses.add(responses.POST, url, json={"result": []}, status=200)

responses.add(responses.POST, url, json={"result": 99}, status=200)

responses.add(
Expand Down Expand Up @@ -2112,6 +2120,8 @@ def test_create_pos_order_without_partner():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/dataset/call_kw"

responses.add(responses.POST, url, json={"result": []}, status=200)

responses.add(responses.POST, url, json={"result": 500}, status=200)

responses.add(
Expand Down Expand Up @@ -2625,6 +2635,93 @@ def test_get_pos_products_success():
assert not data["message"]


@pytest.mark.asyncio
@responses.activate
def test_invalidate_session_success():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/session/destroy"

responses.add(
responses.POST,
url,
json={"result": True},
status=200
)

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 data["success"] is True
assert data["message"] == "Session invalidated successfully"
assert not data["data"]
assert data["error_code"] == 0


@pytest.mark.asyncio
@responses.activate
def test_invalidate_session_odoo_error_response():
base = Utility.environment["pos"]["odoo"]["odoo_url"]
url = f"{base}/web/session/destroy"

responses.add(
responses.POST,
url,
json={
"error": {
"code": 200,
"message": "Session expired"
}
},
status=200
)

response = client.post(
f"/api/bot/{pytest.bot}/pos/odoo/invalidate/session?session_id=invalid-session",
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)

data = response.json()
print(data)

assert data["success"] is True
assert data["error_code"] == 0
assert not data["data"]
assert data["message"] == "Session invalidated successfully"


@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

Comment on lines +2697 to +2723
Copy link

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


def test_secure_collection_crud_lifecycle():
# Step 1: Add a bot
add_bot_resp = client.post(
Expand Down
Loading