Fix builder leaderboard eligibility - #887
Merged
Merged
Conversation
📝 WalkthroughWalkthroughBuilder ranking now excludes configured builder-journey contribution and social-task slugs, and builder eligibility uses accepted ChangesBuilder Ranking Exclusion and Eligibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LeaderboardViewSet
participant SubmittedContribution
participant LeaderboardEntry
LeaderboardViewSet->>SubmittedContribution: _builder_ranking_submissions(state=accepted)
SubmittedContribution-->>LeaderboardViewSet: eligible_builder_submissions
LeaderboardViewSet->>LeaderboardEntry: filter via Exists(eligible_builder_submissions)
LeaderboardEntry-->>LeaderboardViewSet: builder leaderboard rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/leaderboard/models.py`:
- Around line 862-866: The builder contribution aggregation in the leaderboard
logic is still counting all contributions with task category builder, which lets
excluded builder slugs inflate total_points. Update the builder total
calculation in the contribution loop that uses stc['task__category__slug'] and
stc['points_awarded'] so it also skips the same excluded slugs used for builder
eligibility, or refactor to reuse a shared exclusion list. Make sure the guard
is aligned with the existing BUILDER_RANKING_EXCLUDED_SOCIAL_TASK_SLUGS check
and the surrounding leaderboard totals logic in models.py.
In `@backend/leaderboard/tests/test_stats.py`:
- Line 362: The test method
test_builder_lookup_uses_accepted_submission_ranking_eligibility is too large
and triggers the statement-count limit, so split its scenario coverage into
smaller tests. Refactor the existing logic into separate parametrized cases or
helper-driven tests for each variant (welcome-only, simple-only,
github-link-only, contribution-only, accepted) while keeping the shared setup in
reusable helpers so the behavior remains covered without one monolithic test.
- Around line 605-633: The leaderboard expectations in this test are outdated:
the builder entries returned by the relevant lookups are the two accepted
non-excluded submissions with stored ranks 4 and 5. Update the assertions in
test_stats.py around the builder lookup checks so the list response rank
sequence matches [4, 5], and adjust the non_submittable_user-related rank
expectation to 4 in the same test block.
In `@backend/social_tasks/tests/test_leaderboard_integration.py`:
- Around line 55-65: The builder-journey SocialTask setup is duplicated across
multiple tests, so extract the repeated SocialTask.objects.update_or_create
block into a shared helper or fixture and reuse it in the leaderboard
integration tests. Keep the helper centered around the existing builder_journey
task creation logic in test_leaderboard_integration.py, using
settings.BUILDER_JOURNEY_TASK_SLUG and the common defaults so the field list
stays consistent and only one place needs updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ddc5e75-ec2e-4ab9-a339-1570ca8f0b5a
📒 Files selected for processing (6)
backend/leaderboard/models.pybackend/leaderboard/tests/test_stats.pybackend/leaderboard/views.pybackend/social_tasks/tests/test_leaderboard_integration.pybackend/users/tests/test_builder_journey.pybackend/users/views.py
| self.assertEqual(response.data['total_points'], link_points) | ||
|
|
||
| def test_builder_lookup_uses_real_builder_ranking_eligibility(self): | ||
| def test_builder_lookup_uses_accepted_submission_ranking_eligibility(self): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Test method exceeds statement-count threshold (PLR0915).
76 statements in a single test hampers readability and debugging. Consider splitting per-variant scenarios (welcome-only, simple-only, github-link-only, contribution-only, accepted) into separate parametrized/helper-driven tests.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 362-362: Too many statements (76 > 50)
(PLR0915)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/leaderboard/tests/test_stats.py` at line 362, The test method
test_builder_lookup_uses_accepted_submission_ranking_eligibility is too large
and triggers the statement-count limit, so split its scenario coverage into
smaller tests. Refactor the existing logic into separate parametrized cases or
helper-driven tests for each variant (welcome-only, simple-only,
github-link-only, contribution-only, accepted) while keeping the shared setup in
reusable helpers so the behavior remains covered without one monolithic test.
Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/leaderboard/tests/test_stats.py (1)
502-543: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing direct lookup assertion for
contribution_only_user.
_assert_builder_lookup_emptyis called forrole_only_user,welcome_only_user,simple_builder_user, andgithub_link_user, but not forcontribution_only_user— the scenario of a rawContributionwithout an acceptedSubmittedContribution, which is central to this PR's eligibility fix. It's only indirectly checked via the aggregate list exclusion at line 535. Add a direct per-address lookup assertion to close the gap in case the address-scoped endpoint applies different filtering than the list endpoint.✅ Proposed addition
self._assert_builder_lookup_empty(role_only_user) self._assert_builder_lookup_empty(welcome_only_user) self._assert_builder_lookup_empty(simple_builder_user) self._assert_builder_lookup_empty(github_link_user) + self._assert_builder_lookup_empty(contribution_only_user)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/leaderboard/tests/test_stats.py` around lines 502 - 543, The builder stats test is missing a direct address-scoped lookup check for contribution_only_user, so add an explicit _assert_builder_lookup_empty call alongside the existing role_only_user, welcome_only_user, simple_builder_user, and github_link_user assertions in test_stats.py. Use the existing _assert_builder_lookup_empty helper in the same test case to verify that /api/v1/leaderboard/ with user_address for contribution_only_user also returns no builder entry, covering the raw Contribution-only eligibility path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/leaderboard/tests/test_stats.py`:
- Around line 502-543: The builder stats test is missing a direct address-scoped
lookup check for contribution_only_user, so add an explicit
_assert_builder_lookup_empty call alongside the existing role_only_user,
welcome_only_user, simple_builder_user, and github_link_user assertions in
test_stats.py. Use the existing _assert_builder_lookup_empty helper in the same
test case to verify that /api/v1/leaderboard/ with user_address for
contribution_only_user also returns no builder entry, covering the raw
Contribution-only eligibility path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ae2f8d1-ff74-480d-a7fe-dceef1b631ea
📒 Files selected for processing (4)
backend/leaderboard/models.pybackend/leaderboard/tests/test_stats.pybackend/leaderboard/views.pybackend/social_tasks/tests/test_leaderboard_integration.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Tests