(Phase 1) Add weekly team stats aggregation service #2581 #5373
Conversation
|
👋 Hi @Shhazzz! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
github-actions
bot
added
files-changed: 1
needs-peer-review
WalkthroughAdds a new Django service function get_weekly_team_stats(start_date, end_date) that validates the date range, queries TEAM organizations, runs a single aggregated ContributorStats query over the date range for repos in those teams, and returns per-team summaries (zeros when missing); returns [] if no TEAMs exist. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Service as TeamWeeklyStatsService
participant Org as OrganizationModel
participant Stats as ContributorStatsModel
rect rgb(235,240,255)
Caller->>Service: get_weekly_team_stats(start_date, end_date)
end
Service->>Service: validate start_date <= end_date
Service->>Org: query organizations where type == TEAM
Org-->>Service: teams[]
alt no teams
Service-->>Caller: []
else teams found
Service->>Stats: aggregated query\n(filter: granularity='day', date range, repos in teams)\ngroup by repo__organization_id
Note right of Stats: SUM(commits, issues_opened,\nissues_closed, pull_requests, comments)\n(Coalesce -> 0)
Stats-->>Service: aggregated rows per team_id
Service->>Service: map aggregates to each team\n(fill zeros for teams without rows)
Service-->>Caller: list of team stats dictionaries
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
📊 Monthly LeaderboardHi @Shhazzz! Here's how you rank for December 2025:
Leaderboard based on contributions in December 2025. Keep up the great work! 🚀 |
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
website/services/team_weekly_stats.py (3)
16-46: Consider optimizing the N+1 query pattern for better performance.The current implementation executes 2 queries per team (fetching repos + aggregating stats). For N teams, this results in 2N+1 database queries. This can be refactored into a single grouped aggregation query.
🔎 Proposed optimization using a single query with annotation
-from django.db.models import Sum -from website.models import Organization, ContributorStats +from django.db.models import Sum +from website.models import Organization, ContributorStats, OrganisationType def get_weekly_team_stats(start_date, end_date): """ Aggregate weekly stats for teams (Organization type = TEAM). Returns an empty list if no TEAM organizations or contributor stats exist in the given date range. """ - team_stats = [] - - teams = Organization.objects.filter(type="team") - - for team in teams: - # Get all repos owned by this team - repos = team.repos.all() - - # Aggregate contributor stats for these repos in the given week - stats = ContributorStats.objects.filter( - repo__in=repos, - granularity="day", - date__gte=start_date, - date__lte=end_date, - ).aggregate( - commits=Sum("commits"), - issues_opened=Sum("issues_opened"), - issues_closed=Sum("issues_closed"), - pull_requests=Sum("pull_requests"), - comments=Sum("comments"), - ) - - team_stats.append({ - "team_id": team.id, - "team_name": team.name, - "start_date": start_date, - "end_date": end_date, - "stats": { - "commits": stats["commits"] or 0, - "issues_opened": stats["issues_opened"] or 0, - "issues_closed": stats["issues_closed"] or 0, - "pull_requests": stats["pull_requests"] or 0, - "comments": stats["comments"] or 0, - }, - }) - - return team_stats + teams = Organization.objects.filter( + type=OrganisationType.TEAM.value, + repos__contributor_stats__granularity="day", + repos__contributor_stats__date__gte=start_date, + repos__contributor_stats__date__lte=end_date, + ).annotate( + total_commits=Sum("repos__contributor_stats__commits"), + total_issues_opened=Sum("repos__contributor_stats__issues_opened"), + total_issues_closed=Sum("repos__contributor_stats__issues_closed"), + total_pull_requests=Sum("repos__contributor_stats__pull_requests"), + total_comments=Sum("repos__contributor_stats__comments"), + ).distinct() + + return [ + { + "team_id": team.id, + "team_name": team.name, + "start_date": start_date, + "end_date": end_date, + "stats": { + "commits": team.total_commits or 0, + "issues_opened": team.total_issues_opened or 0, + "issues_closed": team.total_issues_closed or 0, + "pull_requests": team.total_pull_requests or 0, + "comments": team.total_comments or 0, + }, + } + for team in teams + ]Note: The optimized version excludes teams with zero stats in the date range. If you need to include all teams (even those with no activity), the current loop-based approach is acceptable for Phase 1, but consider adding
prefetch_related('repos')to the teams queryset to reduce queries.
5-11: Consider adding type hints for better maintainability.Adding type annotations improves IDE support and makes the function contract explicit.
🔎 Proposed enhancement
+from datetime import date +from typing import Any + from django.db.models import Sum from website.models import Organization, ContributorStats -def get_weekly_team_stats(start_date, end_date): +def get_weekly_team_stats(start_date: date, end_date: date) -> list[dict[str, Any]]: """ Aggregate weekly stats for teams (Organization type = TEAM).
14-14: UseOrganisationTypeconstant instead of hardcoded string.The
Organization.typefield is constrained by theOrganisationTypeenum. Line 14 uses the hardcoded string"team"which is fragile and unmaintainable—if the enum value changes, the code breaks silently. Import and useOrganisationType.TEAM.valueinstead.from django.db.models import Sum -from website.models import Organization, ContributorStats +from website.models import Organization, ContributorStats, OrganisationType def get_weekly_team_stats(start_date, end_date): ... - teams = Organization.objects.filter(type="team") + teams = Organization.objects.filter(type=OrganisationType.TEAM.value)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/services/team_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/services/team_weekly_stats.py (1)
website/models.py (2)
Organization(181-289)ContributorStats(1984-2006)
⏰ 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: add_changes_requested_label
- GitHub Check: check_peer_review
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
website/services/team_weekly_stats.py (1)
21-32: Aggregation logic is correct.The filtering by
granularity="day"and date range, along with theSumaggregation on all relevant fields (commits,issues_opened,issues_closed,pull_requests,comments), correctly implements the weekly stats collection as per theContributorStatsmodel structure.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/services/team_weekly_stats.py (1)
15-33: Confirmed: N+1 query issue causes performance degradation.The loop at line 17 triggers 2N additional database queries (N for repos, N for stats aggregation). With many teams, this will significantly degrade performance.
The fix is straightforward: use
prefetch_related('repos')on the teams query to reduce this from 1 + 2N queries to 1 + N queries (1 for teams+repos prefetch, N for stats aggregation per team).🔎 Proposed fix using prefetch_related
- teams = Organization.objects.filter(type="team") + teams = Organization.objects.filter(type="team").prefetch_related('repos') for team in teams: - # Get all repos owned by this team + # Get all repos owned by this team (uses prefetched data, no additional query) repos = team.repos.all()With this change,
team.repos.all()uses the prefetched data instead of triggering a new query for each team. This reduces the query count from O(2N) to O(N) additional queries.
🧹 Nitpick comments (2)
website/services/team_weekly_stats.py (2)
6-12: Consider adding type hints and parameter validation.The function would benefit from:
- Type hints for
start_dateandend_date(likelydatetime.date)- Basic validation to ensure
end_date >= start_date- Updating the docstring to document parameter types and return value structure
This improves code clarity, IDE support, and prevents runtime errors from invalid inputs.
🔎 Example with type hints and validation
Add import at the top:
from datetime import dateThen update the function:
-def get_weekly_team_stats(start_date, end_date): +def get_weekly_team_stats(start_date: date, end_date: date) -> list[dict]: """ Aggregate weekly stats for teams (Organization type = TEAM). + + Args: + start_date: Start date for the stats period (inclusive) + end_date: End date for the stats period (inclusive) + + Returns: + List of dictionaries containing team stats, empty list if no teams exist. + + Raises: + ValueError: If end_date is before start_date """ + if end_date < start_date: + raise ValueError("end_date must be >= start_date") + team_stats = []
15-15: Use the enum constantOrganisationType.TEAM.valueinstead of the hardcoded "team" string.The filter will work with the hardcoded string since
OrganisationType.TEAMequals"team", but using the enum constant directly improves code maintainability and ensures consistency across the codebase. Changetype="team"totype=OrganisationType.TEAM.value.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/services/team_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/services/team_weekly_stats.py (1)
website/models.py (2)
ContributorStats(1984-2006)Organization(181-289)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (2)
website/services/team_weekly_stats.py (2)
1-3: LGTM!Imports are appropriate for the aggregation logic.
41-45: Good defensive programming for null aggregation results.The
or 0pattern correctly handles cases where Sum returns None (when no matching records exist), ensuring consistent integer output.
github-actions
bot
added
tests: passed
github-actions
bot
added
pre-commit: passed
|
@DonnieBLT This PR implements Phase 1 only (service-layer aggregation). I’ll follow up with Phase 2 to wire this into the weekly activity flow once this is approved. |
github-actions
bot
added
last-active: 0d
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 147979 @Jayant2908, I've analyzed the code in Issues Status1. ✘ Robust filtering of TEAM orgs (Line 15)Current code: teams = Organization.objects.filter(type="team")Issue: Uses hardcoded string Fix needed: Use from website.models import Organization, OrganisationType
teams = Organization.objects.filter(type=OrganisationType.TEAM.value)2. ✘ Aggregation may return None (Lines 25-29)Current code: commits=Sum("commits"),
issues_opened=Sum("issues_opened"),
# ... etcIssue: Fix needed: from django.db.models.functions import Coalesce
commits=Coalesce(Sum("commits"), 0),
issues_opened=Coalesce(Sum("issues_opened"), 0),3. ✘ Avoid loading full model instances (Line 15, Line 37-40)Current code: teams = Organization.objects.filter(type="team")
# ... later ...
for team in teams:
# accesses team.id and team.nameIssue: Loads all Organization fields when only Fix needed: teams = Organization.objects.filter(type=OrganisationType.TEAM.value).only('id', 'name')4. ✘ Query efficiency & correctnessRelated to issue #3 - same fix applies. 5. ✘ Input validation and typing (Line 7)Current code: def get_weekly_team_stats(start_date, end_date):Issue: No type hints or validation. Fix needed: from datetime import date
from typing import List, Dict, Any
def get_weekly_team_stats(start_date: date, end_date: date) -> List[Dict[str, Any]]:
"""
Aggregate weekly stats for teams (Organization type = TEAM).
Args:
start_date: Start date for the stats period
end_date: End date for the stats period
Returns:
List of dictionaries containing team statistics
"""6. ✘ Tests and coverageFinding: No test file exists for Action needed: Create
7. ✘ Logging and error handlingIssue: No logging present. Fix needed: import logging
logger = logging.getLogger(__name__)
def get_weekly_team_stats(start_date: date, end_date: date) -> List[Dict[str, Any]]:
logger.info(f"Aggregating team stats from {start_date} to {end_date}")
teams = Organization.objects.filter(type=OrganisationType.TEAM.value).only('id', 'name')
if not teams.exists():
logger.debug("No TEAM organizations found")
return []8. ✘ Granularity field assumption (Line 21)Current code: granularity="day",Issue: No explanation for why "day" is chosen. The Questions to resolve:
Recommendation: Add documentation explaining the choice, or make granularity configurable. SummaryAll 8 concerns are valid and need to be addressed before this PR should be merged. The code works functionally but lacks:
Would you like me to create an issue to track these improvements, @Jayant2908? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
website/tests/test_team_weekly_stats.py (5)
5-6: Move the Repo import to the top-level imports.Line 43 imports
Repoinline with a comment about avoiding unused import warnings. However, sinceRepois used intest_single_team_with_stats, it should be imported at the module level alongside the other models for consistency and clarity.🔎 Proposed fix
-from website.models import Contributor, ContributorStats, OrganisationType, Organization +from website.models import Contributor, ContributorStats, OrganisationType, Organization, Repo from website.services.team_weekly_stats import get_weekly_team_statsThen remove the inline import at line 43:
- # Create a repo under the team - from website.models import Repo # import here to avoid unused import warnings - repo = Repo.objects.create(
30-31: Verify all stats fields for consistency.The test only checks
commitsandissues_opened, but the service returns five stats fields (commits, issues_opened, issues_closed, pull_requests, comments). For consistency withtest_single_team_with_statsand to ensure complete coverage, verify all five fields are zero.🔎 Proposed fix
assert result[0]["stats"]["commits"] == 0 assert result[0]["stats"]["issues_opened"] == 0 + assert result[0]["stats"]["issues_closed"] == 0 + assert result[0]["stats"]["pull_requests"] == 0 + assert result[0]["stats"]["comments"] == 0
25-31: Consider asserting the complete response structure.The test assumes
result[0]exists and only validates the stats. Consider also verifying the response length and other top-level fields (team_id, team_name, start_date, end_date) to ensure the service returns the complete expected structure.🔎 Example enhancement
result = get_weekly_team_stats( start_date=date(2025, 1, 1), end_date=date(2025, 1, 7), ) + assert len(result) == 1 + assert result[0]["team_id"] == team.id + assert result[0]["team_name"] == "Test Team" + assert result[0]["start_date"] == date(2025, 1, 1) + assert result[0]["end_date"] == date(2025, 1, 7) assert result[0]["stats"]["commits"] == 0 assert result[0]["stats"]["issues_opened"] == 0
71-81: Consider asserting the complete response structure.Similar to the previous test, this test assumes
result[0]exists and only validates the stats. Consider also verifying the response length and other top-level fields (team_id, team_name, start_date, end_date) to ensure the service returns the complete expected structure.🔎 Example enhancement
# Call the service result = get_weekly_team_stats( start_date=date(2025, 1, 1), end_date=date(2025, 1, 7), ) # Assert stats are correctly aggregated + assert len(result) == 1 + assert result[0]["team_id"] == team.id + assert result[0]["team_name"] == "Team A" + assert result[0]["start_date"] == date(2025, 1, 1) + assert result[0]["end_date"] == date(2025, 1, 7) assert result[0]["stats"]["commits"] == 5 assert result[0]["stats"]["issues_opened"] == 2 assert result[0]["stats"]["issues_closed"] == 1 assert result[0]["stats"]["pull_requests"] == 1 assert result[0]["stats"]["comments"] == 3
1-81: Expand test coverage for multi-team and multi-repo scenarios.The current tests validate basic scenarios (no teams, single team with/without stats), but don't cover:
- Multiple teams: Verify each team's stats are aggregated independently.
- Multiple repos under one team: Confirm stats are aggregated across all repos belonging to the same team.
- Date range boundaries: Ensure stats at
start_dateandend_dateare included, and stats outside the range are excluded.- Multiple contributors per repo: Verify aggregation across contributors.
These scenarios are critical for validating the service's aggregation logic under realistic conditions.
💡 Example test for multiple teams
@pytest.mark.django_db def test_multiple_teams_aggregate_independently(): team_a = Organization.objects.create( name="Team A", type=OrganisationType.TEAM.value, url="https://example.com/team-a", ) team_b = Organization.objects.create( name="Team B", type=OrganisationType.TEAM.value, url="https://example.com/team-b", ) repo_a = Repo.objects.create( name="repo-a", organization=team_a, repo_url="https://github.com/team-a/repo-a", ) repo_b = Repo.objects.create( name="repo-b", organization=team_b, repo_url="https://github.com/team-b/repo-b", ) contributor = Contributor.objects.create( name="Contributor", github_id=99999, github_url="https://github.com/contributor", avatar_url="https://avatars.githubusercontent.com/u/99999", contributor_type="INDIVIDUAL", contributions=0, ) ContributorStats.objects.create( repo=repo_a, contributor=contributor, granularity="day", date=date(2025, 1, 2), commits=10, ) ContributorStats.objects.create( repo=repo_b, contributor=contributor, granularity="day", date=date(2025, 1, 2), commits=20, ) result = get_weekly_team_stats( start_date=date(2025, 1, 1), end_date=date(2025, 1, 7), ) assert len(result) == 2 teams_by_name = {r["team_name"]: r for r in result} assert teams_by_name["Team A"]["stats"]["commits"] == 10 assert teams_by_name["Team B"]["stats"]["commits"] == 20
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/tests/test_team_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/tests/test_team_weekly_stats.py (2)
website/models.py (5)
Contributor(1351-1363)ContributorStats(1984-2006)OrganisationType(175-178)Organization(181-289)Repo(1904-1981)website/services/team_weekly_stats.py (1)
get_weekly_team_stats(13-69)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
website/tests/test_team_weekly_stats.py (1)
9-15: LGTM!The test correctly verifies that an empty list is returned when no TEAM organizations exist.
github-actions
bot
added
tests: failed
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
website/tests/test_team_weekly_stats.py (2)
45-45: Remove redundant import.
Repois already imported fromwebsite.modelsat the top of the file (line 5).🔎 Proposed fix
- # Create a repo under the team - from website.models import Repo - repo = Repo.objects.create(
1-85: Consider adding tests for more complex scenarios.The current tests cover the basic cases well. To increase confidence in the aggregation logic, consider adding tests for:
- Multiple teams (verify each team's stats are correctly isolated)
- Multiple repos per team (verify aggregation across repos)
- Multiple ContributorStats entries across different dates within the range (verify aggregation across days)
- Stats with dates outside the specified range (verify date filtering)
These can be added in a future iteration or follow-up PR.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/tests/test_team_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/tests/test_team_weekly_stats.py (2)
website/models.py (5)
Contributor(1351-1363)ContributorStats(1984-2006)OrganisationType(175-178)Organization(181-289)Repo(1904-1981)website/services/team_weekly_stats.py (1)
get_weekly_team_stats(13-69)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
website/tests/test_team_weekly_stats.py (1)
9-15: LGTM!This test correctly verifies the empty-list behavior when no TEAM organizations exist.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/services/team_weekly_stats.py (1)
13-70: Complete the test coverage for remaining scenarios.As noted in previous reviews, three of six test scenarios are implemented, but three critical scenarios remain missing:
- Multiple teams with isolated stats - Verify that Team A's stats don't leak into Team B's aggregation
- Date range boundary filtering - Confirm that:
- Stats on
start_dateare included- Stats on
end_dateare included- Stats before
start_dateare excluded- Stats after
end_dateare excluded- Single-day range (
start_date == end_date) - Validate edge case handlingThese tests are important to ensure the aggregation logic correctly isolates teams and respects date boundaries.
Test skeleton for missing scenarios
@pytest.mark.django_db def test_multiple_teams_isolated_aggregations(): """Verify each team's stats are aggregated independently.""" # Create Team A with 5 commits, Team B with 10 commits # Assert Team A result shows 5, Team B shows 10 (no cross-contamination) @pytest.mark.django_db def test_date_range_boundaries(): """Verify date__range includes boundaries and excludes outside dates.""" # Create stats on: start_date, end_date, day before start, day after end # Assert only start_date and end_date records are summed @pytest.mark.django_db def test_single_day_range(): """Verify single-day range works correctly.""" # Call get_weekly_team_stats(date(2025,1,5), date(2025,1,5)) # Assert only records from that exact day are included
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/services/team_weekly_stats.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (4)
website/services/team_weekly_stats.py (4)
1-10: Imports and setup look good.The imports are clean and necessary, and the logger is properly configured for the module.
21-27: Efficient query pattern with proper early return.The code correctly uses
OrganisationType.TEAM.value(enum constant),.only('id', 'name')to limit fetched fields, andlist()to evaluate the queryset once. The early return for no teams is appropriate.
29-47: Well-optimized aggregation query.The implementation correctly addresses previous performance concerns:
- Single database query eliminates N+1 issues
Coalesce(Sum(...), 0)ensures null-safe aggregation- Comment explains daily granularity and weekly aggregation approach
- Efficient filtering using
team_idslistThe query pattern is solid and production-ready.
49-70: Result building logic is correct and efficient.The code uses a dict mapping for O(1) lookups and ensures every team appears in the output with appropriate zero defaults when no stats exist. The structure is consistent and handles missing data gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/services/team_weekly_stats.py (1)
14-19: Consider enhancing the docstring for better API documentation.The docstring is functional but could be more comprehensive. Since this is a public service function that will be integrated in Phase 2, documenting the exact structure of returned dictionaries would help callers understand the API contract without inspecting the implementation.
🔎 Suggested docstring enhancement
def get_weekly_team_stats(start_date: date, end_date: date) -> List[Dict[str, Any]]: """ Aggregate weekly stats for TEAM organizations. + + Args: + start_date: Start date (inclusive) of the aggregation period. + end_date: End date (inclusive) of the aggregation period. + + Returns: + List of dictionaries, one per TEAM organization, each containing: + - team_id (int): Organization ID + - team_name (str): Organization name + - start_date (date): Input start_date + - end_date (date): Input end_date + - stats (dict): Aggregated statistics with keys: + - commits (int) + - issues_opened (int) + - issues_closed (int) + - pull_requests (int) + - comments (int) + Returns an empty list if no TEAM organizations or contributor stats exist in the given date range. + + Raises: + ValidationError: If start_date is after end_date. """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
website/services/team_weekly_stats.pywebsite/tests/test_team_weekly_stats.py
🚧 Files skipped from review as they are similar to previous changes (1)
- website/tests/test_team_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/services/team_weekly_stats.py (1)
website/models.py (3)
ContributorStats(1984-2006)OrganisationType(175-178)Organization(181-289)
⏰ 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 pre-commit
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (6)
website/services/team_weekly_stats.py (6)
1-12: LGTM! Imports and logger setup are correct.All necessary imports are present and correctly sourced from the Django framework and project models. Logger initialization follows best practices.
20-21: LGTM! Date validation is correctly implemented.The validation ensures start_date ≤ end_date and raises a clear ValidationError if violated, addressing previous review feedback.
23-25: Excellent query optimization! All prior feedback addressed.The implementation correctly:
- Logs the aggregation operation for observability
- Uses
OrganisationType.TEAM.valueinstead of a hardcoded string- Applies
.only("id", "name")to limit field loading- Converts the QuerySet to a list to prevent multiple database evaluations
This addresses all query optimization and enum usage concerns from previous reviews.
27-29: LGTM! Early return with appropriate logging.The defensive check prevents unnecessary database queries when no TEAM organizations exist, with debug-level logging to aid troubleshooting.
31-49: Excellent aggregation query! Performance concerns resolved.The single aggregated query efficiently addresses all previous N+1 and null-handling concerns:
- Uses
__inlookup withteam_idslist to fetch all stats in one query- Applies
Coalesce(Sum(...), 0)for null-safe aggregation- Filters by
granularity="day"as documented in the comments- Groups by
repo__organization_idfor per-team resultsThe explanatory comments about daily granularity aggregation are helpful.
51-72: Implementation logic is correct. Test coverage gap remains.The results construction is well-implemented:
- Efficient dictionary lookup with
stats_map- Graceful handling of teams with no stats (defaults to zeros)
- Consistent output structure for all teams
However, as noted in previous bot comments, test coverage is still incomplete. The existing tests cover basic scenarios but are missing:
- Multiple teams with varying stats – verify that each team's aggregation is correctly isolated
- Date boundary filtering – test records on
start_date,end_date, beforestart_date, and afterend_dateto confirm only the inclusive range is aggregated- Single-day edge case – test with
start_date == end_dateThese test gaps were already flagged by the bot. Please add the remaining test cases before merging to ensure the date filtering and multi-team aggregation logic is thoroughly validated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/tests/test_team_weekly_stats.py (1)
10-101: Consider expanding test coverage for additional scenarios.The current test suite covers basic functionality well, but several important scenarios are untested:
- Multiple teams: Verify that the service correctly aggregates stats for each team independently when multiple TEAM organizations exist.
- Multiple repos per team: Ensure stats are aggregated across all repos belonging to a single team.
- Multiple days within range: The current test only creates stats for a single day (Jan 3); adding stats across multiple days would verify the date-range aggregation logic.
- Date range boundaries: Test stats on
start_dateandend_dateto confirm inclusive/exclusive behavior.- Organization type filtering: Add non-TEAM organizations (ORGANIZATION or INDIVIDUAL types) to verify they're correctly excluded from results.
- Stats outside range: Create stats before
start_dateor afterend_dateto ensure they're filtered out.Adding these test cases would increase confidence in the aggregation and filtering logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/tests/test_team_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/tests/test_team_weekly_stats.py (1)
website/models.py (4)
Contributor(1351-1363)OrganisationType(175-178)Organization(181-289)Repo(1904-1981)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (3)
website/tests/test_team_weekly_stats.py (3)
11-16: LGTM!The test correctly validates that
ValidationErroris raised when the date range is invalid (end before start).
18-23: LGTM!The test properly validates the empty-list behavior when no TEAM organizations exist.
25-51: LGTM!The test comprehensively validates zero-initialization for teams without stats, including all metadata fields and all five stats metrics.
github-actions
bot
added
tests: passed
|
Hi @DonnieBLT, all requested changes have been addressed, tests are passing, and CodeRabbitAI has approved. Please review this PR when you get a chance. |
github-actions
bot
added
last-active: 1d
2 participants
Summary
Fixes #2581 (Phase 1)
This PR adds a service-layer helper to aggregate weekly activity statistics for TEAM organizations.
What’s included:
Scope:
This is intended as Phase 1 groundwork for team-level weekly activity summaries.
Next steps (planned)
(Phase 2): Hook this service into the existing weekly activity flow so team-level stats are included along with org-level data.
(Phase 3): Expose team weekly stats through the API and/or UI if needed.
I’ll open separate follow-up PRs for these once Phase 1 is reviewed and approved.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.