Feat: Add created_by, updated_by to conf tables #710
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThis pull request introduces a comprehensive user tracking mechanism across multiple models in the Django application. The changes involve adding Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 1
🧹 Nitpick comments (7)
tests/test_fyle/fixtures.py (1)
Line range hint
1-4000: Consider adding more edge cases for user trackingConsider adding test fixtures for the following scenarios:
- User updates by different users
- Multiple updates tracking history
- Invalid user format validation
- System-generated updates
Example additions:
'expense_group_settings_response': { # ... existing fields ... + 'created_by': '[email protected]', + 'updated_by': '[email protected]', + 'update_history': [ + { + 'updated_by': '[email protected]', + 'updated_at': '2022-01-01T00:00:00Z' + }, + { + 'updated_by': '[email protected]', + 'updated_at': '2022-01-02T00:00:00Z' + } + ] }apps/workspaces/apis/map_employees/views.py (1)
12-20: Well-structured approach for passing user context to the serializer.
Providing the full request object in the serializer context is a common and valid approach in Django REST Framework, allowing the serializer to conveniently access the requesting user forcreated_byorupdated_by. However, if only the user is needed, consider storing just theuserobject (e.g.,context['user'] = self.request.user) to keep the context minimal.apps/fyle/migrations/0041_auto_20241226_1155.py (2)
13-17: Encourage consistent usage ofcreated_by.The new
created_byfield will enhance audit logging. Ensure the adding code in your model or manager sets this field on creation. Consider test coverage that checks for non-nullcreated_byin typical creation flows.
18-22: Validate and handle missingupdated_byusage.Similar to
created_by, ensure that all update flows properly setupdated_by. If the system can't always identify a user (e.g., automated tasks), decide how to handle that scenario to maintain consistent data lineage.apps/mappings/models.py (1)
7-7: Add a note about the new mixin's functionality.It's good that this mixin is imported. Consider adding a short docstring or comment in your model to clarify that this mixin handles
created_byandupdated_bylogic automatically, which will help future contributors quickly understand its purpose.apps/workspaces/apis/export_settings/serializers.py (1)
135-135: Ensure credit card account mapping is user-aware
When creating or updating aMappingSettingfor credit card accounts, confirm that the user context is genuinely needed and used. If theuseris not essential for certain system-level mappings, consider gracefully handlingNone.tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
913-915: Consider implementing comprehensive audit logging.While adding user tracking columns is a good start, consider enhancing the audit trail implementation:
- Add timestamp columns (
created_at,updated_at) if not already present- Implement audit logging table to track all changes with:
- Previous and new values
- Change type (CREATE, UPDATE, DELETE)
- User context (IP, user agent)
- Consider using Django's built-in audit logging capabilities
Would you like me to provide example SQL schema for an audit logging table?
Also applies to: 1110-1112, 1290-1292, 1896-1898
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/fyle/migrations/0041_auto_20241226_1155.py(1 hunks)apps/fyle/models.py(4 hunks)apps/fyle/views.py(1 hunks)apps/mappings/migrations/0011_auto_20241226_1155.py(1 hunks)apps/mappings/models.py(1 hunks)apps/workspaces/apis/advanced_configurations/serializers.py(2 hunks)apps/workspaces/apis/advanced_configurations/views.py(1 hunks)apps/workspaces/apis/export_settings/serializers.py(4 hunks)apps/workspaces/apis/export_settings/views.py(1 hunks)apps/workspaces/apis/import_settings/serializers.py(3 hunks)apps/workspaces/apis/import_settings/views.py(1 hunks)apps/workspaces/apis/map_employees/serializers.py(2 hunks)apps/workspaces/apis/map_employees/views.py(1 hunks)apps/workspaces/migrations/0050_auto_20241226_1155.py(1 hunks)apps/workspaces/models.py(2 hunks)requirements.txt(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(10 hunks)tests/test_fyle/fixtures.py(1 hunks)tests/test_fyle/test_models.py(1 hunks)tests/test_workspaces/fixtures.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🔇 Additional comments (33)
tests/test_fyle/fixtures.py (3)
3163-3164: Add user tracking fields to expense group settings response
The expense group settings response has been updated to include created_by and updated_by fields, which aligns with the PR objective of enhancing user tracking capabilities.
Line range hint 1-3162: LGTM: Test fixtures look comprehensive and well-structured
The test fixtures provide good coverage for testing various scenarios:
- Raw expense data with detailed fields
- Default raw expense with essential fields
- Multiple expense scenarios (spent_at, refunds, etc.)
- Corporate card expenses with different bank transaction ID cases
- Expense group responses with various states
- Custom field configurations
- Employee, category, and cost center data
The data structure is consistent and includes all necessary fields for testing the user tracking enhancements.
Also applies to: 3165-4000
Line range hint 1-4000: Verify test coverage for user tracking fields
While the fixtures include the new user tracking fields, we should verify that there are sufficient test cases covering:
- Creation of new records with user context
- Updates to existing records with user context
- Edge cases like null/empty user values
- Validation of user field formats
apps/workspaces/apis/export_settings/views.py (1)
12-20: Consistent inclusion of request in serializer context for user tracking.
Similar to the MapEmployeesView, passing the request object here is beneficial for extracting the user in the serializer. This consistency across views ensures straightforward maintenance and audit of user-based changes.
apps/workspaces/apis/advanced_configurations/views.py (1)
12-20: Provides a unified pattern for user context availability.
By adding context['request'] = self.request, all update operations that require user information will remain consistent with other parts of the codebase. This pattern supports improved traceability for updates.
apps/mappings/migrations/0011_auto_20241226_1155.py (1)
12-23: Fields added for auditability and traceability.
Adding created_by and updated_by as nullable CharFields is a straightforward solution for capturing user data. Consider these points for long-term maintainability:
• Ensure that 255 characters suffice for storing user identifiers such as emails.
• Validate whether indexing is needed if queries based on these fields become frequent.
• If the user base grows or if verifying references is required, you may want to store a foreign key in the future.
apps/workspaces/migrations/0050_auto_20241226_1155.py (2)
13-17: Ensure the created_by field is consistently populated.
It's good that created_by is nullable and can be blank, but make sure all code paths consistently set this field at creation time. If a user context isn't available, consider documenting or defaulting behavior to maintain data integrity.
18-22: Confirm update logic for updated_by usage.
updated_by is designed to track the last user who updated the record. Verify that the model or resource layer automatically sets this field whenever a record is updated. If not, it may lead to missing user data for certain update paths.
[approve]
apps/workspaces/apis/import_settings/views.py (1)
14-21: Great addition for user context in serializer.
Exposing the request through get_serializer_context is a clean approach. Make sure your serializers handle edge cases where the request might be absent (e.g., background tasks without an HTTP context).
apps/workspaces/apis/map_employees/serializers.py (2)
26-28: Gracefully handle request or user absence.
When request or user is missing, you fall back to None. Make sure your logic downstream can handle None for user (e.g., logging or validations).
39-40: User parameter now passed to update_or_create.
This aligns with the new auditing requirements. Confirm that update_or_create properly consumes user and sets updated_by. If user is None, confirm you have fallback behavior.
apps/mappings/models.py (1)
10-10: Validate user assignment when records are created or updated.
While inheriting from AutoAddCreateUpdateInfoMixin adds fields for created_by and updated_by, ensure the user object is consistently available at all model creation or update call sites so that these fields are populated correctly.
apps/workspaces/apis/advanced_configurations/serializers.py (2)
69-71: Ensure the user context can't be None when needed.
Checking request.user is helpful, but if the user is None, the tracking fields won’t be correctly populated. If there's a scenario where the user might not be logged in, handle it explicitly.
87-87: Great approach to capturing the user context.
Passing user to both WorkspaceGeneralSettings.objects.update_or_create(...) and GeneralMapping.objects.update_or_create(...) ensures accurate tracking of created_by and updated_by. Just confirm these calls fail gracefully when no user is provided.
Also applies to: 90-90
apps/fyle/views.py (1)
86-86: Consistent user tracing.
Including request.user when updating ExpenseGroupSettings aligns with the newly introduced fields. Ensure request authentication is mandatory here so request.user is never undefined.
tests/test_workspaces/fixtures.py (1)
77-78: Good coverage with test fixtures.
Including created_by and updated_by in these fixtures helps validate user tracking logic. Consider adding test assertions that verify these fields are accurately set and retrieved.
apps/workspaces/apis/import_settings/serializers.py (4)
81-83: Ensure robust handling of the user in the request context
The logic for retrieving user from request looks correct, but note that request or user might be None for certain background processes or automated tasks. Consider graceful handling or fallback defaults if user is not found.
110-110: Confirm existence of updated manager or model method
Passing user=user to WorkspaceGeneralSettings.objects.update_or_create() implies that your model manager or the update_or_create method supports a user parameter. Ensure the underlying logic consumes this parameter as intended and handles edge cases (e.g., user is None).
113-113: Check consistent usage of user parameter across all calls
Similar to the previous comment, confirm that GeneralMapping.objects.update_or_create(..., user=user) is fully implemented. If the user parameter is optional, add safe handling.
136-136: Review iteration-based updates
Here, user=user is also passed within a loop. Verify that, for each MappingSetting update, passing the same user is correct, especially if any user-based logic might differ across individual records.
apps/workspaces/apis/export_settings/serializers.py (4)
96-98: Retrieve user from request for export settings
Your approach to extracting the user is consistent with the import settings serializer. Just be sure to handle fallback logic if request.user is unset.
127-127: Confirm user parameter usage in update_or_create
Passing user suggests you have a custom manager or method that records this information. Confirm that the logic handles null or unknown users without errors.
158-158: Validate optional user passing to ExpenseGroupSettings.update_expense_group_settings
Ensure the newly added user parameter is consistently utilized or handle the case when user is None.
176-176: Extend user auditing to general mappings
Similar to the above methods, confirm the logic in GeneralMapping.objects.update_or_create(..., user=user) is well-tested and handles all edge cases.
apps/workspaces/models.py (2)
10-11: New mixin import
By importing and using AutoAddCreateUpdateInfoMixin, you add automatic tracking for creation and update metadata. This import is correct if the mixin logic is tested and aligns with your auditing requirements.
95-95: Inheritance from AutoAddCreateUpdateInfoMixin
Mixing AutoAddCreateUpdateInfoMixin and models.Model ensures WorkspaceGeneralSettings includes required auditing fields or methods. Confirm that existing migrations and DB schema are compatible with the new fields.
apps/fyle/models.py (4)
206-206: Appropriate class extension.
Inheriting from AutoAddCreateUpdateInfoMixin here correctly integrates the new user tracking fields (created_by, updated_by). This is aligned with the PR objective to enhance auditability. The usage looks good.
306-306: Confirm user assignment in update_or_create.
The signature implies that user will be stored in your custom mixin fields (created_by, updated_by). Double-check that the mixin’s save() method properly sets these fields, and confirm that no override is needed for the update_or_create usage.
Do you want me to examine the mixin’s implementation or open a GitHub issue to verify proper usage of created_by and updated_by?
230-230: Validate the user parameter usage.
Ensure that passing user into update_expense_group_settings is always possible from the various call sites. If request.user can be anonymous in any scenario, consider fallback logic or explicitly ensure authentication is required.
✅ Verification successful
Let me verify the authentication mechanisms in the views and APIs where this function is called.
Let me check the specific authentication setup in the views file.
Let me check the specific view class that handles expense group settings.
Let me check one final thing about the base class and middleware setup.
Let me check the Django settings in the correct location.
Let me check the actual content of the main settings file and any middleware configuration.
The user parameter is properly secured in update_expense_group_settings
Based on the verification results:
- The application has global authentication configured in
settings.py:REST_FRAMEWORK = { 'DEFAULT_PERMISSION_CLASSES': ('rest_framework.permissions.IsAuthenticated', 'apps.workspaces.permissions.WorkspacePermissions'), 'DEFAULT_AUTHENTICATION_CLASSES': ('fyle_rest_auth.authentication.FyleJWTAuthentication',) }
- The
ExpenseGroupSettingsViewinherits fromgenerics.ListCreateAPIViewand is protected by the global authentication middleware. - All call sites pass either
request.useror an authenticated user object.
The user parameter is properly secured through Django's authentication system and workspace permissions, ensuring that anonymous users cannot access this functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all function calls to update_expense_group_settings provide a non-null user.
rg -A2 $'update_expense_group_settings\\('
Length of output: 1210
Script:
#!/bin/bash
# Check authentication decorators and base classes in the relevant views
ast-grep --pattern 'class $class_name($$base_classes):
$$$
def $method($params) {
$$$
update_expense_group_settings($$$)
$$$
}' apps/fyle/views.py apps/workspaces/apis/export_settings/serializers.py
# Also check for authentication middleware and permissions
rg -l "permission_classes|IsAuthenticated|authentication_classes" apps/fyle/views.py apps/workspaces/apis/export_settings/views.py
Length of output: 370
Script:
#!/bin/bash
# Check the specific authentication and permission setup in views.py
rg -B2 -A2 "permission_classes|IsAuthenticated|authentication_classes" apps/fyle/views.py
# Also check the base view class for the expense group settings
ast-grep --pattern 'class ExpenseGroupSettings$_($base):
$$$' apps/fyle/views.py
Length of output: 291
Script:
#!/bin/bash
# Find the view class that handles expense group settings
rg -B3 -A3 "class.*ExpenseGroupSettings.*View" apps/fyle/views.py
# Also check the URLs to see the view registration
rg "ExpenseGroupSettings.*View" apps/fyle/urls.py
Length of output: 414
Script:
#!/bin/bash
# Check the content of the ExpenseGroupSettingsView class
rg -B5 -A10 "class ExpenseGroupSettingsView" apps/fyle/views.py
# Also check for any global authentication middleware in settings
rg -l "REST_FRAMEWORK.*authentication" apps/fyle/settings.py apps/fyle/default_settings.py
Length of output: 853
Script:
#!/bin/bash
# Find Django settings files
fd settings.py
# Once found, check for REST_FRAMEWORK authentication settings
rg -l "REST_FRAMEWORK.*authentication" $(fd settings.py)
Length of output: 407
Script:
#!/bin/bash
# Check the main settings file for authentication configuration
rg -A5 "REST_FRAMEWORK|MIDDLEWARE" fyle_qbo_api/settings.py
# Also check if there's any authentication middleware in the base class
rg -A3 "class.*ListCreateAPIView" $(fd -t f -e py)
Length of output: 18487
16-16: Confirm the import path is correct.
Ensure that fyle_accounting_mappings.mixins indeed defines AutoAddCreateUpdateInfoMixin. This avoids potential ImportError or AttributeError if the path is incorrect or the mixin does not exist in that module.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (3)
33272-33277: Data migration strategy looks appropriate.
The data migration approach is correct:
- Existing records are initialized with NULL values for new tracking columns
- This maintains data integrity while allowing future records to capture user information
Also applies to: 33495-33500, 33543-33559, 33924-33929
34031-34031: Sequence update is correct.
The sequence value update to 205 properly accounts for the four new migrations (202-205) that add the user tracking columns.
913-915: Schema changes look good but verify foreign key constraints.
The addition of created_by and updated_by columns is consistent across tables. However, since these columns likely reference user IDs, consider:
- Adding foreign key constraints to the user table
- Adding indexes for query performance
Let's check if there are any foreign key constraints on user-related columns in other tables:
Also applies to: 1110-1112, 1290-1292, 1896-1898
| user = Workspace.objects.get(id=1).user | ||
|
|
||
| ExpenseGroupSettings.update_expense_group_settings(payload, 98) | ||
| ExpenseGroupSettings.update_expense_group_settings(payload, 98, user) |
There was a problem hiding this comment.
Ensure Workspace.objects.get(id=1).user returns a valid user object
Since Workspace.user is a many-to-many field, Workspace.objects.get(id=1).user may return a related manager or queryset rather than a single user. Make sure you fetch the correct user or handle multiple users if the workspace can have more than one.
- user = Workspace.objects.get(id=1).user
+ user = Workspace.objects.get(id=1).user.first() # or handle multiple usersCommittable suggestion skipped: line range outside the PR's diff.
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/6-901605343641-1
Summary by CodeRabbit
Release Notes
New Features
created_byandupdated_byfields to various models for tracking user actions.Bug Fixes
Chores
fyle-accounting-mappings.Tests