Skip to content

Conversation

@mrkeshav-05
Copy link
Contributor

@mrkeshav-05 mrkeshav-05 commented Jan 11, 2026

Proposed change

This PR refactors the member profile page to reuse the existing ContributionHeatmap component for improved design consistency and code reuse across the platform.

Resolves #2705


image image

Checklist

  • Required: I followed the contributing workflow
  • Required: I verified that my code works as intended and resolves the issue as described
  • Required: I ran make check-test locally: all warnings addressed, tests passed
  • I used AI for code, documentation, tests, or communication related to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 11, 2026

Summary by CodeRabbit

  • New Features

    • Added a new "medium" contribution-heatmap size and support for per-user contribution data.
  • UI

    • Replaced canvas-based heatmap with a data-driven ContributionHeatmap that only displays when contribution data exists; layout adjusted for overflow and variants.
  • Backend / Data

    • Persisted per-day contribution data on users, added aggregation tooling to populate it, and included a migration to store the data.
  • Tests

    • Expanded tests and mocks to cover the new variant, data shapes, absence/presence scenarios, and aggregation command.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Replaces the inline Heatmap with the reusable ContributionHeatmap (adds a "medium" variant), persists per-user contribution_data via a migration and aggregation command, exposes contributionData in GraphQL and server queries, and updates frontend tests/mocks to use a data-testid-based heatmap rendering.

Changes

Cohort / File(s) Change Summary
Frontend — Member page
frontend/src/app/members/[memberKey]/page.tsx
Replace inline canvas Heatmap with ContributionHeatmap; derive contributionData from user.contributionData, compute dateRange, and conditionally render the new component (removed canvas/theme-based logic).
Frontend — ContributionHeatmap
frontend/src/components/ContributionHeatmap.tsx
Add medium variant to ContributionHeatmapProps (`'default'
Frontend — Tests & mocks
frontend/__tests__/unit/pages/UserDetails.test.tsx, frontend/__tests__/unit/components/ContributionHeatmap.test.tsx, frontend/__tests__/mockData/mockUserDetails.ts, frontend/__tests__/mockData/*
Add contributionData to mock user data; mock ContributionHeatmap (exports test-id contribution-heatmap) and remove direct drawContributions/fetchHeatmapData mocks; add tests for medium variant and presence/absence rendering.
Frontend — Utils export change
frontend/src/utils/helpers/githubHeatmap
Remove public export drawContributions, keep fetchHeatmapData; tests updated to stop depending on drawContributions.
Frontend — GraphQL query
frontend/src/server/queries/userQueries.ts
Add contributionData field to GetUserData GraphQL selection returned to frontend.
Backend — Model & migration
backend/apps/github/models/user.py, backend/apps/github/migrations/0041_user_contribution_data.py
Add contribution_data JSONField to User model (blank, default dict) and include migration to persist the new field.
Backend — Aggregation command & Makefiles
backend/apps/github/management/commands/github_aggregate_contributions.py, backend/apps/github/Makefile, backend/Makefile
New management command github_aggregate_contributions (aggregates per-user date→count map into user.contribution_data), added Makefile target and wired into top-level update-data.
Backend — GraphQL node
backend/apps/github/api/internal/nodes/user.py
Expose contribution_data in UserNode strawberry type fields so GraphQL can return stored contribution data.
Backend — Tests
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py, backend/tests/apps/github/api/internal/nodes/user_test.py, backend/tests/apps/github/models/user_test.py
Add tests for aggregation command behavior (date bucketing, user handling, format), GraphQL field presence, and contribution_data model defaults/storage/update.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • arkid15r
  • kasya
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring: replacing an inline Heatmap component with the reusable ContributionHeatmap component on the member page for design consistency.
Description check ✅ Passed The description clearly relates to the changeset, explaining the refactoring to reuse ContributionHeatmap and linking to issue #2705.
Linked Issues check ✅ Passed The PR fully meets issue #2705 objectives: replaces inline Heatmap with reusable ContributionHeatmap on members page, aligns API data props (contributionData field), consolidates logic, and ensures visual consistency.
Out of Scope Changes check ✅ Passed All changes directly support the stated objective of making ContributionHeatmap reusable and consolidating heatmap functionality. Backend aggregation command and GraphQL exposure enable frontend data flow; no unrelated changes detected.
Docstring Coverage ✅ Passed Docstring coverage is 95.83% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e7a27ed and 0278be4.

📒 Files selected for processing (1)
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/tests/unit/pages/UserDetails.test.tsx

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)

61-68: Consider accepting props in the mock for better test coverage.

The mock ContributionHeatmap component currently ignores all props. While this simplifies testing, it means tests cannot verify that correct data (contributionData, startDate, endDate, variant) is passed to the component.

💡 Optional enhancement to verify props
 jest.mock('components/ContributionHeatmap', () => {
-  const MockContributionHeatmap = () => <div data-testid="contribution-heatmap">Heatmap</div>
+  const MockContributionHeatmap = ({ contributionData, startDate, endDate, variant }: any) => (
+    <div 
+      data-testid="contribution-heatmap"
+      data-start-date={startDate}
+      data-end-date={endDate}
+      data-variant={variant}
+      data-contribution-count={Object.keys(contributionData || {}).length}
+    >
+      Heatmap
+    </div>
+  )
   MockContributionHeatmap.displayName = 'MockContributionHeatmap'
   return {
     __esModule: true,
     default: MockContributionHeatmap,
   }
 })

This would allow tests to assert on the props passed to the component, e.g., expect(heatmap).toHaveAttribute('data-variant', 'medium').


81-87: Mock data includes unused fields.

The mock data includes a years array and intensity field on contributions, but the actual page component (lines 56-69 in page.tsx) only uses the contributions array and specifically the date and count fields. The years and intensity fields appear to be unused.

Consider simplifying the mock to only include fields that are actually used:

 ;(fetchHeatmapData as jest.Mock).mockResolvedValue({
-  years: [{ year: '2023' }],
   contributions: [
     { date: '2023-01-01', count: 5 },
-    { date: '2023-01-02', count: 3, intensity: '1' },
+    { date: '2023-01-02', count: 3 },
   ],
 })
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 385e6e3 and 77158bc.

📒 Files selected for processing (3)
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
  • frontend/src/app/members/[memberKey]/page.tsx
  • frontend/src/components/ContributionHeatmap.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.

Applied to files:

  • frontend/src/app/members/[memberKey]/page.tsx
🔇 Additional comments (7)
frontend/src/components/ContributionHeatmap.tsx (3)

252-252: LGTM!

Clean addition of the 'medium' variant to the type union. The variant remains optional with a sensible default.


276-294: LGTM!

The medium variant width calculation follows a logical progression:

  • Compact: 13.4px/week, 400px min
  • Medium: 15.5px/week, 500px min
  • Default: 19.5px/week, 600px min

The dependency array is correctly updated to use variant instead of the removed isCompact variable.


325-325: LGTM!

Height calculation correctly implements the three-tier sizing:

  • Compact: 150px
  • Medium: 172px
  • Default: 195px

The chained ternary is appropriate here for straightforward value selection.

frontend/src/app/members/[memberKey]/page.tsx (4)

6-6: LGTM!

The refactored state management is cleaner and aligns well with the ContributionHeatmap API:

  • contributionData as a date-to-count map
  • Separate dateRange for start/end dates
  • Removal of useTheme and useRef simplifies the component

Also applies to: 16-16, 21-26


56-70: Data transformation logic is sound.

The implementation correctly:

  • Validates the contributions array
  • Transforms to a date-count map
  • Derives date range from sorted dates (ISO format sort works correctly)

The conditional check on line 63 ensures the date range is only set when contributions exist, preventing edge case issues.


154-154: LGTM!

Adding overflow-x-auto enables horizontal scrolling for long usernames, multiple badges, and the heatmap on smaller screens. This improves responsive design.


179-194: LGTM!

The conditional rendering logic is thorough and correct:

  • Validates user is not a private contributor
  • Ensures contribution data exists and is non-empty
  • Verifies date range is valid before rendering

Props passed to ContributionHeatmap match the component's interface, and the "medium" variant is appropriate for this layout context.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 11, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

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

⚠️ Outside diff range comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)

49-73: Remove user from dependency array to prevent double-fetching.

The user variable is not used inside this useEffect, but including it in the dependency array causes fetchData to run twice: once on mount (when user is undefined) and again when the GraphQL query completes (when user becomes defined).

🐛 Proposed fix
-  }, [memberKey, user])
+  }, [memberKey])
🧹 Nitpick comments (1)
frontend/src/components/ContributionHeatmap.tsx (1)

325-325: Consider a lookup object for variant dimensions.

The nested ternary works but a lookup object would be more maintainable as variants grow:

♻️ Optional refactor
+const VARIANT_HEIGHT = { compact: 150, medium: 172, default: 195 } as const
+
 // Then in the component:
-            height={variant === 'compact' ? 150 : variant === 'medium' ? 172 : 195}
+            height={VARIANT_HEIGHT[variant]}
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77158bc and 6bba723.

📒 Files selected for processing (3)
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
  • frontend/src/app/members/[memberKey]/page.tsx
  • frontend/src/components/ContributionHeatmap.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/tests/unit/pages/UserDetails.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.

Applied to files:

  • frontend/src/app/members/[memberKey]/page.tsx
🔇 Additional comments (3)
frontend/src/components/ContributionHeatmap.tsx (1)

252-252: LGTM! Clean variant extension with consistent sizing.

The medium variant is well-positioned between compact and default with proportional width/height scaling. The dependency array correctly tracks both heatmapSeries and variant.

Also applies to: 276-294

frontend/src/app/members/[memberKey]/page.tsx (2)

56-70: LGTM! Data transformation is correctly implemented.

The transformation from array to Record<string, number> is clean, and the date range extraction with ISO string sorting works correctly. The empty array case is implicitly handled by the conditional checks at render time.


179-194: LGTM! Defensive rendering with proper guards.

All edge cases are handled: private contributor check, empty data check, and date range validation. The overflow-x-auto container ensures the heatmap scrolls gracefully on smaller viewports.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 11, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)

82-87: Verify if the years field in the mock is necessary.

The mock returns both years and contributions, but the page component (lines 56-70) only appears to use the contributions array. The years field might be unused mock data.

Consider verifying whether years is used elsewhere, and if not, simplify the mock:

♻️ Potential simplification if years is unused
 ;(fetchHeatmapData as jest.Mock).mockResolvedValue({
-  years: [{ year: '2023' }],
   contributions: [
     { date: '2023-01-01', count: 5, intensity: '2' },
     { date: '2023-01-02', count: 3, intensity: '1' },
   ],
 })
frontend/src/app/members/[memberKey]/page.tsx (1)

56-70: Verify browser support for Array.at() or consider a fallback.

Line 67 uses Array.at(-1) which is an ES2022 feature. While Next.js 16 and React 19 should support this, verify that your target browser matrix includes ES2022 support.

If broader browser support is needed, consider this alternative:

♻️ Alternative using traditional array indexing
 if (result.contributions.length > 0) {
   const dates = result.contributions.map((c) => c.date).sort((a, b) => a.localeCompare(b))
   setDateRange({
     startDate: dates[0],
-    endDate: dates.at(-1) ?? '',
+    endDate: dates[dates.length - 1] ?? '',
   })
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6bba723 and c91f4c6.

📒 Files selected for processing (3)
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
  • frontend/src/app/members/[memberKey]/page.tsx
  • frontend/src/components/ContributionHeatmap.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/components/ContributionHeatmap.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.

Applied to files:

  • frontend/src/app/members/[memberKey]/page.tsx
🔇 Additional comments (8)
frontend/__tests__/unit/pages/UserDetails.test.tsx (4)

8-8: LGTM!

The import aligns with the mock setup below and the updated page implementation.


61-68: LGTM!

The mock implementation is appropriately minimal for page-level testing. The data-testid allows tests to verify the component renders without testing its internal implementation details.


279-300: LGTM!

The test correctly verifies that the ContributionHeatmap component renders when valid data is available. The use of waitFor accounts for the asynchronous data fetching in the useEffect.


302-315: LGTM!

The test appropriately verifies that the heatmap doesn't render when fetchHeatmapData returns null. Using queryByTestId for negative assertions is the correct pattern.

frontend/src/app/members/[memberKey]/page.tsx (4)

6-6: LGTM!

All new imports are necessary for the refactored heatmap functionality and align with the PR objectives.

Also applies to: 13-13, 16-16


21-25: LGTM!

The state declarations are well-typed and appropriately initialized. Empty defaults work correctly with the conditional rendering logic below.


154-154: LGTM!

Adding overflow-x-auto improves mobile responsiveness by allowing horizontal scrolling when the heatmap is wider than the viewport. This prevents layout breaking on smaller screens.


179-194: Verify the variant="medium" prop value.

The code passes variant="medium" to ContributionHeatmap. According to the AI summary, the component now supports 'default' | 'medium' | 'compact' variants, so this should be valid. However, since the ContributionHeatmap component file isn't included in this review, please verify that "medium" is correctly implemented and produces the desired visual result.

The conditional rendering logic is excellent—multiple checks ensure the component only renders with complete, valid data.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 11, 2026
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

It looks like it still uses the 3rd party data fetching instead of our local data. The idea is to get rid of both external dependency and components code related to that. We want it to fetch our DB for user contributions data. It means it also needs to be update to day on a daily basis.

@arkid15r arkid15r marked this pull request as draft January 11, 2026 17:42
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @backend/apps/github/management/commands/github_aggregate_contributions.py:
- Around line 34-45: The default for batch size is inconsistent: the
parser.add_argument("--batch-size") sets default=100 but in handle(...) the code
reads options.get("batch_size", 1000); change one to match the other. Either
update parser.add_argument to default=1000 or change options.get("batch_size",
100) in the github_aggregate_contributions.Command.handle method so that the
parser default and the handle fallback use the same value, and ensure the
variable name batch_size remains used consistently.
- Around line 102-113: The current Commit queryset groups by the full datetime
because it uses .values("created_at").annotate(count=Count("id")), so change it
to truncate created_at to date at DB level (use TruncDate on Commit.created_at
in the queryset and .values("date")/.annotate(count=Count("id"))), then use that
truncated date when building date_str; apply the same TruncDate pattern to the
prs and issues queries so all three aggregate by date instead of full datetime
for correct and efficient grouping.

In
@backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py:
- Around line 286-293: The loop verifying YYYY-MM-DD treats the datetime as
timezone-aware by calling datetime.strptime(date_str,
"%Y-%m-%d").astimezone(UTC), which forces a local-TZ assumption on a naive
datetime; simply remove the .astimezone(UTC) call and validate the format with
datetime.strptime(date_str, "%Y-%m-%d") (or use
datetime.fromisoformat(date_str)) and keep the subsequent length and
dash-position assertions on result.
🧹 Nitpick comments (4)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

313-338: Tests verify mock behavior rather than actual field resolution.

These tests only verify that mock_user.contribution_data returns what was assigned to it, which tests the mock itself rather than the UserNode field resolution. Compare with other tests in this file (e.g., test_created_at_field, test_issues_count_field) that call UserNode.field_name(mock_user) to test actual resolver behavior.

Since contribution_data is a passthrough field (no custom resolver), this is acceptable but provides limited value. The real validation happens in the test_meta_configuration test above.

frontend/src/components/ContributionHeatmap.tsx (2)

273-294: LGTM! Consistent width calculation pattern.

The medium variant follows the established pattern with appropriate pixel-per-week (16) and padding (45) values that fall between compact and default. The minimum width of 500px is sensible.

One minor observation: consider extracting the magic numbers (pixelPerWeek, padding, minWidth) into a configuration object for each variant to improve maintainability.

♻️ Optional: Extract variant configuration
const VARIANT_CONFIG = {
  compact: { pixelPerWeek: 13.4, padding: 40, minWidth: 400 },
  medium: { pixelPerWeek: 16, padding: 45, minWidth: 500 },
  default: { pixelPerWeek: 19.5, padding: 50, minWidth: 600 },
} as const

// Then in useMemo:
const config = VARIANT_CONFIG[variant]
const calculatedWidth = weeksCount * config.pixelPerWeek + config.padding
return Math.max(config.minWidth, calculatedWidth)

298-302: Consider memoizing getChartHeight.

getChartHeight is defined as a function inside the component and will be recreated on every render. Since it only depends on variant, it could be memoized or converted to a simple lookup.

♻️ Optional: Convert to memoized value or constant lookup
-  const getChartHeight = () => {
-    if (variant === 'compact') return 150
-    if (variant === 'medium') return 172
-    return 195
-  }
+  const chartHeight = useMemo(() => {
+    const heights = { compact: 150, medium: 172, default: 195 }
+    return heights[variant]
+  }, [variant])

Then use chartHeight directly instead of getChartHeight() on line 331.

backend/apps/github/management/commands/github_aggregate_contributions.py (1)

66-76: Consider using bulk_update instead of individual saves for better performance.

Currently, each user is saved individually inside the loop, resulting in one DB write per user. For large datasets, using bulk_update in batches would significantly improve performance.

♻️ Proposed refactor using bulk_update
processed = 0
users_to_update = []

for user in users.iterator(chunk_size=batch_size):
    contribution_data = self._aggregate_user_contributions(user, start_date)

    if contribution_data:
        user.contribution_data = contribution_data
        users_to_update.append(user)
        processed += 1

        if len(users_to_update) >= batch_size:
            User.objects.bulk_update(users_to_update, ["contribution_data"])
            users_to_update = []
            self.stdout.write(f"Processed {processed}/{total_users} users...")

# Save remaining users
if users_to_update:
    User.objects.bulk_update(users_to_update, ["contribution_data"])
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c91f4c6 and 7953acb.

⛔ Files ignored due to path filters (2)
  • frontend/src/types/__generated__/graphql.ts is excluded by !**/__generated__/**
  • frontend/src/types/__generated__/userQueries.generated.ts is excluded by !**/__generated__/**
📒 Files selected for processing (15)
  • backend/Makefile
  • backend/apps/github/Makefile
  • backend/apps/github/api/internal/nodes/user.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/apps/github/migrations/0041_user_contribution_data.py
  • backend/apps/github/models/user.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/models/user_test.py
  • frontend/__tests__/mockData/mockUserDetails.ts
  • frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
  • frontend/src/app/members/[memberKey]/page.tsx
  • frontend/src/components/ContributionHeatmap.tsx
  • frontend/src/server/queries/userQueries.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/app/members/[memberKey]/page.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.

Applied to files:

  • backend/apps/github/migrations/0041_user_contribution_data.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/models/user_test.py
  • backend/apps/github/api/internal/nodes/user.py
  • backend/apps/github/models/user.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.

Applied to files:

  • backend/apps/github/migrations/0041_user_contribution_data.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/models/user_test.py
  • backend/apps/github/api/internal/nodes/user.py
  • backend/apps/github/models/user.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.

Applied to files:

  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
🧬 Code graph analysis (3)
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (1)
backend/apps/github/management/commands/github_aggregate_contributions.py (2)
  • _aggregate_user_contributions (82-141)
  • handle (41-80)
backend/tests/apps/github/models/user_test.py (1)
backend/apps/github/models/user.py (1)
  • User (28-223)
backend/apps/github/management/commands/github_aggregate_contributions.py (1)
backend/apps/github/models/commit.py (1)
  • Commit (11-110)
🔇 Additional comments (24)
frontend/src/server/queries/userQueries.ts (1)

88-88: LGTM!

The contributionData field is correctly added to the GraphQL query, enabling the frontend to fetch per-day contribution counts for the heatmap component.

backend/Makefile (1)

159-159: LGTM!

The github-aggregate-contributions target is correctly added after github-update-users, ensuring users exist before their contributions are aggregated.

backend/apps/github/Makefile (1)

24-27: LGTM!

The new Makefile target follows the established pattern consistently with other targets in this file.

frontend/__tests__/mockData/mockUserDetails.ts (1)

16-22: LGTM!

The mock contributionData provides good test coverage with varied values including a zero-contribution day, which helps validate edge case handling in the heatmap component.

frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)

629-634: LGTM!

Good test coverage for the new medium variant, verifying the expected height of 172px which fits logically between default (195px) and compact (150px).


661-668: LGTM!

Consistent test coverage for the medium variant's container styling, following the same pattern as existing variant tests.


677-691: Test now uses medium variant for rerender scenario.

The test verifies that title styling remains consistent across variants. Switching from compact to medium in the rerender case still validates the intended behavior (title classes unchanged between variants).

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

26-26: LGTM!

The contribution_data field is correctly added to the expected field names set in alphabetical order, consistent with the existing test pattern.

backend/apps/github/models/user.py (1)

57-62: LGTM!

The contribution_data JSONField is correctly defined:

  • Uses default=dict (callable) to avoid the shared mutable default anti-pattern
  • blank=True without null=True is the correct pattern for JSONField—empty dict is preferable to NULL
  • Clear help text documents the expected date -> count mapping structure
backend/apps/github/migrations/0041_user_contribution_data.py (1)

1-22: LGTM!

The migration correctly adds the contribution_data JSONField with options matching the model definition. This is an additive, backward-compatible change that won't affect existing data.

frontend/__tests__/unit/pages/UserDetails.test.tsx (7)

8-8: LGTM!

Good cleanup—only importing fetchHeatmapData since drawContributions is no longer used directly in tests.


61-68: LGTM!

The mock correctly provides a data-testid for reliable querying in tests, following the pattern used by other mocked components in this file.


81-87: LGTM!

The mock return value now includes the contributions array with the expected shape (date, count, intensity), aligning with the heatmap data structure.


297-299: LGTM!

Switching from alt text querying to data-testid is more reliable and decouples tests from accessibility attributes that might change.


303-321: LGTM!

The test correctly verifies that when contributionData is null, the heatmap component is not rendered. This properly tests the conditional rendering logic in the page component.


827-886: Good test coverage for contribution data edge cases.

The new test suite covers the key scenarios:

  • Heatmap renders when data exists
  • Heatmap doesn't render with empty object {}
  • Heatmap doesn't render with null

This ensures the page handles various backend states gracefully.


843-863: No changes needed. The test correctly verifies conditional rendering behavior.

The conditional rendering check (hasContributionData && ...) is correctly implemented in the page component at line 166. When contributionData is {}, hasContributionData evaluates to false (since Object.keys({}).length > 0 is false), preventing the entire <ContributionHeatmap> component from rendering. The mock is never invoked, so the test correctly passes when the heatmap is not in the DOM.

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

16-16: JSONField requires explicit GraphQL type mapping in strawberry-django v0.73.0

Adding contribution_data to the fields list without an explicit scalar type will cause GraphQL serialization failures. Strawberry-django v0.73.0 does not automatically handle Django's JSONField—that support was added later in Strawberry core v0.99.0+.

To fix this, you need to either:

  • Import and use Strawberry's JSON scalar (from strawberry.scalars import JSON) and define the field explicitly, or
  • Create a custom resolver that returns the JSONField as a Python dict/list

Likely an incorrect or invalid review comment.

frontend/src/components/ContributionHeatmap.tsx (1)

252-252: LGTM! Clean variant extension.

The new 'medium' variant is appropriately added to the union type, maintaining type safety and allowing consumers to use the new size option.

backend/tests/apps/github/models/user_test.py (1)

219-248: LGTM! Good coverage of the new contribution_data field.

The tests appropriately verify:

  1. Default value is an empty dict
  2. JSON data can be stored and retrieved
  3. The field can be updated

These in-memory tests are suitable for unit testing the field definition. If integration tests exist elsewhere that verify actual database persistence, this coverage is complete.

backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (4)

9-42: LGTM! Useful MockQuerySet utility.

The MockQuerySet class provides a clean way to mock Django QuerySet behavior without database access. It supports the necessary methods (filter, exists, count, iterator, values, annotate) used by the command.


47-86: LGTM! Good test for empty contributions scenario.

The test properly mocks timezone.now() and verifies that 30 days of zero-filled data is returned when no contributions exist.


87-135: LGTM! Comprehensive test with contribution data.

The test verifies correct aggregation across commits, PRs, and issues on different dates. The assertions clearly document expected behavior (e.g., Jan 1st = 2 commits + 1 PR = 3).


166-183: LGTM! Good negative test for user not found.

The test properly verifies the error message is written when a specified user doesn't exist.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @backend/apps/github/management/commands/github_aggregate_contributions.py:
- Around line 117-141: The PR and Issue queries are grouping by the full
datetime; change both queries to use TruncDate("created_at") so aggregation
happens in the DB: in the PullRequest and Issue query blocks replace
.values("created_at").annotate(count=Count("id")) with
.annotate(date=TruncDate("created_at")).values("date").annotate(count=Count("id"))
(and update the loops to read pr["date"] / issue["date"] instead of
["created_at"]); also ensure TruncDate is imported from
django.db.models.functions if not already.
🧹 Nitpick comments (2)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

313-337: Tests don't exercise UserNode logic.

These tests only verify that a Mock object returns the value assigned to it, which is tautological. Unlike other tests in this file (e.g., test_url_field calls UserNode.url(mock_user)), these don't invoke any UserNode method.

If contribution_data is a simple passthrough field (auto-resolved by Strawberry from the model), explicit tests may be unnecessary. If there's a custom resolver, test it like other fields:

result = UserNode.contribution_data(mock_user)

Otherwise, consider removing these tests or clarifying their intent.

backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (1)

31-41: Remove unused noqa directives.

Per static analysis, the ARG002 rule is not enabled, making these noqa comments unnecessary.

🔧 Proposed fix
-    def iterator(self, chunk_size=None):  # noqa: ARG002
+    def iterator(self, chunk_size=None):
         """Mock iterator method."""
         return iter(self._items)

-    def values(self, *_args):  # noqa: ARG002
+    def values(self, *_args):
         """Mock values method."""
         return self
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7953acb and 3e6520d.

📒 Files selected for processing (3)
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.

Applied to files:

  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.

Applied to files:

  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.

Applied to files:

  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.

Applied to files:

  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
🧬 Code graph analysis (2)
backend/apps/github/management/commands/github_aggregate_contributions.py (2)
backend/apps/github/models/commit.py (1)
  • Commit (11-110)
backend/apps/github/models/user.py (2)
  • User (28-223)
  • issues (74-81)
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (1)
backend/apps/github/management/commands/github_aggregate_contributions.py (2)
  • _aggregate_user_contributions (83-143)
  • handle (42-81)
🪛 Ruff (0.14.11)
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py

31-31: Unused noqa directive (non-enabled: ARG002)

Remove unused noqa directive

(RUF100)


35-35: Unused noqa directive (non-enabled: ARG002)

Remove unused noqa directive

(RUF100)

🔇 Additional comments (11)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

26-26: LGTM!

The expected field set correctly includes the new contribution_data field, aligning with the model and GraphQL schema updates.

backend/apps/github/management/commands/github_aggregate_contributions.py (3)

1-21: LGTM!

Imports are appropriate. Using django.utils.timezone for timezone-aware datetime handling is correct, and TruncDate/Count are suitable for the aggregation queries.


22-40: LGTM!

Command arguments are well-structured with sensible defaults and clear help text.


42-81: LGTM!

Good use of users.exists() for existence checks and users.iterator(chunk_size=batch_size) for memory-efficient processing. The update_fields parameter in save() is a good practice for performance.

One minor note: Line 71's if contribution_data: will be True as long as the dict has keys (even if all counts are 0). This should work correctly since _aggregate_user_contributions initializes all dates in the range.

backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (7)

47-81: LGTM!

Test correctly validates the empty contribution case with proper mocking of timezone and model queries.


83-127: Test mirrors current implementation inconsistency.

The mock data for PRs and issues uses created_at (datetime) while commits use date. If you apply the suggested fix to use TruncDate for PRs and issues in the command, update these mocks accordingly:

             mock_pr.objects.filter.return_value = MockQuerySet(
-                [{"created_at": datetime(2024, 1, 1, 11, 0, tzinfo=UTC), "count": 1}]
+                [{"date": datetime(2024, 1, 1, tzinfo=UTC).date(), "count": 1}]
             )
             mock_issue.objects.filter.return_value = MockQuerySet(
-                [{"created_at": datetime(2024, 1, 3, 12, 0, tzinfo=UTC), "count": 3}]
+                [{"date": datetime(2024, 1, 3, tzinfo=UTC).date(), "count": 3}]
             )

129-156: LGTM!

Test properly validates the handle flow for a specific user, including verification that save() is called with the correct update_fields.


158-174: LGTM!

Good negative test case that verifies proper error messaging when a user is not found.


176-209: LGTM!

Test validates batch processing of multiple users correctly.


211-243: LGTM!

Test properly validates the custom days parameter affects the start date calculation.


245-277: LGTM!

Test validates the date format contract for contribution data keys.

@mrkeshav-05 mrkeshav-05 force-pushed the fix/member-contribution-heatmap branch 2 times, most recently from f4302b9 to cc0637b Compare January 14, 2026 09:33
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@backend/tests/apps/github/api/internal/nodes/user_test.py`:
- Around line 313-337: The two tests (test_contribution_data_field and
test_contribution_data_field_empty) only set and read attributes on a Mock and
do not call any UserNode logic; either update them to exercise the actual
resolver (e.g., call UserNode.contribution_data(user) or the appropriate
resolver method on UserNode) and assert its returned value based on a mock User
instance, or remove these tests if contribution_data is just a direct field and
already covered by test_meta_configuration; locate the tests by their names and
the UserNode class/resolver to implement the change.
🧹 Nitpick comments (1)
backend/apps/github/management/commands/github_aggregate_contributions.py (1)

83-145: LGTM with optional DRY suggestion.

The aggregation logic is correct. All three queries (Commits, PRs, Issues) properly use TruncDate for database-level date grouping.

♻️ Optional: Extract repeated query pattern into helper

The three query blocks share identical structure. If you want to reduce duplication:

def _aggregate_user_contributions(self, user: User, start_date: datetime) -> dict[str, int]:
    contribution_data = {}
    current_date = start_date.date()
    end_date = timezone.now().date()

    while current_date <= end_date:
        date_str = current_date.strftime("%Y-%m-%d")
        contribution_data[date_str] = 0
        current_date += timedelta(days=1)

    models = [Commit, PullRequest, Issue]
    for model in models:
        queryset = (
            model.objects.filter(author=user, created_at__gte=start_date)
            .annotate(date=TruncDate("created_at"))
            .values("date")
            .annotate(count=Count("id"))
        )
        for entry in queryset:
            date_str = entry["date"].strftime("%Y-%m-%d")
            contribution_data[date_str] = contribution_data.get(date_str, 0) + entry["count"]

    return contribution_data
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4302b9 and cc0637b.

📒 Files selected for processing (3)
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.

Applied to files:

  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.

Applied to files:

  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.

Applied to files:

  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.

Applied to files:

  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
🧬 Code graph analysis (2)
backend/apps/github/management/commands/github_aggregate_contributions.py (2)
backend/apps/github/models/commit.py (1)
  • Commit (11-110)
backend/apps/github/models/user.py (2)
  • User (28-223)
  • issues (74-81)
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (1)
backend/apps/github/management/commands/github_aggregate_contributions.py (2)
  • _aggregate_user_contributions (83-145)
  • handle (42-81)
🔇 Additional comments (13)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)

26-26: LGTM!

The addition of contribution_data to the expected field names correctly validates the new GraphQL field exposure in UserNode.

backend/apps/github/management/commands/github_aggregate_contributions.py (3)

1-15: LGTM!

Imports are well-organized and appropriate. Using TruncDate from django.db.models.functions aligns with best practices for date aggregation at the database level.


22-40: LGTM!

Command arguments are well-defined with appropriate defaults and help text.


42-81: LGTM!

The handle method follows Django conventions well:

  • Uses self.stdout.write with styled output
  • Uses users.exists() for existence check (efficient)
  • Memory-efficient iteration with iterator(chunk_size=batch_size)
  • Targeted save with update_fields=["contribution_data"]
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (9)

1-7: LGTM!

Imports are appropriate for the test module.


9-42: LGTM!

The MockQuerySet is a clean utility for testing without database access. It properly supports the chaining pattern used by the command.


47-81: LGTM!

Test correctly validates zero-filled date range when there are no contributions.


83-127: LGTM!

Test correctly validates aggregation logic across commits, PRs, and issues with proper date summing.


129-156: LGTM!

Test correctly validates the handle method with a found user, including verification of save behavior.


158-174: LGTM!

Test correctly validates the error path when a specified user is not found.


176-209: LGTM!

Test correctly validates bulk processing of multiple users.


211-243: LGTM!

Test correctly validates custom days parameter handling with appropriate datetime comparison tolerance.


245-277: LGTM!

Test correctly validates the YYYY-MM-DD date format for contribution data keys.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@mrkeshav-05 mrkeshav-05 force-pushed the fix/member-contribution-heatmap branch from cc0637b to ba086d0 Compare January 14, 2026 09:55
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 14, 2026
@mrkeshav-05 mrkeshav-05 marked this pull request as ready for review January 14, 2026 10:00
@mrkeshav-05 mrkeshav-05 requested a review from arkid15r January 14, 2026 10:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

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

⚠️ Outside diff range comments (1)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)

56-87: Remove unused fetchHeatmapData mock and import.

The ContributionHeatmap now uses user.contributionData directly from the GraphQL response. The mock at lines 56-87 and the import at line 8 are no longer needed—no tests assert on this mock, and the component doesn't import or call fetchHeatmapData.

♻️ Cleanup steps

Remove the import:

-import { fetchHeatmapData } from 'utils/helpers/githubHeatmap'

Remove the mock definition:

-jest.mock('utils/helpers/githubHeatmap', () => ({
-  fetchHeatmapData: jest.fn(),
-}))

Remove the mock setup from beforeEach (lines 81-87) and the duplicate setup in the 'renders contribution heatmap correctly' test (lines 285-291).

🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)

279-300: Test duplicates coverage from the new describe block.

This test ('renders contribution heatmap correctly') duplicates the test at line 828-841 ('renders contribution heatmap when user has contribution data'). Consider removing this duplicate to keep tests DRY.


302-322: Test name doesn't match the actual scenario being tested.

The test is named 'handles contribution heatmap loading error correctly' but it actually tests the scenario where contributionData is null. There's no actual error handling being tested here. Consider renaming for clarity or removing this test since it duplicates the test at line 865-885.

♻️ Suggested rename or removal

Either rename to match what's being tested:

-  test('handles contribution heatmap loading error correctly', async () => {
+  test('does not render heatmap when contributionData is null', async () => {

Or remove entirely since lines 865-885 cover the same scenario.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ba086d0 and 2c4805a.

📒 Files selected for processing (2)
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
  • frontend/src/app/members/[memberKey]/page.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.

Applied to files:

  • frontend/src/app/members/[memberKey]/page.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/__tests__/mockData/mockUserDetails.ts (1)
  • mockUserDetailsData (1-91)
🔇 Additional comments (5)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)

827-886: LGTM!

The new Contribution Heatmap describe block provides good coverage for the three key scenarios:

  1. Rendering when contribution data exists
  2. Not rendering when contribution data is empty ({})
  3. Not rendering when contribution data is null

This aligns well with the conditional rendering logic in the page component.

frontend/src/app/members/[memberKey]/page.tsx (4)

42-47: LGTM!

The type guard and memoization for contributionData properly handles the various states (null, undefined, non-object) and provides a safe fallback to an empty object. The type assertion to Record<string, number> aligns with the expected data shape from the mock data.


49-58: Date sorting approach is correct for ISO format strings.

Using localeCompare for ISO date strings (e.g., '2025-01-01') works correctly because ISO format is lexicographically sortable. The use of dates.at(-1) with nullish coalescing is a clean way to get the last element.


141-141: Minor: Removed unnecessary width constraint allows better responsiveness.

The change to w-full overflow-x-auto provides better responsive behavior for the text content and heatmap container.


166-177: The variant="medium" prop is properly supported by ContributionHeatmap.

The component defines variant?: 'default' | 'medium' | 'compact' and includes implementation for the 'medium' variant. The prop usage in the code is correct.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@mrkeshav-05
Copy link
Contributor Author

mrkeshav-05 commented Jan 14, 2026

Hi @arkid15r ,
I’ve created a management command github-aggregate-contribution using the GitHub User model. The data collection process is currently quite slow, but it’s working as expected.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

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

⚠️ Outside diff range comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)

132-185: Remove dead code: Heatmap component references undefined variables.

This Heatmap component is leftover from the previous implementation and should be removed:

  1. Uses useRef, useState, useTheme which are not imported
  2. References data, username, drawContributions which don't exist in scope
  3. The component is never invoked—ContributionHeatmap is used instead

This dead code will cause runtime errors if ever called and adds confusion.

🐛 Proposed fix: Remove the dead code
-  const Heatmap = () => {
-    const canvasRef = useRef<HTMLCanvasElement | null>(null)
-    const [imgSrc, setImgSrc] = useState('')
-    const { resolvedTheme } = useTheme()
-    const isDarkMode = (resolvedTheme ?? 'light') === 'dark'
-
-    useEffect(() => {
-      if (canvasRef.current && data?.years?.length) {
-        drawContributions(canvasRef.current, {
-          data,
-          username,
-          themeName: isDarkMode ? 'dark' : 'light',
-        })
-        const imageURL = canvasRef.current.toDataURL()
-        setImgSrc(imageURL)
-      } else {
-        setImgSrc('')
-      }
-    }, [isDarkMode])
-
-    return (
-      <div className="overflow-hidden rounded-lg bg-white dark:bg-gray-800">
-        <div className="relative">
-          <canvas ref={canvasRef} style={{ display: 'none' }} tabIndex={-1}></canvas>
-          {imgSrc ? (
-            <div className="h-32">
-              <Image
-                width={100}
-                height={100}
-                src={imgSrc}
-                className="h-full w-full object-cover object-[54%_60%]"
-                alt="Contribution Heatmap"
-              />
-            </div>
-          ) : (
-            <div className="relative h-32 items-center justify-center">
-              <Image
-                height={100}
-                width={100}
-                src={
-                  isDarkMode
-                    ? '/img/heatmap-background-dark.png'
-                    : '/img/heatmap-background-light.png'
-                }
-                className="heatmap-background-loader h-full w-full border-none object-cover object-[54%_60%]"
-                alt="Heatmap Background"
-              />
-              <div className="heatmap-loader"></div>
-            </div>
-          )}
-        </div>
-      </div>
-    )
-  }
-
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c4805a and 066b32f.

📒 Files selected for processing (1)
  • frontend/src/app/members/[memberKey]/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.

Applied to files:

  • frontend/src/app/members/[memberKey]/page.tsx
🔇 Additional comments (3)
frontend/src/app/members/[memberKey]/page.tsx (3)

6-6: LGTM!

The addition of useMemo and ContributionHeatmap imports aligns with the PR objective of using the reusable component.

Also applies to: 15-15


42-60: LGTM!

The memoization logic correctly derives contribution data and date ranges. The typeof === 'object' check combined with truthiness check adequately filters the data, and localeCompare works correctly for ISO date strings.


221-232: LGTM!

The conditional rendering correctly guards against missing data with a comprehensive check (hasContributionData && dateRange.startDate && dateRange.endDate). The ContributionHeatmap integration with the "medium" variant aligns with the PR objectives for design consistency.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 15, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/UserDetails.test.tsx`:
- Around line 827-841: Duplicate test detected: remove the redundant test titled
'renders contribution heatmap when user has contribution data' (which repeats
the earlier 'renders contribution heatmap correctly') to avoid redundant
assertions; keep the more comprehensive 'renders contribution heatmap correctly'
test, delete the duplicate test block that mocks useQuery and renders
<UserDetailsPage />, and ensure any unique setup from the removed block (e.g.,
specific useQuery mock return) is already covered by the retained test.
- Around line 57-59: Remove the unused fetchHeatmapData mock and import from the
test: delete the import of fetchHeatmapData and the jest.mock(...) block that
stubs fetchHeatmapData, and also remove any mockResolvedValue calls that target
fetchHeatmapData; the UserDetailsPage component provides contribution data via
user.contributionData to ContributionHeatmap, so ensure all fetchHeatmapData
references (including mockResolvedValue usages) are removed from the test to
eliminate dead code.
🧹 Nitpick comments (5)
frontend/src/components/ContributionHeatmap.tsx (1)

298-302: Consider memoizing getChartHeight for consistency.

The calculateChartWidth is wrapped in useMemo, but getChartHeight is a plain function that re-creates on each render. While the performance impact is negligible for this simple conditional logic, memoizing it would maintain consistency with the width calculation pattern.

♻️ Optional: Memoize getChartHeight
-  const getChartHeight = () => {
-    if (variant === 'compact') return 150
-    if (variant === 'medium') return 172
-    return 195
-  }
+  const chartHeight = useMemo(() => {
+    if (variant === 'compact') return 150
+    if (variant === 'medium') return 172
+    return 195
+  }, [variant])

Then update the Chart usage:

-            height={getChartHeight()}
+            height={chartHeight}
frontend/src/app/members/[memberKey]/page.tsx (1)

42-47: Type assertion could be made safer.

The type assertion as Record<string, number> on line 44 assumes the backend always returns the correct shape. If user.contributionData contains unexpected values (e.g., non-numeric counts), this could cause runtime issues in the heatmap component.

Consider adding runtime validation or using a type guard for defense:

♻️ Optional: Add runtime validation
 const contributionData: Record<string, number> = useMemo(() => {
   if (user?.contributionData && typeof user.contributionData === 'object') {
-    return user.contributionData as Record<string, number>
+    const data = user.contributionData as Record<string, unknown>
+    const validated: Record<string, number> = {}
+    for (const [key, value] of Object.entries(data)) {
+      if (typeof value === 'number') {
+        validated[key] = value
+      }
+    }
+    return validated
   }
   return {}
 }, [user?.contributionData])
backend/apps/github/management/commands/github_aggregate_contributions.py (2)

71-73: The if contribution_data: check is always true.

Since _aggregate_user_contributions pre-populates contribution_data with zero values for all dates in the range (lines 98-101), the returned dictionary is never empty. The condition on line 71 will always evaluate to True.

This isn't a bug (users are correctly saved), but the check is misleading. Consider either:

  1. Removing the check if saving all users is intended
  2. Changing the condition to check for actual contributions: if any(contribution_data.values())
♻️ Option 1: Remove redundant check
         for user in users.iterator(chunk_size=batch_size):
             contribution_data = self._aggregate_user_contributions(user, start_date)

-            if contribution_data:
-                user.contribution_data = contribution_data
-                user.save(update_fields=["contribution_data"])
-                processed += 1
+            user.contribution_data = contribution_data
+            user.save(update_fields=["contribution_data"])
+            processed += 1

-                if processed % 100 == 0:
-                    self.stdout.write(f"Processed {processed}/{total_users} users...")
+            if processed % 100 == 0:
+                self.stdout.write(f"Processed {processed}/{total_users} users...")
♻️ Option 2: Only save if there are actual contributions
-            if contribution_data:
+            if any(contribution_data.values()):

67-77: Consider using bulk_update for better performance on large datasets.

The current implementation saves each user individually, which for thousands of users results in N database writes. While acceptable for a background command, using bulk_update could significantly improve performance.

♻️ Batch updates for efficiency
         processed = 0
+        users_to_update = []
         for user in users.iterator(chunk_size=batch_size):
             contribution_data = self._aggregate_user_contributions(user, start_date)

-            if contribution_data:
-                user.contribution_data = contribution_data
-                user.save(update_fields=["contribution_data"])
-                processed += 1
+            user.contribution_data = contribution_data
+            users_to_update.append(user)
+            processed += 1

-                if processed % 100 == 0:
-                    self.stdout.write(f"Processed {processed}/{total_users} users...")
+            if len(users_to_update) >= batch_size:
+                User.objects.bulk_update(users_to_update, ["contribution_data"])
+                users_to_update = []
+                self.stdout.write(f"Processed {processed}/{total_users} users...")
+
+        if users_to_update:
+            User.objects.bulk_update(users_to_update, ["contribution_data"])

         self.stdout.write(
             self.style.SUCCESS(f"Successfully aggregated contributions for {processed} users")
         )
backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (1)

273-277: Optional: Remove unnecessary .replace(tzinfo=UTC) call.

The datetime.strptime() call already validates the date format—if the format doesn't match, it raises ValueError. The .replace(tzinfo=UTC) result is unused and adds no value to the validation.

♻️ Suggested simplification
             for date_str in result:
-                datetime.strptime(date_str, "%Y-%m-%d").replace(tzinfo=UTC)
+                datetime.strptime(date_str, "%Y-%m-%d")
                 assert len(date_str) == 10
                 assert date_str[4] == "-"
                 assert date_str[7] == "-"
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 066b32f and e7a27ed.

⛔ Files ignored due to path filters (2)
  • frontend/src/types/__generated__/graphql.ts is excluded by !**/__generated__/**
  • frontend/src/types/__generated__/userQueries.generated.ts is excluded by !**/__generated__/**
📒 Files selected for processing (15)
  • backend/Makefile
  • backend/apps/github/Makefile
  • backend/apps/github/api/internal/nodes/user.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/apps/github/migrations/0041_user_contribution_data.py
  • backend/apps/github/models/user.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/models/user_test.py
  • frontend/__tests__/mockData/mockUserDetails.ts
  • frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
  • frontend/__tests__/unit/pages/UserDetails.test.tsx
  • frontend/src/app/members/[memberKey]/page.tsx
  • frontend/src/components/ContributionHeatmap.tsx
  • frontend/src/server/queries/userQueries.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • backend/apps/github/models/user.py
  • frontend/tests/unit/components/ContributionHeatmap.test.tsx
  • backend/Makefile
  • frontend/src/server/queries/userQueries.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.

Applied to files:

  • frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.

Applied to files:

  • backend/apps/github/migrations/0041_user_contribution_data.py
  • backend/apps/github/api/internal/nodes/user.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/models/user_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.

Applied to files:

  • backend/apps/github/migrations/0041_user_contribution_data.py
  • backend/apps/github/api/internal/nodes/user.py
  • backend/tests/apps/github/api/internal/nodes/user_test.py
  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
  • backend/tests/apps/github/models/user_test.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.

Applied to files:

  • backend/apps/github/management/commands/github_aggregate_contributions.py
  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.

Applied to files:

  • backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py
🧬 Code graph analysis (2)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/__tests__/mockData/mockUserDetails.ts (1)
  • mockUserDetailsData (1-91)
backend/tests/apps/github/models/user_test.py (1)
backend/apps/github/models/user.py (1)
  • User (28-223)
🔇 Additional comments (21)
backend/apps/github/api/internal/nodes/user.py (1)

16-16: LGTM!

The contribution_data field is correctly added to the GraphQL type definition, maintaining alphabetical ordering with the other fields. The strawberry_django integration will handle serialization of the JSONField automatically.

frontend/__tests__/mockData/mockUserDetails.ts (1)

16-22: LGTM!

The mock contributionData structure correctly mirrors the backend's contribution_data JSONField format (date string keys to integer counts). Including a zero-count entry ('2025-01-04': 0) is helpful for testing edge cases in heatmap rendering.

backend/apps/github/migrations/0041_user_contribution_data.py (1)

11-21: LGTM!

The migration correctly uses default=dict (callable) to avoid the mutable default argument pitfall. The JSONField is well-suited for storing the date→count mapping. Not including null=True means existing records will get an empty dict {} rather than NULL, which provides consistent handling in application code.

frontend/src/components/ContributionHeatmap.tsx (1)

273-294: LGTM!

The width calculation logic correctly handles all three variants with appropriate pixel-per-week ratios and minimum widths. The dependency array [heatmapSeries, variant] correctly captures all values used in the calculation.

frontend/__tests__/unit/pages/UserDetails.test.tsx (2)

843-885: LGTM!

The edge case tests for empty {} and null contribution data are well-structured. Using queryByTestId with not.toBeInTheDocument() is the correct pattern for asserting element absence.


61-68: LGTM!

The mock ContributionHeatmap component is appropriately minimal for testing presence/absence of the heatmap. The displayName assignment aids debugging.

frontend/src/app/members/[memberKey]/page.tsx (2)

49-60: LGTM!

The date range computation correctly leverages lexicographic sorting for ISO date strings (YYYY-MM-DD), and the defensive guards ensure the heatmap only renders when valid data exists.


166-177: LGTM!

The ContributionHeatmap integration is clean with proper conditional rendering and responsive overflow handling. This aligns well with the PR objective of reusing the shared component for design consistency.

backend/tests/apps/github/models/user_test.py (1)

219-248: LGTM!

The tests provide good coverage for the contribution_data field behavior:

  • Default value verification
  • Storage and retrieval of JSON data
  • Update behavior

The tests appropriately validate in-memory model behavior without requiring database fixtures.

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

17-49: LGTM!

The addition of contribution_data to the expected fields set correctly validates that the new field is exposed on the GraphQL UserNode type. This aligns with the backend model changes and API exposure.

backend/apps/github/Makefile (1)

25-27: LGTM!

The new Makefile target follows the established pattern of other targets in this file, using consistent naming and the exec-backend-command wrapper.

backend/apps/github/management/commands/github_aggregate_contributions.py (2)

103-143: LGTM!

The aggregation queries correctly use TruncDate for database-level date grouping, and the pattern is consistently applied across commits, PRs, and issues. The .get(date_str, 0) provides safe accumulation for any edge cases.


22-54: LGTM!

The command arguments are well-defined with sensible defaults, and the initialization correctly uses timezone.now() for timezone-aware datetime handling. Output follows Django conventions using self.stdout.write with styling. Based on learnings, this aligns with Django management command best practices.

backend/tests/apps/github/management/commands/github_aggregate_contributions_test.py (8)

1-6: LGTM!

Imports are clean and appropriate for the test file. Good use of UTC from datetime for timezone-aware datetime handling.


9-41: LGTM!

The MockQuerySet utility class provides a clean abstraction for mocking Django queryset behavior without database access. The chainable methods returning self are appropriate for this testing context where the return data is pre-configured via MockQuerySet initialization.


47-81: LGTM!

Test correctly validates the zero-filled behavior when no contributions exist. The date range (Jan 1 to Jan 30) correctly produces 30 entries with all zero counts.


83-127: LGTM!

Excellent test coverage for the aggregation logic. The test correctly validates that contributions from commits, PRs, and issues are summed by date, and that days without contributions show zero counts.


129-156: LGTM!

Test correctly validates the single-user processing flow. Good verification that save() is called with update_fields=["contribution_data"] for efficient partial updates.


158-174: LGTM!

Test correctly validates the user-not-found error handling. The assertion checking for "not found" in stdout output is a reasonable approach for verifying error messaging.


176-209: LGTM!

Test comprehensively validates the bulk processing flow for all users. Good verification that each user is processed individually with their contribution data updated and saved.


211-243: LGTM!

Test correctly validates that the custom days parameter influences the start date calculation. The tolerance-based comparison (< 1 second) is appropriate for datetime assertions.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@mrkeshav-05 mrkeshav-05 force-pushed the fix/member-contribution-heatmap branch from 631a335 to 0278be4 Compare January 16, 2026 15:29
@sonarqubecloud
Copy link

@echo "Updating GitHub users"
@CMD="python manage.py github_update_users" $(MAKE) exec-backend-command

github-aggregate-contributions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
github-aggregate-contributions:
github-aggregate-user-contributions:

"avatar_url",
"bio",
"company",
"contribution_data",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be consistent and implement it the same way as for chapters/projects. I think introducing an abstract model to derive from is reasonable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If your PR has a migration you must temporary change the docker DB volume name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add reusable Contribution Heatmap to member page for design consistency

2 participants