odoo multiple branch option #2320
Conversation
📝 WalkthroughWalkthroughPropagates Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as API Router (pos.py)
participant Processor as POS Processor
participant Odoo
Client->>Router: POST /create/branch {branch_name, street, city, state, company_id, session_id}
Router->>Processor: create_branch(session_id, branch_name, street, city, state, bot, user)
Processor->>Odoo: JSON-RPC create res.company branch (mapped state id, company_id)
Odoo-->>Processor: branch_id / error
Processor->>Processor: save_branch_details(bot, branch_name, company_id, user, pos_type)
Processor-->>Router: {branch_id, status}
Router-->>Client: HTTP response
sequenceDiagram
participant Client
participant Router as API Router (pos.py)
participant Processor as POS Processor
participant Odoo
Client->>Router: POST /api/orders/create {products, partner_id, company_id, session_id}
Router->>Processor: create_pos_order(session_id, products, partner_id, company_id)
Processor->>Processor: select POS config where config.company_id == company_id
Processor->>Odoo: retrieve open session/payment_method_ids for session_id
alt payment_method_ids present
Processor->>Odoo: create order with selected payment_method_id and company_id
else payment_method_ids absent
Processor->>Odoo: fetch available payment methods (fallback to first)
Processor->>Odoo: create order with resolved payment_method_id and company_id
end
Odoo-->>Processor: order created / error
Processor-->>Router: order response
Router-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kairon/shared/pos/processor.py (2)
577-595: Critical: Add error handling when no matching config is found.If no POS config matches the provided
company_id, the variableconfig_idremainsNone, which will cause failures when creating a session (line 603) or building the order payload (line 651).🐛 Proposed fix
config_id = None for config in pos_configs: comp_id = config["company_id"][0] if config.get("company_id") else False if company_id == comp_id: config_id = config["id"] break + + if config_id is None: + raise AppException(f"No POS Config found for company_id {company_id}")
610-644: Fix inconsistent payment method ID extraction.The payment method handling has a type inconsistency:
- Line 614:
payment_method_idsfrom an open session is a list of integers[1, 2, 3]- Lines 633-639:
search_readreturns a list of dictionaries[{"id": 1, ...}]- Line 644:
payment_method_ids[0]works for the session case but fails for the search_read case (returns a dict, not an int)🔧 Proposed fix
if not payment_method_ids: payment_method_ids = self.jsonrpc_call( session_id=session_id, model="pos.payment.method", method="search_read", args=[[[]]], kwargs={"limit": 1} ) + # search_read returns list of dicts, extract IDs + if payment_method_ids: + payment_method_ids = [pm["id"] for pm in payment_method_ids] if not payment_method_ids: raise HTTPException(status_code=400, detail="No POS payment methods found") payment_method_id = payment_method_ids[0]
🤖 Fix all issues with AI agents
In @kairon/shared/pos/processor.py:
- Line 497: Update the create_pos_order function signature to mark partner_id as
optional per PEP 484 (e.g., partner_id: Optional[int] or partner_id: int | None)
and add the corresponding import (from typing import Optional) if using
Optional; keep the default = None unchanged and ensure any internal uses handle
None.
In @tests/integration_test/services_test.py:
- Line 1683: The test assigns a global value to pytest.session_id which creates
cross-test coupling; remove the shared assignment and instead expose the
session_id via a fixture (e.g., a session-scoped fixture named
session_id_fixture) that returns the session id to dependent tests, or pass the
session_id explicitly between helper calls; update any tests referencing
pytest.session_id to accept and use the fixture (or receive the value from the
helper) so tests remain isolated and order-independent while retaining the same
session value where needed.
🧹 Nitpick comments (4)
kairon/pos/odoo/odoo_pos.py (1)
50-57: Fix spacing for consistency.The implementation is correct, but line 53 has inconsistent spacing compared to line 44.
📝 Minor style fix
- company_id = kwargs.get("company_id",1) + company_id = kwargs.get("company_id", 1)tests/integration_test/services_test.py (3)
1680-1680: Remove debug print statement.The
print(actual)statement should be removed as it's debug code that shouldn't be in production tests.🧹 Proposed cleanup
actual = response.json() - print(actual) assert actual["uid"] == 1
2350-2350: Remove debug print statement.The
print(data)statement should be removed as it's debug code.🧹 Proposed cleanup
data = response.json() - print(data)
2298-2356: Consider validating company_id in test assertions.The mock response at line 2305 includes
company_id: 2, which suggests company context is important for this flow. However, the test assertions don't verify that the company_id was properly handled or returned in the response. Consider adding an assertion to ensure the multi-tenant context is correctly propagated.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
kairon/api/app/routers/pos.pykairon/pos/odoo/odoo_pos.pykairon/shared/pos/models.pykairon/shared/pos/processor.pytests/integration_test/services_test.py
🧰 Additional context used
🧬 Code graph analysis (3)
kairon/api/app/routers/pos.py (2)
kairon/shared/pos/models.py (2)
POSOrderRequest(28-31)BranchRequest(16-20)kairon/pos/odoo/odoo_pos.py (1)
authenticate(24-39)
tests/integration_test/services_test.py (2)
kairon/shared/data/data_objects.py (1)
BotSettings(903-950)kairon/shared/utils.py (1)
Utility(93-2369)
kairon/shared/pos/processor.py (2)
kairon/exceptions.py (1)
AppException(1-3)kairon/api/app/routers/pos.py (1)
create_branch(48-60)
🪛 Ruff (0.14.10)
kairon/api/app/routers/pos.py
51-51: Unused function argument: current_user
(ARG001)
51-51: 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)
kairon/shared/pos/processor.py
497-497: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
588-588: Avoid specifying long messages outside the exception class
(TRY003)
722-722: Local variable branch_data is assigned to but never used
Remove assignment to unused variable branch_data
(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: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (11)
kairon/shared/pos/models.py (3)
16-20: LGTM! Consider adding field validation.The
BranchRequestmodel is well-defined. However, note that thecreate_branchimplementation inprocessor.pyexpects specific Indian state names. Consider adding a Pydantic validator or using an enum to restrictstateto valid values.
14-14: LGTM!The addition of
company_idwith a sensible default maintains backward compatibility while enabling multi-tenant support.
31-31: LGTM!Consistent with the
LoginRequestchanges, enabling company-specific order routing.kairon/pos/odoo/odoo_pos.py (2)
24-39: LGTM!The
authenticatemethod correctly extracts and propagatescompany_idto the appropriate list generation methods based onpage_type.
41-48: LGTM!The dynamic URL construction with
cids={company_id}enables company-specific product listings.kairon/api/app/routers/pos.py (4)
6-6: LGTM!Import addition supports the new branch creation endpoint.
19-29: LGTM!The
company_idis correctly propagated from the request to the POS authentication flow.
47-60: LGTM!The new branch creation endpoint follows established patterns in this module. The static analysis warnings about
current_userandSecurityare false positives—this is the standard FastAPI dependency injection pattern for authentication.
105-114: LGTM!The
company_idis correctly propagated to the order creation flow, enabling multi-tenant order handling.tests/integration_test/services_test.py (2)
1687-1726: LGTM! Well-structured branch creation test.The test properly mocks the
POSProcessor.create_branchmethod, verifies the response structure, and asserts the method was called with the correct parameters.
4165-4170: LGTM! Good backward compatibility test update.Properly updates the existing test to reflect the new
company_idparameter in theauthenticatemethod signature, ensuring backward compatibility with the default value.
| )[0] | ||
|
|
||
| def create_pos_order(self, session_id: str, products: list, partner_id: int = None): | ||
| def create_pos_order(self, session_id: str, products: list, partner_id: int = None, company_id: int = 1): |
There was a problem hiding this comment.
Fix type hint for optional parameter.
The partner_id parameter should be explicitly typed as optional per PEP 484.
🔧 Suggested fix
- def create_pos_order(self, session_id: str, products: list, partner_id: int = None, company_id: int = 1):
+ def create_pos_order(self, session_id: str, products: list, partner_id: int | None = None, company_id: int = 1):🧰 Tools
🪛 Ruff (0.14.10)
497-497: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In @kairon/shared/pos/processor.py at line 497, Update the create_pos_order
function signature to mark partner_id as optional per PEP 484 (e.g., partner_id:
Optional[int] or partner_id: int | None) and add the corresponding import (from
typing import Optional) if using Optional; keep the default = None unchanged and
ensure any internal uses handle None.
kairon/shared/pos/processor.py
Outdated
| def create_branch(self, session_id: str, branch_name: str, street: str, city: str, state: str): | ||
| INDIA_STATE_MAP = { | ||
| "Andaman and Nicobar": 577, | ||
| "Andhra Pradesh": 578, | ||
| "Arunachal Pradesh": 579, | ||
| "Assam": 580, | ||
| "Bihar": 581, | ||
| "Chattisgarh": 583, | ||
| "Chandigarh": 582, | ||
| "Daman and Diu": 585, | ||
| "Delhi": 586, | ||
| "Dadra and Nagar Haveli": 584, | ||
| "Goa": 587, | ||
| "Gujarat": 588, | ||
| "Himachal Pradesh": 590, | ||
| "Haryana": 589, | ||
| "Jharkhand": 592, | ||
| "Jammu and Kashmir": 591, | ||
| "Karnataka": 593, | ||
| "Kerala": 594, | ||
| "Lakshadweep": 595, | ||
| "Maharashtra": 597, | ||
| "Meghalaya": 599, | ||
| "Manipur": 598, | ||
| "Madhya Pradesh": 596, | ||
| "Mizoram": 600, | ||
| "Nagaland": 601, | ||
| "Odisha": 602, | ||
| "Punjab": 604, | ||
| "Puducherry": 603, | ||
| "Rajasthan": 605, | ||
| "Sikkim": 606, | ||
| "Tamil Nadu": 607, | ||
| "Tripura": 609, | ||
| "Telangana": 608, | ||
| "Uttarakhand": 611, | ||
| "Uttar Pradesh": 610, | ||
| "West Bengal": 612 | ||
| } | ||
| state_id=INDIA_STATE_MAP[state] | ||
| branch_data = self.jsonrpc_call( | ||
| session_id=session_id, | ||
| model="res.company", | ||
| method="create", | ||
| args= [ | ||
| { | ||
| "name": branch_name, | ||
| "parent_id": 1, | ||
| "currency_id": 1, | ||
| "country_id": 104, | ||
| "state_id": state_id, | ||
| "street": street, | ||
| "city": city, | ||
| "active": True | ||
| } | ||
| ], | ||
| kwargs= {} | ||
| ) | ||
|
|
||
| return {"status": "created"} |
There was a problem hiding this comment.
Add validation and error handling for branch creation.
The create_branch method has several issues:
- Line 721: Will raise
KeyErrorif an invalid state name is provided - Lines 683-720: The large state mapping dictionary should be a class/module-level constant, not defined inside the method
- Line 722: The
branch_dataresult is never used—should either return it or verify the creation succeeded - Hardcoded values:
parent_id=1,currency_id=1,country_id=104are hardcoded without documentation - Geographic limitation: Method only supports Indian states without documenting this constraint
♻️ Proposed improvements
Move the state map to class level and add validation:
+class POSProcessor:
+
+ INDIA_STATE_MAP = {
+ "Andaman and Nicobar": 577,
+ # ... rest of states ...
+ }
+
def create_branch(self, session_id: str, branch_name: str, street: str, city: str, state: str):
- INDIA_STATE_MAP = {
- "Andaman and Nicobar": 577,
- # ... all states ...
- }
- state_id=INDIA_STATE_MAP[state]
+ """
+ Create a new branch (res.company) in Odoo.
+ Note: Currently supports Indian states only.
+ """
+ if state not in self.INDIA_STATE_MAP:
+ raise AppException(f"Invalid state '{state}'. Must be a valid Indian state.")
+
+ state_id = self.INDIA_STATE_MAP[state]
+
branch_data = self.jsonrpc_call(
session_id=session_id,
model="res.company",
method="create",
args= [
{
"name": branch_name,
"parent_id": 1,
"currency_id": 1,
"country_id": 104,
"state_id": state_id,
"street": street,
"city": city,
"active": True
}
],
kwargs= {}
)
- return {"status": "created"}
+ return {"status": "created", "branch_id": branch_data}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
722-722: Local variable branch_data is assigned to but never used
Remove assignment to unused variable branch_data
(F841)
| print(actual) | ||
| assert actual["uid"] == 1 | ||
| assert actual["session_id"] == "fake-session-id-123" | ||
| pytest.session_id = actual["session_id"] |
There was a problem hiding this comment.
Test isolation concern: shared session state.
Setting pytest.session_id creates a dependency between tests. If tests run in a different order or isolation, this could cause failures. Consider using fixtures or ensuring tests can run independently.
🤖 Prompt for AI Agents
In @tests/integration_test/services_test.py at line 1683, The test assigns a
global value to pytest.session_id which creates cross-test coupling; remove the
shared assignment and instead expose the session_id via a fixture (e.g., a
session-scoped fixture named session_id_fixture) that returns the session id to
dependent tests, or pass the session_id explicitly between helper calls; update
any tests referencing pytest.session_id to accept and use the fixture (or
receive the value from the helper) so tests remain isolated and
order-independent while retaining the same session value where needed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kairon/shared/pos/processor.py (1)
631-665: Type mismatch inpayment_method_idextraction logic.There's inconsistent handling of
payment_method_ids:
- Line 635: Extracts from session as a list of IDs (integers)
- Lines 653-660: Falls back to
search_readwhich returns a list of dictionaries- Line 665: Assumes
payment_method_ids[0]is the ID directlyWhen
payment_method_idscomes from the session, it's a list of integers. When it comes fromsearch_read, it's a list of dicts with anidkey. This will cause incorrect behavior.Proposed fix
if not payment_method_ids: - payment_method_ids = self.jsonrpc_call( + payment_methods = self.jsonrpc_call( session_id=session_id, model="pos.payment.method", method="search_read", args=[[[]]], - kwargs={"limit": 1} + kwargs={"fields": ["id"], "limit": 1} ) + if payment_methods: + payment_method_ids = [pm["id"] for pm in payment_methods] if not payment_method_ids: raise HTTPException(status_code=400, detail="No POS payment methods found") payment_method_id = payment_method_ids[0]
🤖 Fix all issues with AI agents
In @kairon/shared/pos/processor.py:
- Around line 611-616: The loop over pos_configs can leave config_id as None
when no entry matches company_id; after the loop (before any use like the
subsequent domain filter), add an explicit check that raises a clear exception
(e.g., ValueError or a domain-specific exception) if config_id is None,
including the company_id in the message so callers know which company had no POS
config; ensure this check is placed right after the for-loop that sets config_id
and before any code that uses config_id.
- Around line 754-756: Replace the hardcoded parent_id: 1, currency_id: 1, and
country_id: 104 with values read from the existing POS Odoo config object
(Utility.environment["pos"]["odoo"]); update the payload construction in the pos
processor where these keys are set to use
Utility.environment["pos"]["odoo"]["parent_id"], ["currency_id"], and
["country_id"] (or sensible defaults if keys are missing) so different
deployments can override them via config rather than using fixed IDs.
🧹 Nitpick comments (10)
tests/integration_test/services_test.py (3)
1662-1685: Remove@pytest.mark.asynciodecorator and debug print statement.The function is synchronous (
def), so@pytest.mark.asynciois unnecessary and misleading. Also, theprint(actual)on line 1681 appears to be a debug artifact.🧹 Suggested cleanup
-@pytest.mark.asyncio @responses.activate def test_pos_login_with_cid(): bot_settings = BotSettings.objects(bot=pytest.bot).get() bot_settings.pos_enabled = True bot_settings.save() base = Utility.environment["pos"]["odoo"]["odoo_url"] auth = f"{base}/web/session/authenticate" responses.add(responses.POST, auth, json={"result": {"uid": 1}}, headers={"Set-Cookie": "session_id=fake-session-id-123; Path=/;"}, status=200) response = client.post( f"/api/bot/{pytest.bot}/pos/odoo/login", json={"client_name": "Test Client", "company_id": 2}, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) actual = response.json() - print(actual) assert actual["uid"] == 1 assert actual["session_id"] == "fake-session-id-123" pytest.session_id = actual["session_id"] assert actual["url"] == 'http://localhost:8080/web#action=388&model=product.template&view_type=kanban&cids=2&menu_id=233'
1731-1756: Consider adding status code assertion.The test mocks an
HTTPExceptionwithstatus_code=404, but only verifies the error message. Adding a status code assertion would strengthen the test by confirming the HTTP status is propagated correctly.✅ Suggested improvement
actual = response.json() + assert response.status_code == 404 assert actual["message"] == "Error in creating branch"
2319-2386: Remove decorator, debug print, and consider test isolation.Three issues:
@pytest.mark.asynciois unnecessary for this synchronous function.print(data)on line 2380 is a debug artifact.- This test relies on
pytest.session_idset bytest_pos_login_with_cid. If tests run in isolation or a different order, this test will fail. Consider setting up the session_id locally or using a fixture.🧹 Suggested cleanup for decorator and print
-@pytest.mark.asyncio @responses.activate def test_create_pos_order_success_with_cid(): base = Utility.environment["pos"]["odoo"]["odoo_url"] url = f"{base}/web/dataset/call_kw" # ... mocks ... response = client.post( f"/api/bot/{pytest.bot}/pos/odoo/pos_order?session_id={pytest.session_id}", json=payload, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) data = response.json() - print(data) assert data["success"]kairon/shared/pos/data_objects.py (1)
22-22: Consider adding schema documentation or validation for thebranchesfield.The
branchesfield accepts any dictionary structure. While this provides flexibility, it may lead to inconsistent data. Based on thesave_branch_detailsmethod in the processor, expected keys arebranch_nameandcompany_id. Consider documenting the expected schema or using anEmbeddedDocumentfor type safety.tests/unit_test/pos_test.py (1)
131-143: Consider asserting the specific status code and message for invalid state.The test verifies that an
HTTPExceptionis raised but doesn't validate the status code (expected400) or error message. This could mask regressions if the error handling changes.Proposed improvement
def test_create_branch_invalid_state(): service = POSProcessor() - with pytest.raises(HTTPException): + with pytest.raises(HTTPException) as exc: service.create_branch( session_id="session123", branch_name="Unknown Branch", street="Some Street", city="Some City", state="InvalidState", bot="test_bot", user="test_user" ) + + assert exc.value.status_code == 400 + assert "Invalid state" in exc.value.detailkairon/api/app/routers/pos.py (1)
47-62: Fix inconsistent spacing in keyword arguments.Lines 59-60 use
bot =anduser =with spaces around=, which is inconsistent with PEP 8 style for keyword arguments and with the rest of the codebase.Proposed fix
result = pos_processor.create_branch( session_id=session_id, branch_name=req.branch_name, street=req.street, city=req.city, state=req.state, - bot = current_user.get_bot(), - user = current_user.get_user() + bot=current_user.get_bot(), + user=current_user.get_user() )kairon/shared/pos/processor.py (4)
139-158: Remove unused parametersuserandpos_type.The
userandpos_typeparameters are declared but never used in the method body. Either remove them or implement the intended functionality (e.g., logging/auditing withuser).Proposed fix
@staticmethod -def save_branch_details(bot: str, branch_name: str, company_id: int, user: str, pos_type: POSType = POSType.odoo.value): +def save_branch_details(bot: str, branch_name: str, company_id: int): record = POSClientDetails.objects(bot=bot, pos_type=POSType.odoo.value).first() if not record: raise AppException("No POS client configuration found for this bot.")Then update the caller at line 768:
-POSProcessor.save_branch_details(bot, branch_name, branch_data, user) +POSProcessor.save_branch_details(bot, branch_name, branch_data)
742-745: Useraise ... fromto preserve exception context.Per B904, when re-raising an exception within an
exceptblock, useraise ... from errorraise ... from Noneto properly chain exceptions and preserve the traceback.Proposed fix
try: state_id = INDIA_STATE_MAP[state] except KeyError: - raise HTTPException(status_code=400, detail=f"Invalid state: {state}") + raise HTTPException(status_code=400, detail=f"Invalid state: {state}") from None
518-518: Use explicitOptionaltype hint instead of implicitNonedefault.PEP 484 recommends using explicit
Optional[int]orint | Nonewhen a parameter can beNone.Proposed fix
-def create_pos_order(self, session_id: str, products: list, partner_id: int = None, company_id: int = 1): +def create_pos_order(self, session_id: str, products: list, partner_id: int | None = None, company_id: int = 1):
703-741: Consider movingINDIA_STATE_MAPto a constants file.The state map is defined inside the method, making it harder to maintain and test. Moving it to
kairon/shared/pos/constants.pywould improve maintainability and allow reuse.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
kairon/api/app/routers/pos.pykairon/shared/pos/data_objects.pykairon/shared/pos/processor.pytests/integration_test/services_test.pytests/unit_test/pos_test.py
🧰 Additional context used
🧬 Code graph analysis (3)
kairon/api/app/routers/pos.py (3)
kairon/shared/pos/models.py (4)
LoginRequest(11-14)ClientRequest(7-8)POSOrderRequest(28-31)BranchRequest(16-20)kairon/pos/odoo/odoo_pos.py (1)
authenticate(24-39)kairon/shared/pos/processor.py (1)
create_branch(703-769)
kairon/shared/pos/processor.py (4)
kairon/shared/pos/constants.py (1)
POSType(4-5)kairon/shared/pos/data_objects.py (1)
POSClientDetails(13-37)kairon/exceptions.py (1)
AppException(1-3)kairon/api/app/routers/pos.py (1)
create_branch(48-62)
tests/integration_test/services_test.py (1)
kairon/shared/data/data_objects.py (1)
BotSettings(903-950)
🪛 Ruff (0.14.10)
kairon/api/app/routers/pos.py
51-51: 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)
kairon/shared/pos/processor.py
140-140: Unused static method argument: user
(ARG004)
140-140: Unused static method argument: pos_type
(ARG004)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
518-518: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
609-609: Avoid specifying long messages outside the exception class
(TRY003)
745-745: 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: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (7)
tests/integration_test/services_test.py (3)
102-102: LGTM!The
HTTPExceptionimport is appropriately added to support mocking HTTP errors in the new failure test case.
1688-1729: LGTM!The test properly mocks the processor, validates the response structure, and verifies that the correct parameters are passed to the
create_branchmethod.
4195-4200: LGTM!The assertion is correctly updated to verify that
company_id=1is passed to theauthenticatemethod, consistent with the PR's objective of propagatingcompany_idthrough POS flows.tests/unit_test/pos_test.py (2)
75-105: LGTM! Good test coverage for the success path.The test correctly mocks both
jsonrpc_callandsave_branch_details, and validates the return payload and method invocations.
107-129: LGTM! Proper error handling verification.The test correctly validates that a
404HTTPException is raised whenjsonrpc_callreturnsNone, and ensuressave_branch_detailsis not called on failure.kairon/api/app/routers/pos.py (2)
27-27: LGTM!The
company_idpropagation to the authenticate call aligns with the multi-tenant branch support.
113-114: LGTM!The
company_idis correctly propagated tocreate_pos_orderfor multi-tenant order creation.
| config_id = None | ||
| for config in pos_configs: | ||
| comp_id = config["company_id"][0] if config.get("company_id") else False | ||
| if company_id == comp_id: | ||
| config_id = config["id"] | ||
| break |
There was a problem hiding this comment.
Handle the case when no matching POS config is found for the company_id.
If no config matches the provided company_id, config_id remains None, which will cause issues in subsequent operations (e.g., line 624 where it's used in a domain filter). This should raise an explicit error.
Proposed fix
config_id = None
for config in pos_configs:
comp_id = config["company_id"][0] if config.get("company_id") else False
if company_id == comp_id:
config_id = config["id"]
break
+
+ if config_id is None:
+ raise HTTPException(
+ status_code=404,
+ detail=f"No POS config found for company_id {company_id}"
+ )📝 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.
| config_id = None | |
| for config in pos_configs: | |
| comp_id = config["company_id"][0] if config.get("company_id") else False | |
| if company_id == comp_id: | |
| config_id = config["id"] | |
| break | |
| config_id = None | |
| for config in pos_configs: | |
| comp_id = config["company_id"][0] if config.get("company_id") else False | |
| if company_id == comp_id: | |
| config_id = config["id"] | |
| break | |
| if config_id is None: | |
| raise HTTPException( | |
| status_code=404, | |
| detail=f"No POS config found for company_id {company_id}" | |
| ) |
🤖 Prompt for AI Agents
In @kairon/shared/pos/processor.py around lines 611 - 616, The loop over
pos_configs can leave config_id as None when no entry matches company_id; after
the loop (before any use like the subsequent domain filter), add an explicit
check that raises a clear exception (e.g., ValueError or a domain-specific
exception) if config_id is None, including the company_id in the message so
callers know which company had no POS config; ensure this check is placed right
after the for-loop that sets config_id and before any code that uses config_id.
| "parent_id": 1, | ||
| "currency_id": 1, | ||
| "country_id": 104, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the specific file and lines in question
cd /repo && cat -n kairon/shared/pos/processor.py | sed -n '750,760p'Repository: digiteinfotech/kairon
Length of output: 120
🏁 Script executed:
# Search for usage patterns of these IDs across the codebase
rg -n "parent_id|currency_id|country_id" --type py -B2 -A2Repository: digiteinfotech/kairon
Length of output: 2602
🏁 Script executed:
# Check if there's any configuration file or module for Odoo settings
fd -e py -e json -e yaml -e toml -e conf | xargs rg -l "parent_id|currency_id|country_id|odoo.*config" 2>/dev/null | head -20Repository: digiteinfotech/kairon
Length of output: 489
🏁 Script executed:
cat -n kairon/shared/pos/processor.py | sed -n '740,770p'Repository: digiteinfotech/kairon
Length of output: 1290
🏁 Script executed:
# Search for the function containing these lines and understand the context
rg -n "def.*\(" kairon/shared/pos/processor.py | head -20Repository: digiteinfotech/kairon
Length of output: 1651
🏁 Script executed:
# Check if there's any Odoo configuration or settings file
rg -n "odoo|currency_id|country_id" kairon/shared/pos/processor.py -B5 -A5Repository: digiteinfotech/kairon
Length of output: 4795
🏁 Script executed:
# Look for configuration files that might contain these ID mappings
fd -type f \( -name "*.py" -o -name "*.json" -o -name "*.yaml" -o -name "*.toml" -o -name "*.conf" \) | xargs rg -l "config|setting" | grep -E "(config|setting|constant|env)" | head -10Repository: digiteinfotech/kairon
Length of output: 852
🏁 Script executed:
# Look for configuration files and environment setup
rg -n "parent_id|currency_id|country_id|odoo.*config" kairon/ --type py | grep -v test | grep -v ".pyc"Repository: digiteinfotech/kairon
Length of output: 478
🏁 Script executed:
# Check the Utility.environment setup to understand config structure
rg -n "environment\[.*pos" kairon/ --type py | head -20Repository: digiteinfotech/kairon
Length of output: 480
🏁 Script executed:
# Look for any dynamic ID resolution or query patterns for these entities
rg -n "search.*parent|search.*currency|search.*country" kairon/shared/pos/ --type py -B2 -A2Repository: digiteinfotech/kairon
Length of output: 47
🏁 Script executed:
# Check if there are similar hardcoded patterns elsewhere in the codebase
rg -n "jsonrpc_call.*create" kairon/shared/pos/processor.py --type py -B3 -A3Repository: digiteinfotech/kairon
Length of output: 458
🏁 Script executed:
# Check if there's any documentation or comments about India-specificity
rg -n "india|104|country_id" kairon/shared/pos/processor.py -B5 -A5 | head -50Repository: digiteinfotech/kairon
Length of output: 467
🏁 Script executed:
# Check the create_branch function signature and docstring
sed -n '703,720p' kairon/shared/pos/processor.pyRepository: digiteinfotech/kairon
Length of output: 729
🏁 Script executed:
# Search for any test files that might show intended usage
rg -n "create_branch|country_id.*104" tests/ --type pyRepository: digiteinfotech/kairon
Length of output: 1195
🏁 Script executed:
# Check Utility.environment structure to see how config is loaded
rg -n "class Utility" kairon/shared/utils.py -A 30 | head -50Repository: digiteinfotech/kairon
Length of output: 1210
🏁 Script executed:
# Look for environment configuration files
fd -type f -name "*.yaml" -o -name "*.yml" -o -name "*.json" | xargs rg -l "pos.*odoo\|country\|currency" 2>/dev/null | head -10Repository: digiteinfotech/kairon
Length of output: 235
🏁 Script executed:
# Check if the IDs are mentioned in documentation or readme
find . -name "README*" -o -name "*.md" | xargs rg -l "104\|country_id\|create_branch" 2>/dev/nullRepository: digiteinfotech/kairon
Length of output: 49
🏁 Script executed:
# Look at the full create_branch function to understand the context better
sed -n '703,770p' kairon/shared/pos/processor.pyRepository: digiteinfotech/kairon
Length of output: 2306
🏁 Script executed:
# Check if there's any configuration that might override these values
rg -n "parent_id.*=|currency_id.*=|country_id.*=" kairon/ --type py | grep -v test | grep -v ".pyc"Repository: digiteinfotech/kairon
Length of output: 183
🏁 Script executed:
# Look for Odoo documentation or comments about these standard IDs
rg -n "INR|india|odoo" kairon/shared/pos/processor.py -iRepository: digiteinfotech/kairon
Length of output: 1795
Make country_id, currency_id, and parent_id configurable instead of hardcoded.
The hardcoded values parent_id: 1, currency_id: 1, and country_id: 104 are specific to a particular Odoo instance setup. Different deployments may have different ID assignments for the parent company, default currency, and country. Since the codebase already uses Utility.environment["pos"]["odoo"] for configuration (odoo_url, odoo_domain, odoo_master_password), these IDs should be moved to the same configuration mechanism rather than hardcoded. Without this, the code will fail or create incorrect associations when deployed to Odoo instances with different configurations.
🤖 Prompt for AI Agents
In @kairon/shared/pos/processor.py around lines 754 - 756, Replace the hardcoded
parent_id: 1, currency_id: 1, and country_id: 104 with values read from the
existing POS Odoo config object (Utility.environment["pos"]["odoo"]); update the
payload construction in the pos processor where these keys are set to use
Utility.environment["pos"]["odoo"]["parent_id"], ["currency_id"], and
["country_id"] (or sensible defaults if keys are missing) so different
deployments can override them via config rather than using fixed IDs.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.