Flow Action #1225
Flow Action #1225
Conversation
2. added APIs along with unit and integration test cases for the same.
WalkthroughThe recent updates introduce a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Bot
participant ActionFactory
participant ActionFlow
participant Dispatcher
User->>Bot: Initiate flow action
Bot->>ActionFactory: Get action type
ActionFactory->>ActionFlow: Instantiate ActionFlow
ActionFlow->>ActionFlow: Retrieve configuration
ActionFlow->>Dispatcher: Execute action
Dispatcher->>User: Return response
Poem
Warning Review ran into problemsProblems (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- kairon/actions/definitions/factory.py (2 hunks)
- kairon/actions/definitions/flow.py (1 hunks)
- kairon/api/app/routers/bot/action.py (2 hunks)
- kairon/api/models.py (2 hunks)
- kairon/shared/actions/data_objects.py (2 hunks)
- kairon/shared/actions/models.py (2 hunks)
- kairon/shared/actions/utils.py (1 hunks)
- kairon/shared/admin/constants.py (1 hunks)
- kairon/shared/data/processor.py (3 hunks)
- system.yaml (1 hunks)
- tests/integration_test/action_service_test.py (2 hunks)
- tests/integration_test/services_test.py (3 hunks)
- tests/testing_data/system.yaml (1 hunks)
- tests/unit_test/data_processor/data_processor_test.py (6 hunks)
Files skipped from review due to trivial changes (1)
- kairon/shared/admin/constants.py
Additional context used
Ruff
kairon/shared/actions/data_objects.py
892-892: Module level import not at top of file (E402)
kairon/shared/actions/utils.py
25-25:
..data.constant.DEFAULT_NLU_FALLBACK_RESPONSEimported but unused (F401)
593-593:
json.JSONDecodeErrorimported but unused (F401)
665-665: f-string without any placeholders (F541)
787-787: Do not use bare
except(E722)
805-805: Do not use bare
except(E722)
827-827: Do not use bare
except(E722)
997-997: f-string without any placeholders (F541)
1013-1013: f-string without any placeholders (F541)
kairon/shared/data/processor.py
1434-1434: Comparison to
Noneshould becond is None(E711)
5545-5545: f-string without any placeholders (F541)
tests/integration_test/action_service_test.py
389-389: Redefinition of unused
test_live_agent_action_execution_with_exceptionfrom line 278 (F811)
1758-1758: f-string without any placeholders (F541)
3189-3189: Local variable
http_urlis assigned to but never used (F841)
3777-3777: Comparison to
Noneshould becond is None(E711)
4546-4546: Comparison to
Noneshould becond is None(E711)
10993-10993: f-string without any placeholders (F541)
11000-11000: f-string without any placeholders (F541)
11895-11895:
uuid6.uuid7imported but unused (F401)
11902-11902: Local variable
bot_contentis assigned to but never used (F841)tests/integration_test/services_test.py
835-835: f-string without any placeholders (F541)
845-845: f-string without any placeholders (F541)
3046-3046: Local variable
response_threeis assigned to but never used (F841)
3270-3270: Comparison to
Noneshould becond is None(E711)
6095-6095: Avoid equality comparisons to
True; useif actual["success"]:for truth checks (E712)
6304-6304: Do not compare types, use
isinstance()(E721)
9129-9129: f-string without any placeholders (F541)
9669-9669: Local variable
response_oneis assigned to but never used (F841)
9673-9673: Local variable
response_twois assigned to but never used (F841)
9677-9677: Local variable
response_threeis assigned to but never used (F841)
9700-9700: f-string without any placeholders (F541)
9843-9843: f-string without any placeholders (F541)
10323-10323: Local variable
response_delete_story_oneis assigned to but never used (F841)
10328-10328: Local variable
response_delete_story_twois assigned to but never used (F841)
11016-11016: Local variable
payload_responseis assigned to but never used (F841)
13115-13115: f-string without any placeholders (F541)
14635-14635: Comparison to
Noneshould becond is None(E711)
14681-14681: Comparison to
Noneshould becond is None(E711)
17498-17498: f-string without any placeholders (F541)
17509-17509: f-string without any placeholders (F541)
17520-17520: f-string without any placeholders (F541)
17533-17533: f-string without any placeholders (F541)
17543-17543: f-string without any placeholders (F541)
17552-17552: f-string without any placeholders (F541)
17569-17569: f-string without any placeholders (F541)
17575-17575: f-string without any placeholders (F541)
17581-17581: f-string without any placeholders (F541)
17601-17601: f-string without any placeholders (F541)
17605-17605: f-string without any placeholders (F541)
17611-17611: f-string without any placeholders (F541)
17622-17622: f-string without any placeholders (F541)
17639-17639: f-string without any placeholders (F541)
17652-17652: f-string without any placeholders (F541)
17665-17665: f-string without any placeholders (F541)
17679-17679: f-string without any placeholders (F541)
17711-17711: f-string without any placeholders (F541)
19531-19531: Comparison to
Noneshould becond is None(E711)
19572-19572: Comparison to
Noneshould becond is None(E711)
19586-19586: Comparison to
Noneshould becond is None(E711)
20127-20127: f-string without any placeholders (F541)
21424-21424: Local variable
mockis assigned to but never used (F841)
22781-22781: f-string without any placeholders (F541)
22816-22816: Comparison to
Noneshould becond is None(E711)
22905-22905: Comparison to
Noneshould becond is None(E711)
22931-22931: Comparison to
Noneshould becond is None(E711)
23171-23171: Avoid equality comparisons to
False; useif not actual["data"]["logs"][1]["translate_responses"]:for false checks (E712)
23172-23172: Avoid equality comparisons to
False; useif not actual["data"]["logs"][1]["translate_actions"]:for false checks (E712)
23188-23188: f-string without any placeholders (F541)
23434-23434: f-string without any placeholders (F541)
23486-23486: f-string without any placeholders (F541)
24144-24144: f-string without any placeholders (F541)
24158-24158: f-string without any placeholders (F541)
24251-24251: Local variable
passwrd_change_responseis assigned to but never used (F841)
24331-24331: Local variable
regsiter_responseis assigned to but never used (F841)
24363-24363: Local variable
regsiter_responseis assigned to but never used (F841)
24568-24568: Avoid equality comparisons to
False; useif not actual["success"]:for false checks (E712)
24572-24572: f-string without any placeholders (F541)
tests/unit_test/data_processor/data_processor_test.py
90-90: Module level import not at top of file (E402)
978-978: f-string without any placeholders (F541)
985-985: f-string without any placeholders (F541)
991-991: Local variable
useris assigned to but never used (F841)
1013-1013: Local variable
useris assigned to but never used (F841)
1038-1038: Local variable
resultis assigned to but never used (F841)
1048-1048: Local variable
useris assigned to but never used (F841)
1049-1049: Local variable
request_datais assigned to but never used (F841)
1053-1053: Local variable
resultis assigned to but never used (F841)
1066-1066: f-string without any placeholders (F541)
1067-1067: Local variable
resultis assigned to but never used (F841)
1221-1221: Local variable
actionis assigned to but never used (F841)
1374-1374: Local variable
useris assigned to but never used (F841)
1512-1512: Local variable
actionis assigned to but never used (F841)
1532-1532: Local variable
useris assigned to but never used (F841)
1737-1737: Local variable
action_idis assigned to but never used (F841)
1746-1746: Local variable
useris assigned to but never used (F841)
3277-3277: Local variable
eis assigned to but never used (F841)
3284-3284: Local variable
endpointis assigned to but never used (F841)
3820-3820: Redefinition of unused
test_get_multiflow_stories_with_STORY_metadatafrom line 3636 (F811)
3866-3866: Redefinition of unused
test_get_multiflow_stories_with_RULE_metadatafrom line 3682 (F811)
3912-3912: Redefinition of unused
test_get_multiflow_stories_with_empty_metadatafrom line 3728 (F811)
3956-3956: Redefinition of unused
test_get_multiflow_stories_with_empty_path_type_metadatafrom line 3772 (F811)
4142-4142: f-string without any placeholders (F541)
4224-4224: Local variable
storyis assigned to but never used (F841)
5854-5854: Local variable
is_event_datais assigned to but never used (F841)
7123-7123: Redefinition of unused
test_add_lookupfrom line 7100 (F811)
7471-7471: Local variable
useris assigned to but never used (F841)
8473-8473: Local variable
useris assigned to but never used (F841)
8714-8714: Do not compare types, use
isinstance()(E721)
8770-8770: f-string without any placeholders (F541)
8801-8801: f-string without any placeholders (F541)
8808-8808: f-string without any placeholders (F541)
8825-8825: f-string without any placeholders (F541)
8839-8839: f-string without any placeholders (F541)
8877-8877: f-string without any placeholders (F541)
10032-10032: Local variable
intentis assigned to but never used (F841)
10106-10106: Local variable
resultis assigned to but never used (F841)
10509-10509: Local variable
useris assigned to but never used (F841)
10867-10867: Local variable
useris assigned to but never used (F841)
11824-11824: Local variable
story_dictis assigned to but never used (F841)
11868-11868: Local variable
processoris assigned to but never used (F841)
11901-11901: Local variable
story_dictis assigned to but never used (F841)
13136-13136: Dictionary key literal
'web_search_action'repeated (F601)
13572-13572: Local variable
mock_smtpis assigned to but never used (F841)
13589-13589: Local variable
mock_smtpis assigned to but never used (F841)
13672-13672: Local variable
mock_smtpis assigned to but never used (F841)
13689-13689: Local variable
mock_smtpis assigned to but never used (F841)
13710-13710: Local variable
mock_smtpis assigned to but never used (F841)
13769-13769: Local variable
mock_smtpis assigned to but never used (F841)
15099-15099: Local variable
schema_fouris assigned to but never used (F841)
15166-15166: Local variable
processoris assigned to but never used (F841)
15179-15179: Local variable
processoris assigned to but never used (F841)
15195-15195: Local variable
useris assigned to but never used (F841)
15207-15207: Local variable
useris assigned to but never used (F841)
15227-15227: Local variable
useris assigned to but never used (F841)
15435-15435: Local variable
useris assigned to but never used (F841)
15441-15441: Local variable
useris assigned to but never used (F841)
15525-15525: Local variable
useris assigned to but never used (F841)
15614-15614: Local variable
processoris assigned to but never used (F841)
15641-15641: Local variable
processoris assigned to but never used (F841)
15683-15683: Local variable
useris assigned to but never used (F841)
15696-15696: Local variable
useris assigned to but never used (F841)
15702-15702: Local variable
useris assigned to but never used (F841)
15977-15977: Local variable
model_trainingis assigned to but never used (F841)
Additional comments not posted (50)
kairon/shared/actions/models.py (3)
23-25: The addition of theFlowModesenum with valuesdraftandpublishedis clear and follows the established pattern of enum definitions in the system.
28-30: The addition of theFlowActionTypesenum with valuesnavigateanddata_exchangeis appropriate and aligns with the system's modular design for handling different types of actions within flows.
61-61: The addition offlow_actionto theActionTypeenum is necessary to integrate the new flow action functionality into the system's action handling framework.kairon/actions/definitions/factory.py (2)
5-5: The import ofActionFlowis correctly placed and follows Python conventions, enabling its use in the action mapping.
46-46: The mapping offlow_actiontoActionFlowin the__implementationsdictionary is correctly implemented, ensuring thatActionFlowinstances can be properly created by the factory.tests/testing_data/system.yaml (1)
150-154: The addition of theflowsection with configuration for URL and headers is correctly implemented, using environment variables for values to ensure flexibility and security in testing environments.system.yaml (1)
154-158: The addition of theflowsection in the production configuration is consistent with the testing configuration, correctly using environment variables for the URL and headers. This ensures that the new flow action functionality can be seamlessly integrated and managed across environments.kairon/api/app/routers/bot/action.py (4)
10-10: Ensure that the newFlowActionRequestimport is utilized properly in the endpoints.
69-79: The implementation of theadd_flow_actionendpoint correctly handles the addition of new flow actions with appropriate security checks.
82-92: Theupdate_flow_actionendpoint is well-implemented, ensuring secure and correct updates to flow actions.
95-102: Thelist_flow_actionsendpoint correctly retrieves all flow actions for a bot with appropriate security measures.kairon/shared/actions/data_objects.py (1)
475-489: The validation method inFlowActionConfigis comprehensive, covering all required fields. It ensures that essential fields are not empty, which is crucial for maintaining data integrity.kairon/api/models.py (1)
25-26: The import ofFlowModesandFlowActionTypesis correctly placed and used in theFlowActionRequestclass. Ensure that these enums are adequately documented in their definition modules to facilitate understanding and maintenance.kairon/shared/data/processor.py (4)
84-84: Import ofFlowActionConfiglooks good.
3916-3916: Consider moving the import statement to the top of the file for better performance.
[REFACTOR_SUGGESTion]- from kairon.shared.chat.data_objects import Channels + # Move this import to the top of the file
3928-3958: Update method handles various fields and scenarios effectively.
3960-3978: Efficient handling of document IDs in listing method.tests/integration_test/action_service_test.py (6)
23-23: Import statement forFlowActionConfigis correctly placed and relevant for the test cases.
500-591: The test casetest_flow_action_executionis well-structured and covers the basic execution path for a flow action. Ensure that all edge cases are also tested.
594-685: The test casetest_flow_action_execution_with_flow_id_from_slotcorrectly tests the dynamic retrieval of flow ID from slots. Consider adding more assertions to verify the complete flow of data.
688-777: The test casetest_flow_action_execution_with_recipient_phone_from_sloteffectively tests the dynamic retrieval of recipient phone from slots. Ensure comprehensive coverage by adding tests for invalid or missing slot values.
781-872: The test casetest_flow_action_execution_with_header_footeris comprehensive and tests the inclusion of header and footer in the flow action. Consider testing the absence of these fields as well to handle potential errors gracefully.
876-970: The test casetest_flow_action_execution_with_dispatch_response_disabledcorrectly handles the scenario where the dispatch response is disabled. Ensure that the system behaves as expected when the dispatch response is enabled as well.tests/integration_test/services_test.py (14)
1560-1582: Ensure consistent error handling for empty 'name' field in flow actions.This test verifies that the API correctly handles cases where the 'name' field is empty by returning a 422 error with a descriptive message. Good coverage for input validation.
1584-1606: Ensure consistent error handling for empty 'body' field in flow actions.This test checks that the API properly handles cases where the 'body' field is empty, returning a 422 error with a descriptive message. This is important for maintaining robust input validation.
1608-1631: Ensure consistent error handling for empty 'initial_screen' field in flow actions.This test ensures that the API correctly handles cases where the 'initial_screen' field is empty by returning a 422 error with a descriptive message. It's crucial for ensuring that all required fields are provided.
1633-1655: Ensure consistent error handling for empty 'flow_cta' field in flow actions.This test verifies that the API properly handles cases where the 'flow_cta' field is empty, returning a 422 error with a descriptive message. This helps ensure that all necessary fields are validated.
1657-1678: Verify error handling for invalid 'mode' in flow actions.This test checks the API's response when an invalid 'mode' is provided, ensuring that it correctly returns a 422 error. This is essential for validating the 'mode' field against expected values.
1680-1703: Ensure consistent error handling for empty 'flow_token' field in flow actions.This test ensures that the API correctly handles cases where the 'flow_token' field is empty by returning a 422 error with a descriptive message. Proper validation of all fields is crucial for robust API behavior.
1705-1727: Verify error handling for invalid 'flow_action' in flow actions.This test checks the API's response when an invalid 'flow_action' is provided, ensuring that it correctly returns a 422 error. This is important for validating the 'flow_action' field against expected values.
1729-1751: Check for the presence of a WhatsApp channel when adding flow actions.This test verifies that the API correctly checks for the presence of a WhatsApp channel when adding flow actions, returning a 422 error if not found. This is crucial for operations that depend on specific channel configurations.
1753-1769: Ensure successful addition of WhatsApp channel.This test confirms that the API can successfully add a WhatsApp channel, returning a success message and the correct status code. It's important for testing the integration of new channel configurations.
1771-1793: Ensure successful addition of flow actions.This test verifies that the API can successfully add flow actions, returning a success message and the correct status code. This is essential for testing the functionality of adding new actions.
1795-1819: Check for existing flow actions before adding new ones.This test ensures that the API checks for existing flow actions before adding new ones, returning an error if an action already exists. This prevents duplicate entries and maintains data integrity.
1821-1846: Ensure successful addition of flow actions with slot values.This test verifies that the API can successfully add flow actions with slot values, returning a success message and the correct status code. This is crucial for testing the flexibility of action configurations.
1848-1870: Ensure retrieval of flow actions after updates.This test checks that the API can correctly retrieve flow actions after they have been updated, ensuring that the updates are reflected in the retrieved data. This is important for verifying the persistence of changes.
1886-1909: Verify error handling for empty 'body' field when updating flow actions.This test ensures that the API correctly handles cases where the 'body' field is empty during an update, returning a 422 error with a descriptive message. Consistent validation during updates is crucial.
tests/unit_test/data_processor/data_processor_test.py (13)
1218-1235: Good use of exception handling in test cases.The test
test_add_flow_action_empty_namecorrectly anticipates aValidationErrorwhen the action name is empty, which aligns with the expected behavior.Tools
Ruff
1221-1221: Local variable
actionis assigned to but never used (F841)
1237-1254: Effective testing of input validation.The test
test_add_flow_action_empty_bodyeffectively checks for empty body input, ensuring robust error handling in the application.
1256-1273: Consistent validation testing.The test
test_add_flow_action_empty_initial_screenmaintains consistency in testing input validation, which is crucial for maintaining data integrity.
1275-1292: Proper error handling in test cases.The test
test_add_flow_action_empty_flow_ctaproperly handles cases where the flow CTA is empty, ensuring that the system behaves as expected under erroneous inputs.
1294-1311: Thorough testing of required fields.The test
test_add_flow_action_empty_flow_tokenchecks for the absence of a required field, which is essential for ensuring the application's resilience to incorrect or incomplete data.
1314-1330: Ensure all necessary fields are tested.The test
test_add_flow_actioncovers a scenario with all required fields provided, which is important for validating the successful creation of flow actions.
1332-1349: Handle existing entities appropriately in tests.The test
test_add_flow_action_with_name_existscorrectly anticipates anAppExceptionwhen trying to add a flow action with a name that already exists, which is crucial for maintaining data uniqueness.
1351-1370: Test dynamic data handling.The test
test_add_flow_action_with_slot_valueseffectively handles dynamic data through slots, which is essential for applications that need to adapt to varying input conditions.
1372-1389: Comprehensive retrieval functionality testing.The function
test_get_flow_actionsthoroughly tests the retrieval of flow actions, ensuring that the system can fetch and display existing actions correctly.Tools
Ruff
1374-1374: Local variable
useris assigned to but never used (F841)
1410-1428: Ensure error messages are clear and informative.The test
test_update_flow_action_empty_bodyeffectively uses error handling to ensure that users are informed about the necessity of a non-empty body.
1430-1448: Consistent validation for update operations.The test
test_update_flow_action_empty_initial_screenmaintains consistent validation checks during update operations, which is important for data integrity.
1450-1468: Proper handling of empty fields in update scenarios.The test
test_update_flow_action_empty_flow_ctacorrectly handles cases where the flow CTA is empty during an update, ensuring robust error handling.
1470-1487: Thorough testing of token validation.The test
test_update_flow_action_empty_flow_tokenthoroughly checks for the presence of a flow token during updates, which is crucial for the security and functionality of flow actions.
| @auditlogger.log | ||
| @push_notification.apply | ||
| class FlowActionConfig(Auditlog): | ||
| name = StringField(required=True) | ||
| flow_id = EmbeddedDocumentField(CustomActionRequestParameters) | ||
| header = StringField(default=None) | ||
| body = StringField(required=True) | ||
| footer = StringField(default=None) | ||
| mode = StringField(default=FlowModes.published.value, | ||
| choices=[p_type.value for p_type in FlowModes]) | ||
| flow_action = StringField(default=FlowActionTypes.navigate.value, | ||
| choices=[p_type.value for p_type in FlowModes]) | ||
| flow_token = StringField(required=True) | ||
| recipient_phone = EmbeddedDocumentField(CustomActionRequestParameters) | ||
| initial_screen = StringField(required=True) | ||
| flow_cta = StringField(required=True) | ||
| dispatch_response = BooleanField(default=True) | ||
| response = StringField(default=None) | ||
| bot = StringField(required=True) | ||
| user = StringField(required=True) | ||
| timestamp = DateTimeField(default=datetime.utcnow) | ||
| status = BooleanField(default=True) |
There was a problem hiding this comment.
The FlowActionConfig class is well-structured with appropriate use of field types and default values. However, the choices parameter for flow_action seems to incorrectly reference FlowModes instead of FlowActionTypes.
- choices=[p_type.value for p_type in FlowModes]
+ choices=[p_type.value for p_type in FlowActionTypes]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.
| @auditlogger.log | |
| @push_notification.apply | |
| class FlowActionConfig(Auditlog): | |
| name = StringField(required=True) | |
| flow_id = EmbeddedDocumentField(CustomActionRequestParameters) | |
| header = StringField(default=None) | |
| body = StringField(required=True) | |
| footer = StringField(default=None) | |
| mode = StringField(default=FlowModes.published.value, | |
| choices=[p_type.value for p_type in FlowModes]) | |
| flow_action = StringField(default=FlowActionTypes.navigate.value, | |
| choices=[p_type.value for p_type in FlowModes]) | |
| flow_token = StringField(required=True) | |
| recipient_phone = EmbeddedDocumentField(CustomActionRequestParameters) | |
| initial_screen = StringField(required=True) | |
| flow_cta = StringField(required=True) | |
| dispatch_response = BooleanField(default=True) | |
| response = StringField(default=None) | |
| bot = StringField(required=True) | |
| user = StringField(required=True) | |
| timestamp = DateTimeField(default=datetime.utcnow) | |
| status = BooleanField(default=True) | |
| @auditlogger.log | |
| @push_notification.apply | |
| class FlowActionConfig(Auditlog): | |
| name = StringField(required=True) | |
| flow_id = EmbeddedDocumentField(CustomActionRequestParameters) | |
| header = StringField(default=None) | |
| body = StringField(required=True) | |
| footer = StringField(default=None) | |
| mode = StringField(default=FlowModes.published.value, | |
| choices=[p_type.value for p_type in FlowModes]) | |
| flow_action = StringField(default=FlowActionTypes.navigate.value, | |
| choices=[p_type.value for p_type in FlowActionTypes]) | |
| flow_token = StringField(required=True) | |
| recipient_phone = EmbeddedDocumentField(CustomActionRequestParameters) | |
| initial_screen = StringField(required=True) | |
| flow_cta = StringField(required=True) | |
| dispatch_response = BooleanField(default=True) | |
| response = StringField(default=None) | |
| bot = StringField(required=True) | |
| user = StringField(required=True) | |
| timestamp = DateTimeField(default=datetime.utcnow) | |
| status = BooleanField(default=True) |
| class FlowActionRequest(BaseModel): | ||
| name: constr(to_lower=True, strip_whitespace=True) | ||
| flow_id: CustomActionParameter | ||
| header: str = None | ||
| body: str | ||
| footer: str = None | ||
| mode: FlowModes = FlowModes.published.value | ||
| flow_action: FlowActionTypes = FlowActionTypes.navigate.value | ||
| flow_token: str | ||
| recipient_phone: CustomActionParameter | ||
| initial_screen: str | ||
| flow_cta: str | ||
| dispatch_response: bool = True | ||
| response: str = None | ||
|
|
||
| @validator("name") | ||
| def validate_name(cls, v, values, **kwargs): | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| if Utility.check_empty_string(v): | ||
| raise ValueError("name is required") | ||
| return v | ||
|
|
||
| @validator("body") | ||
| def validate_body(cls, v, values, **kwargs): | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| if Utility.check_empty_string(v): | ||
| raise ValueError("body is required") | ||
| return v | ||
|
|
||
| @validator("initial_screen") | ||
| def validate_initial_screen(cls, v, values, **kwargs): | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| if Utility.check_empty_string(v): | ||
| raise ValueError("initial_screen is required") | ||
| return v | ||
|
|
||
| @validator("flow_cta") | ||
| def validate_flow_cta(cls, v, values, **kwargs): | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| if Utility.check_empty_string(v): | ||
| raise ValueError("flow_cta is required") | ||
| return v | ||
|
|
||
| @validator("flow_token") | ||
| def validate_flow_token(cls, v, values, **kwargs): | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| if Utility.check_empty_string(v): | ||
| raise ValueError("flow_token is required") | ||
| return v |
There was a problem hiding this comment.
The FlowActionRequest class is well-structured with appropriate validators for each field. However, consider adding docstrings to each validator method to improve code maintainability and readability.
+ @validator("name")
+ """
+ Validates that the 'name' field is not empty.
+ """
+ def validate_name(cls, v, values, **kwargs):
+ from kairon.shared.utils import Utility
+
+ if Utility.check_empty_string(v):
+ raise ValueError("name is required")
+ return vCommittable suggestion was skipped due to low confidence.
| def test_add_flow_action_empty_name(self): | ||
| bot = 'test_bot' | ||
| user = 'test_user' | ||
| action = "test_add_flow_action_empty_name" |
There was a problem hiding this comment.
Remove unused local variable.
The variable action in test_add_flow_action_empty_name is assigned but never used. Consider removing it if it's not needed.
Tools
Ruff
1221-1221: Local variable
actionis assigned to but never used (F841)
|
|
||
| def test_get_flow_actions(self): | ||
| bot = 'test_bot' | ||
| user = 'test_user' |
There was a problem hiding this comment.
Remove unused local variable.
The variable user in test_get_flow_actions is assigned but never used. Consider removing it if it's not needed.
Tools
Ruff
1374-1374: Local variable
useris assigned to but never used (F841)
| elif action.type == ActionType.flow_action.value: | ||
| Utility.delete_document([FlowActionConfig], name__iexact=name, bot=bot, user=user) |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent deletion methods across different action types. Currently, Utility.hard_delete_document is used extensively, while Utility.delete_document is used specifically for FlowActionConfig. Consider using a consistent method for all action types.
Utility.hard_delete_documentis used for various action types.Utility.delete_documentis used forFlowActionConfig.
Analysis chain
Ensure consistent deletion methods across different action types.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify deletion methods for consistency across different action types.
# Test: Search for deletion methods for different action types. Expect: Consistent usage.
rg --type python $'Utility.delete_document'
Length of output: 73
Script:
#!/bin/bash
# Description: Verify deletion methods for consistency across different action types.
# Test: Search for deletion methods for different action types. Expect: Consistent usage.
rg --type py 'Utility.delete_document'
Length of output: 169
Script:
#!/bin/bash
# Description: Verify deletion methods for different action types.
# Test: Search for deletion methods for different action types. Expect: Consistent usage.
rg --type py 'delete' kairon/shared/data/processor.py
Length of output: 7323
| @patch("kairon.shared.data.utils.DataUtility.get_channel_endpoint", autospec=True) | ||
| def test_add_whatsapp_channel_config(self, mock_get_channel_endpoint): | ||
| from kairon.shared.chat.processor import ChatDataProcessor | ||
|
|
||
| bot = 'test_bot' | ||
| user = 'test_user' | ||
| processor = ChatDataProcessor() | ||
| channel_endpoint = "https://test@test.com/api/bot/whatsapp/test_bot/access_token" | ||
| mock_get_channel_endpoint.return_value = channel_endpoint | ||
| data = {"connector_type": "whatsapp", | ||
| "config": { | ||
| "access_token": "xoxb-801939352912-801478018484-v3zq6MYNu62oSs8vammWOY8K", | ||
| "app_secret": "79f036b9894eef17c064213b90d1042b", | ||
| "verify_token": "3396830255712.3396861654876869879", | ||
| }} | ||
| endpoint = processor.save_channel_config(configuration=data, bot=bot, user=user) | ||
| assert endpoint == channel_endpoint |
There was a problem hiding this comment.
Ensure sensitive data is not hardcoded in tests.
The access token and app secret are hardcoded in the test function test_add_whatsapp_channel_config. Consider using environment variables or mock data to avoid security risks.
- "access_token": "xoxb-801939352912-801478018484-v3zq6MYNu62oSs8vammWOY8K",
- "app_secret": "79f036b9894eef17c064213b90d1042b",
+ "access_token": os.environ.get("WHATSAPP_ACCESS_TOKEN"),
+ "app_secret": os.environ.get("WHATSAPP_APP_SECRET"),Committable suggestion was skipped due to low confidence.
| @staticmethod | ||
| def prepare_flow_body(flow_action_config: dict, tracker: Tracker): | ||
| api_key = Sysadmin.get_bot_secret(flow_action_config['bot'], BotSecretType.d360_api_key.value, raise_err=False) | ||
| http_url = Utility.environment["flow"]["url"] | ||
| header_key = Utility.environment["flow"]["headers"]["key"] | ||
| headers = {header_key: api_key} | ||
| flow_body = { | ||
| "recipient_type": "individual", | ||
| "messaging_product": "whatsapp", | ||
| "type": "interactive", | ||
| "interactive": { | ||
| "type": "flow", | ||
| "action": { | ||
| "name": "flow", | ||
| "parameters": { | ||
| "mode": "published", | ||
| "flow_message_version": "3", | ||
| "flow_token": "AQAAAAACS5FpgQ_cAAAAAD0QI3s.", | ||
| "flow_action": "navigate", | ||
| } | ||
| } | ||
| } | ||
| } | ||
| parameter_type = flow_action_config['recipient_phone']['parameter_type'] | ||
| recipient_phone = tracker.get_slot(flow_action_config['recipient_phone']['value']) \ | ||
| if parameter_type == 'slot' else flow_action_config['recipient_phone']['value'] | ||
| parameter_type = flow_action_config['flow_id']['parameter_type'] | ||
| flow_id = tracker.get_slot(flow_action_config['flow_id']['value']) \ | ||
| if parameter_type == 'slot' else flow_action_config['flow_id']['value'] | ||
| header = flow_action_config.get('header') | ||
| body = flow_action_config['body'] | ||
| footer = flow_action_config.get('footer') | ||
| mode = flow_action_config['mode'] | ||
| flow_action = flow_action_config['flow_action'] | ||
| flow_token = flow_action_config['flow_token'] | ||
| initial_screen = flow_action_config['initial_screen'] | ||
| flow_cta = flow_action_config['flow_cta'] | ||
| flow_body["to"] = recipient_phone | ||
| if header: | ||
| flow_body["interactive"]["header"] = {"type": "text", "text": header} | ||
| flow_body["interactive"]["body"] = {"text": body} | ||
| if footer: | ||
| flow_body["interactive"]["footer"] = {"text": footer} | ||
| flow_body["interactive"]["action"]["parameters"]["mode"] = mode | ||
| flow_body["interactive"]["action"]["parameters"]["flow_action"] = flow_action | ||
| flow_body["interactive"]["action"]["parameters"]["flow_token"] = flow_token | ||
| flow_body["interactive"]["action"]["parameters"]["flow_cta"] = flow_cta | ||
| flow_body["interactive"]["action"]["parameters"]["flow_id"] = flow_id | ||
| flow_body["interactive"]["action"]["parameters"]["flow_action_payload"] = {"screen": initial_screen} | ||
| return flow_body, http_url, headers |
There was a problem hiding this comment.
Refactor prepare_flow_body to improve readability and maintainability.
The prepare_flow_body method is quite lengthy and handles multiple responsibilities. Consider breaking it down into smaller, more focused methods. For example, extracting the logic for setting headers and constructing the flow_body dictionary into separate methods could improve readability and maintainability.
- @staticmethod
- def prepare_flow_body(flow_action_config: dict, tracker: Tracker):
- api_key = Sysadmin.get_bot_secret(flow_action_config['bot'], BotSecretType.d360_api_key.value, raise_err=False)
- http_url = Utility.environment["flow"]["url"]
- header_key = Utility.environment["flow"]["headers"]["key"]
- headers = {header_key: api_key}
- flow_body = {
- "recipient_type": "individual",
- "messaging_product": "whatsapp",
- "type": "interactive",
- "interactive": {
- "type": "flow",
- "action": {
- "name": "flow",
- "parameters": {
- "mode": "published",
- "flow_message_version": "3",
- "flow_token": "AQAAAAACS5FpgQ_cAAAAAD0QI3s.",
- "flow_action": "navigate",
- }
- }
- }
- }
- parameter_type = flow_action_config['recipient_phone']['parameter_type']
- recipient_phone = tracker.get_slot(flow_action_config['recipient_phone']['value']) \
- if parameter_type == 'slot' else flow_action_config['recipient_phone']['value']
- parameter_type = flow_action_config['flow_id']['parameter_type']
- flow_id = tracker.get_slot(flow_action_config['flow_id']['value']) \
- if parameter_type == 'slot' else flow_action_config['flow_id']['value']
- header = flow_action_config.get('header')
- body = flow_action_config['body']
- footer = flow_action_config.get('footer')
- mode = flow_action_config['mode']
- flow_action = flow_action_config['flow_action']
- flow_token = flow_action_config['flow_token']
- initial_screen = flow_action_config['initial_screen']
- flow_cta = flow_action_config['flow_cta']
- flow_body["to"] = recipient_phone
- if header:
- flow_body["interactive"]["header"] = {"type": "text", "text": header}
- flow_body["interactive"]["body"] = {"text": body}
- if footer:
- flow_body["interactive"]["footer"] = {"text": footer}
- flow_body["interactive"]["action"]["parameters"]["mode"] = mode
- flow_body["interactive"]["action"]["parameters"]["flow_action"] = flow_action
- flow_body["interactive"]["action"]["parameters"]["flow_token"] = flow_token
- flow_body["interactive"]["action"]["parameters"]["flow_cta"] = flow_cta
- flow_body["interactive"]["action"]["parameters"]["flow_id"] = flow_id
- flow_body["interactive"]["action"]["parameters"]["flow_action_payload"] = {"screen": initial_screen}
- return flow_body, http_url, headers
+ # Proposed refactoring to be added hereCommittable suggestion was skipped due to low confidence.
kairon/actions/definitions/flow.py
Outdated
| class ActionFlow(ActionsBase): | ||
|
|
||
| def __init__(self, bot: Text, name: Text): | ||
| """ | ||
| Initialize Flow action. | ||
| @param bot: bot id | ||
| @param name: action name | ||
| """ | ||
| self.bot = bot | ||
| self.name = name | ||
|
|
||
| def retrieve_config(self): | ||
| """ | ||
| Fetch Flow action configuration parameters from the database | ||
| :return: FlowActionConfig containing configuration for the action as a dict. | ||
| """ | ||
| try: | ||
| http_config_dict = FlowActionConfig.objects().get(bot=self.bot, | ||
| name=self.name, | ||
| status=True).to_mongo().to_dict() | ||
| logger.debug("flow_action_config: " + str(http_config_dict)) | ||
| return http_config_dict | ||
| except DoesNotExist as e: | ||
| logger.exception(e) | ||
| raise ActionFailure("No Flow action found for given action and bot") | ||
|
|
||
| async def execute(self, dispatcher: CollectingDispatcher, tracker: Tracker, domain: Dict[Text, Any]): | ||
| """ | ||
| Retrieves action config and executes it. | ||
| Information regarding the execution is logged in ActionServerLogs. | ||
| @param dispatcher: Client to send messages back to the user. | ||
| @param tracker: Tracker object to retrieve slots, events, messages and other contextual information. | ||
| @param domain: Bot domain | ||
| :return: Dict containing slot name as keys and their values. | ||
| """ | ||
| bot_response = None | ||
| http_response = None | ||
| exception = None | ||
| status = "SUCCESS" | ||
| http_url = None | ||
| headers = {} | ||
| body = {} | ||
| dispatch_response = True | ||
| try: | ||
| flow_action_config = self.retrieve_config() | ||
| bot_response = flow_action_config['response'] | ||
| dispatch_response = flow_action_config['dispatch_response'] | ||
| body, http_url, headers = ActionUtility.prepare_flow_body(flow_action_config, tracker) | ||
| http_response, status_code, time_elapsed = await ActionUtility.execute_request_async( | ||
| headers=headers, http_url=http_url, request_method="POST", request_body=body | ||
| ) | ||
| logger.info("response: " + str(http_response)) | ||
| except Exception as e: | ||
| exception = str(e) | ||
| logger.exception(e) | ||
| status = "FAILURE" | ||
| bot_response = "I have failed to process your request" | ||
| finally: | ||
| if dispatch_response: | ||
| dispatcher.utter_message(bot_response) | ||
| ActionServerLogs( | ||
| type=ActionType.flow_action.value, | ||
| intent=tracker.get_intent_of_latest_message(skip_fallback_intent=False), | ||
| action=self.name, | ||
| sender=tracker.sender_id, | ||
| bot=tracker.get_slot("bot"), | ||
| url=http_url, | ||
| headers=headers, | ||
| request_params=body, | ||
| exception=exception, | ||
| api_response=str(http_response) if http_response else None, | ||
| bot_response=bot_response, | ||
| status=status, | ||
| user_msg=tracker.latest_message.get('text') | ||
| ).save() | ||
| return {KaironSystemSlots.kairon_action_response.value: bot_response} |
There was a problem hiding this comment.
The ActionFlow class is well-implemented with clear documentation and appropriate methods for its intended functionality. The use of logging and structured exception handling enhances maintainability and debuggability.
Consider adding more specific exception types in the execute method to handle different failure scenarios more gracefully.
- except Exception as e:
+ except (HTTPException, TimeoutException) as e:Committable suggestion was skipped due to low confidence.
2. added APIs along with unit and integration test cases for the same.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- kairon/actions/definitions/flow.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- kairon/actions/definitions/flow.py
Summary by CodeRabbit
New Features
ActionFlowfor managing and executing flow actions in the bot framework.Bug Fixes
delete_actionmethod to handle deletion ofFlowActionConfigactions.Tests
Configuration