Adds automatic user quota management for problem submissions#186
Adds automatic user quota management for problem submissions#186happylittle7 wants to merge 2 commits intodevfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements an automatic quota management system for problem submissions, introducing signal-based quota updates and refactored submission validation logic. The changes aim to ensure user submission limits are consistently managed and automatically updated when problem quota settings change.
Key changes include:
- Signal-based automatic quota reset when a problem's total_quota is modified
- Refactored quota validation in submission logic to support both global and per-user quotas
- Improved admin interface display for quota usage with percentage formatting
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| submissions/views.py | Refactors quota checking logic to occur after problem permission validation, adds support for automatic quota creation based on problem settings, and implements handling for both global and per-user quota overrides |
| submissions/admin.py | Updates quota_usage display method to use integer percentage formatting instead of floating-point format |
| problems/signals.py | Introduces new signal handlers to track total_quota changes and automatically reset UserProblemQuota records when problem quotas are modified |
| problems/apps.py | Registers signal handlers by importing problems.signals in the ready() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| updated_count = UserProblemQuota.objects.filter( | ||
| problem_id=instance.id, | ||
| assignment_id__isnull=True # 只重置全域配額，不影響作業配額 | ||
| ).update( | ||
| total_quota=new_quota, | ||
| remaining_attempts=new_quota | ||
| ) |
There was a problem hiding this comment.
The signal handler resets user quotas without wrapping the update in a transaction. If the update fails partway through (e.g., database connection issues), some users might have their quotas reset while others don't, leading to inconsistent state. The update operation should be wrapped in a transaction.atomic() block to ensure atomicity.
| if quota.remaining_attempts > 0: | ||
| quota.remaining_attempts -= 1 | ||
| quota.save() |
There was a problem hiding this comment.
Similar to the issue in the first quota check block, this redundant condition should be removed. If remaining_attempts is 0, the error is already returned at line 922-927. The check at line 928 before decrementing is unnecessary because we know at that point remaining_attempts must be greater than 0. This check should be removed to simplify the logic.
| if quota.remaining_attempts > 0: | |
| quota.remaining_attempts -= 1 | |
| quota.save() | |
| quota.remaining_attempts -= 1 | |
| quota.save() |
| # 重置所有該題目的配額記錄 | ||
| updated_count = UserProblemQuota.objects.filter( | ||
| problem_id=instance.id, | ||
| assignment_id__isnull=True # 只重置全域配額，不影響作業配額 | ||
| ).update( | ||
| total_quota=new_quota, | ||
| remaining_attempts=new_quota | ||
| ) |
There was a problem hiding this comment.
The signal handler updates all UserProblemQuota records when the problem's total_quota changes, but it doesn't consider users who may have already used some of their quota. When the quota is reset, users who already consumed attempts get a fresh quota equal to the new total, potentially allowing them more submissions than intended. Consider whether remaining_attempts should be calculated based on the usage (e.g., remaining_attempts = new_quota - (old_quota - old_remaining_attempts)) rather than simply resetting to new_quota.
| @receiver(pre_save, sender='problems.Problems') | ||
| def track_quota_change(sender, instance, **kwargs): | ||
| """ | ||
| 在保存前追蹤 total_quota 是否變更 | ||
| 將舊值存入 instance._old_total_quota | ||
| """ | ||
| if instance.pk: | ||
| try: | ||
| old_instance = sender.objects.get(pk=instance.pk) | ||
| instance._old_total_quota = old_instance.total_quota | ||
| except sender.DoesNotExist: | ||
| instance._old_total_quota = None | ||
| else: | ||
| instance._old_total_quota = None |
There was a problem hiding this comment.
The signal is triggered on every save, even when called from update_fields that doesn't include total_quota. Consider checking if 'total_quota' is in update_fields (if available) to avoid unnecessary processing. You can access this via kwargs.get('update_fields') in the pre_save signal.
| quota = UserProblemQuota.objects.select_for_update().get( | ||
| user=user, | ||
| problem_id=problem_id, | ||
| assignment_id__isnull=True |
There was a problem hiding this comment.
Using assignment_id=None versus assignment_id__isnull=True inconsistently could lead to issues. At line 892, the code uses assignment_id=None as a parameter for get_or_create, but at line 918 it uses assignment_id__isnull=True for the query. While both should work for NULL values, for consistency and clarity, use the same pattern throughout the quota checking logic.
| assignment_id__isnull=True | |
| assignment_id=None |
| # 4. Quota 檢查 - 基於題目的 total_quota 設定 | ||
| problem_quota = problem.total_quota # -1 = 無限制, >= 0 = 有限制 | ||
|
|
||
| if problem_quota >= 0: | ||
| # 題目有配額限制，需要檢查/建立該用戶的 UserProblemQuota 記錄 | ||
| with transaction.atomic(): | ||
| quota, created = UserProblemQuota.objects.select_for_update().get_or_create( | ||
| user=user, | ||
| problem_id=problem_id, | ||
| assignment_id=None, # 全域配額（非作業） | ||
| defaults={ | ||
| 'total_quota': problem_quota, | ||
| 'remaining_attempts': problem_quota | ||
| } | ||
| ) | ||
|
|
||
| if quota.remaining_attempts == 0: | ||
| return api_response( | ||
| data=None, | ||
| message="you have used all your quotas", | ||
| status_code=status.HTTP_403_FORBIDDEN | ||
| ) | ||
|
|
||
| # 減少配額 | ||
| if quota.remaining_attempts > 0: | ||
| quota.remaining_attempts -= 1 | ||
| quota.save() | ||
| else: | ||
| # 題目無配額限制 (total_quota == -1) | ||
| # 但仍檢查是否有管理員手動設定的 UserProblemQuota（針對特定用戶的限制） | ||
| try: | ||
| with transaction.atomic(): | ||
| quota = UserProblemQuota.objects.select_for_update().get( | ||
| user=user, | ||
| problem_id=problem_id, | ||
| assignment_id__isnull=True | ||
| ) | ||
| # 只有當 total_quota >= 0 時才檢查（-1 表示此用戶也無限制） | ||
| if quota.total_quota >= 0: | ||
| if quota.remaining_attempts == 0: | ||
| return api_response( | ||
| data=None, | ||
| message="you have used all your quotas", | ||
| status_code=status.HTTP_403_FORBIDDEN | ||
| ) | ||
| if quota.remaining_attempts > 0: | ||
| quota.remaining_attempts -= 1 | ||
| quota.save() | ||
| except UserProblemQuota.DoesNotExist: | ||
| # 沒有任何配額限制，允許提交 | ||
| pass |
There was a problem hiding this comment.
The new quota management logic in the submission view and the signal-based quota reset functionality lack test coverage. Given that other submission functionality in this codebase has comprehensive test coverage (as seen in test_submission_views_api.py and other test files), tests should be added to verify: 1) quota checking and decrementing behavior for problems with limited quotas, 2) handling of unlimited quotas (-1), 3) per-user quota overrides, 4) the signal handler's quota reset behavior when total_quota changes, and 5) edge cases like concurrent submissions and quota exhaustion.
| @receiver(pre_save, sender='problems.Problems') | ||
| def track_quota_change(sender, instance, **kwargs): | ||
| """ | ||
| 在保存前追蹤 total_quota 是否變更 | ||
| 將舊值存入 instance._old_total_quota | ||
| """ | ||
| if instance.pk: | ||
| try: | ||
| old_instance = sender.objects.get(pk=instance.pk) | ||
| instance._old_total_quota = old_instance.total_quota | ||
| except sender.DoesNotExist: | ||
| instance._old_total_quota = None | ||
| else: | ||
| instance._old_total_quota = None | ||
|
|
||
|
|
||
| @receiver(post_save, sender='problems.Problems') | ||
| def reset_user_quotas_on_change(sender, instance, created, **kwargs): | ||
| """ | ||
| 當題目的 total_quota 被修改時，重置所有該題目的 UserProblemQuota 記錄 | ||
|
|
||
| 邏輯： | ||
| - 如果是新建題目，不需要處理 | ||
| - 如果 total_quota 沒有變更，不需要處理 | ||
| - 如果 total_quota 變更了，重置所有相關的 UserProblemQuota | ||
| """ | ||
| if created: | ||
| return | ||
|
|
||
| old_quota = getattr(instance, '_old_total_quota', None) | ||
| new_quota = instance.total_quota | ||
|
|
||
| # 如果 quota 沒有變更，不需要處理 | ||
| if old_quota == new_quota: | ||
| return | ||
|
|
||
| # 延遲導入避免循環依賴 | ||
| from submissions.models import UserProblemQuota | ||
|
|
||
| # 重置所有該題目的配額記錄 | ||
| updated_count = UserProblemQuota.objects.filter( | ||
| problem_id=instance.id, | ||
| assignment_id__isnull=True # 只重置全域配額，不影響作業配額 | ||
| ).update( | ||
| total_quota=new_quota, | ||
| remaining_attempts=new_quota | ||
| ) | ||
|
|
||
| logger.info( | ||
| f"Problem {instance.id} total_quota changed from {old_quota} to {new_quota}. " | ||
| f"Reset {updated_count} user quota records." | ||
| ) |
There was a problem hiding this comment.
The signal handlers in this new file lack test coverage. Given that the problems app has existing test infrastructure (test_api.py), tests should be added to verify: 1) that total_quota changes trigger the signal correctly, 2) that UserProblemQuota records are updated as expected, 3) that only global quotas (assignment_id=None) are affected, 4) edge cases like changing from unlimited to limited quotas and vice versa, and 5) behavior when no UserProblemQuota records exist yet.
| # 減少配額 | ||
| if quota.remaining_attempts > 0: | ||
| quota.remaining_attempts -= 1 | ||
| quota.save() |
There was a problem hiding this comment.
The quota checking and decrement logic has a potential race condition. After checking that remaining_attempts is 0 at line 899, but before the check at line 907, the value could theoretically have changed. Although select_for_update() is used, the logic structure allows remaining_attempts to be decremented even when it's 0 (if it becomes non-zero between checks). The condition at line 907 should be removed since the check at line 899 already handles the case when remaining_attempts is 0, and if we reach line 907, we know remaining_attempts must be positive.
| if instance.pk: | ||
| try: | ||
| old_instance = sender.objects.get(pk=instance.pk) | ||
| instance._old_total_quota = old_instance.total_quota | ||
| except sender.DoesNotExist: | ||
| instance._old_total_quota = None |
There was a problem hiding this comment.
The signal handler performs a database query within the pre_save signal, which can cause performance issues. This query executes for every save operation, even when the total_quota hasn't changed. Consider checking if total_quota is in the instance's changed fields using Django's update_fields or checking the instance's state to avoid unnecessary database hits.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| user=user, | ||
| problem_id=problem_id, | ||
| assignment_id=None # Global quota (no assignment) | ||
| assignment_id__isnull=True |
There was a problem hiding this comment.
There's an inconsistency in how assignment_id filtering is done: line 892 uses 'assignment_id=None' while line 918 uses 'assignment_id__isnull=True'. These have different behaviors in Django - the first filters for NULL explicitly, while the second includes both NULL and potentially other null-like values. For consistency and correctness with the get_or_create logic above, line 918 should use 'assignment_id=None'.
| assignment_id__isnull=True | |
| assignment_id=None # 全域配額（非作業），與上方 get_or_create 邏輯一致 |
| updated_count = UserProblemQuota.objects.filter( | ||
| problem_id=instance.id, | ||
| assignment_id__isnull=True # 只重置全域配額，不影響作業配額 | ||
| ).update( | ||
| total_quota=new_quota, | ||
| remaining_attempts=new_quota | ||
| ) |
There was a problem hiding this comment.
The signal update doesn't wrap the quota reset in a transaction, which could lead to partial updates if the operation fails midway. Wrap the UserProblemQuota.objects.filter().update() operation in a transaction.atomic() block to ensure all-or-nothing updates.
| # 沒有任何配額限制，允許提交 | ||
| pass | ||
|
|
||
| submission = serializer.save() |
There was a problem hiding this comment.
The submission save is outside the transaction block that handles quota decrement, breaking atomicity. If the submission save fails after the quota has been decremented, the user will lose a quota attempt without creating a submission. Move this line inside the transaction block to ensure that both quota updates and submission creation succeed or fail together.
| quota, created = UserProblemQuota.objects.select_for_update().get_or_create( | ||
| user=user, | ||
| problem_id=problem_id, | ||
| assignment_id=None, # 全域配額（非作業） | ||
| defaults={ | ||
| 'total_quota': problem_quota, | ||
| 'remaining_attempts': problem_quota | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Using select_for_update with get_or_create can lead to unexpected behavior. The select_for_update lock is only applied to the SELECT query, but if the record doesn't exist, the subsequent INSERT isn't protected. This means the lock doesn't prevent race conditions during creation. Consider using a try-except pattern with get and select_for_update, followed by create with IntegrityError handling (similar to the original implementation that was removed).
| quota, created = UserProblemQuota.objects.select_for_update().get_or_create( | |
| user=user, | |
| problem_id=problem_id, | |
| assignment_id=None, # 全域配額（非作業） | |
| defaults={ | |
| 'total_quota': problem_quota, | |
| 'remaining_attempts': problem_quota | |
| } | |
| ) | |
| try: | |
| # 先嘗試在行鎖下取得現有配額紀錄 | |
| quota = UserProblemQuota.objects.select_for_update().get( | |
| user=user, | |
| problem_id=problem_id, | |
| assignment_id=None, # 全域配額（非作業） | |
| ) | |
| created = False | |
| except UserProblemQuota.DoesNotExist: | |
| # 如不存在則嘗試建立，若發生競態導致 IntegrityError，再回頭取得 | |
| try: | |
| quota = UserProblemQuota.objects.create( | |
| user=user, | |
| problem_id=problem_id, | |
| assignment_id=None, # 全域配額（非作業） | |
| total_quota=problem_quota, | |
| remaining_attempts=problem_quota, | |
| ) | |
| created = True | |
| except IntegrityError: | |
| quota = UserProblemQuota.objects.select_for_update().get( | |
| user=user, | |
| problem_id=problem_id, | |
| assignment_id=None, # 全域配額（非作業） | |
| ) | |
| created = False |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@happylittle7 I've opened a new pull request, #187, to work on those changes. Once the pull request is ready, I'll request review from you. |
Overview
Implements a robust quota tracking and enforcement mechanism for problem submissions, ensuring user submission limits are consistently managed and automatically updated in response to changes in problem settings.
Key Improvements
Motivation
Addresses the need for accurate and automated quota management, reducing manual intervention and potential errors when updating problem limits. Improves user experience by providing clear feedback on quota status during submissions.