Feat: Adding Created_by and updated_by to conf tables #689
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThis pull request introduces a comprehensive set of changes focused on enhancing user tracking and context across various models and serializers in the Django application. The modifications primarily involve adding Changes
Sequence DiagramsequenceDiagram
participant User
participant View
participant Serializer
participant Model
User->>View: Make Request
View->>Serializer: Pass Request Context
Serializer->>Model: Update with User Context
Model-->>Serializer: Return Updated Instance
Serializer-->>View: Return Response
View-->>User: Send Response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 0
🧹 Nitpick comments (3)
apps/mappings/models.py (1)
26-26: Encourage database-level constraints or foreign keys for users.While adding these user tracking fields (
created_by,updated_by) as text is straightforward, consider the benefits of referencing a user table via a foreign key if the system’s architecture and usage patterns permit it. This approach offers enhanced referential integrity and easier user-level data auditing in the future.apps/workspaces/models.py (1)
10-10: ImportingAutoAddCreateUpdateInfoMixin.This mixin presumably handles
created_by/updated_by. Ensure it does not override any existing fields or signals in a conflicting way.apps/fyle/models.py (1)
261-261: Add parameter documentation for the newuserargument.The
update_expense_group_settingsmethod now takes auserparameter but the docstring does not reflect this. Update the function docstring (and type hints, if needed) to ensure clarity on howuseris used and expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/fyle/migrations/0037_auto_20241226_0929.py(1 hunks)apps/fyle/models.py(4 hunks)apps/fyle/views.py(1 hunks)apps/mappings/migrations/0016_auto_20241226_0929.py(1 hunks)apps/mappings/models.py(3 hunks)apps/workspaces/apis/export_settings/serializers.py(5 hunks)apps/workspaces/apis/export_settings/views.py(1 hunks)apps/workspaces/apis/import_settings/serializers.py(2 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/0043_auto_20241224_1102.py(1 hunks)apps/workspaces/models.py(2 hunks)requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🔇 Additional comments (22)
apps/workspaces/apis/map_employees/views.py (1)
12-20: Good addition for user context
Including the request in the serializer context is a standard approach to enable user-specific logic in updates and validations. This looks good.
apps/workspaces/apis/import_settings/views.py (1)
12-20: Consistency with other views
Your approach to add the request to the serializer context provides a consistent mechanism for accessing the current user in the serializer. This is aligned with best practices.
apps/workspaces/apis/export_settings/views.py (1)
13-21: Context injection confirmed
Exposing the request in the serializer context is correct for enabling user-based logic in the ExportSettingsSerializer. This maintains a uniform pattern across the application.
apps/workspaces/migrations/0043_auto_20241224_1102.py (1)
13-22: Fields added successfully
Adding created_by and updated_by as nullable CharFields is a straightforward approach for tracking user information. Ensure your application logic sets these fields correctly during record creation and updates.
apps/mappings/migrations/0016_auto_20241226_0929.py (1)
13-22: Properly handle null and blank fields for user tracking.
By making created_by and updated_by nullable and allowing blank values, it ensures flexibility. However, consider whether to enforce these fields during object creation or update for consistency. If your workflow requires guaranteed user tracking, you may wish to make these fields mandatory or enforce them at the application logic level.
apps/fyle/migrations/0037_auto_20241226_0929.py (1)
13-22: Keep consistency in user tracking schema.
Similar to the previous migration, both created_by and updated_by are nullable. Confirm that the updated code flow populates these fields consistently where user identity is known. This helps maintain clean, accurate data regarding record creation and updates.
apps/workspaces/apis/map_employees/serializers.py (2)
26-27: Validate the presence of request and user prior to usage.
Since request is accessed from the serializer context, ensure that all calling code paths include this context. Failing to do so may lead to AttributeError in environments without an HTTP request context (e.g., background jobs). Additionally, consider whether any additional checks or fallback logic are needed if request.user is anonymous or missing.
39-40: Supply user data responsibly to ensure accurate auditing.
Passing the user to update_or_create is a good way to track who performed the update. Ensure that any user-handling code in the model or manager sets the corresponding updated_by field automatically or consistently. This may help centralize the logic for user assignments and avoid duplicating it at the serializer level.
apps/mappings/models.py (1)
7-7: Ensure mixin availability and intended behavior.
The newly imported AutoAddCreateUpdateInfoMixin can override or supplement save() to manage timestamps or additional fields. Verify that the mixin doesn’t conflict with existing logic in the model. Review the mixin’s methods for any overrides or pre/post-save hooks that might require special handling.
apps/workspaces/apis/import_settings/serializers.py (3)
112-113: Ensure request object presence and fallback.
Retrieving user from the serializer's context provides flexibility for DRF integration. Consider verifying that request.user is not an anonymous or invalid user before proceeding with the update logic.
128-128: Passing user to Configuration.objects.update_or_create.
This change seems correct for capturing audit information. Ensure that the user parameter is properly handled in the update_or_create call, so it won't inadvertently overwrite critical fields if user is None.
131-131: Extend user-driven auditing.
Similarly, including user in GeneralMapping.objects.update_or_create ensures consistent logging of who performed the operation. Confirm that the GeneralMapping model or mixin properly processes the user parameter for auditing (e.g., updated_by).
apps/workspaces/apis/export_settings/serializers.py (5)
35-35: Trailing comma improvement.
Adding a trailing comma in the fields list is beneficial for clean diffs in future modifications. No functional impact observed.
123-124: Retrieve user from request context.
This approach aligns with Django REST best practices for capturing the authenticated user. If there is a possibility of request being absent, the fallback to None is prudent. However, ensure that audit fields (e.g., created_by, updated_by) do not break if user is None.
139-140: Include user when updating Configuration.
Great step for user-driven auditing. Double-check that nullable or anonymous users are handled correctly, so as not to disrupt mandatory auditing fields.
162-162: User-provided context for ExpenseGroupSettings.update_expense_group_settings.
Passing user ensures future traceability of changes in ExpenseGroupSettings. Ensure that the consuming method properly logs or assigns those fields without raising exceptions when user is missing.
175-176: Consistent auditing for GeneralMapping.
Same concerns as above. Verify that the GeneralMapping model or the relevant mixin properly captures these updates. If the model requires a non-null user, consider adding validations or fallback logic.
apps/workspaces/models.py (1)
138-138: Enhance Configuration with auditing mixin.
By inheriting from AutoAddCreateUpdateInfoMixin, the model can automatically track creation and update information. Confirm that default values for created_by and updated_by match your requirements, including fallback logic if no user is specified.
apps/fyle/views.py (1)
152-152: Provide user to update_expense_group_settings.
Forwarding the request user ensures the updates are attributed correctly in the ExpenseGroupSettings. If your system allows anonymous access in any scenario, be sure to handle user=None gracefully in the model or the update method.
apps/fyle/models.py (3)
18-18: Ensure correct usage of the imported mixin.
The AutoAddCreateUpdateInfoMixin import is presumably used for automatically capturing creation and update metadata. Verify that dependencies and migrations are in place for the mixin’s functionality (i.e., any required model fields such as created_by and updated_by).
221-221: Inheritance structure looks good.
ExpenseGroupSettings now inherits from AutoAddCreateUpdateInfoMixin. Confirm that the mixin correctly populates created_by and updated_by upon creation and updates, and that any required migrations are properly applied. Remember to handle scenarios where the user is null or missing.
331-332: Verify custom usage of user in update_or_create.
Django’s built-in update_or_create doesn’t natively accept a user argument. Confirm that the AutoAddCreateUpdateInfoMixin handles this parameter. If it relies on signals or method overrides, ensure they are implemented correctly to associate records with the correct user.
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_workspaces/test_views.py (1)
300-302: Add assertions to validate changes tocreated_byandupdated_by.
While manually settingconfiguration.created_byandconfiguration.updated_byis valid, consider asserting these fields in the test to ensure they’re stored as expected.tests/sql_fixtures/reset_db_fixtures/reset_db.sql (2)
327-329: LGTM! Consider NOT NULL constraints for future migrations.The schema changes to add
created_byandupdated_bycolumns look good. However, consider adding NOT NULL constraints in a future migration once all existing records are backfilled with appropriate values.
Line range hint
327-11927: Consider implementing a database-level audit trail.While the current approach of adding
created_byandupdated_byfields is good, consider implementing a more comprehensive audit trail at the database level (e.g., using triggers or audit tables) for better tracking of all changes. This would provide:
- Immutable history of all changes
- Ability to track who made what changes and when
- Compliance with audit requirements
tests/test_workspaces/data.json (1)
29-31: Consider using more realistic test data valuesThe current test data uses random strings ("aksndbkas", "asvhdvsaj") for the new user tracking fields. Consider using more realistic values that match the expected format of user identifiers in your system (e.g., email addresses or user IDs) to ensure the tests can catch potential format-related issues.
"is_attachment_upload_enabled": true, - "created_by": "aksndbkas", - "updated_by": "asvhdvsaj" + "created_by": "[email protected]", + "updated_by": "[email protected]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql(8 hunks)tests/test_fyle/test_models.py(1 hunks)tests/test_workspaces/data.json(1 hunks)tests/test_workspaces/test_views.py(1 hunks)
🔇 Additional comments (8)
tests/test_fyle/test_models.py (2)
44-44: Consider verifying the workspace IDs for user retrieval.
You're retrieving the user from Workspace.objects.get(id=1) but using 3 as the workspace ID in the update_expense_group_settings call. Double-check if referencing different IDs is intentional or might cause inconsistencies in data updates.
47-47: Ensure alignment of user context across the codebase.
The addition of the user parameter here is aligned with attaching user context to the settings’ update. Confirm that other calling sites and related model logic now handle the user argument consistently (e.g., avoiding a missing or mismatched user).
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (6)
966-968: LGTM! Schema changes are consistent.
1452-1454: LGTM! Schema changes maintain consistency.
11602-11605: LGTM! Data migration approach is consistent.
The COPY commands for both tables follow the same pattern, maintaining NULL values for existing records.
Also applies to: 11681-11684
11927-11927: LGTM! Sequence value correctly updated.
8013-8015: Verify migration dependencies.
The migration sequence looks good. Let's verify the dependencies between these migrations to ensure they run in the correct order.
✅ Verification successful
Migration dependencies are correctly configured
The migrations have proper dependencies set up:
apps/fyle/migrations/0037_auto_20241226_0929.pydepends on0036_expense_masked_corporate_card_numberapps/mappings/migrations/0016_auto_20241226_0929.pydepends on0015_generalmapping_is_tax_balancing_enabledapps/workspaces/migrations/0043_auto_20241224_1102.pydepends on0042_auto_20241219_1808
Each migration properly references its immediate predecessor within its respective app, maintaining the correct sequence. Since these migrations are in different Django apps (fyle, mappings, workspaces), they can run independently without cross-app dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check migration dependencies
echo "Checking migration dependencies:"
for file in $(fd "0037_auto_20241226_0929.py|0016_auto_20241226_0929.py|0043_auto_20241224_1102.py" "apps/"); do
echo "\nFile: $file"
rg "dependencies" "$file" -A 5
done
Length of output: 1520
2710-2713: Verify application code handles new audit fields.
The data migration looks good with NULL values for existing records. Let's verify the application code properly sets these fields for new records.
✅ Verification successful
Let me gather more specific information about how these fields are handled in the models.
Let me check one final thing about the mixin implementation.
Let me check the signal implementation for Configuration to verify how the audit fields are handled.
Audit fields are properly handled in the Configuration model
The verification shows that:
- The Configuration model inherits from AutoAddCreateUpdateInfoMixin
- The model is used with proper serializers in various endpoints
- While the mixin implementation isn't directly visible, the model's inheritance and presence of post_save signals suggest proper handling of audit fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AutoAddCreateUpdateInfoMixin usage in models
echo "Checking for mixin usage in models:"
rg -l "AutoAddCreateUpdateInfoMixin" "apps/"
# Search for created_by/updated_by handling in views/serializers
echo "\nChecking field handling in views/serializers:"
rg "created_by|updated_by" "apps/" --type py -l
Length of output: 500
Script:
#!/bin/bash
# Check implementation of AutoAddCreateUpdateInfoMixin
echo "Checking AutoAddCreateUpdateInfoMixin implementation:"
rg -A 10 "class AutoAddCreateUpdateInfoMixin" apps/
# Check models using this mixin
echo "\nChecking models with audit fields:"
ast-grep --pattern 'class $_(AutoAddCreateUpdateInfoMixin$$$):'
# Check if configurations model uses this mixin
echo "\nChecking configurations model:"
rg -A 5 "class Configuration" apps/
Length of output: 3551
Script:
#!/bin/bash
# Search for the mixin implementation in common/models.py
echo "Checking mixin implementation in common models:"
rg -A 15 "class AutoAddCreateUpdateInfoMixin" "apps/common/models.py"
# Check if there are any pre_save or post_save signals for Configuration
echo "\nChecking for signals:"
rg "pre_save|post_save.*Configuration" "apps/" --type py
Length of output: 1336
Script:
#!/bin/bash
# Check Configuration signal implementation
echo "Checking Configuration signal handler:"
rg -A 10 "@receiver.*Configuration" "apps/workspaces/signals.py"
# Check if there's a base mixin implementation
echo "\nChecking for base mixin in workspaces:"
rg -A 10 "class AutoAddCreateUpdateInfoMixin" "apps/workspaces/models.py"
Length of output: 765
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_fyle/fixtures.py (1)
1018-1019: Ensure consistency of new fields with data privacy and naming conventions.Adding
created_byandupdated_byfields is aligned with the PR objectives of tracking user context. Consider verifying that the email values are properly anonymized or test-safe, and confirm these fields' data usage complies with any applicable security, privacy, or naming conventions throughout the codebase.
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
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
New Features
created_byandupdated_byfields to multiple models, allowing tracking of user actions.Bug Fixes
Chores
fyle-accounting-mappings.