Feature PR: Thêm tính năng sắp xếp lại contest (có filter) cho phép admin reorder lại contest quan trọng#84
Conversation
…rder function when leaving
…rder function when leaving
Okay. This is the final project for the reorder feature and it's ready for production.
There was a problem hiding this comment.
Pull request overview
PR bổ sung trang admin để kéo-thả sắp xếp lại (reorder) các contest theo bộ lọc (năm tổ chức / tag), đồng thời đưa khái niệm sort_order vào model Contest để điều khiển thứ tự hiển thị.
Changes:
- Thêm trang
/contests/reorder/(UI kéo-thả + lưu thứ tự qua POST JSON) và tab “Reorder contests” cho staff có quyền. - Thêm trường
Contest.sort_ordervà thay đổi logic sắp xếp contest list để ưu tiênsort_order. - Bổ sung asset SortableJS, CSS cho form filter, và chuỗi dịch cho template reorder.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
judge/views/contests.py |
Thêm view ContestReorder; thay đổi thứ tự sắp xếp contest list/tag list để dùng sort_order. |
judge/models/contest.py |
Thêm sort_order và đặt default ordering theo sort_order. |
dmoj/urls.py |
Đăng ký route /contests/reorder/. |
templates/contest/reorder.html |
Template UI reorder (filter + table kéo-thả + lưu). |
templates/contest/contest-list-tabs.html |
Thêm tab “Reorder contests” (kèm query tag). |
resources/contest.scss |
CSS cho form filter. |
resources/Sortable.min.js |
Thêm SortableJS phục vụ kéo-thả. |
locale/vi/LC_MESSAGES/django.po |
Thêm translation cho reorder template (và một số entry khác). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_queryset(self): | ||
| self.search_query = None | ||
| query_set = self._get_queryset().order_by(self.order, 'key').filter(end_time__lt=self._now) | ||
| query_set = self._get_queryset().order_by('sort_order', self.order, 'key').filter(end_time__lt=self._now) |
There was a problem hiding this comment.
order_by('sort_order', self.order, 'key') makes sort_order the primary sort key, so user-selected sorting (order query param: name/user_count/start_time) will not behave as expected (it becomes only a tie-breaker). If the intent is to preserve existing sortable columns, keep self.order first and use sort_order only as a secondary key (or only apply sort_order when using the default ordering).
| query_set = self._get_queryset().order_by('sort_order', self.order, 'key').filter(end_time__lt=self._now) | |
| query_set = self._get_queryset().order_by(self.order, 'sort_order', 'key').filter(end_time__lt=self._now) |
| # active.sort(key=attrgetter('end_time', 'key')) | ||
| # present.sort(key=attrgetter('end_time', 'key')) | ||
| # future.sort(key=attrgetter('start_time')) |
There was a problem hiding this comment.
The old sorting logic is left commented out. Since git history already preserves previous implementations, keeping these commented lines adds noise and makes future maintenance harder; please remove the commented-out sort calls.
| # active.sort(key=attrgetter('end_time', 'key')) | |
| # present.sort(key=attrgetter('end_time', 'key')) | |
| # future.sort(key=attrgetter('start_time')) |
| # active.sort(key=attrgetter('end_time', 'key')) | ||
| # present.sort(key=attrgetter('end_time', 'key')) | ||
| # future.sort(key=attrgetter('start_time')) |
There was a problem hiding this comment.
The old sorting logic is left commented out here as well. Please remove these commented-out lines to avoid accumulating dead code in the view.
| # active.sort(key=attrgetter('end_time', 'key')) | |
| # present.sort(key=attrgetter('end_time', 'key')) | |
| # future.sort(key=attrgetter('start_time')) |
| tag_key = request.GET.get('tag') | ||
|
|
||
| if year: |
There was a problem hiding this comment.
year is taken directly from the query string and passed to start_time__year=year. If a non-numeric value is provided (e.g. year=abc), some DB backends can error, causing a 500. Please validate/coerce year to an int (and ignore/400 on failure) before applying the filter.
| tag_key = request.GET.get('tag') | |
| if year: | |
| if year: | |
| try: | |
| year = int(year) | |
| except (TypeError, ValueError): | |
| year = None | |
| tag_key = request.GET.get('tag') | |
| if year is not None: |
| }) | ||
|
|
||
| def post(self, request): | ||
| data = json.loads(request.body) |
There was a problem hiding this comment.
json.loads(request.body) can raise JSONDecodeError (or UnicodeDecodeError) on malformed requests, returning a 500. Please handle invalid JSON and return a 400 with a small error payload.
| data = json.loads(request.body) | |
| try: | |
| data = json.loads(request.body) | |
| except (json.JSONDecodeError, UnicodeDecodeError): | |
| return JsonResponse({'error': 'invalid_json'}, status=400) |
| } | ||
|
|
||
| @media (max-width: 640px) { | ||
| form.contest-filter-row { |
There was a problem hiding this comment.
The responsive selector in the media query appears wrong: form.contest-filter-row targets a <form> with class contest-filter-row, but the markup uses a <div class="contest-filter-row"> inside <form class="contest-filter-form">. As written, the mobile grid override likely never applies; adjust the selector to match the actual .contest-filter-row element.
| form.contest-filter-row { | |
| form.contest-filter-form .contest-filter-row { |
|
|
||
|
|
||
| @method_decorator(staff_member_required, name='dispatch') | ||
| class ContestReorder(View): |
There was a problem hiding this comment.
staff_member_required allows any is_staff user to access this endpoint, but the UI tab is shown only when the user has judge.edit_all_contest or judge.edit_own_contest. This creates a permission bypass where staff without contest edit permissions can still reorder contests (and see potentially private contests). Consider enforcing the same permissions in dispatch (e.g., PermissionRequiredMixin or explicit has_perm checks) instead of (or in addition to) staff_member_required.
| class ContestReorder(View): | |
| class ContestReorder(PermissionRequiredMixin, View): | |
| permission_required = ('judge.edit_all_contest', 'judge.edit_own_contest') | |
| raise_exception = True | |
| def has_permission(self): | |
| return ( | |
| self.request.user.has_perm('judge.edit_all_contest') or | |
| self.request.user.has_perm('judge.edit_own_contest') | |
| ) |
| (SCOREBOARD_AFTER_CONTEST, _('Hidden for duration of contest')), | ||
| (SCOREBOARD_AFTER_PARTICIPATION, _('Hidden for duration of participation')), | ||
| ) | ||
| sort_order = models.IntegerField(verbose_name=_('sort order'), default=0, db_index=True) |
There was a problem hiding this comment.
With sort_order defaulting to 0, all existing contests will initially share the same value. That makes the new ordering unstable (falls back to key order) and can make the reorder operation ineffective unless sort_order is backfilled/uniqued. Please add a backfill step (migration/data script/management command) to initialize sort_order for existing rows to preserve the current contest ordering semantics.
| #: judge/models/profile.py:170 | ||
| msgid "unpublic logo" | ||
| msgstr "Không công khai logo" | ||
|
|
||
| #: judge/models/profile.py:171 | ||
| msgid "If enabled, only selected users or users within the organization selected below will be able to use the logo. Leave the fields below blank so no one can see this logo" | ||
| msgstr "Nếu bật, chỉ những người dùng được chọn hoặc người dùng trong tổ chức được chọn bên dưới mới có thể sử dụng logo. Để trống những trường bên dưới để ẩn đi logo này" | ||
|
|
||
| #: judge/models/profile.py:175 | ||
| msgid "users are permitted" | ||
| msgstr "Người dùng được phép" | ||
|
|
||
| #: judge/models/profile.py:176 | ||
| msgid "If private, only these users may choose the logo" | ||
| msgstr "Nếu cho private, chỉ những người dùng trên mới được chọn logo" |
There was a problem hiding this comment.
This diff adds several new Vietnamese translations (e.g. "unpublic logo", "users are permitted", "You are not allowed to use this logo") that don't appear to exist anywhere in the current codebase (no references found). This looks unrelated to the contest reorder feature and may be accidental translation churn; please remove these unrelated entries or include the corresponding code changes that introduce the msgids.
| path('contests.ics', contests.ContestICal.as_view(), name='contest_ical'), | ||
| path('contests/<int:year>/<int:month>/', contests.ContestCalendar.as_view(), name='contest_calendar'), | ||
| path('contests/new', contests.CreateContest.as_view(), name='contest_new'), | ||
| path('contests/reorder/', contests.ContestReorder.as_view(), name='contest_reorder'), |
There was a problem hiding this comment.
PR description mentions accessing the feature via /contests/reorder (no trailing slash), but the new route is registered as contests/reorder/. If APPEND_SLASH is disabled in some environments, the documented URL will 404; consider aligning the path/docs (or adding a redirect/alternative path).
Mô tả
Tệp đã thay đổi
Updated
Added
Lưu ý
PR này bao gồm những contributor là ThisIsNotNam từ ThisIsNotNam/oj và hoangthaiminh từ hoangthaiminh/chtOJ