Add permission-based navbar items #2416
Conversation
mircearoata
commented
Mar 31, 2025
mircearoata
commented
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2416 +/- ##
==========================================
+ Coverage 46.76% 47.77% +1.00%
==========================================
Files 251 260 +9
Lines 13317 13741 +424
==========================================
+ Hits 6228 6565 +337
- Misses 7089 7176 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This pull request implements permission-based control for navbar items by filtering navigation bar entries based on the user's permissions.
- Updates the navbar context to filter items using a permission-based method.
- Adds a new "permission" field to the NavigationBar model along with an associated migration.
- Updates the admin interface to include the new permission field with an appropriate widget.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| judge/template_context.py | Filters navbar items based on user permissions using for_user method |
| judge/models/interface.py | Adds a permission field to the NavigationBar model and a static filtering method |
| judge/migrations/0150_navigationbar_permissions.py | Adds the new permission field to the NavigationBar model via migration |
| judge/admin/interface.py | Updates the admin form and field list to support the new permission field |
| return list(item for item in all_navbar_items if item.permission is None or user.has_perm(item.permission)) | ||
|
|
There was a problem hiding this comment.
Django's user.has_perm expects a string (in the format 'app_label.codename') rather than a Permission instance. Consider converting the Permission object to its appropriate permission name before passing it to has_perm.
| return list(item for item in all_navbar_items if item.permission is None or user.has_perm(item.permission)) | |
| return list(item for item in all_navbar_items if item.permission is None or user.has_perm(f"{item.permission.content_type.app_label}.{item.permission.codename}")) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Did you test this code? Is Copilot hallucinating?
There was a problem hiding this comment.
Copilot....might actually not be hallucinating....
There was a problem hiding this comment.
I think I'm the one hallucinating, because I was sure I tested passing the object to has_perm, but I must have been using the admin account, which was skipping the check.
Copilot's suggestion does indeed work, but another way to fix it would be doing the filtering in the query instead of in python, concatenating the permission app label and codename, and checking if this string is in user.get_all_permissions(). This would also result in fewer queries (with copilot's code, there would be 2 queries per has_perm run, one for each field)
mircearoata
force-pushed
the
navbar-permissions
branch
from
4607cd8 to
0724281
Compare
| @@ -59,7 +59,7 @@ def general_info(request): | |||
| path = request.get_full_path() | |||
| return { | |||
| 'nav_tab': FixedSimpleLazyObject(partial(__nav_tab, request.path)), | |||
| 'nav_bar': NavigationBar.objects.all(), | |||
| 'nav_bar': NavigationBar.for_user(request.user), | |||
There was a problem hiding this comment.
What about nav_tab? This is going to break it.
There was a problem hiding this comment.
Right, this would break if multiple navbar items match the current path, but the first result is the one that gets hidden, then nothing would be highlighted.
I can see a few ways to fix this:
- passing the path to
for_userand moving the regex query there, but if anything else would use the navbar in the future it would run into the same issue again - making
for_usertake a queryset and the function would only apply the permission filter, but this would then result inNavigationBar.for_user(NavigationBar.objects.all())andNavigationBar.for_user(NavigationBar.objects.extra(where=['%s REGEXP BINARY regex'], params=[path])) - adding a custom queryset class for NavigationBar that applies the filter in the query (the one from Add permission-based navbar items #2416 (comment))
| @staticmethod | ||
| def for_user(user): | ||
| all_navbar_items = NavigationBar.objects.all() | ||
| return list(item for item in all_navbar_items if item.permission is None or user.has_perm(item.permission)) |
There was a problem hiding this comment.
Why are you using list(...) to emulate list comprehensions when you can use the real thing?
| @@ -61,6 +64,11 @@ def pattern(self, cache={}): | |||
| pattern = cache[self.regex] = re.compile(self.regex, re.VERBOSE) | |||
| return pattern | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
Can we use classmethod? This is unpythonic, especially when you refer to the class in the body.
| return list(item for item in all_navbar_items if item.permission is None or user.has_perm(item.permission)) | ||
|
|
There was a problem hiding this comment.
Did you test this code? Is Copilot hallucinating?
