Add support for editing expenses #379
Conversation
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes incorporate validation enhancements, exception handling, and logging across various modules in the project. Improvements include adding new validation functions, updating function signatures to accommodate additional parameters, rearranging and enhancing test data, and increasing the logging depth to monitor specific actions. These modifications target better error handling, data validation, and performance tracking, improving overall code stability and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExportView
participant QueueModule
participant Logger
participant Validator
Client->>ExportView: POST request with body
ExportView->>ExportView: Extracts workspace_id
ExportView->>Validator: Validate workspace_id
Validator-->>ExportView: Validation result
ExportView->>QueueModule: Call async_import_and_export_expenses(body, workspace_id)
QueueModule->>Logger: Log the process
QueueModule->>QueueModule: Perform import/export operations
QueueModule-->>ExportView: Return result
ExportView-->>Client: Respond with result
Poem
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 (
|
|
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (5)
apps/fyle/queue.py (1)
[!TIP]
Codebase VerificationVerification of
workspace_idparameter usage inasync_import_and_export_expensesfunction calls:
tests/test_fyle/test_queue.py
- Line:
async_import_and_export_expenses(body, worksapce.id)(Note:worksapce.idseems like a typo, should beworkspace.id)
apps/fyle/views.py
- Line:
async_import_and_export_expenses(request.data, int(kwargs['workspace_id']))These lines confirm that the new
workspace_idparameter is being passed in the function calls.
- Typo in
tests/test_fyle/test_queue.py:worksapce.idshould be corrected toworkspace.id.Analysis chain
Line range hint
20-36: Review of updated function with added workspace_id parameter and logging.The addition of the
workspace_idparameter and the corresponding validation and logging enhance the function's capability to handle different workspaces and improve traceability. Ensure that all calls to this function across the application have been updated to pass the new parameter.Consider abstracting the repeated pattern of fetching
org_idand logging into a separate helper function to reduce code duplication and improve maintainability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `async_import_and_export_expenses` include the `workspace_id` parameter. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'async_import_and_export_expenses'Length of output: 82
Script:
#!/bin/bash # Description: Verify all function calls to `async_import_and_export_expenses` include the `workspace_id` parameter. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg -t py 'async_import_and_export_expenses'Length of output: 615
tests/test_fyle/test_task.py (1)
Line range hint
95-95: Refactor boolean checks to follow Pythonic practices.Replace direct comparisons to
TrueandFalsewith more idiomatic Python constructs.- if some_condition == False: - # do something + if not some_condition: + # do something - if some_condition == True: - # do something + if some_condition: + # do somethingAlso applies to: 103-103
apps/fyle/helpers.py (3)
Line range hint
32-32: Refactor string formatting to use f-stringsThe use of
.format()is outdated. Python's f-strings provide a more readable and performant way to format strings.- api_headers["Authorization"] = "Bearer {0}".format(access_token) + api_headers["Authorization"] = f"Bearer {access_token}"Repeat this change for all instances where
.format()is used.Also applies to: 104-104, 145-145, 156-156
Line range hint
110-110: Useis notfor identity testsUsing
is not Noneis more idiomatic and clear thannot ... is None.- if not params[k] is None: + if params[k] is not None:
Line range hint
212-212: Remove redundant conditional expressionThe ternary operation is unnecessary when converting a boolean.
- value = True if str(value) == 'true' else False + value = str(value) == 'true'
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- apps/exceptions.py (2 hunks)
- apps/fyle/helpers.py (2 hunks)
- apps/fyle/queue.py (3 hunks)
- apps/fyle/tasks.py (2 hunks)
- apps/fyle/views.py (1 hunks)
- requirements.txt (1 hunks)
- tests/test_fyle/fixtures.py (1 hunks)
- tests/test_fyle/test_queue.py (2 hunks)
- tests/test_fyle/test_task.py (2 hunks)
Files skipped from review due to trivial changes (1)
- requirements.txt
Additional context used
Ruff
tests/test_fyle/test_task.py
95-95: Avoid equality comparisons to
False; useif not ...:for false checks (E712)Replace comparison
103-103: Avoid equality comparisons to
True; useif ...:for truth checks (E712)Replace comparison
apps/fyle/helpers.py
32-32: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
32-32: Use f-string instead of
formatcall (UP032)Convert to f-string
104-104: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
104-104: Use f-string instead of
formatcall (UP032)Convert to f-string
110-110: Test for object identity should be
is not(E714)Convert to
is not
145-145: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
145-145: Use f-string instead of
formatcall (UP032)Convert to f-string
156-156: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
156-156: Use f-string instead of
formatcall (UP032)Convert to f-string
212-212: Remove unnecessary
True if ... else False(SIM210)Remove unnecessary
True if ... else Falseapps/fyle/tasks.py
325-328: Use
expense_state = expense.accounting_export_summary.get("state", "NOT_EXPORTED")instead of anifblock (SIM401)Replace with
expense_state = expense.accounting_export_summary.get("state", "NOT_EXPORTED")
Additional comments not posted (7)
apps/fyle/queue.py (1)
1-6: Ensure appropriate logging level and consistency in usage.The logger is configured at the INFO level, which is suitable for production environments if the intention is to log high-level operational details. Ensure that this level is consistent with the overall application's logging strategy.
tests/test_fyle/test_queue.py (1)
44-48: Ensure correct handling of workspace creation or update in tests.The test logic for creating or updating a workspace instance seems correct. However, ensure that the workspace handling does not introduce side effects that could affect other tests or the application environment.
apps/fyle/views.py (1)
120-122: Review of updatedpostmethod inExportViewwith workspace_id handling.The addition of
workspace_idhandling in thepostmethod is a necessary update to support multi-tenancy. Ensure that the rest of the application correctly handles this parameter and that it is consistently used in related API calls.Verification successful
Consistent Handling of
workspace_idVerifiedThe
workspace_idparameter is consistently used across theapps/fyle/views.pyfile. The updatedpostmethod in theExportViewclass correctly handles this parameter, and similar methods in the file also useworkspace_idappropriately.
lookup_field = "workspace_id"lookup_url_kwarg = "workspace_id"async_import_and_export_expenses(request.data, int(kwargs['workspace_id']))Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent handling of `workspace_id` across similar views. # Test: Search for similar views handling `workspace_id`. Expect: Consistent usage. rg --type python 'workspace_id' -- 'apps/fyle/views.py'Length of output: 84
Script:
#!/bin/bash # Description: Verify consistent handling of `workspace_id` across similar views. # Test: Search for similar views handling `workspace_id`. Expect: Consistent usage. rg 'workspace_id' apps/fyle/views.pyLength of output: 621
apps/exceptions.py (1)
105-110: Proper handling ofValidationErrorwith detailed logging and response.The handling of
ValidationErroris appropriately implemented with detailed logging and a specific response that includes the error details. This enhances error traceability and user feedback.apps/fyle/helpers.py (2)
8-9: Added import for ValidationErrorThis import is necessary for the new exception handling logic introduced in the
assert_valid_requestfunction.
13-13: Updated imports to include WorkspaceThe addition of
Workspaceto the imports is necessary for the new validation functionassert_valid_requestwhich uses this model.tests/test_fyle/fixtures.py (1)
2-265: Expanded test data structures infixtures.pyThe expansion of the test data dictionary to include detailed fields such as
accounting_export_summary,approvals,custom_fields, etc., enhances the test coverage by providing a more comprehensive dataset for testing.
| def assert_valid_request(workspace_id:int, fyle_org_id:str): | ||
| """ | ||
| Assert if the request is valid by checking | ||
| the url_workspace_id and fyle_org_id workspace | ||
| """ | ||
| workspace = Workspace.objects.get(fyle_org_id=fyle_org_id) | ||
| if workspace.id != workspace_id: | ||
| raise ValidationError('Workspace mismatch') | ||
|
|
There was a problem hiding this comment.
New validation function: assert_valid_request
This function provides essential validation for workspace and organization ID matching, which is crucial for security and data integrity. However, the function could potentially raise an unhandled Workspace.DoesNotExist exception if no workspace matches the provided fyle_org_id.
- workspace = Workspace.objects.get(fyle_org_id=fyle_org_id)
+ workspace = Workspace.objects.filter(fyle_org_id=fyle_org_id).first()
+ if not workspace:
+ raise ValidationError('Workspace not found')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 assert_valid_request(workspace_id:int, fyle_org_id:str): | |
| """ | |
| Assert if the request is valid by checking | |
| the url_workspace_id and fyle_org_id workspace | |
| """ | |
| workspace = Workspace.objects.get(fyle_org_id=fyle_org_id) | |
| if workspace.id != workspace_id: | |
| raise ValidationError('Workspace mismatch') | |
| def assert_valid_request(workspace_id:int, fyle_org_id:str): | |
| """ | |
| Assert if the request is valid by checking | |
| the url_workspace_id and fyle_org_id workspace | |
| """ | |
| workspace = Workspace.objects.filter(fyle_org_id=fyle_org_id).first() | |
| if not workspace: | |
| raise ValidationError('Workspace not found') | |
| if workspace.id != workspace_id: | |
| raise ValidationError('Workspace mismatch') |
| def update_non_exported_expenses(data: Dict) -> None: | ||
| """ | ||
| To update expenses not in COMPLETE, IN_PROGRESS state | ||
| """ | ||
| expense_state = None | ||
| org_id = data['org_id'] | ||
| expense_id = data['id'] | ||
| workspace = Workspace.objects.get(fyle_org_id = org_id) | ||
| expense = Expense.objects.filter(workspace_id=workspace.id, expense_id=expense_id).first() | ||
|
|
||
| if expense: | ||
| if 'state' in expense.accounting_export_summary: | ||
| expense_state = expense.accounting_export_summary['state'] | ||
| else: | ||
| expense_state = 'NOT_EXPORTED' | ||
|
|
||
| if expense_state and expense_state not in ['COMPLETE', 'IN_PROGRESS']: | ||
| expense_obj = [] | ||
| expense_obj.append(data) | ||
| expense_objects = FyleExpenses().construct_expense_object(expense_obj, expense.workspace_id) | ||
| Expense.create_expense_objects( | ||
| expense_objects, expense.workspace_id | ||
| ) |
There was a problem hiding this comment.
New function to update non-exported expenses
This function updates expenses that are not in the COMPLETE or IN_PROGRESS state. It's a valuable addition for handling edge cases in expense management. However, the implementation can be optimized by using .get() for dictionary access with a default value.
- if 'state' in expense.accounting_export_summary:
- expense_state = expense.accounting_export_summary['state']
- else:
- expense_state = 'NOT_EXPORTED'
+ expense_state = expense.accounting_export_summary.get('state', 'NOT_EXPORTED')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 update_non_exported_expenses(data: Dict) -> None: | |
| """ | |
| To update expenses not in COMPLETE, IN_PROGRESS state | |
| """ | |
| expense_state = None | |
| org_id = data['org_id'] | |
| expense_id = data['id'] | |
| workspace = Workspace.objects.get(fyle_org_id = org_id) | |
| expense = Expense.objects.filter(workspace_id=workspace.id, expense_id=expense_id).first() | |
| if expense: | |
| if 'state' in expense.accounting_export_summary: | |
| expense_state = expense.accounting_export_summary['state'] | |
| else: | |
| expense_state = 'NOT_EXPORTED' | |
| if expense_state and expense_state not in ['COMPLETE', 'IN_PROGRESS']: | |
| expense_obj = [] | |
| expense_obj.append(data) | |
| expense_objects = FyleExpenses().construct_expense_object(expense_obj, expense.workspace_id) | |
| Expense.create_expense_objects( | |
| expense_objects, expense.workspace_id | |
| ) | |
| def update_non_exported_expenses(data: Dict) -> None: | |
| """ | |
| To update expenses not in COMPLETE, IN_PROGRESS state | |
| """ | |
| expense_state = None | |
| org_id = data['org_id'] | |
| expense_id = data['id'] | |
| workspace = Workspace.objects.get(fyle_org_id = org_id) | |
| expense = Expense.objects.filter(workspace_id=workspace.id, expense_id=expense_id).first() | |
| if expense: | |
| expense_state = expense.accounting_export_summary.get('state', 'NOT_EXPORTED') | |
| if expense_state and expense_state not in ['COMPLETE', 'IN_PROGRESS']: | |
| expense_obj = [] | |
| expense_obj.append(data) | |
| expense_objects = FyleExpenses().construct_expense_object(expense_obj, expense.workspace_id) | |
| Expense.create_expense_objects( | |
| expense_objects, expense.workspace_id | |
| ) |
Tools
Ruff
325-328: Use
expense_state = expense.accounting_export_summary.get("state", "NOT_EXPORTED")instead of anifblock (SIM401)Replace with
expense_state = expense.accounting_export_summary.get("state", "NOT_EXPORTED")
|
| elif body.get('action') == 'UPDATED_AFTER_APPROVAL' and body.get('data') and body.get('resource') == 'EXPENSE': | ||
| org_id = body['data']['org_id'] | ||
| logger.info("| Updating non-exported expenses through webhook | Content: {{WORKSPACE_ID: {} Payload: {}}}".format(workspace_id, body.get('data'))) | ||
| assert_valid_request(workspace_id=workspace_id, fyle_org_id=org_id) |
There was a problem hiding this comment.
let's move this out of the if block since this is repeated in both blocks and common for both
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
No description provided.