Skip to content

Add permission-based navbar items#2416

Open
mircearoata wants to merge 1 commit into
DMOJ:masterfrom
mircearoata:navbar-permissions
Open

Add permission-based navbar items#2416
mircearoata wants to merge 1 commit into
DMOJ:masterfrom
mircearoata:navbar-permissions

Conversation

@mircearoata
Copy link
Copy Markdown
Contributor

@mircearoata mircearoata commented Mar 31, 2025

image
image
image

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 31, 2025

Codecov Report

❌ Patch coverage is 65.21739% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.75%. Comparing base (fd7fb05) to head (1f72dc0).
⚠️ Report is 82 commits behind head on master.

Files with missing lines Patch % Lines
judge/template_context.py 0.00% 6 Missing ⚠️
judge/models/interface.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2416      +/-   ##
==========================================
+ Coverage   46.76%   47.75%   +0.99%     
==========================================
  Files         251      261      +10     
  Lines       13317    13763     +446     
==========================================
+ Hits         6228     6573     +345     
- Misses       7089     7190     +101     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@WallE256 WallE256 requested a review from Copilot March 31, 2025 23:29
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread judge/models/interface.py Outdated
Comment thread judge/template_context.py
return {
'nav_tab': FixedSimpleLazyObject(partial(__nav_tab, request.path)),
'nav_bar': NavigationBar.objects.all(),
'nav_bar': NavigationBar.for_user(request.user),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about nav_tab? This is going to break it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_user and 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_user take a queryset and the function would only apply the permission filter, but this would then result in NavigationBar.for_user(NavigationBar.objects.all()) and NavigationBar.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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we are already fetching the entire navbar, computing nav_tab on the db layer is unnecessary. You can probably just change nav_tab to be computed in Python based on NavigationBar.for_user(request.user).

Comment thread judge/models/interface.py Outdated
Comment thread judge/models/interface.py Outdated
Comment thread judge/models/interface.py Outdated
@mircearoata mircearoata force-pushed the navbar-permissions branch 2 times, most recently from 2fd4d48 to 814df3a Compare May 11, 2025 22:58
@mircearoata mircearoata force-pushed the navbar-permissions branch from 814df3a to 1f72dc0 Compare May 11, 2025 23:01
@mircearoata mircearoata requested a review from quantum5 May 11, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants