Multiple branchs option in odoo #2322
Conversation
📝 WalkthroughWalkthroughThe changes introduce branch creation functionality to the POS system and propagate company_id throughout authentication, order creation, and configuration flows. New models define branch request structure, processors handle branch creation with state validation and persistence, and API endpoints expose the branch creation capability. Changes
Sequence DiagramssequenceDiagram
actor User
participant API as POS API
participant Processor as POSProcessor
participant Odoo as Odoo POS
participant DB as POSClientDetails
User->>API: POST /create/branch (BranchRequest)
API->>Processor: create_branch(session_id, branch_name, street, city, state, bot, user)
Processor->>Processor: validate state against INDIA_STATE_MAP
alt Invalid State
Processor-->>API: HTTPException 400
else Valid State
Processor->>Odoo: jsonrpc_call (create res.company)
alt Branch Creation Failed
Odoo-->>Processor: None
Processor-->>API: HTTPException 404
else Branch Created Successfully
Odoo-->>Processor: branch_id
Processor->>Processor: save_branch_details(bot, branch_name, company_id, user)
Processor->>DB: update POSClientDetails (append to branches)
DB-->>Processor: success
Processor-->>API: {branch_id, status}
API-->>User: Response 200
end
end
sequenceDiagram
actor User
participant API as POS API
participant Processor as POSProcessor
participant Odoo as Odoo POS
User->>API: POST /login (LoginRequest with company_id)
API->>Processor: authenticate(credentials, company_id)
Processor->>Odoo: authenticate(company_id)
Odoo-->>Processor: session details
Processor->>Odoo: products_list(company_id)
Note over Odoo: URL: cids={company_id}
Odoo-->>Processor: products
Processor->>Odoo: orders_list(company_id)
Note over Odoo: URL: cids={company_id}
Odoo-->>Processor: orders
Processor-->>API: authenticated session
API-->>User: login success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
🤖 Fix all issues with AI agents
In `@kairon/shared/pos/processor.py`:
- Around line 653-665: search_read returns a list of dicts, but the code assigns
payment_method_ids[0] (a dict) to payment_method_id and later passes it to
statement_ids; change the assignment to extract the actual integer id from the
first dict returned by jsonrpc_call (e.g., pull the 'id' field from
payment_method_ids[0]) and keep the existing empty-check; ensure
payment_method_id is an int before using it in creating the order/statement_ids.
- Around line 611-616: The loop over pos_configs may leave config_id as None if
no config matches company_id; after the loop (after the for config in
pos_configs block) add a validation that checks if config_id is None and handle
it explicitly (e.g., log a clear error including the company_id and either raise
a specific exception or return an error response/None to abort POS session
creation) so downstream code that creates the POS session does not operate with
a None config_id; reference the variables config_id, pos_configs, company_id and
the post-loop validation point for the change.
In `@tests/integration_test/services_test.py`:
- Around line 1731-1756: The test test_create_branch_failure is missing an
assertion that the HTTP status code is propagated; update the test to assert
response.status_code == 404 (using the response object returned by client.post)
in addition to the existing message assertion so the test verifies both the
error payload and the HTTP 404 status from POSProcessor.create_branch.
In `@tests/unit_test/pos_test.py`:
- Around line 147-160: The mock for POSClientDetails.objects is incorrect for
the queryset-first call; instead of mocking return_value.first, set the queryset
chain so .filter(...).first() (or .first() directly) returns None: update the
test_save_branch_details_no_pos_client_config to configure
mock_objects.filter.return_value.first.return_value = None (and if your code
calls POSClientDetails.objects(...).first(), set
mock_objects.return_value.first.return_value = None) so that
POSProcessor.save_branch_details receives a None POS client configuration and
the AppException is raised.
🧹 Nitpick comments (11)
kairon/shared/pos/data_objects.py (1)
22-22: Consider adding a default value to avoidNonevs empty list ambiguity.Without a
default, the field will beNonewhen not explicitly set. If consuming code (e.g.,save_branch_detailsin the processor) expects to iterate or append tobranches, this could causeTypeError.♻️ Proposed fix
- branches = ListField(DictField()) + branches = ListField(DictField(), default=list)Additionally, consider adding validation in the
validate()method if branches must contain specific keys (e.g.,branch_id,name,company_id) to ensure data consistency.tests/integration_test/services_test.py (2)
1662-1685: Remove@pytest.mark.asynciodecorator and debug print statement.Two issues:
- The
@pytest.mark.asynciodecorator is used but the function is notasync. Either make the functionasync defor remove the decorator.- Line 1681 has a
print(actual)statement that appears to be a debug artifact.Suggested fix
-@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
2319-2386: Remove@pytest.mark.asynciodecorator and debug print statement.Same issues as
test_pos_login_with_cid:
- The
@pytest.mark.asynciodecorator has no effect on a non-async function.- Line 2380 has a
print(data)debug statement.Suggested fix
-@pytest.mark.asyncio `@responses.activate` def test_create_pos_order_success_with_cid(): # ... test body ... data = response.json() - print(data) assert data["success"]kairon/shared/pos/models.py (1)
16-20: Consider adding field validation forBranchRequest.The
BranchRequestmodel lacks validation constraints. Other parts of the codebase (e.g.,save_client_details) validate for empty strings. Consider adding validators or usingFieldwithmin_lengthto ensure branch_name, street, city, and state are non-empty.♻️ Suggested improvement
+from pydantic import BaseModel, Field +from typing import Optional, List + +from kairon.shared.pos.constants import PageType + + class BranchRequest(BaseModel): - branch_name: str - street: str - city: str - state: str + branch_name: str = Field(..., min_length=1) + street: str = Field(..., min_length=1) + city: str = Field(..., min_length=1) + state: str = Field(..., min_length=1)kairon/shared/pos/processor.py (4)
139-158: Remove unused parameters or document their intended use.The
userandpos_typeparameters are unused insave_branch_details. Either remove them or implement their intended functionality.♻️ 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.")Note: If you remove these parameters, update the caller at line 768 accordingly.
742-745: Chain the exception for proper traceback.Use
raise ... from errto preserve the exception chain, as recommended by static analysis (B904).♻️ Proposed fix
try: state_id = INDIA_STATE_MAP[state] - except KeyError: - raise HTTPException(status_code=400, detail=f"Invalid state: {state}") + except KeyError as err: + raise HTTPException(status_code=400, detail=f"Invalid state: {state}") from err
703-741: Consider movingINDIA_STATE_MAPto module level or constants file.Defining this large dictionary inside the method means it's recreated on every call. Moving it to module level or a constants file improves performance and maintainability.
518-518: Use explicitOptionaltype hint.Per PEP 484 and static analysis hint (RUF013), use explicit
Optional[int]orint | Noneinstead of implicit optional with defaultNone.♻️ 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):kairon/pos/odoo/odoo_pos.py (1)
53-53: Minor: Add space after comma for consistency.♻️ Suggested fix
- company_id = kwargs.get("company_id",1) + company_id = kwargs.get("company_id", 1)kairon/api/app/routers/pos.py (1)
47-62: Minor formatting: Fix inconsistent spacing around=in keyword arguments.Lines 59-60 have spaces around
=in keyword arguments, which is inconsistent with PEP 8 style for function calls.♻️ 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() )tests/unit_test/pos_test.py (1)
133-145: Remove duplicate test case.
test_create_branch_invalid_state(lines 133-145) andtest_create_branch_invalid_state_raises_400(lines 162-183) test the same scenario. The latter provides more detailed assertions. Consider removing the first test to avoid redundancy.♻️ Proposed change
Remove
test_create_branch_invalid_state(lines 133-145) and keep onlytest_create_branch_invalid_state_raises_400which has more comprehensive assertions.Also applies to: 162-183
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kairon/api/app/routers/pos.pykairon/pos/odoo/odoo_pos.pykairon/shared/pos/data_objects.pykairon/shared/pos/models.pykairon/shared/pos/processor.pytests/integration_test/services_test.pytests/unit_test/pos_test.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration_test/services_test.py (1)
kairon/shared/data/data_objects.py (1)
BotSettings(903-950)
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)
🪛 Ruff (0.14.11)
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 (10)
kairon/shared/pos/data_objects.py (1)
3-3: LGTM!Import correctly updated to include
ListFieldwhich is required for the newbranchesfield.tests/integration_test/services_test.py (3)
102-102: LGTM!The
HTTPExceptionimport is appropriately added to support thetest_create_branch_failuretest case.
1688-1729: LGTM!The test correctly mocks
POSProcessor.create_branch, validates the API response, and asserts the mock was called with the expected parameters.
4195-4200: LGTM!The assertion correctly validates that
company_id=1is now passed to theauthenticatemethod, consistent with the PR's objective of propagatingcompany_idthroughout the authentication flow.kairon/shared/pos/models.py (1)
14-14: LGTM oncompany_idadditions.The
company_idfield with a default of1provides backward compatibility while enabling multi-company support. This aligns well with the processor and Odoo integration changes.Also applies to: 31-31
kairon/shared/pos/processor.py (1)
765-769: LGTM on branch creation flow.The branch creation logic correctly validates state, creates the company in Odoo, persists branch details, and returns the branch_id. Error handling for creation failure is appropriate.
kairon/pos/odoo/odoo_pos.py (1)
24-39: LGTM oncompany_idpropagation in authentication flow.The
company_idis correctly extracted from kwargs and passed to the URL generation methods, enabling proper multi-company scoping in Odoo views.kairon/api/app/routers/pos.py (1)
6-6: LGTM oncompany_idintegration in routes.The
BranchRequestimport andcompany_idpropagation through login and order creation endpoints are correctly implemented, aligning with the model and processor changes.Also applies to: 27-27, 113-114
tests/unit_test/pos_test.py (2)
77-107: LGTM on branch creation success test.The test properly mocks the
jsonrpc_callandsave_branch_details, validates the return value, and asserts correct method invocations.
109-131: LGTM on JSON-RPC failure test.Good coverage for the error path when Odoo fails to create the branch.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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 case when no POS config matches the provided company_id.
If no config matches the company_id, config_id remains None, which will cause downstream errors when creating the POS session. Add validation after the loop.
🐛 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}")📝 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 AppException(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 may leave config_id as None if no config matches company_id; after
the loop (after the for config in pos_configs block) add a validation that
checks if config_id is None and handle it explicitly (e.g., log a clear error
including the company_id and either raise a specific exception or return an
error response/None to abort POS session creation) so downstream code that
creates the POS session does not operate with a None config_id; reference the
variables config_id, pos_configs, company_id and the post-loop validation point
for the change.
| 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} | ||
| ) | ||
|
|
||
| if not pay_method: | ||
| if not payment_method_ids: | ||
| raise HTTPException(status_code=400, detail="No POS payment methods found") | ||
|
|
||
| payment_method_id = pay_method[0]["id"] | ||
| payment_method_id = payment_method_ids[0] |
There was a problem hiding this comment.
Type mismatch: search_read returns list of dicts, not list of ints.
When fetching payment methods via search_read (lines 654-660), the result is a list of dictionaries. However, line 665 assigns payment_method_ids[0] directly to payment_method_id, expecting an integer. This will pass a dict to statement_ids, causing the order creation to fail.
🐛 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}
+ kwargs={"fields": ["id"], "limit": 1}
)
+ 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]🤖 Prompt for AI Agents
In `@kairon/shared/pos/processor.py` around lines 653 - 665, search_read returns a
list of dicts, but the code assigns payment_method_ids[0] (a dict) to
payment_method_id and later passes it to statement_ids; change the assignment to
extract the actual integer id from the first dict returned by jsonrpc_call
(e.g., pull the 'id' field from payment_method_ids[0]) and keep the existing
empty-check; ensure payment_method_id is an int before using it in creating the
order/statement_ids.
| def test_create_branch_failure(): | ||
| with patch("kairon.shared.pos.processor.POSProcessor.create_branch") as mock_create_branch: | ||
| mock_create_branch.side_effect = HTTPException( | ||
| status_code=404, | ||
| detail="Error in creating branch" | ||
| ) | ||
|
|
||
| payload = { | ||
| "branch_name": "Test branch", | ||
| "street": "Andheri East", | ||
| "city": "Mumbai", | ||
| "state": "Maharashtra" | ||
| } | ||
|
|
||
| session_id = "dummy_session_id" | ||
|
|
||
| response = client.post( | ||
| url=f"/api/bot/{pytest.bot}/pos/odoo/create/branch?session_id={session_id}", | ||
| json=payload, | ||
| headers={ | ||
| "Authorization": pytest.token_type + " " + pytest.access_token | ||
| }, | ||
| ) | ||
|
|
||
| actual = response.json() | ||
| assert actual["message"] == "Error in creating branch" |
There was a problem hiding this comment.
Add assertion for response status code.
The test should verify that the HTTP 404 status code is correctly propagated to the response.
Suggested fix
actual = response.json()
+ assert response.status_code == 404
assert actual["message"] == "Error in creating branch"📝 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.
| def test_create_branch_failure(): | |
| with patch("kairon.shared.pos.processor.POSProcessor.create_branch") as mock_create_branch: | |
| mock_create_branch.side_effect = HTTPException( | |
| status_code=404, | |
| detail="Error in creating branch" | |
| ) | |
| payload = { | |
| "branch_name": "Test branch", | |
| "street": "Andheri East", | |
| "city": "Mumbai", | |
| "state": "Maharashtra" | |
| } | |
| session_id = "dummy_session_id" | |
| response = client.post( | |
| url=f"/api/bot/{pytest.bot}/pos/odoo/create/branch?session_id={session_id}", | |
| json=payload, | |
| headers={ | |
| "Authorization": pytest.token_type + " " + pytest.access_token | |
| }, | |
| ) | |
| actual = response.json() | |
| assert actual["message"] == "Error in creating branch" | |
| def test_create_branch_failure(): | |
| with patch("kairon.shared.pos.processor.POSProcessor.create_branch") as mock_create_branch: | |
| mock_create_branch.side_effect = HTTPException( | |
| status_code=404, | |
| detail="Error in creating branch" | |
| ) | |
| payload = { | |
| "branch_name": "Test branch", | |
| "street": "Andheri East", | |
| "city": "Mumbai", | |
| "state": "Maharashtra" | |
| } | |
| session_id = "dummy_session_id" | |
| response = client.post( | |
| url=f"/api/bot/{pytest.bot}/pos/odoo/create/branch?session_id={session_id}", | |
| json=payload, | |
| headers={ | |
| "Authorization": pytest.token_type + " " + pytest.access_token | |
| }, | |
| ) | |
| actual = response.json() | |
| assert response.status_code == 404 | |
| assert actual["message"] == "Error in creating branch" |
🤖 Prompt for AI Agents
In `@tests/integration_test/services_test.py` around lines 1731 - 1756, The test
test_create_branch_failure is missing an assertion that the HTTP status code is
propagated; update the test to assert response.status_code == 404 (using the
response object returned by client.post) in addition to the existing message
assertion so the test verifies both the error payload and the HTTP 404 status
from POSProcessor.create_branch.
| def test_save_branch_details_no_pos_client_config(): | ||
| with patch("kairon.shared.pos.processor.POSClientDetails.objects") as mock_objects: | ||
|
|
||
| mock_objects.return_value.first.return_value = None | ||
|
|
||
| with pytest.raises(AppException) as exc: | ||
| POSProcessor.save_branch_details( | ||
| bot="test_bot", | ||
| branch_name="Test Branch", | ||
| company_id=123, | ||
| user="test_user" | ||
| ) | ||
|
|
||
| assert str(exc.value) == "No POS client configuration found for this bot." |
There was a problem hiding this comment.
Fix mock setup for POSClientDetails.objects.
The mock chain appears incorrect. POSClientDetails.objects(...) returns a queryset, and .first() is called on that. The current mock setup may not work as intended.
🐛 Proposed fix
def test_save_branch_details_no_pos_client_config():
with patch("kairon.shared.pos.processor.POSClientDetails.objects") as mock_objects:
-
- mock_objects.return_value.first.return_value = None
+ mock_query = mock_objects.return_value
+ mock_query.first.return_value = None
with pytest.raises(AppException) as exc:
POSProcessor.save_branch_details(
bot="test_bot",
branch_name="Test Branch",
company_id=123,
user="test_user"
)
assert str(exc.value) == "No POS client configuration found for this bot."🤖 Prompt for AI Agents
In `@tests/unit_test/pos_test.py` around lines 147 - 160, The mock for
POSClientDetails.objects is incorrect for the queryset-first call; instead of
mocking return_value.first, set the queryset chain so .filter(...).first() (or
.first() directly) returns None: update the
test_save_branch_details_no_pos_client_config to configure
mock_objects.filter.return_value.first.return_value = None (and if your code
calls POSClientDetails.objects(...).first(), set
mock_objects.return_value.first.return_value = None) so that
POSProcessor.save_branch_details receives a None POS client configuration and
the AppException is raised.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.