fix: optimize badge assignment logic and improve test coverage for sync_user_badges command

Author: Dhirajsharma2060

backend/apps/github/management/commands/github_sync_user_badges.py

@@ -37,27 +37,22 @@ def sync_owasp_employee_badge(self):
             logger.info("Created 'OWASP Employee' badge")
             self.stdout.write(f"Created badge: {badge.name}")
 
-        # Add badge to OWASP employees
-        employees = User.objects.filter(is_owasp_employee=True)
-        count = 0
+        # Assign badge to employees who don't have it (avoiding N+1 queries)
+        employees_without_badge = User.objects.filter(is_owasp_employee=True).exclude(badges=badge)
+        count = employees_without_badge.count()
 
-        for user in employees:
-            # Check if the user already has the badge
-            if not user.badges.filter(id=badge.id).exists():
-                user.badges.add(badge)
-                count += 1
+        for user in employees_without_badge:
+            user.badges.add(badge)
 
-        if count:
-            logger.info("Added 'OWASP Employee' badge to %s users", count)
-            self.stdout.write(f"Added badge to {count} employees")
+        logger.info("Added 'OWASP Employee' badge to %s users", count)
+        self.stdout.write(f"Added badge to {count} employees")
 
         # Remove badge from non-OWASP employees
-        non_employees = User.objects.filter(is_owasp_employee=False).filter(badges=badge)
+        non_employees = User.objects.filter(is_owasp_employee=False, badges=badge)
         removed_count = non_employees.count()
 
         for user in non_employees:
             user.badges.remove(badge)
 
-        if removed_count:
-            logger.info("Removed 'OWASP Employee' badge from %s users", removed_count)
-            self.stdout.write(f"Removed badge from {removed_count} non-employees")
+        logger.info("Removed 'OWASP Employee' badge from %s users", removed_count)
+        self.stdout.write(f"Removed badge from {removed_count} non-employees")

backend/tests/apps/github/management/commands/github_sync_user_badges_test.py

@@ -6,11 +6,13 @@
 import pytest
 from django.core.management import call_command
 
+OWASP_EMPLOYEE_BADGE_NAME = "OWASP Employee"
+
 
 @pytest.fixture
 def mock_badge():
     badge = MagicMock()
-    badge.name = "OWASP Employee"
+    badge.name = OWASP_EMPLOYEE_BADGE_NAME
     badge.css_class = "fa-user-shield"
     badge.id = 1
     return badge
@@ -24,32 +26,28 @@ class TestSyncUserBadgesCommand:
     def test_sync_owasp_employee_badge(self, mock_user_filter, mock_badge_get_or_create):
         # Set up badge mock
         mock_badge = MagicMock()
-        mock_badge.name = "OWASP Employee"
+        mock_badge.name = OWASP_EMPLOYEE_BADGE_NAME
         mock_badge.id = 1
         mock_badge_get_or_create.return_value = (mock_badge, False)
 
-        # Set up employee mocks
+        # Set up employee mocks - with exclude() method properly mocked
         mock_employee = MagicMock()
-        mock_employee.badges.filter.return_value.exists.return_value = False
         mock_employees = MagicMock()
-        mock_employees.__iter__.return_value = [mock_employee]
-        mock_employees.count.return_value = 1
+        mock_employees_without_badge = MagicMock()
+        mock_employees_without_badge.__iter__.return_value = [mock_employee]
+        mock_employees_without_badge.count.return_value = 1
+        mock_employees.exclude.return_value = mock_employees_without_badge
 
         # Set up former employee mocks
         mock_former_employee = MagicMock()
         mock_former_employees = MagicMock()
         mock_former_employees.__iter__.return_value = [mock_former_employee]
         mock_former_employees.count.return_value = 1
 
-        # Set up the double filter behavior
-        mock_non_employees_first_filter = MagicMock()
-        mock_non_employees_first_filter.filter.return_value = mock_former_employees
-
         # Configure filter side effects
         mock_user_filter.side_effect = [
             mock_employees,  # is_owasp_employee=True
-            # is_owasp_employee=False, will need .filter(badges=badge)
-            mock_non_employees_first_filter,
+            mock_former_employees,  # is_owasp_employee=False, badges=badge
         ]
 
         # Call the command
@@ -71,7 +69,7 @@ def test_sync_owasp_employee_badge(self, mock_user_filter, mock_badge_get_or_cre
     def test_badge_creation(self, mock_user_filter, mock_badge_get_or_create):
         # Set up badge creation mock
         mock_badge = MagicMock()
-        mock_badge.name = "OWASP Employee"
+        mock_badge.name = OWASP_EMPLOYEE_BADGE_NAME
         mock_badge_get_or_create.return_value = (mock_badge, True)
 
         # Set up empty querysets
@@ -88,9 +86,67 @@ def test_badge_creation(self, mock_user_filter, mock_badge_get_or_create):
         out = StringIO()
         call_command("github_sync_user_badges", stdout=out)
 
-        # Verify badge creation
-        mock_badge_get_or_create.assert_called_once()
+        # Verify badge creation and defaults
+
+        mock_badge_get_or_create.assert_called_once_with(
+            name="OWASP Employee",
+            defaults={
+                "description": "Official OWASP Employee",
+                "css_class": "fa-user-shield",
+                "weight": 100,
+            },
+        )
 
         # Check command output
         output = out.getvalue()
         assert f"Created badge: {mock_badge.name}" in output
+
+    @patch("apps.owasp.models.badge.Badge.objects.get_or_create")
+    @patch("apps.github.models.user.User.objects.filter")
+    def test_command_idempotency(self, mock_user_filter, mock_badge_get_or_create):
+        """Test that running the command multiple times has the same effect as running it once."""
+        # Set up badge mock
+        mock_badge = MagicMock()
+        mock_badge.name = OWASP_EMPLOYEE_BADGE_NAME
+        mock_badge.id = 1
+        mock_badge_get_or_create.return_value = (mock_badge, False)
+
+        # Set up employee mock that already has the badge
+        mock_employee_with_badge = MagicMock()
+        # This employee already has the badge
+        mock_employee_with_badge.badges.filter.return_value.exists.return_value = True
+        mock_employees = MagicMock()
+        mock_employees.__iter__.return_value = [mock_employee_with_badge]
+        # Using exclude() would return 0 employees without the badge
+        mock_employees.exclude.return_value = MagicMock()
+        mock_employees.exclude.return_value.count.return_value = 0
+
+        # No former employees have the badge
+        mock_non_employees_filter = MagicMock()
+        mock_non_employees_filter.count.return_value = 0
+        mock_non_employees_filter.__iter__.return_value = []
+
+        # Configure filter side effects for two command runs
+        mock_user_filter.side_effect = [
+            mock_employees,  # is_owasp_employee=True (first run)
+            mock_non_employees_filter,  # is_owasp_employee=False (first run)
+            mock_employees,  # is_owasp_employee=True (second run)
+            mock_non_employees_filter,  # is_owasp_employee=False (second run)
+        ]
+
+        # First run
+        out1 = StringIO()
+        call_command("github_sync_user_badges", stdout=out1)
+
+        # Second run
+        out2 = StringIO()
+        call_command("github_sync_user_badges", stdout=out2)
+
+        # Verify no add/remove operations were performed
+        mock_employee_with_badge.badges.add.assert_not_called()
+
+        # Check both outputs contain zero-count messages
+        assert "Added badge to 0 employees" in out1.getvalue()
+        assert "Removed badge from 0 non-employees" in out1.getvalue()
+        assert "Added badge to 0 employees" in out2.getvalue()
+        assert "Removed badge from 0 non-employees" in out2.getvalue()