Feature/daily badge sync job 1763 #2047
Conversation
…github.com:Dhirajsharma2060/Nest into feature/top-contributors-has-full-name-filter-1990 did a merger
…sitoryContributor tests
…3' into feature/daily-badge-sync-job-1763 refact:added some changes
Summary by CodeRabbit
WalkthroughAdds UserBadge model, admin, and migration in nest app; updates github User model fields and migration; introduces a management command and Makefile target to sync an “OWASP Staff” badge based on is_owasp_staff; includes Makefile wiring and comprehensive tests for the command. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
backend/apps/github/management/commands/github_sync_user_badges.py (3)
24-34: Confirm creation semantics: get_or_create vs update_or_createUsing get_or_create will never update description/css_class/weight on subsequent runs. If you want to enforce these defaults continuously (and heal drift), consider update_or_create; if you want to preserve admin-edited values, keep get_or_create.
Example change:
- badge, created = Badge.objects.get_or_create( + badge, created = Badge.objects.update_or_create( name="OWASP Employee", - defaults={ + defaults={ "description": "Official OWASP Employee", "css_class": "fa-user-shield", "weight": 100, # High weight for importance }, )
40-63: Optional: wrap the sync in a transaction for consistencyWrapping the add/remove operations in a transaction ensures the badge state doesn’t end up partially updated if an error occurs mid-sync.
+from django.db import transaction @@ - # Add badge to OWASP employees - employees = User.objects.filter(is_owasp_employee=True) - count = 0 - ... - # Remove badge from non-OWASP employees - non_employees = User.objects.filter(is_owasp_employee=False).filter(badges=badge) - ... + with transaction.atomic(): + # Add badge to OWASP employees + employees = User.objects.filter(is_owasp_employee=True) + count = 0 + ... + # Remove badge from non-OWASP employees + non_employees = User.objects.filter(is_owasp_employee=False).filter(badges=badge) + ...
50-53: PR objective mismatch: counts are only printed when > 0The PR description says the command logs counts of assigned and removed badges. Currently, output is conditional. Consider printing counts always (including 0) for observability and to match the description.
- 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") @@ - 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")Do you want me to update tests accordingly (to assert zero-count messages when no changes are needed)?
Also applies to: 61-63
backend/tests/apps/github/management/commands/github_sync_user_badges_test.py (4)
10-17: Remove unused fixtureThe mock_badge fixture isn’t used. Drop it to reduce noise.
-@pytest.fixture -def mock_badge(): - badge = MagicMock() - badge.name = "OWASP Employee" - badge.css_class = "fa-user-shield" - badge.id = 1 - return badge -
22-24: Patch where used for robustnessPatching the class’s manager works because it’s the same class object, but patching the imported symbol in the command module is more robust to import/path refactors.
- @patch("apps.owasp.models.badge.Badge.objects.get_or_create") - @patch("apps.github.models.user.User.objects.filter") + @patch("backend.apps.github.management.commands.github_sync_user_badges.Badge.objects.get_or_create") + @patch("backend.apps.github.management.commands.github_sync_user_badges.User.objects.filter")
31-37: Consider asserting the add-branch is driven by exclusion, not N+1 existsToday you simulate exists=False per employee. If you adopt the exclude-based optimization, there’s no exists() call. Keeping your test resilient by not depending on exists reduces coupling. You can assert count-based output and add() calls instead.
I can adjust the test to construct mock_employees_without_badge queryset directly (returned by the first filter) and drop the exists mocks if you proceed with the optimization.
1-1: Naming consistency: refer to github_sync_user_badgesDocstrings still say sync_user_badges. Update to the actual command name used by call_command.
-"""Tests for the sync_user_badges management command.""" +"""Tests for the github_sync_user_badges management command.""" @@ - """Test the sync_user_badges management command.""" + """Test the github_sync_user_badges management command."""Also applies to: 20-20
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/management/commands/github_sync_user_badges.py(1 hunks)backend/tests/apps/github/management/commands/github_sync_user_badges_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/management/commands/github_sync_user_badges.py (2)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)backend/apps/owasp/models/badge.py (1)
Badge(10-41)
backend/apps/github/management/commands/github_sync_user_badges.py
Outdated
Show resolved
Hide resolved
backend/apps/github/management/commands/github_sync_user_badges.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/github/management/commands/github_sync_user_badges_test.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/github/management/commands/github_sync_user_badges_test.py
Outdated
Show resolved
Hide resolved
…nc_user_badges command
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/apps/github/management/commands/github_sync_user_badges_test.py (1)
89-90: Remove empty comment lineLine 90 contains only a comment marker without any content. This appears to be a leftover from editing.
call_command("github_sync_user_badges", stdout=out) # Verify badge creation and defaults - mock_badge_get_or_create.assert_called_once_with(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/management/commands/github_sync_user_badges.py(1 hunks)backend/tests/apps/github/management/commands/github_sync_user_badges_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/management/commands/github_sync_user_badges.py
🔇 Additional comments (1)
backend/tests/apps/github/management/commands/github_sync_user_badges_test.py (1)
104-152: Well-implemented idempotency test!This test comprehensively validates the command's idempotent behavior by:
- Setting up a scenario where the badge is already correctly assigned
- Running the command multiple times
- Verifying no unnecessary operations are performed
- Confirming zero-count messages in the output
This addresses the PR's idempotency requirement effectively.
backend/tests/apps/github/management/commands/github_sync_user_badges_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/apps/nest/admin/user_badge.py (2)
1-1: Tweak docstring for accuracy/consistency.It’s the Nest app; consider tightening the docstring.
Apply:
-"""Admin configuration for the user badge model in the OWASP app.""" +"""Admin for the UserBadge model (Nest app)."""
33-43: Avoid N+1 queries in changelist withlist_select_related.The changelist will dereference
userandbadgefor every row. Selecting related FKs upfront prevents N+1.Apply:
class UserBadgeAdmin(admin.ModelAdmin): @@ list_display = ( "user", "badge", "note", "nest_created_at", "nest_updated_at", ) + # Performance: prefetch FK objects for list_display + list_select_related = ("user", "badge") readonly_fields = ( "nest_created_at", "nest_updated_at", )Optionally also add:
+ list_filter = ("badge",) + date_hierarchy = "nest_created_at" + ordering = ("-nest_created_at",)backend/apps/nest/models/user_badge.py (2)
23-29: Polish admin help text grammar.Minor wording improvement; this text is user-facing in admin.
Apply:
note = models.CharField( verbose_name="Note", max_length=255, blank=True, default="", - help_text="Optional note of the user badge.", + help_text="Optional note for the user badge.", )
31-43: Optional: renamerelated_nameonUserBadgefor clarityOur scan of the entire codebase (excluding migrations) yielded no occurrences of
badge.usersoruser.badges, indicating that changing the reverse‐relation names is safe and will reduce collision risk.Diff suggestion:
- badge = models.ForeignKey( - "nest.Badge", - related_name="users", + badge = models.ForeignKey( + "nest.Badge", + related_name="user_badges", on_delete=models.CASCADE, verbose_name="Badge", ) - user = models.ForeignKey( - "github.User", - related_name="badges", + user = models.ForeignKey( + "github.User", + related_name="user_badges", on_delete=models.CASCADE, verbose_name="User", )Next steps:
- Apply the above diff in
backend/apps/nest/models/user_badge.py.- Run
python manage.py makemigrationsto generate a migration that renames the reverse relations.- If any code (e.g. admin
search_fieldsor templates) does reference the old names in the future, update those references touser_badges.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
backend/Makefile(1 hunks)backend/apps/nest/admin/__init__.py(1 hunks)backend/apps/nest/admin/user_badge.py(1 hunks)backend/apps/nest/management/commands/nest_update_badges.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/user_badge.py(1 hunks)cspell/custom-dict.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/Makefile
- cspell/custom-dict.txt
- backend/apps/nest/models/init.py
- backend/apps/nest/management/commands/nest_update_badges.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/apps/nest/admin/user_badge.py (1)
backend/apps/nest/models/user_badge.py (1)
UserBadge(10-47)
backend/apps/nest/admin/__init__.py (1)
backend/apps/nest/admin/user_badge.py (1)
UserBadgeAdmin(8-44)
backend/apps/nest/models/user_badge.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (7)
backend/apps/nest/admin/__init__.py (2)
6-6: Re-export looks good; keeps admin public API consistent.Importing
UserBadgeAdminhere mirrors the pattern used for other admins and makes discovery consistent.
1-7: Management command naming is consistent across the PR. A repository-wide search confirmed that onlynest_update_badgesis referenced (in tests and the Makefile) and there are no remaininggithub_sync_user_badgesoccurrences.backend/apps/nest/admin/user_badge.py (3)
11-11: Verify prerequisites forautocomplete_fields.
autocomplete_fields=("user","badge")requires the corresponding ModelAdmins forgithub.Userandnest.Badgeto definesearch_fields. Otherwise the admin will raise at runtime.If missing, add minimal search fields, e.g., in their admins:
class UserAdmin(admin.ModelAdmin): + search_fields = ("login", "name", "email") class BadgeAdmin(admin.ModelAdmin): + search_fields = ("name", "slug", "description")
12-32: Fieldsets are clear and match the model.Good separation of user/badge, note, and timestamps; timestamps are not editable.
47-47: Registration is correct.Model and admin are properly registered.
backend/apps/nest/models/user_badge.py (2)
13-21: Constraint choice is solid for deduplication.The
(user, badge)UniqueConstraint enforces idempotency at the DB level, which aligns with the sync command’s needs.
1-1: Naming convention confirmed as consistentAcross all Nest models (
user,badge,api_key, anduser_badge), thedb_tablesetting uses thenest_prefix followed by a plural noun (e.g.nest_users,nest_badges,nest_api_keys,nest_user_badges), so no adjustment is needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/apps/nest/migrations/0007_userbadge_is_active.py (3)
13-17: Consider adding a DB-level default (db_default) for stronger invariantsIf inserts can bypass the Django ORM (fixtures, raw SQL, admin scripts), a Python-side default isn’t enough. In Django 5.x you can set a database default with
db_default=Trueto enforce True at the DB layer as well.Apply this diff:
- field=models.BooleanField(default=True, help_text='Indicates if the badge assignment is active.', verbose_name='Is active'), + field=models.BooleanField(default=True, db_default=True, help_text='Indicates if the badge assignment is active.', verbose_name='Is active'),
8-16: Align quotation style with Ruff rule Q000 (double quotes preferred)Style nit: switch single quotes to double quotes to satisfy the configured lint rule.
- ('nest', '0006_userbadge_uq_userbadge_user_badge'), + ("nest", "0006_userbadge_uq_userbadge_user_badge"), @@ - model_name='userbadge', + model_name="userbadge", - name='is_active', + name="is_active", - field=models.BooleanField(default=True, help_text='Indicates if the badge assignment is active.', verbose_name='Is active'), + field=models.BooleanField(default=True, help_text="Indicates if the badge assignment is active.", verbose_name="Is active"),
13-17: Optional: add a partial index to speed common lookupsIf queries frequently fetch active assignments per user (e.g.,
UserBadge.objects.filter(user=..., is_active=True)), consider a partial index conditioned onis_active=True. This keeps the index small and targeted.Example follow-up migration (separate file):
from django.db import migrations, models from django.db.models import Q class Migration(migrations.Migration): dependencies = [ ("nest", "0007_userbadge_is_active"), ] operations = [ migrations.AddIndex( model_name="userbadge", index=models.Index( fields=["user"], name="userbadge_user_active_idx", condition=Q(is_active=True), ), ), ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/apps/nest/management/commands/nest_update_badges.py(1 hunks)backend/apps/nest/migrations/0007_userbadge_is_active.py(1 hunks)backend/apps/nest/models/user_badge.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/nest/management/commands/nest_update_badges.py
- backend/apps/nest/models/user_badge.py
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/apps/nest/migrations/0007_userbadge_is_active.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (2)
backend/apps/nest/migrations/0007_userbadge_is_active.py (2)
13-17: Field addition looks correct and safe to applyAdding a non-nullable BooleanField with a default for existing rows is appropriate here. This will keep current assignments “active” by default and plays nicely with idempotent sync logic.
8-10: ✅ Migration ordering is correct; no duplicates detected
- Listed migrations in
backend/apps/nest/migrations/, and there is only one0007_userbadge_is_active.pyfile- Its
dependencies = [(‘nest’, ‘0006_userbadge_uq_userbadge_user_badge’)]correctly references the immediately preceding0006migrationNo further action needed—migration chain remains linear.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (10)
1-1: Fix test module docstring to reference the correct commandDocstring still references owasp_update_badges; the implemented/tested command is nest_update_badges.
-"""Tests for the owasp_update_badges management command.""" +"""Tests for the nest_update_badges management command."""
69-71: Align class docstring with the actual command nameUse nest_update_badges for consistency throughout.
-class TestSyncUserBadgesCommand: - """Test the sync_user_badges management command.""" +class TestSyncUserBadgesCommand: + """Tests for the nest_update_badges management command."""
8-8: Confirm badge naming: “staff” vs “employee” is inconsistent with PR summaryPR summary calls the badge “OWASP Employee” and the user flag is is_owasp_employee, but tests and constants use OWASP_STAFF_BADGE_NAME/is_owasp_staff. Ensure the command and model fields match. If the domain terms have changed, update constant names and test strings accordingly.
Would you like me to generate a follow-up patch that renames OWASP_STAFF_BADGE_NAME to OWASP_EMPLOYEE_BADGE_NAME across command and tests once the canonical terminology is confirmed?
106-111: Increase coverage by asserting the removal action on the filtered querysetYou verify that UserBadge.objects.filter(...) is called, but not that a mutating operation (e.g., update(...)/delete()) was invoked. Asserting one of these ensures real removal/inactivation happens.
mock_userbadge_filter.assert_any_call( user_id__in=mock_former_employees.values_list.return_value, badge=mock_badge ) + +# Option A: if command uses soft deactivation + assert ( + mock_userbadge_filter.return_value.update.called + or mock_userbadge_filter.return_value.delete.called + ), "Expected removal action (update/delete) on filtered UserBadge queryset"If the command specifically does update(is_active=False) or delete(), assert that explicitly.
113-120: Stabilize stdout assertions to a single canonical messageAllowing multiple phrasings (“staff” vs “employees”) can hide regressions. Assert the exact, intended message once the command’s copy is finalized.
- assert any(s in output for s in ("Added badge to 1 staff", "Added badge to 1 employees")) + assert "Added badge to 1 employees" in output - assert any( - s in output - for s in ("Removed badge from 1 non-staff", "Removed badge from 1 non-employees") - ) + assert "Removed badge from 1 non-employees" in outputIf the final wording differs, update these literals to match. Use the verification script above to confirm the current messages emitted by the command.
129-144: Reduce mock noise in badge_creation by using a small helper for empty querysetsThere’s duplication setting up empty iterable/count/values_list/distinct. A helper keeps this DRY and clearer.
+def make_empty_qs(): + qs = MagicMock() + qs.__iter__.return_value = iter([]) + qs.count.return_value = 0 + qs.values_list.return_value = [] + qs.distinct.return_value = qs + qs.exclude.return_value = qs + return qs @@ - mock_employees = MagicMock() - mock_employees.__iter__.return_value = iter([]) - mock_employees.count.return_value = 0 - mock_employees.exclude.return_value = mock_employees - mock_employees.values_list.return_value = [] - mock_employees.exclude.return_value.values_list.return_value = [] - mock_employees.exclude.return_value.distinct.return_value = mock_employees - mock_employees.exclude.return_value.distinct.return_value.values_list.return_value = [] + mock_employees = make_empty_qs() @@ - mock_former_employees = MagicMock() - mock_former_employees.__iter__.return_value = iter([]) - mock_former_employees.count.return_value = 0 - mock_former_employees.values_list.return_value = [] - mock_former_employees.distinct.return_value = mock_former_employees + mock_former_employees = make_empty_qs()
145-149: Make filter side effects resilient to Q/kwargs like in the first testThis test uses a fixed side_effect list, which can break if the command changes call ordering or uses Q objects. Reuse the factory for consistency.
- mock_user_filter.side_effect = [mock_employees, mock_former_employees] + mock_user_filter.side_effect = user_filter_side_effect_factory( + mock_employees, mock_former_employees + )
150-157: Avoid brittle coupling to default badge metadata; import shared defaults or assert by keysHard-coding description/css_class/weight will break if copy or style changes. Prefer importing a BADGE_DEFAULTS from the command (if exposed), or assert expected keys without values.
Option A (preferred, if you expose BADGE_DEFAULTS in the command):
-from apps.nest.management.commands.nest_update_badges import OWASP_STAFF_BADGE_NAME +from apps.nest.management.commands.nest_update_badges import ( + OWASP_STAFF_BADGE_NAME, + BADGE_DEFAULTS, +) @@ - mock_badge_get_or_create.assert_called_once_with( - name=OWASP_STAFF_BADGE_NAME, - defaults={ - "description": "Official OWASP Staff", - "css_class": "fa-user-shield", - "weight": 100, - }, - ) + mock_badge_get_or_create.assert_called_once_with( + name=OWASP_STAFF_BADGE_NAME, + defaults=BADGE_DEFAULTS, + )Option B (assert by shape):
args, kwargs = mock_badge_get_or_create.call_args assert kwargs["name"] == OWASP_STAFF_BADGE_NAME assert set(kwargs["defaults"].keys()) >= {"description", "css_class", "weight"}
161-219: Strengthen idempotency assertions by ensuring no mutations occur on re-runCurrently you only check stdout. Also assert that no add/remove mutations happen (e.g., no UserBadge get_or_create, no update/delete) on both runs.
- @patch("apps.nest.management.commands.nest_update_badges.Badge.objects.get_or_create") - @patch("apps.nest.management.commands.nest_update_badges.User.objects.filter") - def test_command_idempotency(self, mock_user_filter, mock_badge_get_or_create): + @patch("apps.nest.management.commands.nest_update_badges.UserBadge.objects.get_or_create") + @patch("apps.nest.management.commands.nest_update_badges.UserBadge.objects.filter") + @patch("apps.nest.management.commands.nest_update_badges.Badge.objects.get_or_create") + @patch("apps.nest.management.commands.nest_update_badges.User.objects.filter") + def test_command_idempotency( + self, + mock_user_filter, + mock_badge_get_or_create, + mock_userbadge_filter, + mock_userbadge_get_or_create, + ): @@ - # First run + # First run out1 = StringIO() call_command("nest_update_badges", stdout=out1) @@ - # Second run + # Second run out2 = StringIO() call_command("nest_update_badges", stdout=out2) @@ + # No creations or removals across both runs + mock_userbadge_get_or_create.assert_not_called() + assert not mock_userbadge_filter.return_value.update.called + assert not mock_userbadge_filter.return_value.delete.calledIf the command intentionally re-validates state by calling get_or_create with defaults that no-op, relax the assertion to check call_count is 0 on the second run only.
32-67: Helper utilities are clear; minor robustness nitextract_is_owasp_staff handles simple Q objects; if the command ever nests Q expressions (e.g., (Q(is_owasp_staff=True) & Q(active=True))), recursion would be needed. Not urgent now, just noting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py(1 hunks)
🔇 Additional comments (2)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (2)
83-100: Test scaffolding is solid; good separation of employee/non-employee querysetsThe side-effect factory and mock factories are clean and make intent obvious. No issues here.
72-82: Patch targets verified
Thenest_update_badgescommand module imports
Userfromapps.github.models.userBadgefromapps.nest.models.badgeUserBadgefromapps.nest.models.user_badgedirectly into its namespace (
from … import …) (). Therefore, the test’s patch decorators onapps.nest.management.commands.nest_update_badges.User.objects.filter apps.nest.management.commands.nest_update_badges.Badge.objects.get_or_create apps.nest.management.commands.nest_update_badges.UserBadge.objects.filter apps.nest.management.commands.nest_update_badges.UserBadge.objects.get_or_createcorrectly target the imported symbols, and no adjustments are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (5)
145-149: Replace brittle list side-effects with predicate-based side-effectsUsing a fixed list for side_effect assumes exactly two filter calls in a specific order. If the command adds another filter, the test becomes flaky. Reuse your user_filter_side_effect_factory (after the fix above) to route based on the kwarg/Q filter instead.
Option A — reuse existing factory:
- mock_user_filter.side_effect = [mock_employees, mock_former_employees] + mock_user_filter.side_effect = user_filter_side_effect_factory( + mock_employees, mock_former_employees + )And similarly below:
- mock_user_filter.side_effect = [ - mock_employees, - mock_non_employees_filter, - mock_employees, - mock_non_employees_filter, - ] + mock_user_filter.side_effect = user_filter_side_effect_factory( + mock_employees, mock_non_employees_filter + )Option B — inline a predicate-based side effect (if you prefer not to reuse the factory):
- mock_user_filter.side_effect = [mock_employees, mock_former_employees] + def _side_effect(*args, **kwargs): + flag = kwargs.get("is_owasp_employee", kwargs.get("is_owasp_staff")) + if flag is True: + return mock_employees + if flag is False: + return mock_former_employees + return MagicMock() + mock_user_filter.side_effect = _side_effectAlso applies to: 189-195
106-111: Also assert that deletions are executed, not just selectedYou verify that the filter for non-staff was issued, but not that delete() was called. Asserting the deletion strengthens the behavior guarantee.
mock_userbadge_filter.assert_any_call( user_id__in=mock_former_employees.values_list.return_value, badge=mock_badge ) + + # Ensure we actually perform the deletion for ex-staff. + mock_userbadge_filter.return_value.delete.assert_called_once()
129-138: Trim redundant/mock-chaining noise for readabilityThese chained return-value assignments duplicate earlier ones and make it harder to follow which mock is returned at each step. Simplify to the minimum needed behavior.
mock_employees = MagicMock() mock_employees.__iter__.return_value = iter([]) mock_employees.count.return_value = 0 - mock_employees.exclude.return_value = mock_employees - mock_employees.values_list.return_value = [] - mock_employees.exclude.return_value.values_list.return_value = [] - mock_employees.exclude.return_value.distinct.return_value = mock_employees - mock_employees.exclude.return_value.distinct.return_value.values_list.return_value = [] + mock_employees.exclude.return_value = mock_employees + mock_employees.values_list.return_value = [] + mock_employees.distinct.return_value = mock_employees
161-163: Strengthen idempotency test by asserting no new UserBadge is created on rerunCurrently, idempotency checks only validate output strings. Patch UserBadge.objects.get_or_create and assert it isn’t called when the employee already has the badge.
- @patch("apps.nest.management.commands.nest_update_badges.Badge.objects.get_or_create") - @patch("apps.nest.management.commands.nest_update_badges.User.objects.filter") - def test_command_idempotency(self, mock_user_filter, mock_badge_get_or_create): + @patch("apps.nest.management.commands.nest_update_badges.UserBadge.objects.get_or_create") + @patch("apps.nest.management.commands.nest_update_badges.Badge.objects.get_or_create") + @patch("apps.nest.management.commands.nest_update_badges.User.objects.filter") + def test_command_idempotency( + self, + mock_user_filter, + mock_badge_get_or_create, + mock_userbadge_get_or_create, + ): @@ # First run out1 = StringIO() call_command("nest_update_badges", stdout=out1) # Second run out2 = StringIO() call_command("nest_update_badges", stdout=out2) + + # Employee already has the badge → no creation attempts. + mock_userbadge_get_or_create.assert_not_called()Optional: also assert the badge lookup ran twice (once per run) if you want to lock that in:
assert mock_badge_get_or_create.call_count == 2Also applies to: 197-204
8-8: Confirm canonical naming: “OWASP Staff” vs “OWASP Employee”The PR summary says “OWASP Employee” while the test imports OWASP_STAFF_BADGE_NAME and validates a description of “Official OWASP Staff”. Please confirm the intended canonical name and description so tests and command don’t drift. If “Employee” is canonical, update the constant, test expectations, and the command defaults consistently; if “Staff” is canonical, update the PR summary.
Also applies to: 150-157
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py(1 hunks)
🔇 Additional comments (2)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (2)
72-82: Decorator-to-argument ordering is correct — niceThe order of mocks in the test function matches the stacking order of @patch decorators.
32-43: Confirm filter field usage innest_update_badgesmanagement commandI wasn’t able to locate any occurrences of
is_owasp_employeeoris_owasp_stafffilters in thebackend/apps/nest/management/commands/nest_update_badges.pyfile. Please verify directly whether the command uses:
User.objects.filter(is_owasp_employee=…)- or
User.objects.filter(is_owasp_staff=…)If it filters by
is_owasp_employee, you’ll need the proposed test updates to handle both field names. Otherwise, if it still relies onis_owasp_staff, the existing tests remain correct.
|
@arkid15r i have tried to implement the UserBadge::is_active flag this will do the soft deletion part also tried to run it locally as well , as for the custom dict userbadge we will need to make some changes to the migration file as well if we need to implement that |
|
@arkid15r keeping userbadge in the custom-dict will remove the make check warnings , will there any issue if we let it be in custom-dict ? |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
backend/apps/nest/admin/user_badge.py (3)
1-1: Docstring: use consistent app naming (“Nest” vs “OWASP”).Prefer “Nest app” to match module paths and repo naming.
15-21: Optimize admin changelist queries with select_related.Avoid N+1 on user and badge columns in list_display.
Apply:
class UserBadgeAdmin(admin.ModelAdmin): @@ list_display = ( "user", "badge", "note", "nest_created_at", "nest_updated_at", ) + list_select_related = ("user", "badge")
22-26: Improve operator UX: add date_hierarchy and default ordering.Faster time-based navigation and predictable sort.
Apply:
list_filter = ("is_active", "badge__name") readonly_fields = ( "nest_created_at", "nest_updated_at", ) + date_hierarchy = "nest_created_at" + ordering = ("-nest_created_at",)backend/apps/nest/models/user_badge.py (2)
21-25: Index is_active for faster active/inactive scans.Common filters on is_active benefit from a lightweight index.
Apply:
is_active = models.BooleanField( default=True, verbose_name="Is active", help_text="Indicates if the badge assignment is active.", + db_index=True, )
13-20: Prefer UniqueConstraint and add helpful indexes.UniqueConstraint is the modern, explicit form; indexes align with common lookups.
Apply:
class Meta: db_table = "nest_user_badges" - unique_together = ( - "badge", - "user", - ) + constraints = [ + models.UniqueConstraint( + fields=("badge", "user"), + name="uniq_nest_userbadge_badge_user", + ), + ] + indexes = [ + models.Index(fields=("badge", "user"), name="idx_userbadge_badge_user"), + models.Index(fields=("user", "badge"), name="idx_userbadge_user_badge"), + ] verbose_name_plural = "User badges"Note: migration required.
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (1)
101-111: Tighten assertion to reflect distinct().values_list() chain.Better matches typical ORM usage in the command.
Apply:
- mock_user_badge_filter.assert_any_call( - user_id__in=mock_former_employees.values_list.return_value, badge=mock_badge - ) + mock_user_badge_filter.assert_any_call( + user_id__in=mock_former_employees.distinct.return_value.values_list.return_value, + badge=mock_badge, + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/apps/nest/admin/user_badge.py(1 hunks)backend/apps/nest/migrations/0004_userbadge.py(1 hunks)backend/apps/nest/models/user_badge.py(1 hunks)backend/tests/apps/nest/management/commands/nest_update_badges_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/migrations/0004_userbadge.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/nest/admin/user_badge.py (1)
backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)
backend/apps/nest/models/user_badge.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (5)
backend/apps/nest/admin/user_badge.py (1)
27-32: I’m running the search for theUsermodel inbackend/apps/github/modelsand will report back once I have the class definition and its fields.backend/apps/nest/models/user_badge.py (1)
48-50: str depends on user.login; ensure that field exists or use a safer fallback.If github.User lacks login in some contexts, consider f"{self.user} - {self.badge.name}".
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (3)
121-157: Lock expected badge defaults to constants to avoid brittle string checks.If the command exposes constants for description/css_class/weight, import and assert them; else, keep as-is. Also confirm these values are the intended public contract.
69-120: Overall: tests are readable and exercise happy paths well.Good use of side effects and stdout checks; patch targets now align with module under test.
1-10: Drop naming consistency suggestion: only thenest_update_badgescommand exists and it consistently uses “OWASP Staff” (nogithub_sync_user_badgesor “OWASP Employee” in the code).Likely an incorrect or invalid review comment.
|
@arkid15r can you change this issue to done and can also could you please give points as well |
* test: add unit tests for SortBy component * test: update SortBy component tests for prop naming and accessibility * test: import React in SortBy component tests for consistency * made some chnages * test: update SortBy component tests for improved accessibility and clarity * test: ensure order toggle button functionality in SortBy component tests * feat: add has_full_name filter to top contributors query and model * fix: update regex for user name filtering in RepositoryContributor queryset * Update code * test: enhance RepositoryContributor tests with full name filter validations * refactor: use class constants for regex pattern and mock data in RepositoryContributor tests * test: fix regex filter call in has_full_name filter validation * Update code * Update tests * Update code * Update tests * Update test * feat: periodic job to sync OWASP Employee badges (OWASP#1762) * fix: optimize badge assignment logic and improve test coverage for sync_user_badges command * refactor: clean up comments in sync_user_badges test cases for clarity * refactor: remove unused mock_badge fixture from sync_user_badges test * feat: add management command to sync OWASP Staff badges and corresponding tests * refactor: optimize badge assignment logic for OWASP Staff badge sync * Update code * feat: add badges field to User model and update badge sync logic for OWASP staff feat:add badges field to User model and updated badge sync logic OWASP staff * feat: add badges field to User model and update related tests for badge synchronization * test: ensure no badge operations are performed when no employees are eligible * test: enhance user filter logic and improve badge assignment messages in tests suggested by coderabbit * test: refine user filter logic for badge assignment and enhance handling of keyword and positional arguments * test: refactor user filter logic in badge assignment tests for improved clarity and functionality * test: simplify user filter logic in badge assignment tests for improved readability and maintainability * test: simplify user filter logic by extracting helper functions for improved clarity and maintainability * test: update patch paths for badge and user objects in sync_user_badges tests for consistency * Update code * test: update patch paths for badge and user objects in TestSyncUserBadgesCommand for consistency * Fix OWASP#1912: Added test for SecondaryCard component (OWASP#2069) * Fix OWASP#1912: Added test for SecondaryCard component * Added newline at end of SecondaryCard.test.tsx * Fix make check --------- Co-authored-by: Kate Golovanova <[email protected]> * test: add unit tests for GeneralCompliantComponent (OWASP#2018) * test: add unit tests for GeneralCompliantComponent * fix(OWASP#1835): Refactor tests based on reviewer feedback * Fix make check and revert unnecessary changes * Fix issues --------- Co-authored-by: Kate Golovanova <[email protected]> * Added Light Theme Support for Contribution HeatMap (OWASP#2072) * Added Light Theme Support for Contribution HeatMap * Added Light Theme Support for Contribution HeatMap (Updated) * Added Light Theme Support for Contribution HeatMap (Updated) * Update color scheme for the heatmap * Update code --------- Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * feat: add comprehensive unit tests for DisplayIcon component (OWASP#2048) * feat: add comprehensive unit tests for DisplayIcon component - 30+ test cases covering rendering, props, edge cases, and accessibility - Mock all dependencies and ensure SonarQube compliance - Test both snake_case and camelCase property conventions * Fixed issues flagged by the bot * Fixed issues flagged by the bot * Fixed issues flagged by the bot and added unhover word in cspell/custom-dict.txt --------- Co-authored-by: Kate Golovanova <[email protected]> * enhance:change-hover-color-action-button (OWASP#1978) * enhance:change-hover-color-action-button * Update colors and tests * Add comprehensive hover state check --------- Co-authored-by: Kate <[email protected]> * Test: add unit tests for BarChart component OWASP#1801 (OWASP#1904) * Add unit tests for BarChart component - Added comprehensive test cases for BarChart component - Tests cover rendering, data handling, and prop validation - Follows project testing standards * Add unit tests for BarChart component * Add unit tests for BarChart component * Fix linting issues * Fixed unit tests for BarChart component * Fixed unit tests for BarChart component By CodeRabbit * Fixed unit tests for BarChart component By CodeRabbit * Fix tests and make check issues * Fixed unit tests for BarChart component * Fix issues and add more tests --------- Co-authored-by: Kate Golovanova <[email protected]> * Project Dasbored Drop Down Test case has been made (OWASP#2052) * Project Dasbored Drop Down Test case has been made * Changes made * Made the changes for the code check and test cases * Changes has been made so that it works with eslint * Fix make check issues --------- Co-authored-by: Kate Golovanova <[email protected]> * fix: update command references in badge management tests * Update code * fix: update badge management command to correctly filter users by badge relationship * style: format code for better readability in badge assignment logic * fix: update badge assignment logic to use UserBadge model and remove unnecessary user management calls * fix: include owasp Makefile in backend Makefile * fix: improve badge removal logic for non-OWASP employees * fix: add unique constraint for user and badge in UserBadge model * Update code * Revert volumes * feat: add is_active field to UserBadge model and update badge assignment logic * style: format migration file for userbadge is_active field * test: mock return value for UserBadge get_or_create in badge sync tests * fix: correct docstring for nest_update_badges management command tests * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: rasesh <[email protected]> Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: Karmjeet Chauhan <[email protected]> Co-authored-by: Abhay Bahuguna <[email protected]> Co-authored-by: Rizwaan Ansari <[email protected]> Co-authored-by: Vijesh Shetty <[email protected]> Co-authored-by: Mohd Waqar Ahemed <[email protected]> Co-authored-by: SOHAMPAL23 <[email protected]>
|
@arkid15r here as well the tag does not contain the gssoc label , because of which i am not able to see the points on the contributor id |
We use GSSoC related lables for issues only. |
10 participants
Resolves Implement daily badge sync job #1763
Adds a new management command (
github_sync_user_badges) to automate the assignment and removal of the "OWASP Employee" badge.The command is idempotent and can be scheduled to run daily after the user sync job.
Features
🎯 Badge Assignment & Removal
is_owasp_employee=True.is_owasp_employee=False(if previously assigned).🛠 Auto-Creation
css_class,weight) if it doesn’t exist.♻️ Idempotency
📊 Output Summary
✅ Test Coverage
Usage
Run manually:
Checklist
make check-testlocally; all checks and tests passed.