Skip to content

Conversation

@Piyushrathoree
Copy link
Collaborator

Proposed change

Resolves #1764

Add GraphQL endpoints to expose the new badge functionality to the frontend. The following queries should be included:

  • badges: Return a list of all badges (with fields: id, name, description, weight, css_class)
  • add badges to the user, exposing the list of badges assigned to each user (sorted by weight)
  • add proper test coverage for all new types and queries
  • all checked and run ruff check and pre-commit so that there is not issue

Checklist

  • I've read and followed the contributing guidelines.
  • I've run make check-test locally; all checks and tests passed.

- Add BadgeNode GraphQL type with fields: id, name, description, weight, css_class
- Add badges query to return all badges ordered by weight and name
- Add badges field to UserNode to expose user badges sorted by weight
- Add comprehensive test coverage for BadgeQueries and UserNode badges field
- Integrate BadgeQueries into NestQuery schema

Fixes OWASP#1764
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 28, 2025

Summary by CodeRabbit

  • New Features
    • Introduced a Badge type in the API exposing id, name, description, weight, and css_class.
    • Added a badges field to the User entity, returning the user’s active badges, sorted by weight (descending) and name.
  • Chores
    • Database migration to support the updated badges relation.

Walkthrough

Introduces a Badge GraphQL node and adds a badges field to UserNode resolving active badges via the through model. Renames the UserBadge reverse relation to user_badges with a migration and updates a management command accordingly. Adds tests for UserNode metadata and the badges resolver behavior.

Changes

Cohort / File(s) Summary
GraphQL nodes: badge and user
backend/apps/nest/api/internal/nodes/badge.py, backend/apps/github/api/internal/nodes/user.py
Adds BadgeNode (Relay node for Badge model) and a badges field on UserNode resolving active badges with select_related, filter by is_active, and ordering by weight then name.
Models and migration
backend/apps/nest/models/user_badge.py, backend/apps/nest/migrations/0005_alter_userbadge_user.py
Changes UserBadge.user related_name from badges to user_badges and adds corresponding migration.
Management command update
backend/apps/nest/management/commands/nest_update_badges.py
Updates queryset filters to use user_badges relation path instead of badges.
Tests
backend/tests/apps/github/api/internal/nodes/user_test.py
Updates expected UserNode fields to include badges and adds resolver behavior test asserting ORM calls and ordering.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Assessment against linked issues

Objective Addressed Explanation
Expose badges query returning all badges with fields (id, name, description, weight, css_class) [#1764] No root-level badges query added.
Add badges to user, exposing assigned badges sorted by weight [#1764]
Add proper test coverage for new types, queries, and mutations [#1764] Only UserNode badges resolver tested; no tests for root badges query or mutations.

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
Rename reverse relation to user_badges with migration (backend/apps/nest/models/user_badge.py; backend/apps/nest/migrations/0005_alter_userbadge_user.py) The issue targets GraphQL support; altering model related_name is not explicitly requested. Likely a refactor to support resolver; scope alignment is unclear.
Update management command filters to use user_badges (backend/apps/nest/management/commands/nest_update_badges.py) Management command behavior change is not mentioned in the GraphQL-specific objectives. Probably incidental to the related_name change.

Possibly related PRs

Suggested reviewers

  • arkid15r
  • kasya
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/apps/nest/api/internal/nodes/badge.py (1)

9-21: Subclass Relay Node and remove duplicate id field

  • Define BadgeNode by subclassing strawberry.relay.Node instead of using interfaces=(relay.Node,).
  • Omit "id" from fields—Relay’s GlobalID is auto-generated from the model PK, so exposing "id" adds a conflicting raw PK field.
🧹 Nitpick comments (8)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

20-41: Adding "badges" to expected fields is correct; consider a subset check to reduce brittleness.
Future field additions to UserNode will break equality. Prefer a subset assertion.

-        assert field_names == expected_field_names
+        assert expected_field_names.issubset(field_names)
backend/tests/apps/github/api/internal/nodes/user_badges_field_test.py (1)

18-26: Align mock chain with Django queryset semantics; have order_by return a queryset, not a list.
Reduces test fragility and better mirrors ORM behavior.

-        ordered_qs = MagicMock()
-        ordered_qs.__iter__.return_value = iter([])
+        ordered_qs = MagicMock()
         ub1 = MagicMock()
         ub1.badge = MagicMock()
         ordered_qs.__iter__.return_value = iter([ub1])
         user_badge_qs.filter.return_value = ordered_qs
-        ordered_qs.order_by.return_value = [ub1]
+        ordered_qs.order_by.return_value = ordered_qs

Optionally add an empty-result test to assert [] is returned.

backend/apps/github/api/internal/nodes/user.py (1)

59-66: Comment nit: it's select_related, not prefetch.
Minor doc accuracy tweak.

-        # related_name on UserBadge is "badges"; prefetch badge and filter active
+        # related_name on UserBadge is "badges"; join badge via select_related and filter active
backend/tests/apps/nest/api/internal/queries/badge_test.py (3)

13-18: Avoid relying on Strawberry internals; assert via schema introspection instead.
Accessing strawberry_definition is private API and brittle across Strawberry releases. Prefer GraphQL introspection on a real schema.

Apply:

-    def test_has_strawberry_definition(self):
-        """BadgeQueries should be a valid Strawberry type with 'badges' field."""
-        assert hasattr(BadgeQueries, "__strawberry_definition__")
-        field_names = [f.name for f in BadgeQueries.__strawberry_definition__.fields]
-        assert "badges" in field_names
+    def test_has_strawberry_definition(self):
+        """BadgeQueries should be a valid Strawberry type with 'badges' field."""
+        import strawberry
+        schema = strawberry.Schema(query=BadgeQueries)
+        res = schema.execute_sync('{ __type(name:"BadgeQueries"){ fields { name } } }')
+        assert res.errors is None
+        field_names = [f["name"] for f in res.data["__type"]["fields"]]
+        assert "badges" in field_names

19-25: Make type assertion resilient to Strawberry changes.
Direct identity checks on field.type/of_type can break across versions. Use schema introspection to verify a List of BadgeNode.

-    def test_badges_field_configuration(self):
-        """'badges' field should return a list of BadgeNode."""
-        field = next(
-            f for f in BadgeQueries.__strawberry_definition__.fields if f.name == "badges"
-        )
-        assert field.type.of_type is BadgeNode or field.type is BadgeNode
+    def test_badges_field_configuration(self):
+        """'badges' field should return a list of BadgeNode."""
+        import strawberry
+        schema = strawberry.Schema(query=BadgeQueries)
+        res = schema.execute_sync(
+            '{ __type(name:"BadgeQueries"){ fields { name type { kind ofType { name kind }}}}}'
+        )
+        assert res.errors is None
+        badges_field = next(f for f in res.data["__type"]["fields"] if f["name"] == "badges")
+        assert badges_field["type"]["kind"] == "LIST"
+        assert badges_field["type"]["ofType"]["name"] == "BadgeNode"

26-38: Patch the manager in the tested module instead of the model’s objects.all
Stub Badge.objects where it’s imported in apps.nest.api.internal.queries.badge to avoid Django’s manager‐descriptor edge cases. In your test (backend/tests/apps/nest/api/internal/queries/badge_test.py), change:

-    @patch("apps.nest.models.badge.Badge.objects.all")
-    def test_badges_resolution(self, mock_all):
+    @patch("apps.nest.api.internal.queries.badge.Badge.objects")
+    def test_badges_resolution(self, mock_manager):
         """Resolver should return badges ordered by weight and name."""
         mock_badge = MagicMock(spec=Badge)
-        mock_qs = MagicMock()
-        mock_qs.order_by.return_value = [mock_badge]
-        mock_all.return_value = mock_qs
+        mock_qs = MagicMock()
+        mock_manager.all.return_value = mock_qs
+        mock_qs.order_by.return_value = [mock_badge]
 
         result = BadgeQueries().badges()
 
         assert result == [mock_badge]
-        mock_all.assert_called_once()
+        mock_manager.all.assert_called_once_with()
         mock_qs.order_by.assert_called_once_with("weight", "name")

Optionally, add a test for an empty queryset to cover the no-results case.

backend/apps/nest/api/internal/queries/badge.py (2)

13-16: Consider avoiding unnecessary materialization or leaning on model default ordering (optional).
Returning a QuerySet avoids loading all rows up-front; relying on Meta.ordering removes duplication. If you choose either, adjust tests accordingly (they currently assert .all() and a list).

Two options:

-        return list(Badge.objects.all().order_by("weight", "name"))
+        # Option A: keep explicit ordering, avoid list()
+        return Badge.objects.order_by("weight", "name")
-        return list(Badge.objects.all().order_by("weight", "name"))
+        # Option B: rely on Meta.ordering on the model
+        return Badge.objects.all()

9-16: Plan for growth: expose a Relay connection if badge count can grow.
If badges might scale, consider a Connection field (edges/pageInfo) to support pagination and future filters.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 2f2e260 and 0abfabf.

📒 Files selected for processing (7)
  • backend/apps/github/api/internal/nodes/user.py (2 hunks)
  • backend/apps/nest/api/internal/nodes/badge.py (1 hunks)
  • backend/apps/nest/api/internal/queries/__init__.py (1 hunks)
  • backend/apps/nest/api/internal/queries/badge.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_badges_field_test.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (1 hunks)
  • backend/tests/apps/nest/api/internal/queries/badge_test.py (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.

Applied to files:

  • backend/apps/nest/api/internal/queries/badge.py
🧬 Code graph analysis (6)
backend/apps/github/api/internal/nodes/user.py (2)
backend/apps/nest/api/internal/nodes/badge.py (1)
  • BadgeNode (19-20)
backend/apps/nest/api/internal/queries/badge.py (1)
  • badges (14-16)
backend/tests/apps/nest/api/internal/queries/badge_test.py (3)
backend/apps/nest/api/internal/nodes/badge.py (1)
  • BadgeNode (19-20)
backend/apps/nest/api/internal/queries/badge.py (2)
  • BadgeQueries (10-16)
  • badges (14-16)
backend/apps/nest/models/badge.py (1)
  • Badge (10-41)
backend/apps/nest/api/internal/nodes/badge.py (1)
backend/apps/nest/models/badge.py (1)
  • Badge (10-41)
backend/tests/apps/github/api/internal/nodes/user_badges_field_test.py (1)
backend/apps/github/api/internal/nodes/user.py (2)
  • UserNode (28-68)
  • badges (57-68)
backend/apps/nest/api/internal/queries/__init__.py (2)
backend/apps/nest/api/internal/queries/badge.py (1)
  • BadgeQueries (10-16)
backend/apps/nest/api/internal/queries/api_key.py (1)
  • ApiKeyQueries (11-30)
backend/apps/nest/api/internal/queries/badge.py (3)
backend/apps/nest/api/internal/nodes/badge.py (1)
  • BadgeNode (19-20)
backend/apps/nest/models/badge.py (1)
  • Badge (10-41)
backend/apps/github/api/internal/nodes/user.py (1)
  • badges (57-68)
⏰ 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 unit tests
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run backend tests
🔇 Additional comments (2)
backend/apps/nest/api/internal/queries/__init__.py (1)

4-9: Wiring BadgeQueries into NestQuery — LGTM.
Integration is straightforward and consistent with existing query mixins.

backend/apps/nest/api/internal/queries/badge.py (1)

9-16: Resolver is correct and matches the contract.
Explicit ordering by ("weight", "name") aligns with the requirement and keeps behavior stable even if model Meta.ordering changes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

41-41: Improve assertion failure messaging for missing fields.

Subset check is correct; make failures actionable by reporting exactly which fields are missing.

Apply this diff:

-        assert expected_field_names.issubset(field_names)
+        missing = expected_field_names - field_names
+        assert not missing, f"Missing fields on UserNode: {sorted(missing)}"
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 0abfabf and 2d3a1c7.

📒 Files selected for processing (5)
  • backend/apps/github/api/internal/nodes/user.py (2 hunks)
  • backend/apps/nest/api/internal/queries/badge.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_badges_field_test.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (1 hunks)
  • backend/tests/apps/nest/api/internal/queries/badge_test.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • backend/tests/apps/nest/api/internal/queries/badge_test.py
  • backend/apps/github/api/internal/nodes/user.py
  • backend/apps/nest/api/internal/queries/badge.py
  • backend/tests/apps/github/api/internal/nodes/user_badges_field_test.py
⏰ 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). (5)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: Run backend tests
  • GitHub Check: CodeQL (python)
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

35-35: LGTM: badges field included in meta expectations.

This ensures the new GraphQL field is covered by the meta test.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
backend/apps/nest/api/internal/queries/badge.py (2)

3-14: Prefer strawberry_django.field to return a QuerySet natively (ORM-aware, optimization-ready).

This enables built-in queryset handling, future pagination, and auto-optimizations if you’re using strawberry-django elsewhere.

Apply within these lines:

+import strawberry_django
@@
-@strawberry.field
+@strawberry_django.field

13-16: Consider light caching if badges are rarely updated.

A short TTL cache (e.g., 5–15 minutes) can cut DB hits on hot paths for a static-ish list. Implement at resolver or service layer; invalidate on badge writes.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between eb053a5 and 4c5210f.

📒 Files selected for processing (4)
  • backend/apps/github/api/internal/nodes/user.py (2 hunks)
  • backend/apps/nest/api/internal/nodes/badge.py (1 hunks)
  • backend/apps/nest/api/internal/queries/badge.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • backend/apps/nest/api/internal/nodes/badge.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/api/internal/nodes/user.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/nest/api/internal/queries/badge.py (3)
backend/apps/nest/api/internal/nodes/badge.py (1)
  • BadgeNode (19-20)
backend/apps/nest/models/badge.py (1)
  • Badge (10-41)
backend/apps/github/api/internal/nodes/user.py (1)
  • badges (32-41)
⏰ 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). (1)
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/nest/api/internal/queries/badge.py (2)

9-16: LGTM: Minimal, clear badges list query.

Stable ordering by weight then name aligns with the model’s Meta and the PR objective to expose badges.


5-6: Confirm BadgeNode mapping: BadgeNode uses @strawberry_django.type(Badge,…), so returning a Django QuerySet[Badge] aligns correctly with list[BadgeNode].

@Piyushrathoree
Copy link
Collaborator Author

@arkid15r please review this too

1.deleted the badge queries - which is just querying all badges (not useful)
2.delete the test case for it
3.merge user_badge_field_test.py to user_test.py
4.updated user_badge.py to query for getting badges for each user
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

86-105: Fix Ruff D202: remove blank line after the docstring.

Pre-commit reported formatting issues; this also aligns with the static hint. Apply:

 def test_badges_resolution_orders_and_filters_active(self):
-        """Badges resolution should filter active and order by badge weight/name."""
-
+        """Badges resolution should filter active and order by badge weight/name."""
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d60aecd and 494c82d.

📒 Files selected for processing (3)
  • backend/apps/github/api/internal/nodes/user.py (2 hunks)
  • backend/apps/nest/models/user_badge.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/apps/github/api/internal/nodes/user.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
backend/apps/github/api/internal/nodes/user.py (2)
  • UserNode (28-68)
  • badges (32-42)
🪛 Ruff (0.12.2)
backend/tests/apps/github/api/internal/nodes/user_test.py

87-87: No blank lines allowed after function docstring (found 1)

Remove blank line(s) after function docstring

(D202)

🪛 GitHub Actions: Run CI/CD
backend/tests/apps/github/api/internal/nodes/user_test.py

[error] 1-1: Ruff-format: 2 files reformatted during pre-commit (this test file and related code file).

⏰ 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). (1)
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
backend/tests/apps/github/api/internal/nodes/user_test.py (3)

19-21: LGTM: field-name collection is clear and robust.


24-24: LGTM: asserting badges presence in schema.


43-44: Good change: subset check avoids brittle failures when new fields are added.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/apps/nest/management/commands/nest_update_badges.py (1)

43-56: Fix: inactive badge holders are skipped and never reactivated

Employees who previously had the Staff badge but were deactivated are excluded and won’t be reactivated. Exclude only users with an active badge, not any badge record.

-        employees_without_badge = User.objects.filter(is_owasp_staff=True).exclude(
-            user_badges__badge=badge
-        )
+        employees_without_badge = (
+            User.objects.filter(is_owasp_staff=True)
+            .exclude(user_badges__badge=badge, user_badges__is_active=True)
+            .distinct()
+        )
🧹 Nitpick comments (2)
backend/apps/nest/management/commands/nest_update_badges.py (1)

66-71: Optional: reduce unnecessary writes when deactivating

Limit the update to active assignments only and tighten the selection to users who currently have the active badge.

-        non_employees = User.objects.filter(
-            is_owasp_staff=False,
-            user_badges__badge=badge,
-        ).distinct()
+        non_employees = User.objects.filter(
+            is_owasp_staff=False,
+            user_badges__badge=badge,
+            user_badges__is_active=True,
+        ).distinct()
...
-            ).update(is_active=False)
+            ).filter(is_active=True).update(is_active=False)
backend/apps/github/api/internal/nodes/user.py (1)

37-39: Verify intended sort direction for “weight”

Comment in the management command calls 100 “High weight for importance.” If “higher weight = higher priority,” sort should be descending.

-            .order_by(
-                "badge__weight",
-                "badge__name",
-            )
+            .order_by(
+                "-badge__weight",
+                "badge__name",
+            )
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 494c82d and 1f0ca1d.

📒 Files selected for processing (3)
  • backend/apps/github/api/internal/nodes/user.py (2 hunks)
  • backend/apps/nest/management/commands/nest_update_badges.py (2 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/tests/apps/github/api/internal/nodes/user_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/badge.py (1)
  • BadgeNode (19-20)
⏰ 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 (2)
backend/apps/github/api/internal/nodes/user.py (2)

7-7: Import looks correct

BadgeNode import matches the new GraphQL type introduction.


31-42: Resolver implementation is correct and addresses prior relation issue

Using self.user_badges with select_related('badge'), filtering active, and projecting to ub.badge is correct and avoids N+1 for the per-user case.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

106-118: Exercise ordering semantics by returning a sorted list from the mocked queryset.

Right now the test asserts whatever the mock returns (30,10,20), which doesn’t reflect the resolver’s intended ordering. Have the order_by mock return items in weight-desc, name-asc to prove the resolver would yield that sequence. This addresses the prior feedback that order wasn’t truly tested.

-        unsorted_badges = [ub_heavy, ub_light, ub_medium]
+        # Unsorted input from earlier stages of the ORM chain
+        unsorted_badges = [ub_heavy, ub_light, ub_medium]
+        # What the DB would return given order_by("-badge__weight", "badge__name")
+        sorted_badges = [ub_heavy, ub_medium, ub_light]
@@
-        (
-            user.user_badges.select_related.return_value.filter.return_value.order_by.return_value
-        ) = unsorted_badges
+        (
+            user.user_badges.select_related.return_value
+            .filter.return_value
+            .order_by.return_value
+        ) = sorted_badges
@@
-        assert result == [ub.badge for ub in unsorted_badges]
+        assert result == [ub.badge for ub in sorted_badges]
@@
-        result_weights = [badge.weight for badge in result]
-        assert result_weights == [
-            30,
-            10,
-            20,
-        ]
+        result_weights = [badge.weight for badge in result]
+        assert result_weights == [30, 20, 10]

Optionally, add a tie-breaker case for equal weights:

+    def test_badges_resolver_orders_by_name_on_equal_weight(self):
+        user = Mock()
+        ub_a = Mock(); ub_a.badge = Mock(); ub_a.badge.weight = 20; ub_a.badge.name = "Alpha"
+        ub_b = Mock(); ub_b.badge = Mock(); ub_b.badge.weight = 20; ub_b.badge.name = "Beta"
+        ordered = [ub_a, ub_b]
+        user.user_badges.select_related.return_value.filter.return_value.order_by.return_value = ordered
+
+        result = UserNode.badges(user)
+
+        user.user_badges.select_related.assert_called_once_with("badge")
+        user.user_badges.select_related.return_value.filter.assert_called_once_with(is_active=True)
+        user.user_badges.select_related.return_value.filter.return_value.order_by.assert_called_once_with(
+            "-badge__weight", "badge__name"
+        )
+        assert [b.name for b in result] == ["Alpha", "Beta"]

Also applies to: 120-125

🧹 Nitpick comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

84-86: Clarify test intent: this unit test verifies ORM call chaining, not actual sorting.

Either reword the docstring or add an integration test to validate DB-level ordering.

-    def test_badges_resolver_behavior(self):
-        """Unit test verifies the resolver's interaction with the ORM and sorting logic."""
+    def test_badges_resolver_behavior(self):
+        """Unit test verifies the resolver's interaction with the ORM (call chain)."""
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f0ca1d and d32b708.

📒 Files selected for processing (3)
  • backend/apps/github/api/internal/nodes/user.py (2 hunks)
  • backend/apps/nest/migrations/0005_alter_userbadge_user.py (1 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/apps/github/api/internal/nodes/user.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
backend/apps/github/api/internal/nodes/user.py (2)
  • UserNode (28-67)
  • badges (32-42)
⏰ 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). (4)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: Run backend tests
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
backend/apps/nest/migrations/0005_alter_userbadge_user.py (2)

8-12: Verified migration dependencies exist. Both backend/apps/github/migrations/0035_alter_user_bio_alter_user_is_owasp_staff.py and backend/apps/nest/migrations/0004_userbadge.py are present with no renames.


14-23: No stale .badges references detected: the search only returned valid user_badges__… usages in nest_update_badges.py.

backend/tests/apps/github/api/internal/nodes/user_test.py (3)

22-22: Adding badges to expected fields is correct and aligns with the new GraphQL surface.


41-42: Subset check reduces brittleness of the schema test.

Nice move from strict equality to “contains at least” semantics.


112-116: Good verification of the ORM call chain.

Asserting select_related("badge"), filter(is_active=True), and order_by("-badge__weight","badge__name") directly captures the key performance and correctness guarantees.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 6, 2025

@arkid15r arkid15r enabled auto-merge September 6, 2025 18:50
@arkid15r arkid15r added this pull request to the merge queue Sep 6, 2025
Merged via the queue into OWASP:main with commit d0b1acc Sep 6, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement GraphQL support for badges

3 participants