Rank OWASP Nest community users using total contributions. #1000
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces a new field, Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (2)
backend/apps/github/models/mixins/user.py (1)
110-117: Optimize database query with aggregationWhile the current implementation correctly calculates the total contributions count, it fetches all repository contributor records and performs the sum in Python. This could be inefficient for users with many repository contributions.
Consider using Django's aggregation functionality to calculate the sum at the database level:
@property def idx_contributions_count(self): """Return contributions count for indexing.""" - contributions_counts = [ - rc.contributions_count for rc in RepositoryContributor.objects.filter(user=self) - ] - return sum(contributions_counts) + from django.db.models import Sum + result = RepositoryContributor.objects.filter(user=self).aggregate( + total=Sum('contributions_count') + ) + return result['total'] or 0This approach performs the calculation in a single database query rather than loading all records into memory.
backend/apps/github/index/user.py (1)
44-47: Consider documenting the ranking criteriaThe custom ranking now prioritizes contributions count, followed by creation date and followers count. This logic is important for understanding how users are presented in the UI.
Consider adding a comment to explain the ranking priority:
"customRanking": [ "desc(idx_contributions_count)", # Prioritize users with more contributions "desc(idx_created_at)", # Then by account age (newer first) "desc(idx_followers_count)", # Then by popularity ],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/index/user.py(2 hunks)backend/apps/github/models/mixins/user.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (3)
backend/apps/github/index/user.py (3)
19-19: Field addition aligns with PR goal - looks good!The addition of the
idx_contributions_countfield to the index configuration enables the indexing of user contribution counts, which is essential for the new ranking feature.
44-44: Custom ranking implementation matches requirementsPlacing
desc(idx_contributions_count)at the top of the custom ranking list correctly prioritizes users with higher contribution counts in search results, which aligns perfectly with the PR objective of ranking OWASP Nest community users based on their total contributions.
19-19:❓ Verification inconclusive
Verify the impact of the contribution count ranking
The changes implement the requested feature, but it would be beneficial to understand how this affects the actual user ranking in the UI. Consider testing with a representative dataset to ensure the ranking reflects the expected order of users.
Also applies to: 44-44
🏁 Script executed:
#!/bin/bash # Check if there are any tests that verify the ranking behavior echo "Searching for user index or ranking tests..." fd "test.*user.*\.(py|js)" --exclude node_modules echo "Looking for user ranking logic on frontend..." rg -A 3 -B 3 "contribution|ranking" --glob "frontend/**/*.{js,jsx,ts,tsx}"Length of output: 26046
Action: Validate UI Ranking Consistency with Contribution Count
The updated index
"idx_contributions_count"appears to integrate the contribution count into our ranking mechanism correctly. However, as our frontend mock data (e.g., in various test files) relies on thecontributionsCountfield, please verify that the actual user ranking in the UI reflects the intended order when using a representative dataset. In other words, ensure that users are sorted as expected according to their contributions. This check is applicable to both the change at line 19 and the corresponding update at line 44 inbackend/apps/github/index/user.py.
| contributions_counts = [ | ||
| rc.contributions_count for rc in RepositoryContributor.objects.filter(user=self) | ||
| ] | ||
| return sum(contributions_counts) |
There was a problem hiding this comment.
Could you do it with Django ORM only? It's faster.
There was a problem hiding this comment.
Thank you for the feedback.
|
Resolves #973
idx_contributions_countto theUserIndexMixinthat returns the total number of contributions made by the contributor to OWASP.UserIndex.customRanking.