Improved OWASP Nest member ranking algorithm#4273
Open
anurag2787 wants to merge 4 commits intoOWASP:mainfrom
Open
Improved OWASP Nest member ranking algorithm#4273anurag2787 wants to merge 4 commits intoOWASP:mainfrom
anurag2787 wants to merge 4 commits intoOWASP:mainfrom
Conversation
Contributor
WalkthroughAdds a persisted per-user Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 13 files
Confidence score: 3/5
- There is some concrete regression risk in
backend/apps/github/management/commands/github_sync_user.py: deduplicating committee memberships byentity_idalone (withoutentity_type) can undercount memberships for Generic FK records and affect user-facing counts. backend/apps/owasp/utils/scoring.pyhas a high-confidence logic bug where the 52-week window can span 53 ISO week buckets, allowing consistency scores above the documented 0–100 range; this can produce incorrect displayed/scored results unless capped.- The remaining issues are moderate but still relevant:
docker-compose/local/compose.yamlvolume renaming can reset local developer DB state, and the nullable descending index inbackend/apps/github/migrations/0045_user_calculated_score_user_idx_calc_score_desc.pymay misorder leaderboard rows with NULL scores. - Pay close attention to
backend/apps/github/management/commands/github_sync_user.py,backend/apps/owasp/utils/scoring.py,docker-compose/local/compose.yaml, andbackend/apps/github/migrations/0045_user_calculated_score_user_idx_calc_score_desc.py- membership counting correctness, score range enforcement, local data continuity, and NULL sort behavior are the main risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker-compose/local/compose.yaml">
<violation number="1" location="docker-compose/local/compose.yaml:70">
P2: Renaming the local Postgres volume from `db-data` to `db-data-4200` will cause Docker Compose to create a fresh empty database volume on the next startup, stranding existing developer data in the orphaned old volume. Developers will see their local databases and fixtures appear to disappear without explanation. Since this PR is about the member ranking algorithm, this infrastructure change appears unrelated and lacks documentation or migration guidance. Consider reverting to `db-data` or adding documentation explaining the need for this change and how developers can migrate their existing data.</violation>
</file>
<file name="backend/apps/github/management/commands/github_sync_user.py">
<violation number="1" location="backend/apps/github/management/commands/github_sync_user.py:550">
P2: Committee membership counting deduplicates only by entity_id, which can undercount memberships across different entity types. When using Django's Generic Foreign Key (entity_type + entity_id), distinct() on entity_id alone is insufficient as different content types may share the same primary key value. Should use distinct on both entity_type and entity_id (e.g., `.distinct("entity_type", "entity_id")` or restructure query).</violation>
</file>
<file name="backend/tests/apps/owasp/utils/scoring_test.py">
<violation number="1" location="backend/tests/apps/owasp/utils/scoring_test.py:176">
P2: Test name/docstring claims to test "exactly at 90-day boundary" but tests 89 days, leaving off-by-one case unverified</violation>
</file>
<file name="backend/apps/owasp/utils/scoring.py">
<violation number="1" location="backend/apps/owasp/utils/scoring.py:130">
P2: Consistency score can exceed 100: A 52-week lookback window can span 53 ISO week buckets around year boundaries, causing the consistency score to exceed its documented 0-100 range. Add `min(100.0, ...)` to cap the result, following the pattern used by other scoring functions.</violation>
</file>
<file name="backend/apps/github/migrations/0045_user_calculated_score_user_idx_calc_score_desc.py">
<violation number="1" location="backend/apps/github/migrations/0045_user_calculated_score_user_idx_calc_score_desc.py:16">
P2: Nullable `calculated_score` field with descending index can cause incorrect leaderboard ordering. PostgreSQL sorts NULLs first in descending order unless NULLS LAST is specified. Consider removing `null=True` since there's already a default of 0.0, or ensure ordering queries explicitly handle nulls with `.order_by(F('calculated_score').desc(nulls_last=True))`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/apps/github/migrations/0045_user_calculated_score_user_idx_calc_score_desc.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/apps/owasp/utils/scoring_test.py">
<violation number="1" location="backend/tests/apps/owasp/utils/scoring_test.py:175">
P2: Boundary test no longer exercises the actual 90-day recency cutoff, so an off-by-one bug at exactly 90 days would go undetected. The test name still claims to cover the 'boundary at 90 days' but now tests 89 days instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/api/rest/v0/member_test.py (1)
59-60:⚠️ Potential issue | 🟡 MinorDocstring is inconsistent with the test assertion.
The docstring states the default is
'-created_at', but the test now correctly asserts'-calculated_score'. Update the docstring to match the new behavior.📝 Suggested fix
`@patch`("apps.api.rest.v0.member.UserModel") def test_list_members_no_ordering(self, mock_user_model: MagicMock) -> None: - """Test listing members without ordering applies default '-created_at'.""" + """Test listing members without ordering applies default '-calculated_score'.""" mock_request = MagicMock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/member_test.py` around lines 59 - 60, Update the docstring in the test_list_members_no_ordering function to match the actual assertion: replace the current mention of the default ordering '-created_at' with '-calculated_score' so the test docstring accurately reflects the behavior being asserted in the test.
🧹 Nitpick comments (3)
backend/tests/apps/api/rest/v0/member_test.py (1)
37-50: Consider adding an assertion forcalculated_score.The test adds
calculated_scoretomember_databut doesn't verify its value in the assertions block. While the schema will validate the field during construction, an explicit assertion would improve test completeness.✨ Suggested addition
assert member.twitter_username == member_data["twitter_username"] assert member.updated_at == datetime.fromisoformat(updated_at) assert member.url == member_data["url"] + assert member.calculated_score == member_data["calculated_score"] assert not hasattr(member, "email")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/member_test.py` around lines 37 - 50, The test constructs member_data with a calculated_score but never asserts it; add an assertion in the existing assertions block to verify member.calculated_score equals member_data["calculated_score"] (or an appropriate typed/rounded comparison if the value is a float) so the test explicitly validates the calculated_score field on the Member instance.backend/apps/owasp/utils/scoring.py (1)
128-131: Redundant rounding in _consistency_score.The
round(..., 2)here is unnecessary sincecalculate_member_scorealready rounds the final weighted sum. This double rounding could cause minor precision artifacts.🔧 Suggested simplification
- return min(100.0, round(len(active_weeks) / CONSISTENCY_WEEKS * 100.0, 2)) + return min(100.0, len(active_weeks) / CONSISTENCY_WEEKS * 100.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/utils/scoring.py` around lines 128 - 131, The _consistency_score function redundantly applies round(..., 2) before calculate_member_score also rounds the final weighted sum; remove the inner rounding so _consistency_score returns min(100.0, len(active_weeks) / CONSISTENCY_WEEKS * 100.0) (still capping with min) to avoid double-rounding artifacts—update the return expression in _consistency_score that currently references active_weeks and CONSISTENCY_WEEKS and ensure calculate_member_score continues to perform the final rounding.backend/tests/apps/owasp/utils/scoring_test.py (1)
364-391: Test assertion may be fragile depending on weight balance.The test
test_contributions_are_primary_factorassertsscore_with_contributions > score_with_leadership, butscore_with_leadershipincludes board member (50 pts) + GSoC mentor (10 pts) + 5 chapter leaders (75 pts) + 5 project leaders (100 pts) + 5 committee members (50 pts) = 285 pts, capped to 80. With weights: leadership contributes0.25 * 80 = 20, while contributions at 100 count contribute0.35 * ~66.4 ≈ 23.3(log2(101) * 10 ≈ 66.4). The assertion holds, but it's testing the weighting rather than "contributions are primary factor." The test name may be misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/utils/scoring_test.py` around lines 364 - 391, The test test_contributions_are_primary_factor is misleading because it compares full scores instead of the specific contribution vs leadership components; update the test for clarity by either renaming it to reflect that it compares overall scores (e.g., test_contributions_yield_higher_overall_score_in_this_scenario) or change the assertion to compare the contribution-derived portion to the leadership-derived portion directly by extracting the contribution and leadership components from calculate_member_score (or adding a helper that returns both components) so the test asserts contribution_component > leadership_component while keeping the same input values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/github/management/commands/github_sync_user.py`:
- Around line 515-521: The except is catching AttributeError but accessing the
reverse OneToOne relation user.owasp_profile raises a RelatedObjectDoesNotExist
(subclass of ObjectDoesNotExist); update the try/except in github_sync_user.py
around the user.owasp_profile access to catch either the specific
RelatedObjectDoesNotExist for that relation or the broader
django.core.exceptions.ObjectDoesNotExist (or import and reference
user.owasp_profile.RelatedObjectDoesNotExist) and then set is_board_member =
False and is_gsoc_mentor = False in that except block so missing reverse
OneToOne relations are handled correctly.
- Around line 550-559: The committee_member_count query is including CANDIDATE
roles; update the EntityMember filter to only count confirmed roles by adding a
role filter (e.g. role__in=[EntityMember.Role.MEMBER, EntityMember.Role.LEADER]
or otherwise exclude EntityMember.Role.CANDIDATE) in the committee_member_count
queryset so it matches the explicit role filtering used for chapter_leader_count
and project_leader_count.
- Around line 561-567: The current aggregation uses
contribution_data.update(...) which overwrites duplicate date keys from
MemberSnapshot.contribution_heatmap_data; change aggregation in the
github_sync_user logic to accumulate counts instead: initialize
contribution_data as a numeric accumulator (e.g., collections.Counter or
defaultdict(int)), iterate each snapshot from
MemberSnapshot.objects.filter(...).values("contribution_heatmap_data"), and for
each date/key in snapshot["contribution_heatmap_data"] add its integer count to
contribution_data[date]; ensure this produces the summed heatmap used by
_recency_score() and _consistency_score().
In `@backend/tests/apps/github/management/commands/github_sync_user_test.py`:
- Around line 986-1025: The test test_calculate_member_score_missing_profile
incorrectly sets mock_user.owasp_profile = None; instead simulate Django's
missing OneToOne reverse relation by making attribute access raise
django.core.exceptions.ObjectDoesNotExist so the production handling is
exercised: in the test (test_calculate_member_score_missing_profile) import
ObjectDoesNotExist and PropertyMock and set type(mock_user).owasp_profile =
PropertyMock(side_effect=ObjectDoesNotExist()) before calling
command.calculate_member_score(mock_user); this change ensures the
calculate_member_score code path that expects an ObjectDoesNotExist is triggered
(and reveals if it wrongly catches AttributeError).
In `@docker-compose/local/compose.yaml`:
- Line 70: The compose volume name was changed to "db-data-4200", which will
orphan existing developer volumes and cause local data loss; revert any
occurrences of "db-data-4200" back to the original "db-data" unless this rename
is intentionally required, or explicitly document in the PR why a destructive
rename is needed; update all instances of the string "db-data-4200" (both the
service volume entries and the volumes list) to "db-data" or add a clear PR note
explaining the forced new volume name.
---
Outside diff comments:
In `@backend/tests/apps/api/rest/v0/member_test.py`:
- Around line 59-60: Update the docstring in the test_list_members_no_ordering
function to match the actual assertion: replace the current mention of the
default ordering '-created_at' with '-calculated_score' so the test docstring
accurately reflects the behavior being asserted in the test.
---
Nitpick comments:
In `@backend/apps/owasp/utils/scoring.py`:
- Around line 128-131: The _consistency_score function redundantly applies
round(..., 2) before calculate_member_score also rounds the final weighted sum;
remove the inner rounding so _consistency_score returns min(100.0,
len(active_weeks) / CONSISTENCY_WEEKS * 100.0) (still capping with min) to avoid
double-rounding artifacts—update the return expression in _consistency_score
that currently references active_weeks and CONSISTENCY_WEEKS and ensure
calculate_member_score continues to perform the final rounding.
In `@backend/tests/apps/api/rest/v0/member_test.py`:
- Around line 37-50: The test constructs member_data with a calculated_score but
never asserts it; add an assertion in the existing assertions block to verify
member.calculated_score equals member_data["calculated_score"] (or an
appropriate typed/rounded comparison if the value is a float) so the test
explicitly validates the calculated_score field on the Member instance.
In `@backend/tests/apps/owasp/utils/scoring_test.py`:
- Around line 364-391: The test test_contributions_are_primary_factor is
misleading because it compares full scores instead of the specific contribution
vs leadership components; update the test for clarity by either renaming it to
reflect that it compares overall scores (e.g.,
test_contributions_yield_higher_overall_score_in_this_scenario) or change the
assertion to compare the contribution-derived portion to the leadership-derived
portion directly by extracting the contribution and leadership components from
calculate_member_score (or adding a helper that returns both components) so the
test asserts contribution_component > leadership_component while keeping the
same input values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1f2e640-d50b-4caf-8db9-f94378ebcf56
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (12)
backend/apps/api/rest/v0/member.pybackend/apps/github/api/internal/nodes/user.pybackend/apps/github/index/registry/user.pybackend/apps/github/management/commands/github_sync_user.pybackend/apps/github/migrations/0045_user_calculated_score_user_idx_calc_score_desc.pybackend/apps/github/models/mixins/user.pybackend/apps/github/models/user.pybackend/apps/owasp/utils/scoring.pybackend/tests/apps/api/rest/v0/member_test.pybackend/tests/apps/github/management/commands/github_sync_user_test.pybackend/tests/apps/owasp/utils/scoring_test.pydocker-compose/local/compose.yaml
|
Contributor
Author
|
Hi @arkid15r the pr is ready for review |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/apps/github/management/commands/github_sync_user.py (1)
488-498: Consider using a more specific exception type.The broad
except Exceptioncatch at line 496 will catch all exceptions, including programming errors (e.g.,TypeError,AttributeError) that might indicate bugs in the scoring logic. This could mask issues during development and testing.Consider catching more specific exceptions or at minimum logging at ERROR level for unexpected exceptions:
💡 Optional: More targeted exception handling
# Calculate and update member ranking score try: score = self.calculate_member_score(user) user.calculated_score = score user.save(update_fields=["calculated_score"]) self.stdout.write(self.style.SUCCESS(f"Updated member ranking score: {score:.2f}")) logger.info("Updated member ranking score for %s: %.2f", username, score) - except Exception as e: - self.stderr.write(self.style.WARNING(f"Error calculating member score: {e}")) - logger.exception("Error calculating member score for %s", username) + except (ObjectDoesNotExist, ValueError) as e: + self.stderr.write(self.style.WARNING(f"Error calculating member score: {e}")) + logger.warning("Error calculating member score for %s: %s", username, e) + except Exception as e: + self.stderr.write(self.style.ERROR(f"Unexpected error calculating member score: {e}")) + logger.exception("Unexpected error calculating member score for %s", username)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/github/management/commands/github_sync_user.py` around lines 488 - 498, The current broad except Exception around the calculate/update block for member ranking (the call to calculate_member_score(user), setting user.calculated_score and user.save) is too wide; narrow it to catch only expected/validation errors (e.g., ValueError or your domain-specific errors raised by calculate_member_score) and handle them by logging and continuing, and add a separate catch-all that logs at ERROR and re-raises unexpected exceptions so programming errors aren’t swallowed; specifically, replace the single except Exception block with an except (ValueError, YourDomainScoreError) as e: (write warning and continue) and an except Exception as e: logger.error("Unexpected error calculating member score for %s: %s", username, e, exc_info=True); raise to surface real bugs.backend/tests/apps/owasp/utils/scoring_test.py (1)
350-362: Decimal place assertion may be fragile for edge cases.The assertion
len(str(score).split(".")[-1]) <= 2works for most cases but can be unreliable:
- If
scoreis exactly0.0,str(0.0)is"0.0"which gives["0", "0"][-1]="0"(1 char) ✓- If
scoreis10.0, it gives"0"(1 char) ✓- However, if
scorehappens to be an integer-like float (e.g.,10.0), this passes but doesn't truly verify roundingA more robust approach would check the actual rounding:
💡 Optional: More precise rounding verification
def test_score_is_rounded_to_two_decimals(self): """Test that score is rounded to 2 decimal places.""" score = calculate_member_score( contributions_count=5, distinct_repository_count=2, chapter_leader_count=0, project_leader_count=0, committee_member_count=0, is_board_member=False, is_gsoc_mentor=False, contribution_data=None, ) - assert len(str(score).split(".")[-1]) <= 2 + assert score == round(score, 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/utils/scoring_test.py` around lines 350 - 362, The test test_score_is_rounded_to_two_decimals should assert that the returned value from calculate_member_score is equal to its value rounded to two decimals; replace the fragile string-length check with an assertion like score == round(score, 2) so the test reliably verifies the function calculate_member_score produces a value rounded to two decimal places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/apps/github/management/commands/github_sync_user.py`:
- Around line 488-498: The current broad except Exception around the
calculate/update block for member ranking (the call to
calculate_member_score(user), setting user.calculated_score and user.save) is
too wide; narrow it to catch only expected/validation errors (e.g., ValueError
or your domain-specific errors raised by calculate_member_score) and handle them
by logging and continuing, and add a separate catch-all that logs at ERROR and
re-raises unexpected exceptions so programming errors aren’t swallowed;
specifically, replace the single except Exception block with an except
(ValueError, YourDomainScoreError) as e: (write warning and continue) and an
except Exception as e: logger.error("Unexpected error calculating member score
for %s: %s", username, e, exc_info=True); raise to surface real bugs.
In `@backend/tests/apps/owasp/utils/scoring_test.py`:
- Around line 350-362: The test test_score_is_rounded_to_two_decimals should
assert that the returned value from calculate_member_score is equal to its value
rounded to two decimals; replace the fragile string-length check with an
assertion like score == round(score, 2) so the test reliably verifies the
function calculate_member_score produces a value rounded to two decimal places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3398edc-a794-4584-ba0a-41b70fa5fb25
📒 Files selected for processing (3)
backend/apps/github/management/commands/github_sync_user.pybackend/tests/apps/github/management/commands/github_sync_user_test.pybackend/tests/apps/owasp/utils/scoring_test.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.
Proposed change
This PR improves the OWASP Nest member ranking by introducing a calculated_score.
Instead of ranking members only by total contributions, the score also considers factors like recent activity, leadership roles, and involvement across OWASP projects.
Resolves #4200
Checklist
make check-testlocally: all warnings addressed, tests passed