Conversation
Summary by CodeRabbitRelease Notes
WalkthroughAdds first OWASP contribution timestamp and per-chapter/project contribution tracking; introduces GraphQL nodes/queries for BoardOfDirectors and MemberSnapshot; extends User model with chapters/projects; updates GitHub sync command and admin UIs; adds migrations, backend tests, and a frontend board candidates page with contribution heatmap. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/owasp/management/commands/owasp_create_member_snapshot.py (1)
200-202: Validate date range (start <= end).Add a guard to prevent inverted ranges.
end_at = self.parse_date(options.get("end_at"), default_end) start_at = self.parse_date(options.get("start_at"), default_start) +if start_at > end_at: + msg = f"Start date {start_at.date()} must be on or before end date {end_at.date()}." + self.stderr.write(self.style.ERROR(msg)) + logger.error(msg) + return
🧹 Nitpick comments (21)
backend/apps/owasp/management/commands/owasp_create_member_snapshot.py (2)
102-105: Tighten typing for clarity and tooling.Specify precise types for parameters and return to aid callers and static analysis.
-from typing import Literal +from typing import Literal @@ - def generate_entity_contributions( - self, user, commits, pull_requests, issues, entity_type: str - ) -> dict: + def generate_entity_contributions( + self, + user: User, + commits, + pull_requests, + issues, + entity_type: Literal["chapter", "project"], + ) -> dict[str, int]: """Generate contribution counts per chapter or project led by the user.
140-164: Repo mapped to multiple led entities: define behavior.Currently
repo_to_entityis 1→1; if a repo is linked to multiple led projects, last write wins. Decide whether to:
- attribute to all matching entities, or
- enforce 1→1 mapping and surface a warning.
If attributing to all, switch to
dict[int, set[str]]and increment for each entity key.backend/apps/github/admin/user.py (1)
15-16: LGTM; surfaced as read-only.Exposing
first_contribution_atin the inline and marking it read-only is appropriate.Optionally set
extra = 0on the inline to avoid rendering an extra empty form.backend/tests/apps/owasp/api/internal/nodes/board_of_directors_test.py (1)
7-17: Expand test to exercise resolver behavior, not just presence.Add a case that instantiates the node with a model/mock and asserts
owasp_url,candidates, andmembersresolve to expected values (non-empty lists, correct URL). This will catch resolver regressions and naming mismatches (snake vs camel).backend/apps/owasp/api/internal/queries/__init__.py (1)
16-28: Schema growth sanity-check.With two new mixins, ensure schema snapshot/regression tests were updated if you keep snapshots. If not present, consider adding one to catch accidental field removals/renames in future.
backend/apps/owasp/admin/member_snapshot.py (1)
42-44: Admin UX: pretty-print large JSON fields.chapter_contributions and project_contributions will be hard to read as raw JSON. Prefer read-only display helpers with pretty JSON.
Apply:
@@ - "contribution_heatmap_data", - "chapter_contributions", - "project_contributions", + "contribution_heatmap_data", + "display_chapter_contributions", + "display_project_contributions", @@ - "contribution_heatmap_data", - "chapter_contributions", - "project_contributions", + "contribution_heatmap_data", + "display_chapter_contributions", + "display_project_contributions",Add helpers (outside shown range):
import json from django.utils.html import format_html from django.contrib import admin @admin.display(description="Chapter contributions") def display_chapter_contributions(self, obj): return format_html("<pre>{}</pre>", json.dumps(obj.chapter_contributions or {}, indent=2)) @admin.display(description="Project contributions") def display_project_contributions(self, obj): return format_html("<pre>{}</pre>", json.dumps(obj.project_contributions or {}, indent=2))Also applies to: 79-81
backend/tests/apps/owasp/api/internal/nodes/member_snapshot_test.py (1)
11-14: Expand schema-shape assertions; remove unused mocks.Add asserts for new fields (chapter_contributions, project_contributions) and drop the unused mock_snapshot assignments.
- mock_snapshot = Mock() - mock_snapshot.start_at = "2025-01-01" - mock_snapshot.end_at = "2025-10-01" - mock_snapshot.contribution_heatmap_data = {"2025-01-15": 5} + # no instance needed; we only validate node definition @@ assert "contribution_heatmap_data" in field_names + assert "chapter_contributions" in field_names + assert "project_contributions" in field_names assert "github_user" in field_names assert "commits_count" in field_names assert "pull_requests_count" in field_names assert "issues_count" in field_names assert "total_contributions" in field_namesAlso applies to: 19-26
backend/apps/owasp/models/member_profile.py (1)
34-39: Index first_contribution_at for expected queries/sorts.This timestamp will likely be filtered/ordered on; add db_index for performance.
- first_contribution_at = models.DateTimeField( + first_contribution_at = models.DateTimeField( blank=True, null=True, + db_index=True, verbose_name="First OWASP Contribution", help_text="Date of the member's first contribution to OWASP repositories", )Note: add a migration.
backend/tests/apps/owasp/api/internal/queries/member_snapshot_test.py (1)
70-96: Tighten expectations for ordering and slicing in by-user path.Also assert order_by("-start_at") and limit slicing are applied when user_login is provided.
@@ mock_snapshot_cls.objects.all.return_value = mock_queryset @@ result = query.member_snapshots(user_login="testuser", limit=5) @@ mock_user_cls.objects.get.assert_called_once_with(login="testuser") mock_queryset.filter.assert_called_once_with(github_user=mock_user) + mock_queryset.order_by.assert_called_once_with("-start_at") + mock_queryset.__getitem__.assert_called_once_with(slice(None, 5, None)) assert result == mock_snapshotsfrontend/src/app/board/[year]/candidates/layout.tsx (1)
4-7: Use generateMetadata to include dynamic year in title.Improves SEO and clarity for each year’s page.
-export const metadata: Metadata = { - title: 'Board Candidates | OWASP', - description: 'OWASP Board of Directors election candidates', -} +export async function generateMetadata( + { params }: { params: { year: string } } +): Promise<Metadata> { + const year = params.year + return { + title: `Board Candidates ${year} | OWASP`, + description: `OWASP Board of Directors election candidates for ${year}`, + } +}backend/apps/owasp/models/board_of_directors.py (1)
41-59: Order by explicit 'order' then name; prefetch/select related to avoid N+1.Leverage EntityMember.order for display priority and reduce DB hits when resolving members.
def candidates(self) -> QuerySet[EntityMember]: """Return all candidates for this board election.""" - return EntityMember.objects.filter( + return EntityMember.objects.filter( entity_type=ContentType.objects.get_for_model(self.__class__), entity_id=self.id, role=EntityMember.Role.CANDIDATE, is_active=True, is_reviewed=True, - ).order_by("member_name") + ).select_related("member").order_by("order", "member_name") @@ def members(self) -> QuerySet[EntityMember]: """Return all members of this board.""" - return EntityMember.objects.filter( + return EntityMember.objects.filter( entity_type=ContentType.objects.get_for_model(self.__class__), entity_id=self.id, role=EntityMember.Role.MEMBER, is_active=True, is_reviewed=True, - ).order_by("member_name") + ).select_related("member").order_by("order", "member_name")backend/tests/apps/github/models/user_projects_test.py (1)
1-90: Consider testing alphabetical ordering.The implementation orders projects by name (line 133 in user.py), but there's no test validating this behavior. The
user_chapters_test.pyfile includestest_chapters_ordered_alphabeticallywhich could be mirrored here.Add a test to verify ordering:
def test_projects_ordered_alphabetically(self): user = User(id=1, login="testuser", node_id="U_test123") mock_member_qs = Mock() mock_member_qs.values_list.return_value = [] mock_project_qs = Mock() with ( patch("django.contrib.contenttypes.models.ContentType.objects.get_for_model"), patch("apps.owasp.models.entity_member.EntityMember.objects.filter") as mock_em_filter, patch("apps.owasp.models.project.Project.objects.filter") as mock_proj_filter, patch("apps.owasp.models.entity_member.EntityMember.Role") as mock_role, ): mock_em_filter.return_value = mock_member_qs mock_proj_filter.return_value = mock_project_qs mock_role.LEADER = "leader" _ = user.projects mock_project_qs.order_by.assert_called_once_with("name")backend/apps/github/api/internal/nodes/user.py (1)
66-71: Improve defensive guard for attribute access.The current guard checks
hasattr(self, "owasp_profile")but then directly accessesself.owasp_profile.first_contribution_atwithout verifying the attribute exists onowasp_profile. This could raiseAttributeErrorif the profile exists but lacks thefirst_contribution_atfield.Apply this diff for more robust attribute access:
@strawberry.field def first_owasp_contribution_at(self) -> float | None: """Resolve first OWASP contribution date.""" - if hasattr(self, "owasp_profile") and self.owasp_profile.first_contribution_at: - return self.owasp_profile.first_contribution_at.timestamp() + if ( + hasattr(self, "owasp_profile") + and hasattr(self.owasp_profile, "first_contribution_at") + and self.owasp_profile.first_contribution_at + ): + return self.owasp_profile.first_contribution_at.timestamp() return NoneAlternatively, use
getattrwith a default:@strawberry.field def first_owasp_contribution_at(self) -> float | None: """Resolve first OWASP contribution date.""" - if hasattr(self, "owasp_profile") and self.owasp_profile.first_contribution_at: - return self.owasp_profile.first_contribution_at.timestamp() + owasp_profile = getattr(self, "owasp_profile", None) + first_contribution = getattr(owasp_profile, "first_contribution_at", None) if owasp_profile else None + if first_contribution: + return first_contribution.timestamp() return Nonebackend/apps/owasp/migrations/0059_memberprofile_first_contribution_at.py (1)
12-21: LGTM. Consider indexing if queried later.Field shape is correct. If you’ll filter/order by first_contribution_at, add db_index=True.
backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
26-34: Return types vs QuerySet.Resolvers return QuerySet, annotated as list[EntityMemberNode]. Strawberry can iterate a QuerySet, but to satisfy typing remove the type ignore or cast to list to avoid confusion.
- return self.candidates() # type: ignore[call-arg] + return list(self.candidates())Similarly for members().
frontend/src/app/board/[year]/candidates/page.tsx (4)
126-153: Sequential GraphQL requests. Make concurrent.Fetch chapters with Promise.all to reduce latency.
- const chapters: Chapter[] = [] - for (const key of chapterKeys) { - try { - const { data } = await client.query({ - query: GetChapterByKeyDocument, - variables: { key: key.replace('www-chapter-', '') }, - }) - if (data?.chapter) { - chapters.push(data.chapter) - } - } catch { - // Silently skip chapters that fail to fetch - } - } - setLedChapters(chapters.sort((a, b) => a.name.localeCompare(b.name))) + const results = await Promise.allSettled( + chapterKeys.map((key) => + client.query({ + query: GetChapterByKeyDocument, + variables: { key: key.replace(/^www-chapter-/, '') }, + }) + ) + ) + const chapters = results + .filter((r): r is PromiseFulfilledResult<any> => r.status === 'fulfilled') + .map((r) => r.value.data?.chapter) + .filter(Boolean) as Chapter[] + setLedChapters(chapters.sort((a, b) => a.name.localeCompare(b.name)))
155-183: Do the same for projects.- const projects: Project[] = [] - for (const key of projectKeys) { - try { - const { data } = await client.query({ - query: GetProjectByKeyDocument, - variables: { key: key.replace('www-project-', '') }, - }) - if (data?.project) { - projects.push(data.project) - } - } catch { - // Silently skip projects that fail to fetch - } - } - setLedProjects(projects.sort((a, b) => a.name.localeCompare(b.name))) + const results = await Promise.allSettled( + projectKeys.map((key) => + client.query({ + query: GetProjectByKeyDocument, + variables: { key: key.replace(/^www-project-/, '') }, + }) + ) + ) + const projects = results + .filter((r): r is PromiseFulfilledResult<any> => r.status === 'fulfilled') + .map((r) => r.value.data?.project) + .filter(Boolean) as Project[] + setLedProjects(projects.sort((a, b) => a.name.localeCompare(b.name)))
91-99: Use Apollo’s loading instead of duplicating isLoading.Simplify by using loading from useQuery, removing local isLoading state.
- const [isLoading, setIsLoading] = useState(true) - const { data: graphQLData, error: graphQLRequestError } = useQuery(GetBoardCandidatesDocument, { + const { data: graphQLData, error: graphQLRequestError, loading } = useQuery(GetBoardCandidatesDocument, { variables: { year: parseInt(year) }, }) - useEffect(() => { - if (graphQLData?.boardOfDirectors) { - setCandidates(graphQLData.boardOfDirectors.candidates || []) - setBoardUrl(graphQLData.boardOfDirectors.owaspUrl || '') - setIsLoading(false) - } - if (graphQLRequestError) { - handleAppError(graphQLRequestError) - setIsLoading(false) - } - }, [graphQLData, graphQLRequestError]) + useEffect(() => { + if (graphQLData?.boardOfDirectors) { + setCandidates(graphQLData.boardOfDirectors.candidates || []) + setBoardUrl(graphQLData.boardOfDirectors.owaspUrl || '') + } + if (graphQLRequestError) handleAppError(graphQLRequestError) + }, [graphQLData, graphQLRequestError]) - if (isLoading) { + if (loading) { return <LoadingSpinner /> }Also applies to: 383-395
2-2: Import path for Apollo hooks.Prefer importing from '@apollo/client' to avoid subpath coupling.
-import { useQuery, useApolloClient } from '@apollo/client/react' +import { useQuery, useApolloClient } from '@apollo/client'Also applies to: 108-108
frontend/src/components/ContributionHeatmap.tsx (1)
226-241: Optional: force re-render on theme toggle.ApexCharts sometimes needs a key to fully apply theme changes.
- <Chart options={options} series={heatmapSeries} type="heatmap" height={150} /> + <Chart key={`heatmap-${isDarkMode ? 'dark' : 'light'}`} options={options} series={heatmapSeries} type="heatmap" height={150} />backend/apps/owasp/api/internal/queries/member_snapshot.py (1)
42-43: Consider validating the limit parameter.The
limitparameter accepts any integer value without validation. Consider adding bounds checking to prevent performance issues from excessively large or negative values.Apply this diff to add validation:
@strawberry.field def member_snapshots( - self, user_login: str | None = None, limit: int = 10 + self, user_login: str | None = None, limit: int = 10 ) -> list[MemberSnapshotNode]: """Resolve member snapshots with optional filtering. Args: user_login: Optional filter by GitHub username - limit: Maximum number of snapshots to return (default: 10) + limit: Maximum number of snapshots to return (default: 10, max: 100) Returns: List of MemberSnapshotNode objects """ + # Validate and cap the limit + limit = max(1, min(limit, 100)) + query = MemberSnapshot.objects.all()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/boardQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (28)
backend/apps/github/admin/user.py(1 hunks)backend/apps/github/api/internal/nodes/user.py(3 hunks)backend/apps/github/management/commands/github_sync_user.py(7 hunks)backend/apps/github/models/user.py(3 hunks)backend/apps/owasp/admin/member_snapshot.py(2 hunks)backend/apps/owasp/api/internal/nodes/board_of_directors.py(1 hunks)backend/apps/owasp/api/internal/nodes/member_snapshot.py(1 hunks)backend/apps/owasp/api/internal/queries/__init__.py(2 hunks)backend/apps/owasp/api/internal/queries/board_of_directors.py(1 hunks)backend/apps/owasp/api/internal/queries/member_snapshot.py(1 hunks)backend/apps/owasp/management/commands/owasp_create_member_snapshot.py(3 hunks)backend/apps/owasp/migrations/0059_memberprofile_first_contribution_at.py(1 hunks)backend/apps/owasp/migrations/0060_membersnapshot_chapter_project_contributions.py(1 hunks)backend/apps/owasp/models/board_of_directors.py(2 hunks)backend/apps/owasp/models/member_profile.py(1 hunks)backend/apps/owasp/models/member_snapshot.py(1 hunks)backend/tests/apps/github/models/user_chapters_test.py(1 hunks)backend/tests/apps/github/models/user_projects_test.py(1 hunks)backend/tests/apps/owasp/admin/member_snapshot_test.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/board_of_directors_test.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/member_snapshot_test.py(1 hunks)backend/tests/apps/owasp/api/internal/queries/board_of_directors_test.py(1 hunks)backend/tests/apps/owasp/api/internal/queries/member_snapshot_test.py(1 hunks)frontend/jest.config.ts(1 hunks)frontend/src/app/board/[year]/candidates/layout.tsx(1 hunks)frontend/src/app/board/[year]/candidates/page.tsx(1 hunks)frontend/src/components/ContributionHeatmap.tsx(1 hunks)frontend/src/server/queries/boardQueries.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
backend/tests/apps/github/models/user_projects_test.py (1)
backend/apps/github/models/user.py (1)
projects(114-133)
backend/tests/apps/owasp/api/internal/queries/member_snapshot_test.py (2)
backend/apps/owasp/api/internal/queries/member_snapshot.py (3)
member_snapshot(15-39)MemberSnapshotQuery(11-64)member_snapshots(42-64)backend/apps/github/models/user.py (1)
User(28-212)
backend/apps/owasp/api/internal/queries/member_snapshot.py (3)
backend/apps/github/models/user.py (1)
User(28-212)backend/apps/owasp/api/internal/nodes/member_snapshot.py (2)
MemberSnapshotNode(20-46)github_user(24-26)backend/apps/owasp/models/member_snapshot.py (1)
MemberSnapshot(11-100)
backend/apps/owasp/api/internal/nodes/member_snapshot.py (3)
backend/apps/github/api/internal/nodes/user.py (2)
UserNode(34-91)issues_count(74-76)backend/apps/owasp/api/internal/queries/member_snapshot.py (1)
member_snapshot(15-39)backend/apps/owasp/models/member_snapshot.py (5)
MemberSnapshot(11-100)commits_count(83-85)pull_requests_count(88-90)issues_count(93-95)total_contributions(98-100)
backend/tests/apps/owasp/api/internal/queries/board_of_directors_test.py (2)
backend/apps/owasp/api/internal/queries/board_of_directors.py (3)
board_of_directors(14-27)BoardOfDirectorsQuery(10-40)boards_of_directors(30-40)backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(16-59)
backend/apps/owasp/api/internal/queries/board_of_directors.py (3)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
BoardOfDirectorsNode(18-34)frontend/src/types/__generated__/graphql.ts (1)
BoardOfDirectorsNode(49-59)backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(16-59)
backend/apps/owasp/migrations/0059_memberprofile_first_contribution_at.py (1)
backend/apps/owasp/migrations/0060_membersnapshot_chapter_project_contributions.py (1)
Migration(6-32)
backend/apps/owasp/migrations/0060_membersnapshot_chapter_project_contributions.py (1)
backend/apps/owasp/migrations/0059_memberprofile_first_contribution_at.py (1)
Migration(6-22)
backend/apps/github/models/user.py (3)
backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/project.py (1)
Project(37-384)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/apps/owasp/models/board_of_directors.py (2)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (2)
candidates(27-29)members(32-34)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/tests/apps/github/models/user_chapters_test.py (1)
backend/apps/github/models/user.py (1)
chapters(92-111)
frontend/src/components/ContributionHeatmap.tsx (1)
frontend/src/app/layout.tsx (1)
dynamic(16-16)
backend/apps/owasp/api/internal/queries/__init__.py (2)
backend/apps/owasp/api/internal/queries/board_of_directors.py (2)
board_of_directors(14-27)BoardOfDirectorsQuery(10-40)backend/apps/owasp/api/internal/queries/member_snapshot.py (2)
member_snapshot(15-39)MemberSnapshotQuery(11-64)
backend/tests/apps/owasp/api/internal/nodes/member_snapshot_test.py (4)
backend/apps/owasp/api/internal/queries/member_snapshot.py (1)
member_snapshot(15-39)backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
MemberSnapshotNode(20-46)frontend/src/types/__generated__/graphql.ts (1)
MemberSnapshotNode(231-245)backend/tests/apps/owasp/api/internal/nodes/board_of_directors_test.py (1)
test_node_fields(7-17)
backend/apps/owasp/management/commands/owasp_create_member_snapshot.py (5)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/apps/github/models/user.py (4)
User(28-212)issues(67-74)projects(114-133)chapters(92-111)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/owasp/models/member_snapshot.py (1)
MemberSnapshot(11-100)backend/apps/github/models/generic_issue_model.py (1)
repository_id(70-77)
backend/tests/apps/owasp/api/internal/nodes/board_of_directors_test.py (3)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
BoardOfDirectorsNode(18-34)frontend/src/types/__generated__/graphql.ts (1)
BoardOfDirectorsNode(49-59)backend/tests/apps/owasp/api/internal/nodes/member_snapshot_test.py (1)
test_node_fields(9-26)
backend/apps/github/management/commands/github_sync_user.py (2)
backend/apps/owasp/models/member_profile.py (1)
MemberProfile(11-43)backend/apps/github/models/user.py (2)
User(28-212)bulk_save(163-165)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (3)
frontend/src/types/__generated__/graphql.ts (3)
EntityMemberNode(138-150)BoardOfDirectorsNode(49-59)Node(344-347)backend/apps/owasp/api/internal/queries/board_of_directors.py (1)
board_of_directors(14-27)backend/apps/owasp/models/board_of_directors.py (4)
BoardOfDirectors(16-59)owasp_url(32-39)candidates(41-49)members(51-59)
backend/apps/github/api/internal/nodes/user.py (3)
backend/apps/github/models/user.py (1)
User(28-212)frontend/src/types/__generated__/graphql.ts (1)
BadgeNode(40-47)backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)
frontend/src/app/board/[year]/candidates/page.tsx (3)
backend/apps/owasp/models/member_snapshot.py (1)
MemberSnapshot(11-100)frontend/src/types/__generated__/boardQueries.generated.ts (4)
GetBoardCandidatesDocument(33-33)GetMemberSnapshotDocument(34-34)GetChapterByKeyDocument(35-35)GetProjectByKeyDocument(36-36)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
⏰ 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 backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (16)
backend/tests/apps/owasp/admin/member_snapshot_test.py (1)
56-57: LGTM; admin readonly fields updated.Tests correctly reflect new
chapter_contributionsandproject_contributionsexposure.backend/apps/owasp/management/commands/owasp_create_member_snapshot.py (1)
131-139: ****The review comment is based on a misunderstanding of Python
@propertysemantics. GenericIssueModel.repository_id is decorated with@property(not a regular method), which means accessingpr.repository_idautomatically invokes the getter and returns the actual integer value (self.repository.id), not a bound method object. Similarly, Commit'srepositoryfield is a Django ForeignKey, which providescommit.repository_idas an integer attribute automatically. The code at lines 131-139 correctly stores integers in therepository_idsset, and all subsequent lookups usingrepo_to_entity.get(pr.repository_id)work as intended with integer keys. No fix is required; the original code is correct.Likely an incorrect or invalid review comment.
frontend/jest.config.ts (1)
9-10: Remove or clarify exclusion for non-existent directory.The pattern
'!src/app/board/**'excludes a directory that doesn't exist in the codebase and has no associated files. If this is preparatory for an upcoming feature, add a TODO comment with ticket reference and timeline. Otherwise, remove the exclusion to keep coverage patterns aligned with actual code structure.backend/apps/owasp/api/internal/queries/__init__.py (1)
3-3: Verification complete: no duplicate field names across composed query mixins.All 11 query mixins (BoardOfDirectorsQuery, ChapterQuery, CommitteeQuery, EventQuery, MemberSnapshotQuery, PostQuery, ProjectHealthMetricsQuery, ProjectQuery, SnapshotQuery, SponsorQuery, StatsQuery) define 23 unique field names with no collisions. The composition in OwaspQuery is safe.
backend/apps/owasp/migrations/0060_membersnapshot_chapter_project_contributions.py (1)
1-32: LGTM!The migration correctly adds two JSONField columns with appropriate metadata and default values. The dependency chain is correct, and using
default=dict(the dict constructor as a callable) is the proper pattern for mutable defaults in Django.backend/tests/apps/owasp/api/internal/queries/board_of_directors_test.py (1)
1-76: LGTM!The tests comprehensively cover both success and error cases for BoardOfDirectors queries, including single lookups, not-found scenarios, and list queries with default and custom limits. The mocking strategy appropriately verifies ORM call sequences and return values.
backend/tests/apps/github/models/user_projects_test.py (1)
51-70: Good test coverage for filter conditions.This test correctly verifies both
is_activeandis_reviewedfilters, which is more thorough than the corresponding test inuser_chapters_test.py.backend/apps/github/api/internal/nodes/user.py (1)
43-45: LGTM!The lazy loading pattern with
strawberry.lazyandTYPE_CHECKINGforward reference correctly addresses circular import issues while maintaining proper type hints.frontend/src/app/board/[year]/candidates/page.tsx (1)
26-27: The review comment is incorrect; formatDate is being used correctly.The
formatDatefunction accepts Unix timestamps in seconds and internally multiplies by 1000 to create a Date object (see the comment in the implementation: "Unix timestamp in seconds"). The code correctly passes raw seconds toformatDate()without multiplication, just as it correctly multiplies by 1000 fordayjs().fromNow(). No changes are needed.Likely an incorrect or invalid review comment.
frontend/src/components/ContributionHeatmap.tsx (1)
34-46: No changes needed; timezone configuration is consistent.The review comment warns about potential timezone mismatches between frontend and backend date key generation. However, verification shows this concern does not apply:
- Django is configured with
TIME_ZONE = "UTC"andUSE_TZ = True, meaning all DateTimeField objects (includingcreated_atin Commit, PullRequest, and Issue models) are timezone-aware and stored in UTC.- The backend generates keys using
.date().isoformat()on UTC datetime objects, producing YYYY-MM-DD format in UTC.- The frontend generates keys using
.toISOString().split('T')[0], which also produces YYYY-MM-DD format in UTC.Both the frontend and backend are already using UTC consistently, so there is no off-by-one boundary issue around timezone conversions.
Likely an incorrect or invalid review comment.
frontend/src/server/queries/boardQueries.ts (1)
1-70: LGTM! Well-structured GraphQL queries.The query definitions are clean, properly typed, and follow GraphQL best practices. The nested field selections are appropriate for the use cases (board candidates, member snapshots, chapters, and projects).
backend/apps/owasp/api/internal/queries/member_snapshot.py (1)
14-39: LGTM! Proper error handling and query logic.The resolver correctly handles non-existent users by returning
None, and the filtering/ordering logic is sound. The optionalstart_yearfilter provides useful flexibility.backend/apps/github/management/commands/github_sync_user.py (4)
17-17: LGTM! Clean additions for skip-sync feature.The
MemberProfileimport and--skip-syncCLI argument are properly implemented and well-documented.Also applies to: 49-53
188-204: LGTM! Skip-sync flow is correctly implemented.The skip-sync option properly short-circuits the full sync and only executes the first contribution population logic.
414-423: Excellent deduplication implementation.The committer deduplication by
node_idis a smart improvement that prevents attempting to save duplicate users. Using a dictionary withnode_idas the key is the correct approach.
463-464: LGTM! Ensures first contribution is always populated.Calling
populate_first_contribution_onlyafter a successful sync ensures the field is populated. The method's internal check prevents unnecessary re-computation if already set.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
backend/apps/owasp/api/internal/queries/board_of_directors.py (1)
29-40: The previous review comment about limit validation remains valid.The
limitparameter lacks validation and could accept negative values (causing unexpected slicing behavior) or excessively large values (causing performance issues). The previous suggestion to add validation is appropriate and should be addressed.Apply validation as suggested in the previous review:
@strawberry.field def boards_of_directors(self, limit: int = 10) -> list[BoardOfDirectorsNode]: - """Resolve multiple Board of Directors instances. + """Resolve multiple Board of Directors instances ordered by year (most recent first). Args: - limit: Maximum number of boards to return. + limit: Maximum number of boards to return (1-100, default: 10). Returns: List of BoardOfDirectorsNode objects. """ + # Validate and constrain limit + if limit < 1: + limit = 10 + elif limit > 100: + limit = 100 + return BoardOfDirectors.objects.order_by("-year")[:limit]backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
23-46: N+1 query issue persists from previous review.Each resolver delegates to model
@propertymethods that call.count()on M2M relationships (commits, pull_requests, issues). When resolving multiple MemberSnapshot nodes, this triggers one COUNT query per field per node, compounded by total_contributions which sums three counts.The solutions outlined in the previous review remain valid:
- Pre-compute and store counts in denormalized integer fields
- Use
prefetch_relatedwithPrefetchandCountannotations- Implement a dataloader to batch count queries
frontend/src/app/board/[year]/candidates/page.tsx (3)
113-118: Acknowledged: Missing startYear parameter already flagged.This issue was identified in a previous review. The query should include
startYear: parseInt(year)in variables to filter snapshot data by the election year.
272-274: Use nullish coalescing (??) instead of logical OR (||) for contribution counts.The
||operator treats0as falsy, so if a candidate has exactly 0 contributions, the code will incorrectly fall through to the next value or default. Since 0 is a valid contribution count, use??to only check for null/undefined.Apply this diff:
const directKey = snapshot?.chapterContributions?.[bareKey] const withPrefix = snapshot?.chapterContributions?.[`www-chapter-${bareKey}`] - const contributionCount = directKey || withPrefix || 0 + const contributionCount = directKey ?? withPrefix ?? 0
298-300: Use nullish coalescing (??) instead of logical OR (||) for contribution counts.Same issue as with chapters: the
||operator treats0as falsy, which would incorrectly skip valid zero contribution counts.Apply this diff:
const directKey = snapshot?.projectContributions?.[bareKey] const withPrefix = snapshot?.projectContributions?.[`www-project-${bareKey}`] - const contributionCount = directKey || withPrefix || 0 + const contributionCount = directKey ?? withPrefix ?? 0
🧹 Nitpick comments (5)
backend/tests/apps/owasp/models/board_of_directors_test.py (1)
23-33: Consider adding integration tests for filtering and ordering behavior.The current tests only verify that the methods exist and are callable. Consider adding tests that create EntityMember instances and verify the actual filtering logic (is_active=True, is_reviewed=True) and ordering (by member_name).
For example:
def test_get_candidates_filters_and_orders_correctly(self, db): from django.contrib.contenttypes.models import ContentType from apps.owasp.models.entity_member import EntityMember board = BoardOfDirectors.objects.create(year=2025) content_type = ContentType.objects.get_for_model(BoardOfDirectors) # Create candidates with different states EntityMember.objects.create( entity_type=content_type, entity_id=board.id, member_name="Zoe", role=EntityMember.Role.CANDIDATE, is_active=True, is_reviewed=True, ) EntityMember.objects.create( entity_type=content_type, entity_id=board.id, member_name="Alice", role=EntityMember.Role.CANDIDATE, is_active=True, is_reviewed=True, ) # This should be filtered out EntityMember.objects.create( entity_type=content_type, entity_id=board.id, member_name="Bob", role=EntityMember.Role.CANDIDATE, is_active=False, is_reviewed=True, ) candidates = board.get_candidates() assert candidates.count() == 2 assert list(candidates.values_list("member_name", flat=True)) == ["Alice", "Zoe"]backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
31-34: Property access works but naming could be clearer.While
return self.owasp_urlcorrectly accesses the model's property (sinceselfis the BoardOfDirectors instance in Strawberry Django), the resolver method having the same name as the property can be confusing to readers.Consider accessing the property more explicitly to improve clarity:
@strawberry.field def owasp_url(self) -> str: """Resolve OWASP board election URL.""" - return self.owasp_url + # Access the model property explicitly + board: BoardOfDirectors = self # type: ignore[assignment] + return board.owasp_urlOr simply add a clarifying comment:
@strawberry.field def owasp_url(self) -> str: """Resolve OWASP board election URL.""" + # self is the BoardOfDirectors model instance return self.owasp_urlfrontend/src/app/board/[year]/candidates/page.tsx (1)
127-182: Consider batching chapter and project queries to avoid N+1 problem.Both
fetchChaptersandfetchProjectsiterate over keys and issue one GraphQL query per item, which can cause performance issues when candidates have many chapters or projects.Consider these approaches:
- Add a batch query that accepts multiple keys (if the backend supports it)
- Use Apollo Client's
@apollo/clientbatching features- Fetch all chapters/projects upfront if the total count is reasonable
Example with parallel fetching (minor improvement):
const fetchChapters = async () => { if (!snapshot?.chapterContributions) return const chapterKeys = Object.keys(snapshot.chapterContributions) - const chapters: Chapter[] = [] - for (const key of chapterKeys) { - try { - const { data } = await client.query({ - query: GetChapterByKeyDocument, - variables: { key: key.replace('www-chapter-', '') }, - }) - - if (data?.chapter) { - chapters.push(data.chapter) - } - } catch { - // Silently skip chapters that fail to fetch - } - } + const chapterPromises = chapterKeys.map(async (key) => { + try { + const { data } = await client.query({ + query: GetChapterByKeyDocument, + variables: { key: key.replace('www-chapter-', '') }, + }) + return data?.chapter + } catch { + return null + } + }) + + const chapters = (await Promise.all(chapterPromises)).filter(Boolean) as Chapter[] setLedChapters(chapters.sort((a, b) => a.name.localeCompare(b.name))) }Apply the same pattern to
fetchProjects.backend/apps/github/models/user.py (1)
91-114: Prevent duplicate IDs and minor micro-optimizations in helper.
- Add distinct() to values_list to avoid accidental duplicate entity_ids.
- Compute ContentType once outside filter for clarity.
- Optional: annotate return type for helper.
Apply:
- def _get_led_entities(self, entity_model): + def _get_led_entities(self, entity_model) -> "QuerySet": @@ - leader_memberships = EntityMember.objects.filter( + ct = ContentType.objects.get_for_model(entity_model) + leader_memberships = EntityMember.objects.filter( member=self, - entity_type=ContentType.objects.get_for_model(entity_model), + entity_type=ct, role=EntityMember.Role.LEADER, is_active=True, is_reviewed=True, - ).values_list("entity_id", flat=True) + ).values_list("entity_id", flat=True).distinct()backend/apps/github/management/commands/github_sync_user.py (1)
201-206: Guard against unexpected None user (defensive).User.update_data should return a User, but if it returns None, populate_first_contribution_only will error.
Apply:
- if skip_sync: + if skip_sync: self.stdout.write("Skipping data sync...") - self.populate_first_contribution_only(username, user, gh) + if user is not None: + self.populate_first_contribution_only(username, user, gh) + else: + self.stderr.write(self.style.ERROR("Cannot proceed: user could not be created")) returnPlease confirm User.update_data cannot return None in practice for GitHub users (node_id always present). If guaranteed, this guard can be omitted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/boardQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (11)
backend/apps/github/api/internal/nodes/user.py(1 hunks)backend/apps/github/management/commands/github_sync_user.py(7 hunks)backend/apps/github/models/user.py(3 hunks)backend/apps/owasp/api/internal/nodes/board_of_directors.py(1 hunks)backend/apps/owasp/api/internal/nodes/member_snapshot.py(1 hunks)backend/apps/owasp/api/internal/queries/board_of_directors.py(1 hunks)backend/apps/owasp/models/board_of_directors.py(2 hunks)backend/apps/owasp/models/member_snapshot.py(1 hunks)backend/tests/apps/owasp/models/board_of_directors_test.py(1 hunks)frontend/src/app/board/[year]/candidates/page.tsx(1 hunks)frontend/src/server/queries/boardQueries.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/github/api/internal/nodes/user.py
- frontend/src/server/queries/boardQueries.ts
🧰 Additional context used
🧬 Code graph analysis (8)
backend/apps/owasp/models/board_of_directors.py (1)
backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/tests/apps/owasp/models/board_of_directors_test.py (1)
backend/apps/owasp/models/board_of_directors.py (3)
get_candidates(41-49)BoardOfDirectors(16-59)get_members(51-59)
backend/apps/owasp/api/internal/nodes/member_snapshot.py (3)
backend/apps/github/api/internal/nodes/user.py (2)
UserNode(28-83)issues_count(66-68)backend/apps/owasp/api/internal/queries/member_snapshot.py (1)
member_snapshot(15-39)backend/apps/owasp/models/member_snapshot.py (5)
MemberSnapshot(11-102)commits_count(85-87)issues_count(95-97)pull_requests_count(90-92)total_contributions(100-102)
backend/apps/owasp/api/internal/queries/board_of_directors.py (3)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
BoardOfDirectorsNode(18-34)frontend/src/types/__generated__/graphql.ts (1)
BoardOfDirectorsNode(49-59)backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(16-59)
backend/apps/github/management/commands/github_sync_user.py (2)
backend/apps/owasp/models/member_profile.py (1)
MemberProfile(11-43)backend/apps/github/models/user.py (2)
User(28-216)bulk_save(167-169)
frontend/src/app/board/[year]/candidates/page.tsx (5)
backend/apps/owasp/models/member_snapshot.py (1)
MemberSnapshot(11-102)frontend/src/types/__generated__/boardQueries.generated.ts (4)
GetBoardCandidatesDocument(33-33)GetMemberSnapshotDocument(34-34)GetChapterByKeyDocument(35-35)GetProjectByKeyDocument(36-36)frontend/src/app/global-error.tsx (2)
handleAppError(66-86)ErrorDisplay(28-51)frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
backend/apps/github/models/user.py (3)
backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/project.py (1)
Project(37-384)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (3)
frontend/src/types/__generated__/graphql.ts (3)
EntityMemberNode(138-150)BoardOfDirectorsNode(49-59)Node(344-347)backend/apps/owasp/api/internal/queries/board_of_directors.py (1)
board_of_directors(14-27)backend/apps/owasp/models/board_of_directors.py (4)
BoardOfDirectors(16-59)get_candidates(41-49)get_members(51-59)owasp_url(32-39)
⏰ 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 (14)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
21-29: Type ignores are acceptable here due to Strawberry Django's magic.The
# type: ignore[call-arg]comments are suppressing mypy warnings because the type checker doesn't fully understand Strawberry Django's automatic queryset-to-list conversion. This is a known limitation when using Strawberry Django.backend/apps/owasp/api/internal/queries/board_of_directors.py (1)
13-27: LGTM!The query correctly handles the DoesNotExist case by returning None, which is the appropriate GraphQL pattern for nullable return types.
backend/apps/owasp/models/board_of_directors.py (2)
3-12: Excellent use of modern Python typing patterns.The
from __future__ import annotationsand TYPE_CHECKING block are best practices that enable forward references and avoid runtime import costs. Well done!
41-59: Method implementations look good with appropriate filtering and ordering.The renamed methods follow Django conventions, and the added filters (
is_active=True,is_reviewed=True) along with ordering bymember_nameensure consistent, predictable results. The inlineContentType.objects.get_for_model()call is fine since Django caches ContentType lookups.Note: The added
is_activeandis_reviewedfilters change the method behavior to only return reviewed, active members. This appears intentional for the BoD election transparency feature, but ensure any existing code calling these methods (if any) expects this filtered behavior.backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
10-21: LGTM! Clean GraphQL node definition.The Strawberry Django type decorator correctly exposes the new contribution tracking fields (chapter_contributions, project_contributions) alongside existing fields. The node properly extends strawberry.relay.Node for Relay-compliant GraphQL.
backend/apps/owasp/models/member_snapshot.py (1)
22-39: LGTM! New contribution tracking fields properly configured.Both
chapter_contributionsandproject_contributionsJSONField additions are well-structured with:
- Consistent verbose names for admin UI
default=dictto prevent None-related TypeErrors- Clear help text documenting the key→count mapping structure
The previous issue regarding missing parameters on
project_contributionshas been resolved.frontend/src/app/board/[year]/candidates/page.tsx (2)
285-287: Acknowledged: Year is already dynamic.The past review comment flagged hardcoded "2025", but the current code correctly uses the dynamic
${year}variable. No changes needed.
310-312: Acknowledged: Year is already dynamic.Similar to the chapters section, the code correctly uses the dynamic
${year}variable rather than hardcoded "2025". No changes needed.backend/apps/github/models/user.py (3)
21-26: LGTM: typing-only imports keep runtime lean.Using TYPE_CHECKING with QuerySet/Chapter/Project is correct and avoids import-time coupling.
115-126: LGTM: chapters property via shared helper resolves past duplication.Clear, lazy import avoids cycles; ordering by name is fine.
127-137: LGTM: projects property mirrors chapters through the helper.Consistent behavior and ordering.
backend/apps/github/management/commands/github_sync_user.py (3)
17-17: LGTM: MemberProfile import.Required for first_contribution_at population.
49-53: LGTM: --skip-sync flag UX.Clear behavior; early‑exit is implemented below.
415-426: LGTM: dedupe committers by node_id before bulk save.Prevents redundant writes; logging is useful.
| commit_query = f"author:{username} " + " ".join(f"org:{org}" for org in org_names) | ||
| commit_query += " sort:author-date-asc" | ||
| try: |
There was a problem hiding this comment.
Remove invalid sort qualifiers from query strings; rely on API sort/order args.
GitHub search sorting is controlled by the sort/order parameters, not query qualifiers. Keep the API args and drop the in‑query “sort:...-asc” fragments to avoid unexpected filtering.
Apply:
- commit_query += " sort:author-date-asc"
+ # sorting handled by API args: sort="author-date", order="asc"
@@
- pr_query += " sort:created-asc"
+ # sorting handled by API args: sort="created", order="asc"
@@
- issue_query += " sort:created-asc"
+ # sorting handled by API args: sort="created", order="asc"Also applies to: 129-130, 143-144
🤖 Prompt for AI Agents
In backend/apps/github/management/commands/github_sync_user.py around lines
111-113 (and also apply same change at 129-130 and 143-144), remove the in-query
sort qualifier fragments (e.g., " sort:author-date-asc" or any "sort:..." added
into the search query string) and leave the query to only include author/org
parts; rely on the GitHub API's separate sort and order parameters passed to the
request instead of embedding sort directives in the query string.
| commits = gh.search_commits(query=commit_query, sort="author-date", order="asc") | ||
| if commits.totalCount > 0: | ||
| first_commit = next(iter(commits)) | ||
| earliest_dates.append( | ||
| (first_commit.commit.author.date, "commit", first_commit.repository.name) | ||
| ) | ||
| self.stdout.write( | ||
| f" Found earliest commit: {first_commit.commit.author.date} " | ||
| f"in {first_commit.repository.full_name}" | ||
| ) | ||
| except GithubException as e: |
There was a problem hiding this comment.
Normalize datetimes to aware UTC before comparison/persist.
PyGithub often returns naive UTC datetimes. Mixing naive/aware can raise errors during min() or emit Django warnings when saving. Normalize all dates to aware UTC.
Apply:
@@ def populate_first_contribution_only(self, username: str, user: User, gh):
+ def _to_aware_utc(dt):
+ if dt is None:
+ return None
+ return dt.replace(tzinfo=UTC) if dt.tzinfo is None else dt.astimezone(UTC)
@@
- earliest_dates.append(
- (first_commit.commit.author.date, "commit", first_commit.repository.name)
- )
+ earliest_dates.append(
+ (_to_aware_utc(first_commit.commit.author.date), "commit", first_commit.repository.full_name)
+ )
@@
- earliest_dates.append((first_pr.created_at, "PR", first_pr.repository.name))
+ earliest_dates.append((_to_aware_utc(first_pr.created_at), "PR", first_pr.repository.full_name))
@@
- earliest_dates.append(
- (first_issue.created_at, "issue", first_issue.repository.name)
- )
+ earliest_dates.append(
+ (_to_aware_utc(first_issue.created_at), "issue", first_issue.repository.full_name)
+ )Also applies to: 131-136, 145-152, 155-169
🤖 Prompt for AI Agents
In backend/apps/github/management/commands/github_sync_user.py around lines
114-124 (and similarly at 131-136, 145-152, 155-169), the code uses datetimes
returned by PyGithub (often naive UTC) directly; normalize each datetime to an
aware UTC datetime before any comparisons or persisting. Import Django's
timezone (or datetime.timezone) and for every date value check if date.tzinfo is
None — if so make it aware in UTC (or use timezone.make_aware(date,
timezone.utc)); otherwise convert it to UTC via date.astimezone(timezone.utc);
replace uses of the raw dates with these normalized aware UTC datetimes in
earliest_dates, logs, and wherever saved/compared.
| @strawberry.field | ||
| def github_user(self) -> UserNode: | ||
| """Resolve GitHub user.""" | ||
| return self.github_user |
There was a problem hiding this comment.
🧩 Analysis chain
Verify github_user is prefetched to avoid N+1 queries.
The resolver accesses the ForeignKey github_user directly. When resolving multiple MemberSnapshot nodes, this will trigger one query per node unless select_related("github_user") is applied in the query layer.
Run this script to check if the query layer optimizes github_user access:
🏁 Script executed:
#!/bin/bash
# Search for MemberSnapshot querysets and check for select_related optimization
rg -nP "MemberSnapshot\.objects" --type py -A5 | rg -E "(select_related|prefetch_related)" -B5 -A1Length of output: 396
🏁 Script executed:
#!/bin/bash
# Search for MemberSnapshot.objects usage and nearby context
rg -nP "MemberSnapshot\.objects" --type py -B2 -A10Length of output: 5061
🏁 Script executed:
#!/bin/bash
# Search for any select_related or prefetch_related calls in the codebase
rg -nP "(select_related|prefetch_related)\(" --type py -B3 -A1 | head -100Length of output: 7323
🏁 Script executed:
#!/bin/bash
# Search for files that import MemberSnapshot
rg -nP "from.*MemberSnapshot|import.*MemberSnapshot" --type pyLength of output: 1555
🏁 Script executed:
#!/bin/bash
# Look at the member_snapshot.py file to understand model structure
fd "member_snapshot\.py" --type fLength of output: 275
🏁 Script executed:
#!/bin/bash
# View the MemberSnapshot model to confirm github_user is a ForeignKey
cat backend/apps/owasp/models/member_snapshot.pyLength of output: 3253
🏁 Script executed:
#!/bin/bash
# Check how the query resolvers are invoked in the GraphQL schema
rg -nP "member_snapshots|get_member_snapshot" backend/apps/owasp/api/internal/queries/member_snapshot.py -B5 -A15Length of output: 619
Add select_related("github_user") to MemberSnapshot queries to prevent N+1 lookups.
The member_snapshots query resolver returns unoptimized querysets that don't prefetch the github_user ForeignKey. When resolving multiple MemberSnapshotNode objects (default limit: 10), each node's github_user resolver triggers a separate database query.
Apply select_related("github_user") to both query instances:
- Line 31:
MemberSnapshot.objects.filter(github_user=user) - Line 55:
MemberSnapshot.objects.all()
This pattern is already used in the admin layer (backend/apps/owasp/admin/member_snapshot.py, line 99) and in other API resolvers (e.g., backend/apps/owasp/api/internal/nodes/project.py, lines 80, 85, 90).
🤖 Prompt for AI Agents
In backend/apps/owasp/api/internal/nodes/member_snapshot.py around lines 28 to
31 and 52 to 56, the MemberSnapshot querysets do not include
select_related("github_user"), causing N+1 queries when resolving github_user
for multiple snapshots; update the two query expressions
(MemberSnapshot.objects.filter(github_user=user) at ~line 31 and
MemberSnapshot.objects.all() at ~line 55) to include
.select_related("github_user") so the related User is fetched in the same query
and remove the per-node DB lookups.
| const { data: graphQLData, error: graphQLRequestError } = useQuery(GetBoardCandidatesDocument, { | ||
| variables: { year: Number.parseInt(year) }, | ||
| }) |
There was a problem hiding this comment.
Validate year parameter before parsing.
Number.parseInt(year) returns NaN if the year parameter is invalid, which would be passed to the GraphQL query and could cause unexpected behavior.
Apply this diff to add validation:
+ const parsedYear = Number.parseInt(year)
+
+ if (isNaN(parsedYear)) {
+ return (
+ <ErrorDisplay
+ statusCode={400}
+ title="Invalid Year"
+ message="Please provide a valid year"
+ />
+ )
+ }
+
const { data: graphQLData, error: graphQLRequestError } = useQuery(GetBoardCandidatesDocument, {
- variables: { year: Number.parseInt(year) },
+ variables: { year: parsedYear },
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/app/board/[year]/candidates/page.tsx around lines 91-93, the
code directly calls Number.parseInt(year) which may produce NaN for invalid
input; validate the year param first (e.g., ensure it's a string of digits or
matches /^\d{4}$/), parse with parseInt(year, 10), and check
Number.isInteger(parsedYear) and !Number.isNaN(parsedYear) before passing it to
useQuery. If validation fails, avoid invoking the GraphQL query (pass
undefined/null variables or skip the query), and handle the invalid route by
returning an appropriate fallback (error UI, redirect, or 400 response) so NaN
is never sent to the backend.
2 participants
Proposed change
BoD election transparency report (FE)
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.