Feature/user badge model and implement them on frontend too #2101
Conversation
…ns (add/remove user badges); add User.badges tests
…dge management - Added UserBadge model to manage user badges in the backend. - Created BadgeCount, BadgeList, Badges, and BadgesTooltip components for frontend badge display. - Integrated badge support in UserCard and UsersPage components. - Added unit tests for new badge components and functionality. - Updated user and card types to include badges.
- Improved formatting and readability of the UserBadge model definition. - Updated field definitions and options for consistency. - Refactored test files to streamline assertions and enhance clarity.
Summary by CodeRabbit
WalkthroughAdds badge models and through-tables, GraphQL types/queries and user badge resolvers, frontend badge types/components and integrations (icons, list, tooltip, count), updates migrations, and introduces unit tests for backend and frontend badge functionality. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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: 2
🧹 Nitpick comments (35)
frontend/src/types/user.ts (1)
33-34: Consider ReadonlyArray to prevent accidental mutationsBadges are typically display data; using ReadonlyArray communicates intent and prevents accidental in-place mutations by consumers.
- badges?: Badge[] + badges?: ReadonlyArray<Badge>frontend/src/types/card.ts (1)
89-90: Prefer a named type import for readabilityInline import types work, but importing Badge at the top mirrors the pattern used elsewhere (e.g., types/user.ts) and improves readability.
- badges?: import('types/badge').Badge[] + badges?: Badge[]Add this import at the top of the file:
import type { Badge } from 'types/badge'frontend/src/app/members/[memberKey]/page.tsx (1)
211-214: Tighten null checks with optional chainingSlightly cleaner and avoids repeating the property chain. Behavior remains unchanged.
- {/* Full badges list with names below username */} - {user?.badges && user.badges.length > 0 && ( - <BadgeList badges={user.badges} className="mt-2" /> - )} + {/* Full badges list with names below username */} + {!!user?.badges?.length && <BadgeList badges={user.badges} className="mt-2" />}backend/tests/apps/nest/api/internal/queries/badge_test.py (1)
8-20: Make the type assertion resilient to Strawberry List/NonNull wrappersCurrent check only peeks one or two levels deep. Unwrap the nested
of_typechain to the base to avoid brittleness across Strawberry releases and annotation variations.Apply this diff:
- # badges returns list[BadgeNode] - assert ( - getattr(field.type, "of_type", None) is BadgeNode - or getattr(getattr(field.type, "of_type", None), "of_type", None) is BadgeNode - ) + # badges returns a list[BadgeNode] (unwrap List/NonNull/etc.) + t = field.type + while getattr(t, "of_type", None) is not None: + t = t.of_type + assert t is BadgeNodefrontend/__tests__/unit/components/BadgesTooltip.test.tsx (1)
1-27: Tighten tests: prefer Testing Library queries and add types for badges
- Use
screen.queryByTestIdinstead of DOM selectors for consistency with Testing Library best practices.- Type the test data with
Badgeto keep TS safety as the shape evolves.Apply this diff:
import React from 'react' import { render, screen } from 'wrappers/testUtil' import BadgesTooltip from 'components/BadgesTooltip' +import type { Badge } from 'types/badge' jest.mock('@fortawesome/react-fontawesome', () => ({ FontAwesomeIcon: ({ className }: { className?: string }) => ( <span data-testid="fa-icon" className={className} /> ), })) jest.mock('@heroui/tooltip', () => ({ Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}</>, })) describe('BadgesTooltip', () => { it('renders nothing when no badges', () => { - const { container } = render(<BadgesTooltip badges={[]} />) - expect(container.querySelector('[data-testid="fa-icon"]')).toBeNull() + render(<BadgesTooltip badges={[]} />) + expect(screen.queryByTestId('fa-icon')).not.toBeInTheDocument() }) it('renders trigger icon when badges are present', () => { - const badges = [{ name: 'A', cssClass: 'fa-solid fa-star' }] + const badges: Badge[] = [{ id: 'b1', name: 'A', description: '', weight: 1, cssClass: 'fa-solid fa-star' }] render(<BadgesTooltip badges={badges} />) expect(screen.getByTestId('fa-icon')).toBeInTheDocument() }) })backend/tests/apps/nest/api/internal/nodes/user_test.py (1)
12-12: Relax field set assertion to avoid brittleness as the schema evolvesUsing strict equality on the field set is fragile. Prefer a superset check (consistent with your BadgeNode test).
- assert set(fields.keys()) == {"_id", "is_owasp_staff", "username", "badges"} + assert set(fields.keys()) >= {"_id", "is_owasp_staff", "username", "badges"}Additionally, consider asserting the
badgesfield’s base type isBadgeNodeto mirror the badge query test. Place the following snippet after buildingfields:from apps.nest.api.internal.nodes.badge import BadgeNode # at top of file badges_field = fields["badges"] t = badges_field.type while getattr(t, "of_type", None) is not None: t = t.of_type assert t is BadgeNodebackend/tests/apps/nest/api/internal/nodes/badge_test.py (1)
10-12: Consider adding lightweight type checks for key fieldsYou already validate expected field names. Add a couple of quick type assertions to catch schema drift on primitives.
Example additions after building
fields:weight_field = fields["weight"] assert weight_field.type is int or getattr(weight_field.type, "of_type", None) is int css_class_field = fields["css_class"] assert css_class_field.type is str or getattr(css_class_field.type, "of_type", None) is strfrontend/__tests__/unit/components/Badges.test.tsx (4)
5-11: Harden Next.js Image mock to accept optional alt and forward rest propsPrevents type/prop drift from breaking the mock and preserves attributes (e.g., className, data-testid) for assertions.
jest.mock('next/image', () => ({ __esModule: true, - default: ({ src, alt }: { src: string; alt: string }) => ( + default: ({ src, alt = '', ...rest }: { src: string; alt?: string } & Record<string, unknown>) => ( // eslint-disable-next-line @next/next/no-img-element - <img src={src} alt={alt} /> + <img src={src} alt={alt} {...rest} /> ), }))
26-27: Comment is misleading given Tooltip is mockedSince Tooltip is mocked to a passthrough, HeroUI won’t inject an overlay container in this test. Tighten the comment to avoid confusion.
- // HeroUI injects an overlay container by default; check that no badge content exists + // Badges renders null for empty input; ensure no icon/img is present
30-41: Add a non-compact rendering test for completenessYou’re covering compact mode well. Consider also asserting that non-compact renders all badges (and likely names if that’s part of the UI), to catch regressions in default rendering.
it('renders up to max badges in compact mode and shows overflow count', () => { @@ expect(screen.getByText('+2')).toBeInTheDocument() }) + it('renders all badges when not in compact mode (no overflow)', () => { + const badges = Array.from({ length: 8 }).map((_, i) => ({ + name: `Badge ${i + 1}`, + cssClass: 'fa-solid fa-star', + })) + render(<Badges badges={badges} />) + const icons = screen.getAllByTestId('fa-icon') + expect(icons.length).toBe(8) + expect(screen.queryByText(/\+\d+/)).toBeNull() + })
43-49: Assert accessible alt text for local iconsVerifies graceful a11y: the image alt should reflect the badge name.
it('supports local icons via cssClass local: prefix', () => { const badges = [{ name: 'OWASP', cssClass: 'local:owasp' }] render(<Badges badges={badges} />) const img = screen.getByRole('img') as HTMLImageElement expect(img.src).toContain('/images/icons/owasp.svg') + expect(img.alt.toLowerCase()).toContain('owasp') })backend/tests/apps/nest/api/internal/queries/user_test.py (2)
7-13: Deduplicate mock_info across testsThis helper appears in other test modules. Consider moving it to a shared test utility (e.g., backend/tests/utils/graphql.py) to avoid divergence and ease maintenance.
If you want, I can draft the shared helper and update import sites.
16-26: Add assertion thatmeis protected byIsAuthenticatedpermissionBeyond existence/return value, verify that the field is guarded. Strawberry exposes permission classes on the field definition.
-from apps.nest.api.internal.queries.user import UserQueries +from apps.nest.api.internal.queries.user import UserQueries +from apps.nest.api.internal.permissions import IsAuthenticated @@ class TestUserQueries: @@ def test_me_field_exists(self): assert hasattr(UserQueries, "__strawberry_definition__") field_names = [f.name for f in UserQueries.__strawberry_definition__.fields] assert "me" in field_names + + def test_me_field_is_authenticated(self): + field = next(f for f in UserQueries.__strawberry_definition__.fields if f.name == "me") + # Strawberry stores permission classes on the field + assert any(pc is IsAuthenticated for pc in getattr(field, "permission_classes", []))backend/apps/nest/api/internal/queries/user.py (1)
13-16: Annotateinfotype for clarity and editor supportType-annotate the resolver parameter to advertise the Strawberry Info shape and improve DX.
-from apps.nest.api.internal.permissions import IsAuthenticated +from apps.nest.api.internal.permissions import IsAuthenticated +from strawberry.types import Info @@ - def me(self, info) -> AuthUserNode: + def me(self, info: Info) -> AuthUserNode: """Return the authenticated user.""" return info.context.request.userbackend/apps/nest/api/internal/nodes/user.py (1)
20-23: Stabilize ordering with a deterministic tie-breakerAdd a final tiebreaker (pk) to ensure deterministic order when weight/name collide.
- return self.badges.order_by("weight", "name") + return self.badges.order_by("weight", "name", "pk")frontend/src/components/BadgesTooltip.tsx (2)
22-24: Improve a11y for the tooltip triggerThe trigger is a non-focusable span. Add a button role, keyboard focus, and an accessible label so keyboard and screen-reader users can discover and open the tooltip.
Apply this diff:
- <span className={`inline-flex h-6 w-6 items-center justify-center ${className}`}> + <span + role="button" + tabIndex={0} + aria-label="Show user badges" + className={`inline-flex h-6 w-6 items-center justify-center ${className}`} + > <FontAwesomeIconWrapper icon="fa-solid fa-award" className="h-4 w-4 text-gray-500" /> </span>
11-13: Minor: redundant nullish check
badgesalready defaults to[]. The!badgespart is redundant.Apply this diff:
-const BadgesTooltip = ({ badges = [], className = '' }: BadgesTooltipProps) => { - if (!badges || badges.length === 0) return null +const BadgesTooltip = ({ badges = [], className = '' }: BadgesTooltipProps) => { + if (badges.length === 0) return nullfrontend/src/components/UserCard.tsx (1)
68-69: Render badge count only when there are badgesAvoid rendering the badge count placeholder when
badgesis empty/undefined to keep the UI minimal, per the PR objectives.Apply this diff:
- <BadgeCount badges={badges} /> + {badges && badges.length > 0 && <BadgeCount badges={badges} />}backend/tests/apps/nest/api/internal/mutations/badge_test.py (4)
8-14: Deduplicate mock_info helper across testsThis helper duplicates logic already present in backend/tests/apps/nest/api/internal/queries/user_test.py. Prefer centralizing into a shared test utility to avoid divergence.
I can draft a small tests/utils/graphql.py with mock_info() and update imports across tests if you’d like.
32-38: Strengthen the assertion on the returned valueAsserting isinstance(result, list) is weak and tightly coupled to the stubbed return. In production, order_by returns a QuerySet, not a list, so this assertion could mislead future refactors.
Apply this diff to assert the resolver returns exactly what order_by returns:
- assert isinstance(result, list) + assert result == user.badges.order_by.return_value
48-54: Strengthen the assertion on the returned value (remove_badge test)Same concern as above: assert the resolver returns the exact object provided by order_by.
Apply this diff:
- assert isinstance(result, list) + assert result == user.badges.order_by.return_value
22-31: Add a negative-path test for missing badges and permission behaviorCurrent tests cover the happy paths only. Consider:
- Verifying Badge.objects.get raises DoesNotExist is surfaced as a GraphQL error (or is converted to a domain error).
- Verifying permissions: calling the resolver directly bypasses Strawberry’s permission system. An integration test via the GraphQL schema would catch permission misconfigurations.
If helpful, I can add:
- A test that patches Badge.objects.get to raise Badge.DoesNotExist and asserts error handling.
- A schema-level test executing the mutation to ensure IsAuthenticated is enforced.
Also applies to: 39-47
frontend/src/components/BadgeCount.tsx (1)
26-31: Improve accessibility of the tooltip triggerMake the trigger discoverable to screen readers by adding an aria-label (and an optional title) to convey purpose. This keeps the UI unchanged but improves a11y.
Apply this diff:
- <div className={`flex cursor-pointer items-center gap-1 ${className || ''}`}> + <div + className={`flex cursor-pointer items-center gap-1 ${className || ''}`} + aria-label={`User has ${badges.length} badge${badges.length === 1 ? '' : 's'}`} + title={`${badges.length} badge${badges.length === 1 ? '' : 's'}`} + >frontend/__tests__/unit/components/UserCard.badges.test.tsx (1)
49-70: Nice isolation of BadgeCount; add an undefined case testLGTM overall. Consider one more test where badges is undefined (prop omitted) to ensure the component path degrades gracefully and still doesn’t render the count.
frontend/src/components/BadgeList.tsx (2)
49-55: Avoid index as key for stable reconciliationUsing the array index as a key can cause unnecessary re-renders or subtle UI bugs as the list changes. Prefer a stable unique key. If badges have unique names, use badge.name; otherwise extend Badge with an id.
Apply this diff:
- {badges.map((badge, index) => ( + {badges.map((badge, index) => ( <div - key={index} + key={badge.name ?? index} className="flex items-center gap-2 rounded-full bg-gray-100 px-3 py-1 dark:bg-gray-700" >
31-37: Verify Font Awesome library setupPassing a tuple requires the icon packs to be registered (e.g., via @fortawesome/fontawesome-svg-core library.add(...)). If you rely solely on css_class strings and haven’t registered icons, nothing will render. Alternative fallback is to render a plain if library registration is not desired.
I can help wire up the FA library globally or add a graceful fallback if the tuple lookup fails.
backend/apps/nest/api/internal/mutations/badge.py (3)
21-24: Handle missing badge IDs with a user-friendly GraphQL errorBadge.objects.get will raise DoesNotExist, returning an opaque error to clients. Consider surfacing a structured GraphQLError with a clear code.
Apply this diff in both mutations:
- badge = Badge.objects.get(pk=badge_id) - user.badges.add(badge) - return user.badges.order_by("weight", "name") + try: + badge = Badge.objects.get(pk=badge_id) + except Badge.DoesNotExist as exc: + from graphql import GraphQLError + raise GraphQLError("Badge not found", extensions={"code": "BADGE_NOT_FOUND"}) from exc + user.badges.add(badge) + return user.badges.order_by("weight", "name") @@ - badge = Badge.objects.get(pk=badge_id) - user.badges.remove(badge) - return user.badges.order_by("weight", "name") + try: + badge = Badge.objects.get(pk=badge_id) + except Badge.DoesNotExist as exc: + from graphql import GraphQLError + raise GraphQLError("Badge not found", extensions={"code": "BADGE_NOT_FOUND"}) from exc + user.badges.remove(badge) + return user.badges.order_by("weight", "name")Additionally, add this import at the top-level if you prefer not to inline it in the try/except:
from graphql import GraphQLErrorAlso applies to: 29-32
16-16: Consider using global Node IDs for Relay consistencySince BadgeNode implements strawberry.relay.Node, consider accepting badge_id: strawberry.ID and decoding it to the DB pk for consistency with the rest of the schema.
I can update the mutation signature and add an ID decoding helper if your schema uses global IDs elsewhere.
24-24: Optionally materialize the queryset to a listIf you want to avoid exposing a lazy queryset (and potential surprises if it’s evaluated later in a different context), convert to list before returning.
Apply this diff:
- return user.badges.order_by("weight", "name") + return list(user.badges.order_by("weight", "name"))Also applies to: 32-32
frontend/src/components/Badges.tsx (5)
17-25: Harden local icon path handling to prevent traversal and invalid filenamesSanitize the local icon name so a malformed cssClass (e.g., "local:../../foo") can’t escape /images/icons or inject unexpected characters.
Apply this diff:
- if (badge.iconPath || cssClass.startsWith('local:')) { - const localPath = badge.iconPath || `/images/icons/${cssClass.replace('local:', '')}.svg` + if (badge.iconPath || cssClass.startsWith('local:')) { + // Sanitize local icon names to avoid path traversal and ensure predictable filenames + const raw = cssClass.startsWith('local:') ? cssClass.slice('local:'.length) : '' + const safe = raw.replace(/[^a-z0-9._-]/gi, '') // allow only alnum, dot, underscore, dash + const file = safe && !safe.endsWith('.svg') ? `${safe}.svg` : safe + const localPath = badge.iconPath || `/images/icons/${file}` return ( <Tooltip content={title} placement="top" showArrow delay={100} closeDelay={100}> <Image src={localPath} alt={badge.name} width={16} height={16} className="h-4 w-4" /> </Tooltip> ) }
27-35: Add accessible label to non-image icon render pathFontAwesome icons lack alt text. Expose a readable label on the wrapper and hide the decorative icon from screen readers.
Apply this diff:
- return ( - <Tooltip content={title} placement="top" showArrow delay={100} closeDelay={100}> - <span className="inline-flex h-4 w-4 items-center justify-center"> - <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" /> - </span> - </Tooltip> - ) + return ( + <Tooltip content={title} placement="top" showArrow delay={100} closeDelay={100}> + <span className="inline-flex h-4 w-4 items-center justify-center" aria-label={title} role="img"> + <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" aria-hidden="true" /> + </span> + </Tooltip> + )
36-36: Graceful fallback when neither iconPath nor cssClass is presentCurrent behavior renders nothing. Consider a neutral default (e.g., medal) to satisfy “graceful degradation.”
Apply this diff:
- return null + return ( + <Tooltip content={title} placement="top" showArrow delay={100} closeDelay={100}> + <span className="inline-flex h-4 w-4 items-center justify-center" aria-label={title} role="img"> + <FontAwesomeIconWrapper icon="fa-solid fa-medal" className="h-4 w-4" aria-hidden="true" /> + </span> + </Tooltip> + )
42-44: Guard against negative max valuesA negative max produces surprising results with slice(). Clamp to positive values.
Apply this diff:
- const visibleBadges = typeof max === 'number' ? badges.slice(0, max) : badges - const hiddenCount = badges.length - visibleBadges.length + const limit = typeof max === 'number' && max > 0 ? max : undefined + const visibleBadges = typeof limit === 'number' ? badges.slice(0, limit) : badges + const hiddenCount = badges.length - visibleBadges.length
47-49: Use stable keys (prefer id) instead of index-derived keysIndex-based keys can cause unnecessary re-renders. Prefer the badge id when available.
Apply this diff:
- {visibleBadges.map((badge, idx) => ( - <BadgeIcon key={`${badge.name}-${idx}`} badge={badge} /> - ))} + {visibleBadges.map((badge, idx) => ( + <BadgeIcon key={(badge as any).id ?? `${badge.name}-${idx}`} badge={badge} /> + ))}If BadgeType includes id, remove the cast and use key={badge.id}.
backend/apps/nest/migrations/0004_userbadge_user_badges.py (1)
60-66: Prefer a named UniqueConstraint and add useful indexesunique_together is deprecated in favor of constraints. Also, add indexes to speed up common queries (by user, by badge, and ordering by awarded_at).
Apply this diff:
options={ "verbose_name_plural": "User Badges", "db_table": "nest_user_badges", - "ordering": ["-awarded_at"], - "unique_together": {("user", "badge")}, + "ordering": ["-awarded_at"], + "constraints": [ + models.UniqueConstraint( + fields=("user", "badge"), + name="nest_user_badges_user_badge_uniq", + ), + ], + "indexes": [ + models.Index(fields=("user",)), + models.Index(fields=("badge",)), + models.Index(fields=("-awarded_at",)), + ], },
📜 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 (27)
backend/apps/nest/api/internal/mutations/__init__.py(1 hunks)backend/apps/nest/api/internal/mutations/badge.py(1 hunks)backend/apps/nest/api/internal/nodes/badge.py(1 hunks)backend/apps/nest/api/internal/nodes/user.py(2 hunks)backend/apps/nest/api/internal/queries/__init__.py(1 hunks)backend/apps/nest/api/internal/queries/badge.py(1 hunks)backend/apps/nest/api/internal/queries/user.py(1 hunks)backend/apps/nest/migrations/0004_userbadge_user_badges.py(1 hunks)backend/apps/nest/models/user.py(1 hunks)backend/tests/apps/nest/api/internal/mutations/badge_test.py(1 hunks)backend/tests/apps/nest/api/internal/nodes/badge_test.py(1 hunks)backend/tests/apps/nest/api/internal/nodes/user_test.py(1 hunks)backend/tests/apps/nest/api/internal/queries/badge_test.py(1 hunks)backend/tests/apps/nest/api/internal/queries/user_test.py(1 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/components/BadgesTooltip.test.tsx(1 hunks)frontend/__tests__/unit/components/UserCard.badges.test.tsx(1 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/app/members/page.tsx(1 hunks)frontend/src/components/BadgeCount.tsx(1 hunks)frontend/src/components/BadgeList.tsx(1 hunks)frontend/src/components/Badges.tsx(1 hunks)frontend/src/components/BadgesTooltip.tsx(1 hunks)frontend/src/components/UserCard.tsx(4 hunks)frontend/src/types/badge.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/user.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/nodes/badge.pybackend/apps/nest/api/internal/queries/user.pybackend/apps/nest/api/internal/queries/badge.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/apps/nest/api/internal/nodes/badge.py
🧬 Code Graph Analysis (18)
backend/apps/nest/api/internal/queries/__init__.py (3)
backend/apps/nest/api/internal/queries/badge.py (1)
BadgeQueries(11-17)backend/apps/nest/api/internal/queries/user.py (1)
UserQueries(10-16)backend/apps/nest/api/internal/queries/api_key.py (1)
ApiKeyQueries(11-30)
frontend/__tests__/unit/components/UserCard.badges.test.tsx (1)
frontend/src/types/card.ts (1)
UserCardProps(77-90)
backend/tests/apps/nest/api/internal/nodes/badge_test.py (1)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)
frontend/__tests__/unit/components/BadgesTooltip.test.tsx (1)
frontend/src/wrappers/testUtil.tsx (1)
render(14-14)
frontend/__tests__/unit/components/Badges.test.tsx (1)
frontend/src/wrappers/testUtil.tsx (1)
render(14-14)
backend/tests/apps/nest/api/internal/queries/badge_test.py (2)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)backend/apps/nest/api/internal/queries/badge.py (1)
BadgeQueries(11-17)
backend/apps/nest/api/internal/queries/user.py (2)
backend/apps/nest/api/internal/nodes/user.py (1)
AuthUserNode(12-23)backend/apps/nest/api/internal/permissions.py (1)
IsAuthenticated(10-21)
backend/tests/apps/nest/api/internal/mutations/badge_test.py (3)
backend/apps/nest/api/internal/mutations/badge.py (3)
BadgeMutations(12-32)add_badge_to_user(16-24)remove_badge_from_user(27-32)backend/tests/apps/nest/api/internal/queries/user_test.py (1)
mock_info(7-13)backend/apps/nest/api/internal/nodes/user.py (1)
badges(21-23)
backend/apps/nest/api/internal/queries/badge.py (2)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)backend/apps/nest/api/internal/nodes/user.py (1)
badges(21-23)
backend/apps/nest/models/user.py (2)
backend/apps/nest/api/internal/nodes/user.py (1)
badges(21-23)backend/apps/nest/api/internal/queries/badge.py (1)
badges(15-17)
backend/tests/apps/nest/api/internal/queries/user_test.py (3)
backend/apps/nest/api/internal/nodes/user.py (1)
AuthUserNode(12-23)backend/apps/nest/api/internal/queries/user.py (1)
UserQueries(10-16)backend/tests/apps/nest/api/internal/mutations/badge_test.py (1)
mock_info(8-14)
frontend/src/types/user.ts (1)
frontend/src/types/badge.ts (1)
Badge(1-6)
backend/apps/nest/api/internal/mutations/badge.py (4)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)backend/apps/nest/api/internal/permissions.py (1)
IsAuthenticated(10-21)backend/apps/nest/api/internal/nodes/user.py (1)
badges(21-23)backend/apps/nest/api/internal/queries/badge.py (1)
badges(15-17)
backend/apps/nest/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(15-17)
frontend/src/components/BadgeList.tsx (1)
frontend/src/types/badge.ts (1)
Badge(1-6)
frontend/src/app/members/page.tsx (1)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)
frontend/src/components/BadgeCount.tsx (1)
frontend/src/types/badge.ts (1)
Badge(1-6)
backend/apps/nest/api/internal/mutations/__init__.py (1)
backend/apps/nest/api/internal/mutations/badge.py (1)
BadgeMutations(12-32)
🔇 Additional comments (18)
frontend/src/types/badge.ts (1)
1-6: Extend Badge type withidandweight; verify GraphQL aliasing• In
frontend/src/types/badge.ts, add optionalidandweightto enable stable React keys and client-side sorting:export type Badge = { + id?: string name: string description?: string cssClass?: string iconPath?: string + weight?: number }• Ensure all GraphQL queries alias the snake_case field to camelCase, e.g.:
{ badges { cssClass: css_class … } }Our automated search did not locate any raw
css_classusages or existing aliases in.ts,.tsx, or.graphqlfiles—please manually review your frontend GraphQL selections to confirm they’re correctly aliased.frontend/src/types/user.ts (1)
1-1: Type-only import is correctUsing a type-only import avoids bundling and keeps the module lean.
frontend/src/app/members/page.tsx (2)
49-49: Passing badges through to UserCard aligns with the new propsThis hooks up the list view to the badges UI without leaking list-specific logic into the page. LGTM.
49-49: Verify upstream data shape and compact badges behaviorPlease ensure the following before landing this change:
Confirm that the “users” data source (e.g. the Algolia index or whatever backs
useSearchPage({ indexName: 'users' })) actually includes abadgesfield for each user—otherwiseuser.badgeswill always beundefined.
• I ran a quick grep across TS/JS files and didn’t find where “users” are indexed or wherebadgesis populated, so please double-check the upstream mapping or search-client configuration.Ensure
UserCardimplements the intended compact-view spec (show the top 6 badges with hover-to-reveal the rest, per issue #1765), rather than only rendering a count.frontend/src/app/members/[memberKey]/page.tsx (2)
24-24: Importing BadgeList here is appropriateLocalizes the badges rendering to the detail page and keeps the component tree clear. LGTM.
211-214: Confirm sort order (by weight) from backend is preservedIf the backend already returns badges sorted by weight, ensure no client-side reshuffling occurs inside BadgeList. If not, consider sorting there using the optional weight suggested for Badge.
backend/apps/nest/api/internal/queries/__init__.py (1)
4-9: LGTM: Query aggregation via mixins is clear and conventionalIncluding
BadgeQueriesandUserQueriesinNestQuerykeeps the schema modular. Just ensure there are no overlapping field names across mixins to avoid resolver clashes; current imports look disjoint.frontend/__tests__/unit/components/Badges.test.tsx (1)
23-50: Solid unit coverage and resilient mockingGood isolation of external UI libs and image/icon dependencies. Scenarios cover null rendering, compact overflow, and local icon mapping.
backend/tests/apps/nest/api/internal/queries/user_test.py (1)
16-26: LGTM: tests validate schema shape and resolver behaviorThe checks for field presence and resolver return path are concise and effective.
backend/apps/nest/api/internal/queries/user.py (1)
9-16: LGTM: simplemeresolver with auth guardClean resolver, leveraging Strawberry-Django’s model-to-node conversion.
backend/apps/nest/api/internal/nodes/user.py (1)
11-23: LGTM: badges field on user nodeExposes M2M cleanly and orders by weight/name as per requirements.
backend/apps/nest/api/internal/nodes/badge.py (1)
9-20: Fix Relayidcollision; expose DB id as_idexplicitlyThe current test (
TestBadgeNode.test_badge_node_definition) expects both the Relay globalidfield and the database primary key exposed as_id, along with the other model fields. To satisfy this without conflict:• Remove the automatic
fields=[…]list so that the model’sidisn’t mapped onto Relay’sid.
• Explicitly declare:
_id: int = strawberry_django.field(field_name="id")- Relay’s
id(inherited fromstrawberry.relay.Node)- The remaining fields (
name,description,weight,css_class)Suggested diff for
backend/apps/nest/api/internal/nodes/badge.py:- @strawberry_django.type( - Badge, - fields=[ - "id", - "name", - "description", - "weight", - "css_class", - ], - ) - class BadgeNode(strawberry.relay.Node): - """GraphQL node for Badge model.""" + @strawberry_django.type(Badge) + class BadgeNode(strawberry.relay.Node): + """GraphQL node for Badge model.""" + # Expose DB primary key separately to avoid clashing with Relay’s global `id` + _id: int = strawberry_django.field(field_name="id") + # Relay’s `id` is inherited from strawberry.relay.Node + name: str + description: str | None + weight: int + css_class: str | NoneThis ensures
BadgeNode.__strawberry_definition__.fieldsincludes exactly{ "id", "_id", "name", "description", "weight", "css_class" }, satisfying the existing tests.backend/apps/nest/api/internal/mutations/__init__.py (1)
11-12: LGTM: Badge mutations correctly surfaced via NestMutationsIncluding
BadgeMutationsin the MRO exposes the new badge assignment APIs at the root without impact to existing mutations. Order between mixins looks fine.backend/apps/nest/api/internal/queries/badge.py (1)
14-17: LGTM: Simple, efficient resolver with stable orderingReturning the queryset is supported by strawberry-django and leverages its automatic conversions (consistent with previous learnings). Ordering by
weight, namematches the API/UX expectations.backend/apps/nest/api/internal/mutations/badge.py (1)
15-16: Confirm authorization model: self-assigning badgesCurrently any authenticated user can add/remove any badge to themselves. If badges are meant to be curated/admin-assigned, this is overly permissive.
Would you like to scope these mutations to staff/admins, or to a narrower set of badge IDs? If yes, I can draft a simple permission class (e.g., IsStaff) or a policy check.
Also applies to: 26-27
frontend/src/components/Badges.tsx (1)
39-55: Clean, compact, and matches requirementsOverall component structure, props, and conditional rendering align well with the PR goals (compact view with +N, tooltip titles, local/FA paths).
backend/apps/nest/migrations/0004_userbadge_user_badges.py (2)
14-59: Through model structure looks solidFields, relationships, and on_delete behaviors are appropriate. Using awarded_by and notes is a good call for auditability.
67-78: Explicit ordering confirmed in GraphQL resolversThe user.badges relation is explicitly ordered by weight then name in all relevant places:
- AuthUserNode.badges (apps/nest/api/internal/nodes/user.py:23):
return self.badges.order_by("weight", "name")- BadgeQueries.badges (apps/nest/api/internal/queries/badge.py:17):
return Badge.objects.all().order_by("weight", "name")- add_badge_to_user / remove_badge_from_user mutations (apps/nest/api/internal/mutations/badge.py:24 & 32):
return user.badges.order_by("weight", "name")No additional ordering changes are needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
I get this when I run it with make run
nest-backend | Traceback (most recent call last):
nest-backend | File "/home/owasp/manage.py", line 14, in <module>
nest-backend | execute_from_command_line(sys.argv)
nest-backend | ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
nest-backend | utility.execute()
nest-backend | ~~~~~~~~~~~~~~~^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/management/__init__.py", line 436, in execute
nest-backend | self.fetch_command(subcommand).run_from_argv(self.argv)
nest-backend | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/management/base.py", line 416, in run_from_argv
nest-backend | self.execute(*args, **cmd_options)
nest-backend | ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/management/base.py", line 457, in execute
nest-backend | self.check(**check_kwargs)
nest-backend | ~~~~~~~~~~^^^^^^^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/management/base.py", line 492, in check
nest-backend | all_issues = checks.run_checks(
nest-backend | app_configs=app_configs,
nest-backend | ...<2 lines>...
nest-backend | databases=databases,
nest-backend | )
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/checks/registry.py", line 89, in run_checks
nest-backend | new_errors = check(app_configs=app_configs, databases=databases)
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/core/checks/model_checks.py", line 36, in check_all_models
nest-backend | errors.extend(model.check(**kwargs))
nest-backend | ~~~~~~~~~~~^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/db/models/base.py", line 1719, in check
nest-backend | *cls._check_fields(**kwargs),
nest-backend | ~~~~~~~~~~~~~~~~~^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/db/models/base.py", line 1916, in _check_fields
nest-backend | errors.extend(field.check(from_model=cls, **kwargs))
nest-backend | ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/db/models/fields/related.py", line 1448, in check
nest-backend | *self._check_relationship_model(**kwargs),
nest-backend | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
nest-backend | File "/home/owasp/.venv/lib/python3.13/site-packages/django/db/models/fields/related.py", line 1702, in _check_relationship_model
nest-backend | for f in through._meta.fields:
nest-backend | ^^^^^^^^^^^^^
nest-backend | AttributeError: 'str' object has no attribute '_meta'
I didn't review the FE part yet, but based on the screenshot I suggest:
- make the badge color consistent with other icons on the member list page
- use badge icons instead of tag-like looking components
|
sorry for being late I'll resolve this ASAP |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
backend/apps/github/api/internal/nodes/user.py (2)
30-33: Badges field looks correct; consider using strawberry_django.field to avoid N+1 queriesReturning a queryset is fine, but when listing many users this can cause N+1 queries without prefetching. Using strawberry_django.field also lets the optimizer kick in.
Apply this refactor:
- @strawberry.field - def badges(self) -> list[BadgeNode]: - """List badges assigned to the user sorted by weight and name.""" - return self.badges.order_by("weight", "name") + @strawberry_django.field + def badges(self) -> list[BadgeNode]: + """List badges assigned to the user sorted by weight and name.""" + # Default ordering on Badge is ["weight", "name"]; keep explicit order for clarity. + return self.badges.order_by("weight", "name").all()
34-34: Fix trailing whitespace to satisfy Ruff (W293) and unblock CIThe blank line contains whitespace and is flagged by Ruff. Remove it or run ruff format.
Apply formatting:
- +And run locally:
- pre-commit run -a
- ruff format
backend/apps/github/models/user.py (1)
48-56: M2M to Badge via through-model is well wired; add help_text and fix trailing spaceThe relation correctly targets nest.Badge via nest.GithubUserBadge with proper through_fields. Minor polish: add help_text for admin/API docs and remove trailing space on Line 54 (Ruff W291).
Apply:
badges = models.ManyToManyField( "nest.Badge", verbose_name="Badges", + help_text="Badges associated with this GitHub user (via nest.GithubUserBadge).", related_name="github_users", blank=True, - through="nest.GithubUserBadge", + through="nest.GithubUserBadge", through_fields=("github_user", "badge"), )backend/apps/nest/models/badge.py (2)
39-54: Modernize constraints for UserBadge (unique_together → UniqueConstraint)unique_together is discouraged in modern Django in favor of explicit UniqueConstraint. Functionally identical, clearer migration history.
Apply:
class UserBadge(TimestampedModel): """Through model for User and Badge.""" class Meta: db_table = "nest_user_badges" - verbose_name_plural = "User badges" - unique_together = ("user", "badge") + verbose_name_plural = "User badges" + constraints = [ + models.UniqueConstraint( + fields=("user", "badge"), name="uniq_nest_user_badge" + ) + ]
56-69: Same constraint modernization for GithubUserBadge and optional related_nameMirror the UniqueConstraint refactor and consider adding explicit related_name to avoid default *_set names if these are surfaced in admin or used elsewhere.
Apply:
class GithubUserBadge(TimestampedModel): """Through model for Github User and Badge.""" class Meta: db_table = "nest_github_user_badges" verbose_name_plural = "GitHub user badges" - unique_together = ("github_user", "badge") + constraints = [ + models.UniqueConstraint( + fields=("github_user", "badge"), name="uniq_github_user_badge" + ) + ] - github_user = models.ForeignKey("github.User", on_delete=models.CASCADE) - badge = models.ForeignKey("nest.Badge", on_delete=models.CASCADE) + github_user = models.ForeignKey( + "github.User", on_delete=models.CASCADE, related_name="badge_links" + ) + badge = models.ForeignKey( + "nest.Badge", on_delete=models.CASCADE, related_name="github_user_links" + )backend/apps/nest/migrations/0005_userbadge_options.py (1)
1-84: General: Keep migrations formatted; CI indicates ruff-format wants changesRuff formatting is failing in CI for migrations. Please run ruff format or pre-commit locally and commit the changes.
📜 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 (5)
backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/migrations/0035_user_badges.py(1 hunks)backend/apps/github/models/user.py(1 hunks)backend/apps/nest/migrations/0005_userbadge_options.py(1 hunks)backend/apps/nest/models/badge.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
backend/apps/github/models/user.py (2)
backend/apps/github/api/internal/nodes/user.py (1)
badges(31-33)backend/apps/nest/api/internal/nodes/user.py (1)
badges(21-23)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)
backend/apps/nest/models/badge.py (1)
backend/apps/common/models.py (1)
TimestampedModel(37-46)
backend/apps/nest/migrations/0005_userbadge_options.py (1)
backend/apps/github/migrations/0035_user_badges.py (1)
Migration(6-26)
backend/apps/github/migrations/0035_user_badges.py (1)
backend/apps/nest/migrations/0005_userbadge_options.py (1)
Migration(8-83)
🪛 Ruff (0.12.2)
backend/apps/github/models/user.py
53-53: Trailing whitespace
Remove trailing whitespace
(W291)
backend/apps/github/api/internal/nodes/user.py
34-34: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Actions: Run CI/CD
backend/apps/github/models/user.py
[error] 1-1: Ruff formatting applied by pre-commit (ruff-format).
backend/apps/github/api/internal/nodes/user.py
[error] 1-1: Ruff formatting applied by pre-commit (ruff-format).
backend/apps/nest/models/badge.py
[error] 1-1: Ruff formatting applied by pre-commit (ruff-format).
backend/apps/nest/migrations/0005_userbadge_options.py
[error] 1-1: Ruff formatting applied by pre-commit (ruff-format).
backend/apps/github/migrations/0035_user_badges.py
[error] 1-1: Ruff formatting applied by pre-commit (ruff-format).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/apps/github/api/internal/nodes/user_test.py (2)
20-41: Add resolver-focused tests for badges ordering and empty states.Right now we only assert presence. Add targeted tests that:
- Verify badges are ordered by weight, then name (ensures deterministic UI order).
- Verify empty badge relations resolve to an empty list (no None surprises).
Example tests to append in this file:
def test_badges_field_orders_by_weight_then_name(self): mock_qs = Mock() mock_qs.order_by.return_value = ["badge_a", "badge_b"] mock_user = Mock() mock_user.badges = mock_qs result = UserNode.badges(mock_user) assert result == ["badge_a", "badge_b"] mock_qs.order_by.assert_called_once_with("weight", "name") def test_badges_field_handles_no_badges(self): mock_qs = Mock() mock_qs.order_by.return_value = [] mock_user = Mock() mock_user.badges = mock_qs assert UserNode.badges(mock_user) == []If the resolver signature differs (e.g., requires info/context), adjust the call accordingly.
I can push a patch with these tests wired to your preferred test style (pytest/unittest mix) if you confirm the resolver signature.
20-41: Optional improvement: make the field-names assertion future-proofI verified that:
- The
UserNodeinbackend/apps/github/api/internal/nodes/user.pydefines abadgesfield resolver that orders by weight then name- The
BadgeNodeinbackend/apps/nest/api/internal/nodes/badge.pyincludes the expected fields (id,name,description,weight,css_class)Since everything is implemented correctly, this change is purely optional to reduce churn if new non-breaking fields are ever added.
File: backend/tests/apps/github/api/internal/nodes/user_test.py
Lines: 20–41- assert field_names == expected_field_names + # ensure required fields exist while allowing future non-breaking additions + assert expected_field_names.issubset(field_names)
📜 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/github/api/internal/nodes/user_test.py(1 hunks)
🔇 Additional comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
20-23: Good: badges now part of the GraphQL surface contract for UserNode.Including "badges" in expected_field_names keeps the schema contract explicit and guarded against accidental removals.
|
@Piyushrathoree hi! Could you run the checks locally and address those so I could start reviewing this PR? Thanks! |
I'll do this ASAP |
- Introduced a new BadgeIcon component for rendering individual badge icons with tooltips. - Updated UserDetailsPage to use BadgeIcon instead of BadgeList for displaying badges. - Refactored tests to accommodate the new BadgeIcon component and ensure proper rendering. - Removed the BadgeList component as it is no longer needed. - Updated badge data structures to include an 'id' field for better identification.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
frontend/src/types/badge.ts (1)
1-7: Type shape looks right; consider making weight required if backend always returns itIf BadgeNode.weight is non-null in GraphQL, make
weightnon-optional to simplify consumers and avoid defensive checks. Otherwise keep as-is.frontend/__tests__/unit/data/mockUserDetails.ts (1)
16-45: Add a negative-case badge to exercise fallback icon/tooltip behaviorInclude one badge without
cssClass(and optionally withoutdescription) so BadgeIcon/tooltip fallback paths are covered by tests.Example patch:
{ id: '4', name: 'Project Lead', description: 'Leads multiple OWASP projects', cssClass: 'fa-solid fa-crown', weight: 1, }, + { + id: '5', + name: 'Early Supporter', + // description omitted on purpose to test fallback + // cssClass omitted on purpose to test default icon + weight: 5, + },frontend/src/server/queries/userQueries.ts (3)
84-90: Ensure ordering is stable; otherwise sort client-sideBackends claim weight ordering, but add a stable secondary (e.g., name) on client if needed to prevent UI jitter.
Example client sort before render:
const sortedBadges = (user.badges ?? []).toSorted( (a, b) => (a.weight ?? 0) - (b.weight ?? 0) || a.name.localeCompare(b.name), )
100-106: Rename duplicate operation names to avoid ambiguityAll three queries are named
GetUser. Use distinct names to ease debugging and avoid tooling confusion.Apply:
-export const GET_LEADER_DATA = gql` - query GetUser($key: String!) { +export const GET_LEADER_DATA = gql` + query GetLeaderData($key: String!) { @@ -export const GET_USER_DATA = gql` - query GetUser($key: String!) { +export const GET_USER_DATA = gql` + query GetUserData($key: String!) { @@ -export const GET_USER_METADATA = gql` - query GetUser($key: String!) { +export const GET_USER_METADATA = gql` + query GetUserMetadata($key: String!) {
9-15: Ensure GraphQL exposescssClassand optionally optimize payload & cacheI verified that the Django model defines the field as
css_class, and Strawberry’s defaultauto_camel_casesetting (enabled by default) converts this tocssClassin the GraphQL schema—no change required unless you’ve overriddenauto_camel_casein your schema configuration (strawberry.rocks).Optional refinements:
- If the leader view only needs the badge icon, consider omitting
descriptionto slim the payload.- Adding
__typenamecan improve Apollo cache stability and debugging.Suggested diff:
badges { + __typename id name - description + description cssClass weight }frontend/__tests__/unit/data/mockUserData.ts (1)
11-33: Broaden mocks to cover overflow (>6), missing fields, and tie-breaker sorting
- Add one user with >6 badges to exercise compact/overflow + tooltip logic.
- Include at least one badge missing
cssClassand/ordescriptionto verify fallback icon/labels.- You already have duplicate weights—assert stable secondary sort (e.g., by name).
Examples:
@@ user_2 badges { id: '5', name: 'Mentor', description: 'Mentors new contributors and developers', cssClass: 'fa-solid fa-graduation-cap', weight: 2, }, + { id: '10', name: 'Speaker', cssClass: 'fa-solid fa-microphone', weight: 4 }, + { id: '11', name: 'Tester', cssClass: 'fa-solid fa-vial', weight: 4 }, + { id: '12', name: 'Designer', cssClass: 'fa-solid fa-pen-nib', weight: 4 }, + { id: '13', name: 'Organizer', cssClass: 'fa-solid fa-calendar-check', weight: 4 }, @@ user_3 badges { id: '6', name: 'First Contribution', description: 'Made their first contribution to OWASP', cssClass: 'fa-solid fa-seedling', weight: 4, }, + { + id: '14', + name: 'No Icon Badge', + // cssClass omitted intentionally to test default icon path + description: 'Validate fallback', + weight: 5, + },Also applies to: 43-59, 60-77, 86-108
frontend/src/wrappers/FontAwesomeIconWrapper.tsx (2)
6-6: Makeiconoptional to reflect the default prop.Minor typing cleanup; default is provided.
-interface MyIconProps extends Omit<FontAwesomeIconProps, 'icon'> { - icon: string | IconProp -} +interface MyIconProps extends Omit<FontAwesomeIconProps, 'icon'> { + icon?: string | IconProp +}
10-10: Comment contradicts behavior.You say “not using string-based icons” but you pass string-based icons. Remove or reword.
- // For now, just use the default icon since we're not using string-based icons + // Accepts both IconProp and FA CSS class strings (e.g., "fa-solid fa-medal")frontend/src/components/BadgeIcon.tsx (2)
11-11: Prefer description in tooltip with name fallback.Aligns with Badges.tsx and provides richer context.
- const cssClass = badge.cssClass || 'fa-solid fa-medal' // Default to medal icon if no cssClass + const cssClass = badge.cssClass || 'fa-solid fa-medal' // Default to medal icon if no cssClassAnd update the Tooltip line:
- <Tooltip content={badge.name} placement="top" showArrow delay={100} closeDelay={100}> + <Tooltip content={badge.description || badge.name} placement="top" showArrow delay={100} closeDelay={100}>
15-19: Accessibility: add an ARIA label to the icon container.Tooltips are not always announced to assistive tech. Add
aria-label.- <span + <span + aria-label={badge.description || badge.name} className={`inline-flex h-5 w-5 items-center justify-center text-gray-600 hover:text-gray-800 dark:text-gray-300 dark:hover:text-gray-100 ${className}`} >frontend/__tests__/unit/components/BadgeIcon.test.tsx (2)
5-9: Capture theiconprop in the mock so we can assert default vs provided icons.This makes the tests validate behavior (provided cssClass vs default medal), not just presence.
-jest.mock('@fortawesome/react-fontawesome', () => ({ - FontAwesomeIcon: ({ className }: { className?: string }) => ( - <span data-testid="fa-icon" className={className} /> - ), -})) +jest.mock('@fortawesome/react-fontawesome', () => ({ + FontAwesomeIcon: ({ className, icon }: { className?: string; icon?: unknown }) => ( + <span data-testid="fa-icon" className={className} data-icon={typeof icon === 'string' ? icon : JSON.stringify(icon)} /> + ), +}))Update assertions:
- it('renders badge icon with correct cssClass', () => { + it('renders badge icon with correct cssClass', () => { render(<BadgeIcon badge={mockBadge} />) - expect(screen.getByTestId('fa-icon')).toBeInTheDocument() + const el = screen.getByTestId('fa-icon') + expect(el).toBeInTheDocument() + expect(el).toHaveAttribute('data-icon', 'fa-solid fa-star') }) @@ - it('uses default medal icon when no cssClass provided', () => { + it('uses default medal icon when no cssClass provided', () => { const badgeWithoutCssClass = { ...mockBadge, cssClass: undefined } render(<BadgeIcon badge={badgeWithoutCssClass} />) - expect(screen.getByTestId('fa-icon')).toBeInTheDocument() + const el = screen.getByTestId('fa-icon') + expect(el).toBeInTheDocument() + expect(el).toHaveAttribute('data-icon', 'fa-solid fa-medal') })
11-13: Tooltip is fully mocked; consider verifying content via props capture.Optional: have the Tooltip mock expose its
contentprop (e.g., render a wrapper withdata-tooltip), then assert it equalsbadge.description || badge.name.frontend/src/components/Badges.tsx (2)
12-23: Duplicate BadgeIcon implementation; import and reuse the shared component.Avoids drift with BadgeIcon.tsx and centralizes tooltip/icon logic.
-import { Tooltip } from '@heroui/tooltip' -import FontAwesomeIconWrapper from 'wrappers/FontAwesomeIconWrapper' +import BadgeIcon from 'components/BadgeIcon' import type { Badge as BadgeType } from 'types/badge' @@ -const BadgeIcon = ({ badge }: { badge: BadgeType }) => { - const title = badge.description || badge.name - const cssClass = badge.cssClass || 'fa-solid fa-medal' - - return ( - <Tooltip content={title} placement="top" showArrow delay={100} closeDelay={100}> - <span className="inline-flex h-4 w-4 items-center justify-center"> - <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" /> - </span> - </Tooltip> - ) -} +// Use shared BadgeIcon for consistencyIf you need the 16px size here, pass a className:
- <BadgeIcon key={`${badge.name}-${idx}`} badge={badge} /> + <BadgeIcon key={`${badge.id ?? `${badge.name}-${idx}`}`} badge={badge} className="h-4 w-4" />
33-35: Use stable keys.Prefer
badge.idover name+index to prevent unnecessary re-renders.- {visibleBadges.map((badge, idx) => ( - <BadgeIcon key={`${badge.name}-${idx}`} badge={badge} /> - ))} + {visibleBadges.map((badge, idx) => ( + <BadgeIcon key={badge.id ?? `${badge.name}-${idx}`} badge={badge} className="h-4 w-4" /> + ))}
📜 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 (13)
frontend/__tests__/unit/components/BadgeIcon.test.tsx(1 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/components/BadgesTooltip.test.tsx(1 hunks)frontend/__tests__/unit/components/UserCard.badges.test.tsx(1 hunks)frontend/__tests__/unit/data/mockUserData.ts(2 hunks)frontend/__tests__/unit/data/mockUserDetails.ts(1 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/components/BadgeCount.tsx(1 hunks)frontend/src/components/BadgeIcon.tsx(1 hunks)frontend/src/components/Badges.tsx(1 hunks)frontend/src/server/queries/userQueries.ts(3 hunks)frontend/src/types/badge.ts(1 hunks)frontend/src/wrappers/FontAwesomeIconWrapper.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/unit/components/BadgesTooltip.test.tsx
- frontend/src/app/members/[memberKey]/page.tsx
- frontend/tests/unit/components/Badges.test.tsx
- frontend/tests/unit/components/UserCard.badges.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/BadgeIcon.tsx (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
frontend/src/components/BadgeCount.tsx (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
🪛 GitHub Actions: Run CI/CD
frontend/src/components/BadgeIcon.tsx
[error] 10-10: ESLint: 'React' is not defined. (no-undef)
🔇 Additional comments (3)
frontend/src/components/BadgeCount.tsx (1)
11-22: BadgeCount: Prop optionality & accessibility enhancements appliedThe proposed changes have been implemented and verified:
- BadgeCountProps updated to make
badgesoptional, with a default empty array- Destructuring default (
badges = []) prevents any run-time/TS errors- Added
aria-labelfor screen-reader support- Marked the medal icon as
aria-hiddenand improved dark-mode contrast (dark:text-yellow-400)- Formatted the count with
toLocaleString()for locale-aware digit grouping- Verified call sites: only one occurrence—
<BadgeCount badges={badges} />inUserCard.tsx—and it always passes a defined array, so no explicitundefinedis ever sentAll checks pass.
frontend/src/components/BadgeIcon.tsx (1)
10-21: Re-run verification after stripping React.FC annotationsPlease remove the
: React.FC<...>typing from BadgeIcon (and any other components) so that noReact.FCreferences remain. Then, in your project root, re-run the following checks:
- Search for leftover
React.FCtypings in TSX files:rg -nP '\bReact\.FC\b' -C1 -g '*.tsx' frontend/src || true- Install dependencies and run CI checks:
npm install npm run lint npm testLocation to verify:
frontend/src/components/BadgeIcon.tsx(lines 10–21)frontend/src/components/Badges.tsx (1)
25-41: Behavior looks good overall.Null-return when empty, compact overflow count, and slicing to
maxare correct.
- Updated migration 0035_user_badges to include a ManyToManyField for user badges. - Created new migration 0005_alter_userbadge_options_remove_user_badges_and_more to adjust userbadge model options and remove unnecessary fields. - Deleted obsolete migration file 0005_userbadge_options.py to clean up the migration history.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py (3)
9-12: Add swappable dependency for AUTH_USER_MODEL to avoid migration ordering issues.When targeting settings.AUTH_USER_MODEL, include migrations.swappable_dependency to guard against non-nest user models in different deployments.
Apply this diff:
class Migration(migrations.Migration): dependencies = [ - ("github", "0034_merge_20250804_1817"), - ("nest", "0004_userbadge_user_badges"), + ("github", "0034_merge_20250804_1817"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("nest", "0004_userbadge_user_badges"), ]
24-34: Dropping awarded_at/awarded_by/notes will permanently erase history; confirm business intent or archive first.If any production data exists, add a small data migration to snapshot these fields before removal, or move into an audit table/JSON metadata.
I can draft a follow-up migration that writes a JSON export to a file or migrates into a BadgeAwardAudit model. Want me to provide it?
47-76: Modernize uniqueness constraints with UniqueConstraintWhile uniqueness on both
(github_user, badge)and(user, badge)is already enforced viaunique_together, it’s recommended to use Django’sUniqueConstraintfor clearer naming and future-proofing.• In
backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py, replace theunique_togetheroption for GitHubUserBadge with aUniqueConstraint:options={ "verbose_name_plural": "GitHub user badges", "db_table": "nest_github_user_badges", - "unique_together": {("github_user", "badge")}, + "constraints": [ + models.UniqueConstraint( + fields=("github_user", "badge"), + name="uniq_github_user_badge", + ), + ], },• Likewise, you may choose to modernize the UserBadge constraint in
backend/apps/nest/migrations/0004_userbadge_user_badges.pyby swapping itsunique_together = {("user", "badge")}for:options={ "db_table": "nest_user_badges", "ordering": ["-awarded_at"], - "unique_together": {("user", "badge")}, + "constraints": [ + models.UniqueConstraint( + fields=("user", "badge"), + name="uniq_user_badge", + ), + ], },• If you prefer consistency at the model level, update the
Metaclasses inbackend/apps/nest/models/badge.pyfor both UserBadge and GithubUserBadge, replacing theirunique_togetherdefinitions withconstraints = [ UniqueConstraint(...) ].These changes are optional refactors that improve clarity and leverage modern Django features.
📜 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 (2)
backend/apps/github/migrations/0035_user_badges.py(1 hunks)backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/migrations/0035_user_badges.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py (1)
backend/apps/github/migrations/0035_user_badges.py (1)
Migration(6-25)
⏰ 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/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py (3)
15-18: LGTM: clearer pluralization for UserBadge.
35-39: Confirm cascade deletes are desired.on_delete=CASCADE for both user and badge will silently wipe link rows on deletion. If auditability is required, consider PROTECT or soft delete patterns.
Also applies to: 41-46
9-12: Migration dependencies ordering is correctI’ve verified that:
nestmigration 0005 (0005_alter_userbadge_options_remove_user_badges_and_more) declares dependencies ongithub.0034_merge_20250804_1817andnest.0004_userbadge_user_badges.githubmigration 0035 (0035_user_badges.py) in turn depends onnest.0005_alter_userbadge_options_remove_user_badges_and_more.This establishes a clean, linear dependency chain:
- Apply all earlier
githubmigrations up through 0034- Apply
nestmigrations through 0004- Apply
nest0005- Finally apply
github0035With this setup, both a fresh database and an existing one will follow the same topologically sorted order without conflicts or branching. No further adjustments are needed here.
backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py
Show resolved
Hide resolved
- Updated UserNode and AuthUserNode to retrieve badges using the Badge model instead of relying on the previous method. - Adjusted User model to ensure proper ManyToMany relationship through the GithubUserBadge model. - Refined migration files to implement unique constraints for user-badge relationships, improving data integrity. - Added new entries to the custom dictionary for spell checking related to user badges.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cspell/custom-dict.txt (2)
63-63: Consider adding plural/casing variants for stability.If code or GraphQL types use plural or different casing (e.g., GithubUserBadge/GitHubUserBadge, githubuserbadges), adding them now prevents future cspell churn. Keep alphabetical order.
Proposed additions:
geopy -githubuserbadge +githubuserbadge +githubuserbadges graphiqlOptional (only if your repo commonly uses brand-accurate casing in identifiers):
+GitHubUserBadge
124-124: Add plural to cover typical identifier/field names.Projects often use both UserBadge and UserBadges (e.g., relation names). Adding the plural avoids intermittent spellcheck failures.
usefixtures -userbadge +userbadge +userbadges winsrdf
📜 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 (7)
backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/models/user.py(1 hunks)backend/apps/nest/api/internal/nodes/user.py(2 hunks)backend/apps/nest/migrations/0004_userbadge_user_badges.py(1 hunks)backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py(1 hunks)backend/apps/nest/models/badge.py(1 hunks)cspell/custom-dict.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/apps/github/models/user.py
- backend/apps/nest/api/internal/nodes/user.py
- backend/apps/nest/migrations/0004_userbadge_user_badges.py
- backend/apps/nest/migrations/0005_alter_userbadge_options_remove_user_badges_and_more.py
- backend/apps/github/api/internal/nodes/user.py
- backend/apps/nest/models/badge.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). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- Changed the assertion for user cards to check for 'John Doe' and updated the length check. - Refactored the button selection logic to filter for 'View Profile' buttons, ensuring the correct count is asserted.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/Users.test.tsx (1)
91-92: Avoid asserting a singular occurrence of a user’s name.Asserting exactly one "John Doe" can be flaky if the name appears in tooltips/alt text. Prefer asserting the number of user cards, or scope the query within each card (e.g., using
within(card).getByText(...)) or adata-testidon the card.
📜 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)
frontend/__tests__/unit/pages/Users.test.tsx(1 hunks)
⏰ 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
| const buttons = screen.getAllByRole('button') | ||
|
|
||
| const viewProfileButtons = buttons.filter((button) => | ||
| button.textContent?.includes('View Profile') | ||
| ) | ||
| expect(viewProfileButtons).toHaveLength(4) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use accessible queries and avoid hardcoded counts.
Filter by accessible name instead of textContent, and derive the expected count from mock data to reduce brittleness and i18n issues.
- const buttons = screen.getAllByRole('button')
-
- const viewProfileButtons = buttons.filter((button) =>
- button.textContent?.includes('View Profile')
- )
- expect(viewProfileButtons).toHaveLength(4)
+ const viewProfileButtons = screen.getAllByRole('button', { name: /view profile/i })
+ expect(viewProfileButtons).toHaveLength(mockUserData.users.length)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const buttons = screen.getAllByRole('button') | |
| const viewProfileButtons = buttons.filter((button) => | |
| button.textContent?.includes('View Profile') | |
| ) | |
| expect(viewProfileButtons).toHaveLength(4) | |
| const viewProfileButtons = screen.getAllByRole('button', { name: /view profile/i }) | |
| expect(viewProfileButtons).toHaveLength(mockUserData.users.length) |
🤖 Prompt for AI Agents
In frontend/__tests__/unit/pages/Users.test.tsx around lines 93 to 98, the test
currently filters buttons by button.textContent and asserts a hardcoded length;
change it to use an accessible query that matches the button's accessible name
(e.g., getAllByRole('button', { name: /view profile/i }) or getByRole variants)
and derive the expected count from the mock users data used in the test (use the
mock array length or a helper that returns the number of users), then assert the
result length against that derived value.
- Added imports for UserBadge and GithubUserBadge to enhance badge functionality. - Retained the existing User import for continued user model support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/nest/models/__init__.py (1)
1-3: Optional: make exports explicit via allDeclaring all clarifies the public API of models and avoids accidental re-exports.
Apply this diff:
from .api_key import ApiKey from .badge import Badge, GithubUserBadge, UserBadge from .user import User + +__all__ = [ + "ApiKey", + "Badge", + "GithubUserBadge", + "User", + "UserBadge", +]
📜 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/apps/nest/models/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/nest/models/__init__.py (1)
backend/apps/nest/models/badge.py (3)
Badge(10-37)GithubUserBadge(56-69)UserBadge(40-53)
🪛 GitHub Actions: Run CI/CD
backend/apps/nest/models/__init__.py
[error] 2-4: Ruff: F811 Redefinition of unused UserBadge (imported from .badge) at line 2; redefined at line 4 (import from .user_badge). Command: pre-commit run --show-diff-on-failure --color=always --all-files.
⏰ 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 (1)
backend/apps/nest/models/__init__.py (1)
2-4: No duplicate UserBadge import detected
The file already imports UserBadge only from.badgeand does not import it from.user_badge. No changes required.Likely an incorrect or invalid review comment.
- Included UserBadge and GithubUserBadge in the __all__ list to enhance module exports. - This change facilitates easier access to these classes when importing from the nest.models package.
|
|
I'm closing this issue and restart working on this I'll submit the PR soon , ( I'm having some issue and they are not resolving ) |
Proposed change
#1764
#1765
it will show something like that
Issues Resolved
Closes #1765
Closes #1764
##PR descriptions
##Requirements Implemented
##Technical Implementation
##Features
##Quality Assurance
Checklist
make check-testlocally; all checks and tests passed.