Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements IP whitelist filtering functionality for submission queries and adds automatic statistics tracking for user problem submissions. The changes include support for both simple IP prefix matching and CIDR notation, along with a new function to update global user problem statistics when submissions are judged.
Key changes:
- Added IP prefix filtering with support for simple prefix (e.g.,
192.168.) and CIDR notation (e.g.,192.168.1.0/24) - Implemented
update_user_problem_statsfunction to track submission statistics, solve status, and performance metrics - Updated callback endpoint to automatically update user statistics after judging
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| submissions/views.py | Added IP prefix filtering logic for submission queries, implemented statistics tracking function, integrated stats updates into callback flow |
| submissions/test_file/get_test_token.py | Enhanced test user creation with email verification setup and improved output formatting |
| docs/submissions.MD | Documented new IP prefix filtering parameter with examples, updated version history to v1.1, minor formatting corrections |
Comments suppressed due to low confidence (10)
submissions/views.py:616
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:683
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:763
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:1291
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:1295
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:1487
- This import of module logging is redundant, as it was previously imported on line 15.
import logging
submissions/views.py:1670
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:1806
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:1855
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
submissions/views.py:1991
- This import of module logging is redundant, as it was previously imported on line 15.
This import of module logging is redundant, as it was previously imported on line 1487.
import logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # IP 網段前綴篩選 | ||
| if ip_prefix: | ||
| try: | ||
| # 支援 CIDR 格式 (例如 192.168.1.0/24) 或簡單前綴 (例如 192.168.) | ||
| if '/' in ip_prefix: | ||
| # CIDR 格式：使用 ipaddress 模組 | ||
| import ipaddress | ||
| network = ipaddress.ip_network(ip_prefix, strict=False) | ||
| # 獲取網段範圍 | ||
| start_ip = str(network.network_address) | ||
| end_ip = str(network.broadcast_address) | ||
| # 篩選 IP 在範圍內的提交 | ||
| queryset = queryset.filter( | ||
| ip_address__gte=start_ip, | ||
| ip_address__lte=end_ip | ||
| ) | ||
| else: | ||
| # 簡單前綴匹配：例如 "192.168." 會匹配所有 192.168.x.x | ||
| queryset = queryset.filter(ip_address__startswith=ip_prefix) | ||
| except (ValueError, TypeError) as e: | ||
| import logging | ||
| logger = logging.getLogger(__name__) | ||
| logger.warning(f'Invalid ip_prefix parameter: {ip_prefix}, error: {e}') | ||
| pass # 忽略無效的 IP 前綴 |
There was a problem hiding this comment.
The IP address range filtering using string comparison (gte and lte) is incorrect for IP addresses. String comparison doesn't work correctly for IP addresses because they are not lexicographically ordered (e.g., "192.168.1.10" > "192.168.1.9" in string comparison is false). This will produce incorrect results when filtering by CIDR ranges. Consider using a proper IP address field type or converting IPs to integers for comparison.
submissions/views.py
Outdated
| elif stats.total_submissions > 0: | ||
| stats.solve_status = 'attempted' | ||
| else: | ||
| stats.solve_status = 'never_tried' |
There was a problem hiding this comment.
The condition at line 113 (total_submissions > 0) will always be true at this point in the code since total_submissions is incremented at line 76 and saved at line 90. This means the 'never_tried' status can never be set in this function, making the else branch unreachable code.
| '''這邊也不需要了，算成績的部分交給前端處理 | ||
|
|
||
| # 6. 更新 UserProblemStats（作業層級） | ||
| # 嘗試從 submission 找到對應的 assignment_id | ||
| # submission model 需要加上 assignment_id 欄位，或者從 context 傳入 | ||
| # 目前先跳過，等 submission model 更新後再啟用 | ||
| # if hasattr(submission, 'assignment_id') and submission.assignment_id: | ||
| # update_user_assignment_stats(submission, submission.assignment_id) | ||
|
|
||
| ''' |
There was a problem hiding this comment.
Large blocks of commented-out code should be removed rather than kept in the codebase. If this code may be needed later, it can be retrieved from version control history. Keeping commented code reduces readability and maintainability.
| '''這邊也不需要了，算成績的部分交給前端處理 | |
| # 6. 更新 UserProblemStats（作業層級） | |
| # 嘗試從 submission 找到對應的 assignment_id | |
| # submission model 需要加上 assignment_id 欄位，或者從 context 傳入 | |
| # 目前先跳過，等 submission model 更新後再啟用 | |
| # if hasattr(submission, 'assignment_id') and submission.assignment_id: | |
| # update_user_assignment_stats(submission, submission.assignment_id) | |
| ''' |
|
|
||
| Sandbox 判題完成後會 POST 到這個 endpoint | ||
| URL: POST /api/submissions/callback/ | ||
| URL: POST /{callback_url}/submissions/callback/ |
There was a problem hiding this comment.
The URL path in the docstring is corrected but the placeholder format is inconsistent. The path shows "/{callback_url}/submissions/callback/" which suggests a variable path segment, but this doesn't match typical REST API documentation patterns. Consider using a clearer format like "/api/submissions/callback/" if that's the actual endpoint.
| URL: POST /{callback_url}/submissions/callback/ | |
| URL: POST /api/submissions/callback/ |
| # IP 網段前綴篩選 | ||
| if ip_prefix: | ||
| try: | ||
| # 支援 CIDR 格式 (例如 192.168.1.0/24) 或簡單前綴 (例如 192.168.) | ||
| if '/' in ip_prefix: | ||
| # CIDR 格式：使用 ipaddress 模組 | ||
| import ipaddress | ||
| network = ipaddress.ip_network(ip_prefix, strict=False) | ||
| # 獲取網段範圍 | ||
| start_ip = str(network.network_address) | ||
| end_ip = str(network.broadcast_address) | ||
| # 篩選 IP 在範圍內的提交 | ||
| queryset = queryset.filter( | ||
| ip_address__gte=start_ip, | ||
| ip_address__lte=end_ip | ||
| ) | ||
| else: | ||
| # 簡單前綴匹配：例如 "192.168." 會匹配所有 192.168.x.x | ||
| queryset = queryset.filter(ip_address__startswith=ip_prefix) | ||
| except (ValueError, TypeError) as e: | ||
| import logging | ||
| logger = logging.getLogger(__name__) | ||
| logger.warning(f'Invalid ip_prefix parameter: {ip_prefix}, error: {e}') | ||
| pass # 忽略無效的 IP 前綴 |
There was a problem hiding this comment.
The new IP prefix filtering functionality lacks test coverage. While this repository has comprehensive test coverage for other submission features (as evidenced by test files in the test_file directory), there are no tests for the IP filtering logic introduced in this PR. Consider adding tests to cover simple prefix matching, CIDR notation parsing, and edge cases like invalid IP formats.
submissions/views.py
Outdated
| import logging | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
The logging module is imported inside the exception handler, which is inefficient and inconsistent with the module-level import at line 15. Use the module-level logger instead of importing it locally.
| # 支援 CIDR 格式 (例如 192.168.1.0/24) 或簡單前綴 (例如 192.168.) | ||
| if '/' in ip_prefix: | ||
| # CIDR 格式：使用 ipaddress 模組 | ||
| import ipaddress |
There was a problem hiding this comment.
The ipaddress module is imported inside the try block, which is inefficient if this code path is frequently executed. Consider importing it at the module level alongside other imports at the top of the file.
| except (ValueError, TypeError) as e: | ||
| import logging | ||
| logger = logging.getLogger(__name__) | ||
| logger.warning(f'Invalid ip_prefix parameter: {ip_prefix}, error: {e}') | ||
| pass # 忽略無效的 IP 前綴 |
There was a problem hiding this comment.
The exception is caught and logged, but then execution continues with 'pass', which silently ignores invalid IP prefix parameters. This could lead to confusion where users expect filtering but get unfiltered results. Consider returning an error response to inform the user that the IP prefix parameter is invalid.
| ''' 這邊不再需要了，算成績的部分交給前端處理 | ||
| def update_user_assignment_stats(submission, assignment_id): | ||
| """ | ||
| 更新使用者作業題目統計（作業層級） | ||
|
|
||
| 根據提交結果更新 UserProblemStats，包括： | ||
| - 總提交數 | ||
| - 最佳分數 | ||
| - 首次 AC 時間 | ||
| - 最後提交時間 | ||
| - 解題狀態（unsolved/partial/solved） | ||
| - 最佳執行時間和記憶體使用 | ||
| - 遲交處理 | ||
| """ | ||
| from django.utils import timezone | ||
| from django.db.models import F | ||
| from .models import UserProblemStats | ||
| from assignments.models import Assignments, Assignment_problems | ||
|
|
||
| try: | ||
| # 檢查是否屬於作業 | ||
| if not assignment_id: | ||
| return | ||
|
|
||
| # 檢查 assignment 和 problem 的關聯 | ||
| try: | ||
| assignment = Assignments.objects.get(id=assignment_id) | ||
| assignment_problem = Assignment_problems.objects.get( | ||
| assignment=assignment, | ||
| problem_id=submission.problem_id | ||
| ) | ||
| except (Assignments.DoesNotExist, Assignment_problems.DoesNotExist): | ||
| # 如果作業或題目不存在，跳過 | ||
| return | ||
|
|
||
| # 取得或創建 UserProblemStats | ||
| stats, created = UserProblemStats.objects.get_or_create( | ||
| user=submission.user, | ||
| assignment_id=assignment_id, | ||
| problem_id=submission.problem_id, | ||
| defaults={ | ||
| 'total_submissions': 0, | ||
| 'best_score': 0, | ||
| 'max_possible_score': assignment_problem.weight * 100 if assignment_problem.weight else 100, | ||
| 'solve_status': 'unsolved', | ||
| } | ||
| ) | ||
|
|
||
| # 更新總提交數 | ||
| stats.total_submissions = F('total_submissions') + 1 | ||
|
|
||
| # 更新最後提交時間 | ||
| stats.last_submission_time = submission.created_at | ||
|
|
||
| # 檢查是否遲交 | ||
| if assignment.due_time and submission.created_at > assignment.due_time: | ||
| stats.is_late = True | ||
| # 計算遲交罰分 | ||
| if assignment.late_penalty > 0: | ||
| stats.penalty_score = assignment.late_penalty | ||
|
|
||
| # 先保存以計算 F() 表達式 | ||
| stats.save() | ||
| stats.refresh_from_db() | ||
|
|
||
| # 更新最佳分數（考慮遲交罰分） | ||
| final_score = submission.score | ||
| if stats.is_late and stats.penalty_score > 0: | ||
| final_score = int(submission.score * (1 - float(stats.penalty_score) / 100)) | ||
|
|
||
| if final_score > stats.best_score: | ||
| stats.best_score = final_score | ||
| stats.best_submission = submission | ||
|
|
||
| # 如果是 AC (status='0') 且還沒有 first_ac_time | ||
| if submission.status == '0' and not stats.first_ac_time: | ||
| stats.first_ac_time = submission.judged_at or timezone.now() | ||
|
|
||
| # 更新最佳執行時間 | ||
| if submission.execution_time > 0: | ||
| if stats.best_execution_time is None or submission.execution_time < stats.best_execution_time: | ||
| stats.best_execution_time = submission.execution_time | ||
|
|
||
| # 更新最佳記憶體使用 | ||
| if submission.memory_usage > 0: | ||
| if stats.best_memory_usage is None or submission.memory_usage < stats.best_memory_usage: | ||
| stats.best_memory_usage = submission.memory_usage | ||
|
|
||
| # 更新解題狀態 | ||
| if stats.best_score >= stats.max_possible_score: | ||
| stats.solve_status = 'solved' | ||
| elif stats.best_score > 0: | ||
| stats.solve_status = 'partial' | ||
| else: | ||
| stats.solve_status = 'unsolved' | ||
|
|
||
| stats.save() | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.info(f'Updated assignment stats for user {submission.user.id} ' | ||
| f'assignment {assignment_id} problem {submission.problem_id}: ' | ||
| f'status={stats.solve_status}, best_score={stats.best_score}, ' | ||
| f'is_late={stats.is_late}') | ||
|
|
||
| except Exception as e: | ||
| logger = logging.getLogger(__name__) | ||
| logger.error(f'Failed to update user assignment stats: {str(e)}', exc_info=True) | ||
| ''' |
There was a problem hiding this comment.
Large blocks of commented-out code (over 100 lines) should be removed rather than left in the codebase. If this code may be needed in the future, it should be preserved in version control history. Keeping extensive commented code reduces maintainability and creates confusion about what is actually active.
| def update_user_problem_stats(submission): | ||
| """ | ||
| 更新使用者題目解題統計（全域層級） | ||
|
|
||
| 根據提交結果更新 UserProblemSolveStatus，包括： | ||
| - 總提交數 | ||
| - AC 提交數 | ||
| - 最佳分數 | ||
| - 首次解題時間 | ||
| - 最後提交時間 | ||
| - 解題狀態（never_tried/attempted/partial_solved/fully_solved） | ||
| - 最佳執行時間和記憶體使用 | ||
| """ | ||
| from django.utils import timezone | ||
| from django.db.models import F | ||
|
|
||
| try: | ||
| # 取得或創建 UserProblemSolveStatus | ||
| stats, created = UserProblemSolveStatus.objects.get_or_create( | ||
| user=submission.user, | ||
| problem_id=submission.problem_id, | ||
| defaults={ | ||
| 'total_submissions': 0, | ||
| 'ac_submissions': 0, | ||
| 'best_score': 0, | ||
| 'solve_status': 'never_tried', | ||
| } | ||
| ) | ||
|
|
||
| # 更新總提交數 | ||
| stats.total_submissions = F('total_submissions') + 1 | ||
|
|
||
| # 更新最後提交時間 | ||
| stats.last_submission_time = submission.created_at | ||
|
|
||
| # 如果是 AC (status='0') | ||
| if submission.status == '0': | ||
| stats.ac_submissions = F('ac_submissions') + 1 | ||
|
|
||
| # 更新首次解題時間（如果是第一次 AC） | ||
| if not stats.first_solve_time: | ||
| stats.first_solve_time = submission.judged_at or timezone.now() | ||
|
|
||
| # 先保存以計算 F() 表達式 | ||
| stats.save() | ||
| stats.refresh_from_db() | ||
|
|
||
| # 更新最佳分數 | ||
| if submission.score > stats.best_score: | ||
| stats.best_score = submission.score | ||
|
|
||
| # 更新最佳執行時間（只在有效時更新） | ||
| if submission.execution_time > 0: | ||
| if stats.best_execution_time is None or submission.execution_time < stats.best_execution_time: | ||
| stats.best_execution_time = submission.execution_time | ||
| stats.total_execution_time += submission.execution_time | ||
|
|
||
| # 更新最佳記憶體使用（只在有效時更新） | ||
| if submission.memory_usage > 0: | ||
| if stats.best_memory_usage is None or submission.memory_usage < stats.best_memory_usage: | ||
| stats.best_memory_usage = submission.memory_usage | ||
|
|
||
| # 更新解題狀態 | ||
| if stats.best_score >= 100: | ||
| stats.solve_status = 'fully_solved' | ||
| elif stats.best_score > 0: | ||
| stats.solve_status = 'partial_solved' | ||
| elif stats.total_submissions > 0: | ||
| stats.solve_status = 'attempted' | ||
| else: | ||
| stats.solve_status = 'never_tried' | ||
|
|
||
| stats.save() | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.info(f'Updated solve status for user {submission.user.id} problem {submission.problem_id}: ' | ||
| f'status={stats.solve_status}, best_score={stats.best_score}, ' | ||
| f'total_submissions={stats.total_submissions}') | ||
|
|
||
| except Exception as e: | ||
| logger = logging.getLogger(__name__) | ||
| logger.error(f'Failed to update user problem solve status: {str(e)}', exc_info=True) | ||
|
|
There was a problem hiding this comment.
The new update_user_problem_stats function lacks test coverage. While this repository has comprehensive test coverage for other submission features, there are no tests verifying the statistics update logic introduced in this PR. Consider adding tests to cover score updates, solve status transitions, execution time tracking, and edge cases like duplicate submissions.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@happylittle7 I've opened a new pull request, #175, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ns/custom-test-callback/ to {settings.BACKEND_BASE_URL} and {settings.BACKEND_BASE_URL}/submissions/callback/ to {settings.BACKEND_BASE_URL}.
I add IP whitelist.