Conversation
WalkthroughThis PR adds comprehensive test coverage across two test files: integration tests for a chatbot management API (covering bot operations, configurations, integrations, media, and scheduling) and unit tests for Odoo POS integration (covering products, orders, onboarding, and authentication flows). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
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: 1
🧹 Nitpick comments (3)
tests/unit_test/pos_test.py (3)
40-44: Consider verifying the arguments passed to the mocked function.The test confirms that
onboarding_clientis called and returns the expected value, but it doesn't verify that the correct arguments (client_name,bot,user) are passed through.Apply this diff to make the test more robust:
def test_onboarding(odoo_pos): with patch.object(POSProcessor, "onboarding_client", return_value={"ok": True}) as mock_fn: resp = odoo_pos.onboarding(client_name="demo", bot="test_bot", user="aniket.kharkia@nimblework.com") - mock_fn.assert_called_once() + mock_fn.assert_called_once_with(client_name="demo", bot="test_bot", user="aniket.kharkia@nimblework.com") assert resp == {"ok": True}
46-58: Consider verifying arguments passed topos_login.The test correctly verifies the data flow through
set_odoo_session_cookie, but it could also verify thatpos_loginreceives the expectedclient_nameandbotparameters.Apply this diff to enhance the test:
resp = odoo_pos.authenticate(client_name="demo", bot="test_bot") - p_login.assert_called_once() + p_login.assert_called_once_with("demo", "test_bot") p_list.assert_called_once() p_cookie.assert_called_once_with({"session": "s1", "url": "p_url"})
60-71: Consider verifying arguments passed topos_login.Similar to
test_authenticate_products, this test could be enhanced by verifying thatpos_loginreceives the expected parameters.Apply this diff:
resp = odoo_pos.authenticate(client_name="C", bot="B", page_type="pos_orders") - p_login.assert_called_once() + p_login.assert_called_once_with("C", "B") o_list.assert_called_once() p_cookie.assert_called_once_with({"session": "xyz", "url": "o_url"}) assert resp == {"ok": 1}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(1 hunks)tests/unit_test/pos_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration_test/services_test.py (2)
kairon/pos/odoo/odoo_pos.py (2)
onboarding(12-22)authenticate(24-38)kairon/pos/definitions/base.py (2)
onboarding(9-10)authenticate(13-14)
tests/unit_test/pos_test.py (2)
kairon/pos/odoo/odoo_pos.py (1)
OdooPOS(9-54)kairon/shared/pos/processor.py (1)
POSProcessor(18-732)
🪛 Ruff (0.14.7)
tests/unit_test/pos_test.py
25-25: Unused function argument: module
(ARG001)
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
🔇 Additional comments (5)
tests/unit_test/pos_test.py (4)
1-13: LGTM! Proper environment patching before imports.The pattern of patching
kairon.Utility.environmentbefore importing the modules that depend on it is correct and ensures the mock configuration is used throughout the tests.
15-22: LGTM! Clean fixture definition.The fixture properly instantiates OdooPOS for use across multiple tests.
25-27: LGTM! Proper cleanup of the environment patch.The
moduleparameter is required by pytest'steardown_modulesignature. The static analysis warning is a false positive and can be safely ignored.
30-37: LGTM! Tests verify environment configuration is used.Both tests appropriately verify that the mocked Odoo URL is used when constructing product and order list URLs.
tests/integration_test/services_test.py (1)
2788-2814: Same mock configuration concern applies here.This test uses the same
mock_pos.return_value.authenticatepattern. If the previous test's mock setup is incorrect, apply the same fix here:mock_pos = MagicMock() -mock_pos.return_value.authenticate.return_value = {"ok": True} +mock_pos.authenticate.return_value = {"ok": True} mock_factory.return_value = mock_pos ... -mock_pos.return_value.authenticate.assert_called_once_with( +mock_pos.authenticate.assert_called_once_with( client_name="XYZ_Pvt_Ltd", page_type="pos_products", bot=pytest.bot )
| with patch("kairon.pos.definitions.factory.POSFactory.get_instance") as mock_factory: | ||
|
|
||
| mock_pos = MagicMock() | ||
| mock_pos.return_value.onboarding.return_value = {"ok": True} | ||
|
|
||
| mock_factory.return_value = mock_pos | ||
|
|
||
| payload = { | ||
| "client_name": "XYZ_Pvt_Ltd" | ||
| } | ||
|
|
||
|
|
||
| response = client.post( | ||
| url=f"/api/bot/{pytest.bot}/pos/odoo/register", | ||
| json=payload, | ||
| headers={"Authorization": pytest.token_type + " " + pytest.access_token}, | ||
| ) | ||
|
|
||
| actual = response.json() | ||
| assert actual["success"] | ||
| assert actual["data"] == {'ok': True} | ||
| mock_factory.assert_called_once_with("odoo") | ||
| mock_pos.return_value.onboarding.assert_called_once_with( | ||
| client_name="XYZ_Pvt_Ltd", | ||
| bot=pytest.bot, | ||
| user='integration@demo.ai' | ||
| ) |
There was a problem hiding this comment.
Fix mock configuration to match actual factory usage pattern.
The mock is incorrectly set up with mock_pos.return_value.onboarding.return_value, but POSFactory.get_instance("odoo") returns an OdooPOS instance directly (not a callable). The endpoint code calls pos_instance.onboarding(...) where pos_instance is the returned instance, not a function to be invoked.
Correct the mock setup:
mock_pos = MagicMock()
-mock_pos.return_value.onboarding.return_value = {"ok": True}
+mock_pos.onboarding.return_value = {"ok": True}
mock_factory.return_value = mock_pos
...
-mock_pos.return_value.onboarding.assert_called_once_with(
+mock_pos.onboarding.assert_called_once_with(
client_name="XYZ_Pvt_Ltd",
bot=pytest.bot,
user='integration@demo.ai'
)The same issue exists in test_odoo_login — mock_pos.return_value.authenticate should be mock_pos.authenticate.
📝 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.
| with patch("kairon.pos.definitions.factory.POSFactory.get_instance") as mock_factory: | |
| mock_pos = MagicMock() | |
| mock_pos.return_value.onboarding.return_value = {"ok": True} | |
| mock_factory.return_value = mock_pos | |
| payload = { | |
| "client_name": "XYZ_Pvt_Ltd" | |
| } | |
| response = client.post( | |
| url=f"/api/bot/{pytest.bot}/pos/odoo/register", | |
| json=payload, | |
| headers={"Authorization": pytest.token_type + " " + pytest.access_token}, | |
| ) | |
| actual = response.json() | |
| assert actual["success"] | |
| assert actual["data"] == {'ok': True} | |
| mock_factory.assert_called_once_with("odoo") | |
| mock_pos.return_value.onboarding.assert_called_once_with( | |
| client_name="XYZ_Pvt_Ltd", | |
| bot=pytest.bot, | |
| user='integration@demo.ai' | |
| ) | |
| with patch("kairon.pos.definitions.factory.POSFactory.get_instance") as mock_factory: | |
| mock_pos = MagicMock() | |
| mock_pos.onboarding.return_value = {"ok": True} | |
| mock_factory.return_value = mock_pos | |
| payload = { | |
| "client_name": "XYZ_Pvt_Ltd" | |
| } | |
| response = client.post( | |
| url=f"/api/bot/{pytest.bot}/pos/odoo/register", | |
| json=payload, | |
| headers={"Authorization": pytest.token_type + " " + pytest.access_token}, | |
| ) | |
| actual = response.json() | |
| assert actual["success"] | |
| assert actual["data"] == {'ok': True} | |
| mock_factory.assert_called_once_with("odoo") | |
| mock_pos.onboarding.assert_called_once_with( | |
| client_name="XYZ_Pvt_Ltd", | |
| bot=pytest.bot, | |
| user='integration@demo.ai' | |
| ) |
🤖 Prompt for AI Agents
In tests/integration_test/services_test.py around lines 2760 to 2786, the mock
config uses mock_pos.return_value.* but POSFactory.get_instance("odoo") returns
an instance directly, so set expectations on the mock instance itself: assign
mock_factory.return_value = mock_pos and replace uses of
mock_pos.return_value.onboarding.return_value with
mock_pos.onboarding.return_value (and similarly change
mock_pos.return_value.onboarding.assert_called_once_with to
mock_pos.onboarding.assert_called_once_with). Do the same fix for the other test
(test_odoo_login): use mock_pos.authenticate and
mock_pos.authenticate.assert_called_once_with instead of
mock_pos.return_value.authenticate variants so the mocked methods match how the
factory is used.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.