Skip to content

Impove chapter/project leaders presentation #2411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Oct 12, 2025
Merged

Impove chapter/project leaders presentation #2411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4fa213c
add `entity_leaders` node and field
rudransh-shrivastava Oct 10, 2025
89e5fac
refactor and extract Leaders component
rudransh-shrivastava Oct 10, 2025
87afa73
add tests, Coderabbit and Sonar bot suggestions
rudransh-shrivastava Oct 10, 2025
5f4839c
Update code
rudransh-shrivastava Oct 10, 2025
e638ef5
sort alphabetically
rudransh-shrivastava Oct 10, 2025
c523c7b
use fontawesome icon
rudransh-shrivastava Oct 10, 2025
e31ed65
Update docstring
rudransh-shrivastava Oct 10, 2025
218efdb
Update code
rudransh-shrivastava Oct 10, 2025
269daf7
sort alphabetically
rudransh-shrivastava Oct 10, 2025
fa340f7
Merge branch 'main' into feature/leaders-presentation
rudransh-shrivastava Oct 10, 2025
e468270
Merge branch 'main' into pr/rudransh-shrivastava/2411
arkid15r Oct 12, 2025
f7c3fa2
Update code
arkid15r Oct 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions backend/apps/owasp/api/internal/nodes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@
import strawberry

from apps.github.api.internal.nodes.repository_contributor import RepositoryContributorNode
from apps.owasp.api.internal.nodes.entity_member import EntityMemberNode


@strawberry.type
class GenericEntityNode(strawberry.relay.Node):
"""Base node class for OWASP entities with common fields and resolvers."""

@strawberry.field
def entity_leaders(self) -> list[EntityMemberNode]:
"""Resolve entity leaders."""
return self.entity_leaders

@strawberry.field
def leaders(self) -> list[str]:
"""Resolve leaders."""
Expand Down
25 changes: 25 additions & 0 deletions backend/apps/owasp/api/internal/nodes/entity_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
"""OWASP app entity member GraphQL node."""

import strawberry
import strawberry_django

from apps.github.api.internal.nodes.user import UserNode
from apps.owasp.models.entity_member import EntityMember


@strawberry_django.type(
EntityMember,
fields=[
"description",
"is_active",
"is_reviewed",
"member_email",
"member_name",
"order",
"role",
],
)
class EntityMemberNode(strawberry.relay.Node):
"""Entity member node."""

member: UserNode | None
Comment on lines +22 to +25
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

LGTM! Verify dataloader usage for optimal performance.

The member relationship field is correctly typed as UserNode | None, matching the nullable foreign key in the EntityMember model. Strawberry Django should automatically resolve this relationship using a dataloader to avoid N+1 queries when fetching multiple leaders.

To confirm this optimization is working, verify that the generated GraphQL queries for lists of leaders (e.g., on chapter/project pages) batch the user lookups rather than issuing separate queries per leader.

Run the following script to check if strawberry_django dataloaders are configured in your schema:

🏁 Script executed:

#!/bin/bash
# Description: Check for dataloader configuration in the GraphQL schema setup

# Search for strawberry schema instantiation and dataloader config
rg -n --type=py -A 5 -B 2 'strawberry\.Schema|GraphQLView|GraphQLRouter' | head -50

Length of output: 1322

🏁 Script executed:

#!/bin/bash
# Inspect Schema instantiation and plugin configuration
rg -nP 'schema\s*=\s*strawberry\.Schema' -A5 -B5 backend/settings/graphql.py

# Check for plugins argument in the Schema call
rg -n 'plugins' -C3 backend/settings/graphql.py

# Search for any dataloader imports or decorators
rg -n 'dataloader' -C3 -g '*.py'

Length of output: 125

Configure DataLoaderPlugin in the Strawberry Schema

The schema instantiation in backend/settings/graphql.py (around line 43) is currently:

schema = strawberry.Schema(mutation=Mutation, query=Query)

Without the DataLoader plugin, member relationships won’t be batched, leading to N+1 queries. Update it to:

from strawberry.django.plugin import DjangoPlugin
from strawberry.dataloader import DataLoaderPlugin

schema = strawberry.Schema(
  query=Query,
  mutation=Mutation,
  plugins=[DjangoPlugin(), DataLoaderPlugin()],
)

This ensures all FK relations (like member) are fetched in batches.

🤖 Prompt for AI Agents 
In backend/apps/owasp/api/internal/nodes/entity_member.py around lines 22 to 25
the EntityMemberNode has a member FK that will cause N+1 queries unless the
Strawberry schema is configured with the DataLoader plugin; update
backend/settings/graphql.py (around line 43) to import DjangoPlugin and
DataLoaderPlugin and instantiate the schema with plugins=[DjangoPlugin(),
DataLoaderPlugin()] (ensure the imports are added at the top) so FK relations
like member are batched.

Copy link
Collaborator Author

@rudransh-shrivastava rudransh-shrivastava Oct 10, 2025

Choose a reason for hiding this comment

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

is this relevant to the current PR objective?

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025

Choose a reason for hiding this comment

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

@rudransh-shrivastava, you're right to question this—it's not strictly required for the functional PR objective. The leaders block will work and display correctly without the DataLoader plugin configuration.

However, it's a performance consideration: without DataLoaders, when rendering multiple leaders on chapter/project pages, Django will issue separate database queries for each leader's member relationship (the N+1 query problem). This could impact page load times, especially for entities with many leaders.

Your options:

  1. Address it now (adds ~3 lines to backend/settings/graphql.py) to proactively prevent performance issues
  2. Defer to a follow-up if you want to keep this PR focused solely on the functional requirements

Would you like me to create a separate issue to track this performance optimization?

Copy link
Collaborator Author

@rudransh-shrivastava rudransh-shrivastava Oct 10, 2025

Choose a reason for hiding this comment

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

@arkid15r Hello, is this something we should consider? I assume the actual solution might be a bit more complex since DataLoaderPlugin import CR suggests, does not exist. DataLoader exists, but I'm not sure.

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

@arkid15r arkid15r Oct 12, 2025

Choose a reason for hiding this comment

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

@rudransh-shrivastava could you create an issue to look into this later? Thank you!

21 changes: 11 additions & 10 deletions backend/apps/owasp/models/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
GITHUB_REPOSITORY_RE,
GITHUB_USER_RE,
)
from apps.github.models.user import User
from apps.github.utils import get_repository_file_content
from apps.owasp.models.entity_member import EntityMember
from apps.owasp.models.enums.project import AudienceChoices
Expand Down Expand Up @@ -90,6 +89,17 @@ class Meta:
blank=True,
)

@property
def entity_leaders(self) -> models.QuerySet[EntityMember]:
"""Return entity's leaders."""
return EntityMember.objects.filter(
entity_id=self.id,
entity_type=ContentType.objects.get_for_model(self.__class__),
is_active=True,
is_reviewed=True,
role=EntityMember.Role.LEADER,
).order_by("order")

@property
def github_url(self) -> str:
"""Get GitHub URL."""
Expand All @@ -115,15 +125,6 @@ def info_md_url(self) -> str | None:
else None
)

@property
def entity_leaders(self) -> models.QuerySet[User]:
"""Return entity's leaders."""
return User.objects.filter(
pk__in=self.members.filter(role=EntityMember.Role.LEADER).values_list(
"member_id", flat=True
)
)

@property
def leaders_md_url(self) -> str | None:
"""Return entity's raw leaders.md GitHub URL."""
Expand Down
11 changes: 11 additions & 0 deletions backend/apps/owasp/models/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import datetime
from functools import lru_cache
from typing import TYPE_CHECKING

from django.contrib.contenttypes.fields import GenericRelation
from django.db import models
Expand All @@ -27,6 +28,11 @@
from apps.owasp.models.mixins.project import ProjectIndexMixin
from apps.owasp.models.project_health_metrics import ProjectHealthMetrics

if TYPE_CHECKING:
from apps.owasp.models.entity_member import EntityMember

MAX_LEADERS_COUNT = 5


class Project(
BulkSaveModel,
Expand Down Expand Up @@ -124,6 +130,11 @@ def __str__(self) -> str:
"""Project human readable representation."""
return f"{self.name or self.key}"

@property
def entity_leaders(self) -> models.QuerySet[EntityMember]:
"""Return project leaders."""
return super().entity_leaders[:MAX_LEADERS_COUNT]

@property
def health_score(self) -> float | None:
"""Return project health score."""
Expand Down
6 changes: 6 additions & 0 deletions frontend/__tests__/e2e/pages/ChapterDetails.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,10 @@ test.describe('Chapter Details Page', () => {
await page.getByRole('button', { name: 'Show less' }).click()
await expect(page.getByRole('button', { name: 'Show more' })).toBeVisible()
})

test('should have leaders block', async ({ page }) => {
await expect(page.getByRole('heading', { name: 'Leaders' })).toBeVisible()
await expect(page.getByText('Bob')).toBeVisible()
await expect(page.getByText('Chapter Leader')).toBeVisible()
})
})
7 changes: 7 additions & 0 deletions frontend/__tests__/e2e/pages/ProjectDetails.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,11 @@ test.describe('Project Details Page', () => {
await expect(page.getByText('Forks Trend')).toBeVisible()
await expect(page.getByText('Days Since Last Commit and Release')).toBeVisible()
})

test('should have leaders block', async ({ page }) => {
await expect(page.getByRole('heading', { name: 'Leaders' })).toBeVisible()
const userCard = page.getByRole('button', { name: /Alice/ })
await expect(userCard.locator('h3')).toHaveText('Alice')
await expect(userCard.getByText('Project Leader')).toBeVisible()
})
})
19 changes: 19 additions & 0 deletions frontend/__tests__/unit/components/CardDetailsPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,25 @@ describe('CardDetailsPage', () => {
expect(screen.getByTestId('leaders-list')).toBeInTheDocument()
})

it('renders Leaders component when entityLeaders are provided', () => {
const entityLeaders = [
{
description: 'Project Leader',
memberName: 'Alice',
member: {
id: '1',
login: 'alice',
name: 'Alice',
avatarUrl: 'https://avatars.githubusercontent.com/u/12345?v=4',
},
},
]
render(<CardDetailsPage {...defaultProps} entityLeaders={entityLeaders} />)
expect(screen.getByText('Leaders')).toBeInTheDocument()
expect(screen.getByText('Alice')).toBeInTheDocument()
expect(screen.getByText('Project Leader')).toBeInTheDocument()
})

it('capitalizes entity type in details title', () => {
render(<CardDetailsPage {...defaultProps} type="project" />)

Expand Down
52 changes: 52 additions & 0 deletions frontend/__tests__/unit/components/Leaders.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { render, screen, fireEvent } from '@testing-library/react'
import { mockProjectDetailsData } from '@unit/data/mockProjectDetailsData'
import { useRouter } from 'next/navigation'
import Leaders from 'components/Leaders'

jest.mock('next/navigation', () => ({
useRouter: jest.fn(),
}))

jest.mock('components/AnchorTitle', () => ({
__esModule: true,
default: ({ title }: { title: string }) => <span>{title}</span>,
}))

jest.mock('wrappers/FontAwesomeIconWrapper', () => ({
__esModule: true,
default: () => <i className="fa" />,
}))

describe('Leaders Component', () => {
const push = jest.fn()
beforeEach(() => {
;(useRouter as jest.Mock).mockReturnValue({ push })
})

afterEach(() => {
jest.clearAllMocks()
})

it('renders a list of leaders', () => {
render(<Leaders users={mockProjectDetailsData.project.entityLeaders} />)

expect(screen.getByText('Leaders')).toBeInTheDocument()
expect(screen.getByText('Alice')).toBeInTheDocument()
expect(screen.getByText('Project Leader')).toBeInTheDocument()
})

it('renders the section title even with no leaders', () => {
render(<Leaders users={[]} />)
expect(screen.getByText('Leaders')).toBeInTheDocument()
expect(screen.queryByText('Alice')).not.toBeInTheDocument()
})

it('navigates to the correct page on button click', () => {
render(<Leaders users={mockProjectDetailsData.project.entityLeaders} />)

const viewProfileButton = screen.getByText('View Profile')
fireEvent.click(viewProfileButton)

expect(push).toHaveBeenCalledWith('/members/alice')
})
})
12 changes: 12 additions & 0 deletions frontend/__tests__/unit/data/mockChapterDetailsData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ export const mockChapterDetailsData = {
lat: 23.2584857,
lng: 77.401989,
},
entityLeaders: [
{
description: 'Chapter Leader',
memberName: 'Bob',
member: {
id: '2',
login: 'bob',
name: 'Bob',
avatarUrl: 'https://avatars.githubusercontent.com/u/67890?v=4',
},
},
],
establishedYear: 2020,
key: 'test-chapter',
},
Expand Down
12 changes: 12 additions & 0 deletions frontend/__tests__/unit/data/mockProjectDetailsData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
export const mockProjectDetailsData = {
project: {
contributorsCount: 1200,
entityLeaders: [
{
description: 'Project Leader',
memberName: 'Alice',
member: {
id: '1',
login: 'alice',
name: 'Alice',
avatarUrl: 'https://avatars.githubusercontent.com/u/12345?v=4',
},
},
],
forksCount: 10,
healthMetricsList: [
{
Expand Down
Loading