apps/workspaces/apis/import_settings/serializers.py (1)

Line range hint 122-122: Pass user to trigger methods to maintain user context

Since the methods pre_save_workspace_general_settings, post_save_workspace_general_settings, pre_save_mapping_settings, and post_save_mapping_settings have been updated to accept the user parameter, consider passing user=user when calling these methods to ensure that user context is maintained throughout the trigger process.

Apply this diff to include user in the method calls:

-            trigger.pre_save_workspace_general_settings(workspace_general_settings)
+            trigger.pre_save_workspace_general_settings(workspace_general_settings, user=user)

-            trigger.post_save_workspace_general_settings(workspace_general_settings_instance)
+            trigger.post_save_workspace_general_settings(workspace_general_settings_instance, user=user)

-            trigger.pre_save_mapping_settings()
+            trigger.pre_save_mapping_settings(user=user)

-            trigger.post_save_mapping_settings(workspace_general_settings_instance)
+            trigger.post_save_mapping_settings(workspace_general_settings_instance, user=user)

Also applies to: 155-155, 160-160, 216-216

tests/test_fyle/fixtures.py (1)

560-561: Use standard test email domains.

While the addition of created_by and updated_by fields aligns with the PR objectives, consider using example.com domain for test emails to follow best practices.

-        "created_by": "ashut@asjkdnsa.com",
-        "updated_by": "asdksaj@gnioa.com"
+        "created_by": "user1@example.com",
+        "updated_by": "user2@example.com"

apps/workspaces/apis/import_settings/views.py (1)

13-20: LGTM! Consider enhancing the docstring.

The implementation correctly adds the request to the serializer context, following Django REST framework patterns.

Consider expanding the docstring to include:

    def get_serializer_context(self):
        """
        Override to include the request in the serializer context.
        This allows serializers to access the current user.
+       Returns:
+           dict: The serializer context including the request object
        """

apps/workspaces/apis/export_settings/views.py (1)

13-20: Consider using a mixin for shared context logic.

While the implementation is correct, this exact code appears in multiple view files. Consider creating a mixin to reduce duplication.

class UserContextMixin:
    def get_serializer_context(self):
        """
        Include the request in the serializer context.
        This allows serializers to access the current user.
        
        Returns:
            dict: The serializer context including the request object
        """
        context = super().get_serializer_context()
        context['request'] = self.request
        return context

class ExportSettingsView(UserContextMixin, generics.RetrieveUpdateAPIView):
    ...

apps/mappings/migrations/0007_auto_20250108_0817.py (1)

14-17: Consider using EmailField for email validation.

Since these fields store email addresses, using EmailField instead of CharField would provide built-in email validation.

-field=models.CharField(blank=True, help_text='Email of the user who created this record', max_length=255, null=True),
+field=models.EmailField(blank=True, help_text='Email of the user who created this record', max_length=255, null=True),

apps/fyle/migrations/0023_auto_20250108_0817.py (1)

13-22: Consider adding database indexes for user tracking fields.

The migration correctly adds the user tracking fields. However, since these fields might be used in filtering or searching operations, consider adding database indexes to improve query performance.

 migrations.AddField(
     model_name='expensegroupsettings',
     name='created_by',
-    field=models.CharField(blank=True, help_text='Email of the user who created this record', max_length=255, null=True),
+    field=models.CharField(blank=True, help_text='Email of the user who created this record', max_length=255, null=True, db_index=True),
 ),
 migrations.AddField(
     model_name='expensegroupsettings',
     name='updated_by',
-    field=models.CharField(blank=True, help_text='Email of the user who last updated this record', max_length=255, null=True),
+    field=models.CharField(blank=True, help_text='Email of the user who last updated this record', max_length=255, null=True, db_index=True),
 ),

apps/workspaces/migrations/0042_auto_20250108_0817.py (1)

13-22: Consider creating a shared migration utility for common field patterns.

The migration is correct but duplicates the pattern from ExpenseGroupSettings. Consider creating a shared migration utility for adding these user tracking fields, as they might be needed in other models.

Example utility in a shared migration file:

def add_user_tracking_fields(apps, schema_editor):
    models_to_update = [
        ('fyle', 'ExpenseGroupSettings'),
        ('workspaces', 'WorkspaceGeneralSettings'),
        # Add more models here
    ]
    
    for app_label, model_name in models_to_update:
        Model = apps.get_model(app_label, model_name)
        schema_editor.add_field(
            Model,
            models.CharField(
                'created_by',
                blank=True,
                null=True,
                max_length=255,
                db_index=True,
                help_text='Email of the user who created this record'
            )
        )
        # Add updated_by field similarly

tests/test_workspaces/fixtures.py (1)

24-25: Use more realistic test data for email addresses.

The current test email addresses (asdvhjasb@anjfas.com, asbdja@nkasnd.com) appear to be randomly generated. Consider using more realistic test email addresses that follow common patterns (e.g., test.user@example.com) for better readability and maintainability.

-        "created_by": "asdvhjasb@anjfas.com",
-        "updated_by": "asbdja@nkasnd.com"
+        "created_by": "test.creator@example.com",
+        "updated_by": "test.updater@example.com"

apps/workspaces/models.py (1)

10-11: Consider consistent application of user tracking across models.

Good use of AutoAddCreateUpdateInfoMixin for adding user tracking. However, other models in the file (like Workspace, XeroCredentials, etc.) might also benefit from this tracking capability.

Also applies to: 146-146

apps/workspaces/apis/export_settings/serializers.py (1)

104-106: Consider extracting user context retrieval to a mixin.

This user context retrieval logic is duplicated across serializers. Consider creating a mixin to handle this common functionality.

class UserContextMixin:
    def get_user_from_context(self):
        request = self.context.get('request')
        return request.user if request and hasattr(request, 'user') else None

tests/test_fyle/test_models.py (1)

51-53: Improve test data setup clarity.

Getting the user from workspace ID 1 while testing workspace ID 98 is confusing and could be fragile. Consider:

1. Using a fixture to create test user data
2. Making the test user creation explicit in the test setup

@pytest.fixture
def test_user(db):
    return User.objects.create(email='test@test.com')

def test_expense_group_settings(create_temp_workspace, test_user, db):
    workspace_id = 98
    payload = data["expense_group_settings_payload"]
    
    ExpenseGroupSettings.update_expense_group_settings(payload, workspace_id, test_user)

apps/fyle/models.py (3)

281-281: Update docstring to document user tracking capabilities.

While the mixin inheritance is correctly implemented, the class docstring should be updated to reflect the new user tracking capabilities.

 class ExpenseGroupSettings(AutoAddCreateUpdateInfoMixin, models.Model):
     """
     ExpenseGroupCustomizationSettings
+    
+    Tracks the user who creates and updates expense group settings through created_by
+    and updated_by fields provided by AutoAddCreateUpdateInfoMixin.
     """

---

342-342: Add type hints and docstring for the method.

The method should include:

1. Type hint for the user parameter
2. Docstring documenting the parameters and return value

     @staticmethod
-    def update_expense_group_settings(expense_group_settings: Dict, workspace_id: int, user):
+    def update_expense_group_settings(expense_group_settings: Dict, workspace_id: int, user: Any) -> Tuple[ExpenseGroupSettings, bool]:
+        """
+        Update expense group settings for a workspace.
+
+        Args:
+            expense_group_settings (Dict): Settings to update
+            workspace_id (int): ID of the workspace
+            user (Any): User performing the update
+
+        Returns:
+            Tuple[ExpenseGroupSettings, bool]: Updated settings and creation status
+        """

---

Line range hint 342-438: Consider refactoring the method for better maintainability.

The method is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused methods:

1. Split field processing logic into separate methods
2. Extract claim number and report ID handling logic
3. Optimize list operations

Example refactoring:

@staticmethod
def _process_claim_number_logic(settings: List[str]) -> None:
    """Handle claim number and report ID relationship in settings."""
    if "report_id" not in settings:
        if "claim_number" in settings:
            settings.remove("claim_number")
    else:
        settings.append("claim_number")

@staticmethod
def _process_export_date_fields(grouped_by: List[str], export_date_type: str) -> None:
    """Process export date fields in grouped by settings."""
    for field in ALLOWED_FORM_INPUT["export_date_type"]:
        if field in grouped_by:
            grouped_by.remove(field)
    
    if export_date_type != "current_date":
        grouped_by.append(export_date_type)

tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)

820-821: Consider adding default values and NOT NULL constraints.

The new created_by and updated_by columns are nullable without default values. Since these columns track audit information:

1. Consider making them NOT NULL to ensure data integrity
2. Add a default value (e.g., 'system') for existing records

-    created_by character varying(255),
-    updated_by character varying(255)
+    created_by character varying(255) NOT NULL DEFAULT 'system',
+    updated_by character varying(255) NOT NULL DEFAULT 'system'

Also applies to: 1078-1079, 1204-1205, 1546-1547

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro