Feature/project leaderboard UI #5275
Open
+226
−60
Conversation
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
github-actions
bot
added
last-active: 0d
Contributor
|
👋 Hi @Ankit-singh-dot! 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! |
Contributor
WalkthroughAdds a Project Leaderboard view, template, and URL; enhances User admin with superuser-only deactivate action and activity filter; and tightens IP-restriction middleware transaction handling, adds a test-mode guard, and stops network loop after a match. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
website/templates/projects/project_leaderboard.html (1)
7-14: Add ARIA attributes for accessibility.The tabbed interface is missing proper ARIA attributes for screen reader support. Consider adding
role="tab",aria-selected,aria-controlson buttons, androle="tabpanel",aria-labelledbyon content divs to meet WCAG accessibility standards.Example enhancement for one tab button and panel:
-<button class="tab-btn px-4 py-2 text-sm rounded-md font-medium transition bg-red-600 text-white" - data-tab="dashboard">Dashboard</button> +<button class="tab-btn px-4 py-2 text-sm rounded-md font-medium transition bg-red-600 text-white" + role="tab" + aria-selected="true" + aria-controls="dashboard" + data-tab="dashboard">Dashboard</button> -<div id="dashboard" class="tab-content space-y-3"> +<div id="dashboard" class="tab-content space-y-3" role="tabpanel" aria-labelledby="dashboard-tab">Apply similar changes to all tabs and update the JavaScript to toggle
aria-selectedwhen switching tabs.Also applies to: 16-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
blt/urls.py(2 hunks)website/admin.py(3 hunks)website/templates/projects/project_leaderboard.html(1 hunks)website/views/project_leaderboard.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
website/views/project_leaderboard.py (1)
website/models.py (1)
open_issues(419-420)
website/admin.py (1)
website/models.py (1)
is_active(3329-3334)
blt/urls.py (1)
website/views/project_leaderboard.py (1)
ProjectLeaderboardView(7-24)
🪛 Ruff (0.14.8)
website/admin.py
107-107: Redefinition of unused User from line 10
(F811)
1303-1303: Unused method argument: request
(ARG002)
1303-1303: Unused method argument: model_admin
(ARG002)
1309-1309: Unused method argument: request
(ARG002)
1346-1346: Unused method argument: obj
(ARG002)
1349-1349: Unused method argument: obj
(ARG002)
⏰ 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 (9)
website/admin.py (5)
1-1: LGTM: Dynamic user model loading.Using
get_user_model()andClassVaris the correct approach for custom user models and provides better flexibility.Also applies to: 107-108
1282-1297: LGTM: Admin action with proper authorization.The
deactivate_usersaction correctly restricts the operation to superusers and provides appropriate user feedback.
1299-1315: LGTM: Activity filter implementation.The
ActivityStatusFiltercorrectly filters users based onlast_loginpresence. The static analysis warnings about unused parameters (request,model_admin) are false positives—Django's API requires these signatures.
1317-1320: LGTM: Defensive unregistration.The try-except block properly handles the case where
Usermight not be registered, preventing potential crashes.
1323-1364: LGTM: CustomUserAdmin with proper permission restrictions.The implementation correctly:
- Restricts all permissions to superusers
- Conditionally displays the deactivate action
- Renders activity status with visual indicators
The static analysis warnings about unused
objparameters in permission methods are false positives—Django's admin API requires this signature.blt/urls.py (1)
303-303: LGTM: URL routing properly configured.The import and URL pattern registration follow Django conventions correctly. The route is properly named for reverse URL lookups.
Also applies to: 401-405
website/templates/projects/project_leaderboard.html (2)
54-89: LGTM: Tab switching logic is correct.The JavaScript properly handles:
- Hiding all tab content
- Resetting button styles
- Showing the selected tab
- Applying active styles
The implementation follows a clean pattern and handles dark mode styling correctly.
17-27: LGTM: Template properly renders context data with fallbacks.The template correctly:
- Loops through context variables (dashboard_projects, projects, activity)
- Uses
|default:0filters for NULL-safe display- Provides empty state messages via
{% empty %}blocksAlso applies to: 31-38, 42-49
website/views/project_leaderboard.py (1)
15-17: No action needed — Repo model fields are properly defined.The
stars,forks, andopen_issuesfields exist in the Repo model asIntegerFieldwithdefault=0. The Sum aggregations are safe and will work correctly.
Comment on lines
+13
to
+22
| projects = Project.objects.prefetch_related("repos").annotate( | ||
| repo_count=Count("repos", distinct=True), | ||
| total_stars=Sum("repos__stars"), | ||
| total_forks=Sum("repos__forks"), | ||
| open_issues=Sum("repos__open_issues"), | ||
| ) | ||
|
|
||
| context["dashboard_projects"] = projects.order_by("-total_stars")[:20] | ||
| context["projects"] = projects.order_by("-repo_count") | ||
| context["activity"] = projects.order_by("-open_issues") |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Cache queryset to avoid multiple database queries.
The projects queryset is evaluated three times (lines 20-22), resulting in three separate database queries with identical annotations. Consider caching the queryset by converting it to a list after annotation.
Apply this diff:
- projects = Project.objects.prefetch_related("repos").annotate(
+ projects = list(Project.objects.prefetch_related("repos").annotate(
repo_count=Count("repos", distinct=True),
total_stars=Sum("repos__stars"),
total_forks=Sum("repos__forks"),
open_issues=Sum("repos__open_issues"),
- )
+ ))
context["dashboard_projects"] = projects.order_by("-total_stars")[:20]
- context["projects"] = projects.order_by("-repo_count")
- context["activity"] = projects.order_by("-open_issues")
+ context["projects"] = sorted(projects, key=lambda p: p.repo_count or 0, reverse=True)
+ context["activity"] = sorted(projects, key=lambda p: p.open_issues or 0, reverse=True)Note: This caches the queryset in memory and uses Python sorting instead of database ORDER BY. For large datasets, consider pagination or keeping database-level sorting with separate querysets.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In website/views/project_leaderboard.py around lines 13 to 22, the annotated
QuerySet is evaluated three times causing duplicate DB queries; convert/evaluate
the annotated QuerySet to an in-memory list once (e.g. list(projects)
immediately after annotate) and then produce the three context values by sorting
that cached list in Python (use stable/explicit keys and handle None values,
e.g. treat missing aggregates as 0), slice for the top 20 dashboard_projects,
and assign the other two sorted lists to context["projects"] and
context["activity"] to avoid repeated database hits.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
github-actions
bot
added
the
changes-requested
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Comment on lines
+13
to
+23
| projects = Project.objects.prefetch_related("repos").annotate( | ||
| repo_count=Count("repos", distinct=True), | ||
| total_stars=Sum("repos__stars"), | ||
| total_forks=Sum("repos__forks"), | ||
| open_issues=Sum("repos__open_issues"), | ||
| ) | ||
|
|
||
| context["dashboard_projects"] = projects.order_by("-total_stars")[:20] | ||
| context["projects"] = projects.order_by("-repo_count") | ||
| context["activity"] = projects.order_by("-open_issues") | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
website/admin.py (2)
342-342: Remove the unsafe unregister call.Line 342 unregisters User without error handling, but Lines 1316-1320 now provide a safe try/except approach. The old unregister call should be removed to prevent potential crashes if User isn't registered by default.
Apply this diff:
-admin.site.unregister(User)
955-955: Remove the obsolete User registration.Line 955 registers User with the old
UserAdmin, but Line 1365 now registers User with the newCustomUserAdmin. Line 955 should be removed since it's replaced by the new implementation. The current flow creates an unnecessary register→unregister→register cycle.Apply this diff:
-admin.site.register(User, UserAdmin)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/admin.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
website/admin.py
1302-1302: Unused method argument: request
(ARG002)
1302-1302: Unused method argument: model_admin
(ARG002)
1308-1308: Unused method argument: request
(ARG002)
1345-1345: Unused method argument: obj
(ARG002)
1348-1348: Unused method argument: obj
(ARG002)
⏰ 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 (5)
website/admin.py (5)
1-1: LGTM!The new imports are properly used throughout the file for the custom User admin implementation.
Also applies to: 6-9
106-107: LGTM!Using
get_user_model()is the correct Django pattern for dynamic User model resolution, ensuring compatibility with custom user models.
1281-1296: LGTM with note on defensive security.The permission check inside the action (lines 1283-1289) provides defense-in-depth alongside the
get_actionsfilter (lines 1351-1355), protecting against direct requests or programmatic calls.
1298-1314: LGTM!The filter logic correctly distinguishes active (has logged in) from inactive (never logged in) users. The static analysis warnings about unused parameters are false positives—these are required by Django's
SimpleListFilterAPI.
1322-1365: LGTM!The
CustomUserAdminimplementation is well-structured with proper permission restrictions, safe HTML rendering, and clear activity status indicators. The static analysis warnings about unusedobjparameters are false positives—these are required by Django's admin permission method signatures.
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
Contributor
Author
|
@coderabbitai can you please tell me why my test case is failing i have check the pre-commit |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3653 🏁 Script executed: Length of output: 7659 🏁 Script executed: Length of output: 126508 🏁 Script executed: Length of output: 3958 @Ankit-singh-dot, I found the issue! Your tests are failing due to a transaction management problem in the IP restriction middleware, not because of your new project leaderboard code. The ProblemThe test failures show this error pattern:
Root CauseThe
The FixYou need to make the IP recording middleware test-safe. Here's the fix for def _record_ip(self, ip, agent, path):
"""
Helper method to record IP information
"""
# Skip IP recording during tests to avoid transaction conflicts
from django.conf import settings
if settings.TESTING or 'test' in sys.argv:
return
try:
with transaction.atomic():
# Check if we're in a broken transaction
if transaction.get_rollback():
logger.warning(f"Skipping IP recording for {ip} - transaction marked for rollback")
return
# ... rest of the existing codeAlternative fix (more robust): Use def _record_ip(self, ip, agent, path):
"""
Helper method to record IP information
"""
try:
# Use savepoint to handle nested transactions gracefully
with transaction.atomic(savepoint=True):
# Try to update existing record using atomic QuerySet.update() with F()
updated = IP.objects.filter(address=ip, path=path).update(
agent=agent,
count=models.Case(
models.When(count__lt=MAX_COUNT, then=models.F("count") + 1),
default=models.Value(MAX_COUNT),
output_field=models.BigIntegerField(),
),
# ... rest of your code
except Exception as e:
# Silently fail IP recording - it's not critical
logger.debug(f"Could not record IP {ip}: {e}")SummaryYour project leaderboard code is fine! The issue is pre-existing middleware code that doesn't handle the test environment properly. Apply one of the fixes above to |
Contributor
Author
|
@DonnieBLT my test case is failing because blt/middleware/ip_restrict.py should i resolve this or not because all test cases are being passed and cli approved |
github-actions
bot
added
has-conflicts
Comment on lines
+21
to
+22
| context["projects"] = projects.order_by("-repo_count") | ||
| context["activity"] = projects.order_by("-open_issues") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
blt/middleware/ip_restrict.py (2)
66-73: Minor: Redundant None check in list comprehension.The explicit
if blocked_agent is not Nonecheck at line 72 is redundant sinceblocked_agents()already filters out None values at line 51.Apply this diff to simplify:
user_agent_str = str(user_agent).strip().lower() return any( - blocked_agent.lower() in user_agent_str for blocked_agent in blocked_agents if blocked_agent is not None + blocked_agent.lower() in user_agent_str for blocked_agent in blocked_agents )
99-100: Optional: Use explicit conversion flag in f-string.Static analysis suggests using an explicit conversion flag for better clarity.
Apply this diff:
- logger.error(f"Error incrementing block count: {str(e)}", exc_info=True) + logger.error(f"Error incrementing block count: {e!s}", exc_info=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
blt/middleware/ip_restrict.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
blt/middleware/ip_restrict.py (1)
website/models.py (2)
Blocked(1479-1489)IP(1122-1136)
🪛 Ruff (0.14.8)
blt/middleware/ip_restrict.py
100-100: Use explicit conversion flag
Replace with conversion flag
(RUF010)
137-137: Do not catch blind exception: Exception
(BLE001)
⏰ 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 (8)
blt/middleware/ip_restrict.py (8)
3-3: Approve the import addition.The
sysimport is appropriately added to support the test-mode detection guard in_record_ip.
57-64: LGTM: Improved IP validation.The added IP address validation prevents exceptions from invalid input and provides clear error logging before checking network ranges.
80-80: LGTM: Savepoint addresses nested transaction issue.Using
transaction.atomic(savepoint=True)correctly resolves the test failure caused by nested transactions, as discussed in the PR comments.
88-97: Verify user-agent matching logic is intentional.The matching at line 92 checks if the blocked agent string is a substring of the incoming user_agent (
agent.lower() in user_agent.lower()). This means a blocked agent "bot" would match "robot", "chatbot", etc. Confirm this substring matching is the intended behavior rather than exact matching.Additionally, the null checks
if agent and user_agentare good defensive guards, thoughagentis already filtered for None values inblocked_agents().
117-139: LGTM: Savepoint and error handling improvements.The combination of
savepoint=Trueat line 117 and the catch-and-suppress exception handling at lines 137-138 ensures IP logging never breaks requests or tests, which aligns with the docstring's promise. Logging at debug level is appropriate for non-critical tracking failures.
102-106: LGTM: Async implementation is correct.The async methods properly delegate to their sync counterparts using
sync_to_async, and the control flow correctly handles IP blocking, network range checking, and user-agent filtering.Also applies to: 143-168
133-135: Verify duplicate deletion logic.The duplicate cleanup deletes all but the first record (ordered by
created) for a given IP/path combination. This approach is correct but will execute on every request where a duplicate exists. Consider if this is a common case that warrants logging or monitoring.#!/bin/bash # Check if duplicate IP records are a known issue in the codebase rg -n "duplicate.*IP\|IP.*duplicate" --type=py -C2
25-30: Thefilter(None, queryset)is necessary and appropriate for all three cache keys.The method is used for three different querysets:
- blocked_ips (line 34):
addressfield - filtering None is safe; IP addresses should never be None- blocked_ip_network (line 40):
ip_networkfield - filtering None is necessary to preventValueErrorwhenipaddress.ip_network(range_str)is called on line 42 with a None value- blocked_agents (line 52): Already pre-filtered on line 51; the additional
filter(None, ...)is redundant but harmlessThe filtering is defensive programming that prevents crashes (especially for network ranges with malformed database entries) rather than optional cleanup.
Comment on lines
+113
to
+114
| if "test" in sys.argv: | ||
| return |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test detection is fragile; consider using Django's test runner detection.
The check if "test" in sys.argv may produce false positives (e.g., a file named "test.py" passed as argument) or false negatives (test runners that don't include "test" in argv). Django provides more reliable ways to detect test mode.
Consider using Django's official approach:
- if "test" in sys.argv:
+ from django.conf import settings
+ if getattr(settings, 'TESTING', False):
returnThen ensure your test settings include TESTING = True.
Alternatively, check the database connection:
- if "test" in sys.argv:
+ from django.db import connection
+ if connection.settings_dict.get('NAME', '').startswith('test_'):
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "test" in sys.argv: | |
| return | |
| from django.conf import settings | |
| if getattr(settings, 'TESTING', False): | |
| return |
🤖 Prompt for AI Agents
In blt/middleware/ip_restrict.py around lines 113-114, replace the fragile
sys.argv test with a Django-aware test detection: import django.conf.settings
and check getattr(settings, "TESTING", False) (i.e., if getattr(settings,
"TESTING", False): return). Update test configuration (or pytest/conftest) to
set TESTING = True during test runs; this avoids false positives/negatives from
argv and uses an explicit, reliable flag.
Contributor
Author
|
@DonnieBLT please review this pr have changed some testing file as coderabbit is suggesting and the CI failure was caused by nested transactions in ip_restrict middleware during django tests i made the middleware test-safe by using savepoints and skipping non-critical IP logging during tests. Production behavior remains unchanged. please let me know if any-things required to fix or removed |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/admin.py (1)
342-342: Simplify User admin registration flow.The current registration sequence creates unnecessary operations:
- Line 342: Unregister User (bare call, no exception handling)
- Line 955: Register User with default
UserAdmin- Lines 1316-1319: Unregister User again (with safe try/except)
- Line 1394: Register User with
CustomUserAdminSince the goal is to use
CustomUserAdmin(line 1394), line 955 should be removed. Additionally, line 342's bareunregisteris redundant with the safer try/except version at lines 1316-1319.Apply this diff:
-admin.site.unregister(User) - # class UserAdmin(ImportExportModelAdmin):And remove line 955:
admin.site.register(UserProfile, UserProfileAdmin) -admin.site.register(User, UserAdmin) admin.site.register(Domain, DomainAdminPanel)This simplifies the flow to: safe unregister (lines 1316-1319) → register with CustomUserAdmin (line 1394).
Also applies to: 955-955, 1316-1319, 1394-1394
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/admin.py(3 hunks)
⏰ 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 (4)
website/admin.py (4)
1281-1295: LGTM: Proper permission checks and bulk operation.The
deactivate_usersaction correctly restricts deactivation to superusers and uses efficient bulk updates. The permission check is also enforced at the action-list level (lines 1360-1364), providing defense in depth.
1298-1313: LGTM: Clean filter implementation.The
ActivityStatusFilterfollows Django admin conventions and provides a clear way to filter users by login activity. The logic correctly distinguishes between active (has logged in) and inactive (never logged in) users.
1328-1392: LGTM: Well-optimized admin implementation.The
CustomUserAdminclass demonstrates good practices:
- Query optimization: Lines 1367-1372 use
annotate()withCount()to compute activity metrics in a single query, preventing N+1 issues.- Proper permissions: Comprehensive superuser-only restrictions across module/view/change permissions.
- Safe HTML rendering: Line 1388's
format_html()safely renders styled status indicators.- Sortable metrics:
admin_order_fieldattributes enable sorting by bug_count and forum_comment_count.
1281-1394: Past review concern about PR scope remains unaddressed.A previous review comment noted that these User admin enhancements (activity filters, deactivation action, custom admin) are not clearly connected to the Project Leaderboard feature described in the PR objectives. That comment was not marked as addressed.
Per the previous review: "Consider moving unrelated changes to a separate PR for better traceability, or document why these admin changes are necessary for the leaderboard feature."
Please clarify whether these admin changes:
- Are a necessary prerequisite for the leaderboard feature (if so, document the dependency), or
- Should be split into a separate PR for better change tracking
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/admin.py(3 hunks)
⏰ 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/admin.py (4)
1-9: LGTM!The import additions are appropriate and correctly used throughout the file:
ClassVarfor type hints in the CustomUserAdminNotRegisteredfor safe unregistrationget_user_model()for dynamic User model resolutionDjangoUserAdminaliased to avoid conflicts with the existingUserAdminimport
106-107: LGTM!Using
get_user_model()is the correct Django pattern for retrieving the User model dynamically, ensuring compatibility with custom user models.
1281-1295: LGTM!The
deactivate_usersadmin action is well-implemented with proper superuser permission checks and efficient bulk updates. The error handling and user feedback are appropriate.
1298-1313: LGTM!The
ActivityStatusFiltercorrectly filters users based on login activity using thelast_loginfield. The implementation follows Django's SimpleListFilter pattern properly.
Comment on lines
+1316
to
+1365
| try: | ||
| admin.site.unregister(User) | ||
| except NotRegistered: | ||
| pass | ||
|
|
||
|
|
||
| class CustomUserAdmin(DjangoUserAdmin): | ||
| actions: ClassVar[list] = [deactivate_users] | ||
|
|
||
| list_display = ( | ||
| "username", | ||
| "email", | ||
| "is_active", | ||
| "activity_status", | ||
| "last_login", | ||
| "date_joined", | ||
| ) | ||
|
|
||
| list_filter = ( | ||
| "is_active", | ||
| ActivityStatusFilter, | ||
| ) | ||
|
|
||
| search_fields = ("username", "email") | ||
| ordering = ("-last_login",) | ||
|
|
||
| def has_module_permission(self, request): | ||
| return request.user.is_superuser | ||
|
|
||
| def has_view_permission(self, request, obj=None): | ||
| return request.user.is_superuser | ||
|
|
||
| def has_change_permission(self, request, obj=None): | ||
| return request.user.is_superuser | ||
|
|
||
| def get_actions(self, request): | ||
| actions = super().get_actions(request) | ||
| if not request.user.is_superuser: | ||
| actions.pop("deactivate_users", None) | ||
| return actions | ||
|
|
||
| def activity_status(self, obj): | ||
| if obj.last_login: | ||
| return format_html('<span style="color: green; font-weight: 600;">Active</span>') | ||
| return format_html('<span style="color: red; font-weight: 600;">Inactive</span>') | ||
|
|
||
| activity_status.short_description = "Activity" | ||
|
|
||
|
|
||
| admin.site.register(User, CustomUserAdmin) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant User registration at line 955.
The CustomUserAdmin implementation is well-designed with proper superuser-only access controls and the safe unregister pattern. However, line 955 registers User with the basic UserAdmin, and then line 1365 registers it again with CustomUserAdmin, resulting in the second registration overwriting the first.
To clean up this redundancy, remove line 955:
-admin.site.register(User, UserAdmin)The proper registration with CustomUserAdmin at line 1365 will then be the only registration, making the code clearer and avoiding the redundant intermediate registration.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In website/admin.py around lines 1316 to 1365, remove the earlier redundant
registration of User at line 955 so the module only registers User once with
CustomUserAdmin at line 1365; locate and delete the standalone
admin.site.register(User) (or the block that registers User with the default
UserAdmin) at line 955, keep the safe unregister pattern and the CustomUserAdmin
class intact, and run tests / Django server to confirm no duplicate-registration
errors.
Contributor
Author
|
@DonnieBLT please review this pr by mistake pushed into wrong branch but fixed it now .. I have changed some testing file as coderabbit is suggesting and the CI failure was caused by nested transactions in ip_restrict middleware during django tests i made the middleware test-safe by using savepoints and skipping non-critical IP logging during tests. Production behavior remains unchanged. please let me know if any-things required to fix or removed |
Collaborator
|
this should show some data - @nitinawari was working on this and had a good looking dashboard - please collaborate to work together or pickup |
DonnieBLT
requested changes
Dec 14, 2025
Collaborator
DonnieBLT
left a comment
DonnieBLT
left a comment
There was a problem hiding this comment.
limit changes only to the project leaderboard
| blocked_ip_network.append(network) | ||
| except ValueError as e: | ||
| logger.error(f"Invalid IP network {range_str}: {str(e)}") | ||
| continue |
Collaborator
There was a problem hiding this comment.
why is this PR modifying anything in this file?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #3314
Summary
Adds an initial Project Leaderboard page with tabbed views (Dashboard, Projects, Activity).
What’s included
/project_leaderboard/pageScope
This PR focuses on the initial backend + template wiring only.
Advanced features (filters, charts, caching) will be added in follow-up PRs.
Issue
Related to #3314
Summary by CodeRabbit
New Features
UI
Admin Enhancements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.